KF5::BreezeIcons shared library to have all breeze/breeze-dark icons and default to breeze icons set
AbandonedPublic

Authored by cullmann on Nov 2 2019, 4:09 PM.

Details

Summary

At the moment, one big problem for non-unices is the deployment of icons
Close to all things in KDE stuff relies on some breeze like icon theme
At the moment, we help out with some hack in KIconThemes to load a binary resource
This makes it impossible to e.g. support dark mode properly, as only one theme is around

This patch provides an optional dynamic library containing both breeze icon sets (normal
and dark) and sets the fallback theme to breeze on load

Applications wanting to have breeze icons available, can just link
with this and be done

Diff Detail

Repository
R266 Breeze Icons
Branch
breeze-icon-shared-lib
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18445
Build 18463: arc lint + arc unit
cullmann created this revision.Nov 2 2019, 4:09 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 2 2019, 4:09 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
cullmann requested review of this revision.Nov 2 2019, 4:09 PM

Some reviewers.

I am not sure this is the "perfect" solution, but it will make deploying applications a lot easier given just linking against this library provides the icons most of them will need.

And it will allow later to be able to switch to the "dark" variant for the dark mode most platforms provide today.

cullmann added a subscriber: rempt.

Perhaps @rempt has some input on this, for the cross-platform aspects.

At the moment this patch will only create the shared lib if we have binary resources configured.
One could always create the lib and only compile in the resources if wanted.
That would allow e.g. applications like Kate to always link against this on all platforms.

broulik added a subscriber: broulik.Nov 2 2019, 6:00 PM

Can you expand on that dark theme issue? We just colorize the same SVGs as needed.

At the moment, the hack in kiconthemes only allows to load one theme.
This means you have just "breeze".
If you e.g. want to switch to "breeze-dark" this fails as the icons will not be found.

ndavis added a comment.EditedNov 2 2019, 7:50 PM

I don't think my review is really necessary since I lack the technical expertise to know what the right solution is, but +1 to light and dark Breeze icon support on other platforms.

This patch by itself will not make the dark mode work, but e.g. Kate will be able to use it as the color theme chooser will set breeze-dark as theme and the icons will be "there".
Beside this, it avoids loading 20MB of data, as before the resource file got loaded into memory on Windows, with this the dll is just mapped.

rempt added a comment.Nov 3 2019, 2:20 PM

It sounds like it would be something very useful and easy to use. Right now, we package all icons Krita uses in rc files that are compiled in as well. Also, our icon loading has kind of evolved and we've got a bit of code for switching between dark and light icons ourselves.

This patch will still need some work to make dark mode available.
Kate will be able to switch to it via the color theme switcher, as that will just switch over to breeze-dark.

Could you elaborate on

"Also, our icon loading has kind of evolved and we've got a bit of code for switching between dark and light icons ourselves."

If you have some hints where to look in the Krita code for ideas how that is done, I would be grateful ;=)

If you e.g. want to switch to "breeze-dark" this fails as the icons will not be found.

But why would you need this anyway? At least with KIconEngine we colorize the icons dynamically. I don't have to switch to breeze-dark when I use a dark color scheme on Plasma at least.

If you e.g. want to switch to "breeze-dark" this fails as the icons will not be found.

But why would you need this anyway? At least with KIconEngine we colorize the icons dynamically. I don't have to switch to breeze-dark when I use a dark color scheme on Plasma at least.

Hmm, where does that happen?

And does that work for icons you get via QIcon::fromTheme?

For me, on Windows, just switching to dark mode via the color theme chooser nicely changes the palette colors but my icons stay "normal".

Hmm, where does that happen?

https://cgit.kde.org/kiconthemes.git/tree/src/kiconloader.cpp#n888

And does that work for icons you get via QIcon::fromTheme?

Yes. As long as you pass a QIcon to the button or widget instead of an already rendered QPixmap.

On Windows you probably don't use KIconEngine but whatever Qt provides by itself.
There's some discussion on a fdo symbolic icon colorization (@davidedmundson should know more), which would be useful having in Qt then?

Hmm, where does that happen?

https://cgit.kde.org/kiconthemes.git/tree/src/kiconloader.cpp#n888

And does that work for icons you get via QIcon::fromTheme?

Yes. As long as you pass a QIcon to the button or widget instead of an already rendered QPixmap.

On Windows you probably don't use KIconEngine but whatever Qt provides by itself.
There's some discussion on a fdo symbolic icon colorization (@davidedmundson should know more), which would be useful having in Qt then?

Hmm, all KIconThemes related stuff should be in the application installer.
Need to investigate this more.

Ok, I traced this now a bit.
The KIconLoaderPrivate::createIconImage processing seems already not to work for resource based themes on Linux.

To try, e.g. use Kate, add the binary resource icon theme to the right location

cp /local/cullmann/kde/usr/share/icons/breeze/breeze-icons.rcc ~/.local/share/kate/icontheme.rcc

Switch in Kate to dark mode with Settings -> Color Theme -> Breeze Dark.

Without the rcc file, this works.
With the rcc file, the icons stay dark and are more or less invisible.

As soon as we use the rcc file for the icons, we set an icon theme name.

If you have an icon theme name set, QIcon::fromTheme will do:

bool hasUserTheme = QIconLoader::instance()->hasUserTheme();
QIconEngine * const engine = (platformTheme && !hasUserTheme) ? platformTheme->createIconEngine(name)
                                           : new QIconLoaderEngine(name);

