Investigate if KIconThemes is needed as a framework
Open, Needs TriagePublic

Description

With Qt having theme support and having "QIcon::fromTheme", is there any point to having KIconThemes be a lib and not just some code in our QPT?

There's some public APIs for overlays, which maybe could be upstreamed / moved into KGuiAddons.
But little else

Details

Differential Revisions
D26370: Port away from KIconThemes
D26352: Fully port away from KIconThemes
D26345: Don't create pixmaps for logmode icons
D26237: Port from deprecated KIconLoader::IconSize()
D26074: Port away from depreacted IconSize() method
D26072: Port away from KIconThemes
D26057: Deprecate the top-level IconSize() function
D25955: Port away from global IconSize() method
D25957: Port away from KIconThemes
D25958: Remove unused KIconThemes dependency
D25858: Use QStyle for determining icon sizes
D25862: Port determining icon sizes to QStyle
D25898: Determine icon sizes via QStyle, not via KIconThemes
D25899: Don't manually propagate sub-window icons
D25697: Port away from KIconLoader::SizeSmallMedium
D25681: Get icon size from QStyle
D25385: Use icon name instead of pixmap in notification
D25378: Get icon size from QStyle instead of KIconLoader
D24964: [applets/weather] Port from KIconLoader to QIcon
D24796: Prefer giving KNotification icons rather than fixed-size pixmaps
D24795: Let QStyle handle icon sizes on buttons
D24801: Cleanup icon handling
D24781: Use QStyle to determine icon sizes, where necessary at all
D24777: Don't mess with button icon sizes, QStyle takes care of that for us
D24778: Don't add a non-existent icon search path
D24779: Use QStyle to determine icon size
D24780: Remove unnecessary window and button icon code
D23904: Add QIcon setters for the password dialogs
D23903: Remove dead icon loading code
D23899: Port from KIconLoader to QIcon::fromTheme

Related Objects

There are a very large number of changes, so older changes are hidden. Show Older Changes

Yes, please! The only thing I'm aware of that isn't available by other means is obtaining the file path of the icon, which is e.g. needed for use in Grantlee/HTML.

As disccused with @davidedmundson : review where icon file path access is used, might be replaceable with an icon "image provider" thingy for QTextBrowser/QWebEngine.

The KPixmapSequence stuff seems to be mostly used for "busy indiators" for which we now have a lovely KBusyIndicatorWidget

There's still many users of the icon loading and icon sizes, might make sense to port that first to QIcon, and then assess what's left. Right now it's hard to identify the problematic bits.

cfeck added a subscriber: cfeck.Sep 12 2019, 12:04 PM

KIconThemes also offers the icon chooser (button and dialog).

KIconThemes also offers the icon chooser (button and dialog).

Right, for which we need listing all icons in the theme, which QIcon doesn't have either I think. This is however used far less often, so dependency-wise it's the smaller problem I think.

dfaure added a subscriber: dfaure.Sep 12 2019, 12:26 PM

Then this task has the wrong title. It's not about killing the framework, but about not using it for simple icon loading use cases. Right?

I'd say that depends on what actually remains. The button and dialog could also fit in something like widget addons, unless forced to be separate by dependencies. Having this separate I think was mainly needed due to this being used all over the place.

KWidgetAddons is Tier 1. KIconThemes was separated, because it needs KConfig (and other dependencies).

The part of this that seems to cause the most wide-spread use is the size constants; overlays, icon dialogs, icon listing, icon file paths, etc are much much less common. So if we want to address something regarding dependencies, the sizes is the thing to go after.

Is there an alternative API for listing installed icon themes? I added functionality to Cuttlefish that uses this and the KCM may use it?

There is not. You should use this.

For icon sizes, would that be something worth adding to QStyleHints in Qt? There's also already some icon size stuff in QStyle. Our QPT could then read those values from kdeglobals like normal.

QStyle currently has:

  • PM_ToolBarIconSize
  • PM_SmallIconSize
  • PM_LargeIconSize
  • PM_IconViewIconSize
  • PM_ListViewIconSize
  • PM_TabBarIconSize
  • PM_MessageBoxIconSize
  • PM_ButtonIconSize
  • PM_TitleBarButtonIconSize

KIconThemes has the follow "semantic" sizes:

  • Desktop
  • Toolbar
  • MainToolbar
  • Small
  • Panel
  • Dialog

as well as the following "syntactic" sizes (ie. merely constants for fixed numeric values, without configuration):

  • SizeSmall
  • SizeSmallMedium
  • SizeMedium
  • SizeLarge
  • SizeHuge
  • SizeEnormous

I guess the main question would be how to map the KIconThemes semantic sizes to the QStyle values.

From an application point of view that seems possible:

  • Toolbar, MainToolbar -> PM_ToolBarIconSize
  • Small -> PM_SmallIconSize
  • Dialog -> PM_MessageBoxIconSize

Panel and Desktop should only be relevant for parts of Plasma based on their description, but are harder to map to QStyle.

Plasma abstracts icon sizes through another layer. PlasmaFramework -> units.cpp we could hardcode equivalent in there.

