From a281f2bce1d1e67925aa4261ca3cdadffaeb57fc Mon Sep 17 00:00:00 2001 From: Vlad Zagorodniy Date: Wed, 10 Oct 2018 10:36:45 +0300 Subject: [PATCH] [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 --- .../effects/scripted_effects_test.cpp | 62 ++++++++++++++++++- .../effects/scripts/keepAliveTest.js | 13 ++++ .../effects/scripts/keepAliveTestDontKeep.js | 13 ++++ .../package/contents/code/main.js | 1 + libkwineffects/anidata.cpp | 16 ++++- libkwineffects/anidata_p.h | 22 ++++++- libkwineffects/kwinanimationeffect.cpp | 45 +++++++------- libkwineffects/kwinanimationeffect.h | 11 ++-- scripting/scriptedeffect.cpp | 36 ++++++++--- scripting/scriptedeffect.h | 4 +- 10 files changed, 184 insertions(+), 39 deletions(-) create mode 100644 autotests/integration/effects/scripts/keepAliveTest.js create mode 100644 autotests/integration/effects/scripts/keepAliveTestDontKeep.js diff --git a/autotests/integration/effects/scripted_effects_test.cpp b/autotests/integration/effects/scripted_effects_test.cpp index 4e477b0aaf..ab7667bcbf 100644 --- a/autotests/integration/effects/scripted_effects_test.cpp +++ b/autotests/integration/effects/scripted_effects_test.cpp @@ -23,7 +23,7 @@ along with this program. If not, see . #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(); qRegisterMetaType(); + qRegisterMetaType(); qRegisterMetaType(); 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("file"); + QTest::addColumn("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" diff --git a/autotests/integration/effects/scripts/keepAliveTest.js b/autotests/integration/effects/scripts/keepAliveTest.js new file mode 100644 index 0000000000..cf3732dc5b --- /dev/null +++ b/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"); +}); diff --git a/autotests/integration/effects/scripts/keepAliveTestDontKeep.js b/autotests/integration/effects/scripts/keepAliveTestDontKeep.js new file mode 100644 index 0000000000..72bdc5d669 --- /dev/null +++ b/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"); +}); diff --git a/effects/dialogparent/package/contents/code/main.js b/effects/dialogparent/package/contents/code/main.js index 2ca3c0e19b..9bf63de0d6 100644 --- a/effects/dialogparent/package/contents/code/main.js +++ b/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, diff --git a/libkwineffects/anidata.cpp b/libkwineffects/anidata.cpp index 443b67197c..cd9f5c249f 100644 --- a/libkwineffects/anidata.cpp +++ b/libkwineffects/anidata.cpp @@ -3,6 +3,7 @@ This file is part of the KDE project. Copyright (C) 2011 Thomas Lübking +Copyright (C) 2018 Vlad Zagorodniy 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) { } diff --git a/libkwineffects/anidata_p.h b/libkwineffects/anidata_p.h index eafd6f0fb3..95e92c1c13 100644 --- a/libkwineffects/anidata_p.h +++ b/libkwineffects/anidata_p.h @@ -3,6 +3,7 @@ This file is part of the KDE project. Copyright (C) 2011 Thomas Lübking +Copyright (C) 2018 Vlad Zagorodniy 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 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 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; bool waitAtSource, keepAtTarget; + bool keepAlive; + KeepAliveLockPtr keepAliveLock; }; } // namespace diff --git a/libkwineffects/kwinanimationeffect.cpp b/libkwineffects/kwinanimationeffect.cpp index 5f0d8dc373..144996f227 100644 --- a/libkwineffects/kwinanimationeffect.cpp +++ b/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 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(); - d->m_zombies << w; + + auto it = d->m_animations.find(w); + if (it == d->m_animations.end()) { + return; + } + + KeepAliveLockPtr keepAliveLock; + + QList &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 ); } diff --git a/libkwineffects/kwinanimationeffect.h b/libkwineffects/kwinanimationeffect.h index ce46f34493..a71bf1908b 100644 --- a/libkwineffects/kwinanimationeffect.h +++ b/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) - { return p_animate(w, a, meta, ms, to, curve, delay, from, false, fullScreen); } + 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, 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) - { return p_animate(w, a, meta, ms, to, curve, delay, from, true, fullScreen); } + 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, 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; diff --git a/scripting/scriptedeffect.cpp b/scripting/scriptedeffect.cpp index d3837ea32b..730146e56a 100644 --- a/scripting/scriptedeffect.cpp +++ b/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(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 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(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(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) diff --git a/scripting/scriptedeffect.h b/scripting/scriptedeffect.h index 8a30d0cded..0b667bb1b7 100644 --- a/scripting/scriptedeffect.h +++ b/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 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 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, 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);