[kcm] Don't transform button to show rotated icon
ClosedPublic

Authored by davidedmundson on Oct 8 2019, 11:02 AM.

Details

Summary

Rotating a button leads to tooltips being broken, it will lead to
shadows being broken on a theme that uses shadows and is generally not
great.

This patch sets the contentItem explicitly and rotates that

BUG: 412092

Diff Detail

Repository
R104 KScreen
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidedmundson created this revision.Oct 8 2019, 11:02 AM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 8 2019, 11:02 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Oct 8 2019, 11:02 AM

An alternative approach could have been to set a custom content item and rotate only that.
But I actually quite like the the "arrow" in the new icons.

(Also, why is this a switch in Component.onCompleted and not just bindings...)

ngraham added a subscriber: ngraham.Oct 8 2019, 1:34 PM

Hmm, the "no rotation" and "upside down" icons aren't accurate anymore (especially "no rotation").

mart added a subscriber: mart.Oct 16 2019, 9:22 AM

Hmm, the "no rotation" and "upside down" icons aren't accurate anymore (especially "no rotation").

I think it's the right approach... icons should be fixed?

From usability perspective I don't like the arrow. It is not as easy to grasp what will be the outcome since you have a current state and a transformation of that applied to symbolize a different state (the rotated one). Directly showing the rotated state, i.e. the outcome, like the current icons do is more easy to grasp imo.

That being said the current implementation is obviously broken and using separate icons for that could be a solution. Maybe using @broulik's custom content approach would be a good solution for that as well?

I agree with roman that the rotated icons are preferable from a usability perspective. Maybe we need new icons then? VDG

I would still suggest overriding the contentItem of the Button and put a rotated Kirigami.Icon in there without actually transforming the entire button.

davidedmundson edited the summary of this revision. (Show Details)

update

If Kai says this is the right approach, it probably is

ngraham accepted this revision.Oct 16 2019, 7:33 PM
ngraham added inline comments.
kcm/package/contents/ui/RotationButton.qml
53–54

Kirigami.units.smallSpacing instead of hardcoded 2

This revision is now accepted and ready to land.Oct 16 2019, 7:33 PM
romangg accepted this revision.Oct 16 2019, 8:18 PM

5.17

broulik added inline comments.Oct 16 2019, 8:18 PM
kcm/package/contents/ui/RotationButton.qml
41

why not just use rotation property?

Use units
Use rotation directly

This revision was automatically updated to reflect the committed changes.