This IMHO means no longer KIconLoader will be used at all.
(On Windows it will never be used, as the platform theme doesn't use it there anyways I assume)

But yes, packaging Breeze Dark icons in addition will not help to fix this :/

cullmann updated this revision to Diff 74839.Feb 1 2020, 4:58 PM

set normal or dark icons based on current palette

@cullmann is this still planned to be released?

That depends.
If somebody else would test this and we can agree that this is a nicer way to hard-code breeze icons compared to having that code in kiconthemes it would make sense.

wrobelda added a comment.EditedJun 9 2020, 2:16 AM

@cullmann I will do my best to try and test it with KMyMoney on macOS and maybe Windows this coming weekend. I'll keep you posted.

sars added a subscriber: sars.Jun 14 2020, 9:18 AM
aboyer added a subscriber: aboyer.Oct 30 2020, 9:47 PM

I've tested this patch since I'd really like to use Kate with the dark theme on Windows. I built Kate on Windows using Craft, added KF5::BreezeIcons as a dependency for the kate-bin target, and set the QIcon theme name in the KateColorSchemeChooser class. This did not work.

The first problem I ran into was a CMake issue when adding KF5::BreezeIcons as a dependency. It turns out that KF5BreezeIcons.lib is not created since the new KF5BreezeIcons shared library does not export any symbols. Exporting a dummy function with __declspec(dllexport) fixed the problem and Kate compiled successfully.

The second problem was that no icons were rendered regardless of which theme was selected. This was caused by the fact that Kate did not actually link to KF5BreezeIcons since no symbol from that library was used. Calling the dummy function added earlier from main() fixed the linking problem. With this, Kate loaded KF5BreezeIcons.dll and the correct icons were used when switching to the Breeze dark theme. Success!

I did my testing with MSVC and I'm wondering if the patch was originally tested with MinGW. I can't recall if MinGW has a different behavior when it comes to exporting symbols from a shared library and linking to said shared library.

Applications wanting to have breeze icons available, can just link with this and be done

It might not be quite so simple as just linking to a library. Obviously, the hacks described above to make this patch work are not acceptable. However, they could be modified slightly. For example, the call to QIcon::setThemeName() could be made in KF5BreezeIcons.dll instead of in Kate. We could export a class with a static function that sets the theme name based on the palette lightness, just like it is now done for the fallback theme. Kate would then call this static function in KateColorSchemeChooser. It's a bit less elegant but would address the issues mentioned above. Unless someone knows how to force the linking to a shared library even if no symbols are exported and/or used. Thoughts?

I think exporting some namespaced function or such that will do the setting of the theme and calling it would be ok for me.
This is anyways an "opt-in" approach people must decide to use.
Hannah, what do you think?

I love it.
Could we add a option to the exported target, so that you can basically always link that library with out an effect, until a cmake flag is set?
This way we can bring it to all applications and enable it from craft.
-DENABLE_BREEZE_BINARY_THEME=ON (or what ever)

src/main.cpp
33

I prefer lightnessF() < 0.5 here but that's a matter of taste ;D

From my limited understanding of the theming system for KDE applications on Windows, the application needs to expose a drop down menu with a list of themes so that a user can switch to their desired theme. Even if we linked a disabled version of the library and enabled it in Craft, it wouldn't do anything since the application needs code to display a list of themes and a slot to change the theme. Given this requirement, I see this as a compile time option where an application must opt-in, link to KF5BreezeIcons.dll and add the code displaying the theme list. This is based on my understanding of how Kate handles themes. I have not looked into any other application. There might be a more generic way of changing/configuring themes for KDE applications on Windows that I am not aware of.

That being said, putting the icon themes behind a CMake feature flag and linking all applications to KF5BreezeIcons.dll will not work for another reason. When disabled, the KF5BreezeIcons.dll library would contain no exported symbols, the KF5BreezeIcons.lib file would not get generated and the build would break for all applications. This is precisely the problem I am trying to fix with my proposal.

If there are no other comments, I will go forward with my proposal when I get a chance. I need to do some testing and will update the patch here.

Yea my idea didn't make much sense.
Reading your comment I realized that we should probably implement a theme chooser in a kf5 and provide the themes as plugins, that way we could also provide other themes, not just breeze.

And change those during runtime....

+1 to adding the breeze icons support for other OSes. (Thanks @nicolasfella for linking me to this diff).

I have hit the same problem with the KDE Connect Windows port. Thanks a lot for working on this issue!

Hmm, I don't think you need a theme chooser, we just need to watch for the palette change signal and then trigger again the theme name setting based on the background palette.
That would be enough to detect bright/dark mode.

Hmm, I don't think you need a theme chooser, we just need to watch for the palette change signal and then trigger again the theme name setting based on the background palette.
That would be enough to detect bright/dark mode.

But with a theme chooser and a plugin approach we can handle multiple different themes which can easily be added.
At the moment we would need to link too breeze, to add support for oxygen we would need to link to oxygen 😅

I need to finalize this, people start to complain more and more about this missing on Windows for Kate/etc... ;(

cullmann abandoned this revision.Jan 26 2021, 7:28 PM

I have now a working version that handles palette changes, too.

https://invent.kde.org/frameworks/breeze-icons/-/merge_requests/79

+ usage in Kate

https://invent.kde.org/utilities/kate/-/merge_requests/204

We should discuss further stuff there.