[Task Manager] Tooltips redesign
ClosedPublic

Authored by romangg on Dec 19 2016, 6:22 PM.

Details

Summary

This patch reworks the design of the task manager tooltips as requested by task T2029.

Comparision video: https://youtu.be/6MKfBK63kpo

Features:

  • New tooltip design:
    • Less screen space occupied by tooltips
    • Tooltips have always the same size
    • Better structured informations about a task: Appname, window title, desktop, activity if useful
    • Close button better looking and more visible
  • Group tooltips show per group task all individual informations
  • Group tooltips provide context menu per task and its group
  • Group tooltips in vertical bar are lined up as column
  • If supported by media player, controls are available for each instance separately
  • Fixed: In any case secondary media players instances are always also shown in group tooltip
  • Fixed: Flickable area when exceeding available display space
  • Fixed: mpris2 source name detection only activated on changed sources and not on every new data tick
  • Fixed: Many warning and error messages

Technical overview:

  • TooltipDelegate is loaded per task, which simplifies code and helps to prevent graphical glitches.
  • For groups instead of relying on the window list provided by X, we now use a proper submodel of the grouped tasks (because of a Qt bug for now we need to do some workaround here by loading it delayed on a parent index change)
  • Window titles are processed by some reg exp to show only the relevant part of it
  • Media player instances are distinguished by their process ids

Pictures:

Default tooltip of opened application:

Media player tooltip:

Group tooltip horizontal panel:

Group tooltip vertical panel:

Test Plan

Manually tested on X, can't login to Wayland session for some other reason at the moment, so not tested there (but the old tooltips were broken in Wayland anyway).

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
romangg updated this revision to Diff 9170.Dec 19 2016, 6:22 PM
romangg retitled this revision from to [Task Manager] Tooltips redesign.
romangg updated this object.
romangg edited the test plan for this revision. (Show Details)
romangg added a reviewer: Plasma.
romangg set the repository for this revision to R119 Plasma Desktop.
romangg added a project: Plasma.
romangg added a subscriber: plasma-devel.
romangg added a reviewer: VDG.Dec 19 2016, 6:31 PM
romangg added a project: VDG.

Great improvements!
Is the desktop / Activity only shown if

  • there are multiple ones and
  • the task manager shows tasks from different ones?

Great improvements!
Is the desktop / Activity only shown if

  • there are multiple ones and
  • the task manager shows tasks from different ones?

Glad you like it! And yes, yes. :)

hein requested changes to this revision.Jan 3 2017, 12:07 PM
hein added a reviewer: hein.
hein added a subscriber: hein.
hein added inline comments.
applets/taskmanager/package/contents/ui/ContextMenu.qml
59

Maybe s/get/data to be more conventional, but could you argue for kicks why this churn is needed vs. just making sure the visualParent has the m prop?

applets/taskmanager/package/contents/ui/Task.qml
77

Plasma coding style: Terminate lines in procedural blocks with semicolae.

https://community.kde.org/Plasma/QMLStyle

Please fix throughout the patch.

103

Why did you remove the tooltip hiding?

See also https://phabricator.kde.org/D3916 btw

224

I know we talked about this, but this approach strikes me as weird. A component is something to be instanciated; reloading the sourceComponent value seems wrong since that's not about a particular instance, and any problem you may have with the index would be related to a particular instance. You may want to reinstanciate the component, but why reset the component?

235

Bad coding style / bad coupling: Referencing a non-generic name outside the delegate ... now this delegate only works right if there's a taskRepeater outside somewhere.

applets/taskmanager/package/contents/ui/ToolTipDelegate.qml
40

Sanity check: Why count children instead of the IsGroupParent role?

Also:

(1) For performance and code hygiene reasons please mark properties readonly if you won't assign them.

(2) Why copy tons of data roles into local props in the first place instead of just accessing the model directly?

174

Why 0.75? That's not an even number of pixels ...

199

Why 0.6? Is that in the HIG somewhere? Maybe instead of Label we could use a Heading of a particular level? We can't have consistent typography across applets if applets contain magic numbers for sizes.

446

Is this your code? What's "emergence"?

489

Needs a code comment explaining what it wants to achieve.

applets/taskmanager/package/contents/ui/main.qml
372

Please use proper capitalization and punctuation in code comments throughout the patch.

379

"Do things later" timers tend to be hacks that need a comment justifying their existence ...

This revision now requires changes to proceed.Jan 3 2017, 12:07 PM
romangg updated this revision to Diff 9670.Jan 3 2017, 9:10 PM
romangg edited edge metadata.
romangg marked 6 inline comments as done.
  • Rebased on current master
  • Review changes
applets/taskmanager/package/contents/ui/ContextMenu.qml
59

