blue-systems / plasma-5.5

Plasma 5.2 - 5.5
0 stars 0 forks source link

Taskmanager: (normal, IO, EITM): blinking icons when expanding or shrinking tasks #31

Closed star-buck closed 8 years ago

star-buck commented 9 years ago

https://www.dropbox.com/s/kqk8lk69h5cnfvk/TM-blinking%20icons.mkv?dl=0

would be nice if it had the same clean appearance/disappearance than kde4.x

eikehein commented 9 years ago

We have a patch for PlasmaComponents.IconItem on ReviewBoard right now that might help with this (it fixes a delay in icon rendering).

star-buck commented 9 years ago

any link and when it will be packaged for testing?

eikehein commented 9 years ago

https://git.reviewboard.kde.org/r/121219/

ETA unknown, it's still being debugged. I also don't know if it will help, but it might. If it's not possible to eliminate low-hanging fruit delaying rendering it's unlikely to be addressed in 5.2 since it might need a larger redesign of either the model or the applet to eliminate the delegate reinstanciation (which is difficult because it currently happens for quite good reasons).

star-buck commented 9 years ago

just curious: why it didnt happen in kde4 TMs?

eikehein commented 9 years ago

It's either the bug this patch tries to address, or we were just lucky that certain things were faster in Qt Quick 1. What's going on is basically that one task button is destroyed and a new one is created, and right now that hand-over is slow enough and has an UI update inbetween so that we get flicker.

So there's three ways to do this kind of thing:

3 is theoretically ideal, except in reality it ends up being cheating as well, since you're abandoning representing the actual state of the system directly and immediately, and have to instead try to keep things around on the assumption that something is going to come along and replace it. That means you need timed error bars to handle the case where it doesn't, and all of those things. libtaskmanager already tries to do some of that trickery because it has no choice (mostly around startup notifications and partly written by me) for the same goals of seamless in-place replacements, but if it were to wind up being necessary, the complexity of that trickery would need to be scaled up quite a bit. Complexity means harder testing and more bugs.

So it'd be neat if we could get lucky on #1.

eikehein commented 9 years ago

Good news: I just tried the patch and it helps, no more flicker.

eikehein commented 9 years ago

Patch is merged now.

star-buck commented 9 years ago

still blinking, can you check if the patch is in 20150109 KCI?

eikehein commented 9 years ago

It should be in there, yes.

star-buck commented 9 years ago

here is video, though this is minor and not really important compared to other bugs for p5.2: https://www.dropbox.com/s/iutsa3p23hvo51z/icon-blink-iotm.mkv?dl=0

eikehein commented 9 years ago

I'll investigate this on the ISO and see what's up there. It's possible that the performance improvement varies with the graphics stack. Maybe we need to try even harder to improve it, as described above :).

star-buck commented 9 years ago

the icon does not blink on the same hardware but kde4, i doubt its hardware, as you always notice. under kde4 the task simply shrinks back without affecting the icon to disappear and reappear.

eikehein commented 9 years ago

What I meant is that hardware might account for why the performance improving patch isn't enough on some systems.

star-buck commented 9 years ago

again, why didnt it happen in kde4? from your list https://github.com/blue-systems/plasma-5.2/issues/31#issuecomment-66530938

1 is imo not going to work, no matter how fast, the faster the blink the more irritating it will be

3 is how it seems to have worked in kde4, it was just shrinking the task and leaving the icon as is

eikehein commented 9 years ago

again, why didnt it happen in kde4?

That's really hard to say. Plasma 1 and Plasma 2 use completely different rendering toolkits. There's a few hundred thousand lines of code there I (and no one else in KDE either) didn't write, and that's before you get to the drivers :).

the faster the blink the more irritating it will be

If a replace happens inbetween rendered frame x and rendered frame x+1, it becomes invisible because there's no blink left. A blink needs at least three frames (shown, not shown, shown again). Assuming you render at 60 hz, that means you have ~16ms per frame, so you need things to happen in <16ms. If the replace happens faster than that, it's gone.

... unless Qt forces a repaint inbetween those and thus forces that middle frame, which would be unfortunate, since we don't have low-level control to change that.

