Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Settings buttons' background not themeable. #1806

Closed
Spekular opened this issue Mar 2, 2015 · 26 comments
Closed

Settings buttons' background not themeable. #1806

Spekular opened this issue Mar 2, 2015 · 26 comments

Comments

@Spekular
Copy link
Member

Spekular commented Mar 2, 2015

Tested in master and 1.1, I don't see anywhere in style.css to change the background color of the buttons on the left of the settings dialogue.

@tresf
Copy link
Member

tresf commented Mar 2, 2015

These are of type TabButton (extends QPushButton) contained inside a QWidget known as a TabBar.

For starters you could try styling QPushButton generically in the style.css and see if that does it?

Not sure if that helps.

This is where I found the code and traversed through the classes from there...
https://github.com/LMMS/lmms/blob/master/src/gui/SetupDialog.cpp#L146

@Spekular
Copy link
Member Author

Spekular commented Mar 2, 2015 via email

@tresf
Copy link
Member

tresf commented Mar 2, 2015

@tresf neither QPushButton nor TabButton are present in styles.css. I
assume this means they are not styleable at the moment?

Have you tried adding them? :)

QPushButton {
    background-color: rgb(255, 0, 0);
}

I'm discovering various issues with styling lmms/themes as I try to make a theme

Yes, we are well aware. Keep up the good work. 👍

I'm not sure creating a new issue for each one is correct. Should I instead use
one master issue? I could edit this one if/when I find more perhaps.

Whatever works. The more we have these conversations, the less you should need help locating the place to change this stuff... :)

image

@badosu
Copy link
Contributor

badosu commented Mar 2, 2015

@Spekular If you find a case where a widget style is too generic for your purposes, let us know that we can take a look and change the code to use specific classes.

@tresf
Copy link
Member

tresf commented Mar 2, 2015

@Spekular If you find a case where a widget style is too generic for your purposes, let us know that we can take a look and change the code to use specific classes.

👍 This is probably a good candidate, but I'll let @Spekular chime in with that requirement. 👍

@Spekular
Copy link
Member Author

Spekular commented Mar 2, 2015

@tresf then I shall! These buttons could definitely use separate colors
imo.
Also, whats up with them not being in style if they can be themed? I'm
guessing there's a hardcoded default but wouldn't it be better to always
pull from the .css file? Theming is technically (as opposed to creatively)
easy because you don't need to worry about code, which lowers the barrier
of entry and lets more people contribute. If themers have to search through
code to find out what element to theme the barrier to entry is raised
again. Of course I should really be better at this (sorry about that :/),
but we shouldn't expect artists trying to make themes to do so. (Once again
though maybe I should stop blabbing and fix it myself. Been a bit busy
lately though.)
Finally, the track label background only covers the part with the name, the
rest is either themeable if you add it yourself, or not themeable. It seems
to use the default background color (which the fx mixer also uses), unless
it's just set to transparent.
On Mar 2, 2015 7:35 PM, "Tres Finocchiaro" notifications@github.com
wrote:

@Spekular https://github.com/Spekular If you find a case where a widget
style is too generic for your purposes, let us know that we can take a look
and change the code to use specific classes.

[image: 👍] This is probably a good candidate, but I'll let @Spekular
https://github.com/Spekular chime in with that requirement. [image:
👍]


Reply to this email directly or view it on GitHub
#1806 (comment).

@tresf
Copy link
Member

tresf commented Mar 2, 2015

Also, whats up with them not being in style if they can be themed?

It's just classic style sheeting. You can style the parent, <body>, you can style the specific element <... id="specific_element"> or you can style the <p> type of element.

LMMS is very similar. The styles cascade downward so they inherit their parent style unless we override them. If they aren't specified, they take defaults.

Styling is as granular as you'd like it and it is a coordinated effort between stylers and coders that makes a good theme possible. 👍

@tresf
Copy link
Member

tresf commented Mar 2, 2015

Theming is technically (as opposed to creatively) easy because you don't need to worry about code, which lowers the barrier of entry and lets more people contribute. If themers have to search through code to find out what element to theme the barrier to entry is raised again.

