Map Icons with IDs #10948

jagannatharjun · 2019-07-19T16:01:54Z

Map icons in qbt-theme folder
Map icons in skins folder
Make System Icon theme work

jagannatharjun

...

src/icons/icons.json

src/icons/icons.qrc

LordNyriox · 2019-07-25T23:33:51Z

@jagannatharjun: Thanks for all your work on this feature. :}

After this PR is finalized and merged, would you be willing to post a template theme (including icons) for qBittorrent?

jagannatharjun · 2019-07-26T14:25:32Z

@LordNyriox yes

glassez

Well, congratulations, good job 👍
Now it's time to fine tune it.
We should have such names that they are as obvious as possible, but without excesses in them.
I will review them as soon as I have time and make my suggestions and corrections.

glassez · 2019-08-04T09:42:51Z

src/gui/aboutdialog.cpp

@@ -45,7 +45,7 @@ AboutDialog::AboutDialog(QWidget *parent)
    // Title
    m_ui->labelName->setText(QString("<b><h2>qBittorrent " QBT_VERSION " (%1-bit)</h2></b>").arg(QT_POINTER_SIZE * 8));

-    m_ui->logo->setPixmap(Utils::Gui::scaledPixmapSvg(":/icons/skin/qbittorrent-tray.svg", this, 32));
+    m_ui->logo->setPixmap(Utils::Gui::scaledPixmapSvg("QBittorrent.Logo", this, 32));


I would use "AboutDialog.Logo".

@glassez: It is my understanding that the same icon is used in multiple places (including the system tray).

Do you really want to name the icon based on only one of those components?

@LordNyriox, if you had followed #10903, you might have understood my intentions...
#10903 (comment)

…nto icon-theme

jagannatharjun · 2019-08-06T14:50:57Z

Should I fall back to the default icon mapping if the icon path can't be resolved with custom iconconfig.json . May be provide an option

I am thinking to replace the current selection method with something like this.
With the current setup, It seems more natural.

glassez · 2019-08-06T17:25:30Z

My main suggestions for naming are the following:
ID should contain two main parts.
First part (actually last part of ID) is "common semantic" (e.g. "AddAction" or "Logo").
The second part is "scope" of particular ID, possible multilevel (e.g. "AddTorrentDialog.TorrentContentView").
"Scope" can be omitted in icon mapping (partially or fully) by replacing with "*" character. It allows the theme creator to achieve any desired detail. E.g. someone can set different icons for each particular ID but someone else can create very reduced theme (by having only entries like "*.AddAction", i.e. set the same icon for all similar places).
We should avoid any "internals" in ID parts. I.e. "ContentTree" is not so good, IMO (due to "Tree" in it). It should just point to some widget (or view) that shows content of torrent (regardless of the particular type of this widget), so "TorrentContentView" is better name.
We can avoid too much detail in ID scopes, but only if it is not likely to be needed in the future (because we will not be able to change existing IDs without breaking existing themes).

glassez · 2019-08-07T13:46:52Z

Should I fall back to the default icon mapping if the icon path can't be resolved with custom iconconfig.json

What do you mean exactly?

If there is no such ID (or appropriate generic ID that matches it) in theme then you should use built-in icon (and cache it for this ID). This situation can occur when some new qBittorrent version introduces new ID so there is no matches in existing themes. You also should indicate this event in some way (maybe show message box about missing icons or some less annoying way). Or maybe don't fallback and just show some predefined icon placeholder?

If the theme contains some ID but mapped path is invalid then it's real bug in theme and you shouldn't fallback to correct icon.

src/gui/uithememanager.cpp

glassez · 2019-08-09T18:39:27Z

src/gui/uithememanager.cpp

@@ -140,5 +156,41 @@ QString UIThemeManager::getIconPath(const QString &iconId) const
        return path;
    }
 #endif
-    return IconProvider::instance()->getIconPath(iconId);
+
+    const auto iter = m_iconMap.find(iconId);


Seems this method has logical errors. I need to get a fresh look at it.

…nto icon-theme

src/gui/uithememanager.cpp

jagannatharjun · 2019-08-27T13:06:42Z

Here is a theme if anyone wants to try out.
https://github.com/jagannatharjun/qbt-theme/blob/master/material-white/material.qbtheme
@glassez update?

glassez · 2019-08-28T06:50:13Z

@glassez update?

I'm sorry, I just can't get around to it for lack of free time. It's on my to-do list, so I'll be back here as soon as I can.

glassez

getIconPath() is still unchecked...

src/gui/uithememanager.cpp

glassez · 2019-08-29T17:13:33Z

src/gui/uithememanager.cpp

+
+    m_iconsDir = useCustomUITheme ? ":uitheme/icons/" : ":icons/";
+
+    QFile iconJsonMap(m_iconsDir + iconConfigFile);


I'm not sure that's right... Does it make sense to override mapping for "system theme" icons in a custom theme? Shouldn't there just be a three-state option (default-system-custom)? @Chocobo1?

