From fb69b791a16f4a89fd79a010ce8f67419de16004 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Mon, 31 Oct 2016 11:22:24 +0100 Subject: [PATCH] Ensure that all Effects honour the grab roles correctly Summary: When windows get added some effects grab the window and want to be the only one animating this window. For this the grab roles exists. An effect being notified later on evaluates the grab state and does not start the animation. This process failed due to being dependent on the order the effects are loaded. Window Added/Closed are signals emitted by EffectsHandler, thus first come, first serve. The requested effect order does not play into it. Due to that it could happen that an Effect which should not animate, started to animate as the grab was still there. This change adds the possibility to be notified whenever the window data changes. A new signal is added to EffectsHandler which is emitted whenever the windowData changes. The interested effects connect to it and cancel their (just started) animation for the window. Adjusted effects are: * ScaleIn * Fade * WobblyWindows In case of WobblyWindows an additional logical error was fixed that the animations were only run when an effect grabbed instead of the other way around. BUG: 336866 FIXED-IN: 5.8.4 Reviewers: #kwin, #plasma, broulik Subscribers: plasma-devel, kwin Tags: #kwin Differential Revision: https://phabricator.kde.org/D3211 --- .../effects/slidingpopups_test.cpp | 12 ------- effects.cpp | 1 + effects/fade/package/contents/code/main.js | 21 +++++++++++-- effects/scalein/package/contents/code/main.js | 14 ++++++++- effects/slidingpopups/slidingpopups.cpp | 1 - effects/wobblywindows/wobblywindows.cpp | 31 +++++++++++++++++-- effects/wobblywindows/wobblywindows.h | 1 + libkwineffects/kwineffects.h | 18 +++++++++++ 8 files changed, 81 insertions(+), 18 deletions(-) diff --git a/autotests/integration/effects/slidingpopups_test.cpp b/autotests/integration/effects/slidingpopups_test.cpp index 3437a6a3b1..b5d59d5a18 100644 --- a/autotests/integration/effects/slidingpopups_test.cpp +++ b/autotests/integration/effects/slidingpopups_test.cpp @@ -222,10 +222,6 @@ void SlidingPopupsTest::testWithOtherEffect() // sliding popups should be active QVERIFY(windowAddedSpy.wait()); QTRY_VERIFY(slidingPoupus->isActive()); - QEXPECT_FAIL("scale, slide", "bug 336866", Continue); - QEXPECT_FAIL("fade, slide", "bug 336866", Continue); - QEXPECT_FAIL("wobblywindows, slide", "bug 336866", Continue); - QEXPECT_FAIL("slide, wobblywindows", "bug 336866", Continue); QVERIFY(!otherEffect->isActive()); // wait till effect ends @@ -246,8 +242,6 @@ void SlidingPopupsTest::testWithOtherEffect() // again we should have the sliding popups active QVERIFY(slidingPoupus->isActive()); - QEXPECT_FAIL("wobblywindows, slide", "bug 336866", Continue); - QEXPECT_FAIL("slide, wobblywindows", "bug 336866", Continue); QVERIFY(!otherEffect->isActive()); QVERIFY(windowDeletedSpy.wait()); @@ -347,10 +341,6 @@ void SlidingPopupsTest::testWithOtherEffectWayland() // sliding popups should be active QCOMPARE(windowAddedSpy.count(), 1); QTRY_VERIFY(slidingPoupus->isActive()); - QEXPECT_FAIL("scale, slide", "bug 336866", Continue); - QEXPECT_FAIL("fade, slide", "bug 336866", Continue); - QEXPECT_FAIL("wobblywindows, slide", "bug 336866", Continue); - QEXPECT_FAIL("slide, wobblywindows", "bug 336866", Continue); QVERIFY(!otherEffect->isActive()); // wait till effect ends @@ -371,8 +361,6 @@ void SlidingPopupsTest::testWithOtherEffectWayland() // again we should have the sliding popups active QVERIFY(slidingPoupus->isActive()); - QEXPECT_FAIL("wobblywindows, slide", "bug 336866", Continue); - QEXPECT_FAIL("slide, wobblywindows", "bug 336866", Continue); QVERIFY(!otherEffect->isActive()); QVERIFY(windowDeletedSpy.wait()); diff --git a/effects.cpp b/effects.cpp index f50c96b744..e6e636c99c 100644 --- a/effects.cpp +++ b/effects.cpp @@ -1688,6 +1688,7 @@ void EffectWindowImpl::setData(int role, const QVariant &data) dataMap[ role ] = data; else dataMap.remove(role); + emit effects->windowDataChanged(this, role); } QVariant EffectWindowImpl::data(int role) const diff --git a/effects/fade/package/contents/code/main.js b/effects/fade/package/contents/code/main.js index 10730365ba..57aaca826a 100644 --- a/effects/fade/package/contents/code/main.js +++ b/effects/fade/package/contents/code/main.js @@ -43,12 +43,12 @@ effect.configChanged.connect(function() { }); effects.windowAdded.connect(function(w) { if (fadeWindows && isFadeWindow(w)) { - effect.animate(w, Effect.Opacity, fadeInTime, 1.0, 0.0); + w.fadeInWindowTypeAnimation = effect.animate(w, Effect.Opacity, fadeInTime, 1.0, 0.0); } }); effects.windowClosed.connect(function(w) { if (fadeWindows && isFadeWindow(w)) { - animate({ + w.fadeOutWindowTypeAnimation = animate({ window: w, duration: fadeOutTime, animations: [{ @@ -59,3 +59,20 @@ effects.windowClosed.connect(function(w) { }); } }); +effects.windowDataChanged.connect(function (window, role) { + if (role == Effect.WindowAddedGrabRole) { + if (effect.isGrabbed(window, Effect.WindowAddedGrabRole)) { + if (window.fadeInWindowTypeAnimation !== undefined) { + cancel(window.fadeInWindowTypeAnimation); + window.fadeInWindowTypeAnimation = undefined; + } + } + } else if (role == Effect.WindowClosedGrabRole) { + if (effect.isGrabbed(window, Effect.WindowClosedGrabRole)) { + if (window.fadeOutWindowTypeAnimation !== undefined) { + cancel(window.fadeOutWindowTypeAnimation); + window.fadeOutWindowTypeAnimation = undefined; + } + } + } +}); diff --git a/effects/scalein/package/contents/code/main.js b/effects/scalein/package/contents/code/main.js index 5c6a93a9fe..5961183856 100644 --- a/effects/scalein/package/contents/code/main.js +++ b/effects/scalein/package/contents/code/main.js @@ -34,7 +34,7 @@ var scaleInEffect = { }, scaleIn: function (window) { "use strict"; - animate({ + window.scaleInWindowTypeAnimation = animate({ window: window, duration: scaleInEffect.duration, curve: QEasingCurve.InOutQuad, @@ -56,10 +56,22 @@ var scaleInEffect = { } scaleInEffect.scaleIn(window); }, + dataChanged: function (window, role) { + "use strict"; + if (role == Effect.WindowAddedGrabRole) { + if (effect.isGrabbed(window, Effect.WindowAddedGrabRole)) { + if (window.scaleInWindowTypeAnimation !== undefined) { + cancel(window.scaleInWindowTypeAnimation); + window.scaleInWindowTypeAnimation = undefined; + } + } + } + }, init: function () { "use strict"; effect.configChanged.connect(scaleInEffect.loadConfig); effects.windowAdded.connect(scaleInEffect.added); + effects.windowDataChanged.connect(scaleInEffect.dataChanged); } }; scaleInEffect.init(); diff --git a/effects/slidingpopups/slidingpopups.cpp b/effects/slidingpopups/slidingpopups.cpp index 931ce71e44..9f82b91408 100644 --- a/effects/slidingpopups/slidingpopups.cpp +++ b/effects/slidingpopups/slidingpopups.cpp @@ -305,7 +305,6 @@ void SlidingPopupsEffect::startForShow(EffectWindow *w) mAppearingWindows.insert(w, new QTimeLine(mWindowsData[ w ].fadeInDuration, this)); mAppearingWindows[ w ]->setCurveShape(QTimeLine::EaseInOutCurve); - // Tell other windowAdded() and windowClosed() effects to ignore this window w->setData(WindowAddedGrabRole, QVariant::fromValue(static_cast(this))); w->setData(WindowClosedGrabRole, QVariant::fromValue(static_cast(this))); w->setData(WindowForceBlurRole, true); diff --git a/effects/wobblywindows/wobblywindows.cpp b/effects/wobblywindows/wobblywindows.cpp index 7e3ddcea85..2b32105b80 100644 --- a/effects/wobblywindows/wobblywindows.cpp +++ b/effects/wobblywindows/wobblywindows.cpp @@ -153,6 +153,8 @@ WobblyWindowsEffect::WobblyWindowsEffect() connect(effects, SIGNAL(windowStepUserMovedResized(KWin::EffectWindow*,QRect)), this, SLOT(slotWindowStepUserMovedResized(KWin::EffectWindow*,QRect))); connect(effects, SIGNAL(windowFinishUserMovedResized(KWin::EffectWindow*)), this, SLOT(slotWindowFinishUserMovedResized(KWin::EffectWindow*))); connect(effects, SIGNAL(windowMaximizedStateChanged(KWin::EffectWindow*,bool,bool)), this, SLOT(slotWindowMaximizeStateChanged(KWin::EffectWindow*,bool,bool))); + + connect(effects, &EffectsHandler::windowDataChanged, this, &WobblyWindowsEffect::cancelWindowGrab); } WobblyWindowsEffect::~WobblyWindowsEffect() @@ -472,7 +474,7 @@ void WobblyWindowsEffect::stepMovedResized(EffectWindow* w) void WobblyWindowsEffect::slotWindowAdded(EffectWindow* w) { - if (m_openEffectEnabled && w->data(WindowAddedGrabRole).value() != this) { + if (m_openEffectEnabled && w->data(WindowAddedGrabRole).value() == nullptr) { if (windows.contains(w)) { // could this happen ?? WindowWobblyInfos& wwi = windows[w]; @@ -499,7 +501,7 @@ void WobblyWindowsEffect::slotWindowClosed(EffectWindow* w) if (windows.isEmpty()) effects->addRepaintFull(); } - } else if (m_closeEffectEnabled && w->data(WindowAddedGrabRole).value() != this) { + } else if (m_closeEffectEnabled && w->data(WindowClosedGrabRole).value() == nullptr) { WindowWobblyInfos new_wwi; initWobblyInfo(new_wwi, w->geometry()); wobblyCloseInit(new_wwi, w); @@ -1200,6 +1202,31 @@ void WobblyWindowsEffect::heightRingLinearMean(Pair** data_pointer, WindowWobbly wwi.buffer = tmp; } + +void WobblyWindowsEffect::cancelWindowGrab(KWin::EffectWindow *w, int grabRole) +{ + if (grabRole == WindowAddedGrabRole) { + if (w->data(WindowAddedGrabRole).value() != this) { + auto it = windows.find(w); + if (it != windows.end()) { + freeWobblyInfo(it.value()); + windows.erase(it); + } + } + } else if (grabRole == WindowClosedGrabRole) { + if (w->data(WindowClosedGrabRole).value() != this) { + auto it = windows.find(w); + if (it != windows.end()) { + if (it.value().status == Closing) { + w->unrefWindow(); + } + freeWobblyInfo(it.value()); + windows.erase(it); + } + } + } +} + bool WobblyWindowsEffect::isActive() const { return !windows.isEmpty(); diff --git a/effects/wobblywindows/wobblywindows.h b/effects/wobblywindows/wobblywindows.h index 165ec726aa..33cc410080 100644 --- a/effects/wobblywindows/wobblywindows.h +++ b/effects/wobblywindows/wobblywindows.h @@ -136,6 +136,7 @@ public Q_SLOTS: private: + void cancelWindowGrab(KWin::EffectWindow *w, int grabRole); void startMovedResized(EffectWindow* w); void stepMovedResized(EffectWindow* w); bool updateWindowWobblyDatas(EffectWindow* w, qreal time); diff --git a/libkwineffects/kwineffects.h b/libkwineffects/kwineffects.h index f0782b3432..834c7d5c3a 100644 --- a/libkwineffects/kwineffects.h +++ b/libkwineffects/kwineffects.h @@ -1537,6 +1537,21 @@ Q_SIGNALS: **/ void windowHidden(KWin::EffectWindow *w); + /** + * This signal gets emitted when the data on EffectWindow @p w for @p role changed. + * + * An Effect can connect to this signal to read the new value and react on it. + * E.g. an Effect which does not operate on windows grabbed by another Effect wants + * to cancel the already scheduled animation if another Effect adds a grab. + * + * @param w The EffectWindow for which the data changed + * @param role The data role which changed + * @see EffectWindow::setData + * @see EffectWindow::data + * @since 5.8.4 + **/ + void windowDataChanged(KWin::EffectWindow *w, int role); + protected: QVector< EffectPair > loaded_effects; //QHash< QString, EffectFactory* > effect_factories; @@ -2017,6 +2032,9 @@ public: /** * Can be used to by effects to store arbitrary data in the EffectWindow. + * + * Invoking this method will emit the signal EffectsHandler::windowDataChanged. + * @see EffectsHandler::windowDataChanged */ Q_SCRIPTABLE virtual void setData(int role, const QVariant &data) = 0; Q_SCRIPTABLE virtual QVariant data(int role) const = 0;