Until we have more people trying to theme our software, we don't know how granular people want to get. IIRC, QButton can style all of that stuff, so that is likely the point you are missing -- we may already have that part themed, but at a global level.

the track label background only covers the part with the name, the rest is either themeable if you add it yourself, or not themeable.

Ok... I'm not sure if that is a question that needs helps or not... 😕

Of course I should really be better at this (sorry about that :/), but we shouldn't expect artists trying to make themes to do so.

We don't.. that's why we're here to answer your questions!

@Spekular
Copy link
Member Author

Spekular commented Mar 2, 2015 via email

@Spekular
Copy link
Member Author

Spekular commented Mar 2, 2015 via email

@tresf
Copy link
Member

tresf commented Mar 2, 2015

At no point along the line did I see setThisButtonsColorTo(getColorFromStylesheet()),

It is here:
https://github.com/LMMS/lmms/blob/master/src/gui/LmmsStyle.cpp#L133

:)

Yeah, this is going to get tricky... I think we have quite a few widgets which aren't accessible via the style.css because of how they are initialized. This is where @badosu said we'll have to help out. This could be your calling... we may finally have good themability!!

trackOperationsWidget, trackLabelButton  {
    background-color: rgb(255, 0, 0);
}

image

-Tres

@tresf
Copy link
Member

tresf commented Mar 2, 2015

Also, warning that theming stable-1.1 will break on stable-1.2 due to some re-factoring of class names between branches. i.e. trackLabelButton => TrackLabelButton

@badosu
Copy link
Contributor

badosu commented Mar 2, 2015

@Spekular Also, besides providing access to themeability of some widgets that are not possible today, we may granularize widgets classes to provide for specific themeing needs. Just open an issue whenever you find one of these cases.

@tresf
Copy link
Member

tresf commented Mar 3, 2015

Just open an issue whenever you find one of these cases.

I think I agree... We'll be inundated fast though... There are a ton of these cases from my testing today.

@Sti2nd
Copy link
Contributor

Sti2nd commented Mar 3, 2015

Also, warning that theming

Oh, I thought the plan was to wait with break theming backwardscompatibility for LMMS2?

@tresf
Copy link
Member

tresf commented Mar 3, 2015

Oh, I thought the plan was to wait with break theming backwardscompatibility for LMMS2?

As far as I'm concerned, every single version breaks theme backwards compatibility. Expect this for years to come, unfortunately.... Just the nature of re-factoring and cleaning up code.

If you're interested in shimming in backward-compat code for themes, we can cross that bridge as well, but IMO this isn't new with 1.1 or 1.2 so shouldn't really come as a surprise:+1:

@Sti2nd
Copy link
Contributor

Sti2nd commented Mar 3, 2015

It does come as a surprise when I am pretty sure Vesa and others have waited with putting code into 1.2 and instead targeted master because of that plan. However I am perfectly fine with it :) So if you are going to break backwardscompat you could probably include a lot of the changes in master branch regarding theming too, I mean why wait?

OFF TOPIC: We should label themes (on lsp) with version too, btw, cause the majority of them cannot be used unless you run really old LMMS versions

@tresf
Copy link
Member

tresf commented Mar 3, 2015

So if you are going to break backwardscompat

Who is "you"? Every release has broken this. I'm not sure what part of my post isn't clear. 0.4.15 themes don't work on 1.0.0. 1.0.0 themes don't work on 1.1.0. What is different for 1.2.0 then?

To give you some background on the decision, Vesa originally wanted the major re-factoring to happen at the 2.0 stage, but Lukas re-factored anyways. I'm very grateful he did because we need to keep moving forward with this software (and the inconsistent naming was very hard to follow).

The 2.0 milestone was created by Vesa for some very large impacting changes and IMO it would be irresponsible of us to make all of them at once. The rate we're progressing at right now has a lot of momentum and I feel the 1.2.0 milestone to be at a happy place between staying with 1.1 forever and moving all of our eggs to the 2.0 basket. 1.2.0 is exactly where it should be I feel and re-factoring is part of that milestone.

