-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Color and font themes #6698
Color and font themes #6698
Conversation
I don't think WebUI matters much in this case, there are 3rd party interfaces to use.
+1, something like a basic mockup of qbt.
Doesn't this boost up the complexity? for devs, dependency cycles? for users also, imagine a creator shares one of his theme but the theme depends on another one he didn't share....
I imagine there will be import button and maybe export button, then this shouldn't be an issue. |
This would greatly simplify things.
Yes, but advantages outweigh, I think. We can deal with the problem you mentioned in the following way: when a theme inherits a non-existent one, or inherits nothing, we can modify it on loading to inherit one of the base qBt themes. In this way the theme will continue to work somehow. But what we get in return is flexibility to add new elements to our base themes without breaking all the user ones.
I meant not a theme file, but implementation of a new color scheme, like |
PR Updated: all files are now in |
Before making the final push and polishing I would very much appreciate your opinions. |
src/base/preferences.cpp
Outdated
#if (defined(Q_OS_UNIX) && !defined(Q_OS_MAC)) | ||
bool Preferences::useSystemIconTheme() const | ||
{ | ||
return value("Preferences/Appearance/useSystemIconTheme", true).toBool(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need "Preferences" in these paths? Why not just "Appearance/useSystemIconTheme" and so on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, inherited code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many coding style issues! Did you forget use uncrustify?
src/base/preferences.cpp
Outdated
} | ||
|
||
// Move RSS cookies to global storage | ||
QList<QNetworkCookie> cookies = getNetworkCookies(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the following lines are unrelated here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should belong to #6375
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines was in this place earlier but they were removed in current master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... Mistake during rebasing then, I guess.... Thanks.
src/gui/executionlog.cpp
Outdated
@@ -28,15 +28,21 @@ | |||
* Contact : [email protected] | |||
*/ | |||
|
|||
#include "executionlog.h" | |||
|
|||
#include "guiiconprovider.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these includes on bottom of Qt includes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK ,but I still think that it is better to place them in the following order:
- corresponding header file
- app includes
- Qt include
- STL and other standard includes
Why? Because standard library includes are of less importance, everybody knows what is inside , but what is much more important is app modules inter-dependencies, showed by app includes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second point: system includes are usually not sensitive to the include order, while app includes might have implicit dependencies. Listing them closer to the start of the includes list helps to find such dependencies if any.
src/gui/mainwindow.cpp
Outdated
@@ -141,6 +146,10 @@ MainWindow::MainWindow(QWidget *parent) | |||
{ | |||
m_ui->setupUi(this); | |||
|
|||
Theme::Serialization::registerGuiColorProviders(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it is time to create explicit GUI component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean something like:
src/base/ui.h
:
struct UI
{
virtual void show() = 0;
}
and in `src/gui/gui.h'
#include "base/ui.h"
class GraphicalUI: public UI
{
void show() override {
MainWindow::show();
}
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this PR, of course...
I didn't think about implementation, I have the idea itself. Now we have MainWindow as GUI manager, but this is wrong approach.
@@ -30,6 +30,9 @@ | |||
|
|||
#include "downloadedpiecesbar.h" | |||
|
|||
#include "theme/colorprovider.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
App includes should be after system/Qt includes.
src/gui/theme/colorprovider.h
Outdated
|
||
namespace Log | ||
{ | ||
enum MsgType : int; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum MsgType: int;
|
||
const QString Theme::ThemeInfo::sectionName = QLatin1String(GROUP_NAME); | ||
|
||
Theme::ThemeInfo::ThemeInfo() = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not in declaration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise it would be inlined, and we do not want it, right?
src/gui/theme/themeinfo.cpp
Outdated
return debug; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant blank line
src/gui/theme/themeinfo.h
Outdated
* Description_uk=Description in Ukrainian | ||
* | ||
*/ | ||
class ThemeInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brace
src/gui/theme/themeinfo.h
Outdated
|
||
QDebug operator<<(QDebug debug, const ThemeInfo &info); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant blank line
src/gui/theme/themeinfo.h
Outdated
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant blank line
src/base/preferences.cpp
Outdated
QList<QPair<QString, QString>> prefsToMigrate = { | ||
{ "Preferences/General/Locale", "Preferences/Appearance/Locale" }, | ||
{ "Preferences/General/AlternatingRowColors", "Preferences/Appearance/AlternatingRowColors" }, | ||
{ "Preferences/Advanced/useSystemIconTheme", "Preferences/Appearance/useSystemIconTheme" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@glassez , probably you suggest to replace names here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Here is another question. Consider font themes. Quite likely the default font theme should use default application font. In terms of [Info]
Name=Default
Description=Default qBittorrent font theme.
[Fonts]
TransferList=QFont:
TorrentProperties=QFont:
ExecutionLog=QFont: which is not very good, because of the empty descriptions. @LordNyriox believes that a descriptive placeholder is better: [Fonts]
TransferList=QFont:<default>
TorrentProperties=QFont:<default>
ExecutionLog=QFont:<default> But here is another decision: when exporting a font theme, should we compare fonts to the default one and write empty string or the placeholder? Or should we write full font string? I tend towards the latter, because if user wants the default font, they can simply erase the description string from the theme. Or, we may check whether theme contains default fonts and ask user first how they need to be serialized: as full descriptions of as empty strings. |
Why should default theme define something? It has only default values, isn't it? |
Well, default colour theme define colours. Why default font theme can't? |
I just mean we can omit default values... |
If you suggest to omit entries with default values in the default theme files, then I disagree. It would perplex theme loading code and theme interface. Naturally, inherited values are omitted in a theme. But they are simply loaded from the inherited theme. Otherwise, the theme loading code (resides in the theme class now) needs to know what value is the default one, and while it is more or less obvious for fonts, there is no a single default value for colours. That is why I decided that it will be simpler for us to control default values via default theme files, but not from source code. Those theme files are bundled as resources and always present. The only thing code needs to know is the name of the default theme. |
Then I don't know how the previous questions? If you need explicit values, then use them. What empty strings and placeholders? |
Because I also need "default application font" value. OK, thank you for the discussion. I think to implement empty strings and optionally replace them with font descriptions when exporting a font theme. |
What do you mean by that? |
Please see the current built-in font theme. It contains empty values which mean "application default font". |
Thanks for notification. I'm glad to know that this is being worked on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding style mostly.
src/gui/theme/themecommon.cpp
Outdated
} | ||
|
||
Theme::ThemeNotFoundException::ThemeNotFoundException(const QString& themeName) | ||
: std::runtime_error("Could not file theme named '" + themeName.toStdString() + "'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Could not find theme file"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, themes are addressed by theme name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "Could not find a theme file named"? I could be wrong but
file theme
doesn't seem right, theme file
seems more correct. Anyway in the current one you forgot "Could not find file theme named"? Unless the class name is deceiving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right! Thanks! There is a typo. Has to be "Could not find theme named".
src/gui/theme/themeprovider.cpp
Outdated
#include "themeinfo.h" | ||
#include "base/logger.h" | ||
#include "base/profile.h" | ||
#include "base/settingsstorage.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section should be at the end and base should be on top according to the guidelines.
src/gui/torrentmodel.cpp
Outdated
@@ -39,9 +39,10 @@ | |||
#include "base/torrentfilter.h" | |||
#include "base/utils/fs.h" | |||
#include "torrentmodel.h" | |||
#include "theme/colortheme.h" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant extra line.
src/gui/transferlistwidget.cpp
Outdated
@@ -58,6 +58,8 @@ | |||
#include "transferlistdelegate.h" | |||
#include "transferlistsortmodel.h" | |||
#include "updownratiodlg.h" | |||
#include "theme/themeprovider.h" | |||
#include "theme/fonttheme.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One line up.
#define QBT_THEME_SERIALIZABLECOLORTHEME_H | ||
|
||
#include "colortheme.h" | ||
#include "colorprovider_p.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One line up.
src/gui/mainwindow.cpp
Outdated
@@ -95,6 +95,11 @@ | |||
#include "lineedit.h" | |||
#include "executionlog.h" | |||
#include "hidabletabwidget.h" | |||
|
|||
#include "theme/themeprovider.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 lines down.
src/gui/executionlog.cpp
Outdated
#include "guiiconprovider.h" | ||
#include "loglistwidget.h" | ||
|
||
#include "theme/themeprovider.h" | ||
#include "theme/colortheme.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One line up.
src/gui/theme/themecommon.h
Outdated
#define QBT_THEME_THEMECOMMON_H | ||
|
||
#include <QString> | ||
#include <QLoggingCategory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One line up.
|
||
namespace Theme | ||
{ | ||
class SerializableFontTheme: public FontTheme { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bracket here and a few lines below empty line before private:
#define QBT_THEME_SERIALIZABLEFONTTHEME_H | ||
|
||
#include "fonttheme.h" | ||
#include "fontprovider_p.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One line up.
@thalieht: thanks, addressed! |
I still don't understand why you can't omit these entries instead of having empty values. |
Suppose you are modifying a theme element or extending theme type. For example, you are renaming theme element. You have to rename it in base theme file and in the source code. And suppose you forgot to change its name in one of the places or mistyped it. With current approach you get error during theme loading, while in the opposite case you would silently get default font. It is even more important, I think, that themes of both types behave uniformly, because there is no "default colour" and one may not omit colour elements. |
src/base/preferences.cpp
Outdated
|
||
if (!preValue.isNull()) { | ||
qDebug() << "Migrating preference" << pre << "->" << post; | ||
setValue(post, pre); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setValue(post, preValue);
@evsh, by the way, why not implement theme support using Qt style sheets? |
Stylesheets change widget rendering engine, and the widgets do not look native anymore. While with this PR I try yo get more native look for qBt. However, this PR offers a way to add more theme types. |
Should the dark theme work? I tried on win10 and ubuntu gnome and without success. |
Please explain what does not work, I don't understand what do you mean by "dark theme". This PR controls only colours of the custom-drawn UI elements (i.e. the widget colours are controlled by a system style). The whole aim of this PR is to fit qBt internal colours into the existing colour/style environments. |
I meant |
No, it will not, sorry. That is controlled by Qt Style. |
@evsh |
@zero77: no, there are no such plans at the moment, and see the discussion above, please. |
So it for now change colors for Missing icons for
Progress bar in General panel should reflect this color scheme too. |
Also for the download progress bar, i.e. all the custom qBt colours.
Yes, it is covered too.
Thanks! |
@evsh, this PR became unmaintainable. 13 commits (not so bad) and 315 changed files (!!!). GitHub is terribly slow here, and navigation is not working. |
@LordNyriox: thanks! |
@LordNyriox: could you redo the status icons inversion with the updated icons, please? |
@LordNyriox: thanks a lot! I don't need PNGs for this PR (which in fact drops PNG icons in favor of SVG files). |
1. Use SVG icons and drop png files 2. Add function to change SVG fill color and use it to produce monochrome icon set. 3. Allow torrent state icons to be colored corresponding to state colors.
Is this still being worked on? |
Guys, it's been quite some time of silence on this. Can we expect theming in any version, at all, considering it's been almost a year since the PR came up? I'd love the feature to be in qbit. I wish I could help even, but I don't have much knowledge of C++ and Qt. :/ |
Will fix #6434 |
Sad it's been closed. The simplistic yet informative uTorrent look is the only thing keeping me from switching to qBittorrent completely. |
Hi @qbittorrent/frequent-contributors, hi @magao,
At first, I link here related discussion in #6363 and related PRs: #3936 #4057 #6156 #6186 #6196.
So, the idea is to let user customise colours and fonts. But instead of providing a UI panel with a list of widgets to set all the colors and fonts, I decided to take another approach and implement colour and font themes. Why?
On the other hand, themes in files encapsulate all this stuff, and while that does not simplify editing much, at least it happens outside of the app.
What are the main ideas for the colour themes? Most of these properties are modelled after X11 icon theme and KDE themes.
QSettings
).QColor::QColor(QString)
and some other objects. They are stored as "Scheme:value" strings. There are two known schemes now: 'RGB' and 'QPalette'. Obviously, 'RGB' maps ontoQColor::QColor(QString)
and 'QPalette' maps ontoQPalette::color()
.Take a look at two supplied examples: 'Default (Light)' and 'Default (Dark)'. The latter one inherits the former one and modifies it. It is supposed that all the themes will inherit 'Default (Light)' at the end.
This allows us to extend themes, modifying only these two default ones and all user themes will inherit new values.
Plans:
The new "Appereance" settings tab look:
Bundled theme files: https://gist.github.com/evsh/03e56bfb49ba3b014d1a7bc63f9e7e1c
Icons
Added some tune-ups for icons. The goal is to provide the following two possibilities: correlate state icons with the color theme (done) and make all icons monochrome (partially done). To implement those tasks, I propose to drop PNGs and use SVG icons. Then I implement a simple "search-and-replace" function to change icon colour, which is then used to colourise state icons and to de-colourise default theme. Right now the de-colourised icons use fixed colour (defined by application palette), but we may provide user a colour chooser.
Screenshots with monochrome icons follow.
Light theme:
Dark theme:
Please note the changed progress bar tooltip:
Here are two most significant problems with the icons:
So, the question is: how to extend our icon theme?