The context menu can be called now also for a grouped task. The visualParent is the grouped task parent though. So we need to get the modelProps of property var modelIndex (which is in case of a grouped task different from visualParent.index, i.e. visualParent.m)

applets/taskmanager/package/contents/ui/Task.qml
103

When the context menu is opened, it requests focus and the tooltip area loses mouse over, which results anyway in hiding the tooltip. The hideToolTip() call is unnecessary therefore.

D3916 only concerns left click. Since it was created after I created this diff, it wasn't yet integrated.

224

How would reinstating look in contrast to undefining the sourceComponent and then setting it back to the component?

235

This is also part of the workaround hack and therefore hopefully only temporary until the Qt bug is fixed.

applets/taskmanager/package/contents/ui/ToolTipDelegate.qml
40

(1) Done
(2) Was carried over from old tooltips. Otherwise we get lots of "can not assign undefined" warning messages on loading the component, because the model is not yet ready.

199

I wanted to use Headings with different levels. The problem is, that they always automatically create huge margins. Since we wanted to minimize the size of the tooltips as much as possible, I used instead relative sizes, which worked way better for me, because they give a more direct control over the look in the end.

They are not really magic numbers, are they? Read the 0.75 value for example as: the window title font size is 75% as large as the font size of the app name.

Of course on the other side you raise a valid point with the consistency. Though I've seen relative font sizes also elsewhere (digital clock).

446

No, not my code. These were the buttons from the old revision. I would remove this code portion, if there is no problem with my rewrite of them as IconItems directly above, which don't produce error messages on emergence (when the tooltip of a player opens) and which imo look better here in this context. I'll just remove it in the second diff to minimize confusion.

applets/taskmanager/package/contents/ui/main.qml
379

It's the same hack / workaround from Task.qml because of the Qt bugs of the submodel index. Clarified it.

hein added inline comments.Jan 4 2017, 5:33 AM
applets/taskmanager/package/contents/ui/ContextMenu.qml
59

That's kinda what I was wondering. If the context menu is opened for an element in the tooltip, why is the visualParent the task item?

applets/taskmanager/package/contents/ui/Task.qml
224

Cycling the 'active' prop.

applets/taskmanager/package/contents/ui/ToolTipDelegate.qml
40

Hrm ... ok, but somehow still feels wrong :)

199

The problem is that the next time someone else does this in another UI they might decide 0.70 is right-er than 0.75 - we need to discuss this at the next Monday hangout maybe.

446

Aye, thx!

hein requested changes to this revision.Jan 4 2017, 5:35 AM
hein edited edge metadata.
hein added inline comments.
applets/taskmanager/package/contents/ui/Task.qml
229

Can you flesh this out a bit more and explain exactly what situation (as it pertains specifically to our UI) breaks without the workaround? This at the very least needs to be cleaned up / fixed at a later date if it can't be now, and whoever does this will need a testcase description so they know their changed code satisfies the requirements.

This revision now requires changes to proceed.Jan 4 2017, 5:35 AM
anthonyfieroni added inline comments.
applets/taskmanager/package/contents/ui/ToolTipDelegate.qml
199

Why we want to make tooltip smaller? Your patch will effect all screens with higher resolution than your i.e. will be smaller to read. Do not use magic number, get it pro rata of screen resolution. You must apply your patch to FullHD or bigger to see the difference.

239

Again, do not make a bad decisions, use Screen width, height and pixel ratio for HiDPI.

import QtQuick.Window 2.2
property int maxWidth: Screen.width / Screen.devicePixelRatio
property int maxHeight: Screen.height / Screen.devicePixelRatio
romangg updated this revision to Diff 9695.Jan 4 2017, 10:29 AM
romangg edited edge metadata.
romangg marked 12 inline comments as done.
  • Better description of the workaround
  • Use active property instead of reloading component for workaround
  • hideToolTipTemporarily() to not rely on the workaround when DelegateModel gets fixed upstream
applets/taskmanager/package/contents/ui/ContextMenu.qml
59

It's basically the second best solution. ;)

When I tried to set the tooltip as visual parent, it opened the context menu at the border of the tooltip how it is expected, but it also closed the tooltip, because the context menu requested focus. So the context menu is suddenly standing in the middle of the screen without being next to the grouped task.

If you know a way how the context menu can be opened at the border of the grouped task while not hiding the tooltip automatically, I would gladly implement this solution instead.

applets/taskmanager/package/contents/ui/ToolTipDelegate.qml
199

The font sizes are directly based on theme.mSize(theme.defaultFont).height, which scales with default font size and screen DPI.
See here: https://api.kde.org/frameworks/plasma-framework/html/classPlasma_1_1Theme.html#a7d8a89c8f7c04382c229600c4f5d934a

