Merge pull request #12389 from peppy/fix-editor-ctrl-drag-deselection

Fix ctrl-dragging on an existing selection unexpectedly causing deselection

osu.Game.Tests/Visual/Editing/TestSceneEditorQuickDelete.cs

@@ -1,88 +0,0 @@
-// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
-// See the LICENCE file in the repository root for full licence text.
-
-using System.Linq;
-using NUnit.Framework;
-using osu.Framework.Testing;
-using osu.Game.Beatmaps;
-using osu.Game.Rulesets;
-using osu.Game.Rulesets.Objects;
-using osu.Game.Rulesets.Osu;
-using osu.Game.Rulesets.Osu.Objects;
-using osu.Game.Rulesets.Osu.Edit.Blueprints.HitCircles.Components;
-using osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders.Components;
-using osu.Game.Tests.Beatmaps;
-using osu.Game.Screens.Edit.Compose.Components;
-using osuTK;
-using osuTK.Input;
-
-namespace osu.Game.Tests.Visual.Editing
-{
-    public class TestSceneEditorQuickDelete : EditorTestScene
-    {
-        protected override Ruleset CreateEditorRuleset() => new OsuRuleset();
-
-        protected override IBeatmap CreateBeatmap(RulesetInfo ruleset) => new TestBeatmap(ruleset, false);
-
-        private BlueprintContainer blueprintContainer
-            => Editor.ChildrenOfType<BlueprintContainer>().First();
-
-        [Test]
-        public void TestQuickDeleteRemovesObject()
-        {
-            var addedObject = new HitCircle { StartTime = 1000 };
-
-            AddStep("add hitobject", () => EditorBeatmap.Add(addedObject));
-
-            AddStep("select added object", () => EditorBeatmap.SelectedHitObjects.Add(addedObject));
-
-            AddStep("move mouse to object", () =>
-            {
-                var pos = blueprintContainer.ChildrenOfType<HitCirclePiece>().First().ScreenSpaceDrawQuad.Centre;
-                InputManager.MoveMouseTo(pos);
-            });
-            AddStep("hold shift", () => InputManager.PressKey(Key.ShiftLeft));
-            AddStep("right click", () => InputManager.Click(MouseButton.Right));
-            AddStep("release shift", () => InputManager.ReleaseKey(Key.ShiftLeft));
-
-            AddAssert("no hitobjects in beatmap", () => EditorBeatmap.HitObjects.Count == 0);
-        }
-
-        [Test]
-        public void TestQuickDeleteRemovesSliderControlPoint()
-        {
-            Slider slider = new Slider { StartTime = 1000 };
-
-            PathControlPoint[] points =
-            {
-                new PathControlPoint(),
-                new PathControlPoint(new Vector2(50, 0)),
-                new PathControlPoint(new Vector2(100, 0))
-            };
-
-            AddStep("add slider", () =>
-            {
-                slider.Path = new SliderPath(points);
-                EditorBeatmap.Add(slider);
-            });
-
-            AddStep("select added slider", () => EditorBeatmap.SelectedHitObjects.Add(slider));
-
-            AddStep("move mouse to controlpoint", () =>
-            {
-                var pos = blueprintContainer.ChildrenOfType<PathControlPointPiece>().ElementAt(1).ScreenSpaceDrawQuad.Centre;
-                InputManager.MoveMouseTo(pos);
-            });
-            AddStep("hold shift", () => InputManager.PressKey(Key.ShiftLeft));
-
-            AddStep("right click", () => InputManager.Click(MouseButton.Right));
-            AddAssert("slider has 2 points", () => slider.Path.ControlPoints.Count == 2);
-
-            // second click should nuke the object completely.
-            AddStep("right click", () => InputManager.Click(MouseButton.Right));
-            AddAssert("no hitobjects in beatmap", () => EditorBeatmap.HitObjects.Count == 0);
-
-            AddStep("release shift", () => InputManager.ReleaseKey(Key.ShiftLeft));
-        }
-    }
-}

osu.Game.Tests/Visual/Editing/TestSceneEditorSelection.cs

