fix(animations): comprehensive PR review fixes — safety, perf, DRY, a11y

Fixes 28 issues from PR #291 review:

Bugs: spring velocity sign inversion, negative ring buffer index,
velocity index not reset on drag start, unchecked TimingMode/duration/
styleParam from JSON, elastic easing division-by-zero.

Edge cases: near-critical omegaD guard, clamped initialVelocity from
JSON, spring duration cap 10s→5s, tree resolution cycle guard,
resolvedProfile default fallback, dampingRatio≤0 guard, epsilon slider
step alignment.

DRY: resolvedProfileOrDefault() helper in WTA, AnimationSubPage.qml
base component, preset filtering helpers, ConfigDefaults for spring
defaults, QLatin1String consistency in shader registries and
geometryutils.

Style: per-window usesOpacity(), showStyleSelector:false on fade/
border/preview, Kirigami.Units for hardcoded pixels, Accessible.name
on sliders, required properties on SpringPreview/CurveThumbnail.

Co-Authored-By: claude-flow <[email protected]>

Author: fuddlesworth

kwin-effect/dragtracker.cpp

@@ -36,6 +36,7 @@ void DragTracker::handleWindowStartMoveResize(KWin::EffectWindow* w)
 
     m_draggedWindow = w;
     m_draggedWindowId = m_effect->getWindowId(w);
+    m_velocitySampleIndex = 0;
     m_lastCursorPos = KWin::effects->cursorPos();
     m_dragMovedThrottle.start();
 
