[effects/dialogparent] Fix flickering of parent windows

Summary:
If a modal window is closed and some alternative effect that animates
the disappearing of windows is enabled(e.g. the Glide effect, or the
Scale effect), the Dialog Parent effect can cause flickering of the
parent window because its animation duration doesn't match duration of
those alternative effects.

Also, if the Fade effect, the Glide effect, and the Scale effect are
disabled, the Dialog Parent will keep the parent window alive for no
good reason.

This change addresses that problem by adding keepAlive property to
`animate` function so scripted effects have more control over lifetime
of animated windows.

If both a modal window and its parent window are closed at the same time
(and there is no effect that animates the disappearing of windows), the
Dialog Parent will stop immediately(because windowDeleted will be
emitted right after windowClosed signal).

If both a modal window and its parent window are closed at the same time
(and there is effect that animates the disappearing of windows), the
Dialog Parent won't reference the latter window. Thus, it won't cause
flickering. I.e. it will "passively" animate parent windows.

BUG: 355036
FIXED-IN: 5.15.0

Reviewers: #kwin, davidedmundson

Reviewed By: #kwin, davidedmundson

Subscribers: davidedmundson, kwin

Tags: #kwin

Differential Revision: https://phabricator.kde.org/D14919

autotests/integration/effects/scripted_effects_test.cpp

@@ -23,7 +23,7 @@ along with this program.  If not, see <http:// www.gnu.org/licenses/>.
 
 #include "composite.h"
 #include "cursor.h"
-#include "cursor.h"
+#include "deleted.h"
 #include "effect_builtins.h"
 #include "effectloader.h"
 #include "effects.h"
@@ -68,6 +68,9 @@ private Q_SLOTS:
     void testScreenEdgeTouch();
     void testFullScreenEffect_data();
     void testFullScreenEffect();
+    void testKeepAlive_data();
+    void testKeepAlive();
+
 private:
     ScriptedEffect *loadEffect(const QString &name);
 };
@@ -132,6 +135,7 @@ void ScriptedEffectsTest::initTestCase()
 {
     qRegisterMetaType<KWin::ShellClient*>();
     qRegisterMetaType<KWin::AbstractClient*>();
+    qRegisterMetaType<KWin::Deleted*>();
     qRegisterMetaType<KWin::Effect*>();
     QSignalSpy workspaceCreatedSpy(kwinApp(), &Application::workspaceCreated);
     QVERIFY(workspaceCreatedSpy.isValid());
@@ -421,6 +425,62 @@ void ScriptedEffectsTest::testFullScreenEffect()
     QCOMPARE(effects->activeFullScreenEffect(), nullptr);
 }
 
+void ScriptedEffectsTest::testKeepAlive_data()
+{
+    QTest::addColumn<QString>("file");
+    QTest::addColumn<bool>("keepAlive");
+
+    QTest::newRow("keep")        << "keepAliveTest"         << true;
+    QTest::newRow("don't keep")  << "keepAliveTestDontKeep" << false;
+}
+
+void ScriptedEffectsTest::testKeepAlive()
+{
+    // this test checks whether closed windows are kept alive
+    // when keepAlive property is set to true(false)
+
+    QFETCH(QString, file);
+    QFETCH(bool, keepAlive);
+
+    auto *effect = new ScriptedEffectWithDebugSpy;
+    QSignalSpy effectOutputSpy(effect, &ScriptedEffectWithDebugSpy::testOutput);
+    QVERIFY(effectOutputSpy.isValid());
+    QVERIFY(effect->load(file));
+
+    // create a window
+    using namespace KWayland::Client;
+    auto *surface = Test::createSurface(Test::waylandCompositor());
+    QVERIFY(surface);
+    auto *shellSurface = Test::createXdgShellV6Surface(surface, surface);
+    QVERIFY(shellSurface);
+    auto *c = Test::renderAndWaitForShown(surface, QSize(100, 50), Qt::blue);
+    QVERIFY(c);
+    QCOMPARE(workspace()->activeClient(), c);
+
+    // no active animations at the beginning
+    QCOMPARE(effect->state().count(), 0);
+
+    // trigger windowClosed signal
+    surface->deleteLater();
+    QVERIFY(effectOutputSpy.count() == 1 || effectOutputSpy.wait());
+
+    if (keepAlive) {
+        QCOMPARE(effect->state().count(), 1);
+
+        QTest::qWait(500);
+        QCOMPARE(effect->state().count(), 1);
+
+        QTest::qWait(500 + 100); // 100ms is extra safety margin
+        QCOMPARE(effect->state().count(), 0);
+    } else {
+        // the test effect doesn't keep the window alive, so it should be
+        // removed immediately
+        QSignalSpy deletedRemovedSpy(workspace(), &Workspace::deletedRemoved);
+        QVERIFY(deletedRemovedSpy.isValid());
+        QVERIFY(deletedRemovedSpy.count() == 1 || deletedRemovedSpy.wait(100)); // 100ms is less than duration of the animation
+        QCOMPARE(effect->state().count(), 0);
+    }
+}
 
 WAYLANDTEST_MAIN(ScriptedEffectsTest)
 #include "scripted_effects_test.moc"

