Color and font themes part 2

[zeule]

This PR contains the main part of the themes support code. It introduces code for theme handling (loading and accessing), applying themes and reacting to application palette changes. Here we also introduce a tab in options dialog to consolidate all settings related to app appearance.

Please see #6698 for screenshots.

[LordNyriox]

@evsh: For the record, will the "extra" color-themes from [3137c24] be included in Part-3 or Part-4?

[zeule]

@LordNyriox: in #3 if these two commits will be accepted.

[thalieht]

Uncrustify please. I'd spam the PR if i pointed out everything.

[thalieht]

Coding style

[glassez]

@evsh, I looked here a little bit...
Although previously we came to some agreement that have cross-dependencies is a bad idea (and you even wrote that fixed it, as I recall), but it's still here! E.g.:

const QColor messageColor = Theme::ColorTheme::current().logMessageColor(msg.type);

Why ColorTheme needs to know in which place the item is used?

[zeule]

@glassez: I can't understand your concerns, sorry. Theme elements are named, and as such it is known from the very beginning (from the theme file) where each element will be used.

[sledgehammer999]

Marking this for 4.0.1, since I am really close to releasing 4.0.0 and this seems to be a bit big.

[glassez]

@glassez: I can't understand your concerns, sorry.

Again, I haven't watched the entire code thoroughly. I'm just trying to evaluate it outside, i.e. to answer the question "how the application uses this feature?". What I see is contrary to my vision. Maybe I just don't understand the goals of given design... So I describe my vision first.
I believe that (when it will be completed) I (or someone else) can just use this feature where it will needed. E.g. I have some widget A and I need to make its color cofigurable via theme. I want just to apply A.color = Theme.getColor(itemId). I don't want to make any changes in theme feature implementation.

[glassez]

The other question is why color and font themes should be separated?

[zeule]

@glassez, I guess you are missing the starting point for the approach taken here. These theme files are not general by design, i.e. a colour theme does not extend QPalette (but also see below). I proposed already that approach and it got rejected. If the team changed their minds, I can, of course, implement that one again. Let me summarise the two approaches here.

The problem: qBt uses more colour roles than QPalette contains. We use colours for torrent states, log message severities, and download progress bar. There may be two principal solutions: extend QPalette with more roles or define colours for each use case. Extending roles does not mean that we get a role for each our UI element, but we need to map role colours onto UI elements either statically or dynamically. Let me point out right away that this PR combines both approaches and makes mapping dynamic.

Extending QPalette roles

It is hard for me to come up with a proposal here, because I use KDE/Plasma colour themes for many years, and those themes do exactly this task, and do that well. So, let me briefly describe their approach for those who never used KDE. They extend colours roles in three directions. Firstly, they add new colour roles for "Active", "Inactive", "Positive", "Neutral", and "Negative" states. Here is the full list:

BackgroundAlternate
BackgroundNormal
DecorationFocus
DecorationHover
ForegroundActive
ForegroundInactive
ForegroundLink
ForegroundNegative
ForegroundNeutral
ForegroundNormal
ForegroundPositive
ForegroundVisited

Then these roles are defined for several groups of UI: views, buttons, tooltips, selection, window decoration. Finally they combine these groups into sets (QPalette == set) and define three sets: active, inactive and disabled colours. Transition to these sets are determined by a fixed function and KDE palette just defines values for its parameters. Additionally, there are two standard function to lighten and darken a colour.

Taking this approach we would need to define new roles and map them onto UI elements. Additionally, we need to take colour values for these roles from a system palette when possible (Linux desktops).

Define explicit colours for our purposes

This is exactly what we are doing now, the only thing that can be changed is to pass the definition to the user side allowing them to customize the colours to their tastes or to match their environment.

So, what exactly this PR offers?

