Merge pull request #5975 from bdach/key-binding-container-unrooting

Fix `KeyBindingContainer` propagating release events to removed drawables
ppy · Aug 22, 2023 · 036defa · 036defa
2 parents d8052f2 + d1aa9e1
commit 036defa
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 15 deletions.
diff --git a/osu.Framework.Tests/Visual/Input/TestSceneKeyBindingContainer.cs b/osu.Framework.Tests/Visual/Input/TestSceneKeyBindingContainer.cs
@@ -71,14 +71,14 @@ public void TestPressKeyBeforeKeyBindingContainerAdded()
             });
 
             AddStep("press key A", () => InputManager.PressKey(Key.A));
-            AddAssert("only one action triggered", () => pressedActions.Count == 1);
-            AddAssert("ActionA triggered", () => pressedActions[0] == TestAction.ActionA);
-            AddAssert("no actions released", () => releasedActions.Count == 0);
+            AddAssert("only one action triggered", () => pressedActions, () => Has.Count.EqualTo(1));
+            AddAssert("ActionA triggered", () => pressedActions[0], () => Is.EqualTo(TestAction.ActionA));
+            AddAssert("no actions released", () => releasedActions, () => Is.Empty);
 
             AddStep("release key A", () => InputManager.ReleaseKey(Key.A));
-            AddAssert("only one action triggered", () => pressedActions.Count == 1);
-            AddAssert("only one action released", () => releasedActions.Count == 1);
-            AddAssert("ActionA released", () => releasedActions[0] == TestAction.ActionA);
+            AddAssert("only one action triggered", () => pressedActions, () => Has.Count.EqualTo(1));
+            AddAssert("only one action released", () => releasedActions, () => Has.Count.EqualTo(1));
+            AddAssert("ActionA released", () => releasedActions[0], () => Is.EqualTo(TestAction.ActionA));
         }
 
         [Test]
@@ -126,8 +126,8 @@ public void TestKeyHandledByOtherDrawableDoesNotTrigger()
             AddStep("release enter", () => InputManager.ReleaseKey(Key.Enter));
             AddStep("release mouse button", () => InputManager.ReleaseButton(MouseButton.Left));
 
-            AddAssert("no pressed actions", () => pressedActions.Count == 0);
-            AddAssert("no released actions", () => releasedActions.Count == 0);
+            AddAssert("no pressed actions", () => pressedActions, () => Is.Empty);
+            AddAssert("no released actions", () => releasedActions, () => Is.Empty);
         }
 
         [Test]
@@ -196,18 +196,18 @@ public void TestSingleKeyRepeatEvents()
             });
 
             AddStep("press A", () => InputManager.PressKey(Key.A));
-            AddAssert("press received", () => pressedReceived == 1);
+            AddAssert("press received", () => pressedReceived, () => Is.EqualTo(1));
 
             for (int i = 0; i < 10; i++)
             {
                 int localI = i + 1;
-                AddUntilStep($"repeat #{1 + i} received", () => repeatedReceived >= localI);
+                AddUntilStep($"repeat #{1 + i} received", () => repeatedReceived, () => Is.GreaterThanOrEqualTo(localI));
             }
 
             AddStep("release A", () => InputManager.ReleaseKey(Key.A));
             AddAssert("release received", () => releasedReceived);
 
-            AddAssert("only one press received", () => pressedReceived == 1);
+            AddAssert("only one press received", () => pressedReceived, () => Is.EqualTo(1));
         }
 
         [Test]
@@ -236,18 +236,18 @@ public void TestKeyRepeatDoesntFireWhenNotAlive()
             });
 
             AddStep("press A", () => InputManager.PressKey(Key.A));
-            AddUntilStep("wait for non-zero repeated", () => repeatedReceived > 0);
+            AddUntilStep("wait for non-zero repeated", () => repeatedReceived, () => Is.GreaterThan(0));
 
             AddStep("hide receptor", () => receptor.Hide());
 
             int stopReceivingCheck = 0;
             AddStep("store count", () => stopReceivingCheck = repeatedReceived);
             AddWaitStep("wait some", 5);
-            AddAssert("ensure not incrementing", () => stopReceivingCheck == repeatedReceived);
+            AddAssert("ensure not incrementing", () => stopReceivingCheck, () => Is.EqualTo(repeatedReceived));
 
             AddStep("release A", () => InputManager.ReleaseKey(Key.A));
             AddAssert("release received", () => releasedReceived);
-            AddAssert("only one press received", () => pressedReceived == 1);
+            AddAssert("only one press received", () => pressedReceived, () => Is.EqualTo(1));
         }
 
         [Test]
@@ -349,6 +349,43 @@ public void TestPrioritisedPositionalInput([Values] bool prioritised)
                 AddAssert("container did not receive input", () => !containerReceivedInput);
         }
 
+        [Test]
+        public void TestReleaseKeyAfterReceptorRemovedFromHierarchy()
+        {
+            TestKeyBindingContainer container = null!;
+            TestKeyBindingReceptor receptor = null!;
+            List<TestAction> pressedActions = new List<TestAction>();
+            List<TestAction> releasedActions = new List<TestAction>();
+
+            AddStep("add container", () =>
+            {
+                pressedActions.Clear();
+                releasedActions.Clear();
+
+                Child = container = new TestKeyBindingContainer
+                {
+                    Child = receptor = new TestKeyBindingReceptor
+                    {
+                        Pressed = a => pressedActions.Add(a),
+                        Released = a => releasedActions.Add(a)
+                    }
+                };
+            });
+
+            AddStep("press key A", () => InputManager.PressKey(Key.A));
+            AddAssert("only one action triggered", () => pressedActions, () => Has.Count.EqualTo(1));
+            AddAssert("ActionA triggered", () => pressedActions[0], () => Is.EqualTo(TestAction.ActionA));
+            AddAssert("no actions released", () => releasedActions, () => Is.Empty);
+
+            AddStep("remove receptor", () => container.Remove(receptor, disposeImmediately: false));
+
+            AddStep("release key A", () => InputManager.ReleaseKey(Key.A));
+            AddAssert("only one action triggered", () => pressedActions, () => Has.Count.EqualTo(1));
+            AddAssert("no actions released", () => releasedActions, () => Is.Empty);
+
+            AddStep("dispose of receptor", () => receptor.Dispose());
+        }
+
         private partial class TestKeyBindingReceptor : Drawable, IKeyBindingHandler<TestAction>
         {
             public Action<TestAction>? Pressed;

diff --git a/osu.Framework/Input/Bindings/KeyBindingContainer.cs b/osu.Framework/Input/Bindings/KeyBindingContainer.cs
@@ -350,7 +350,7 @@ private void handleNewReleased(InputState state, InputKey releasedKey)
             {
                 pressedBindings.Remove(binding);
 
-                PropagateReleased(getInputQueue(binding), state, binding.GetAction<T>());
+                PropagateReleased(getInputQueue(binding).Where(d => d.IsRootedAt(this)), state, binding.GetAction<T>());
                 keyBindingQueues[binding].Clear();
             }
         }