diff --git a/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs b/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs index 216c35de65..40aadc8164 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs +++ b/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().Any()); } + [Test] + public void TestSelectedRetainedOverUndo() + { + AddStep("Select first timing point", () => + { + InputManager.MoveMouseTo(Child.ChildrenOfType().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().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() { diff --git a/osu.Game/Screens/Edit/EditorTable.cs b/osu.Game/Screens/Edit/EditorTable.cs index b79d71b42b..e5dc540b06 100644 --- a/osu.Game/Screens/Edit/EditorTable.cs +++ b/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); diff --git a/osu.Game/Screens/Edit/Timing/ControlPointList.cs b/osu.Game/Screens/Edit/Timing/ControlPointList.cs index 22e37b9efb..7cd1dbc630 100644 --- a/osu.Game/Screens/Edit/Timing/ControlPointList.cs +++ b/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); diff --git a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs index b078e3fa44..7a27056da3 100644 --- a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs +++ b/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 selectedGroup { get; set; } = null!; + private Bindable 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() {