Avoid calling updateState() for completed animations and add tests (#4737)

* Fix redundant updateState call for finished animations

* Narrow finished-animation fix to AnimationManager and expand lifecycle tests

Author: liannacasper

CodenameOne/src/com/codename1/ui/AnimationManager.java

@@ -69,7 +69,7 @@ void updateAnimations() {
             if (c.isInProgress()) {
                 c.updateAnimationState();
             } else {
-                c.updateAnimationState();
+                c.completeAnimation();
                 anims.remove(c);
             }
         } else {

CodenameOne/src/com/codename1/ui/animations/ComponentAnimation.java

@@ -126,24 +126,35 @@ public int getMaxSteps() {
     public final void updateAnimationState() {
         updateState();
         if (!isInProgress()) {
-            if (!completed) {
-                completed = true;
-                if (notifyLock != null) {
-                    synchronized (notifyLock) {
-                        notifyLock.notifyAll();
-                    }
-                }
-                if (onCompletion != null) {
-                    onCompletion.run();
+            completeIfNeeded();
+        } else { //ensure completed would be set to false if animation has been restarted
+            completed = false;
+        }
+    }
+    
+    /// Completes the animation if needed, without forcing an additional updateState() call.
+    public final void completeAnimation() {
+        if (!isInProgress()) {
+            completeIfNeeded();
+        }
+    }
+    
+    private void completeIfNeeded() {
+        if (!completed) {
+            completed = true;
+            if (notifyLock != null) {
+                synchronized (notifyLock) {
+                    notifyLock.notifyAll();
                 }
-                if (post != null) {
-                    for (Runnable p : post) {
-                        p.run();
-                    }
+            }
+            if (onCompletion != null) {
+                onCompletion.run();
+            }
+            if (post != null) {
+                for (Runnable p : post) {
+                    p.run();
                 }
             }
-        } else { //ensure completed would be set to false if animation has been restarted
-            completed = false;
         }
     }
 

maven/core-unittests/src/test/java/com/codename1/ui/AnimationManagerTest.java

@@ -0,0 +1,76 @@
+package com.codename1.ui;
+
+import com.codename1.junit.FormTest;
+import com.codename1.junit.UITestBase;
+import com.codename1.ui.animations.ComponentAnimation;
+import org.junit.jupiter.api.Test;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+class AnimationManagerTest extends UITestBase {
+
+    @FormTest
+    void testFinishedAnimationDoesNotUpdateStateAgainBeforeRemoval() {
+        Form form = CN.getCurrentForm();
+        AnimationManager manager = form.getAnimationManager();
+        AtomicInteger updateStateCalls = new AtomicInteger();
+        AtomicInteger completionCalls = new AtomicInteger();
+
+        ComponentAnimation animation = new ComponentAnimation() {
+            private int remainingSteps = 1;
+
+            @Override
+            public boolean isInProgress() {
+                return remainingSteps > 0;
+            }
+
+            @Override
+            protected void updateState() {
+                updateStateCalls.incrementAndGet();
+                remainingSteps--;
+            }
+        };
+
+        manager.addAnimation(animation, completionCalls::incrementAndGet);
+
+        manager.updateAnimations();
+        assertEquals(1, updateStateCalls.get(), "Animation should update once while in progress");
+        assertEquals(1, completionCalls.get(), "Completion callback should run exactly once");
+        assertFalse(manager.isAnimating(), "Animation should report as not animating once completed");
+
+        manager.updateAnimations();
+        assertEquals(1, updateStateCalls.get(), "Finished animation should not receive another update");
+        assertEquals(1, completionCalls.get(), "Completion callback should not run a second time");
+    }
+
+    @Test
+    void testAlreadyFinishedAnimationRunsCompletionWithoutUpdateState() {
+        Form form = new Form();
+        AnimationManager manager = form.getAnimationManager();
+        AtomicInteger updateStateCalls = new AtomicInteger();
+        AtomicInteger completionCalls = new AtomicInteger();
+
+        ComponentAnimation animation = new ComponentAnimation() {
+            @Override
+            public boolean isInProgress() {
+                return false;
+            }
+
+            @Override
+            protected void updateState() {
+                updateStateCalls.incrementAndGet();
+            }
+        };
+
+        manager.addAnimation(animation, completionCalls::incrementAndGet);
+        assertFalse(manager.isAnimating(), "Already-finished animation should not mark manager as animating");
+
+        manager.updateAnimations();
+
+        assertEquals(0, updateStateCalls.get(), "Already-finished animation must not mutate state");
+        assertEquals(1, completionCalls.get(), "Completion callback should still run once");
+    }
+}

maven/core-unittests/src/test/java/com/codename1/ui/animations/ComponentAnimationLifecycleTest.java

@@ -0,0 +1,71 @@
+package com.codename1.ui.animations;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+class ComponentAnimationLifecycleTest {
+
+    @Test
+    void testUpdateAnimationStateStillInvokesUpdateStateWhenAlreadyFinished() {
+        AtomicInteger updateStateCalls = new AtomicInteger();
+        AtomicInteger completionCalls = new AtomicInteger();
+
+        ComponentAnimation animation = new ComponentAnimation() {
+            @Override
+            public boolean isInProgress() {
+                return false;
+            }
+
+            @Override
+            protected void updateState() {
+                updateStateCalls.incrementAndGet();
+            }
+        };
+        animation.setOnCompletion(completionCalls::incrementAndGet);
+
+        animation.updateAnimationState();
+
+        assertEquals(1, updateStateCalls.get(), "Direct updateAnimationState() should still invoke updateState()");
+        assertEquals(1, completionCalls.get(), "Completion callback should run when animation is finished");
+    }
+
+    @Test
+    void testRestartedAnimationResetsCompletionLifecycle() {
+        AtomicInteger completionCalls = new AtomicInteger();
+
+        class RestartableAnimation extends ComponentAnimation {
+            private int remainingSteps = 2;
+
+            @Override
+            public boolean isInProgress() {
+                return remainingSteps > 0;
+            }
+
+            @Override
+            protected void updateState() {
+                remainingSteps--;
+            }
+
+            public void restart() {
+                remainingSteps = 2;
+            }
+        }
+        RestartableAnimation animation = new RestartableAnimation();
+        animation.setOnCompletion(completionCalls::incrementAndGet);
+
+        animation.updateAnimationState();
+        animation.updateAnimationState();
+        assertEquals(1, completionCalls.get(), "Completion should run for first lifecycle");
+
+        animation.updateAnimationState();
+        assertEquals(1, completionCalls.get(), "No extra completion should fire without restart");
+
+        animation.restart();
+        animation.updateAnimationState();
+        animation.updateAnimationState();
+        assertEquals(2, completionCalls.get(), "Completion should run again after restart");
+    }
+}