@@ -0,0 +1,219 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System;
+using System.Linq;
+using NUnit.Framework;
+using osu.Framework.Testing;
+using osu.Game.Beatmaps;
+using osu.Game.Rulesets;
+using osu.Game.Rulesets.Objects;
+using osu.Game.Rulesets.Osu;
+using osu.Game.Rulesets.Osu.Objects;
+using osu.Game.Rulesets.Osu.Edit.Blueprints.HitCircles.Components;
+using osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders.Components;
+using osu.Game.Tests.Beatmaps;
+using osu.Game.Screens.Edit.Compose.Components;
+using osuTK;
+using osuTK.Input;
+
+namespace osu.Game.Tests.Visual.Editing
+{
+    public class TestSceneEditorSelection : EditorTestScene
+    {
+        protected override Ruleset CreateEditorRuleset() => new OsuRuleset();
+
+        protected override IBeatmap CreateBeatmap(RulesetInfo ruleset) => new TestBeatmap(ruleset, false);
+
+        private BlueprintContainer blueprintContainer
+            => Editor.ChildrenOfType<BlueprintContainer>().First();
+
+        private void moveMouseToObject(Func<HitObject> targetFunc)
+        {
+            AddStep("move mouse to object", () =>
+            {
+                var pos = blueprintContainer.SelectionBlueprints
+                                            .First(s => s.HitObject == targetFunc())
+                                            .ChildrenOfType<HitCirclePiece>()
+                                            .First().ScreenSpaceDrawQuad.Centre;
+
+                InputManager.MoveMouseTo(pos);
+            });
+        }
+
+        [Test]
+        public void TestBasicSelect()
+        {
+            var addedObject = new HitCircle { StartTime = 100 };
+            AddStep("add hitobject", () => EditorBeatmap.Add(addedObject));
+
+            moveMouseToObject(() => addedObject);
+            AddStep("left click", () => InputManager.Click(MouseButton.Left));
+
+            AddAssert("hitobject selected", () => EditorBeatmap.SelectedHitObjects.Single() == addedObject);
+
+            var addedObject2 = new HitCircle
+            {
+                StartTime = 100,
+                Position = new Vector2(100),
+            };
+
+            AddStep("add one more hitobject", () => EditorBeatmap.Add(addedObject2));
+            AddAssert("selection unchanged", () => EditorBeatmap.SelectedHitObjects.Single() == addedObject);
+
+            moveMouseToObject(() => addedObject2);
+            AddStep("left click", () => InputManager.Click(MouseButton.Left));
+            AddAssert("hitobject selected", () => EditorBeatmap.SelectedHitObjects.Single() == addedObject2);
+        }
+
+        [Test]
+        public void TestMultiSelect()
+        {
+            var addedObjects = new[]
+            {
+                new HitCircle { StartTime = 100 },
+                new HitCircle { StartTime = 200, Position = new Vector2(50) },
+                new HitCircle { StartTime = 300, Position = new Vector2(100) },
+                new HitCircle { StartTime = 400, Position = new Vector2(150) },
+            };
+
+            AddStep("add hitobjects", () => EditorBeatmap.AddRange(addedObjects));
+
+            moveMouseToObject(() => addedObjects[0]);
+            AddStep("click first", () => InputManager.Click(MouseButton.Left));
+
+            AddAssert("hitobject selected", () => EditorBeatmap.SelectedHitObjects.Single() == addedObjects[0]);
+
+            AddStep("hold control", () => InputManager.PressKey(Key.ControlLeft));
+
+            moveMouseToObject(() => addedObjects[1]);
+            AddStep("click second", () => InputManager.Click(MouseButton.Left));
+            AddAssert("2 hitobjects selected", () => EditorBeatmap.SelectedHitObjects.Count == 2 && EditorBeatmap.SelectedHitObjects.Contains(addedObjects[1]));
+
+            moveMouseToObject(() => addedObjects[2]);
+            AddStep("click third", () => InputManager.Click(MouseButton.Left));
+            AddAssert("3 hitobjects selected", () => EditorBeatmap.SelectedHitObjects.Count == 3 && EditorBeatmap.SelectedHitObjects.Contains(addedObjects[2]));
+
+            moveMouseToObject(() => addedObjects[1]);
+            AddStep("click second", () => InputManager.Click(MouseButton.Left));
+            AddAssert("2 hitobjects selected", () => EditorBeatmap.SelectedHitObjects.Count == 2 && !EditorBeatmap.SelectedHitObjects.Contains(addedObjects[1]));
+        }
+
+        [TestCase(false)]
+        [TestCase(true)]
+        public void TestMultiSelectFromDrag(bool alreadySelectedBeforeDrag)
+        {
+            HitCircle[] addedObjects = null;
+
+            AddStep("add hitobjects", () => EditorBeatmap.AddRange(addedObjects = new[]
+            {
+                new HitCircle { StartTime = 100 },
+                new HitCircle { StartTime = 200, Position = new Vector2(50) },
+                new HitCircle { StartTime = 300, Position = new Vector2(100) },
+                new HitCircle { StartTime = 400, Position = new Vector2(150) },
+            }));
+
+            moveMouseToObject(() => addedObjects[0]);
+            AddStep("click first", () => InputManager.Click(MouseButton.Left));
+
+            AddStep("hold control", () => InputManager.PressKey(Key.ControlLeft));
+
+            moveMouseToObject(() => addedObjects[1]);
+
+            if (alreadySelectedBeforeDrag)
+                AddStep("click second", () => InputManager.Click(MouseButton.Left));
+
+            AddStep("mouse down on second", () => InputManager.PressButton(MouseButton.Left));
+
+            AddAssert("2 hitobjects selected", () => EditorBeatmap.SelectedHitObjects.Count == 2 && EditorBeatmap.SelectedHitObjects.Contains(addedObjects[1]));
+
+            AddStep("drag to centre", () => InputManager.MoveMouseTo(blueprintContainer.ScreenSpaceDrawQuad.Centre));
+
+            AddAssert("positions changed", () => addedObjects[0].Position != Vector2.Zero && addedObjects[1].Position != new Vector2(50));
+
+            AddStep("release control", () => InputManager.ReleaseKey(Key.ControlLeft));
+            AddStep("mouse up", () => InputManager.ReleaseButton(MouseButton.Left));
+        }
+
+        [Test]
+        public void TestBasicDeselect()
+        {
+            var addedObject = new HitCircle { StartTime = 100 };
+            AddStep("add hitobject", () => EditorBeatmap.Add(addedObject));
+
+            moveMouseToObject(() => addedObject);
+            AddStep("left click", () => InputManager.Click(MouseButton.Left));
+
+            AddAssert("hitobject selected", () => EditorBeatmap.SelectedHitObjects.Single() == addedObject);
+
+            AddStep("click away", () =>
+            {
+                InputManager.MoveMouseTo(blueprintContainer.ScreenSpaceDrawQuad.Centre);
+                InputManager.Click(MouseButton.Left);
+            });
+
+            AddAssert("selection lost", () => EditorBeatmap.SelectedHitObjects.Count == 0);
+        }
+
+        [Test]
+        public void TestQuickDeleteRemovesObject()
+        {
+            var addedObject = new HitCircle { StartTime = 1000 };
+
+            AddStep("add hitobject", () => EditorBeatmap.Add(addedObject));
+
+            AddStep("select added object", () => EditorBeatmap.SelectedHitObjects.Add(addedObject));
+
+            moveMouseToObject(() => addedObject);
+
+            AddStep("hold shift", () => InputManager.PressKey(Key.ShiftLeft));
+            AddStep("right click", () => InputManager.Click(MouseButton.Right));
+            AddStep("release shift", () => InputManager.ReleaseKey(Key.ShiftLeft));
+
+            AddAssert("no hitobjects in beatmap", () => EditorBeatmap.HitObjects.Count == 0);
+        }
+
+        [Test]
+        public void TestQuickDeleteRemovesSliderControlPoint()
+        {
+            Slider slider = null;
+
+            PathControlPoint[] points =
+            {
+                new PathControlPoint(),
+                new PathControlPoint(new Vector2(50, 0)),
+                new PathControlPoint(new Vector2(100, 0))
+            };
+
+            AddStep("add slider", () =>
+            {
+                slider = new Slider
+                {
+                    StartTime = 1000,
+                    Path = new SliderPath(points)
+                };
+
+                EditorBeatmap.Add(slider);
+            });
+
+            AddStep("select added slider", () => EditorBeatmap.SelectedHitObjects.Add(slider));
+
+            AddStep("move mouse to controlpoint", () =>
+            {
+                var pos = blueprintContainer.ChildrenOfType<PathControlPointPiece>().ElementAt(1).ScreenSpaceDrawQuad.Centre;
+                InputManager.MoveMouseTo(pos);
+            });
+            AddStep("hold shift", () => InputManager.PressKey(Key.ShiftLeft));
+
+            AddStep("right click", () => InputManager.Click(MouseButton.Right));
+            AddAssert("slider has 2 points", () => slider.Path.ControlPoints.Count == 2);
+
+            AddStep("right click", () => InputManager.Click(MouseButton.Right));
+
+            // second click should nuke the object completely.
+            AddAssert("no hitobjects in beatmap", () => EditorBeatmap.HitObjects.Count == 0);
+
+            AddStep("release shift", () => InputManager.ReleaseKey(Key.ShiftLeft));
+        }
+    }
+}

