Skip to content

Commit f6884f1

Browse files
fuddlesworthruvnet
fuddlesworth
and
ruvnet
committed
fix(animations): PR #291 review fixes — bugs, architecture, DRY, conventions
- Shared tree hierarchy: create animationtreedata.h as single source of truth for animation event tree edges, eliminating duplication between animationprofile.cpp and plasmazoneseffect.cpp - ConfigKeys alignment: AnimationParams::fromJson now uses ConfigKeys accessors instead of inline QLatin1String literals - Default tree fallback: resolveAnimationFromCachedTree fills missing fields with defaults matching the daemon's defaultTree() - Bounce off-by-one: clamp arc parameter u to [0,1] via qBound - Overlay style handling: explicit switch cases for FadeIn/SlideUp/ScaleIn with debug log and morph fallback in WindowAnimator::applyTransform - usesOpacity: include overlay styles in opacity check - Opacity count guard: qMax(0,...) after every decrement to prevent negative m_opacityAnimationCount - File-watching dedup: extract watchShaderDirectory() and watchAllShaderDirectories() helpers in BaseShaderRegistry - validateParams delegation: ShaderRegistry delegates to base validateParameterValue() instead of reimplementing type checks Co-Authored-By: claude-flow <ruv@ruv.net>
1 parent 0314cbc commit f6884f1

8 files changed

Lines changed: 192 additions & 238 deletions

File tree

‎kwin-effect/plasmazoneseffect.cpp‎

Lines changed: 20 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
#include "plasmazoneseffect.h"
55

6+
#include "../src/common/animationtreedata.h"
67
#include "../src/config/configkeys.h"
78
#include <algorithm>
89
#include <memory>
@@ -2710,45 +2711,13 @@ std::optional<AnimationParams> PlasmaZonesEffect::resolveAnimationFromCachedTree
27102711
if (m_cachedProfileTreeJson.isEmpty())
27112712
return std::nullopt;
27122713

2713-
// IMPORTANT: Keep in sync with src/core/animationprofile.h AnimationProfileTree hierarchy
2714-
// Static parent lookup (mirrors daemon's AnimationProfileTree::parentOf)
2715-
static const QHash<QString, QString> parentMap = {
2716-
// Domain nodes → global
2717-
{QStringLiteral("windowGeometry"), QStringLiteral("global")},
2718-
{QStringLiteral("overlay"), QStringLiteral("global")},
2719-
{QStringLiteral("dim"), QStringLiteral("global")},
2720-
// Window geometry categories
2721-
{QStringLiteral("snap"), QStringLiteral("windowGeometry")},
2722-
{QStringLiteral("snapIn"), QStringLiteral("snap")},
2723-
{QStringLiteral("snapOut"), QStringLiteral("snap")},
2724-
{QStringLiteral("snapResize"), QStringLiteral("snap")},
2725-
{QStringLiteral("layoutSwitch"), QStringLiteral("windowGeometry")},
2726-
{QStringLiteral("layoutSwitchIn"), QStringLiteral("layoutSwitch")},
2727-
{QStringLiteral("layoutSwitchOut"), QStringLiteral("layoutSwitch")},
2728-
{QStringLiteral("autotileBorder"), QStringLiteral("windowGeometry")},
2729-
{QStringLiteral("borderIn"), QStringLiteral("autotileBorder")},
2730-
{QStringLiteral("borderOut"), QStringLiteral("autotileBorder")},
2731-
// Overlay categories
2732-
{QStringLiteral("zoneHighlight"), QStringLiteral("overlay")},
2733-
{QStringLiteral("zoneHighlightIn"), QStringLiteral("zoneHighlight")},
2734-
{QStringLiteral("zoneHighlightOut"), QStringLiteral("zoneHighlight")},
2735-
{QStringLiteral("osd"), QStringLiteral("overlay")},
2736-
{QStringLiteral("layoutOsdIn"), QStringLiteral("osd")},
2737-
{QStringLiteral("layoutOsdOut"), QStringLiteral("osd")},
2738-
{QStringLiteral("navigationOsdIn"), QStringLiteral("osd")},
2739-
{QStringLiteral("navigationOsdOut"), QStringLiteral("osd")},
2740-
{QStringLiteral("popup"), QStringLiteral("overlay")},
2741-
{QStringLiteral("layoutPickerIn"), QStringLiteral("popup")},
2742-
{QStringLiteral("layoutPickerOut"), QStringLiteral("popup")},
2743-
{QStringLiteral("snapAssistIn"), QStringLiteral("popup")},
2744-
{QStringLiteral("snapAssistOut"), QStringLiteral("popup")},
2745-
{QStringLiteral("zoneSelector"), QStringLiteral("overlay")},
2746-
{QStringLiteral("zoneSelectorIn"), QStringLiteral("zoneSelector")},
2747-
{QStringLiteral("zoneSelectorOut"), QStringLiteral("zoneSelector")},
2748-
{QStringLiteral("preview"), QStringLiteral("overlay")},
2749-
{QStringLiteral("previewIn"), QStringLiteral("preview")},
2750-
{QStringLiteral("previewOut"), QStringLiteral("preview")},
2751-
};
2714+
// Parent lookup built from shared tree data (single source of truth in animationtreedata.h)
2715+
static const QHash<QString, QString> parentMap = [] {
2716+
QHash<QString, QString> m;
2717+
for (const auto& edge : AnimationTreeEdges)
2718+
m[QString::fromLatin1(edge.child)] = QString::fromLatin1(edge.parent);
2719+
return m;
2720+
}();
27522721

