Allow Styling through QSS #10702

jagannatharjun · 2019-05-24T19:04:18Z

EDIT: Following is my initial proposal, a lot has changed after that will update the docs soon

searches *.qss in {config-folder}/styles, and use those as style-sheet with mainwindow

src/gui/mainwindow.cpp

src/base/preferences.cpp

glassez

The implementation looks a bit inconsistent. You've mixed style management "by name" and "by file path", so there may be conflicts (e.g. user can manually set style in config file outside of style directory).
If you want to have styles in a predefined directory, you should store the style name in the settings (this way require some simple "style manager" to be implemented to avoid duplicating the logic of mapping the style name in the file path).
Otherwise you should allow user to specify any arbitrary style file (then you have to change the GUI).

src/gui/mainwindow.cpp

jagannatharjun · 2019-05-25T16:58:39Z

ok i took the "stylemanager" approach, i thought this may help in future

glassez

Sorry, I have no time to review it in detail now, so I make only general remarks.

To make it easier to review your changes further, please learn the qBittorrent Coding Style (you can find it in the source tree) and apply it to your contributions to the project.
Although your StyleManager doesn't depend directly on GUI classes/modules it's part of GUI in its essence (it has no essence outside GUI) so you should place it in qBittorrent GUI module.

Chocobo1 · 2019-05-26T10:22:53Z

Why not just let user select the qss file? Having to place qss file under a special directory is unintuitive IMO.
It can be a combo box with a fixed first entry "System default" and a second entry "Select..." which will pop up a file selector. And this way you probably could scrap the stylemanager class...

BTW, qss works better than I thought!

glassez · 2019-05-26T10:52:32Z

BTW, qss works better than I thought!

I have always been surprised by the attempts of some contributors to implement theme support in some custom way, although I initially pointed them to the built-in support...

jagannatharjun · 2019-05-26T10:53:40Z

