allow to set a custom palette instead of colorSets
ClosedPublic

Authored by mart on Sep 29 2017, 6:01 PM.

Details

Summary

remove the public dependency to KConfigWidgets as
this is breaking existing apps, but instead let
applications set their own QPalette, so doesn't add
dependencies and is slightly more flexible

Test Plan

old clients work, no new dependencies,
the kirigami custom palette icons is adaped and works

Diff Detail

Repository
R302 KIconThemes
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mart created this revision.Sep 29 2017, 6:01 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 29 2017, 6:01 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
cfeck added a subscriber: cfeck.EditedSep 29 2017, 10:53 PM

remove the public dependency to KConfigWidgets as this is breaking existing apps

Does it work if you use a forward declaration instead of an include? As far as I know, both "Type f()" and "void f(Type *p)" do not need the actual class/structure declaration, because both are passed as pointers.

Uh, forget what I wrote. You wanted to pass only the colorSet enum, not the complete color scheme.

+1 to this.

BUG: 385175

broulik added a subscriber: broulik.Oct 1 2017, 9:13 AM
broulik added inline comments.
src/kiconloader.cpp
870

You don't seem to be following qApp->palette() anymore if no custom one is set?

mart added inline comments.Oct 2 2017, 9:12 AM
src/kiconloader.cpp
870

it uses an empty QPalette(), which accordning to the docs, it should fall back to the application's palette

davidedmundson accepted this revision.Oct 2 2017, 10:27 AM
davidedmundson added a subscriber: davidedmundson.

1 bug needs fixing.

src/kiconloader.cpp
142

pal.window() should be in here too so we cover every palette var used in
processSvg

src/kiconloader.h
465

@since

This revision is now accepted and ready to land.Oct 2 2017, 10:27 AM
mart marked 2 inline comments as done.Oct 2 2017, 10:45 AM
This revision was automatically updated to reflect the committed changes.

Just to let you know, these last 2 commits to KIconThemes break the icon coloring in existing apps like Kdenlive and KDevelop.
Previously, in a KXmlGuiWindow, doing:

QPalette plt = KColorScheme::createApplicationPalette(config);
setPalette(plt);
qApp->setPalette(palette());

Made the icon theme adapt to the palette. The recent commits break this and it is now required to add :

KIconLoader::global()->setCustomPalette(plt);

To make icon color theming work in existing apps that allow color theme switch.
Regression can be easily tested with KDevelop or Kdenlive using the "Color theme" menu from Settings

mart added a comment.Oct 6 2017, 10:58 AM

KIconLoader::global()->setCustomPalette(plt);

To make icon color theming work in existing apps that allow color theme switch.
Regression can be easily tested with KDevelop or Kdenlive using the "Color theme" menu from Settings

please test with kiconthemes after https://commits.kde.org/kiconthemes/d15a221128bc0727f8dac96a28472f8f3a455160
it should sove the issue

Thanks I can confirm it is fixed with last commit.