Review Request 115859: Force-allow background contrast while sliding desktops

[Sebastian Kügler]

> On Feb. 18, 2014, 2:15 p.m., Thomas Lübking wrote:
> > kwin/effects/slide/slide.cpp, line 72
> > <https://git.reviewboard.kde.org/r/115859/diff/3/?file=244774#file244774line72>
> >
> >     what about setting and removing this with setting the "slide" flag and catch windowAdded() while slide is true for that matter?
> 
> Sebastian Kügler wrote:
>     This means duplicating the logic which window to transform from ::prePaintWindow. We can do that, but it doesn't make the code less complex and more prone to errors in the future. I suppose setting a flag isn't that expensive?
>     
>     (I can do it, but wanted to make sure it's worth it, first.)
> 
> Thomas Lübking wrote:
>     the logics could be moved into "shouldSlide(EffectWindow*)"
>     it's not a flag - setData() operates on a QHash where non-trivial writing has some overhead (tree balancing)
>     
>     also (and at least) ::postPaintWindow should shortcut on "!slide"
> 
> Sebastian Kügler wrote:
>     Okay. :)
>     
>     Changing the patch to do that and your other comments. (Thanks for reviewing.)
> 
> Thomas Lübking wrote:
>     Isn't this more like https://git.reviewboard.kde.org/r/115857/ ?
>     As mentioned there, i do not believe that aligning effect runtimes is a good way to prevent artifacts/flicker etc. - esp. not by a "magic" value.
>     
>     Also i'm not sure why alignment would be required here.
>     The only clash would be if a popup hides while the desktop is switched.
>     Ie. the slide effect sets the WindowForceBackgroundContrastRole and withdraws it, while the slidingpopup effect would prefer to keep it a little longer, right?
>     
>     In that case the role could be interpreted as integer, rather than bool ("still required? still? still? no more? 'key.") ie. keep bg contrast processing as long as the data().toInt() > 0

It's not about aligning on a technical level. It just felt weird to have it appear rather fast and vanish rather slow. I've excluded this bit from this review, anyway.

The sliding popup can't be sensibly debugged for this issue right now, since it's way to erratic in its behavior (that *is* the flickering from the other RR you linked).


- Sebastian


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


On Feb. 19, 2014, 12:06 p.m., Sebastian Kügler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115859/
> -----------------------------------------------------------
> 
> (Updated Feb. 19, 2014, 12:06 p.m.)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> -------
> 
> Force-allow background contrast while sliding
> 
> When the sliding effect is active, the background blur of the panel
> would be disabled unless forced with WindowForceBackgroundContrastRole,
> which is actually what we want: during sliding, the backgroundcontrast
> effect would otherwise be disabled, leading to the panel and popups
> flickering between contrast enabled and disabled.
> 
> With this patch, the panel keeps its coloring during desktop changes.
> 
> 
> Diffs
> -----
> 
>   kwin/effects/slide/slide.h c8e0a84 
>   kwin/effects/slide/slide.cpp 8ecb2a6 
> 
> Diff: https://git.reviewboard.kde.org/r/115859/diff/
> 
> 
> Testing
> -------
> 
> Switched desktops, panel keeps color, fullscreen video player doesn't seem to be affected negatively, everything still works as expected, except that the flickering in Plasma Dialogs is gone.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

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