Skip to content
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

Icon theme support in qbittorrent #10903

Open
jagannatharjun opened this issue Jul 9, 2019 · 17 comments

Comments

Projects
None yet
3 participants
@jagannatharjun
Copy link
Contributor

commented Jul 9, 2019

Proposal

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.

Being new to qbit's sources, this was my first reaction with the current icon arrangement.

@glassez glassez changed the title [Discussion] Icon theme support in qbittorrent Icon theme support in qbittorrent Jul 9, 2019

@glassez glassez added this to the 4.2.0 milestone Jul 9, 2019

@glassez

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

When you add icon support as part of an UI theme, GUIIconProvider class becomes an unnecessary layer. All necessary logic should be implemented in UIThemeManager class. For the first step you should transfer all existing logic from GuiIconProvider into UIThemeManager, replace all calls to GuiIconProvider::getIcon() with UIThemeManager::getIcon() and remove GuiIconProvider class.
Second step is making icons structure layout to be convenient for perfect theming support. The main problem of current implementation is that qBittorrent components call icons by name that produces unnecessary dependency (reverse dependency). Currently we use the same icon in different places but if the theme creator will want to use different icons it can't do it. I suggest to call icons by used component id (e.g. "TransferListWidget.ContextMenu.EditTrackersAction") and create a "component id to icon name" mapping (just some configuration file). Thus we will have predefined componentId set that can be mapped into some arbitrary icon set.
Of course it will require more work than implementing it as a patch for current icon support implementation that will produce inconsistencies, inconveniences and poorly maintainable code.

@jagannatharjun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

  1. Should UIThemeManager inherit from iconprovider?
  2. By "configuration file" did you mean ini file, component id as key and filenames as value?
  3. Can't we use alias tag from qrc file format instead of config file.
  4. qbt-theme and skins folder shall be combined into one, maybe icons or simply put what shall be the new folder icon structure?
@jagannatharjun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

I think IconProvider is only used as a base for GuiIconProvider, should I remove it as well.

@glassez

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Should UIThemeManager inherit from iconprovider?

I think No. Let's keep IconProvider as a separate component. At least in the early stages, until some details are clarified. IconProvider is required not only by GUI, I don't want we break something accidentally. It is better to make changes incrementally, otherwise we can drown in it. UIThemeManager can use it when it's needed.

By "configuration file" did you mean ini file, component id as key and filenames as value?

I would prefer use JSON formatted dictionary with component id as key and filenames as value. Qt doesn't provide public INI parser and I strongly dislike use QSettings just to read .ini files. @Chocobo1?

Can't we use alias tag from qrc file format instead of config file.

You can try several options and compare their pros and cons. But it seems to me that the way with a separate configuration file will be easier to maintain.

qbt-theme and skins folder shall be combined into one, maybe icons or simply put what shall be the new folder icon structure?

In the same way as for the first question. I wouldn't touch the existing icon layout structure yet (when using mapping, the physical layout doesn't matter, so the theme Creator can have any). There is a risk that we can break something else, like web UI.

@Chocobo1

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

I would prefer use JSON formatted dictionary with component id as key and filenames as value. Qt doesn't provide public INI parser and I strongly dislike use QSettings just to read .ini files. @Chocobo1?

I'm fine with using json as the format.

Personally I think this task could be hard, IIRC you might encounter the following: loading of different image formats: vector or scalar. Image caching. Size issues on HiDPI screens. So no need rush it, think carefully and do extensive tests if your free time allows.

@glassez

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

loading of different image formats: vector or scalar.

We can have some requirements for supported formats.

Personally I think this task could be hard

So I suppose to make changes incrementally (even as separate commits/PRs):

  1. Transfer all existing logic from GuiIconProvider into UIThemeManager, replace all calls to GuiIconProvider::getIcon() with UIThemeManager::getIcon() and remove GuiIconProvider class. Test and make sure that nothing is broken.
  2. Define so-called "Component ID"s, create mapping for existing (built-in) icons and use Component IDs to access icons from the qBittorrent code. Test and make sure that nothing is still broken.
  3. Implement loading icons from the UI theme.
  4. Solve the problems that are meantioned by @Chocobo1 above (and other possible drawbacks).

This approach should ensure that after each stage, the existing functionality of the application will be maintained, which will facilitate the development and testing of the entire feature (we can merge them into the master one by one to test it carefully, etc.).

@jagannatharjun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

 m_iconMap.insert("AddNewTorrentDialog.ActionRename","qbt-theme/edit-rename");
 m_iconMap.insert("CategoryFilterModel.Folder","qbt-theme/inode-directory");
 m_iconMap.insert("CategoryFilter.ActionAdd","qbt-theme/list-add");
 m_iconMap.insert("CategoryFilter.ActionAddSub","qbt-theme/list-add");
 m_iconMap.insert("CategoryFilter.ActionRemove","qbt-theme/list-remove");
 m_iconMap.insert("CategoryFilter.ActionRemoveUnused","qbt-theme/list-remove");

are these okay? There will be around 200 such entries 😫

I plan to use these as follows

const auto iconDir = useCustomTheme() ? ":/uitheme/" : ":/icons/";
const auto iconPath = iconDir + m_iconMap[iconId];
@glassez

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

are these okay?