Though AFAIK we don't actually use the panel size hint

Panel icon size is used as *maximum* size a widget may grow in the panel so they don't get insanely huge when you have a thick panel.

I think one important part for non-unices is the ability that iconthemes will auto-load qrc files with icons for e.g. Windows.
(I intend to extend that to support the loading of multiple themes, as at the moment this falls flat for supporting e.g. normal + dark mode)

Looking at internal dependencies, it seems the only reason KIconThemes is a tier 3+ framework with a dependency on KAuth etc is KColorScheme, used in two place. With T11587 KIconThemes would therefore automatically become a tier 2 framework.

I think one important part for non-unices is the ability that iconthemes will auto-load qrc files with icons for e.g. Windows.

Please can you expand on this.

How do the non-Plasma end up using KIconLoader? On windows or OSX you'll use their icon loader implementation. Unless you're explicitly forcing it somewhere.

cullmann added a comment.EditedOct 28 2019, 6:02 PM

You don't need to use the loader, but KIconThemes will setup QIcon themeName in a usable way.

See the code at top of

kicontheme.cpp

Without that, each and every application that e.g. wants to use breeze icons (aka all KDE applications) needs to do that stuff manually.

I have here some patch that moves this icon loader "hacks" to breeze-icons themself.

https://phabricator.kde.org/D25119

One thing I learned in https://phabricator.kde.org/D25119 is that KIconThemes provides the means to adjust the SVGs out of e.g. Breeze to match the palette (e.g. for Dark Mode).

This would be nice to have.

But at the moment this only works for non-resource icons on Linux with the platform integration plasma-integration.

ndavis added a subscriber: ndavis.Nov 10 2019, 8:19 AM

The only place where we use KIconThemes in KDE Connect is using KIconloader to get the file path from an icon name.
https://invent.kde.org/kde/kdeconnect-kde/blob/master/plugins/sendnotifications/notificationslistener.cpp#L165

  • Dialog -> PM_MessageBoxIconSize

KStyle currently though does not map those, instead it does:

int KStyle::pixelMetric(PixelMetric metric, const QStyleOption *option, const QWidget *widget) const
	{
	    switch (metric) {
	    case PM_SmallIconSize:
	    case PM_ButtonIconSize:
	        return KIconLoader::global()->currentSize(KIconLoader::Small);
	
	    case PM_ToolBarIconSize:
	        return KIconLoader::global()->currentSize(KIconLoader::Toolbar);
	
	    case PM_LargeIconSize:
	        return KIconLoader::global()->currentSize(KIconLoader::Dialog);
	
	    case PM_MessageBoxIconSize:
	        // TODO return KIconLoader::global()->currentSize(KIconLoader::MessageBox);
	        return KIconLoader::SizeHuge;
	    default:
	        break;
	    }
	
	    return QCommonStyle::pixelMetric(metric, option, widget);
	}

see https://phabricator.kde.org/source/frameworkintegration/browse/master/src/kstyle/kstyle.cpp$439

And given that KIconThemes API dox defines "Dialog" to be for
"Icons for use in dialog titles, page lists, etc"
this indeed might not be a match then, as this seems more about some header-inlined icon, not the "picture" symbol icon in the left dialog column. Also confirmed by

Looking back, KMessageBox also used KIconLoader::SizeHuge (64, in older version SizeLarge 48) to create the pixmap:
https://phabricator.kde.org/source/kdelibs/browse/KDE%252F4.14/kdeui/dialogs/kmessagebox.cpp$102

Also is the default value for "Dialog" "32" (https://phabricator.kde.org/source/kiconthemes/browse/master/src/kicontheme.cpp$374), which is not the size one expects for the message box side picture icon.

So, KIconLoader::Dialog -> PM_LargeIconSize like done by KStyle seems the better mapping.

You are right (and that matches how we have been porting this when moving away from IconSize() so far). It's actually rather KIconLoader::Desktop that maps to PM_MessageBoxIconSize, when used in application code.

From KDevelop I can report those usages of KIconThemes:

  • KIconLoader: used to get path to icon linked as embedded images in generated html for richtext display -> perhaps can be ported to QTextDocument resources, not investigated yet
  • KIconEffect: used to tint icons with KColorScheme::NegativeText to match color of removed lines in patch review -> needs investigation if proper UI and whether separate icons are better
  • KIconButton: used as custom icon picker for added QCH documentation files -> seems valid use, something like KIconButton also wanted in future KF

Semi related: I'm trying to standardise the way SVG icons are colourised: https://gitlab.gnome.org/GNOME/gtk/issues/1762
If this does become a standard we can /maybe/ move it into Qt?

One thing I thought about today. KIconLoader/Theme has MatchExact. Using this I can for example get an icon for "start-here-kde-plasma" without falling back to "start-here" which is a foot in Adwaita. I think this is not possible with QIcon

What's the use-case of doing that?

What's the use-case of doing that?

I don't have a particular use case right now I thought of it during thinking about https://bugs.kde.org/show_bug.cgi?id=415173 . We surely don't want to display a gnome icon in systemsettings. But that can calso be solved by shipping the icon in the repo.