From 2d74609f5030b07844fab427bb4f11e7915ebdbd Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 26 Jan 2020 19:06:50 +0900 Subject: [PATCH 1/5] Fix crash due to misordered selection events --- osu.Game/Screens/Select/BeatmapCarousel.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 4433543ca1..420aa14a99 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -559,6 +559,14 @@ namespace osu.Game.Screens.Select { if (state.NewValue == CarouselItemState.Selected) { + if (!AllowSelection) + { + // CarouselBeatmap may trigger a state change from OnClick, unaware that it is not allowed to. + // we revert this change here to ensure sanity. + c.State.Value = state.OldValue; + return; + } + selectedBeatmapSet = set; SelectionChanged?.Invoke(c.Beatmap); From 1f0aaabf7b72966c0eb741cd11822bce123a96fd Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 27 Jan 2020 12:21:17 +0900 Subject: [PATCH 2/5] Add tests --- .../SongSelect/TestScenePlaySongSelect.cs | 111 ++++++++++++++++++ .../Visual/ManualInputManagerTestScene.cs | 7 +- 2 files changed, 117 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs index eb812f5d5a..81a44f9f8a 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs @@ -14,6 +14,7 @@ using osu.Framework.Extensions; using osu.Framework.Utils; using osu.Framework.Platform; using osu.Framework.Screens; +using osu.Framework.Testing; using osu.Game.Beatmaps; using osu.Game.Configuration; using osu.Game.Overlays; @@ -25,6 +26,7 @@ using osu.Game.Rulesets.Taiko; using osu.Game.Screens.Select; using osu.Game.Screens.Select.Carousel; using osu.Game.Screens.Select.Filter; +using osuTK.Input; namespace osu.Game.Tests.Visual.SongSelect { @@ -95,6 +97,115 @@ namespace osu.Game.Tests.Visual.SongSelect AddAssert("filter count is 1", () => songSelect.FilterCount == 1); } + [Test] + public void TestChangeBeatmapBeforeEnter() + { + addRulesetImportStep(0); + + createSongSelect(); + + WorkingBeatmap selected = null; + + AddStep("store selected beatmap", () => selected = Beatmap.Value); + + AddStep("select next and enter", () => + { + InputManager.PressKey(Key.Down); + InputManager.ReleaseKey(Key.Down); + InputManager.PressKey(Key.Enter); + InputManager.ReleaseKey(Key.Enter); + }); + + AddAssert("ensure selection changed", () => selected != Beatmap.Value); + + AddUntilStep("wait for return to song select", () => songSelect.IsCurrentScreen()); + AddUntilStep("bindable lease returned", () => !Beatmap.Disabled); + } + + [Test] + public void TestChangeBeatmapAfterEnter() + { + addRulesetImportStep(0); + + createSongSelect(); + + WorkingBeatmap selected = null; + + AddStep("store selected beatmap", () => selected = Beatmap.Value); + + AddStep("select next and enter", () => + { + InputManager.PressKey(Key.Enter); + InputManager.ReleaseKey(Key.Enter); + InputManager.PressKey(Key.Down); + InputManager.ReleaseKey(Key.Down); + }); + + AddAssert("ensure selection didn't change", () => selected == Beatmap.Value); + + AddUntilStep("wait for return to song select", () => songSelect.IsCurrentScreen()); + AddUntilStep("bindable lease returned", () => !Beatmap.Disabled); + } + + [Test] + public void TestChangeBeatmapViaMouseBeforeEnter() + { + addRulesetImportStep(0); + + createSongSelect(); + + WorkingBeatmap selected = null; + + AddStep("store selected beatmap", () => selected = Beatmap.Value); + + AddStep("select next and enter", () => + { + InputManager.MoveMouseTo(songSelect.Carousel.ChildrenOfType() + .First(b => ((CarouselBeatmap)b.Item).Beatmap != songSelect.Carousel.SelectedBeatmap)); + + InputManager.PressButton(MouseButton.Left); + InputManager.ReleaseButton(MouseButton.Left); + + InputManager.PressKey(Key.Enter); + InputManager.ReleaseKey(Key.Enter); + }); + + AddAssert("ensure selection changed", () => selected != Beatmap.Value); + + AddUntilStep("wait for return to song select", () => songSelect.IsCurrentScreen()); + AddUntilStep("bindable lease returned", () => !Beatmap.Disabled); + } + + [Test] + public void TestChangeBeatmapViaMouseAfterEnter() + { + addRulesetImportStep(0); + + createSongSelect(); + + WorkingBeatmap selected = null; + + AddStep("store selected beatmap", () => selected = Beatmap.Value); + + AddStep("select next and enter", () => + { + InputManager.MoveMouseTo(songSelect.Carousel.ChildrenOfType() + .First(b => ((CarouselBeatmap)b.Item).Beatmap != songSelect.Carousel.SelectedBeatmap)); + + InputManager.PressButton(MouseButton.Left); + + InputManager.PressKey(Key.Enter); + InputManager.ReleaseKey(Key.Enter); + + InputManager.ReleaseButton(MouseButton.Left); + }); + + AddAssert("ensure selection didn't change", () => selected == Beatmap.Value); + + AddUntilStep("wait for return to song select", () => songSelect.IsCurrentScreen()); + AddUntilStep("bindable lease returned", () => !Beatmap.Disabled); + } + [Test] public void TestNoFilterOnSimpleResume() { diff --git a/osu.Game/Tests/Visual/ManualInputManagerTestScene.cs b/osu.Game/Tests/Visual/ManualInputManagerTestScene.cs index 86191609a4..a0af07013c 100644 --- a/osu.Game/Tests/Visual/ManualInputManagerTestScene.cs +++ b/osu.Game/Tests/Visual/ManualInputManagerTestScene.cs @@ -8,6 +8,7 @@ using osu.Framework.Testing.Input; using osu.Game.Graphics.Cursor; using osu.Game.Graphics.Sprites; using osu.Game.Graphics.UserInterface; +using osu.Game.Input.Bindings; using osuTK; using osuTK.Graphics; @@ -30,7 +31,11 @@ namespace osu.Game.Tests.Visual InputManager = new ManualInputManager { UseParentInput = true, - Child = content = new MenuCursorContainer { RelativeSizeAxes = Axes.Both }, + Child = new GlobalActionContainer(null) + { + RelativeSizeAxes = Axes.Both, + Child = content = new MenuCursorContainer { RelativeSizeAxes = Axes.Both } + }, }, new Container { From 1c64b70b0674f270cef8aa25d3a28615f8373518 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 26 Jan 2020 19:06:50 +0900 Subject: [PATCH 3/5] Revert "Fix crash due to misordered selection events" This reverts commit 2d74609f5030b07844fab427bb4f11e7915ebdbd. --- osu.Game/Screens/Select/BeatmapCarousel.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 420aa14a99..4433543ca1 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -559,14 +559,6 @@ namespace osu.Game.Screens.Select { if (state.NewValue == CarouselItemState.Selected) { - if (!AllowSelection) - { - // CarouselBeatmap may trigger a state change from OnClick, unaware that it is not allowed to. - // we revert this change here to ensure sanity. - c.State.Value = state.OldValue; - return; - } - selectedBeatmapSet = set; SelectionChanged?.Invoke(c.Beatmap); From 7aa5e8c23eec292b2f3b7d669203ca639d2478e2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 27 Jan 2020 14:55:47 +0900 Subject: [PATCH 4/5] Limit input propagation correctly --- osu.Game/Screens/Select/BeatmapCarousel.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 4433543ca1..2bb5ba612e 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -56,6 +56,9 @@ namespace osu.Game.Screens.Select public override bool HandleNonPositionalInput => AllowSelection; public override bool HandlePositionalInput => AllowSelection; + public override bool PropagatePositionalInputSubTree => AllowSelection; + public override bool PropagateNonPositionalInputSubTree => AllowSelection; + /// /// Whether carousel items have completed asynchronously loaded. /// @@ -449,8 +452,6 @@ namespace osu.Game.Screens.Select return true; } - protected override bool ReceivePositionalInputAtSubTree(Vector2 screenSpacePos) => ReceivePositionalInputAt(screenSpacePos); - protected override void Update() { base.Update(); From fd7f07433da4b6f95d75bbce6c328c29e5d72ca4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 27 Jan 2020 18:26:44 +0900 Subject: [PATCH 5/5] Ensure selection has occurred before storing value --- .../Visual/SongSelect/TestScenePlaySongSelect.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs index 81a44f9f8a..98b8e3c5d6 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs @@ -104,6 +104,8 @@ namespace osu.Game.Tests.Visual.SongSelect createSongSelect(); + AddUntilStep("wait for initial selection", () => !Beatmap.IsDefault); + WorkingBeatmap selected = null; AddStep("store selected beatmap", () => selected = Beatmap.Value); @@ -129,6 +131,8 @@ namespace osu.Game.Tests.Visual.SongSelect createSongSelect(); + AddUntilStep("wait for initial selection", () => !Beatmap.IsDefault); + WorkingBeatmap selected = null; AddStep("store selected beatmap", () => selected = Beatmap.Value); @@ -154,6 +158,8 @@ namespace osu.Game.Tests.Visual.SongSelect createSongSelect(); + AddUntilStep("wait for initial selection", () => !Beatmap.IsDefault); + WorkingBeatmap selected = null; AddStep("store selected beatmap", () => selected = Beatmap.Value); @@ -183,6 +189,8 @@ namespace osu.Game.Tests.Visual.SongSelect createSongSelect(); + AddUntilStep("wait for initial selection", () => !Beatmap.IsDefault); + WorkingBeatmap selected = null; AddStep("store selected beatmap", () => selected = Beatmap.Value);