Review Request 121411: Don't trigger animation if size changed.

[David Edmundson]

> On Dec. 9, 2014, 6:11 p.m., Kai Uwe Broulik wrote:
> > Wasn't that part of the idea? Having it scale up the pixmap first when resizing and then re-rendering it later?
> 
> Xuetian Weng wrote:
>     1. icon size (the widget size) doesn't change frequently. Usually it only happens when user resize the UI or changes the settings.
>     2. the animation IMHO is for smooth transition between different icons/different icon effect (check volume icon, media player play/pause, hovering with opacity changes), I don't see we need to have animated transition from a scaled icon to the actual correct size icon.
> 
> David Edmundson wrote:
>     Icon size changes may be infrequent but they do happen. It's important that when we resize the panel we don't re-render a tonne of SVGs constantly, we need resizing applets to be smooth.
>     
>     I don't understand where this blurriness is coming from; that implies we're loading it at a wrong size then resizing; if that's the case, lets fix that rather than hide it.
> 
> Sebastian Kügler wrote:
>     During resize, we're rescaling a pixmap instead of re-rendering the SVG for every frame. That can induce blur.
>     
>     Also, rendering an SVG at random sizes van induce blur, since not everything can be aligned to pixels all the time, our SVGs are optimized for "standard sizes".
> 
> Xuetian Weng wrote:
>     The problem is not loading it at wrong size. It is the widget is not resized to correct size yet.
>     The most significant one on plasma here is Device Notifier (When it's popped up at the first time), I print out some debug message at ::geometryChanged
>     ICON QSizeF(1, 86) QSizeF(1, 1) QVariant(QString, "device-notifier")
>     ICON QSizeF(129, 86) QSizeF(1, 86) QVariant(QString, "device-notifier")
>     
>     As you can see, the size changed from 1x1 to 1x86 then to 129x86. So they are valid size, and the blurriness is exposed by the transition animation.
> 
> David Edmundson wrote:
>     Thanks. I can reproduce with the Device Notifier + debug
>     
>     What I'm trying to argue is that it should go directly to 129x86. (or at least be an invalid size till then)
>     
>     Otherwise we're loading an SVG 3 times and not using two of them this patch will hide the visual artifacts of that, but we'll still be wasting a lot of CPU cycles doing something silly.
> 
> Xuetian Weng wrote:
>     emm, seems the 1x1 size comes from lines 67 and 68: http://quickgit.kde.org/?p=plasma-workspace.git&a=blob&h=071705a85aee9ee00a3c88657c93d20544eb5c0f&hb=8b670015582e6b501a2a81a23f38eb5772a36e85&f=applets%2Fsystemtray%2Fplugin%2Fprotocols%2Fplasmoid%2Fplasmoidtask.cpp
>     
>     from current code I don't see an easy way to avoid that... I think there's one resize event can be avoided by call setSize(), but anyway there need to one extra resize.

Good find. We could try setting that to 0x0 and see what happens. We'll save a load of cycles on all sorts of other code that way.

if it goes from 0 to something sensible, it wont' animate as the old m_oldIconPixmap will be null


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121411/#review71665
-----------------------------------------------------------


On Dec. 11, 2014, 7:49 p.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121411/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2014, 7:49 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma, Aleix Pol Gonzalez, and Kai Uwe Broulik.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> Making transition between two different size doesn't make much sense, since repainting is usually happens at that time and it could take some time to finish. And animation need to be stopped if m_animValue is set manually.
> 
> 
> Diffs
> -----
> 
>   src/declarativeimports/core/iconitem.cpp 145a7cd 
> 
> Diff: https://git.reviewboard.kde.org/r/121411/diff/
> 
> 
> Testing
> -------
> 
> Looks fine on tray icon and lock screen, less blurry transition.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20141211/c8e3b31e/attachment-0001.html>