osu.Game/Screens/Edit/Compose/Components/BlueprintContainer.cs

@@ -135,11 +135,12 @@ namespace osu.Game.Screens.Edit.Compose.Components
 
         protected override bool OnMouseDown(MouseDownEvent e)
         {
-            if (!beginClickSelection(e)) return true;
+            bool selectionPerformed = performMouseDownActions(e);
 
+            // even if a selection didn't occur, a drag event may still move the selection.
             prepareSelectionMovement();
 
-            return e.Button == MouseButton.Left;
+            return selectionPerformed || e.Button == MouseButton.Left;
         }
 
         private SelectionBlueprint clickedBlueprint;
@@ -154,7 +155,7 @@ namespace osu.Game.Screens.Edit.Compose.Components
 
             // Deselection should only occur if no selected blueprints are hovered
             // A special case for when a blueprint was selected via this click is added since OnClick() may occur outside the hitobject and should not trigger deselection
-            if (endClickSelection() || clickedBlueprint != null)
+            if (endClickSelection(e) || clickedBlueprint != null)
                 return true;
 
             deselectAll();
@@ -177,7 +178,12 @@ namespace osu.Game.Screens.Edit.Compose.Components
         protected override void OnMouseUp(MouseUpEvent e)
         {
             // Special case for when a drag happened instead of a click
-            Schedule(() => endClickSelection());
+            Schedule(() =>
+            {
+                endClickSelection(e);
+                clickSelectionBegan = false;
+                isDraggingBlueprint = false;
+            });
 
             finishSelectionMovement();
         }
@@ -226,7 +232,6 @@ namespace osu.Game.Screens.Edit.Compose.Components
                     Beatmap.Update(obj);
 
                 changeHandler?.EndChange();
-                isDraggingBlueprint = false;
             }
 
             if (DragBox.State == Visibility.Visible)
@@ -338,7 +343,7 @@ namespace osu.Game.Screens.Edit.Compose.Components
         /// </summary>
         /// <param name="e">The input event that triggered this selection.</param>
         /// <returns>Whether a selection was performed.</returns>
-        private bool beginClickSelection(MouseButtonEvent e)
+        private bool performMouseDownActions(MouseButtonEvent e)
         {
             // Iterate from the top of the input stack (blueprints closest to the front of the screen first).
             // Priority is given to already-selected blueprints.
@@ -346,7 +351,7 @@ namespace osu.Game.Screens.Edit.Compose.Components
             {
                 if (!blueprint.IsHovered) continue;
 
-                return clickSelectionBegan = SelectionHandler.HandleSelectionRequested(blueprint, e);
+                return clickSelectionBegan = SelectionHandler.MouseDownSelectionRequested(blueprint, e);
             }
 
             return false;
@@ -355,13 +360,28 @@ namespace osu.Game.Screens.Edit.Compose.Components
         /// <summary>
         /// Finishes the current blueprint selection.
         /// </summary>
+        /// <param name="e">The mouse event which triggered end of selection.</param>
         /// <returns>Whether a click selection was active.</returns>
-        private bool endClickSelection()
+        private bool endClickSelection(MouseButtonEvent e)
         {
-            if (!clickSelectionBegan)
+            if (!clickSelectionBegan && !isDraggingBlueprint)
-                return false;
+            {
+                // if a selection didn't occur, we may want to trigger a deselection.
+                if (e.ControlPressed && e.Button == MouseButton.Left)
+                {
+                    // Iterate from the top of the input stack (blueprints closest to the front of the screen first).
+                    // Priority is given to already-selected blueprints.
+                    foreach (SelectionBlueprint blueprint in SelectionBlueprints.AliveChildren.Reverse().OrderByDescending(b => b.IsSelected))
+                    {
+                        if (!blueprint.IsHovered) continue;
+
+                        return clickSelectionBegan = SelectionHandler.MouseUpSelectionRequested(blueprint, e);
+                    }
+                }
+
+                return false;
+            }
 
-            clickSelectionBegan = false;
             return true;
         }
 

osu.Game/Screens/Edit/Compose/Components/SelectionHandler.cs

@@ -220,20 +220,39 @@ namespace osu.Game.Screens.Edit.Compose.Components
         /// <param name="blueprint">The blueprint.</param>
         /// <param name="e">The mouse event responsible for selection.</param>
         /// <returns>Whether a selection was performed.</returns>
-        internal bool HandleSelectionRequested(SelectionBlueprint blueprint, MouseButtonEvent e)
+        internal bool MouseDownSelectionRequested(SelectionBlueprint blueprint, MouseButtonEvent e)
         {
             if (e.ShiftPressed && e.Button == MouseButton.Right)
             {
                 handleQuickDeletion(blueprint);
-                return false;
+                return true;
             }
 
-            if (e.ControlPressed && e.Button == MouseButton.Left)
+            // while holding control, we only want to add to selection, not replace an existing selection.
+            if (e.ControlPressed && e.Button == MouseButton.Left && !blueprint.IsSelected)
+            {
                 blueprint.ToggleSelection();
-            else
+                return true;
-                ensureSelected(blueprint);
+            }
 
-            return true;
+            return ensureSelected(blueprint);
+        }
+
+        /// <summary>
+        /// Handle a blueprint requesting selection.
+        /// </summary>
+        /// <param name="blueprint">The blueprint.</param>
+        /// <param name="e">The mouse event responsible for deselection.</param>
+        /// <returns>Whether a deselection was performed.</returns>
+        internal bool MouseUpSelectionRequested(SelectionBlueprint blueprint, MouseButtonEvent e)
+        {
+            if (blueprint.IsSelected)
+            {
+                blueprint.ToggleSelection();
+                return true;
+            }
+
+            return false;
         }
 
         private void handleQuickDeletion(SelectionBlueprint blueprint)
@@ -247,13 +266,19 @@ namespace osu.Game.Screens.Edit.Compose.Components
                 deleteSelected();
         }
 
-        private void ensureSelected(SelectionBlueprint blueprint)
+        /// <summary>
+        /// Ensure the blueprint is in a selected state.
+        /// </summary>
+        /// <param name="blueprint">The blueprint to select.</param>
+        /// <returns>Whether selection state was changed.</returns>
+        private bool ensureSelected(SelectionBlueprint blueprint)
         {
             if (blueprint.IsSelected)
-                return;
+                return false;
 
             DeselectAll?.Invoke();
             blueprint.Select();
+            return true;
         }
 
         private void deleteSelected()