Appearance tab. Issue #6363. #6375

magao · 2017-02-12T01:24:23Z

Moves existing options to a new "Appearance tab" as discussed in issue #6363.

Changed preferences icons:

Existing Behaviour icon moved to Appearance.
Behaviour now has the gear icon.
WebUI icon changed to qbt-theme/webui.png (which I think it was probably intended to be).

2 new signals emitted from Preferences when theme-related options are changed or dark/light theme change detected:

Preferences::configureThemeProvider() emitted first, theme providers (e.g. GuiIconProvider) need to connect directly to ensure they are configured before being used;
Preferences::themeChanged() for components that need to update themselves when a theme change occurs.

Both of these signals are emitted before Preferences::changed().

--HG-- branch : magao-dev

zeule · 2017-02-13T11:36:39Z

@magao, thank for posting the code! Frankly speaking, I don't like the result of our plan (#6363 (comment)) because it results in overconcentration of responsibilities in the Preferences class: I do believe that neither theme detection nor signals for changes in environment belong to it. But here is no placeholder created for them yet, because of the order in the plan. I belieave the plan needs to be adjusted a bit. Thus perhaps we better create the theme management components (as bare signletons) right away, move settings and signals there (a PR), move current colors and fonts selection into the default theme (a PR), and then add additional themese?

magao · 2017-02-13T20:16:37Z

I'll have to think about this some more (just woke up, no caffeine yet ...).

The advantage of having the Preferences object signal themeChanged() is that it makes it trivial to detect changed options that affect the theme.

But we could easily have a mixture of the 2 - a separate GuiPreferences singleton (or maybe ThemePreferences) which handles theme providers and emits themeChanged() once things are configured.

And palette changes would be forwarded to the GuiPreferences.

I don't think we should move the actual preference accessors to it - I think leaving them on Preferences is the way to go.

zeule · 2017-02-13T23:48:21Z

The advantage of having the Preferences object signal themeChanged() is...

Can not agree with you, sorry. Changes in the environment are not changes in the application preferences.

And there is another point which needs to be raised here: the project tries to move away from concentrating everything in the Preferences class, but instead serialise settings for a component inside the component itself (directly using SettingsStorage). When our settings were not united, there was no common place for them and they were left in the Preferences. Now we introduce a well defined component which will manage the application theme. Therefore we should move those settings out of the Preferences into the corresponding singletons. The options page should access them via these new classes too.

And that is why I proposed to change our roadmap a bit and settle these components before implementing all their future stuff, but not moving it into them later. Thus I propose you to limit this PR within options consolidation in the new options page.

magao · 2017-02-16T20:52:30Z

@evsh Done. Please set up the code how you want it with the default theme, and once that's merged I'll port my icon fixes (inverting, coloured to match state) and dark/light theme detection on top of the default theme.

zeule

Sorry, I did not write these remarks last time.

zeule · 2017-02-16T20:57:49Z

src/base/preferences.cpp