27532722
// Build chain from root to eventName
27542723
QStringList chain;
@@ -2772,6 +2741,18 @@ std::optional<AnimationParams> PlasmaZonesEffect::resolveAnimationFromCachedTree
27722741
if (merged.isEmpty())
27732742
return std::nullopt;
27742743

2744+
// Fill defaults for fields not present (mirrors daemon's defaultTree() fallback)
2745+
if (!merged.contains(ConfigKeys::animProfileEnabledKey()))
2746+
merged[ConfigKeys::animProfileEnabledKey()] = true;
2747+
if (!merged.contains(ConfigKeys::animProfileTimingModeKey()))
2748+
merged[ConfigKeys::animProfileTimingModeKey()] = 0; // Easing
2749+
if (!merged.contains(ConfigKeys::animProfileDurationKey()))
2750+
merged[ConfigKeys::animProfileDurationKey()] = 300;
2751+
if (!merged.contains(ConfigKeys::animProfileEasingCurveKey()))
2752+
merged[ConfigKeys::animProfileEasingCurveKey()] = QStringLiteral("0.33,1.00,0.68,1.00");
2753+
if (!merged.contains(ConfigKeys::animProfileStyleKey()))
2754+
merged[ConfigKeys::animProfileStyleKey()] = QStringLiteral("morph");
2755+
27752756
return AnimationParams::fromJson(merged);
27762757
}
27772758

‎kwin-effect/windowanimator.cpp‎

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// SPDX-License-Identifier: GPL-3.0-or-later
33

44
#include "windowanimator.h"
5+
#include "../src/config/configkeys.h"
56