@@ -104,7 +105,8 @@ void DragTracker::finishDrag(bool cancelled)
         const int lastIdx = (m_velocitySampleIndex - 1) % VelocitySampleCount;
         // Search backwards for a sample at least 5ms older to avoid noise
         for (int offset = 1; offset < qMin(m_velocitySampleIndex, VelocitySampleCount); ++offset) {
-            const int prevIdx = (m_velocitySampleIndex - 1 - offset) % VelocitySampleCount;
+            const int prevIdx = ((m_velocitySampleIndex - 1 - offset) % VelocitySampleCount + VelocitySampleCount)
+                % VelocitySampleCount;
             const qint64 dtMs = m_velocitySamples[lastIdx].timestampMs - m_velocitySamples[prevIdx].timestampMs;
             if (dtMs >= 5) {
                 const QPointF dp = m_velocitySamples[lastIdx].position - m_velocitySamples[prevIdx].position;

kwin-effect/plasmazoneseffect.cpp

@@ -2728,7 +2728,7 @@ std::optional<AnimationParams> PlasmaZonesEffect::resolveAnimationFromCachedTree
     // Build chain from root to eventName
     QStringList chain;
     QString current = eventName;
-    while (!current.isEmpty()) {
+    while (!current.isEmpty() && chain.size() < 10) {
         chain.prepend(current);
         current = parentMap.value(current);
     }
@@ -3216,7 +3216,7 @@ void PlasmaZonesEffect::prePaintWindow(KWin::RenderView* view, KWin::EffectWindo
         data.setTransformed();
         // Mark as translucent if any active animation uses opacity (Slide, Popin, SlideFade)
         // or if the window is redirected for custom shader rendering (shaders may modify alpha)
-        if (m_windowAnimator->hasOpacityAnimations() || m_redirectedWindows.contains(w))
+        if (m_windowAnimator->usesOpacity(w) || m_redirectedWindows.contains(w))
             data.setTranslucent();
     }
 

kwin-effect/windowanimator.cpp

@@ -43,7 +43,6 @@ qreal EasingCurve::evaluateBounceOut(qreal t, qreal amp, int n)
     // Structure: one half-parabola (approach: 0→1) followed by n full parabolic
     // arcs of decreasing height. Amplitude scales the bounce dip depths.
     constexpr qreal r = 0.5;
-    static_assert(r != 1.0, "r == 1.0 causes division by zero in bounce calculation");
     n = qBound(1, n, 8);
 
     // Compute base arc duration d so all arcs fit in [0, 1]:
@@ -269,16 +268,22 @@ qreal SpringAnimation::evaluate(qreal t) const
     if (zeta < 1.0) {
         // Underdamped: oscillates
         const qreal omegaD = omega * qSqrt(1.0 - zeta * zeta);
+        // Guard near-critical damping where omegaD approaches zero — fall through
+        // to the critically-damped formula to avoid division by near-zero.
+        if (omegaD < 1e-6) {
+            const qreal decay = qExp(-omega * t);
+            return 1.0 - decay * (1.0 + (omega - initialVelocity) * t);
+        }
         const qreal decay = qExp(-zeta * omega * t);
         const qreal v0 = initialVelocity;
-        // x(t) = 1 - e^(-zeta*omega*t) * (cos(omegaD*t) + ((zeta*omega + v0) / omegaD) * sin(omegaD*t))
-        // where v0 is initial velocity contribution (0 when starting from rest toward target)
-        const qreal sinTerm = (zeta * omega + v0) / omegaD;
+        // x(t) = 1 - e^(-zeta*omega*t) * (cos(omegaD*t) + ((zeta*omega - v0) / omegaD) * sin(omegaD*t))
+        // Positive v0 (drag toward target) reduces the sin coefficient to arrive faster.
+        const qreal sinTerm = (zeta * omega - v0) / omegaD;
         return 1.0 - decay * (qCos(omegaD * t) + sinTerm * qSin(omegaD * t));
     } else if (qFuzzyCompare(zeta, 1.0)) {
         // Critically damped: fastest without oscillation
         const qreal decay = qExp(-omega * t);
-        return 1.0 - decay * (1.0 + (omega + initialVelocity) * t);
+        return 1.0 - decay * (1.0 + (omega - initialVelocity) * t);
     } else {
         // Overdamped: no oscillation, slower
         const qreal disc = qSqrt(zeta * zeta - 1.0);
@@ -288,7 +293,7 @@ qreal SpringAnimation::evaluate(qreal t) const
         // critically damped formula to avoid division by near-zero (s2 - s1).
         if (qAbs(s2 - s1) < 1e-6) {
             const qreal decay = qExp(-omega * t);
-            return 1.0 - decay * (1.0 + (omega + initialVelocity) * t);
+            return 1.0 - decay * (1.0 + (omega - initialVelocity) * t);
         }
         const qreal c2 = (initialVelocity - s1) / (s2 - s1);
         const qreal c1 = 1.0 - c2;
@@ -316,7 +321,7 @@ qreal SpringAnimation::estimatedDuration() const
     // Forward scan: require 3 consecutive settled samples to avoid
     // false positives from underdamped springs briefly crossing epsilon.
     constexpr qreal dt = 0.005;
-    constexpr qreal maxT = 10.0;
+    constexpr qreal maxT = 5.0;
     constexpr int requiredConsecutive = 3;
     int consecutive = 0;
     for (qreal t = dt; t <= maxT; t += dt) {
@@ -357,7 +362,7 @@ AnimationParams AnimationParams::fromJson(const QJsonObject& obj)
         p.spring.dampingRatio = qBound(0.1, sp.value(QLatin1String("dampingRatio")).toDouble(1.0), 10.0);
         p.spring.stiffness = qBound(1.0, sp.value(QLatin1String("stiffness")).toDouble(800.0), 2000.0);
         p.spring.epsilon = qBound(0.00001, sp.value(QLatin1String("epsilon")).toDouble(0.0001), 0.1);
-        p.spring.initialVelocity = sp.value(QLatin1String("initialVelocity")).toDouble(0.0);
+        p.spring.initialVelocity = qBound(-10.0, sp.value(QLatin1String("initialVelocity")).toDouble(0.0), 10.0);
     }
 
     p.style = animationStyleFromString(obj.value(QLatin1String("style")).toString());

kwin-effect/windowanimator.h

@@ -329,6 +329,13 @@ class WindowAnimator : public QObject
         return m_opacityAnimationCount > 0;
     }
 
+    /// Whether a specific window's animation uses opacity (Slide, Popin, SlideFade)
+    bool usesOpacity(KWin::EffectWindow* w) const
+    {
+        auto it = m_animations.find(w);
+        return it != m_animations.end() && it->usesOpacity();
+    }
+
     void removeAnimation(KWin::EffectWindow* window);
     void clear();
 

src/core/animationprofile.cpp

@@ -37,14 +37,16 @@ SpringParams SpringParams::fromJson(const QJsonObject& obj)
 {
     SpringParams sp;
     if (obj.contains(QLatin1String("dampingRatio")))
-        sp.dampingRatio =
-            qBound(ConfigDefaults::springDampingRatioMin(), obj[QLatin1String("dampingRatio")].toDouble(1.0),
-                   ConfigDefaults::springDampingRatioMax());
+        sp.dampingRatio = qBound(ConfigDefaults::springDampingRatioMin(),
+                                 obj[QLatin1String("dampingRatio")].toDouble(ConfigDefaults::springDampingRatio()),
+                                 ConfigDefaults::springDampingRatioMax());
     if (obj.contains(QLatin1String("stiffness")))
-        sp.stiffness = qBound(ConfigDefaults::springStiffnessMin(), obj[QLatin1String("stiffness")].toDouble(800.0),
+        sp.stiffness = qBound(ConfigDefaults::springStiffnessMin(),
+                              obj[QLatin1String("stiffness")].toDouble(ConfigDefaults::springStiffness()),
                               ConfigDefaults::springStiffnessMax());
     if (obj.contains(QLatin1String("epsilon")))
-        sp.epsilon = qBound(ConfigDefaults::springEpsilonMin(), obj[QLatin1String("epsilon")].toDouble(0.0001),
+        sp.epsilon = qBound(ConfigDefaults::springEpsilonMin(),
+                            obj[QLatin1String("epsilon")].toDouble(ConfigDefaults::springEpsilon()),
                             ConfigDefaults::springEpsilonMax());
     return sp;
 }
@@ -111,18 +113,23 @@ AnimationProfile AnimationProfile::fromJson(const QJsonObject& obj)
     AnimationProfile p;
     if (obj.contains(QLatin1String("enabled")))
         p.enabled = obj[QLatin1String("enabled")].toBool();
-    if (obj.contains(QLatin1String("timingMode")))
-        p.timingMode = static_cast<TimingMode>(obj[QLatin1String("timingMode")].toInt());
+    if (obj.contains(QLatin1String("timingMode"))) {
+        const int tm = obj[QLatin1String("timingMode")].toInt();
+        if (tm >= 0 && tm <= 1)
+            p.timingMode = static_cast<TimingMode>(tm);
+    }
     if (obj.contains(QLatin1String("duration")))
-        p.duration = obj[QLatin1String("duration")].toInt();
+        p.duration = qBound(0, obj[QLatin1String("duration")].toInt(), 10000);
     if (obj.contains(QLatin1String("easingCurve")))
         p.easingCurve = obj[QLatin1String("easingCurve")].toString();
     if (obj.contains(QLatin1String("spring")))
         p.spring = SpringParams::fromJson(obj[QLatin1String("spring")].toObject());
     if (obj.contains(QLatin1String("style")))
         p.style = animationStyleFromString(obj[QLatin1String("style")].toString());
-    if (obj.contains(QLatin1String("styleParam")))
-        p.styleParam = obj[QLatin1String("styleParam")].toDouble();
+    if (obj.contains(QLatin1String("styleParam"))) {
+        const double sp = obj[QLatin1String("styleParam")].toDouble();
+        p.styleParam = std::isfinite(sp) ? qBound(0.0, sp, 10.0) : 0.0;
+    }
     if (obj.contains(QLatin1String("shaderPath")))
         p.shaderPath = obj[QLatin1String("shaderPath")].toString();
     if (obj.contains(QLatin1String("shaderParams")))
@@ -258,6 +265,31 @@ AnimationProfile AnimationProfileTree::resolvedProfile(const QString& eventName)
         if (it != m_profiles.end())
             resolved.mergeFrom(*it);
     }
+
+    // If the user tree left fields unset (e.g. no global node at all),
+    // fill remaining nullopt fields from the built-in default tree.
+    if (!resolved.enabled.has_value()) {
+        const auto& def = defaultTree().resolvedProfile(eventName);
+        if (!resolved.enabled.has_value())
+            resolved.enabled = def.enabled;
+        if (!resolved.timingMode.has_value())
+            resolved.timingMode = def.timingMode;
+        if (!resolved.duration.has_value())
+            resolved.duration = def.duration;
+        if (!resolved.spring.has_value())
+            resolved.spring = def.spring;
+        if (!resolved.easingCurve.has_value())
+            resolved.easingCurve = def.easingCurve;
+        if (!resolved.style.has_value())
+            resolved.style = def.style;
+        if (!resolved.styleParam.has_value())
+            resolved.styleParam = def.styleParam;
+        if (!resolved.shaderPath.has_value())
+            resolved.shaderPath = def.shaderPath;
+        if (!resolved.shaderParams.has_value())
+            resolved.shaderParams = def.shaderParams;
+    }
+
     return resolved;
 }
 
@@ -341,7 +373,8 @@ AnimationProfileTree AnimationProfileTree::defaultTree()
     global.timingMode = TimingMode::Easing;
     global.duration = 300;
     global.easingCurve = QStringLiteral("0.33,1.00,0.68,1.00"); // ease-out cubic
-    global.spring = SpringParams{1.0, 800.0, 0.0001};
+    global.spring = SpringParams{ConfigDefaults::springDampingRatio(), ConfigDefaults::springStiffness(),
+                                 ConfigDefaults::springEpsilon()};
     global.style = AnimationStyle::Morph;
     tree.setProfile(QStringLiteral("global"), global);
 

src/core/baseshaderregistry.cpp

@@ -481,8 +481,8 @@ QVariantList BaseShaderRegistry::shaderPresetsVariant(const QString& id) const
     QVariantList result;
     for (auto it = info.presets.constBegin(); it != info.presets.constEnd(); ++it) {
         QVariantMap entry;
-        entry[QStringLiteral("name")] = it.key();
-        entry[QStringLiteral("params")] = it.value();
+        entry[QLatin1String("name")] = it.key();
+        entry[QLatin1String("params")] = it.value();
         result.append(entry);
     }
     return result;
@@ -640,71 +640,71 @@ QVariantMap BaseShaderRegistry::translateParamsToUniforms(const QString& id, con
 QVariantMap BaseShaderRegistry::shaderInfoToVariantMap(const BaseShaderInfo& info) const
 {
     QVariantMap map;
-    map[QStringLiteral("id")] = info.id.isEmpty() ? QStringLiteral("unknown") : info.id;
-    map[QStringLiteral("name")] = info.name.isEmpty() ? info.id : info.name;
-    map[QStringLiteral("description")] = info.description;
-    map[QStringLiteral("author")] = info.author;
-    map[QStringLiteral("version")] = info.version;
-    map[QStringLiteral("isUserShader")] = info.isUserShader;
-    map[QStringLiteral("category")] = info.category;
-    map[QStringLiteral("isValid")] = info.isValid();
+    map[QLatin1String("id")] = info.id.isEmpty() ? QStringLiteral("unknown") : info.id;
+    map[QLatin1String("name")] = info.name.isEmpty() ? info.id : info.name;
+    map[QLatin1String("description")] = info.description;
+    map[QLatin1String("author")] = info.author;
+    map[QLatin1String("version")] = info.version;
+    map[QLatin1String("isUserShader")] = info.isUserShader;
+    map[QLatin1String("category")] = info.category;
+    map[QLatin1String("isValid")] = info.isValid();
 
     if (info.shaderUrl.isValid()) {
-        map[QStringLiteral("shaderUrl")] = info.shaderUrl.toString();
+        map[QLatin1String("shaderUrl")] = info.shaderUrl.toString();
     } else {
-        map[QStringLiteral("shaderUrl")] = QString();
+        map[QLatin1String("shaderUrl")] = QString();
     }
 
     if (!info.previewPath.isEmpty()) {
-        map[QStringLiteral("previewPath")] = info.previewPath;
+        map[QLatin1String("previewPath")] = info.previewPath;
     } else {
-        map[QStringLiteral("previewPath")] = QString();
+        map[QLatin1String("previewPath")] = QString();
     }
 
     // Parameters list
     QVariantList params;
     for (const ParameterInfo& param : info.parameters) {
         params.append(parameterInfoToVariantMap(param));
     }
-    map[QStringLiteral("parameters")] = params;
+    map[QLatin1String("parameters")] = params;
 
     // Presets list
     QVariantList presetsList;
     for (auto it = info.presets.constBegin(); it != info.presets.constEnd(); ++it) {
         QVariantMap presetMap;
-        presetMap[QStringLiteral("name")] = it.key();
-        presetMap[QStringLiteral("params")] = it.value();
+        presetMap[QLatin1String("name")] = it.key();
+        presetMap[QLatin1String("params")] = it.value();
         presetsList.append(presetMap);
     }
-    map[QStringLiteral("presets")] = presetsList;
+    map[QLatin1String("presets")] = presetsList;
 
     return map;
 }
 
 QVariantMap BaseShaderRegistry::parameterInfoToVariantMap(const ParameterInfo& param) const
 {
     QVariantMap map;
-    map[QStringLiteral("id")] = param.id;
-    map[QStringLiteral("name")] = param.name;
-    map[QStringLiteral("type")] = param.type;
-    map[QStringLiteral("slot")] = param.slot;
-    map[QStringLiteral("mapsTo")] = param.uniformName();
-    map[QStringLiteral("useZoneColor")] = param.useZoneColor;
+    map[QLatin1String("id")] = param.id;
+    map[QLatin1String("name")] = param.name;
+    map[QLatin1String("type")] = param.type;
+    map[QLatin1String("slot")] = param.slot;
+    map[QLatin1String("mapsTo")] = param.uniformName();
+    map[QLatin1String("useZoneColor")] = param.useZoneColor;
     if (!param.wrap.isEmpty()) {
-        map[QStringLiteral("wrap")] = param.wrap;
+        map[QLatin1String("wrap")] = param.wrap;
     }
 
     if (!param.group.isEmpty()) {
-        map[QStringLiteral("group")] = param.group;
+        map[QLatin1String("group")] = param.group;
     }
     if (param.defaultValue.isValid()) {
-        map[QStringLiteral("default")] = param.defaultValue;
+        map[QLatin1String("default")] = param.defaultValue;
     }
     if (param.minValue.isValid()) {
-        map[QStringLiteral("min")] = param.minValue;
+        map[QLatin1String("min")] = param.minValue;
     }
     if (param.maxValue.isValid()) {
-        map[QStringLiteral("max")] = param.maxValue;
+        map[QLatin1String("max")] = param.maxValue;
     }
 
     return map;

src/core/geometryutils.cpp

@@ -364,13 +364,13 @@ QString serializeRotationEntriesWithAnimation(const QVector<RotationEntry>& entr
     QJsonArray array;
     for (const RotationEntry& entry : entries) {
         QJsonObject obj;
-        obj[QStringLiteral("windowId")] = entry.windowId;
-        obj[QStringLiteral("sourceZoneId")] = entry.sourceZoneId;
-        obj[QStringLiteral("targetZoneId")] = entry.targetZoneId;
-        obj[QStringLiteral("x")] = entry.targetGeometry.x();
-        obj[QStringLiteral("y")] = entry.targetGeometry.y();
-        obj[QStringLiteral("width")] = entry.targetGeometry.width();
-        obj[QStringLiteral("height")] = entry.targetGeometry.height();
+        obj[QLatin1String("windowId")] = entry.windowId;
+        obj[QLatin1String("sourceZoneId")] = entry.sourceZoneId;
+        obj[QLatin1String("targetZoneId")] = entry.targetZoneId;
+        obj[QLatin1String("x")] = entry.targetGeometry.x();
+        obj[QLatin1String("y")] = entry.targetGeometry.y();
+        obj[QLatin1String("width")] = entry.targetGeometry.width();
+        obj[QLatin1String("height")] = entry.targetGeometry.height();
 
         // Resolve animation profile: __restore__ entries use zoneSnapOut
         QString event =

src/dbus/windowtrackingadaptor.h

@@ -4,6 +4,7 @@
 #pragma once
 
 #include "plasmazones_export.h"
+#include "../core/animationprofile.h"
 #include "../core/windowtrackingservice.h"
 #include <QObject>
 #include <QDBusAbstractAdaptor>
@@ -907,6 +908,13 @@ private Q_SLOTS:
      */
     QString resolveScreenForSnap(const QString& callerScreen, const QString& zoneId) const;
 
+    /**
+     * @brief Resolve animation profile from settings, returning empty profile if settings unavailable
+     * @param eventName Animation event name (e.g. "zoneSnapIn", "zoneSnapOut")
+     * @return Resolved profile, or default-constructed AnimationProfile{} if no settings
+     */
+    AnimationProfile resolvedProfileOrDefault(const QString& eventName) const;
+
     /**
      * @brief Apply a successful SnapResult: assign outputs, mark auto-snapped,
      *        clear floating state, and track the zone assignment.

src/dbus/windowtrackingadaptor/convenience.cpp

@@ -13,6 +13,14 @@
 
 namespace PlasmaZones {
 
+AnimationProfile WindowTrackingAdaptor::resolvedProfileOrDefault(const QString& eventName) const
+{
+    if (m_settings) {
+        return m_settings->animationProfileTree().resolvedProfile(eventName);
+    }
+    return AnimationProfile{};
+}
+
 void WindowTrackingAdaptor::moveWindowToZone(const QString& windowId, const QString& zoneId)
 {
     if (!validateWindowId(windowId, QStringLiteral("moveWindowToZone"))) {
@@ -39,8 +47,7 @@ void WindowTrackingAdaptor::moveWindowToZone(const QString& windowId, const QStr
     m_service->recordSnapIntent(windowId, true);
 
     // Request compositor to apply geometry (with animation metadata)
-    auto snapInProfile = m_settings ? m_settings->animationProfileTree().resolvedProfile(QStringLiteral("zoneSnapIn"))
-                                    : AnimationProfile{};
+    auto snapInProfile = resolvedProfileOrDefault(QStringLiteral("zoneSnapIn"));
     QString geometryJson = GeometryUtils::rectToJsonWithAnimation(geo, snapInProfile);
     Q_EMIT applyGeometryRequested(windowId, geometryJson, zoneId, screenId);
 
@@ -88,8 +95,7 @@ void WindowTrackingAdaptor::swapWindowsById(const QString& windowId1, const QStr
     windowSnapped(windowId2, zoneId1, screen1);
 
     // Emit geometry requests for both (with animation metadata)
-    auto snapInProfile = m_settings ? m_settings->animationProfileTree().resolvedProfile(QStringLiteral("zoneSnapIn"))
-                                    : AnimationProfile{};
+    auto snapInProfile = resolvedProfileOrDefault(QStringLiteral("zoneSnapIn"));
     Q_EMIT applyGeometryRequested(windowId1, GeometryUtils::rectToJsonWithAnimation(geo1, snapInProfile), zoneId2,
                                   screen2);
     Q_EMIT applyGeometryRequested(windowId2, GeometryUtils::rectToJsonWithAnimation(geo2, snapInProfile), zoneId1,