1. It reads color definitions for our UI elements from a file, so users can change them to their taste.
2. Colour definitions can be dynamic too, referring to explicit RGB values, QPalette roles, or (in the next part of the series of these small PRs, but you can go and look in Color and font themes #6698) KDE palette entries. The syntax in the theme files is extensible and other colour sources can be added.

I hope, @glassez, this explains why qBt UI elements appear in the theme interface.

[glassez]

I hope, @glassez, this explains why qBt UI elements appear in the theme interface.

Unfortunatelly, No!
Your answer passed my question, not having points of contact with it.
I will try to ask using an example from your code:

const QColor messageColor = Theme::ColorTheme::current().logMessageColor(msg.type);

Why not to do as:

const QColor messageColor = Theme::current().getColor(colorIdByMsgType(msg.type));
// assuming colorIdByMsgType() returns string key used in theme file and defined somewwhere above

without any cross-dependencies.

And Yes, I purposely used Theme instead of Theme::ColorTheme in my example to once again ask "why separate font and colort hemes?".

[zeule]

Why not to do as: …

Because colorIdByMsgType() is exactly what the ColorTheme interface is. And, more importantly, this is all a client needs. Let me remind you, that the theme is just a map from our UI element to a color role.

why separate font and color themes?'

No, why join them? Is there any logical relation between a colour set for UI and font set?

[glassez]

No, why join them? Is there any logical relation between a colour set for UI and font set?

Isn't there this dependency?
Really, I'm so far from it... I thought it was used side by side. You say all this in order to follow the system theme. So I thought, what if we have some kind of exotic theme, that uses some colors and fonts, and the native interface of the application is contrary to this, we custom fonts and colors using theme feature. If I'm wrong, and system themes doesn't affect fonts, forget it. Let it be separately.

[glassez]

Because colorIdByMsgType() is exactly what the ColorTheme interface is. And, more importantly, this is all a client needs.

That is why I don't understand why this extra (and completely unnecessary) layer that creates cross-dependencies? I don't want to change ColorTheme to get the color for new element. I just want to say "give it to me"!

[zeule]

What is the new element?

[glassez]

Look at QSettings! If I want to get settings for class A, I don't have to change QSettings and add QSettings::getSettingForClassA(settingId), I just call QSettings::value("SomeClassASettingId"). QSettings doesn't know who uses which settings. It just how to load it from some specific source (.ini file, Windows registry etc.) and returns it in some unified way.
Also your Theme. It should be able to load colors (fonts) from different sources (encoded in the theme file itself, taken from Plasma, etc.), and return it in the form QColor (QFont).

[glassez]

What is the new element?

For example, in the future we will need to use some custom colors in a new widget.

[glassez]

@evsh, I'm actually surprised this discussion. I thought you agreed with my arguments last time.

[zeule]

@glassez: I consolidated the code which makes you unhappy in src/gui/theme/colortheme.cpp. I still believe that 3 functions make more good than evil, producing an easy to use interface and referring only to enumerations from the other parts of the app. Enum ColorTheme::Element breaks rules for identifier names, but I had to use something to mark group delimiters. If you know a cleaner solution, I will be thankful. As to your last remark: no, theme class is not a QSettings or anything similar. The entity you are referring to is the SerializableTheme class. That one is indeed more or less resembles a general storage element. String keys are unsalable here due to performance requirements (colours for the transfers list have to be fetched very quickly, QMap<QString,...> creates noticeable penalty).

@evsh, I'm actually surprised this discussion. I thought you agreed with my arguments last time.

Apparently, we did not understand each other at that time. Let's try harder :)

For example, in the future we will need to use some custom colors in a new widget.

One has to add a new item to the corresponding enum and update the default theme file to add the new default value.

[glassez]

Let's try harder :)

Ok.

I still believe that 3 functions make more good than evil, producing an easy to use interface and referring only to enumerations from the other parts of the app.

IMO, I see no good from it. It's just redundant level only complicates the use of this feature by requiring some additional steps to introduce new theme elements.

Enum ColorTheme::Element breaks rules for identifier names, but I had to use something to mark group delimiters.

I don't suggest to use enum ids.

String keys are unsalable here due to performance requirements (colours for the transfers list have to be fetched very quickly, QMap<QString,...> creates noticeable penalty).

But you still use string keys.
I believe what I want is code refactoring to make it simplest. Try to consider my offer not through the prism of your current implementation (this way is not constructive), but from the perspective of an application workflow under this part. What I propose will not change this workflow so we can't talk about performance loss. To demonstrate this, I need a few to delve into your code, and I have to give you a detailed example. I'll try to do it as soon as I'll have little time for this.

[zeule]

IMO, I see no good from it. It's just redundant level only complicates the use of this feature by requiring some additional steps to introduce new theme elements.

No, function to map a new element to existing application enum is not required.

But you still use string keys.

I use them for serialization only. As soon as file is parsed, string keys are not used.

[thalieht]

Just noticed that there is a private slots section already.

[anwarsfaizul]

Just found out that seeding (black) icons stays black for unregistered torrent, This is not what it supposed to be. Whenever tracker sends a error message the icon should change accordingly... or how else we are going to get informed... I was seeding ~200 unregistered torrent for more than 3 months

Btw another dropped project like qbittorrent (cutetorrent, qt libtorrent rasterbar) has option to use theme as well as changing the interface from list(utorrent/qbittorrent) to rows(transmission). Its open source and you can check it out to improve here.

[ghost]

god damnit

[zeule]

Pardon?

[LordNyriox]

@zeule:

Pardon?

I believe the swearing was in response to the fact that you closed this PR.

For what it is worth, I cannot help but wonder why you closed this PR.

Are you canceling your theming project? Are you rewriting it? Or are you simply suspending it, until you have some time to spare?

[zeule]

Because my motivation was to make the app look nicer in my desktop environment (Plasma with a dark colour theme). However, @sledgehammer999 expressed clearly that dependencies on any platform except Windows, MacOS, and bare Linux are out of the table. Thus the goal is unreachable. Because of that and a strong resistance from @glassez I rethought the plan and decided that keeping the code in my own fork, which I merge the qBittorrent codebase into, better suit my needs and my (current) time opportunities.

[LordNyriox]

@zeule:

I rethought the plan and decided that keeping the code in my own fork, which I merge the qBittorrent codebase into

Why not make PRs for everything but the Plasma theming, and keep just the Plasma component in your own fork?

That way, Windows/Mac users can benefit from the basic theming system, while Plasma users can use your fork.

I know that I, personally, was really looking forward to using ColorThemes on my Windows system.

I use a custom "Dark Red on Lightless Black" system-wide theme (to reduce eyestrain), and I prefer to use app themes that match it.

[glassez]

@sledgehammer999 expressed clearly that dependencies on any platform except Windows, MacOS, and bare Linux are out of the table. Thus the goal is unreachable.

I think @sledgehammer999 could approve it if this dependency is optional and it doesn't affect the main code.

strong resistance from @glassez

I do not oppose this feature in general, although I would personally change the application to not to use any non-system elements (colors, fonts, etc.), but I don't want to discuss it.
My "strong resistance" affects only the over complicated way the feature is implemented by. I see an obscure, confusing code that is hardly maintainable. We must not forget that the original author may leave the project at any time, and most of the team just hobyist, not so tough professionals like @zeule (although, IMO, a professional always strives for simplicity).
You don't need to be a "cool" professional to see that this task has a simpler and clearer solution. But for all my questions "why don't you make it easier" I just got an answer like "because it's done that way, and therefore, this is the only right way".

P.S. All of the above is my personal POV, and it may not coincide with the POV of other project members. If most people think that everything is OK, I will not interfere.

[vit9696]

I am roughly an outsider here, yet as a person trying to somewhat improve the look and feel on macOS, which remains disgusting, I find this change fairly important. Not only as a change itself but as a base to other changes. For example, greyscale icons are more common on modern macOS, and no icons should change their opacity on selection.

I very well understand that code-wise it may be imperfect, yet it is always a trade off between having better code or working code.
Does it bring more features than complications?
— In my opinion, it does.
Does it crash or hang?
— From what I remember it does not.
Does it have other bugs?
— From what I remember the ones visible from the user's perspective were fixed.

I would rather wait for other opinions, but personally it seems to be the case where we should just take a deep breath, rebase and accept it. As all people are hobbists here, little time could be spared to fully rewrite it anyway.

[glassez]

Does it bring more features than complications?
— In my opinion, it does.

How can you compare them?
I didn't say it brings many complications to the main application code. Its own code is complicated so it hard understandable by someone except than its author.

I find this change fairly important. Not only as a change itself but as a base to other changes.

These changes are bad exactly as a base for other changes.

As all people are hobbists here, little time could be spared to fully rewrite it anyway.

I would fully rewrite it than try to change it if this will needed.

@Chocobo1, @sledgehammer999, please comment it in general.

[Chocobo1]

@Chocobo1, @sledgehammer999, please comment it in general.

I mostly agree with your previous post and here I only want to add that, if the code require maintenance in the future, it is out of my league, I won't be of much help.
Please disregard my ramblings, I believe @sledgehammer999 should have the final decision.

[zeule]

Thanks @glassez for showing right away what type of resistance I referred to. Let me iterate a few points of your latest comment.

I do not oppose this feature in general, although I would personally change the application to not to use any non-system elements (colors, fonts, etc.), but I don't want to discuss it.

Sounds strange at least. The whole point of this PR is exactly that: use system colours and icons (fonts is a side quest, easily solvable). QPalette simply does not contain significant aspects of the system colour theme on almost all supported platforms, the bundled icons are awful. It is surprising that putting so much efforts in reviewing this PR you twist its very core purpose that way.

My "strong resistance" affects only the over complicated way the feature is implemented by. I see an obscure, confusing code that is hardly maintainable.

That, as far as I remember, boils down to these 4 functions and a class that hide theme serialisation interface and theme file format and elements naming from the application logic layer. And I did not receive and reasoning why that is an evil.

...not so tough professionals like @zeule (although, IMO, a professional always strives for simplicity).
You don't need to be a "cool" professional to see that this task has a simpler and clearer solution. But for all my questions "why don't you make it easier" I just got an answer like "because it's done that way, and therefore, this is the only right way".

But instead I received quite a lot of comments of this kind, which go on personalities and insults...

[glassez]

The whole point of this PR is exactly that: use system colours and icons

I mean "use general purpose system colors" which are compatible with all supported platforms and look good everywhere without any additional "theming support". But since many people prefer to see all sorts of decorations that use specific colors that stand out from this "general purpose" palette, I do not call to change it (as I said "I don't want to discuss it").
Besides, you have a strange view of "use system colours and icons". Personally I mean by this "use it automatically" as it happens with "general purpose" colors. But it is impossible for custom colors which set not by its role but explicitly. You just give the user the ability to change them (in a particular case, assigning some color from the current theme).

That, as far as I remember, boils down to these 4 functions and a class that hide theme serialisation interface and theme file format and elements naming from the application logic layer.

I'm not talking about the feature interface, but mostly about its implementation. It's complicated. And I don't mean any brain-dead optimizations. I just compare it with the model that first comes to my narrow mind (I have described it superficially before, but I can repeat it if there are those who want to know it). And I don't understand why this can't be done so easily.
But again, I'm not hiding my stupidity (everyone can look into this feature implementation and get their own opinion about it) and I've repeatedly said that if the other project members do not agree with my concerns and they approve it, I will not interfere, we will simply deal with the routine cleaning of this PR and merge it.

But instead I received quite a lot of comments of this kind, which go on personalities and insults...

I'm sorry if some of my remarks are too personal and offensive for you. It's just that your reluctance to critically evaluate your own code is really annoying at times, although I try to be diplomatic (it does not always work). I hope that constructive spirit will prevail in our cooperation.

[zeule]

I mean "use general purpose system colors" which are compatible with all supported platforms and look good everywhere without any additional "theming support".

That's simply impossible with bare Qt. Please follow.

1. We want to draw interface elements using platform colours. OK, where once can get those colours from?
2. QPalette provides very basic set of colours that cover nothing except QStyle needs. Consider these examples:
   a) Windows: complete mess in terms of styling, each new UI API (sorry, do not know their names) is drawn in its own way and looks in its own way. The dark theme in Windows 10 which has zero impact even on the system File Explorer app is a good example.
   b) Qt does not support dark mode on Mojave QTBUG-68891. Notably, https://codereview.qt-project.org/#/c/232685/ just disables the dark mode colours completely instead of fixing that. Given the status of the Widgets module I would not be surprised if the dark mode gets delayed and will not be available with Mojave release.
   c) Plasma provides an extended palette with definitions for accents and shades.
   d) Gnome and GTK3. Well, those guys even do not have any standard for colour schemes, each GTK style defines whatever it wants via CSS, thus there is no even standard fro colour names.
