WindowServer: Only register animations when they're running

This allows us to keep Animation objects around, and the compositor
will only use them when the animation is actually running.

Userland/Services/WindowServer/Animation.cpp

@@ -10,14 +10,9 @@
 
 namespace WindowServer {
 
-Animation::Animation()
-{
-    Compositor::the().register_animation({}, *this);
-}
-
 Animation::~Animation()
 {
-    if (!m_was_removed)
+    if (m_running)
         Compositor::the().unregister_animation({}, *this);
 }
 
@@ -28,24 +23,36 @@ void Animation::set_duration(int duration_in_ms)
 
 void Animation::start()
 {
+    if (m_running)
+        return;
     m_running = true;
     m_timer.start();
-    Compositor::the().animation_started({});
+    Compositor::the().register_animation({}, *this);
 }
 
 void Animation::stop()
 {
+    if (!m_running)
+        return;
     m_running = false;
+    Compositor::the().unregister_animation({}, *this);
+
+    if (on_stop)
+        on_stop();
+}
+
+void Animation::call_stop_handler(Badge<Compositor>)
+{
     if (on_stop)
         on_stop();
 }
 
 void Animation::was_removed(Badge<Compositor>)
 {
-    m_was_removed = true;
+    m_running = false;
 }
 
-bool Animation::update(Badge<Compositor>, Gfx::Painter& painter, Screen& screen, Gfx::DisjointIntRectSet& flush_rects)
+bool Animation::update(Gfx::Painter& painter, Screen& screen, Gfx::DisjointIntRectSet& flush_rects)
 {
     i64 const elapsed_ms = m_timer.elapsed();
     float progress = min((float)elapsed_ms / (float)m_duration, 1.0f);

Userland/Services/WindowServer/Animation.h

@@ -33,18 +33,18 @@ public:
     void set_duration(int duration_in_ms);
     int duration() const { return m_duration; }
 
-    bool update(Badge<Compositor>, Gfx::Painter&, Screen&, Gfx::DisjointIntRectSet& flush_rects);
+    bool update(Gfx::Painter&, Screen&, Gfx::DisjointIntRectSet& flush_rects);
+    void call_stop_handler(Badge<Compositor>);
 
     Function<void(float progress, Gfx::Painter&, Screen&, Gfx::DisjointIntRectSet& flush_rects)> on_update;
     Function<void()> on_stop;
 
 private:
-    Animation();
+    Animation() = default;
 
     Core::ElapsedTimer m_timer;
     int m_duration { 0 };
     bool m_running { false };
-    bool m_was_removed { false };
 };
 
 }

Userland/Services/WindowServer/Compositor.cpp

@@ -17,6 +17,7 @@
 #include <AK/Debug.h>
 #include <AK/Memory.h>
 #include <AK/ScopeGuard.h>
+#include <AK/TemporaryChange.h>
 #include <LibCore/Timer.h>
 #include <LibGfx/AntiAliasingPainter.h>
 #include <LibGfx/Font/Font.h>
@@ -1527,43 +1528,45 @@ void Compositor::recompute_occlusions()
 
 void Compositor::register_animation(Badge<Animation>, Animation& animation)
 {
+    VERIFY(!m_animations_running);
     bool was_empty = m_animations.is_empty();
     auto result = m_animations.set(&animation);
     VERIFY(result == AK::HashSetResult::InsertedNewEntry);
-    if (was_empty)
+    if (was_empty) {
-        start_compose_async_timer();
-}
-
-void Compositor::animation_started(Badge<Animation>)
-{
         m_invalidated_any = true;
         start_compose_async_timer();
     }
+}
 
 void Compositor::unregister_animation(Badge<Animation>, Animation& animation)
 {
+    VERIFY(!m_animations_running);
     bool was_removed = m_animations.remove(&animation);
     VERIFY(was_removed);
 }
 
 void Compositor::update_animations(Screen& screen, Gfx::DisjointIntRectSet& flush_rects)
 {
+    Vector<NonnullRefPtr<Animation>, 16> finished_animations;
+    ScopeGuard call_stop_handlers([&] {
+        for (auto& animation : finished_animations)
+            animation->call_stop_handler({});
+    });
+
+    TemporaryChange animations_running(m_animations_running, true);
     auto& painter = *screen.compositor_screen_data().m_back_painter;
     // Iterating over the animations using remove_all_matching we can iterate
     // and immediately remove finished animations without having to keep track
     // of them in a separate container.
     m_animations.remove_all_matching([&](auto* animation) {
-        if (!animation->update({}, painter, screen, flush_rects)) {
+        VERIFY(animation->is_running());
+        if (!animation->update(painter, screen, flush_rects)) {
             // Mark it as removed so that the Animation::on_stop handler doesn't
             // trigger the Animation object from being destroyed, causing it to
             // unregister while we still loop over them.
             animation->was_removed({});
 
-            // Temporarily bump the ref count so that if the Animation::on_stop
+            finished_animations.append(*animation);
-            // handler clears its own reference, it doesn't immediately destroy
-            // itself while we're still in the Function<> call
-            NonnullRefPtr<Animation> protect_animation(*animation);
-            animation->stop();
             return true;
         }
         return false;

Userland/Services/WindowServer/Compositor.h

@@ -130,7 +130,6 @@ public:
         invalidate_screen();
     }
 
-    void animation_started(Badge<Animation>);
     void invalidate_occlusions() { m_occlusions_dirty = true; }
     void overlay_rects_changed();
 
@@ -223,6 +222,7 @@ private:
     bool m_invalidated_window { false };
     bool m_invalidated_cursor { false };
     bool m_overlay_rects_changed { false };
+    bool m_animations_running { false };
 
     IntrusiveList<&Overlay::m_list_node> m_overlay_list;
     Gfx::DisjointIntRectSet m_overlay_rects;