[applets/Task Manager] Refine tooltip highlight effect
ClosedPublic

Authored by ngraham on Jan 17 2020, 9:06 PM.

Details

Summary

This patch indents the window thumbnail and album art view by one pixel,
ensuring that the edges of the tooltip effect which is drawn under it are
never completely covered up and improving consistency between contents.

BUG: 416390
FIXED-IN: 5.18.0

Test Plan

Just a window thumbnail:
Before:


After:

Player controls + icon:
Before:


After:

Player controls + window thumbnail:
Before:


After:

Player controls + album art
Before:


After:

Diff Detail

Repository
R119 Plasma Desktop
Branch
tooltip-highlights-take-2 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21729
Build 21747: arc lint + arc unit
ngraham created this revision.Jan 17 2020, 9:06 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 17 2020, 9:06 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Jan 17 2020, 9:06 PM

I'm not 100% sold on drawing the highlight on top and tinting everything, and can revert that part if everyone else hates it.

ndavis added a subscriber: ndavis.Jan 18 2020, 2:12 AM

Not a fan of the inner frame. We should avoid making frames within frames. I don't think it really needs changing, except that the sides cover the blue border frame.

filipf added a subscriber: filipf.Jan 18 2020, 3:08 AM

Not a fan of the inner frame. We should avoid making frames within frames. I don't think it really needs changing, except that the sides cover the blue border frame.

+1

Yeah. I don't disagree. But if I just ensure that the highlight effect is the highest item, the borders still get obscured by the player controls and album art. And the rounded corners of the highlight leave a pixel or two of the art to escape the bounds too.

ndavis added inline comments.Jan 18 2020, 5:08 AM
applets/taskmanager/package/contents/ui/ToolTipInstance.qml
185

I recall seeing a QML dos and don'ts webinar that said not to use large z values.

Yeah. I don't disagree. But if I just ensure that the highlight effect is the highest item, the borders still get obscured by the player controls and album art. And the rounded corners of the highlight leave a pixel or two of the art to escape the bounds too.

You can't shrink the size of it to fit within the blue border?

You can't shrink the size of it to fit within the blue border?

That's what this patch does in its current form.

filipf requested changes to this revision.Jan 18 2020, 11:08 AM

I think what Noah means is that the media controls row should perfectly touch the left, right and bottom border. The margins in this patch are too big so they effectively create another frame in a frame.

But what's more important is that having the highlight effect above everything just doesn't work with themes that have a filled highlight style. We should scrap that idea.

This revision now requires changes to proceed.Jan 18 2020, 11:08 AM
trmdi added a subscriber: trmdi.Jan 20 2020, 2:09 PM

Could you clarify before/after screenshots?

trmdi added a comment.Jan 20 2020, 4:13 PM

How about this?

ngraham updated this revision to Diff 73984.Jan 21 2020, 3:37 AM
ngraham marked an inline comment as done.

Better, less disruptive version

ngraham edited the summary of this revision. (Show Details)Jan 21 2020, 3:39 AM
ngraham edited the test plan for this revision. (Show Details)
ndavis added a comment.EditedJan 22 2020, 4:32 PM

This needs to be rebased.

Better, less disruptive version

It doesn't look right on programs with no album art. Something is wrong with the margins.

However, this looks pretty good:

How about this?

I think this looks pretty good. Maybe it's just because I'm not used to it yet, but I kind of prefer having the background highlight take up all the height for all task manager previews.

ngraham updated this revision to Diff 74180.Jan 22 2020, 11:08 PM

Fix the rebase

ngraham updated this revision to Diff 74183.Jan 22 2020, 11:21 PM

Fix the margins for the window thumbnail too

ngraham edited the summary of this revision. (Show Details)Jan 22 2020, 11:26 PM
ngraham edited the test plan for this revision. (Show Details)

Looks like it still needs more margin work on the bottom.

Hmm, I can't get that to happen on my machine. :/

trmdi added a comment.Jan 23 2020, 6:14 PM

Pretty nice.
But clicking on playerControlsRow (exclude buttons) does not behave the same way like clicking on the cover art, while the effect would make users think that they can click on the control row to open the window.

ngraham planned changes to this revision.Jan 23 2020, 9:14 PM
ngraham updated this revision to Diff 74464.Jan 27 2020, 9:01 PM

That is nicer; let's do that instead