3. Instead of implementing these many ways to pull colour information in the qBt I decided to provide a way for users to define their themes to match the desktop environment (because what else they would want to use themes for?).
4. Where possible, we can provide extraction of colour names from the environment (see color providers in this PR), but I don't know how to generalise colour roles. And even if I was able to, the Mojave case is not covered.

I'm not talking about the feature interface, but mostly about its implementation. It's complicated. ...

I can't agree with you that lesser number of complex entities is simpler than larger number of simple ones. I attempted to design the classes to be as primitive as possible. That was far from perfect, and thanks to your help they get improved. However, at this point I can't see how mixing different concepts into a single multi-powerful class would simplify maintenance. I would document them well instead.

[vit9696]

I did get through a half of this pull request changes and the comments posted above. Honestly said, I think it is not a problem of the implementation but rather architecture preference. The level of abstraction and entity separation depends on the project, and to my experience a much bigger maintenance issue is a misconception and a problem misunderstanding, both of which feel comparably reasonable here.

Regarding the amount of colour preferences I have to agree with Eugene. It should be beared that Qt is a piece of crap every Mac user hates. It unfortunately gives a hey to native Cocoa widgets and attempts to terribly mimic colours, which results in borked stuff. To give you and idea: even the right-click context menu is broken (border, background, selection, padding, font, rounding…). I am not sure it can be easily fixed with theming on Mac, yet I perfectly understand that Qt authors in their attempt to "unify" the UI made it in a way that makes everyone blow. To make the UI at least somewhat better a lot of effort is needed, commonly specific to each element in charge (similarly to how I addressed the stuff via ifdefs and patches on Mac). Mapping stuff to "common" colours/widgets/fonts is simply borked from the beginning.

