Bug 321897 - Sheet effect doesn't set WindowClosedGrabRole and collides with fade effect on close for different duration
Summary: Sheet effect doesn't set WindowClosedGrabRole and collides with fade effect o...
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Plasma
Component: effects-various (show other bugs)
Version: 4.10.4
Platform: Ubuntu Linux
: NOR minor
Target Milestone: ---
Assignee: KWin default assignee
URL: https://git.reviewboard.kde.org/r/111...
Keywords:
: 322600 346442 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-07-03 10:57 UTC by masterdany88
Modified: 2018-08-09 09:04 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:
thomas.luebking: ReviewRequest+


Attachments
qdbus org.kde.kwin /KWin supportInformation (4.92 KB, text/plain)
2013-09-25 17:40 UTC, Nikos Chantziaras
Details
Updated patch (1.73 KB, patch)
2013-09-25 21:11 UTC, Thomas Lübking
Details

Note You need to log in before you can comment on or make changes to this bug.
Description masterdany88 2013-07-03 10:57:15 UTC
I dont know how that animation effect is originally called, in polish tranlation its just "Arkusz". Version of that effect is 0.1.0. This problem occur in few earliers realeses. Effect is apply to modal windows that are opening from "parent".
The "child" (modal) window fall down correctly. It is all the time at the top. But when I am closing it, it goes behind the parent window, then goes animation, but hard to see it.

Reproducible: Always

Steps to Reproduce:
1.Open modal window (like window with information or setting of animation effect)
2.close it
3.
Actual Results:  
I am unable to watch animation of flying away  to up of screen, cause it is behind main window

Expected Results:  
Animation should be at the top off windows, and after animation it should get behind the main window.
Comment 1 masterdany88 2013-07-03 11:18:22 UTC
Here is some short video where problem occur
wrong behaviour of "arkusz" effect
Comment 2 masterdany88 2013-07-03 11:18:50 UTC
http://youtu.be/4mk5Zpf_rKw
Comment 3 Thomas Lübking 2013-07-03 12:15:29 UTC
LC_ALL="C" kcmshell4 kwincompositing

