[Cuttlefish] Compare an icon in different themes
ClosedPublic

Authored by davidre on Sep 30 2019, 10:25 AM.

Details

Summary

The icon in different installed themes is shown in an overlay sheet.

Test Plan

Diff Detail

Repository
R118 Plasma SDK
Branch
comparison (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17174
Build 17192: arc lint + arc unit
davidre created this revision.Sep 30 2019, 10:25 AM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 30 2019, 10:25 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidre requested review of this revision.Sep 30 2019, 10:25 AM
davidre edited the test plan for this revision. (Show Details)Sep 30 2019, 10:26 AM
davidre added reviewers: Plasma, VDG.
filipf added a subscriber: filipf.Sep 30 2019, 10:32 AM

LGTM. Can the overlay be spawned in the vertical center of the window as well or is that defined in Kirigami itself? Also what happens when there are a lot of themes, does the overlay remain the same size and just get a scrollbar?

LGTM. Can the overlay be spawned in the vertical center of the window as well or is that defined in Kirigami itself? Also what happens when there are a lot of themes, does the overlay remain the same size and just get a scrollbar?

I didn't look into sizing positioning the overlay yet. But a scrollbar appears if the content doesn't fit into the available size.

davidre updated this revision to Diff 67070.Sep 30 2019, 2:08 PM

Show a symbol when a theme doesn't have the icon

davidre edited the test plan for this revision. (Show Details)Sep 30 2019, 2:09 PM
ngraham accepted this revision.Sep 30 2019, 3:45 PM
ngraham added a subscriber: ngraham.

Very nice!

This revision is now accepted and ready to land.Sep 30 2019, 3:45 PM

Thanks for accepting this so quickly! Does anyone have any comments on the code? I would like to improve my qml and any suggestion/best practice is welcome.

The QML is generally fine! Since you specifically requested it, I added nitpicky minor comments below. :) I don't consider them blockers, but feel free to take a look anyway!

cuttlefish/package/contents/ui/Comparison.qml
1

2.4

3

1.3

65

indentation

69

height: width

70

Rather than creating a complicated additional item for the "no icon" case, I would just do source: modelData.iconPath ? modelData.iconPath : "paint-none". If you don't like the paint-none icon, we can adjust it.

davidre updated this revision to Diff 67118.Oct 1 2019, 11:39 AM
davidre marked 4 inline comments as done.

nitpicks

davidre added inline comments.Oct 1 2019, 11:39 AM
cuttlefish/package/contents/ui/Comparison.qml
70

That's what I did first acutally but I didn't like that for two reasons. First it would scale with the other icons and when you are looking at paint-none you couldn't distinguish it anymore.

ngraham added inline comments.Oct 1 2019, 11:49 AM
cuttlefish/package/contents/ui/Comparison.qml
70

In that case what about just setting color: Kirigami.Theme.disabledTextColor when there's no icon? That should serve to distinguish the actual paint-none icon from the use of that icon to denote "no icon available"

I don't think having it scale is a huge deal.

davidre updated this revision to Diff 67120.Oct 1 2019, 12:02 PM
davidre marked 3 inline comments as done.

Use paint-none directly

ngraham added inline comments.Oct 1 2019, 12:09 PM
cuttlefish/package/contents/ui/Comparison.qml
67

Now you don't need this wrapper anymore; just use Kirigami.Icon directly and set Layout.preferredWidth and Layout.preferredHeight on it.

davidre added inline comments.Oct 1 2019, 12:17 PM
cuttlefish/package/contents/ui/Comparison.qml
67

I use it for centering the icon in the middle of the 128x128 free space for the icon. Qt.AlignCenter is not enough for that. In the first version of the diff I tried make up for that with setting of top and bottom margins depending on the text height and icon size but that was to complicated and also didn't produce the correct result I think.

ngraham added inline comments.Oct 1 2019, 12:19 PM
cuttlefish/package/contents/ui/Comparison.qml
67

There is no Qt.AlignCenter (though there should be IMO); I think you need to do Qt.AlignHCenter | Qt.AligtVCenter

davidre added inline comments.Oct 1 2019, 12:30 PM
cuttlefish/package/contents/ui/Comparison.qml
67
ngraham accepted this revision.Oct 1 2019, 12:41 PM

You're right! Shipit.

This revision was automatically updated to reflect the committed changes.