autotests/integration/effects/scripts/keepAliveTest.js

@@ -0,0 +1,13 @@
+"use strict";
+
+effects.windowClosed.connect(function (window) {
+    animate({
+        window: window,
+        type: Effect.Scale,
+        duration: 1000,
+        from: 1.0,
+        to: 0.0
+        // by default, keepAlive is set to true
+    });
+    sendTestResponse("triggered");
+});

autotests/integration/effects/scripts/keepAliveTestDontKeep.js

@@ -0,0 +1,13 @@
+"use strict";
+
+effects.windowClosed.connect(function (window) {
+    animate({
+        window: window,
+        type: Effect.Scale,
+        duration: 1000,
+        from: 1.0,
+        to: 0.0,
+        keepAlive: false
+    });
+    sendTestResponse("triggered");
+});

effects/dialogparent/package/contents/code/main.js

@@ -77,6 +77,7 @@ var dialogParentEffect = {
             animate({
                 window: w,
                 duration: dialogParentEffect.duration,
+                keepAlive: false,
                 animations: [{
                     type: Effect.Saturation,
                     from: 0.4,

libkwineffects/anidata.cpp

@@ -3,6 +3,7 @@
  This file is part of the KDE project.
 
 Copyright (C) 2011 Thomas Lübking <thomas.luebking@web.de>
+Copyright (C) 2018 Vlad Zagorodniy <vladzzag@gmail.com>
 
 This program is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License as published by
@@ -41,6 +42,17 @@ FullScreenEffectLock::~FullScreenEffectLock()
     effects->setActiveFullScreenEffect(nullptr);
 }
 
+KeepAliveLock::KeepAliveLock(EffectWindow *w)
+    : m_window(w)
+{
+    m_window->refWindow();
+}
+
+KeepAliveLock::~KeepAliveLock()
+{
+    m_window->unrefWindow();
+}
+
 AniData::AniData()
  : attribute(AnimationEffect::Opacity)
  , customCurve(0) // Linear
@@ -51,12 +63,13 @@ AniData::AniData()
  , windowType((NET::WindowTypeMask)0)
  , waitAtSource(false)
  , keepAtTarget(false)
+ , keepAlive(true)
 {
 }
 
 AniData::AniData(AnimationEffect::Attribute a, int meta_, int ms, const FPx2 &to_,
                  QEasingCurve curve_, int delay, const FPx2 &from_, bool waitAtSource_, bool keepAtTarget_,
-                 FullScreenEffectLockPtr fullScreenEffectLock_)
+                 FullScreenEffectLockPtr fullScreenEffectLock_, bool keepAlive)
  : attribute(a)
  , curve(curve_)
  , from(from_)
@@ -69,6 +82,7 @@ AniData::AniData(AnimationEffect::Attribute a, int meta_, int ms, const FPx2 &to
  , fullScreenEffectLock(fullScreenEffectLock_)
  , waitAtSource(waitAtSource_)
  , keepAtTarget(keepAtTarget_)
+ , keepAlive(keepAlive)
 {
 }
 

libkwineffects/anidata_p.h

@@ -3,6 +3,7 @@
  This file is part of the KDE project.
 
 Copyright (C) 2011 Thomas Lübking <thomas.luebking@web.de>
+Copyright (C) 2018 Vlad Zagorodniy <vladzzag@gmail.com>
 
 This program is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License as published by
@@ -40,11 +41,28 @@ private:
 };
 typedef QSharedPointer<FullScreenEffectLock> FullScreenEffectLockPtr;
 
+/**
+ * Keeps windows alive during animation after they got closed
+ **/
+class KeepAliveLock
+{
+public:
+    KeepAliveLock(EffectWindow *w);
+    ~KeepAliveLock();
+
+private:
+    EffectWindow *m_window;
+    Q_DISABLE_COPY(KeepAliveLock)
+};
+typedef QSharedPointer<KeepAliveLock> KeepAliveLockPtr;
+
 class KWINEFFECTS_EXPORT AniData {
 public:
     AniData();
     AniData(AnimationEffect::Attribute a, int meta, int ms, const FPx2 &to,
-            QEasingCurve curve, int delay, const FPx2 &from, bool waitAtSource, bool keepAtTarget = false, FullScreenEffectLockPtr=FullScreenEffectLockPtr());
+            QEasingCurve curve, int delay, const FPx2 &from, bool waitAtSource,
+            bool keepAtTarget = false, FullScreenEffectLockPtr=FullScreenEffectLockPtr(),
+            bool keepAlive = true);
     inline void addTime(int t) { time += t; }
     inline bool isOneDimensional() const {
         return from[0] == from[1] && to[0] == to[1];
@@ -62,6 +80,8 @@ public:
     NET::WindowTypeMask windowType;
     QSharedPointer<FullScreenEffectLock> fullScreenEffectLock;
     bool waitAtSource, keepAtTarget;
+    bool keepAlive;
+    KeepAliveLockPtr keepAliveLock;
 };
 
 } // namespace

libkwineffects/kwinanimationeffect.cpp

@@ -44,7 +44,6 @@ public:
         m_justEndedAnimation = 0;
     }
     AnimationEffect::AniMap m_animations;
-    EffectWindowList m_zombies;
     static quint64 m_animCounter;
     quint64 m_justEndedAnimation; // protect against cancel
     QWeakPointer<FullScreenEffectLock> m_fullScreenEffectLock;
@@ -217,7 +216,7 @@ void AnimationEffect::validate(Attribute a, uint &meta, FPx2 *from, FPx2 *to, co
     }
 }
 
