Differential D12595

Fix dark HUD icons when using light system theme
ClosedPublic
Actions

Authored by huoni on Apr 29 2018, 6:51 AM.

Details

Reviewers
rkflx
Group Reviewers
Gwenview
Commits
R260:3f540441b394: Fix dark HUD icons when using light system theme
Summary

The HUD widgets are custom and always dark themed. When using a light
system theme like Breeze, the icons were rendered with a dark color,
which was bad because it didn't contrast with the dark HUD. This was
not an issue in full screen mode, because we force the whole of
Gwenview to use a dark theme, and therefore the icons rendered a light
color.

This affects:

  1. Compare / Light table mode
  2. Video controls popup
  3. The "You are now viewing the new document" message after Save As

(Other places the HUD is used do not use icons)

So, when setting an icon using HudButton::setIcon, this patch now
re-loads the icon using the KIconLoader global instance, setting a
custom palette with a white foreground color, so the loader loads a
light icon. KIconLoader's palette is then reset as to not interfere
with other icons.

BUG: 380257
FIXED-IN: 18.08.0

Note: we bump KF5 version to 5.39 for KIconLoader::setCustomPalette
and KIconLoader::resetPalette.

Compare/Lighttable Before:

Compare/Lighttable After:

Video Controls Before:

Video Controls After:

Save As message Before:

Save As message After:

Test Plan

The following have HUD widgets with icons:

  1. Compare / Light table mode
  2. Video controls popup
  3. The "You are now viewing the new document" message after Save As

Note for 3), the HUD only appears for a split second, due to a bug. See
D9342.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
huoni requested review of this revision.Apr 29 2018, 6:51 AM
huoni created this revision.
huoni edited the summary of this revision. (Show Details)Apr 29 2018, 6:55 AM
huoni added a project: Gwenview.
huoni edited the summary of this revision. (Show Details)
huoni added inline comments.Apr 29 2018, 7:05 AM
lib/hud/hudbutton.cpp
123–125

I'm pretty confident changing the palette for the global KIconLoader instance just here won't affect other parts of the app. Alternatively we can create our own instance for this use, but that might be expensive as it "load[s] the current icon theme and all its base themes".

124

KIconLoader::loadIconSet returns a QIcon, but the compiler said it's deprecated, therefore I changed mIcon to a QPixmap instead.

huoni updated this revision to Diff 33279.Apr 29 2018, 7:09 AM
  • Change back to QIcon
huoni added inline comments.Apr 29 2018, 7:10 AM
lib/hud/hudbutton.cpp
124

Spoke too soon, realised this caused bad scaling, so I switched back.

huoni edited the summary of this revision. (Show Details)Apr 29 2018, 7:11 AM
muhlenpfordt added a subscriber: muhlenpfordt.Apr 29 2018, 10:10 AM

I can't compile this using Kubuntu 17.10 - seems KIconLoader::setCustomPalette() and KIconLoader::resetPalette() does not exist here ("Since 5.38"). But this shouldn't be a problem for KDE 18.08.

lib/hud/hudbutton.cpp
124

The API doc for KIconLoader::loadIconSet() says it should be replaced by QIcon::fromTheme().

rkflx added a comment.Apr 29 2018, 10:16 AM
In D12595#255516, @muhlenpfordt wrote:

I can't compile this using Kubuntu 17.10 - seems KIconLoader::setCustomPalette() and KIconLoader::resetPalette() does not exist here ("Since 5.38").

Ah, that's why we might have had trouble trying to fix it back then.

But this shouldn't be a problem for KDE 18.08.

…in case @huoni does not forget to bump the version.

huoni added a comment.Apr 29 2018, 10:29 PM
In D12595#255519, @rkflx wrote:

But this shouldn't be a problem for KDE 18.08.

…in case @huoni does not forget to bump the version.

Sorry not sure what you mean.

Is using a deprecated function unacceptable?

lib/hud/hudbutton.cpp
124

If only QIcon::fromTheme supported custom palettes...
I trid using it while KIconLoader had the custom theme, but that didn't work.

muhlenpfordt added a comment.Apr 30 2018, 12:53 PM
In D12595#255741, @huoni wrote:
In D12595#255519, @rkflx wrote:

But this shouldn't be a problem for KDE 18.08.

…in case @huoni does not forget to bump the version.

Sorry not sure what you mean.

I meant the library version which include the functions setCustomPalette() and resetPalette(). I think you have to set ... KF5 5.3x REQUIRED... in the CMakeLists.txt.
But I'm not sure if the docu is correct here. I just noticed I have library v5.38 and both functions appear only after checking out tag v5.39.0.

rkflx added a comment.Apr 30 2018, 10:46 PM
In D12595#255741, @huoni wrote:
In D12595#255519, @rkflx wrote:

But this shouldn't be a problem for KDE 18.08.

…in case @huoni does not forget to bump the version.

Sorry not sure what you mean.

Shouldn't have used too much jargon, sorry. "Bumping" normally means increasing the version number of a variable. In this case "version" referred to the version of KDE Frameworks Gwenview requires. If you want to use API's that are only available since a certain version, you'd have to required at least that version (but mind the dependency freeze, after which you cannot change versions of those anymore).

Peter already pointed to the correct file, but perhaps use git blame on the @since of that function to check whether it is correct indeed.

Is using a deprecated function unacceptable?

In general it's best (but not totally forbidden) to avoid deprecated functions, because eventually they might be removed, creating more porting work later on.

From a cursory look I guess you patch is fine (except for the missing version increase). I'll try to look at it tomorrow in detail.

