diff --git a/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs b/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs index cf07ce2431..eecfb7cb6e 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs @@ -32,6 +32,7 @@ namespace osu.Game.Tests.Visual.Editing private TimingScreen timingScreen; private EditorBeatmap editorBeatmap; + private BeatmapEditorChangeHandler changeHandler; protected override bool ScrollUsingMouseWheel => false; @@ -46,6 +47,7 @@ namespace osu.Game.Tests.Visual.Editing private void reloadEditorBeatmap() { editorBeatmap = new EditorBeatmap(Beatmap.Value.GetPlayableBeatmap(Ruleset.Value)); + changeHandler = new BeatmapEditorChangeHandler(editorBeatmap); Child = new DependencyProvidingContainer { @@ -53,6 +55,7 @@ namespace osu.Game.Tests.Visual.Editing CachedDependencies = new (Type, object)[] { (typeof(EditorBeatmap), editorBeatmap), + (typeof(IEditorChangeHandler), changeHandler), (typeof(IBeatSnapProvider), editorBeatmap) }, Child = timingScreen = new TimingScreen @@ -72,8 +75,10 @@ namespace osu.Game.Tests.Visual.Editing AddUntilStep("Wait for rows to load", () => Child.ChildrenOfType().Any()); } + // TODO: this is best-effort for now, but the comment out test below should probably be how things should work. + // Was originally working as of https://github.com/ppy/osu/pull/26141; Regressed at some point. [Test] - public void TestSelectedRetainedOverUndo() + public void TestSelectionDismissedOnUndo() { AddStep("Select first timing point", () => { @@ -95,25 +100,52 @@ namespace osu.Game.Tests.Visual.Editing return timingScreen.SelectedGroup.Value.ControlPoints.Any(c => c is TimingControlPoint) && timingScreen.SelectedGroup.Value.Time > 2170; }); - AddStep("simulate undo", () => - { - var clone = editorBeatmap.ControlPointInfo.DeepClone(); + AddStep("undo", () => changeHandler?.RestoreState(-1)); - 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; - }); + AddUntilStep("selection dismissed", () => timingScreen.SelectedGroup.Value, () => Is.Null); } + // [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("undo", () => changeHandler?.RestoreState(-1)); + // + // AddUntilStep("selection retained", () => + // { + // return timingScreen.SelectedGroup.Value.ControlPoints.Any(c => c is TimingControlPoint) && timingScreen.SelectedGroup.Value.Time > 2170; + // }); + // + // AddAssert("check group count", () => editorBeatmap.ControlPointInfo.Groups.Count, () => Is.EqualTo(10)); + // + // AddStep("Adjust offset", () => + // { + // InputManager.MoveMouseTo(timingScreen.ChildrenOfType().First().ScreenSpaceDrawQuad.Centre + new Vector2(20, 0)); + // InputManager.Click(MouseButton.Left); + // }); + // + // AddAssert("check group count", () => editorBeatmap.ControlPointInfo.Groups.Count, () => Is.EqualTo(10)); + // } + [Test] public void TestScrollControlGroupIntoView() { diff --git a/osu.Game/Screens/Edit/Timing/ControlPointList.cs b/osu.Game/Screens/Edit/Timing/ControlPointList.cs index 49e5b76dd6..12c6390812 100644 --- a/osu.Game/Screens/Edit/Timing/ControlPointList.cs +++ b/osu.Game/Screens/Edit/Timing/ControlPointList.cs @@ -34,6 +34,9 @@ namespace osu.Game.Screens.Edit.Timing [Resolved] private Bindable selectedGroup { get; set; } = null!; + [Resolved] + private IEditorChangeHandler? editorChangeHandler { get; set; } + [BackgroundDependencyLoader] private void load(OsuColour colours, OverlayColourProvider colourProvider) { @@ -110,6 +113,9 @@ namespace osu.Game.Screens.Edit.Timing } }, }; + + if (editorChangeHandler != null) + editorChangeHandler.OnStateChange += onUndoRedo; } protected override void LoadComplete() @@ -185,5 +191,21 @@ namespace osu.Game.Screens.Edit.Timing selectedGroup.Value = group; } + + private void onUndoRedo() + { + // Best effort. We have no tracking of control points through undo/redo changes. + // If we don't deselect, things like offset changes could spawn groups to be added from previous states (see https://github.com/ppy/osu/issues/31098). + if (selectedGroup.Value != null && !Beatmap.ControlPointInfo.Groups.Contains(selectedGroup.Value)) + selectedGroup.Value = null; + } + + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + + if (editorChangeHandler != null) + editorChangeHandler.OnStateChange -= onUndoRedo; + } } }