67
#include <effect/effect.h>
78
#include <effect/effecthandler.h>
@@ -62,7 +63,7 @@ qreal EasingCurve::evaluateBounceOut(qreal t, qreal amp, int n)
6263
for (int k = 0; k < n; ++k) {
6364
const qreal dk = d * qPow(r, k);
6465
if (t < tAccum + dk || k == n - 1) {
65-
const qreal u = (t - tAccum) / dk; // 0..1 within this arc
66+
const qreal u = qBound(0.0, (t - tAccum) / dk, 1.0); // 0..1 within this arc
6667
const qreal height = qPow(r, 2 * (k + 1)); // standard dip depth
6768
// Parabola: 1.0 at u=0 and u=1, dips to (1 - height*amp) at u=0.5
6869
const qreal dip = 1.0 - 4.0 * (u - 0.5) * (u - 0.5); // 0 at edges, 1 at center
@@ -353,26 +354,27 @@ AnimationParams AnimationParams::fromJson(const QJsonObject& obj)
353354
if (obj.isEmpty())
354355
return p;
355356

356-
p.timingMode = static_cast<TimingMode>(obj.value(QLatin1String("timingMode")).toInt(0));
357-
p.duration = obj.value(QLatin1String("duration")).toInt(-1);
358-
p.easingCurveStr = obj.value(QLatin1String("easingCurve")).toString();
357+
// Use ConfigKeys accessors to stay in sync with AnimationProfile::toJson() on the daemon side
358+
p.timingMode = static_cast<TimingMode>(obj.value(ConfigKeys::animProfileTimingModeKey()).toInt(0));
359+
p.duration = obj.value(ConfigKeys::animProfileDurationKey()).toInt(-1);
360+
p.easingCurveStr = obj.value(ConfigKeys::animProfileEasingCurveKey()).toString();
359361

360-
if (obj.contains(QLatin1String("spring"))) {
361-
QJsonObject sp = obj.value(QLatin1String("spring")).toObject();
362-
p.spring.dampingRatio = qBound(0.1, sp.value(QLatin1String("dampingRatio")).toDouble(1.0), 10.0);
363-
p.spring.stiffness = qBound(1.0, sp.value(QLatin1String("stiffness")).toDouble(800.0), 2000.0);
364-
p.spring.epsilon = qBound(0.00001, sp.value(QLatin1String("epsilon")).toDouble(0.0001), 0.1);
362+
if (obj.contains(ConfigKeys::animProfileSpringKey())) {
363+
QJsonObject sp = obj.value(ConfigKeys::animProfileSpringKey()).toObject();
364+
p.spring.dampingRatio = qBound(0.1, sp.value(ConfigKeys::animProfileSpringDampingKey()).toDouble(1.0), 10.0);
365+
p.spring.stiffness = qBound(1.0, sp.value(ConfigKeys::animProfileSpringStiffnessKey()).toDouble(800.0), 2000.0);
366+
p.spring.epsilon = qBound(0.00001, sp.value(ConfigKeys::animProfileSpringEpsilonKey()).toDouble(0.0001), 0.1);
365367
p.spring.initialVelocity = qBound(-10.0, sp.value(QLatin1String("initialVelocity")).toDouble(0.0), 10.0);
366368
}
367369

368-
p.style = animationStyleFromString(obj.value(QLatin1String("style")).toString());
369-
p.styleParam = obj.value(QLatin1String("styleParam")).toDouble(0.87);
370+
p.style = animationStyleFromString(obj.value(ConfigKeys::animProfileStyleKey()).toString());
371+
p.styleParam = obj.value(ConfigKeys::animProfileStyleParamKey()).toDouble(0.87);
370372

371-
if (obj.contains(QLatin1String("enabled")))
372-
p.enabled = obj.value(QLatin1String("enabled")).toBool(true);
373+
if (obj.contains(ConfigKeys::animProfileEnabledKey()))
374+
p.enabled = obj.value(ConfigKeys::animProfileEnabledKey()).toBool(true);
373375

374-
p.shaderPath = obj.value(QLatin1String("shaderPath")).toString();
375-
p.geometryMode = obj.value(QLatin1String("geometry")).toString(QStringLiteral("morph"));
376+
p.shaderPath = obj.value(ConfigKeys::animProfileShaderPathKey()).toString();
377+
p.geometryMode = obj.value(ConfigKeys::animProfileGeometryKey()).toString(QStringLiteral("morph"));
376378

377379
return p;
378380
}
@@ -474,6 +476,7 @@ void WindowAnimator::removeAnimation(KWin::EffectWindow* window)
474476
if (it->usesOpacity())
475477
--m_opacityAnimationCount;
476478
m_animations.erase(it);
479+
m_opacityAnimationCount = qMax(0, m_opacityAnimationCount);
477480
}
478481
}
479482

@@ -525,6 +528,7 @@ void WindowAnimator::advanceAnimations(std::chrono::milliseconds presentTime)
525528
if (it->usesOpacity())
526529
--m_opacityAnimationCount;
527530
m_animations.erase(it);
531+
m_opacityAnimationCount = qMax(0, m_opacityAnimationCount);
528532
continue;
529533
}
530534