I'm confused... why in code? I meant the JSON file. Otherwise how will a custom theme have its own mapping?

@glassez

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

@jagannatharjun, @Chocobo1, what do you think about my step-by-step plan above?

@jagannatharjun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

I'm confused... why in code?

QJsonDocument doc(m_iconMap);
doc.toJson();

The naming scheme is ok?

what do you think about my step-by-step plan above?

I am fine with it. I was actually thinking about doing it this way.

@jagannatharjun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

{
    "AddNewTorrentDialog.ActionRename": "qbt-theme/edit-rename",
    "CategoryFilter.ActionAdd": "qbt-theme/list-add",
    "CategoryFilter.ActionAddSub": "qbt-theme/list-add",
    "CategoryFilter.ActionRemove": "qbt-theme/list-remove",
    "CategoryFilter.ActionRemoveUnused": "qbt-theme/list-remove",
    "CategoryFilter.Folder": "qbt-theme/inode-directory"
}

@glassez

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

The naming scheme is ok?

If you mean c++ variable names etc. let's discuss them when we start reviewing the code.
If you mean so-called "component IDs"... main idea is that they should be self-descriptive for the theme creators (i.e. for users who may not be familiar with qBittorrent internals). E.g. "CategoryFilter.ContextMenu.ActionAdd" means Action "Add" from the Context menu for "Category" filter.

I am fine with it. I was actually thinking about doing it this way.

I just see that you've started thinking about the second step...

@jagannatharjun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Adding one more layer with "ContextMenu" seems good enough but makes this task more difficult, I mean there are around 200 such entries.

@glassez

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Adding one more layer with "ContextMenu" seems good enough but makes this task more difficult, I mean there are around 200 such entries.

You're right, there will be a lot of routine work. But we can not be lazy when laying the foundation, otherwise the whole building may collapse...
While we won't be able to avoid assigning unique names to all the "components" of the theme (otherwise we'll limit customization), I'm actually thinking of a way to reduce mapping files (when theme creator don't want to produce too detailed differences).
But more on that later... I'm too sleepy right now. Please be patient.
I hope I'm not boring you with my ideas.

@glassez

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

we won't be able to avoid assigning unique names to all the "components" of the theme (otherwise we'll limit customization)

Don't want anyone to impose unnecessary worries, I just thought it would be nice to provide a theme creator more freedom. But if you guys have any objections, we can limit that.

I'm actually thinking of a way to reduce mapping files

E.g.: We have several similar IDs ("CategoryFilter.AddAction", "TagFilter.AddAction", "TrackerList.AddAction", etc.). One theme creator want to assign different icons to each of them so it provide the following mapping:

{
    "CategoryFilter.AddAction" : "add_category",
    "TagFilter.AddAction" : "add_tag",
    "TrackerList.AddAction" : "add_tracker"
}

Other creator want to create less detailed theme (assign the same icon to similar actions):

{
    "CategoryFilter.AddAction" : "add_something",
    "TagFilter.AddAction" : "add_something",
    "TrackerList.AddAction" : "add_something"
}

I suggest to allow some kind of placeholders in mapping files:

{
    "*.AddAction" : "add_something",
    "TrackerList.AddAction" : "add_tracker"
}

So when "TrackerList.AddAction" is requested "add_tracker" will be returned, but when "TagFilter.AddAction" is requested and there's no explicit matching it will search for less strict matching ("*.AddAction").

@glassez

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Adding one more layer with "ContextMenu" seems good enough but makes this task more difficult

Of course we can avoid this extra layer if this does not cause ID conflicts.

@jagannatharjun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

is this correct?

{
    "AddNewTorrentDialog.ContentTree.RenameAction": "qbt-theme/edit-rename",
    "CategoryFilter.ContextMenu.AddAction": "qbt-theme/list-add",
    "CategoryFilter.ContextMenu.AddSubAction": "qbt-theme/list-add",
    "CategoryFilter.ContextMenu.EditAction": "qbt-theme/document-edit",
    "CategoryFilter.ContextMenu.RemoveAction": "qbt-theme/list-remove",
    "CategoryFilter.ContextMenu.Torrent.DeleteAction": "qbt-theme/edit-delete",
    "CategoryFilter.ContextMenu.Torrent.PauseAction": "qbt-theme/media-playback-pause",
    "CategoryFilter.ContextMenu.Torrent.ResumeAction": "qbt-theme/media-playback-start",
    "CategoryFilter.ContextMenu.Unused.RemoveAction": "qbt-theme/list-remove",
    "CategoryFilter.SubCategory": "qbt-theme/inode-directory",
    "CookiesDialog.AddAction": "qbt-theme/list-add",
    "CookiesDialog.RemoveAction": "qbt-theme/list-remove",
    "CookiesDialog.WindowsIcon": "qbt-theme/preferences-web-browser-cookies",
    "ExecutionLog.BlockedIPsTab": "qbt-theme/view-filter",
    "ExecutionLog.GeneralTab": "qbt-theme/view-calendar-journal",
    "LineEdit.FindAction": "qbt-theme/edit-find",
    "LogList.ContextMenu.ClearAction": "qbt-theme/edit-clear",
    "LogList.ContextMenu.CopyAction": "qbt-theme/edit-copy"
}

How should I go on naming this?
MainWindow.AddTorrentFile.AddAction?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.