239

header.width, i.e. thumbnail.width is controlled by a constant factor of theme.mSize(theme.defaultFont).width, which again is font and screen DPI dependent, such that the thumbnail will always have the width of the maximal possible font and DPI dependent width of the app name in the header plus close the button width.

Directly using Screen.width / ... doesn't make much sense to me. We want the thumbnail to fill out the available space defined by the header above it.

The height variable in contrast is arbitrary and could be any value, which would always make somehow sense since windows can have any format by resizing them. Using the Screen height-width ratio on heider.width could be also a solution, but I'm not sure if people, who use their monitor in potrait mode want their thumbnails to be suddenly a lot higher than on the normal screen next to it, which also means that the tooltip needs more space on the portrait mode screen, since we cannot reduce the width on the other hand (needs horizontal space for app name / window title).

hein added a comment.Jan 5 2017, 8:47 AM

I just tried running the patch, and instead of smooth morphs when going between tooltip contents I get an instant change with a 1-frame glitchy stretch ... it's pretty bad.

In D3738#74290, @hein wrote:

I just tried running the patch, and instead of smooth morphs when going between tooltip contents I get an instant change with a 1-frame glitchy stretch ... it's pretty bad.

Sorry, I can't replicate this issue. On my testing machine the tooltips morph smoothly. I've tried it also with different animation speed settings in the Compositor KCM. It's always working flawlessly.

romangg updated this revision to Diff 9787.Jan 6 2017, 1:42 AM
romangg edited edge metadata.

Fallback to previously used method: One ToolTipDelegate, which gets repositioned to the the task currently containing the mouse.

To do this cleanly, the ToolTipDelegate needed to be split up. On the plus side the ugly workaround is not any longer necessary.

Tested the new version on a PC and a laptop and there were no visual artifacts.

hein accepted this revision.Jan 6 2017, 5:21 AM
hein edited edge metadata.

Ok, let's merge then.

This revision is now accepted and ready to land.Jan 6 2017, 5:21 AM

Maybe we can add configuration for tooltip size? I, personally, like very much biggest ones.

hein added a comment.Jan 6 2017, 5:56 AM

I'm against adding a config option for it for a while. We've had a lot of user feedback that the old ones were too big and fluffy, which this is a reaction to. And note that as Roman points out hidpi scaling is accounted for (I use a 3000x200 13" screen myself). Let's live with the new tooltips for a while and get used to them.

This revision was automatically updated to reflect the committed changes.
broulik added a subscriber: broulik.EditedJan 8 2017, 2:58 PM

I like thew new layout a lot, it might let me keeping them enabled! :D Good job.

Few nitpicks:

  • Font color of the large heading in the tooltip is darker/stronger, I think Heading (used in the default tooltip) sets some opacity making it softer
  • The tooltip no longer shows information like "on all desktops", "on desktop 2" etc
  • Descenders are truncated in launcher tooltips (e.g. the subtext "Dateiverwaltung" has its "g" cutoff)
  • The icon in launcher tooltips is pretty small compared to the rest

This now also introduces some inconsistency with the other tooltips. Task manager has very compact ones now whereas all others are huge compared to it. Especially noticeable when you move from Kickoff to a launcher next to it. I think we should apply a similar more compact design to the default tooltips. (I never liked the massiveness of Plasma tooltips anyway)

markg reopened this revision.Feb 19 2017, 4:32 PM
markg added a subscriber: markg.

Hmm, i don't know if this is the appropriate way in a phabricator workflow. But this does reach exactly those involved in this change which is what i intent.

On to the point.
When this change just appeared i looked at it. At the video, the code and had an impression of: "Ohh, that's a rather nice improvement! Nicely done!"

But now that we have a plasma version with this code in it i do have a few remarks. Nothing serious, just some minor but notable details.

  • The tooltips of the task manager now look out of place compared to the tooltips in other areas of a panel (think of the clock, kickoff, etc.. everything non task manager).
  • The tooltips clearly have a different style compared to other tooltips. The text is much closer to the corners.

The issues are easily fixable. It just needs to follow the margins that the other tooltips use.
Or the other tooltips have to be adjusted, either way makes it consistent again.

This revision is now accepted and ready to land.Feb 19 2017, 4:32 PM
In D3738#87601, @markg wrote:

...

There has already been a commit to master tackling this issue: https://phabricator.kde.org/D4491

romangg closed this revision.Feb 19 2017, 7:45 PM
markg added a comment.Feb 19 2017, 8:01 PM
In D3738#87617, @subdiff wrote:
In D3738#87601, @markg wrote:

...

There has already been a commit to master tackling this issue: https://phabricator.kde.org/D4491

That's great, thank you!