+        { "Preferences/Advanced/useSystemIconTheme", "Preferences/Appearance/useSystemIconTheme" },
+    };
+
+    for (QList<QPair<QString, QString>>::iterator iter = prefsToMigrate.begin(); iter != prefsToMigrate.end(); ++iter) {


you can write:

for (auto iter = prefsToMigrate.begin(); iter != prefsToMigrate.end(); ++iter) {

I'll change it to that (auto is another C++11 thing I wasn't aware of).

zeule · 2017-02-16T20:58:26Z

src/base/preferences.cpp

@@ -1611,10 +1642,9 @@ void Preferences::upgrade()
    QStringList labels = value("TransferListFilters/customLabels").toStringList();
    if (!labels.isEmpty()) {
        QVariantMap categories = value("BitTorrent/Session/Categories").toMap();
-        foreach (const QString &label, labels) {
+        foreach (const QString &label, labels)


for (const QString &label: labels)?

This change was from uncrustify.

No, I meant that one can use range-based for here too, instead of Qt's foreach macro

Yes - but my point is that this an automated change in the code cleanup commit - it's not part of the changes for the feature.

zeule · 2017-02-16T23:24:58Z

src/gui/optionsdlg.cpp

@@ -77,13 +77,14 @@ OptionsDialog::OptionsDialog(QWidget *parent)
    setModal(true);

    // Icons
-    m_ui->tabSelection->item(TAB_UI)->setIcon(GuiIconProvider::instance()->getIcon("preferences-desktop"));
+    m_ui->tabSelection->item(TAB_APPEARANCE)->setIcon(GuiIconProvider::instance()->getIcon("preferences-desktop"));
+    m_ui->tabSelection->item(TAB_BEHAVIOUR)->setIcon(GuiIconProvider::instance()->getIcon("gear"));


The icon name has to be FDO compatible. "preferences-desktop-theme" seems to be suitable.

I didn't change the names of any of the icons - just used what was already there.

What is FDO? I don't think you mean Feedback-Directed Optimization - I'm guessing it's something to do with FreeDesktop?

FDO stands for FreeDesktop.org, and here is the icon theme specification. You may also look at QIcon::fromTheme() documentation.

I'd prefer to leave doing any of this to someone who knows something about it e.g. you.

OK, I'll change icon names

~~OK, I'll change icon names~~

No, there will be a problem then at merging. Could you, please, copy the icon "gear" into "preferences-desktop-theme" and change this call to:

m_ui->tabSelection->item(TAB_BEHAVIOUR)->setIcon(GuiIconProvider::instance()->getIcon("preferences-desktop-theme"));

or, if you do not want to copy the icon:

m_ui->tabSelection->item(TAB_BEHAVIOUR)->setIcon(GuiIconProvider::instance()->getIcon("preferences-desktop-theme", "gear"));

Or, no! The icon "preferences-desktop-theme" is for the "Appearance" tab. "preferences-desktop" should be used for the "Behaviour" tab as it is now. Things get complicated :) If you like, leave it as is.

There is no preferences-desktop-theme.png in the existing codebase. I didn't change any icon names - just moved around the ones that were already being used since we didn't have an icon for the Appearance tab.

I will revert them all (and not have an icon for Appearance) and you can do whatever you want to.

zeule · 2017-02-16T23:26:12Z

src/gui/optionsdlg.cpp

    m_ui->tabSelection->item(TAB_BITTORRENT)->setIcon(GuiIconProvider::instance()->getIcon("preferences-system-network"));
    m_ui->tabSelection->item(TAB_CONNECTION)->setIcon(GuiIconProvider::instance()->getIcon("network-wired"));
    m_ui->tabSelection->item(TAB_DOWNLOADS)->setIcon(GuiIconProvider::instance()->getIcon("folder-download"));
    m_ui->tabSelection->item(TAB_SPEED)->setIcon(GuiIconProvider::instance()->getIcon("speedometer", "chronometer"));
 #ifndef DISABLE_WEBUI
-    m_ui->tabSelection->item(TAB_WEBUI)->setIcon(GuiIconProvider::instance()->getIcon("network-server"));
+    m_ui->tabSelection->item(TAB_WEBUI)->setIcon(GuiIconProvider::instance()->getIcon("webui"));


Same here: "network-server" is the name which is found in almost any icon theme.

Revert this change, please.

Or you may do:

GuiIconProvider::instance()->getIcon("network-server", "webui")

zeule · 2017-02-16T23:29:03Z

@magao thank you. I will play my round on top of this.

magao · 2017-02-16T23:34:41Z

@evsh I'll address the review comments this weekend. I didn't want to change any icon names, etc in this commit - just wanted the minimal changes to get the Appearance tab. Maybe I shouldn't have changed the WebUI icon.

I'm happy for you to reorganise as you think best - the only investment I have in this is that I want to get the changes from my other PR implemented ...

zeule · 2017-02-18T00:46:42Z

@magao, you forgot to remove cb_use_icon_theme member declaration from advancedsettings.h.

--HG-- branch : magao-dev

magao · 2017-02-19T00:50:27Z

@evsh Minimal version pushed - addressed comments, reverted all icon changes.

zeule · 2017-04-21T19:40:51Z

@LordNyriox, stop your flooding already, please.

sledgehammer999 · 2017-04-30T13:53:39Z

@magao are you still interested in this? If yes, please rebase.
@evsh is this PR still valid?
Personally, I agree with the icon change in the first post. I also agree that theme management should no be inside Preferences.
However, I don't know if you have already reached a consensus on this somewhere else or this is still pending?

zeule · 2017-04-30T13:57:24Z

@evsh is this PR still valid?

I'm working on top of this. Current approach is in #6698, and I plan to update it with fully functional color themes part later today.

FranciscoPombal · 2020-02-27T17:08:18Z

@magao are you still interested in this?

magao · 2020-02-27T21:01:41Z

Don't really care. If someone else wants to take the code and create a new PR feel free to do so.

Follow project coding style. Issue qbittorrent#2192.

98707fd

--HG-- branch : magao-dev

magao force-pushed the issue6363_tab_signal branch from 09a06c4 to e96078f Compare February 16, 2017 20:49

magao changed the title ~~Appearance tab and themeChanged() signal. Issue #6363.~~ Appearance tab. Issue #6363. Feb 16, 2017

zeule suggested changes Feb 16, 2017

View reviewed changes

Appearance options tab

8d8f4a4

--HG-- branch : magao-dev

magao force-pushed the issue6363_tab_signal branch from e96078f to 8d8f4a4 Compare February 19, 2017 00:50

LordNyriox mentioned this pull request Mar 15, 2017

[Wishlist] Windows Dark Theme #2641

Closed

LordNyriox approved these changes Apr 21, 2017

View reviewed changes

zeule mentioned this pull request May 4, 2017

Color and font themes #6698

Closed

12 tasks

LordNyriox mentioned this pull request Aug 29, 2019

Move "Check for program updates" checkbox out of Advanced settings #11143

Merged

FranciscoPombal added GUI GUI-related issues/changes Look and feel Affect UI "Look and feel" only without changing the logic labels Feb 27, 2020

FranciscoPombal added the PR abandoned label Mar 7, 2020

FranciscoPombal closed this Mar 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Appearance tab. Issue #6363. #6375

Appearance tab. Issue #6363. #6375

magao commented Feb 12, 2017

zeule commented Feb 13, 2017

magao commented Feb 13, 2017 •

edited

zeule commented Feb 13, 2017

magao commented Feb 16, 2017

zeule left a comment •

edited

zeule Feb 16, 2017 •

edited

magao Feb 16, 2017

zeule Feb 16, 2017

magao Feb 16, 2017

zeule Feb 17, 2017

magao Feb 17, 2017

zeule Feb 17, 2017

zeule Feb 16, 2017

magao Feb 16, 2017 •

edited

zeule Feb 17, 2017

magao Feb 17, 2017

zeule Feb 17, 2017

zeule Feb 17, 2017 •

edited

zeule Feb 17, 2017

magao Feb 17, 2017

zeule Feb 16, 2017

zeule Feb 17, 2017

zeule Feb 17, 2017

zeule commented Feb 16, 2017

magao commented Feb 16, 2017

zeule commented Feb 18, 2017

magao commented Feb 19, 2017

zeule commented Apr 21, 2017

sledgehammer999 commented Apr 30, 2017

zeule commented Apr 30, 2017

FranciscoPombal commented Feb 27, 2020

magao commented Feb 27, 2020

Appearance tab. Issue #6363. #6375

Appearance tab. Issue #6363. #6375

Conversation

magao commented Feb 12, 2017

zeule commented Feb 13, 2017

magao commented Feb 13, 2017 • edited

zeule commented Feb 13, 2017

magao commented Feb 16, 2017

zeule left a comment • edited

Choose a reason for hiding this comment

zeule Feb 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magao Feb 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeule Feb 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeule commented Feb 16, 2017

magao commented Feb 16, 2017

zeule commented Feb 18, 2017

magao commented Feb 19, 2017

zeule commented Apr 21, 2017

sledgehammer999 commented Apr 30, 2017

zeule commented Apr 30, 2017

FranciscoPombal commented Feb 27, 2020

magao commented Feb 27, 2020

magao commented Feb 13, 2017 •

edited

zeule left a comment •

edited

zeule Feb 16, 2017 •

edited

magao Feb 16, 2017 •

edited

zeule Feb 17, 2017 •

edited