As Workaround, lower the sheet time (default is 500ms) and raise the fade out time (default is 150ms, it's in /usr/share/apps/kwin/effects/kwin4_effect_fade/contents/code/main.js) to match at eg. 300ms

@Martin
I guess the AnimationEffect should keep done Deleted's in an extra list and at their target value (until they become deleted by own or foreign unreferencing)
Comment 4 Martin Flöser 2013-07-03 12:36:23 UTC
> I guess the AnimationEffect should keep done Deleted's in an extra list and
> at their target value (until they become deleted by own or foreign
> unreferencing)
sounds like a reasonable approach. Would be nice to know how many effects are 
operating on it, so that we don't copy the data needlessly into another list. 
Maybe we should export the refCount on the window?
Comment 5 Thomas Lübking 2013-07-03 14:52:15 UTC
(In reply to comment #4)
> sounds like a reasonable approach.
That on a second thought will introduce another issue, though:
The fade will run in 150% ie only during 30% of the sheet effect - which then basically does not happen :-\

Regardless of keeping deleted windows at their target value to prevent ugly flicker, we should think about aligning closing times.
Either trivially by just picking a constant time for all effects or by some expansion system, ie let effects set a (qMax(old, new)) property role of "MinimumCloseEffectTime" (when the window is added..) and let them use that when animating the close.

Ie. since a complex 3D transformation needs longer visualization, fade would expand to that value on demand.


> Would be nice to know how many effects are operating on it
Yes, but rather for debugging ;-)

> so that we don't copy the data needlessly into another list. 
Given the amount of windows that one would likely operate on, one would end up with a QList<QPair> and the way QList is implemented, that should not be much overhead at all.

The problem here is the lack of prediction, ie. if you've three effects, all releasing the window at the same time, two would still copy the data.

The way the scripted effects are implemented (one effect per script) we probably have too sparse animation lists anyway.

I thought about sharing the Private and have flags on whether ::prePaintScreen etc. already have been handled.

The only pitfall here is for inheriting classes which no longer could rely on execution order when calling the inherited function, ie.

void MyEffect::prePaintScreen( ScreenPrePaintData& data, int time )
{
   foo();
   AnimationEffect::prePaintScreen(data, time);
   bar();
}

may not act as expected because AnimationEffect::prePaintScreen(data, time); had been globally executed before.

No idea how much of a problem that would be.
Comment 6 Thomas Lübking 2013-07-20 11:57:50 UTC
*** Bug 322600 has been marked as a duplicate of this bug. ***
Comment 7 Thomas Lübking 2013-07-20 21:51:29 UTC
OP illustrating video:
http://youtu.be/4mk5Zpf_rKw
Comment 8 Nikos Chantziaras 2013-07-28 15:04:10 UTC
Bug persists with 4.10.97.

Btw, the bug was not there in 4.10.4 and older versions. At least the one I reported before it was marked as a dupe of this one. The steps to reproduce the one I'm experiencing with 4.11 but not with older versions are:

1. Run KWrite
2. Type something
3. Quit and click "Discard" in the save dialog. This will result in the KWrite window fading then pop-up and then pop-out again.
Comment 9 Thomas Lübking 2013-07-28 15:12:37 UTC
(In reply to comment #8)
> Bug persists with 4.10.97.
Because the RR commit obviously was not pushed.
We'll resolve this differently.

> Btw, the bug was not there in 4.10.4 and older versions.
Yes, we know what's causing this and how to resolve the bug.
Comment 10 Thomas Lübking 2013-09-24 19:58:05 UTC
Git commit b6390fd44c6efa7f56e486e386432c806d3f2c8e by Thomas Lübking.
Committed on 30/07/2013 at 14:09.
Pushed by luebking into branch 'KDE/4.11'.

extend fadeout animation duration to 600ms

compensated by a QuartOut shape, this keeps the
effect alive while the sheet or similar effects run
without much visual stretch

Covers issue until there's AnimationEffect::determine()
semi Fixed in 4.11.2
REVIEW: 111798

M  +2    -2    kwin/effects/fade/package/contents/code/main.js

http://commits.kde.org/kde-workspace/b6390fd44c6efa7f56e486e386432c806d3f2c8e
Comment 11 Nikos Chantziaras 2013-09-24 23:32:21 UTC
(In reply to comment #10)
> http://commits.kde.org/kde-workspace/b6390fd44c6efa7f56e486e386432c806d3f2c8e

Just applied this on 4.11.1. It doesn't fix the issue.
Comment 12 Thomas Lübking 2013-09-24 23:45:07 UTC
(In reply to comment #11)
> Just applied this on 4.11.1. It doesn't fix the issue.

Then you perceive something completely different.
You did restart kwin after aplpying the "patch" (to the proper javascript resource), did you?
Comment 13 Nikos Chantziaras 2013-09-24 23:53:02 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > Just applied this on 4.11.1. It doesn't fix the issue.
> 
> Then you perceive something completely different.
> You did restart kwin after aplpying the "patch" (to the proper javascript
> resource), did you?

Yep. I logged out and also restarted KDM. The specific problem I'm talking about is the one in comment 8.
Comment 14 Thomas Lübking 2013-09-25 08:56:27 UTC
that's indeed different since the kwrite (editor, i assume?) window is no dialog, thus not affected by the sheet effect. might fall into same category, though.
please attach the output of "qdbus org.kde.kwin /KWin supportInformation"
Comment 15 Nikos Chantziaras 2013-09-25 17:40:24 UTC
Created attachment 82490 [details]
qdbus org.kde.kwin /KWin supportInformation

(In reply to comment #14)
> that's indeed different since the kwrite (editor, i assume?) window is no
> dialog, thus not affected by the sheet effect. might fall into same
> category, though.
> please attach the output of "qdbus org.kde.kwin /KWin supportInformation"

Attached.

Should bug 322600 be reopened?
Comment 16 Thomas Lübking 2013-09-25 18:03:52 UTC
(In reply to comment #15)

> Should bug 322600 be reopened?
Ah, that one.

Not until we figured why this still occurs for you - this bug here is not resolved "fixed" anyway.

The problem "was" that the dialogparent effect has an animationtime of 300ms, where the fade effect ran only 150ms - therefore it was reasonable an reproducible that the running dialogparent effect would cause the window to re-appear on full opacity (when the fade effect had stopped and didn't manipulate the windows opacity anymore but the dialogparent effect kept the closed window alive)

This shall no longer be the reason (the fade effect now runs 600ms *BY DEFAULT* and keeps the window nearly invisible for the last 400ms) and the issue is neither any longer reproducible here.

a) supportInformation does not print settings for scripted effects, therefore please inspect ~/.kde/share/config/kwinrc for a custom value for "FadeOutTime" - if one's there, the patch has virtually no impact.

b) Please make sure that there's no local fade effect shadowing the patched one, try altering /usr/share/apps/kwin/effects/kwin4_effect_fade/contents/code/main.js  in a way that you will notice the extended fade time (revert the patch and just set the default paramter for "FadeOutTime" to 3000 - you should be able to notice the extended fade time ;-)


@Martin
I guess the patch could/should be improved by multiplying the read "FadeOutTime" by 4 instead of manipulating just the default.
Comment 17 Nikos Chantziaras 2013-09-25 18:25:25 UTC
(In reply to comment #16)
> (In reply to comment #15)
>  [...]
> This shall no longer be the reason (the fade effect now runs 600ms *BY
> DEFAULT* and keeps the window nearly invisible for the last 400ms) and the
> issue is neither any longer reproducible here.
> 
> a) supportInformation does not print settings for scripted effects,
> therefore please inspect ~/.kde/share/config/kwinrc for a custom value for
> "FadeOutTime" - if one's there, the patch has virtually no impact.

Hm, FadeOutTime is not set anywhere in my kwinrc.


> b) Please make sure that there's no local fade effect shadowing the patched
> one, try altering
> /usr/share/apps/kwin/effects/kwin4_effect_fade/contents/code/main.js  in a
> way that you will notice the extended fade time (revert the patch and just
> set the default paramter for "FadeOutTime" to 3000 - you should be able to
> notice the extended fade time ;-)

If I do:

    fadeOutTime = animationTime(effect.readConfig("FadeOutTime", 3000));

then the problem is still there. It seems like readConfig() does load a setting from somewhere. If not from kwinrc, then from where?

If I force it:

    fadeOutTime = 3000;

then indeed it works. I've not set it to 300, which is just enough to get rid of the pop-back-in issue here.
Comment 18 Thomas Lübking 2013-09-25 18:47:12 UTC
somewhere out of these paths:
         kde4-config --path config

Keeps the bug a dupe, though.
I'll just push a fix multiplying the configured value (so thanks for bringing this up)
Comment 19 Nikos Chantziaras 2013-09-25 19:03:48 UTC
Wouldn't something like this make more sense:

    fadeOutTime = animationTime(effect.readConfig("FadeOutTime", 310));
    if (fadeOutTime < 310) {
        fadeOutTime = 310;
    }

That is, just make sure it's higher than 300?
Comment 20 Thomas Lübking 2013-09-25 19:51:07 UTC
(In reply to comment #19)
> Wouldn't something like this make more sense:

This here will eb the actual fix:
https://git.reviewboard.kde.org/r/111622/

Stretching the effect (which needs a minimum of 600ms because of this bug here) and compensating that by an altered animation curve is just a workaround.

Setting an long linear animation is no option as it makes fading feel slow and since the minimum required visual time is 150ms (= 600/4) is rather low, this shall suffice to shadow the problem until we can add the real fix.
Comment 21 Thomas Lübking 2013-09-25 21:11:19 UTC
Created attachment 82491 [details]
Updated patch

It's been actually worse: the patch actually no longer worked as expected and I always tried on the determine variant.... :-\

Please try the new attachment.

@Martin
As soon as the new patch is confirmed to work i intend to push it.
Comment 22 Martin Flöser 2013-09-26 06:28:13 UTC
> @Martin
> As soon as the new patch is confirmed to work i intend to push it.
*nod*
Comment 23 Nikos Chantziaras 2013-09-26 17:32:05 UTC
(In reply to comment #21)
> Created attachment 82491 [details]
> Updated patch
> [...]
> Please try the new attachment.

Works fine now. The window stays faded out.
Comment 24 Thomas Lübking 2013-09-26 17:42:09 UTC
Git commit 28231bfe0080b79c85e7bed03eaee81c5ea809e4 by Thomas Lübking.
Committed on 25/09/2013 at 19:57.
Pushed by luebking into branch 'KDE/4.11'.

workaround bug #321897 by multiplying fadeout time

with 4 instead of just altering the default value
It seems the value is actually written for some ppl.

Worse: former patch didn't actually work as expected
anymore

M  +10   -2    kwin/effects/fade/package/contents/code/main.js

http://commits.kde.org/kde-workspace/28231bfe0080b79c85e7bed03eaee81c5ea809e4
Comment 25 Thomas Lübking 2013-09-26 17:44:14 UTC
FTR:
it seems the default parameter for effect.readConfig() is ignored and probably the config/main.xml entry used.
Comment 26 Nikos Chantziaras 2013-09-26 17:50:25 UTC
I just noticed that this introduces a glitch in the application launcher. When opening the launcher and starting an application, the fadeout animation of the launcher is all messed up. It pops out and in again several times per second while fading out.
Comment 27 Thomas Lübking 2013-09-26 18:05:07 UTC
(In reply to comment #26)
> I just noticed that this introduces a glitch in the application launcher.
What launcher in partcular?
Kickoff and krunner perform a slide-in (so they should be ignored by the fade effect)
Comment 28 Nikos Chantziaras 2013-09-26 18:08:39 UTC
(In reply to comment #27)
> What launcher in partcular?
> Kickoff and krunner perform a slide-in (so they should be ignored by the
> fade effect)

Kickoff. I have disabled the slide effect and am using fading for it.
Comment 29 Thomas Lübking 2013-09-26 18:49:34 UTC
(In reply to comment #28)

> Kickoff. I have disabled the slide effect and am using fading for it.

I bet on bug #307112 which becomes more visible due to the longer lasting animation.

In the blur effect config, disable "cache intermediate blur results" - the flicker is then hopefuly gone?
Comment 30 Nikos Chantziaras 2013-09-26 19:11:23 UTC
(In reply to comment #29)
> I bet on bug #307112 which becomes more visible due to the longer lasting
> animation.
> 
> In the blur effect config, disable "cache intermediate blur results" - the
> flicker is then hopefuly gone?

Yep, that's it. Disabling that option gets rid of the flicker.
Comment 31 S. Christian Collins 2013-11-02 15:43:47 UTC
Here's a video of this bug happening with the dashboard: https://dl.dropboxusercontent.com/u/8126161/fade-out_flicker.ogv

When the dashboard fades out, it flickers badly. The graphical corruption in the video (off-color squares) are only in the video, btw. Disabling "Save intermediate rendering results." in the blur effect settings resolves this issue for me as well.
Comment 32 S. Christian Collins 2013-11-02 15:45:21 UTC
Oh, forgot to mention this is with KDE 4.11.2 and confirmed on three different systems.
Comment 33 Thomas Lübking 2013-11-02 17:48:01 UTC
Please see bug #307112 - it's rather unrelated here (stretching the animation time to "slow" or so would have caused the same problem to be emphasized)
Comment 34 Thomas Lübking 2015-04-21 21:28:26 UTC
*** Bug 346442 has been marked as a duplicate of this bug. ***
Comment 35 Vlad Zahorodnii 2018-08-09 09:04:13 UTC
The sheet effect grabs modal windows and the fade effect honors grab roles. So, marking this bug as fixed.

If we want modal windows to fade in/out with the sheet effect, then the latter should fade them. Otherwise, effects don't have clear domain boundaries and we're screwed.