[effects/slidingpopups] Overhaul the animations data struct

Summary:
The existing Data struct has some serious problems:

(a) naming is not intuitive, SlideInterface uses more cleaner terminology
    so let's use that (and because Wayland is the future);

(b) fadeInTime and fadeOutTime should be slideInDuration and slideOutDuration
    respectively. The Sliding popups effect doesn't fade windows, it slides
    them;

(c) mWindowsData should be m_animationsData because other parts of this
    effect refer to it as "anim data"(e.g. setupAnimData).

This effect uses its own Location enum class instead of KWayland::
Server::SlideInterface::Location because it would be better to not
depend on platform specific data structures.

As a side effect, this change also fixes QHash abuse. The Sliding popups
effect is still hashing windows twice in prePaintWindow and paintWindow.
But I think that's acceptable because usually there would be only one
active sliding window.

CCBUG: 331118

Reviewers: #kwin, davidedmundson

Reviewed By: #kwin, davidedmundson

Subscribers: davidedmundson, kwin

Tags: #kwin

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

effects/slidingpopups/slidingpopups.cpp

@@ -69,25 +69,25 @@ void SlidingPopupsEffect::reconfigure(ReconfigureFlags flags)
 {
     Q_UNUSED(flags)
     SlidingPopupsConfig::self()->read();
-    mFadeInTime = std::chrono::milliseconds(
+    m_slideInDuration = std::chrono::milliseconds(
         static_cast<int>(animationTime(SlidingPopupsConfig::slideInTime() != 0 ? SlidingPopupsConfig::slideInTime() : 150)));
-    mFadeOutTime = std::chrono::milliseconds(
+    m_slideOutDuration = std::chrono::milliseconds(
         static_cast<int>(animationTime(SlidingPopupsConfig::slideOutTime() != 0 ? SlidingPopupsConfig::slideOutTime() : 250)));
 
     auto animationIt = m_animations.begin();
     while (animationIt != m_animations.end()) {
         const auto duration = ((*animationIt).kind == AnimationKind::In)
-            ? mFadeInTime
+            ? m_slideInDuration
-            : mFadeOutTime;
+            : m_slideOutDuration;
         (*animationIt).timeLine.setDuration(duration);
         ++animationIt;
     }
 
-    QHash< const EffectWindow*, Data >::iterator wIt = mWindowsData.begin();
+    auto dataIt = m_animationsData.begin();
-    while (wIt != mWindowsData.end()) {
+    while (dataIt != m_animationsData.end()) {
-        wIt.value().fadeInDuration = mFadeInTime;
+        (*dataIt).slideInDuration = m_slideInDuration;
-        wIt.value().fadeOutDuration = mFadeOutTime;
+        (*dataIt).slideOutDuration = m_slideOutDuration;
-        ++wIt;
+        ++dataIt;
     }
 }
 
@@ -114,52 +114,46 @@ void SlidingPopupsEffect::paintWindow(EffectWindow* w, int mask, QRegion region,
         return;
     }
 
-    const int start = mWindowsData[ w ].start;
+    const AnimationData &animData = m_animationsData[w];
-
+    const int slideLength = (animData.slideLength > 0) ? animData.slideLength : mSlideLength;
-    int slideLength;
-    if (mWindowsData[ w ].slideLength > 0) {
-        slideLength = mWindowsData[ w ].slideLength;
-    } else {
-        slideLength = mSlideLength;
-    }
 
     const QRect screenRect = effects->clientArea(FullScreenArea, w->screen(), w->desktop());
     int splitPoint = 0;
     const QRect geo = w->expandedGeometry();
     const qreal t = (*animationIt).timeLine.value();
 
-    switch(mWindowsData[ w ].from) {
+    switch(animData.location) {
-    case West:
+    case Location::Left:
         if (slideLength < geo.width()) {
             data.multiplyOpacity(t);
         }
         data.translate(-interpolate(qMin(geo.width(), slideLength), 0.0, t));
-        splitPoint = geo.width() - (geo.x() + geo.width() - screenRect.x() - start);
+        splitPoint = geo.width() - (geo.x() + geo.width() - screenRect.x() - animData.offset);
         region = QRegion(geo.x() + splitPoint, geo.y(), geo.width() - splitPoint, geo.height());
         break;
-    case North:
+    case Location::Top:
         if (slideLength < geo.height()) {
             data.multiplyOpacity(t);
         }
         data.translate(0.0, -interpolate(qMin(geo.height(), slideLength), 0.0, t));
-        splitPoint = geo.height() - (geo.y() + geo.height() - screenRect.y() - start);
+        splitPoint = geo.height() - (geo.y() + geo.height() - screenRect.y() - animData.offset);
         region = QRegion(geo.x(), geo.y() + splitPoint, geo.width(), geo.height() - splitPoint);
         break;
-    case East:
+    case Location::Right:
         if (slideLength < geo.width()) {
             data.multiplyOpacity(t);
         }
         data.translate(interpolate(qMin(geo.width(), slideLength), 0.0, t));
-        splitPoint = screenRect.x() + screenRect.width() - geo.x() - start;
+        splitPoint = screenRect.x() + screenRect.width() - geo.x() - animData.offset;
         region = QRegion(geo.x(), geo.y(), splitPoint, geo.height());
         break;
-    case South:
+    case Location::Bottom:
     default:
         if (slideLength < geo.height()) {
             data.multiplyOpacity(t);
         }
         data.translate(0.0, interpolate(qMin(geo.height(), slideLength), 0.0, t));
-        splitPoint = screenRect.y() + screenRect.height() - geo.y() - start;
+        splitPoint = screenRect.y() + screenRect.height() - geo.y() - animData.offset;
         region = QRegion(geo.x(), geo.y(), geo.width(), splitPoint);
     }
 
@@ -206,7 +200,7 @@ void SlidingPopupsEffect::slotWindowAdded(EffectWindow *w)
 void SlidingPopupsEffect::slotWindowDeleted(EffectWindow* w)
 {
     m_animations.remove(w);
-    mWindowsData.remove(w);
+    m_animationsData.remove(w);
     effects->addRepaint(w->expandedGeometry());
 }
 
@@ -223,33 +217,49 @@ void SlidingPopupsEffect::slotPropertyNotify(EffectWindow* w, long a)
             w->setData(WindowClosedGrabRole, QVariant());
         }
         m_animations.remove(w);
-        mWindowsData.remove(w);
+        m_animationsData.remove(w);
         return;
     }
 
     auto* d = reinterpret_cast< uint32_t* >(data.data());
-    Data animData;
+    AnimationData &animData = m_animationsData[w];
-    animData.start = d[ 0 ];
+    animData.offset = d[ 0 ];
-    animData.from = (Position)d[ 1 ];
+
+    switch (d[1]) {
+    case 0: // West
+        animData.location = Location::Left;
+        break;
+    case 1: // North
+        animData.location = Location::Top;
+        break;
+    case 2: // East
+        animData.location = Location::Right;
+        break;
+    case 3: // South
+    default:
+        animData.location = Location::Bottom;
+        break;
+    }
+
     //custom duration
     animData.slideLength = 0;
     if (data.length() >= (int)(sizeof(uint32_t) * 3)) {
-        animData.fadeInDuration = std::chrono::milliseconds(d[2]);
+        animData.slideInDuration = std::chrono::milliseconds(d[2]);
         if (data.length() >= (int)(sizeof(uint32_t) * 4))
             //custom fadein
-            animData.fadeOutDuration = std::chrono::milliseconds(d[3]);
+            animData.slideOutDuration = std::chrono::milliseconds(d[3]);
         else
             //custom fadeout
-            animData.fadeOutDuration = std::chrono::milliseconds(d[2]);
+            animData.slideOutDuration = std::chrono::milliseconds(d[2]);
 
         //do we want an actual slide?
         if (data.length() >= (int)(sizeof(uint32_t) * 5))
             animData.slideLength = d[4];
     } else {
-        animData.fadeInDuration = mFadeInTime;
+        animData.slideInDuration = m_slideInDuration;
-        animData.fadeOutDuration = mFadeOutTime;
+        animData.slideOutDuration = m_slideOutDuration;
     }
-    mWindowsData[ w ] = animData;
+
     setupAnimData(w);
 }
 
@@ -257,37 +267,39 @@ void SlidingPopupsEffect::setupAnimData(EffectWindow *w)
 {
     const QRect screenRect = effects->clientArea(FullScreenArea, w->screen(), effects->currentDesktop());
     const QRect windowGeo = w->geometry();
-    if (mWindowsData[w].start == -1) {
+    AnimationData &animData = m_animationsData[w];
-        switch (mWindowsData[w].from) {
+
-        case West:
+    if (animData.offset == -1) {
-            mWindowsData[w].start = qMax(windowGeo.left() - screenRect.left(), 0);
+        switch (animData.location) {
+        case Location::Left:
+            animData.offset = qMax(windowGeo.left() - screenRect.left(), 0);
             break;
-        case North:
+        case Location::Top:
-            mWindowsData[w].start = qMax(windowGeo.top() - screenRect.top(), 0);
+            animData.offset = qMax(windowGeo.top() - screenRect.top(), 0);
             break;
-        case East:
+        case Location::Right:
-            mWindowsData[w].start = qMax(screenRect.right() - windowGeo.right(), 0);
+            animData.offset = qMax(screenRect.right() - windowGeo.right(), 0);
             break;
-        case South:
+        case Location::Bottom:
         default:
-            mWindowsData[w].start = qMax(screenRect.bottom() - windowGeo.bottom(), 0);
+            animData.offset = qMax(screenRect.bottom() - windowGeo.bottom(), 0);
             break;
         }
     }
     // sanitize
-    switch (mWindowsData[w].from) {
+    switch (animData.location) {
-    case West:
+    case Location::Left:
-        mWindowsData[w].start = qMax(windowGeo.left() - screenRect.left(), mWindowsData[w].start);
+        animData.offset = qMax(windowGeo.left() - screenRect.left(), animData.offset);
         break;
-    case North:
+    case Location::Top:
-        mWindowsData[w].start = qMax(windowGeo.top() - screenRect.top(), mWindowsData[w].start);
+        animData.offset = qMax(windowGeo.top() - screenRect.top(), animData.offset);
         break;
-    case East:
+    case Location::Right:
-        mWindowsData[w].start = qMax(screenRect.right() - windowGeo.right(), mWindowsData[w].start);
+        animData.offset = qMax(screenRect.right() - windowGeo.right(), animData.offset);
         break;
-    case South:
+    case Location::Bottom:
     default:
-        mWindowsData[w].start = qMax(screenRect.bottom() - windowGeo.bottom(), mWindowsData[w].start);
+        animData.offset = qMax(screenRect.bottom() - windowGeo.bottom(), animData.offset);
         break;
     }
 
@@ -307,28 +319,28 @@ void SlidingPopupsEffect::slotWaylandSlideOnShowChanged(EffectWindow* w)
     }
 
     if (surf->slideOnShowHide()) {
-        Data animData;
+        AnimationData &animData = m_animationsData[w];
-        animData.start = surf->slideOnShowHide()->offset();
+
+        animData.offset = surf->slideOnShowHide()->offset();
 
         switch (surf->slideOnShowHide()->location()) {
         case KWayland::Server::SlideInterface::Location::Top:
-            animData.from = North;
+            animData.location = Location::Top;
             break;
         case KWayland::Server::SlideInterface::Location::Left:
-            animData.from = West;
+            animData.location = Location::Left;
             break;
         case KWayland::Server::SlideInterface::Location::Right:
-            animData.from = East;
+            animData.location = Location::Right;
             break;
         case KWayland::Server::SlideInterface::Location::Bottom:
         default:
-            animData.from = South;
+            animData.location = Location::Bottom;
             break;
         }
         animData.slideLength = 0;
-        animData.fadeInDuration = mFadeInTime;
+        animData.slideInDuration = m_slideInDuration;
-        animData.fadeOutDuration = mFadeOutTime;
+        animData.slideOutDuration = m_slideOutDuration;
-        mWindowsData[ w ] = animData;
 
         setupAnimData(w);
     }
@@ -344,8 +356,8 @@ void SlidingPopupsEffect::slideIn(EffectWindow *w)
         return;
     }
 
-    auto dataIt = mWindowsData.constFind(w);
+    auto dataIt = m_animationsData.constFind(w);
-    if (dataIt == mWindowsData.constEnd()) {
+    if (dataIt == m_animationsData.constEnd()) {
         return;
     }
 
@@ -353,7 +365,7 @@ void SlidingPopupsEffect::slideIn(EffectWindow *w)
     animation.kind = AnimationKind::In;
     animation.timeLine.reset();
     animation.timeLine.setDirection(TimeLine::Forward);
-    animation.timeLine.setDuration((*dataIt).fadeInDuration);
+    animation.timeLine.setDuration((*dataIt).slideInDuration);
     animation.timeLine.setEasingCurve(QEasingCurve::InOutSine);
 
     w->setData(WindowAddedGrabRole, QVariant::fromValue(static_cast<void*>(this)));
@@ -373,8 +385,8 @@ void SlidingPopupsEffect::slideOut(EffectWindow *w)
         return;
     }
 
-    auto dataIt = mWindowsData.constFind(w);
+    auto dataIt = m_animationsData.constFind(w);
-    if (dataIt == mWindowsData.constEnd()) {
+    if (dataIt == m_animationsData.constEnd()) {
         return;
     }
 
@@ -386,7 +398,7 @@ void SlidingPopupsEffect::slideOut(EffectWindow *w)
     animation.kind = AnimationKind::Out;
     animation.timeLine.reset();
     animation.timeLine.setDirection(TimeLine::Backward);
-    animation.timeLine.setDuration((*dataIt).fadeOutDuration);
+    animation.timeLine.setDuration((*dataIt).slideOutDuration);
     animation.timeLine.setEasingCurve(QEasingCurve::InOutSine);
 
     w->setData(WindowClosedGrabRole, QVariant::fromValue(static_cast<void*>(this)));

effects/slidingpopups/slidingpopups.h

@@ -31,8 +31,8 @@ class SlidingPopupsEffect
     : public Effect
 {
     Q_OBJECT
-    Q_PROPERTY(int fadeInTime READ fadeInTime)
+    Q_PROPERTY(int slideInDuration READ slideInDuration)
-    Q_PROPERTY(int fadeOutTime READ fadeOutTime)
+    Q_PROPERTY(int slideOutDuration READ slideOutDuration)
 public:
     SlidingPopupsEffect();
     ~SlidingPopupsEffect();
@@ -51,13 +51,9 @@ public:
 
     // TODO react also on virtual desktop changes
 
-    // for properties
+    int slideInDuration() const;
-    int fadeInTime() const {
+    int slideOutDuration() const;
-        return mFadeInTime.count();
+
-    }
-    int fadeOutTime() const {
-        return mFadeOutTime.count();
-    }
 public Q_SLOTS:
     void slotWindowAdded(KWin::EffectWindow *c);
     void slotWindowDeleted(KWin::EffectWindow *w);
@@ -70,26 +66,11 @@ public Q_SLOTS:
 private:
     void setupAnimData(EffectWindow *w);
 
-    enum Position {
-        West = 0,
-        North = 1,
-        East = 2,
-        South = 3
-    };
-    struct Data {
-        int start; //point in screen coordinates where the window starts
-        //to animate, from decides if this point is an x or an y
-        Position from;
-        std::chrono::milliseconds fadeInDuration;
-        std::chrono::milliseconds fadeOutDuration;
-        int slideLength;
-    };
     long mAtom;
 
-    QHash< const EffectWindow*, Data > mWindowsData;
     int mSlideLength;
-    std::chrono::milliseconds mFadeInTime;
+    std::chrono::milliseconds m_slideInDuration;
-    std::chrono::milliseconds mFadeOutTime;
+    std::chrono::milliseconds m_slideOutDuration;
 
     enum class AnimationKind {
         In,
@@ -101,8 +82,34 @@ private:
         TimeLine timeLine;
     };
     QHash<const EffectWindow*, Animation> m_animations;
+
+    enum class Location {
+        Left,
+        Top,
+        Right,
+        Bottom
+    };
+
+    struct AnimationData {
+        int offset;
+        Location location;
+        std::chrono::milliseconds slideInDuration;
+        std::chrono::milliseconds slideOutDuration;
+        int slideLength;
+    };
+    QHash<const EffectWindow*, AnimationData> m_animationsData;
 };
 
+inline int SlidingPopupsEffect::slideInDuration() const
+{
+    return m_slideInDuration.count();
+}
+
+inline int SlidingPopupsEffect::slideOutDuration() const
+{
+    return m_slideOutDuration.count();
+}
+
 } // namespace
 
 #endif