Does it make sense to override mapping for "system theme" icons in a custom theme?

I think not.

Shouldn't there just be a three-state option (default-system-custom)?

I agree although I haven't checked how the GUI should look like.

src/gui/uithememanager.cpp

src/gui/aboutdialog.cpp

glassez · 2019-08-29T17:40:51Z

src/gui/uithememanager.cpp

        if (icon.name() != iconId)
-            icon = QIcon::fromTheme(fallback, QIcon(IconProvider::instance()->getIconPath(iconId)));
+            icon = QIcon::fromTheme(fallback, QIcon(getIconPath(iconId)));


Also how do you suppose to provide fallback icon now? It seems that it should come from (system theme) icon mapping now.

I now have changed the fallback to fallbackSysThemeIcon

Should I add separate icon id for fallback as well? I think there are only like 4 instances where fallback is actually used.

Well, It does make sense to add IconIds for fallbacks as well. Will replace direct references with Icon ids.

src/gui/uithememanager.cpp

jagannatharjun · 2019-09-22T04:11:09Z

ubuntu icon theme - link

src/gui/uithememanager.cpp

src/gui/iconconfig.cpp

src/gui/uithememanager.cpp

glassez · 2019-09-30T06:45:00Z

src/gui/aboutdialog.cpp

@@ -32,6 +32,7 @@

 #include "base/unicodestrings.h"
 #include "base/utils/misc.h"
+#include "gui/uithememanager.h"


Don't use "gui/" when include in sibling files.

There are actually only two such reference one is this one you mention and other is src\gui\rss\articlelistwidget.cpp. should I don't use this prefix even in gui\rss folder?

Just follow other include names style in the same file.

glassez · 2019-09-30T06:54:24Z

src/gui/uithememanager.cpp