huoni added a comment.May 1 2018, 3:56 AM

Seems Peter is right, both commits relating to these functions have v5.39.0-rc1 as their earliest tag.
Should I submit a patch that updates the @since ... comment?

I looked at the source code for loadIcon and loadIconSet and the latter simply calls the former for three different states. So I've changed back to loadIcon and fixed the scaling issue by not scaling at all, since we specify KIconLoader::Small when re-loading the icon (which is the same size as it was scaling to).

If this method of forcing light icons is acceptable, we could do the same for the thumbnail hover icons if we wanted to force a dark theme for those.
E.g. an option would be to switch all HUD to standard widgets, apply the full screen palette, and force light icons.

In an ideal world, icons would change based on their parent widget's palette, rather than the entire applications palette...

huoni updated this revision to Diff 33371.May 1 2018, 3:58 AM
  • Change back to loadIcon and don't scale
  • Bump KF5 version to 5.39 to get setCustomPalette and resetPalette
huoni edited the summary of this revision. (Show Details)May 1 2018, 4:00 AM
huoni added a comment.May 1 2018, 4:26 AM

In fact, if this method of forcing light icons (and the corresponding KF5 version bump) is all good, I would prefer not landing this patch but instead convert the HUD to standard widgets (keeping the general HUD framework).
E.g. I would:

  1. Use standard widgets where possible, in order to adapt to the current system widget style
  2. Theme it using the full screen palette so it's consistently dark
  3. Force light icons

We could optionally do 2 & 3 for the thumbnail hover icons as well.

rkflx added a comment.May 1 2018, 10:59 PM
In D12595#256267, @huoni wrote:

Seems Peter is right, both commits relating to these functions have v5.39.0-rc1 as their earliest tag.
Should I submit a patch that updates the @since ... comment?

Sure, fixing bugs in KDE's Frameworks is always appreciated ;)

In an ideal world, icons would change based on their parent widget's palette, rather than the entire applications palette...

Are you saying this is only a matter of finding the place where application should be changed to parent? Or perhaps just ask Marco what he thinks about that?

I would prefer not landing this patch but instead convert the HUD

Why not both?

huoni added a comment.May 1 2018, 11:12 PM
In D12595#256844, @rkflx wrote:
In D12595#256267, @huoni wrote:

Seems Peter is right, both commits relating to these functions have v5.39.0-rc1 as their earliest tag.
Should I submit a patch that updates the @since ... comment?

Sure, fixing bugs in KDE's Frameworks is always appreciated ;)

In an ideal world, icons would change based on their parent widget's palette, rather than the entire applications palette...

Are you saying this is only a matter of finding the place where application should be changed to parent? Or perhaps just ask Marco what he thinks about that?

I don't think it's that simple, from what I've seen so far. Since changing an icon from dark to light isn't just about changing a color, but requires all the logic to determine which theme to use, potentially reading from disk, etc.

I would prefer not landing this patch but instead convert the HUD

Why not both?

None of this code would be kept for the HUD revamp. HudButton would be removed, and the KIconLoader code would be put somewhere more generic.

But I can't say when the revamp will be done, so I guess there's no harm landing this now.

rkflx added a comment.May 1 2018, 11:18 PM

One more thought: I case users already have a version >=5.39, we could already enable the fix for them on stable (with #ifdefs around the new API calls). OTOH, it's probably not worth the time figuring out how to do that with CMake…

(And sorry the review is not done yet, there's just too much going on currently…)

huoni added a comment.May 1 2018, 11:36 PM
In D12595#256848, @rkflx wrote:

One more thought: I case users already have a version >=5.39, we could already enable the fix for them on stable (with #ifdefs around the new API calls). OTOH, it's probably not worth the time figuring out how to do that with CMake…

I'd agree it's not worth it.

(And sorry the review is not done yet, there's just too much going on currently…)

No worries at all, 18.08 is a while off :)

rkflx requested changes to this revision.EditedMay 2 2018, 1:11 PM

Sorry to say, but your patch breaks on HiDPI, which is best seen on the video control icons.

Example for QT_SCALE_FACTOR=2.3:

Before:

After:

I'm sure you'll be able to fix it, though ;)

Edit: Note that in some other Diffs porting from KIconLoader to QIcon::fromTheme often solved HiDPI issues. OTOH, in some places KIconLoader is also able to properly work in HiDPI mode by itself. Hm…

This revision now requires changes to proceed.May 2 2018, 1:11 PM
huoni updated this revision to Diff 33541.May 3 2018, 3:08 AM
  • Fix HiDPI by changing how we generate the icon
huoni added a comment.May 3 2018, 3:17 AM

I'm understanding this a bit better now. QIcon itself doesn't contain light/dark (palette) information. Only when you call QIcon::pixmap does it use the palette to generate either a light or dark icon (in this scenario). The palette used is the same as the QApplication palette, unless a custom palette is set with KIconLoader::setCustomPalette().

rkflx accepted this revision.May 3 2018, 8:58 PM

That was a fast fix. Awesome ;)

LGTM, and good idea with the caching.

This revision is now accepted and ready to land.May 3 2018, 8:58 PM
Closed by commit R260:3f540441b394: Fix dark HUD icons when using light system theme (authored by huoni). · Explain WhyMay 3 2018, 11:39 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
MCMakeLists.txt (2 lines)
Mlib/hud/hudbutton.cpp (25 lines)

Diff 33593

View Options

CMakeLists.txt

Loading...
View Options

lib/hud/hudbutton.cpp

Loading...