@Chocobo1
I am planning a follow up for icons,
Idea is to search icons in stylefolder()/{style-name}/*, that's why I choose stylemanager
I think it will be more systematic and people can then distribute these files without letting end-user worry about the details

I can try that if you insist but i don't like that way

jagannatharjun · 2019-05-26T11:04:11Z

How about changing the style folder to the install directory, this way it will be easier to find
or add open style folder button along side the styleComboBox

glassez · 2019-05-26T11:14:36Z

i am planning a follow up for icons,
idea is to search icons in stylefolder()/{style-name}/*,
I think it will be more systematic and people can then distribute these files without letting end-user the details

We can implement some kind of theme bundles (i.e. set of "stylename.qss" and appropriate "stylename" folder with resource files, probably zipped in single file).

jagannatharjun · 2019-05-26T11:17:49Z

@glassez this can be done in follow up pr for icons,
Bundles can be extracted in stylefolder()

Chocobo1 · 2019-05-26T11:39:52Z

Idea is to search icons in stylefolder()/{style-name}/*, that's why I choose stylemanager

I really think showing style name/bundle name is redundant, user don't care about what name it is and it is just a file name.

I think it will be more systematic and people can then distribute these files without letting end-user worry about the details

Maybe stylemanager could still be useful, we'll see then.

We can implement some kind of theme bundles (i.e. set of "stylename.qss" and appropriate "stylename" folder with resource files, probably zipped in single file).

I imagine we could let user select a .zip bundle (only .zip as we have zlib already, otherwise too many dependencies...) and qbt will extract it to <config path>/guibundle folder. qbt will clear the guibundle folder before extracting.
And the UI could show as "GUI theme:" and a combo box with options: ["Default", "Custom", "", "Select..."]. "Custom" will be auto selected after user imports a bundle via "Select...".

ADDED: The "Custom" entry could also be named after the .zip bundle name, but there should be only one custom bundle at most as to cut down maintenance for novice users.

@jagannatharjun
If you're up to, some of the ideas here is eligible for this PR, no pressure.

jagannatharjun · 2019-05-26T11:51:16Z

I want a dark theme in the official repository because let's face it many users are lazy to google search and download files that's why bing is still out there.
I don't know how this can be done using "custom". Identifying styles by names will allow user to quickly change styles and test what better matches his style.

Chocobo1 · 2019-05-26T11:58:45Z

I want a dark theme in the official repository because let's face it many users are lazy to google search and download files that's why bing is still out there.

Do you pledge to maintain the dark theme for some time? I won't.
And are you sure a single (dark) theme will satisfy all users ranging from windows to linux and also macOS?

I don't know how this can be done using "custom". Identifying styles by names will allow user to quickly change styles and test what better matches his style.

I'm not sure what you mean by don't know.
When "Custom" is selected, qbt will search in guibundle folder and will apply the first found qss file and icons (when we are there).
Also the "Custom" could be named after the .zip bundle name, as mentioned before.

jagannatharjun · 2019-05-26T12:07:57Z

May be have it in the official repo but mark it as unofficial just like cmake support,
Having it in official repo will attract more audiences and support

I like the style-name way and having multiple qss to choose from. Do you assert your method? @qbt-users opinions?

Chocobo1 · 2019-05-26T12:19:55Z

May be have it in the official repo but mark it as unofficial just like cmake support,

Big difference! cmake will work or is suitable on many platforms while a single theme will not.

I like the style-name way and having multiple qss to choose from.

Multiple qss entry leads to the needs of removing it, you'll then have to introduce an UI element to delete the entries. Users will request it sooner or later.

Do you assert your method?

:) So far, I can only speak for myself (I do not represent this project). And from my limited experiences shared my own opinion about a (feasible) way to realize it, if anyone have better ideas, they are welcome!

jagannatharjun · 2019-05-26T12:23:48Z

I will do the delete and install button. how about that?

Chocobo1 · 2019-05-26T12:26:47Z

I will do the delete and install button. how about that?

No problem for me.

Also what you are going to do when name collision occurs when importing?

jagannatharjun · 2019-05-26T12:27:29Z

Ok then it doesn't make much sense if you will not have the dark theme in official repo. I can't maintain the theme myself. I am not a designer.

Any designer up for it?

jagannatharjun · 2019-05-26T12:35:24Z

Well I am sure some one someday will request to have multiple themes.

and about the collision we can check in availableStyles() then abort it or concatenate some suffix and try again

glassez · 2019-05-26T12:53:25Z

Hey, guys, stop, otherwise we'll be stuck with it, as has happened with other theme support implementations.
As I mentioned, I am a supporter of the minimum cost implementation of minor features. So I approve of the way "Select some theme file or reset to the default" (no need for any installation and other management). And when I mentioned "theme bundles", I meant nothing more than the ability to "Select some theme file or reset to the default" (again no management, just take the specified file and apply the resources bundled in it).

Well I am sure some one someday will request to have multiple themes.

Just have multiple theme (bundle) files somewhere in filesystem.

glassez · 2019-05-26T13:06:50Z

only .zip as we have zlib already, otherwise too many dependencies

AFAIK, zlib isn't about ZIP, it's just about .gz. Of course, it can be used to compress files inside the ZIP archive... But then you have to implement the ZIP parser manually. In addition, it can only extract a subset of .zip files.

Anyway we can just start with .qss only support until it died before it was born.

src/gui/mainwindow.cpp

jagannatharjun · 2019-05-26T14:24:11Z

styleCombo has additional text Select... which let user select new Qss file

jagannatharjun · 2019-05-27T14:45:23Z

@glassez @Chocobo1 review?

jagannatharjun · 2019-05-28T07:31:58Z

@glassez @Chocobo1 now?

glassez · 2019-05-28T08:31:57Z

@glassez @Chocobo1 now?

@jagannatharjun, sorry, but as I stated in
#10702 (comment) I'm against any kind of management of stylesheet files (I.e. install/uninstall etc.) unless it's strongly required.

Chocobo1 · 2019-07-09T03:52:12Z

src/gui/uithememanager.cpp

+    if (Preferences::instance()->useCustomUITheme()
+        && !QResource::registerResource(Preferences::instance()->customUIThemePath()
+                                        , "/uitheme"))
+        LogMsg(tr("Failed to load .qbtheme file - %1").arg(Preferences::instance()->customUIThemePath()));


I would write: "Failed to register .qbtheme file. Path: \"%1\""

I would produce more user-friendly Log messages (unless some internals are required to be described there): Failed to load UI theme from file \"%1\". There is enough information to understand what we are talking about both for the user and for the developer.

src/gui/uithememanager.cpp

Chocobo1 · 2019-07-09T03:58:03Z

src/gui/uithememanager.cpp

+        qApp->setStyleSheet(qssFile.readAll());
+        return;
+    }
+    LogMsg(tr("Can't open :/uitheme/stylesheet.qss, error: %1").arg(qssFile.errorString())


error: \"%1\"

I still don't see the point in showing the application internals in this message...

maybe the creator miss spell the file and these are for the creator anyway, more information is always good 😜

any suggestions for another message.

maybe the creator miss spell the file and these are for the creator anyway, more information is always good

Yes. But you still should print file path related to theme source root but not to the internal path used by qBittorrent.

any suggestions for another message

Couldn't apply theme stylesheet. stylesheet.qss couldn't be opened. Reason: %1. IMO, the message like this is both user-friendly and informative for the theme creator.

The message should give the user a clear understanding of what is at stake. It should also contain a minimum of information for the developer, sufficient to match the problem with the application code, if necessary.

src/gui/uithememanager.cpp

jagannatharjun · 2019-07-09T12:09:39Z

What's the point in wrapping the reason part by commas? E.g. Reason: no such file is readable enough, isn't it?

@Chocobo1 asked me to put the errors in quotes for other message and the previous message also.
Will remove it if you ask.

src/gui/uithememanager.cpp

Chocobo1 · 2019-07-09T13:41:01Z

@jagannatharjun
Please fix those warnings:
https://ci.appveyor.com/project/qbittorrent/qbittorrent/builds/25848426#L357

glassez · 2019-07-09T13:47:23Z

Please fix those warnings

@Chocobo1, I think we shouldn't wait after this is done and commits are squashed. All sorts of unnoticed little things can be fixed later. In addition, it is assumed that this feature will be expanded further.

Chocobo1 · 2019-07-09T13:52:47Z

@Chocobo1, I think we shouldn't wait after this is done and commits are squashed. All sorts of unnoticed little things can be fixed later. In addition, it is assumed that this feature will be expanded further.

What do you mean? This PR is still evolving or do you suggest merging it as-is?

jagannatharjun

I can't think of better name. XD

glassez · 2019-07-09T14:01:31Z

What do you mean? This PR is still evolving or do you suggest merging it as-is?

I think this PR is done within its scope and the feature can be expanded further without breaking its main interface. So it should be merged now (unless you find some critical flaws at the last moment).

Chocobo1 · 2019-07-09T14:26:21Z

So it should be merged now (unless you find some critical flaws at the last moment).

IMO the current state is very close to being finished judging by our standards, one last thing is squash the commits into the first one and I can approve.

glassez · 2019-07-09T14:31:56Z

@jagannatharjun, can you create appropriate wiki page with some info about theme format and creation instructions? You can attach your theme as an example there.

jagannatharjun · 2019-07-09T17:09:01Z

for icons, I would replace all direct references of :icons with helper function from iconprovider classes, for example, there are lots of direct references of icons in skin directory, would like to replace this with guiiconprovider::getSkinIconPath. This with getIconPath will internally call UIThemeManager::getIconDir for icons dir. UIThemeManager::getIconDir will return :icons\ or :uitheme\icons based upon settings.

glassez · 2019-07-09T17:23:46Z

IMO, current icons layout not so good to support theming in a convenient way. So I would make changes on both sides - how the icons are handled by the app and how the theme will provide them.
I have some suggestions (maybe not much thought). I want you to open up the relevant Issue where we can discuss the main aspects of the required changes before you start coding it. I really do not want this feature to be implemented in a lazy manner as some kind of patch to the existing code. It should have a well defined design.

jagannatharjun · 2019-07-09T17:33:55Z

@glassez #10903

Chocobo1 · 2019-07-10T11:33:10Z

@jagannatharjun
Thanks for fulfilling many people's wish.

jagannatharjun · 2019-07-10T16:05:02Z

It was my first big PR to any repo, so thanks to you guys for bearing with me for 2 months XD

glassez · 2019-07-10T16:10:42Z

@jagannatharjun, thank you for bringing this matter to an end, despite all the difficulties 👍
Many people throw their ideas unfinished when they realize that there are rules and requirements and we do not allow the wrong/unsuitable code, even if the idea is good in itself.

Piccirello · 2019-07-22T08:28:38Z

This feature will be used by so many more people if we bundle a dark mode theme in qBittorrent. #6434 has 293 👍 , the most of any issue (and 4x the number of the next highest issue).

@jagannatharjun if you're willing to maintain the dark theme, I think we should bundle it with qBittorrent.

glassez · 2019-07-22T08:45:36Z

bundle a dark mode theme in qBittorrent.

Personally, I would not consider this possibility until the entire "Theme support" feature is fully implemented and tested. "Dark mode" theme can't be complete (to release it officially) without having the appropriate icons to match it in color.
But until then, we can provide the appropriate resources in another way (e.g. Wiki page, pinned GitHub Issue etc.).

Piccirello · 2019-07-22T09:00:04Z

Completely agree regarding waiting for matching icons. We can use the wiki in the mean time. Once #10903 is resolved, we should bundle the dark theme with the app (assuming someone can support it).

Chocobo1 · 2019-07-22T09:05:55Z

Once #10903 is resolved, we should bundle the dark theme with the app (assuming someone can support it).

I would say "support" as in long-term support otherwise I think it would be wise to have contributors maintaining their own repo/forks.

Piccirello · 2019-07-24T06:41:51Z

I think their stance is the only correct one- bundle a dark mode only if we’re able to support it long term. It would be a disservice to our users to provide a tool that breaks in future versions.

Stanzilla · 2019-09-30T02:52:53Z

Where would I put style.qss files so the client sees them?

I tried %APPDATA%\qBittorrent and %APPDATA%\qBittorrent\styles\ and %APPDATA%\qBittorrent\styles\test\

jagannatharjun · 2019-10-05T07:33:52Z

@Stanzilla the method is changed in the course of PR (first post updated accordingly), I will try to write documents after #10948 or if 4.2 is released before that.

Meanwhile, you can use any style file(*.qbtheme) from here with 4.2 alphas.

glassez added the Look and feel Affect UI "Look and feel" only without changing the logic label May 25, 2019

glassez requested changes May 25, 2019

View reviewed changes

src/gui/mainwindow.cpp Outdated Show resolved Hide resolved

jagannatharjun force-pushed the qss-styling branch from fa0a605 to 732a9d9 Compare May 25, 2019 08:39

glassez requested changes May 25, 2019

View reviewed changes

src/base/preferences.cpp Outdated Show resolved Hide resolved

glassez requested changes May 25, 2019

View reviewed changes

src/gui/mainwindow.cpp Outdated Show resolved Hide resolved

jagannatharjun force-pushed the qss-styling branch from 4789cbf to eb19487 Compare May 25, 2019 17:49

glassez requested changes May 26, 2019

View reviewed changes

glassez reviewed May 26, 2019

View reviewed changes

src/gui/mainwindow.cpp Outdated Show resolved Hide resolved

Chocobo1 reviewed Jul 9, 2019

View reviewed changes

src/gui/uithememanager.cpp Outdated Show resolved Hide resolved

jagannatharjun commented Jul 9, 2019

View reviewed changes

Allow styling with QSS stylesheets

a24925c

jagannatharjun force-pushed the qss-styling branch from 6702880 to a24925c Compare July 9, 2019 14:27

Chocobo1 approved these changes Jul 9, 2019

View reviewed changes

glassez approved these changes Jul 9, 2019

View reviewed changes

Chocobo1 merged commit 51fa98a into qbittorrent:master Jul 10, 2019

jagannatharjun mentioned this pull request Aug 6, 2019

Map Icons with IDs #10948

Closed

3 tasks

thomas-mckay mentioned this pull request Apr 7, 2020

List row height inconsistency: Use QStyledItemDelegate consistently #12290

Open

Allow Styling through QSS #10702

Allow Styling through QSS #10702

Conversation

jagannatharjun commented May 24, 2019 • edited

glassez left a comment

Choose a reason for hiding this comment

jagannatharjun commented May 25, 2019 • edited

glassez left a comment

Choose a reason for hiding this comment

Chocobo1 commented May 26, 2019 • edited

glassez commented May 26, 2019

jagannatharjun commented May 26, 2019 • edited

jagannatharjun commented May 26, 2019 • edited

glassez commented May 26, 2019

jagannatharjun commented May 26, 2019

Chocobo1 commented May 26, 2019 • edited

jagannatharjun commented May 26, 2019

Chocobo1 commented May 26, 2019 • edited

jagannatharjun commented May 26, 2019 • edited

Chocobo1 commented May 26, 2019 • edited

jagannatharjun commented May 26, 2019

Chocobo1 commented May 26, 2019

jagannatharjun commented May 26, 2019 • edited

jagannatharjun commented May 26, 2019

glassez commented May 26, 2019

glassez commented May 26, 2019

jagannatharjun commented May 26, 2019

jagannatharjun commented May 27, 2019

jagannatharjun commented May 28, 2019

glassez commented May 28, 2019

Chocobo1 Jul 9, 2019

Choose a reason for hiding this comment

glassez Jul 9, 2019

Choose a reason for hiding this comment

Chocobo1 Jul 9, 2019

Choose a reason for hiding this comment

glassez Jul 9, 2019

Choose a reason for hiding this comment

jagannatharjun Jul 9, 2019

Choose a reason for hiding this comment

glassez Jul 9, 2019

Choose a reason for hiding this comment

glassez Jul 9, 2019

Choose a reason for hiding this comment

jagannatharjun commented Jul 9, 2019

Chocobo1 commented Jul 9, 2019

glassez commented Jul 9, 2019

Chocobo1 commented Jul 9, 2019

jagannatharjun left a comment

Choose a reason for hiding this comment

glassez commented Jul 9, 2019

Chocobo1 commented Jul 9, 2019

glassez commented Jul 9, 2019 • edited

jagannatharjun commented Jul 9, 2019 • edited

glassez commented Jul 9, 2019

jagannatharjun commented Jul 9, 2019

Chocobo1 commented Jul 10, 2019

jagannatharjun commented Jul 10, 2019 • edited

glassez commented Jul 10, 2019

Piccirello commented Jul 22, 2019

glassez commented Jul 22, 2019 • edited

Piccirello commented Jul 22, 2019

Chocobo1 commented Jul 22, 2019

Piccirello commented Jul 24, 2019

Stanzilla commented Sep 30, 2019

jagannatharjun commented Oct 5, 2019

jagannatharjun commented May 24, 2019 •

edited

jagannatharjun commented May 25, 2019 •

edited

Chocobo1 commented May 26, 2019 •

edited

jagannatharjun commented May 26, 2019 •

edited

jagannatharjun commented May 26, 2019 •

edited

Chocobo1 commented May 26, 2019 •

edited

Chocobo1 commented May 26, 2019 •

edited

jagannatharjun commented May 26, 2019 •

edited

Chocobo1 commented May 26, 2019 •

edited

jagannatharjun commented May 26, 2019 •

edited

glassez commented Jul 9, 2019 •

edited

jagannatharjun commented Jul 9, 2019 •

edited

jagannatharjun commented Jul 10, 2019 •

edited

glassez commented Jul 22, 2019 •

edited