Also, it has been quite some time Eugene had been here with this pull request (more than a year from what I remember). I admit that he still maintains it. From this point of view the question of maintenance is not a concern, he could fix himself what is needed.

[glassez]

Well, again, I'm not going to discuss the global problems of cross-platform development. I only expressed my opinion about some "ideal" model (for me), when all the colors are not explicitly set, but through their roles and dependencies on each other. But, apparently, this is unattainable in the current situation, when there are no (supported) standards in this area. So let's leave it and get back to the earth (to the current PR).

However, at this point I can't see how mixing different concepts into a single multi-powerful class would simplify maintenance.

You're constantly changing my words. I'm the first enemy of extremes. But as soon as I urge you to get out of your extreme, you imagine it's like I'm pulling you to the opposite shore. It's not!
I repeat, any tool, any idea, any pattern, etc. cannot be good in itself. It should be in its place. Any good idea can be brought to the point of absurdity if not stopped in time. You can not use some method everywhere just because it is "new" and "fashionable".

I can't agree with you that lesser number of complex entities is simpler than larger number of simple ones.

But when the implementation is so fragmented into simple parts that it hides all the basic logic, it's also a bad idea.
When you create some global elements that are not used and never supposed to be used outside the feature implementation, this is unacceptable.
When you impose numerical identifiers on theme entities that initially have string identifiers (ostensibly for better performance), it's unclear (why not just push them to a higher level?). It can be optimized in a more convenient place. The "feature user" level knows exactly which type of optimization is more appropriate for it.

