Mark applications that play audio, for all task icon sizes
ClosedPublic

Authored by gvgeo on Dec 12 2019, 4:05 PM.

Details

Summary

Fix cases where indicator not showing. Eg. icon-only task manager.
Place the indicator in the top right corner of the icon. And move it to the side or top if there is enough space.
Audio icon will no more move the task icon off-center.
When placed in the corner, use a smaller click area for muting audio, to avoid accidental mute.
Click area drops to 0.45 and Icon size to 0.63 from what would normally had.

Issues:
Has overlapping issues with other overlays: badge, and group expander.
Poor visibility.
Breeze icon is off center and overlaps(with the blue line at the right). For now,
moved audio icon a touch left, some other theme icons may look off-center.

BUG: 381656
FIXED-IN: 5.18.0

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
gvgeo requested review of this revision.Dec 12 2019, 4:05 PM
gvgeo created this revision.
gvgeo created this object with visibility "gvgeo (George Vogiatzis)".
gvgeo edited the summary of this revision. (Show Details)Dec 12 2019, 5:21 PM
gvgeo edited the summary of this revision. (Show Details)Dec 12 2019, 5:53 PM
gvgeo updated this revision to Diff 71386.Dec 12 2019, 5:57 PM
gvgeo added a reviewer: VDG.
gvgeo changed the visibility from "gvgeo (George Vogiatzis)" to "Public (No Login Required)".

Wow, I think this is the first time I've actually seen the audio indicator in my vertical Icons-Only Task Manager. :) Very nice patch. Code looks sane to me.

However overlapping the indicator over the icon sometimes doesn't look that great:

I wonder if it would make sense to put a contrasting background under the indicator. What do you think?

Or maybe what we need is a new "audio playing" embem/icon that's designed to be used to badge icons and comes with its own background. Of course that woludn't be FDO compatible with other icon themes, unless we named it something like audio-volume-high-emblem, which puts the emblem bit in the wrong place, and it would look bad with other themes.

gvgeo added a comment.Dec 13 2019, 9:40 AM

My main problem is, the difficulty to control the size of the icon.
While it appears to be a general icon issue, I hope that I'm doing something wrong.

Here looks to be too big. Although the main issue is, that the cassette is small.


With a square (or even circular) background, will have to shrink the icon. Otherwise would dwarf the task icon.
But if we shrink with a box, audio icon will look small below.

I would prefer a solution, as is, in the second row.

The 3rd row shows the progress indicator coloring that background.
But with a different task icon colors, could look bad.
The 4th row keeps the background color where it overlaps with the task icon,
while the rest(upper part) takes the progress indicator color.
3rd, and 4th solutions probably are too complex to implement and unnecessary.

Then again, I have no idea how to implement any solution.
I haven't look themes at all yet. And couldn't find what FDO stands for.
Any direction would be appreciated, if you 'd like to purse it any further.

gvgeo edited the summary of this revision. (Show Details)Dec 13 2019, 9:42 AM

Yes, the second one looks the best out of those for sure. FDO = "FreeDesktop.org", in this case referring to the FreeDesktop spec for icon naming. It ensures that icon themes work (at least tolerably).

Maybe what you could do is add a drop shadow to the icon, and give it hard edges and a contrasting color so it looks like an outline instead. See https://doc.qt.io/qt-5/qml-qtgraphicaleffects-dropshadow.html

ndavis added a comment.EditedDec 14 2019, 3:09 AM

Or maybe what we need is a new "audio playing" embem/icon that's designed to be used to badge icons and comes with its own background. Of course that woludn't be FDO compatible with other icon themes, unless we named it something like audio-volume-high-emblem, which puts the emblem bit in the wrong place, and it would look bad with other themes.

Maybe we should be using QML for those backgrounds instead of building them into the icons? Like a badge icon widget where you can scale the badge, put any icon of any size onto the badge and give the background any color.

gvgeo updated this revision to Diff 71662.Dec 16 2019, 11:51 AM
gvgeo edited the summary of this revision. (Show Details)

Added background when is in the top right corner (normally overlapping task icon).
Made icon take any size based on iconBox size.
Some adjustments in the sizes.

gvgeo edited the summary of this revision. (Show Details)Dec 16 2019, 12:01 PM

One thing that may look strange now is when hovering a task, the audio icon will not take the widget colorization.
Not sure how it would be best to manage this. As it gets currently takes a hover box (possible smaller in size and off-center) when you want to mute.

Btw both of your comments were helpful to update the patch.
Thank you.

ngraham accepted this revision.Dec 16 2019, 11:45 PM
ngraham added a subscriber: hein.

Thanks, this looks perfect to me with both Breeze Light and Breeze Dark, which is a good sign for color scheme compatibility:

You aren't able to get the glow/outline to perfectly match the color of the task's background color because that color is programmatically determined by the plasma theme itself, and is overlaid on a panel that's 10% transparent (by default). I think this is fine in its current state. A lovely improvement. Code seems sane to me given the complexity of what's going on. @hein, what's your take?

This revision is now accepted and ready to land.Dec 16 2019, 11:45 PM
hein added inline comments.Dec 22 2019, 9:13 PM
applets/taskmanager/package/contents/ui/AudioStream.qml
93

What's with the magic number 14?

95

Introducing a glow is a pretty big design change, and it's also very expensive rendering-wise. This isn't covered by the merge request description, what's the motivation here?

ngraham added inline comments.Dec 22 2019, 11:13 PM
applets/taskmanager/package/contents/ui/AudioStream.qml
95

I requested this to improve the display with an IOTM. Until this patch, display of the audio indicator was so buggy that I never saw it and never noticed the visual issues with having it overlap an icon like an emblem. I think it's a logical enough part of making the display work properly, but we could easily do this in a follow-up patch if you prefer.

gvgeo updated this revision to Diff 72065.Dec 23 2019, 11:43 AM
gvgeo edited the summary of this revision. (Show Details)

Change visibility. (iconBox from >14 to >=16)
Change visibility comment.
Change the Glow's radius. (audioStreamIcon 0.2 -> 0.17)
Decrease Glow's samples.

gvgeo marked an inline comment as done.EditedDec 23 2019, 12:00 PM

16 is the magic number that keep task icons in a reasonable dimension.
Below that point go crazy tiny, and generally space is too small.
Especially if there is an expander icon too, where it becomes too crowded.

gvgeo updated this revision to Diff 72073.Dec 23 2019, 12:10 PM

Add 'cached' Glow property.

Perhaps for the sake of reducing magic numbers, you could replace 16 with Units.gridUnit, which is 18`.

gvgeo updated this revision to Diff 72136.Dec 24 2019, 10:00 AM

Replace magic number with iconSize.

ngraham accepted this revision.Dec 24 2019, 3:40 PM
hein added a comment.EditedMon, Jan 6, 3:44 PM

I'm still not a big fan of the Glow for a number of reasons:

  • This is hard-coding a particular visual, when Plasma generally strives to put visuals under user control through theming. Whenever we end up hard-coding a particular look, it tends to cause bug reports.
  • We may have verified this appearance with the Breeze icon theme (I don't think it looks great), but not with all icon themes.
  • The part of the code doing the Glow is the main use of magic numbers (e.g. the 0.17 because it happens to look OK with Breeze).
  • Qt Graphical Effects are relatively heavy, but also have higher GPU/driver requirements than simpler Qt Quick items. They sometimes break in environments like VMs. Using them in core UI is always a bit risky.

The precedent we have for "adding a little icon on top of a bigger icon while keeping both readable" is the emblem icons in the icon themes. It may be a better approach to define new emblem-playing and emblem-muted icons, add some to Breeze and use them in the vertical TM. This would also further consistency with other instances of icon-with-emblem throughout Plasma. The main argument against it is that it will make one out of n cases look different. Thoughts?

All right, that makes sense to me. Let's get some new icons then: https://bugs.kde.org/show_bug.cgi?id=415937

hein added a comment.Mon, Jan 6, 3:57 PM

Maybe there's a third option: Another precedent we already have is the circular carve-out we use for the notification number badge. Perhaps we could reuse that for the indicator.

Maybe for now, we can just remove the glow and land this as-is, since it doesn't represent a regression at all. That way we'll have it for 5.18. Then for 5.19, we can investigate using an emblem icon or a notification-style bubble.

How does that sound?

hein added a comment.Mon, Jan 6, 5:23 PM

That's good with me, the rest of the patch is good and much appreciated.

Great. @gvgeo, can you remove the glow effect then? Sorry for cluttering up this patch with that request.

gvgeo updated this revision to Diff 72929.Mon, Jan 6, 8:23 PM

Removed glow
Small increase of icon size.

Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptMon, Jan 6, 8:23 PM
gvgeo added a comment.Mon, Jan 6, 8:26 PM

No problem ngraham, was tiny change anyway.

Btw, Vlc would love a follow up patch.

gvgeo marked 2 inline comments as done.Mon, Jan 6, 8:26 PM
gvgeo edited the summary of this revision. (Show Details)
ngraham accepted this revision.Mon, Jan 6, 9:30 PM

Yes it would. :)

@hein, you good with this now?

hein accepted this revision.Sat, Jan 11, 12:25 AM
ngraham edited the summary of this revision. (Show Details)Sat, Jan 11, 4:14 AM
This revision was automatically updated to reflect the committed changes.
broulik added inline comments.
applets/taskmanager/package/contents/ui/AudioStream.qml
92

This breaks displaying the icon on small panels like mine.

gvgeo marked an inline comment as done.Mon, Jan 13, 2:41 PM