+{
+    static const QHash<QString, QString> iconConfig = {
+        {"AboutDialog.Logo", ":icons/skin/qbittorrent-tray.svg"},
+        {"AddNewTorrentDialog.TorrentContentView.RenameAction", ":icons/qbt-theme/edit-rename.svg"},


Maybe we shouldn't add "Action" suffix? Now it seems superfluous. What do you think?

Hmm yes It does seem superfluous.

I think we should recheck all IDs at last step.

src/gui/uithememanager.cpp

glassez · 2019-09-30T08:00:34Z

src/gui/uithememanager.cpp

@@ -95,50 +601,74 @@ QIcon UIThemeManager::getIcon(const QString &iconId) const
    return getIcon(iconId, iconId);
 }

-QIcon UIThemeManager::getIcon(const QString &iconId, const QString &fallback) const
+QIcon UIThemeManager::getIcon(const QString &iconId, const QString &fallbackSysThemeIcon) const


This method interface is still contradicts the implemented model. As I said before fallback should be a part of mapping. Client just want to call getIcon("SomeIconId"). Which icon is really used is determined by the current configuration.

What shall be the name?
MainMenu.Fallback.ConfigureAction ,
Fallback.MainMenu.ConfigureAction,
MainMenu.ConfigureAction.Fallback,
MainMenu.ConfigureAction2

What shall be the name?

Sorry, don't understand you.
There is only one name for each IconID. But there should be the way to assign several mappings to it. You can use either "multi map" for system icon config or map each id to list of values.

I am thinking to use "multi map".
it should only be limited to the system icon config?

Of course. In case of custom icon theme author should provide truly existed files so no fallback is needed.

since system icon config is internal mapping does it make sense to not do pattern based searching here.

I am not doing pattern-based searching for system icon map.

Yes, at a first glance.
But I need to look at the whole algorithm to answer exactly.

glassez · 2019-09-30T08:05:09Z

src/gui/rss/articlelistwidget.cpp

@@ -98,7 +99,7 @@ void ArticleListWidget::handleArticleRead(RSS::Article *rssArticle)
    if (!item) return;

    item->setData(Qt::ForegroundRole, QPalette().color(QPalette::Inactive, QPalette::WindowText));
-    item->setData(Qt::DecorationRole, QIcon(":/icons/sphere.png"));
+    item->setData(Qt::DecorationRole, UIThemeManager::instance()->getIcon("RSS.ReadArticle"));


I would use something like "RSSView" instead of "RSS".

May be RSSTab?

Tabs is just a way to organise Views/Widgets in container (Main window in this case).

They're actually used in RSS Feed List so may be RSSFeedList, but RSSView seems good to

jagannatharjun · 2019-09-30T16:58:27Z

src/gui/rss/feedlistwidget.cpp

    }
    else {
-        icon = UIThemeManager::instance()->getIcon("inode-directory");
+        icon = UIThemeManager::instance()->getIcon("RSS");


Where it is actually used? I mean I can't able to understand in which context this icon is actually used thats why such a weird name.

glassez · 2020-06-03T13:12:26Z

Superseded by #12861.

jagannatharjun commented Jul 19, 2019

View reviewed changes

glassez reviewed Jul 19, 2019

View reviewed changes

src/icons/icons.json Outdated Show resolved Hide resolved

src/icons/icons.qrc Outdated Show resolved Hide resolved

glassez changed the title ~~[WIP] Map Icons with IDs in gui~~ [WIP] Map Icons with IDs Jul 20, 2019

glassez self-assigned this Jul 20, 2019

glassez added the GUI GUI-related issues/changes label Jul 20, 2019

glassez added this to the 4.2.0 milestone Jul 20, 2019

jagannatharjun mentioned this pull request Jul 23, 2019

Allow Styling through QSS #10702

Merged

Map icons with IDs

ce7aa45

jagannatharjun force-pushed the icon-theme branch from a459d39 to ce7aa45 Compare July 25, 2019 17:42

Fix

4f48df5

jagannatharjun changed the title ~~[WIP] Map Icons with IDs~~ Map Icons with IDs Jul 25, 2019

glassez reviewed Aug 4, 2019

View reviewed changes

jagannatharjun added 4 commits August 6, 2019 13:37

Merge branch 'master' of https://github.com/qbittorrent/qBittorrent i…

23100b4

…nto icon-theme

Review

6d7ac46

Cache by IconPath

c9b298f

Don't search again

07bae56

glassez reviewed Aug 9, 2019

View reviewed changes

jagannatharjun added 2 commits August 10, 2019 18:24

Merge branch 'master' of https://github.com/qbittorrent/qBittorrent i…

a55a72c

…nto icon-theme

may be improve

6d74291

Chocobo1 reviewed Aug 11, 2019

View reviewed changes

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

jagannatharjun added 2 commits August 16, 2019 13:05

System Theme

06eada5

Add toString

137b12d

glassez reviewed Aug 29, 2019

View reviewed changes

jagannatharjun force-pushed the icon-theme branch from d660d83 to 03f0208 Compare September 26, 2019 15:08

Try to improve

1ad64ed

jagannatharjun force-pushed the icon-theme branch from 03f0208 to 1ad64ed Compare September 26, 2019 15:18

jagannatharjun mentioned this pull request Sep 26, 2019

Release plan for 4.2.0 final release #11280

Closed

xavier2k6 mentioned this pull request Sep 26, 2019

A new theme option showed up but I can't find a theme file it asked. Is it out there? #11282

Closed

glassez reviewed Sep 27, 2019

View reviewed changes

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

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

jagannatharjun added 3 commits September 28, 2019 18:56

Move iconconfig inside cpp and remove custom flagsdir

8ccfc8e

Correct spelling

e11bf4b

Fix

fa0615e

glassez reviewed Sep 30, 2019

View reviewed changes

jagannatharjun added 2 commits September 30, 2019 19:54

Review

7586691

use multimap for system iconconfig

2b5ef75

jagannatharjun commented Sep 30, 2019

View reviewed changes

Rename RSS -> RSSView in mappings and remove unused headers

291d59b

sledgehammer999 modified the milestones: 4.2.0, 4.2.1 Dec 3, 2019

DoomsDay007 mentioned this pull request Dec 4, 2019

Dark Theme #6434

Closed

sledgehammer999 modified the milestones: 4.2.1, 4.2.2 Dec 19, 2019

sledgehammer999 modified the milestones: 4.2.2, 4.2.3 Mar 24, 2020

glassez modified the milestones: 4.2.3, 4.2.4 Apr 4, 2020

sledgehammer999 modified the milestones: 4.2.4, 4.2.5, 4.2.6 Apr 22, 2020

glassez closed this Jun 3, 2020


		m_iconsDir = useCustomUITheme ? ":uitheme/icons/" : ":icons/";

		QFile iconJsonMap(m_iconsDir + iconConfigFile);

Map Icons with IDs #10948

Map Icons with IDs #10948

Conversation

jagannatharjun commented Jul 19, 2019 • edited

jagannatharjun left a comment • edited

Choose a reason for hiding this comment

LordNyriox commented Jul 25, 2019 • edited

jagannatharjun commented Jul 26, 2019

glassez left a 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

jagannatharjun commented Aug 6, 2019

glassez commented Aug 6, 2019 • edited

glassez commented Aug 7, 2019

Choose a reason for hiding this comment

jagannatharjun commented Aug 27, 2019 • edited

glassez commented Aug 28, 2019

glassez left a 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

jagannatharjun Sep 26, 2019 • edited

Choose a reason for hiding this comment

jagannatharjun commented Sep 22, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glassez commented Jun 3, 2020

jagannatharjun commented Jul 19, 2019 •

edited

jagannatharjun left a comment •

edited

LordNyriox commented Jul 25, 2019 •

edited

glassez commented Aug 6, 2019 •

edited

jagannatharjun commented Aug 27, 2019 •

edited

jagannatharjun Sep 26, 2019 •

edited