This is my point of view and I do not pretend to be "the ultimate truth". As I said:

if the other project members do not agree with my concerns and they approve it (this PR), I will not interfere, we will simply deal with the routine cleaning of this PR and merge it.

[LordNyriox]

@zeule, @glassez: How about you try a middle ground?

@zeule: Create two different implementations of this theming system (as separate branches / PRs)—one the way you think it should be best coded (reduced to a more primitive level)—and the other the way you seem to think @glassez would prefer it (larger shared classes).

Based on the feedback from the first two "draft" implementations, you can then create a third branch/PR combining the best traits of both.

@Both-of-You: And for goodness' sake, butt heads somewhere other than an active PR. Create a new "Issue" thread for it if you have to—just do not clutter up a PR thread with a personal debate.

Please.

[zeule]

You're constantly changing my words.

Excuse me, I do not do that intentionally, only try to make conclusions from what I read.

But as soon as I urge you to get out of your extreme, you imagine it's like I'm pulling you to the opposite shore.

No, I simply need a justification for the move.

But when the implementation is so fragmented into simple parts that it hides all the basic logic, it's also a bad idea.

Not sure I get it. Could you be more specific, please?

When you create some global elements that are not used and never supposed to be used outside the feature implementation, this is unacceptable.

What global elements? What usage are you talking about? Again, be less general, please.

