Merge pull request #26141 from peppy/control-point-fixes

Fix editor timing mode not handling undo/redo correctly

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

@@ -17,6 +17,7 @@ using osu.Game.Rulesets.Edit;
 using osu.Game.Screens.Edit;
 using osu.Game.Screens.Edit.Timing;
 using osu.Game.Screens.Edit.Timing.RowAttributes;
+using osuTK;
 using osuTK.Input;
 
 namespace osu.Game.Tests.Visual.Editing
@@ -69,6 +70,48 @@ namespace osu.Game.Tests.Visual.Editing
             AddUntilStep("Wait for rows to load", () => Child.ChildrenOfType<EffectRowAttribute>().Any());
         }
 
+        [Test]
+        public void TestSelectedRetainedOverUndo()
+        {
+            AddStep("Select first timing point", () =>
+            {
+                InputManager.MoveMouseTo(Child.ChildrenOfType<TimingRowAttribute>().First());
+                InputManager.Click(MouseButton.Left);
+            });
+
+            AddUntilStep("Selection changed", () => timingScreen.SelectedGroup.Value.Time == 2170);
+            AddUntilStep("Ensure seeked to correct time", () => EditorClock.CurrentTimeAccurate == 2170);
+
+            AddStep("Adjust offset", () =>
+            {
+                InputManager.MoveMouseTo(timingScreen.ChildrenOfType<TimingAdjustButton>().First().ScreenSpaceDrawQuad.Centre + new Vector2(20, 0));
+                InputManager.Click(MouseButton.Left);
+            });
+
+            AddUntilStep("wait for offset changed", () =>
+            {
+                return timingScreen.SelectedGroup.Value.ControlPoints.Any(c => c is TimingControlPoint) && timingScreen.SelectedGroup.Value.Time > 2170;
+            });
+
+            AddStep("simulate undo", () =>
+            {
+                var clone = editorBeatmap.ControlPointInfo.DeepClone();
+
+                editorBeatmap.ControlPointInfo.Clear();
+
+                foreach (var group in clone.Groups)
+                {
+                    foreach (var cp in group.ControlPoints)
+                        editorBeatmap.ControlPointInfo.Add(group.Time, cp);
+                }
+            });
+
+            AddUntilStep("selection retained", () =>
+            {
+                return timingScreen.SelectedGroup.Value.ControlPoints.Any(c => c is TimingControlPoint) && timingScreen.SelectedGroup.Value.Time > 2170;
+            });
+        }
+
         [Test]
         public void TestTrackingCurrentTimeWhileRunning()
         {

osu.Game/Screens/Edit/EditorTable.cs

@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
+using System.Diagnostics;
 using osu.Framework.Allocation;
 using osu.Framework.Extensions.LocalisationExtensions;
 using osu.Framework.Graphics;
@@ -46,15 +47,42 @@ namespace osu.Game.Screens.Edit
             });
         }
 
-        protected void SetSelectedRow(object? item)
+        protected int GetIndexForObject(object? item)
         {
+            for (int i = 0; i < BackgroundFlow.Count; i++)
+            {
+                if (BackgroundFlow[i].Item == item)
+                    return i;
+            }
+
+            return -1;
+        }
+
+        protected virtual bool SetSelectedRow(object? item)
+        {
+            bool foundSelection = false;
+
             foreach (var b in BackgroundFlow)
             {
                 b.Selected = ReferenceEquals(b.Item, item);
 
                 if (b.Selected)
+                {
+                    Debug.Assert(!foundSelection);
                     OnRowSelected?.Invoke(b);
+                    foundSelection = true;
+                }
             }
+
+            return foundSelection;
+        }
+
+        protected object? GetObjectAtIndex(int index)
+        {
+            if (index < 0 || index > BackgroundFlow.Count - 1)
+                return null;
+
+            return BackgroundFlow[index].Item;
         }
 
         protected override Drawable CreateHeader(int index, TableColumn? column) => new HeaderText(column?.Header ?? default);

osu.Game/Screens/Edit/Timing/ControlPointList.cs

@@ -109,8 +109,13 @@ namespace osu.Game.Screens.Edit.Timing
             controlPointGroups.BindTo(Beatmap.ControlPointInfo.Groups);
             controlPointGroups.BindCollectionChanged((_, _) =>
             {
-                table.ControlGroups = controlPointGroups;
-                changeHandler?.SaveState();
+                // This callback can happen many times in a change operation. It gets expensive.
+                // We really should be handling the `CollectionChanged` event properly.
+                Scheduler.AddOnce(() =>
+                {
+                    table.ControlGroups = controlPointGroups;
+                    changeHandler?.SaveState();
+                });
             }, true);
 
             table.OnRowSelected += drawable => scroll.ScrollIntoView(drawable);

osu.Game/Screens/Edit/Timing/ControlPointTable.cs

@@ -21,7 +21,7 @@ namespace osu.Game.Screens.Edit.Timing
     public partial class ControlPointTable : EditorTable
     {
         [Resolved]
-        private Bindable<ControlPointGroup> selectedGroup { get; set; } = null!;
+        private Bindable<ControlPointGroup?> selectedGroup { get; set; } = null!;
 
         [Resolved]
         private EditorClock clock { get; set; } = null!;
@@ -32,6 +32,8 @@ namespace osu.Game.Screens.Edit.Timing
         {
             set
             {
+                int selectedIndex = GetIndexForObject(selectedGroup.Value);
+
                 Content = null;
                 BackgroundFlow.Clear();
 
@@ -44,7 +46,7 @@ namespace osu.Game.Screens.Edit.Timing
                     {
                         Action = () =>
                         {
-                            selectedGroup.Value = group;
+                            SetSelectedRow(group);
                             clock.SeekSmoothlyTo(group.Time);
                         }
                     });
@@ -53,7 +55,16 @@ namespace osu.Game.Screens.Edit.Timing
                 Columns = createHeaders();
                 Content = value.Select(createContent).ToArray().ToRectangular();
 
-                updateSelectedGroup();
+                // Attempt to retain selection.
+                if (SetSelectedRow(selectedGroup.Value))
+                    return;
+
+                // Some operations completely obliterate references, so best-effort reselect based on index.
+                if (SetSelectedRow(GetObjectAtIndex(selectedIndex)))
+                    return;
+
+                // Selection could not be retained.
+                selectedGroup.Value = null;
             }
         }
 
@@ -61,10 +72,18 @@ namespace osu.Game.Screens.Edit.Timing
         {
             base.LoadComplete();
 
-            selectedGroup.BindValueChanged(_ => updateSelectedGroup(), true);
+            // Handle external selections.
+            selectedGroup.BindValueChanged(g => SetSelectedRow(g.NewValue), true);
         }
 
-        private void updateSelectedGroup() => SetSelectedRow(selectedGroup.Value);
+        protected override bool SetSelectedRow(object? item)
+        {
+            if (!base.SetSelectedRow(item))
+                return false;
+
+            selectedGroup.Value = item as ControlPointGroup;
+            return true;
+        }
 
         private TableColumn[] createHeaders()
         {