ngraham edited the test plan for this revision. (Show Details)Jan 27 2020, 9:13 PM
ngraham updated this revision to Diff 74465.Jan 27 2020, 9:14 PM

Tweak pixels once more and add explanatory comments

ndavis accepted this revision.Jan 28 2020, 1:21 AM

I like this. It's pretty elegant.


trmdi added a comment.Jan 28 2020, 1:40 AM

What happens when you hover the mouse on the control bar? Did you address it?

What happens when you hover the mouse on the control bar? Did you address it?

The buttons on the control bar use normal plasmashell toolbutton hover effects. The bar itself does nothing.

ndavis requested changes to this revision.Jan 28 2020, 3:06 AM

Actually, I just noticed a problem. When you hover over the bar, the thumbnail area gets highlighted, bit if you click on the bar, it doesn't bring the window to the front.

This revision now requires changes to proceed.Jan 28 2020, 3:06 AM
trmdi added a comment.EditedJan 28 2020, 3:08 AM

Actually, I just noticed a problem. When you hover over the bar, the thumbnail area gets highlighted, bit if you click on the bar, it doesn't bring the window to the front.

Yes, that is what I'm asking. :)
I think there are 2 ways to handle this:

  • do not highlight the thumbnail when hovering over the bar (I like this option)
  • highlight, but clicking on the bar brings the window to the front
ngraham updated this revision to Diff 74474.Jan 28 2020, 4:43 AM

Fix that and also make the code simpler and more sensible

BTW, I tried fixing the old style just to see if I wanted it more and got most of the way there (I had to edit the size of the 2nd Rectangle in playerControlsFrostedGlass), but realized that this new style is still better.

ngraham updated this revision to Diff 74475.Jan 28 2020, 4:51 AM

Correct comment; last change now :)

ndavis accepted this revision.Jan 28 2020, 4:53 AM

LGTM

Not yet looked at the code carefully, but I feel the top margin and the bottom one of the thumbnail are not identical.

Not yet looked at the code carefully, but I feel the top margin and the bottom one of the thumbnail are not identical.

With the latest iteration of the patch, they should be. Feel free to try it out for yourself!

trmdi added a comment.EditedJan 28 2020, 5:57 PM

Not yet looked at the code carefully, but I feel the top margin and the bottom one of the thumbnail are not identical.

With the latest iteration of the patch, they should be. Feel free to try it out for yourself!

Do you see this or just me?

It seems that PlasmaComponent.HighLight does not really fill whole hoverHandler, there is unfilled 1px at the bottom.

filipf accepted this revision.Jan 28 2020, 6:46 PM

^ I can't reproduce that, the patch works as intended for me. (we should investigate this though)

One thing to just comment is that this is tailor-made for Breeze (the 1px thing), but it looks fine with other themes and I don't really know of any other solution.

This revision is now accepted and ready to land.Jan 28 2020, 6:46 PM
This revision was automatically updated to reflect the committed changes.
trmdi added a comment.Jan 29 2020, 7:19 AM

^ I can't reproduce that, the patch works as intended for me. (we should investigate this though)

One thing to just comment is that this is tailor-made for Breeze (the 1px thing), but it looks fine with other themes and I don't really know of any other solution.

Try changing the General Font size to see if you can reproduce it. On my system, I can reproduce it with Noto 10, but not with Noto 11.

^ I can't reproduce that, the patch works as intended for me. (we should investigate this though)

One thing to just comment is that this is tailor-made for Breeze (the 1px thing), but it looks fine with other themes and I don't really know of any other solution.

Try changing the General Font size to see if you can reproduce it. On my system, I can reproduce it with Noto 10, but not with Noto 11.

Still can't see it, except for this one case:

Argh, now I can see it too:

No idea what's causing this. The code seems conceptually right to me. Maybe something's getting rounded up instead of down or something.

trmdi added a comment.Jan 30 2020, 4:35 AM

Argh, now I can see it too:

No idea what's causing this. The code seems conceptually right to me. Maybe something's getting rounded up instead of down or something.

Yep, that is. A possible solution I found is to round up winTitle.height like this: https://phabricator.kde.org/source/latte-dock/browse/master/plasmoid/package/contents/ui/previews/ToolTipInstance.qml$175

Nice find. Fixed in 093e2d917ff386e627a062f3337c9ff23294e95c.

Hopefully that's done it!