When you impose numerical identifiers on theme entities that initially have string identifiers (ostensibly for better performance), it's unclear (why not just push them to a higher level?). It can be optimized in a more convenient place. The "feature user" level knows exactly which type of optimization is more appropriate for it.

Not sure that it's about enum with theme elements, although if the guess is correct, the intention is to make compiler check that addressing to the elements is correct (and express the idea that theme elements are well defined set).

@zeule, @glassez: How about you try a middle ground?

A compromise has to be reasoned by non-subjective grounds.

@zeule: Create two different implementations of this theming system (as separate branches / PRs)—one the way you think it should be best coded (reduced to a more primitive level)—and the other the way you seem to think @glassez would prefer it (larger shared classes).

Sorry, I have neither enough time nor passion to do that.

[stampis]

As an end user it saddens me to hear that this feature will probably never be implemented.
The current color scheme of qbittorent is like nails in my eyes (running everything else with dark themes), and i have hoped for quite some time that a way to theme / change color scheme would be added.

Alas it was not meant to be it seems.

[ghost]

@stampis Yeah, this whole situation is ridiculous. Dark themes are so standard these days that even Windows RS5 has a dark mode explorer (will probably be the fall update).

If you google Deluge Dark Theme, there's a theme that covers 99% of the UI elements. I didn't like Deluge very much at first, but it has grown on me. There's a large repository of plugins that basically never go obsolete for it

[ghost]

I also am forced to look elsewhere for my bit torrent needs.. Deluge or Transmission maybe or maybe back to a patched version of utorrent
Pity I am unsubscribing from this thread and leave all to their own devices
Good Day to you

[zeule]

Re-submitted as #9283 because GitHub does not allow to reopen a PR after a forced push.

[glassez]

@zeule, @sledgehammer999, @qbittorrent/demigods etc.
I will not continue to comment on the details of the current implementation yet, but try to analyze the problem in General.
The main purpose of this feature is provide "Themes support" to allow user adjusting some color and fonts.
What is the simplest way to do it? Just implementing some color/font providers. The minimal interface of such provider (e.g. color provider) can be the following:

QColor getColor(const QString &colorID, const QColor &fallback) const;
bool loadTheme(const QString &themeFilePath);
(signal) void themeChanged();

ColorProvider::loadTheme() just reads given theme file and transforms its values into QColor's (depending of values format). ColorProvider knows nothing about usage of provided colors.
Such an implementation should be very simple, it fully satisfies the solution of the main task, and it can be easily reviewed and merged. And, most importantly, it can be easily extended in the future if a compatible theme file format is used, so it can be a good starting point if most advanced feature is really needed.

About reviewed implementation (this PR).
It handles more additional information about usage of each color, its source and format so its implementation requires much more code, and this code is more complex. But why do we need this additional information? The only thing that comes to mind is to have an advanced theme editor. The question is whether we really need it. Do we need to further inflate the application by adding support for this minor opportunity (theme editor)?
Anyway, the first step (basic themes support) shouldn't have many objections, but the second one is debatable and it can slow down the entire feature release.