3 is how it seems to have worked in kde4, it was just shrinking the task and leaving the icon as is

Nothing about the behavior has changed between 4 and 5 in the library or applet code. This is really about adjusting to the new rendering stack.

star-buck commented 9 years ago

of course if the icons stays visible in every frame then its not "blinking". if blinking simply becomes faster, thus is not eliminated, the irritation could be worse though. But since you mentioned there is no flicker with the patch, it must be the KCI has somehow the patch not working over here, how could I check that? What package is the patch applied to?

eikehein commented 9 years ago

of course if the icons stays visible in every frame then its not "blinking".

Yep, and #1 wasn't about making the blinking faster but about making the replace reliably fast enough that it becomes invisible by not showing up in a frame. That's assuming that's a viable strategy and there isn't a forced repaint with a blank frame from the Qt side. This is a general problem with scenegraph-style retained mode rendering systems where you stop having control over when you repaint.

But since you mentioned there is no flicker with the patch, it must be the KCI has somehow the patch not working over here, how could I check that? What package is the patch applied to?

What the patch fixed was a new icon item instance having 150ms delay before the icon is actually rendered, which obviously has to cause blank frames. This also caused flickering e.g. in the OSD popups. It's in plasma-framework and was in KF 5.6.

star-buck commented 9 years ago

from how long the icon "dissappears" it seems it could well be 150ms or even longer as mentioned in the patch submit thread: https://git.reviewboard.kde.org/r/121219/ So maybe the patch didnt apply in KCI or kf5.6 wasnt packaged fully, any method to test this?

eikehein commented 9 years ago

Maybe the screen brightness OSD is a good testcase?

eikehein commented 9 years ago

FWIW, this is definitely down to system-specific slowness in PlasmaComponents.IconItem. I can get the blink on my ISO system, too (even with the patch). I was working on improving the performance of how libtaskmanager gets the window icon from the X server thanks to tips from Martin (still worth doing, too) and played around with using QIconItem in the delegate instead, and the blinking just goes away (also notice how when you use Breeze the active bar appears immediately before the icon blinks, so it's not the delegate as a whole, just the icon). Unfortunately QIconItem isn't a suitable replacement in all cases, but it means we know where to attack this further.

aleixpol commented 9 years ago

Why isn't QIconItem suitable? Maybe we should make it suitable?

eikehein commented 9 years ago

Note that QIconItem isn't provided by Qt Quick, it's a component we provide and basically use whenever IconItem is stupid and slow. The real answer is to fix IconItem.

E.g. IconItem hard-codes using the Plasma theme's icon overlay, so anything that needs to get real app icons (e.g. Kicker) ends up using QIconItem. However, QIconItem has no support for things like the active state effect. We have UIs (like Kickoff) where we mix the types and get different look and feel for two things right next to each other just because one wants to use the overlay and one does not. I've previously proposed we turn the overlay into a mix-in (e.g. a PlasmaThemeIcon type that can be used as an IconItem.source) to address that.

Aside from the active state effect (which the Task Manager uses on hover) QIconItem also doesn't have support for doing animated blends between icons, which is also desirable in the Task Manager.

The rest of the differences is in format support; IconItem supports more data types, e.g. SVG.

notmart commented 9 years ago

I fear adding overlay effects and animate d transitions would significantly weight on qiconitem as much as iconitem is weighing now. i don't have numbers, but i think this weighs more than having support for svgs that for most taskmanager icons ends up not being used

eikehein commented 9 years ago

I'm not proposing to add effects support to QIconItem though, I want us to get rid of QIconItem and make IconItem fast ...

aleixpol commented 9 years ago

QIconItem cannot go as IconItem adds a dependency on plasma-framework.

notmart commented 9 years ago

even just adding a property that would make it avoid ever using svgs would make it slightly better, since it would ensure svg instances never being created, i'm not sure how much would help (note that having a separate object for providing pixmas means one more qobject per icon, causing probably a negative net cost)

eikehein commented 9 years ago

QIconItem cannot go as IconItem adds a dependency on plasma-framework.

I'm talking about users that already depend on plasma-framework.

Using two different icon components with different look, behavior and sets of bugs seems undesirable to me. It has numerous costs, and some of them are hard to avoid (e.g. maintenance effort, and developer education - Plasma UI authors need to decide which one to use). From a Plasma platform POV it's just better to make sure Plasma developers have no need to use QIconItem.

notmart commented 9 years ago

QIconItem and IconItem have different use cases, and that's fine. I'm all for making iconsitem bug free and faster (and supporting a mode of only icons from theme) but on a perspective of things given to 3rd party developers and particular use cases (where for instance memory and speed of creation are the one and only priority due to the quantity of stuff created) will always need both of them.

eikehein commented 9 years ago

Let me take a step back maybe and explain better what I mean: The current situation definitely lowers the quality of both the user and the developer experience. From the user perspective, we have consistency (asset selection, scaling, interaction feedback, rendition) and performance problems. From the developer perspective, it's arguably worse - the type names provide no clue as to use cases, the documentation doesn't provide guidelines, the types are in two different libs with no cross-links. Between the low quality of the components, the low quality of the documentation, and the organization of the SDK, it becomes hard to write a high-quality UI with this stuff, and you don't need to look any further than how those most qualified to do it (us) have failed to do so.

This is a set of separate bugs that need to be individually addressed, but I think the best outcome would be one where the reasons to ever use QIconItem in a Plasma UI would be very narrowly defined, and developers can rely on IconItem usually doing the right thing and having the features and performance level they need.

notmart commented 9 years ago

in particular here there are two issues: having a way to disable svg icons from IconItem, and i can eventually add a property for that and a different thing, that is this flicker bug: here when a launcher foes to full task it's indeed visable that the icon disappears from an instant when the task item switches mode.. if i understood correctly what's happening is that the old launcher item gets actually destroyed and a new one with the full task gets created? if it's so i'm not sure it can really be fixed inside IconItem (having it start slightly faster is ok, but at least one frame without icon will always happen i think) is it technically feasible to use the same graphicsitem for both the laucher and the task?

eikehein commented 9 years ago

if it's so i'm not sure it can really be fixed inside IconItem (having it start slightly faster is ok, but at least one frame without icon will always happen i think)

It's fine with QIconItem. Qt Quick is smart enough not to blank on a delegate swap.

is it technically feasible to use the same graphicsitem for both the laucher and the task?

It's hard without inviting a lot of complexity.

I've described this earlier in this thread. The lifecycle is complicated: launcher <-> startup notification <-> task. The hand-offs between these stages are complicated: Stages can be omitted entirely (startup notifications can be disabled, or can just be broken for an app, or an app can fail to start, or never show a window). They can overlap, because the timing for when particular notifications arrive on X is not guaranteed. They can be incomplete because you can get a start notification but not a stop one, so there are timeout contingencies.

The model tries to compromise between directly representing the state it knows about already, and smoothing these things over (e.g. reuse actually happens for the startup notification -> task part). Doing this sort of smoothing-over is easier on some other toolkits than on Qt (e.g. toolkits like Cocoa, where model change transaction dispatch is detached from making model changes, while Qt requires you to dispatch immediately and with strict ordering).

notmart commented 9 years ago

ok, i'll try to add api for that and see if it always avoids to even create and empty svg* is enough

alternatively, the problem may be in the shader/animation, will try to make sure it never tries to animate the first time the thing is created (animation is for icon change, icon state change such as mouseover highlight, and iirc resize, that may well be removed)

star-buck commented 8 years ago

opened at end of 2014, I am finally closing this nearly solved: The expanding part of expanding icon manager or normal taskmanager which opens from pinned launchers still removes the icon first here, displays a throbber and then makes the icon appear, so from that shrinking back to a launcher works (I recall it opened before with overlaying a throbber to the actual icon, but now the launcher icon accidently opr intentionally disappears).

eikehein commented 8 years ago

Swapping the icon for the throbber is intentional, and was done to hide the icon morph because the startup and the window icon may not be the same. However since the logic changes to make the Task Manager prefer app icons over window icons when available, usually they would more often be the same now, avoiding a morph, so this could be reassessed.