But your comment about breaking themes... If you want to take the time to fix a 1.1.0 theme to ALSO work with 1.2.0, now is the time (yes it is technically possible to do this in pure CSS and pngs), but that is a responsibility of the 3rd party themers for now (sorry!). We can't guarantee compatibility between versions until we make theming more comprehensive (a goal that Specular, Cusis, Rebecca are helping us understand). We also need to settle on permanent names for things.. and there's just too much good happening now to put a pause on small incremental changes for a 3rd party skin that a small percentage of our users use.

-Tres

@Sti2nd
Copy link
Contributor

Sti2nd commented Mar 3, 2015

Ok, so I missed out on that Lucas re-factored anyways.

@tresf
Copy link
Member

tresf commented Mar 4, 2015

Ok, so I missed out on that Lucas re-factored anyways.

312 files affected. It's a monster... #1353

Here's Vesa's comments

Well I'm mainly concerned because there's so much stuff going on right
now... there's the lmms2 branch where I'm trying to start up the efforts
to redesign and clean up LMMS, several people are currently targeting
master with pull requests... lots of things that will conflict.

Then again, there's never a really "good" time for this kind of stuff,
but still...

@diizy
Copy link
Contributor

diizy commented Mar 4, 2015

Backwards compat for themes is almost impossible to provide since there will always be new things in new versions that didn't exist in old versions that now have to be themed.

So backwards compat for themes will likely break in every new release, like Tres said.

LMMS 2.0 would break compat in more than just themes though, it's a completely different level of breakage.

@Spekular
Copy link
Member Author

Spekular commented Mar 4, 2015 via email

@tresf
Copy link
Member

tresf commented Mar 4, 2015

Yeah, we need a better way to do it. I think we have many GUI elements simply unreachable by CSS. For starters, you can add setObjectName() to the QWidgets that need styling.

https://github.com/LMMS/lmms/blob/master/src/gui/MainWindow.cpp#L178

Once you are comfortable with doing that, we can decide on a permanent solution. I'm just hesitant to decide on a direction without some feedback from the other core developers. 👍

@tresf tresf mentioned this issue Mar 8, 2015
5 tasks
@tresf tresf added the duplicate label Mar 8, 2015
@tresf
Copy link
Member

tresf commented Mar 8, 2015

@Spekular consolidating this bug report into #1839 for better organization. However, please feel free to post here as needed.

@tresf tresf closed this as completed Mar 8, 2015
@musikBear
Copy link

good stuf here
-but also stuf that 'rincles mi forehead'
-this could end up as a dogs dinner, unless it is seriously controlled
Having a 'group' of more or less unrelated peeps working simultaneus on this, will not only result in duplicates -eg same stuff done several times.. but also be almost 100% sure to create absurd confusion. I dont think this can be achived by a mixed uncontrolled group -This needs rigid organized control

finally a Q :

Have you tried adding them? :)
QPushButton {
background-color: rgb(255, 0, 0);
}
Have you tried adding them

Where have you these 'them' from
(Here QPushButton)
Does a complete list of all elements exists? -or is that some ad-hoc read-the-code and discover expirience..
And if so:
Are every them denoted by the 'Q' -That would at least make 'them' searchable
Let me know, because a complete list of all is definately first step.
A list with
Qname, picture of element, usage, placePos
f.i. in a spread-sheet
Then some control would be in place at least

@tresf
Copy link
Member

tresf commented Mar 9, 2015

@musikBear if you're interested in helping with this, please chime in.

This thread is a learning location for @Spekular and whoever else wants to chime in.

Does a complete list of all elements exists?

No, that is the problem and that's what we're discussing. 👍

Are every them denoted by the 'Q' -That would at least make 'them' searchable

Actually, we want to avoid the Qs for most operations. That's a brute force hack because we don't have the objects named in a way we can style them directly.

Most of our developers' initiatives this past year have been around the code stability and features, not so much the themes. Themes have taken a back seat. We're getting there though... We're having the necessary discussions and we should have much better theme support in the no-so-distant future. 👍

We'll do it, we'll do it right, and we'll do our best to make it very easy to make a custom theme. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants