[applets/taskmanager] Clean up Tooltip code
ClosedPublic

Authored by ngraham on Feb 6 2020, 5:01 AM.

Details

Summary

The ToolTip code was kind of messy. This patch adjusts it to use modern Layouts,
fixes some binding loops, reduces redundancy, adds comments, removes a lot of
unnecessary code, and ports almost everything to PlasmaComponents3. We still
need PC2 for Highlight so it can't be removed entirely.

There is only one unavoidable visual change, which was made in this patch
because it is consistent with the latest VDG mockup (T12640): the blurred
album art background no longer goes under the player controls bar. This
simplifies the code substantially, and as the mockup shows, it just looks better
anyway (IMO).

This is not intended to fully implement the mockup in T12640.

Test Plan

Everything looks the same, except for this:

Diff Detail

Repository
R119 Plasma Desktop
Branch
clean-up-tooltip-code (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22111
Build 22129: arc lint + arc unit
ngraham created this revision.Feb 6 2020, 5:01 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 6 2020, 5:01 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Feb 6 2020, 5:01 AM
ngraham edited the summary of this revision. (Show Details)Feb 6 2020, 5:03 AM
ndavis accepted this revision.Feb 6 2020, 5:52 AM
This revision is now accepted and ready to land.Feb 6 2020, 5:52 AM
broulik added a subscriber: broulik.Feb 6 2020, 7:51 AM
broulik added inline comments.
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
151

Can we please get this fixed in PlasmaComponents3 - this is being set all over the place, so clearly a bug in the component itself.

ndavis added inline comments.Feb 6 2020, 10:11 AM
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
151

I suppose it depends on whether or not you think PC3 toolbuttons should reflect the look and size of Qt Widget toolbar buttons by default. Toolbar buttons in Qt widgets use 22px icons (equivalent to units.iconSizes.smallMedium).

151

Another thing to consider is that window-close mimics the look of the titlebar close button with the margins of a normal breeze-icons monochrome icon, but the titlebar close button with the circle is larger than the other symbols until you hover over them. This leads to units.iconSizes.smallMedium being used for close buttons in some cases while other icons use units.iconSizes.small to get the same proportions. Should the margins be removed from window-close so that we can use the same icon size in the code everywhere to get the same kind of proportions?

trmdi added a subscriber: trmdi.Feb 6 2020, 2:59 PM
trmdi added inline comments.
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
39

Why do you remove all spacing? This increases lots of wasted space which is not good at least on small screens like 1366x768.

79

Should we make the height const like this too? e.g. make it a golden rectangle?

Otherwise, the size of the tooltip may be not consistent in all cases, e.g. a normal window vs a window which is available on all activities.

broulik added inline comments.Feb 6 2020, 3:03 PM
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
151

I'm merely stating that this is a behavioral change from PC2 to PC3 and realistically everyone using PC3 so far has manually hardcoded the icon sizes to some value.

No problem, I can fix it in PC3.

trmdi added inline comments.Feb 6 2020, 3:27 PM
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
79

How do you choose the number 17? On small screens, the tooltip is a bit too big.
Could it be smaller? e.g. 14-15 looks better on my small screen.

Actually it looks like there's nothing to fix in PC3. smallMedium is already the default size, and all the overrides in this patch are because we want to use non-default sizes in certain contexts.

applets/taskmanager/package/contents/ui/ToolTipInstance.qml
79

the number was chosen to make the tooltip exactly the same size as the current one.

ngraham updated this revision to Diff 75109.Feb 6 2020, 4:27 PM
ngraham marked 7 inline comments as done.

Re-add spacing

ngraham added inline comments.Feb 6 2020, 4:28 PM
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
79

If we do that, then the size of the preview/album art area can get very short, which I don't think is desirable.

ngraham marked 2 inline comments as done.Feb 6 2020, 4:29 PM
trmdi added inline comments.Feb 6 2020, 4:40 PM
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
79

Could we have a fixed size rectangle with some ratio?

97

Could it be zero here? Because Text lines already has its own padding, isn't it?

trmdi added inline comments.Feb 6 2020, 5:05 PM
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
79

I think it still looks acceptable, not tool short. This is the worst case:

The better case without the "Activities..." line, the albumArt will be bigger.

@trmdi I don't really agree with your idea of always using a fixed size rectangle, sorry. :)

applets/taskmanager/package/contents/ui/ToolTipInstance.qml
97

Depends on the font; you can't necessarily count on that.

ngraham marked 4 inline comments as done.Feb 6 2020, 5:41 PM
trmdi added a comment.Feb 7 2020, 3:52 PM

It's super big on my 1366x768 screen:

Before:


After:

And the Before and After size are not the same.

Is gridUnit screen-size-aware? That would be kind of weird.

trmdi added inline comments.Feb 7 2020, 5:03 PM
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
97

Heading has lineHeight: 1.2, so there are 2 things for padding here right? Could we remove one?

hein added a subscriber: hein.Feb 7 2020, 6:03 PM

trmdi's point is good, otherwise I like it.

ngraham updated this revision to Diff 75188.Feb 7 2020, 6:07 PM

Remove unnecessary spacing in the ColumnLayout with all Headings in it

ngraham marked an inline comment as done.Feb 7 2020, 6:07 PM
ngraham updated this revision to Diff 75189.Feb 7 2020, 6:09 PM

@trmdi was right, I did make it a bit bigger by accident

This revision was automatically updated to reflect the committed changes.