@@ -538,6 +542,7 @@ void WindowAnimator::advanceAnimations(std::chrono::milliseconds presentTime)
538542
if (it->usesOpacity())
539543
--m_opacityAnimationCount;
540544
m_animations.erase(it);
545+
m_opacityAnimationCount = qMax(0, m_opacityAnimationCount);
541546
window->addRepaintFull();
542547
if (bounds.isValid()) {
543548
KWin::effects->addRepaint(bounds.toAlignedRect());
@@ -575,6 +580,13 @@ void WindowAnimator::applyTransform(KWin::EffectWindow* window, KWin::WindowPain
575580
case AnimationStyle::SlideFade:
576581
applySlideFadeTransform(window, *it, data);
577582
break;
583+
case AnimationStyle::FadeIn:
584+
case AnimationStyle::SlideUp:
585+
case AnimationStyle::ScaleIn:
586+
// Overlay-only styles should not reach compositor window animations; fall back to morph
587+
qCDebug(lcEffect) << "Overlay style" << static_cast<int>(it->style) << "in window animation, using morph";
588+
applyMorphTransform(window, *it, data);
589+
break;
578590
case AnimationStyle::Morph:
579591
case AnimationStyle::Custom:
580592
default:

‎kwin-effect/windowanimator.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,8 @@ struct WindowAnimation
172172
/// Whether this animation uses opacity (needs translucent rendering)
173173
bool usesOpacity() const
174174
{
175-
return style == AnimationStyle::Slide || style == AnimationStyle::Popin || style == AnimationStyle::SlideFade;
175+
return style == AnimationStyle::Slide || style == AnimationStyle::Popin || style == AnimationStyle::SlideFade
176+
|| style == AnimationStyle::FadeIn || style == AnimationStyle::SlideUp || style == AnimationStyle::ScaleIn;
176177
}
177178

178179
/// Check if the animation has been initialized

‎src/common/animationtreedata.h‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// SPDX-FileCopyrightText: 2026 fuddlesworth
2+
// SPDX-License-Identifier: GPL-3.0-or-later
3+
4+
#pragma once
5+
6+
namespace PlasmaZones {
7+
8+
/**
9+
* @brief Single source of truth for the animation event tree hierarchy.
10+
*
11+
* Shared between src/core/animationprofile.cpp (daemon) and
12+
* kwin-effect/plasmazoneseffect.cpp (compositor plugin) via header-only include.
13+
*
14+
* "global" is the root and has no entry here (no parent).
15+
*
16+
* Tree structure:
17+
* global
18+
* ├── windowGeometry
19+
* │ ├── snap → snapIn, snapOut, snapResize
20+
* │ ├── layoutSwitch → layoutSwitchIn, layoutSwitchOut
21+
* │ └── autotileBorder → borderIn, borderOut
22+
* ├── overlay
23+
* │ ├─�� zoneHighlight → zoneHighlightIn, zoneHighlightOut
24+
* �� ├── osd → layoutOsdIn, layoutOsdOut, navigationOsdIn, navigationOsdOut
25+
* │ ├── popup → layoutPickerIn, layoutPickerOut, snapAssistIn, snapAssistOut
26+
* │ ├── zoneSelector → zoneSelectorIn, zoneSelectorOut
27+
* │ └── preview → previewIn, previewOut
28+
* └── dim
29+
*/
30+
31+
struct AnimationTreeEdge
32+
{
33+
const char* child;
34+
const char* parent;
35+
};
36+
37+
// clang-format off
38+
static constexpr AnimationTreeEdge AnimationTreeEdges[] = {
39+
// Domain nodes → global
40+
{"windowGeometry", "global"},
41+
{"overlay", "global"},
42+
{"dim", "global"},
43+
// Window geometry categories
44+
{"snap", "windowGeometry"},
45+
{"snapIn", "snap"},
46+
{"snapOut", "snap"},
47+
{"snapResize", "snap"},
48+
{"layoutSwitch", "windowGeometry"},
49+
{"layoutSwitchIn", "layoutSwitch"},
50+
{"layoutSwitchOut", "layoutSwitch"},
51+
{"autotileBorder", "windowGeometry"},
52+
{"borderIn", "autotileBorder"},
53+
{"borderOut", "autotileBorder"},
54+
// Overlay categories
55+
{"zoneHighlight", "overlay"},
56+
{"zoneHighlightIn", "zoneHighlight"},
57+
{"zoneHighlightOut", "zoneHighlight"},
58+
{"osd", "overlay"},
59+
{"layoutOsdIn", "osd"},
60+
{"layoutOsdOut", "osd"},
61+
{"navigationOsdIn", "osd"},
62+
{"navigationOsdOut", "osd"},
63+
{"popup", "overlay"},
64+
{"layoutPickerIn", "popup"},
65+
{"layoutPickerOut", "popup"},
66+
{"snapAssistIn", "popup"},
67+
{"snapAssistOut", "popup"},
68+
{"zoneSelector", "overlay"},
69+
{"zoneSelectorIn", "zoneSelector"},
70+
{"zoneSelectorOut", "zoneSelector"},
71+
{"preview", "overlay"},
72+
{"previewIn", "preview"},
73+
{"previewOut", "preview"},
74+
};
75+
// clang-format on
76+
77+
static constexpr int AnimationTreeEdgeCount = sizeof(AnimationTreeEdges) / sizeof(AnimationTreeEdges[0]);
78+
79+
} // namespace PlasmaZones

‎src/core/animationprofile.cpp‎

Lines changed: 5 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
#include "animationprofile.h"
55
#include "interfaces.h"
6+
#include "../common/animationtreedata.h"
67
#include "../config/configdefaults.h"
78
#include <QJsonArray>
89
#include <QJsonDocument>
@@ -221,57 +222,13 @@ bool AnimationProfile::operator==(const AnimationProfile& other) const
221222
// AnimationProfileTree — static tree structure
222223
// ═══════════════════════════════════════════════════════════════════════════════
223224

224-
// Single source of truth for the animation event tree hierarchy.
225-
// "global" is the root and has no entry here (no parent).
226-
struct TreeEdge
227-
{
228-
const char* child;
229-
const char* parent;
230-
};
231-
232-
static constexpr TreeEdge treeEdges[] = {
233-
// Domain nodes
234-
{"windowGeometry", "global"},
235-
{"overlay", "global"},
236-
{"dim", "global"},
237-
// Window geometry categories
238-
{"snap", "windowGeometry"},
239-
{"snapIn", "snap"},
240-
{"snapOut", "snap"},
241-
{"snapResize", "snap"},
242-
{"layoutSwitch", "windowGeometry"},
243-
{"layoutSwitchIn", "layoutSwitch"},
244-
{"layoutSwitchOut", "layoutSwitch"},
245-
{"autotileBorder", "windowGeometry"},
246-
{"borderIn", "autotileBorder"},
247-
{"borderOut", "autotileBorder"},
248-
// Overlay categories
249-
{"zoneHighlight", "overlay"},
250-
{"zoneHighlightIn", "zoneHighlight"},
251-
{"zoneHighlightOut", "zoneHighlight"},
252-
{"osd", "overlay"},
253-
{"layoutOsdIn", "osd"},
254-
{"layoutOsdOut", "osd"},
255-
{"navigationOsdIn", "osd"},
256-
{"navigationOsdOut", "osd"},
257-
{"popup", "overlay"},
258-
{"layoutPickerIn", "popup"},
259-
{"layoutPickerOut", "popup"},
260-
{"snapAssistIn", "popup"},
261-
{"snapAssistOut", "popup"},
262-
{"zoneSelector", "overlay"},
263-
{"zoneSelectorIn", "zoneSelector"},
264-
{"zoneSelectorOut", "zoneSelector"},
265-
{"preview", "overlay"},
266-
{"previewIn", "preview"},
267-
{"previewOut", "preview"},
268-
};
225+
// Tree hierarchy data is in src/common/animationtreedata.h (shared with kwin-effect).
269226

270227
static const QHash<QString, QString>& parentMap()
271228
{
272229
static const QHash<QString, QString> map = [] {
273230
QHash<QString, QString> m;
274-
for (const auto& edge : treeEdges)
231+
for (const auto& edge : AnimationTreeEdges)
275232
m[QString::fromLatin1(edge.child)] = QString::fromLatin1(edge.parent);
276233
return m;
277234
}();
@@ -282,7 +239,7 @@ static const QHash<QString, QStringList>& childrenMap()
282239
{
283240
static const QHash<QString, QStringList> map = [] {
284241
QHash<QString, QStringList> m;
285-
for (const auto& edge : treeEdges)
242+
for (const auto& edge : AnimationTreeEdges)
286243
m[QString::fromLatin1(edge.parent)].append(QString::fromLatin1(edge.child));
287244
return m;
288245
}();
@@ -294,7 +251,7 @@ static const QStringList& allNames()
294251
static const QStringList names = [] {
295252
QStringList list;
296253
list.append(QStringLiteral("global"));
297-
for (const auto& edge : treeEdges)
254+
for (const auto& edge : AnimationTreeEdges)
298255
list.append(QString::fromLatin1(edge.child));
299256
return list;
300257
}();

0 commit comments

Comments
 (0)