Color icons in titlebar if possible
ClosedPublic

Authored by davidre on Mar 27 2020, 9:31 AM.

Details

Summary

Some icons can be recolored. However if the titlebar color is different
compared to the normal color scheme, this can lead to bad contrast.

Test Plan

Before:


After:

Diff Detail

Repository
R31 Breeze
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidre created this revision.Mar 27 2020, 9:31 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 27 2020, 9:31 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidre requested review of this revision.Mar 27 2020, 9:31 AM
davidre edited the test plan for this revision. (Show Details)Mar 27 2020, 9:36 AM
davidre added reviewers: Plasma, VDG.
davidre edited the test plan for this revision. (Show Details)
davidre updated this revision to Diff 78639.Mar 27 2020, 1:03 PM

Use decoration()->fontColor()

ngraham accepted this revision.Mar 27 2020, 2:29 PM
ngraham added a subscriber: ngraham.

Quite nice, quite nice.

This revision is now accepted and ready to land.Mar 27 2020, 2:29 PM
kossebau added inline comments.
kdecoration/breezebutton.cpp
148

Is there a chance the cast could fail?
Otherwise also use a static_cast, given you do not check the result, is faster (or is it a cross-cast?).

davidre added inline comments.Mar 27 2020, 3:11 PM
kdecoration/breezebutton.cpp
148

I don't think it can fail. decoration() is a QPointer (https://api.kde.org/kdecoration/html/classKDecoration2_1_1DecorationButton.html) do you mean that with cross-cast?

With cross-casting I meant the type of cast where the types involved are not in same inheritance branch

kdecoration/breezebutton.cpp
147–160

Assumes there is no other custom palette set before we set ours here. Does that assumption hold? Or could KWin & Co do mess with that palette as well?

148

Looking at the rest of the code, it always tests for decoration() resolving to an existing Decoration pointer.
While I would have expected it might be ensured that decoration always is of type Breeze::Decoration, but Button::create(...) takes a KDecoration2::Decoration, so at least by code in this class this is not ensured.

Now, Button::paint(...) already has half the check ealier, by the

if (!decoration()) return;

But this does not ensure we have a Breeze::Decoration type.

And QPointer only ensures that the pointer is updated if the object is deleted elsewhere. It still can be nullptr, and possibly this is done here as the decoration object is not owned by us, so something might delete it behind the button's back.

So IMHO this logic might need another check by someone who can tell if the right type can be still assumed (which ideally then should be reflected in the API, so it is clear no other type could be present.

davidre added inline comments.Mar 27 2020, 4:27 PM
kdecoration/breezebutton.cpp
147–160

I will look into it

davidre updated this revision to Diff 78859.Mar 30 2020, 9:01 AM

Check if cast is successfull

davidre added inline comments.Mar 30 2020, 9:05 AM
kdecoration/breezebutton.cpp
153

KWin doesn't change the palette, it uses to global one to draw the icons for example when you right click on a window titlebar.

davidre updated this revision to Diff 78860.Mar 30 2020, 9:08 AM

forgot else

davidre updated this revision to Diff 79068.Apr 1 2020, 5:05 PM

Be more careful when resetting palette

This revision was automatically updated to reflect the committed changes.