-quint64 AnimationEffect::p_animate( EffectWindow *w, Attribute a, uint meta, int ms, FPx2 to, QEasingCurve curve, int delay, FPx2 from, bool keepAtTarget, bool fullScreenEffect)
+quint64 AnimationEffect::p_animate( EffectWindow *w, Attribute a, uint meta, int ms, FPx2 to, QEasingCurve curve, int delay, FPx2 from, bool keepAtTarget, bool fullScreenEffect, bool keepAlive)
 {
     const bool waitAtSource = from.isValid();
     validate(a, meta, &from, &to, w);
@@ -248,7 +247,7 @@ quint64 AnimationEffect::p_animate( EffectWindow *w, Attribute a, uint meta, int
             fullscreen = d->m_fullScreenEffectLock.toStrongRef();
         }
     }
-    it->first.append(AniData(a, meta, ms, to, curve, delay, from, waitAtSource, keepAtTarget, fullscreen));
+    it->first.append(AniData(a, meta, ms, to, curve, delay, from, waitAtSource, keepAtTarget, fullscreen, keepAlive));
     quint64 ret_id = ++d->m_animCounter;
     it->first.last().id = ret_id;
     it->second = QRect();
@@ -298,11 +297,6 @@ bool AnimationEffect::cancel(quint64 animationId)
             if (anim->id == animationId) {
                 entry->first.erase(anim); // remove the animation
                 if (entry->first.isEmpty()) { // no other animations on the window, release it.
-                    const int i = d->m_zombies.indexOf(entry.key());
-                    if ( i > -1 ) {
-                        d->m_zombies.removeAt( i );
-                        entry.key()->unrefWindow();
-                    }
                     d->m_animations.erase(entry);
                 }
                 if (d->m_animations.isEmpty())
@@ -378,11 +372,6 @@ void AnimationEffect::prePaintScreen( ScreenPrePaintData& data, int time )
             }
         }
         if (entry->first.isEmpty()) {
-            const int i = d->m_zombies.indexOf(entry.key());
-            if ( i > -1 ) {
-                d->m_zombies.removeAt( i );
-                entry.key()->unrefWindow();
-            }
             data.paint |= entry->second;
 //             d->m_damageDirty = true; // TODO likely no longer required
             entry = d->m_animations.erase(entry);
@@ -397,11 +386,6 @@ void AnimationEffect::prePaintScreen( ScreenPrePaintData& data, int time )
     // janitorial...
     if (d->m_animations.isEmpty()) {
         disconnectGeometryChanges();
-        if (!d->m_zombies.isEmpty()) { // this is actually not supposed to happen
-            foreach (EffectWindow *w, d->m_zombies)
-                w->unrefWindow();
-            d->m_zombies.clear();
-        }
     }
 
     effects->prePaintScreen(data, time);
@@ -924,16 +908,33 @@ void AnimationEffect::_expandedGeometryChanged(KWin::EffectWindow *w, const QRec
 void AnimationEffect::_windowClosed( EffectWindow* w )
 {
     Q_D(AnimationEffect);
-    if (d->m_animations.contains(w) && !d->m_zombies.contains(w)) {
+
-        w->refWindow();
+    auto it = d->m_animations.find(w);
-        d->m_zombies << w;
+    if (it == d->m_animations.end()) {
+        return;
+    }
+
+    KeepAliveLockPtr keepAliveLock;
+
+    QList<AniData> &animations = (*it).first;
+    for (auto animationIt = animations.begin();
+            animationIt != animations.end();
+            ++animationIt) {
+        if (!(*animationIt).keepAlive) {
+            continue;
+        }
+
+        if (keepAliveLock.isNull()) {
+            keepAliveLock = KeepAliveLockPtr::create(w);
+        }
+
+        (*animationIt).keepAliveLock = keepAliveLock;
     }
 }
 
 void AnimationEffect::_windowDeleted( EffectWindow* w )
 {
     Q_D(AnimationEffect);
-    d->m_zombies.removeAll( w ); // TODO this line is a workaround for a bug in KWin 4.8.0 & 4.8.1
     d->m_animations.remove( w );
 }
 

libkwineffects/kwinanimationeffect.h

@@ -162,18 +162,19 @@ protected:
      * @param delay - When the animation will start compared to "now" (the window will remain at the "from" position until then)
      * @param from - The starting value, the default is invalid, ie. the attribute for the window is not transformed in the beginning
      * @param fullScreen - Sets this effect as the active full screen effect for the duration of the animation
+     * @param keepAlive - Whether closed windows should be kept alive during animation
      * @return an ID that you can use to cancel a running animation
      */
-    quint64 animate( EffectWindow *w, Attribute a, uint meta, int ms, FPx2 to, QEasingCurve curve = QEasingCurve(), int delay = 0, FPx2 from = FPx2(), bool fullScreen = false)
+    quint64 animate( EffectWindow *w, Attribute a, uint meta, int ms, FPx2 to, QEasingCurve curve = QEasingCurve(), int delay = 0, FPx2 from = FPx2(), bool fullScreen = false, bool keepAlive = true)
-    { return p_animate(w, a, meta, ms, to, curve, delay, from, false, fullScreen); }
+    { return p_animate(w, a, meta, ms, to, curve, delay, from, false, fullScreen, keepAlive); }
 
     /**
      * Equal to ::animate() with one important difference:
      * The target value for the attribute is kept until you ::cancel() this animation
      * @return an ID that you need to use to cancel this manipulation
      */
-    quint64 set( EffectWindow *w, Attribute a, uint meta, int ms, FPx2 to, QEasingCurve curve = QEasingCurve(), int delay = 0, FPx2 from = FPx2(), bool fullScreen = false)
+    quint64 set( EffectWindow *w, Attribute a, uint meta, int ms, FPx2 to, QEasingCurve curve = QEasingCurve(), int delay = 0, FPx2 from = FPx2(), bool fullScreen = false, bool keepAlive = true)
-    { return p_animate(w, a, meta, ms, to, curve, delay, from, true, fullScreen); }
+    { return p_animate(w, a, meta, ms, to, curve, delay, from, true, fullScreen, keepAlive); }
 
     /**
      * this allows to alter the target (but not type or curve) of a running animation
@@ -212,7 +213,7 @@ protected:
     AniMap state() const;
 
 private:
-    quint64 p_animate(EffectWindow *w, Attribute a, uint meta, int ms, FPx2 to, QEasingCurve curve, int delay, FPx2 from, bool keepAtTarget, bool fullScreenEffect);
+    quint64 p_animate(EffectWindow *w, Attribute a, uint meta, int ms, FPx2 to, QEasingCurve curve, int delay, FPx2 from, bool keepAtTarget, bool fullScreenEffect, bool keepAlive);
     QRect clipRect(const QRect &windowRect, const AniData&) const;
     void clipWindow(const EffectWindow *, const AniData &, WindowQuadList &) const;
     float interpolated( const AniData&, int i = 0 ) const;

scripting/scriptedeffect.cpp

@@ -103,7 +103,14 @@ QScriptValue kwinUnregisterTouchScreenEdge(QScriptContext *context, QScriptEngin
 }
 
 struct AnimationSettings {
-    enum { Type = 1<<0, Curve = 1<<1, Delay = 1<<2, Duration = 1<<3, FullScreen = 1<<4 };
+    enum {
+        Type       = 1<<0,
+        Curve      = 1<<1,
+        Delay      = 1<<2,
+        Duration   = 1<<3,
+        FullScreen = 1<<4,
+        KeepAlive  = 1<<5
+    };
     AnimationEffect::Attribute type;
     QEasingCurve::Type curve;
     FPx2 from;
@@ -113,6 +120,7 @@ struct AnimationSettings {
     uint set;
     uint metaData;
     bool fullScreenEffect;
+    bool keepAlive;
 };
 
 AnimationSettings animationSettingsFromObject(QScriptValue &object)
@@ -164,6 +172,14 @@ AnimationSettings animationSettingsFromObject(QScriptValue &object)
         settings.fullScreenEffect = false;
     }
 
+    QScriptValue keepAlive = object.property(QStringLiteral("keepAlive"));
+    if (keepAlive.isValid() && keepAlive.isBool()) {
+        settings.keepAlive = keepAlive.toBool();
+        settings.set |= AnimationSettings::KeepAlive;
+    } else {
+        settings.keepAlive = true;
+    }
+
     return settings;
 }
 
@@ -230,6 +246,9 @@ QList<AnimationSettings> animationSettings(QScriptContext *context, ScriptedEffe
                 if (!(s.set & AnimationSettings::FullScreen)) {
                     s.fullScreenEffect = settings.at(0).fullScreenEffect;
                 }
+                if (!(s.set & AnimationSettings::KeepAlive)) {
+                    s.keepAlive = settings.at(0).keepAlive;
+                }
 
                 s.metaData = 0;
                 typedef QMap<AnimationEffect::MetaType, QString> MetaTypeMap;
@@ -298,7 +317,8 @@ QScriptValue kwinEffectAnimate(QScriptContext *context, QScriptEngine *engine)
                                     setting.metaData,
                                     setting.curve,
                                     setting.delay,
-                                    setting.fullScreenEffect));
+                                    setting.fullScreenEffect,
+                                    setting.keepAlive));
         ++i;
     }
     return array;
@@ -328,7 +348,9 @@ QScriptValue kwinEffectSet(QScriptContext *context, QScriptEngine *engine)
                                setting.from,
                                setting.metaData,
                                setting.curve,
-                               setting.delay));
+                               setting.delay,
+                               setting.fullScreenEffect,
+                               setting.keepAlive));
     }
 
     return engine->newVariant(animIds);
@@ -610,24 +632,24 @@ void ScriptedEffect::signalHandlerException(const QScriptValue &value)
     }
 }
 
-quint64 ScriptedEffect::animate(KWin::EffectWindow* w, KWin::AnimationEffect::Attribute a, int ms, KWin::FPx2 to, KWin::FPx2 from, uint metaData, int curve, int delay, bool fullScreen)
+quint64 ScriptedEffect::animate(KWin::EffectWindow* w, KWin::AnimationEffect::Attribute a, int ms, KWin::FPx2 to, KWin::FPx2 from, uint metaData, int curve, int delay, bool fullScreen, bool keepAlive)
 {
     QEasingCurve qec;
     if (curve < QEasingCurve::Custom)
         qec.setType(static_cast<QEasingCurve::Type>(curve));
     else if (curve == GaussianCurve)
         qec.setCustomType(qecGaussian);
-    return AnimationEffect::animate(w, a, metaData, ms, to, qec, delay, from, fullScreen);
+    return AnimationEffect::animate(w, a, metaData, ms, to, qec, delay, from, fullScreen, keepAlive);
 }
 
-quint64 ScriptedEffect::set(KWin::EffectWindow* w, KWin::AnimationEffect::Attribute a, int ms, KWin::FPx2 to, KWin::FPx2 from, uint metaData, int curve, int delay, bool fullScreen)
+quint64 ScriptedEffect::set(KWin::EffectWindow* w, KWin::AnimationEffect::Attribute a, int ms, KWin::FPx2 to, KWin::FPx2 from, uint metaData, int curve, int delay, bool fullScreen, bool keepAlive)
 {
     QEasingCurve qec;
     if (curve < QEasingCurve::Custom)
         qec.setType(static_cast<QEasingCurve::Type>(curve));
     else if (curve == GaussianCurve)
         qec.setCustomType(qecGaussian);
-    return AnimationEffect::set(w, a, metaData, ms, to, qec, delay, from, fullScreen);
+    return AnimationEffect::set(w, a, metaData, ms, to, qec, delay, from, fullScreen, keepAlive);
 }
 
 bool ScriptedEffect::retarget(quint64 animationId, KWin::FPx2 newTarget, int newRemainingTime)

scripting/scriptedeffect.h

@@ -102,8 +102,8 @@ public:
 
 public Q_SLOTS:
     //curve should be of type QEasingCurve::type or ScriptedEffect::EasingCurve
-    quint64 animate(KWin::EffectWindow *w, Attribute a, int ms, KWin::FPx2 to, KWin::FPx2 from = KWin::FPx2(), uint metaData = 0, int curve = QEasingCurve::Linear, int delay = 0, bool fullScreen = false);
+    quint64 animate(KWin::EffectWindow *w, Attribute a, int ms, KWin::FPx2 to, KWin::FPx2 from = KWin::FPx2(), uint metaData = 0, int curve = QEasingCurve::Linear, int delay = 0, bool fullScreen = false, bool keepAlive = true);
-    quint64 set(KWin::EffectWindow *w, Attribute a, int ms, KWin::FPx2 to, KWin::FPx2 from = KWin::FPx2(), uint metaData = 0, int curve = QEasingCurve::Linear, int delay = 0, bool fullScreen = false);
+    quint64 set(KWin::EffectWindow *w, Attribute a, int ms, KWin::FPx2 to, KWin::FPx2 from = KWin::FPx2(), uint metaData = 0, int curve = QEasingCurve::Linear, int delay = 0, bool fullScreen = false, bool keepAlive = true);
     bool retarget(quint64 animationId, KWin::FPx2 newTarget, int newRemainingTime = -1);
     bool cancel(quint64 animationId) { return AnimationEffect::cancel(animationId); }
     virtual bool borderActivated(ElectricBorder border);