From 2f12c7d9e1e9c1231c82c2bd4766f198dfa709a8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 4 May 2022 19:09:22 +0900 Subject: [PATCH 01/10] Change `ChatTextBox` to not handle up/down arrows --- osu.Game/Online/Chat/StandAloneChatDisplay.cs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/osu.Game/Online/Chat/StandAloneChatDisplay.cs b/osu.Game/Online/Chat/StandAloneChatDisplay.cs index 6a7da52416..df210fcaf8 100644 --- a/osu.Game/Online/Chat/StandAloneChatDisplay.cs +++ b/osu.Game/Online/Chat/StandAloneChatDisplay.cs @@ -14,6 +14,7 @@ using osu.Game.Graphics.UserInterface; using osu.Game.Overlays.Chat; using osu.Game.Resources.Localisation.Web; using osuTK.Graphics; +using osuTK.Input; namespace osu.Game.Online.Chat { @@ -119,6 +120,20 @@ namespace osu.Game.Online.Chat public class ChatTextBox : FocusedTextBox { + protected override bool OnKeyDown(KeyDownEvent e) + { + // Chat text boxes are generally used in places where they retain focus, but shouldn't block interaction with other + // elements on the same screen. + switch (e.Key) + { + case Key.Up: + case Key.Down: + return false; + } + + return base.OnKeyDown(e); + } + protected override void LoadComplete() { base.LoadComplete(); From df1f4aecdceee273e3fbafbe7e7fa2c208e50419 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 4 May 2022 19:09:40 +0900 Subject: [PATCH 02/10] Add support for traversing playlist items using next/previous bindings Addresses https://github.com/ppy/osu/discussions/18061. --- .../OnlinePlay/DrawableRoomPlaylist.cs | 53 ++++++++++++++++++- .../Lounge/Components/RoomsContainer.cs | 2 +- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs index 57bb4253cb..d1b21ca1b0 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs @@ -6,7 +6,10 @@ using System.Linq; using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; +using osu.Framework.Input.Bindings; +using osu.Framework.Input.Events; using osu.Game.Graphics.Containers; +using osu.Game.Input.Bindings; using osu.Game.Online.Rooms; using osuTK; @@ -15,7 +18,7 @@ namespace osu.Game.Screens.OnlinePlay /// /// A scrollable list which displays the s in a . /// - public class DrawableRoomPlaylist : OsuRearrangeableListContainer + public class DrawableRoomPlaylist : OsuRearrangeableListContainer, IKeyBindingHandler { /// /// The currently-selected item. Selection is visually represented with a border. @@ -169,5 +172,53 @@ namespace osu.Game.Screens.OnlinePlay }); protected virtual DrawableRoomPlaylistItem CreateDrawablePlaylistItem(PlaylistItem item) => new DrawableRoomPlaylistItem(item); + + #region Key selection logic (shared with BeatmapCarousel and RoomsContainer) + + public bool OnPressed(KeyBindingPressEvent e) + { + switch (e.Action) + { + case GlobalAction.SelectNext: + selectNext(1); + return true; + + case GlobalAction.SelectPrevious: + selectNext(-1); + return true; + } + + return false; + } + + public void OnReleased(KeyBindingReleaseEvent e) + { + } + + private void selectNext(int direction) + { + if (SelectedItem.Disabled) + return; + + var visibleRooms = ListContainer.AsEnumerable().Where(r => r.IsPresent); + + PlaylistItem item; + + if (SelectedItem.Value == null) + item = visibleRooms.FirstOrDefault()?.Model; + else + { + if (direction < 0) + visibleRooms = visibleRooms.Reverse(); + + item = visibleRooms.SkipWhile(r => r.Model != SelectedItem.Value).Skip(1).FirstOrDefault()?.Model; + } + + // we already have a valid selection only change selection if we still have a room to switch to. + if (item != null) + SelectedItem.Value = item; + } + + #endregion } } diff --git a/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs b/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs index 175cd2c44e..9bcda90e6d 100644 --- a/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs +++ b/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs @@ -139,7 +139,7 @@ namespace osu.Game.Screens.OnlinePlay.Lounge.Components return base.OnClick(e); } - #region Key selection logic (shared with BeatmapCarousel) + #region Key selection logic (shared with BeatmapCarousel and RoomsContainer) public bool OnPressed(KeyBindingPressEvent e) { From 3b04daddaa9ec3de743eb590003df4f4cc17592d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 4 May 2022 12:36:43 +0200 Subject: [PATCH 03/10] Fix self-reference in region name --- osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs b/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs index 9bcda90e6d..74e4225f11 100644 --- a/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs +++ b/osu.Game/Screens/OnlinePlay/Lounge/Components/RoomsContainer.cs @@ -139,7 +139,7 @@ namespace osu.Game.Screens.OnlinePlay.Lounge.Components return base.OnClick(e); } - #region Key selection logic (shared with BeatmapCarousel and RoomsContainer) + #region Key selection logic (shared with BeatmapCarousel and DrawableRoomPlaylist) public bool OnPressed(KeyBindingPressEvent e) { From ec27fa8e857a357e7e07f205dcdc6c3891666416 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 4 May 2022 13:12:21 +0200 Subject: [PATCH 04/10] Add test coverage for keyboard selection --- .../TestSceneDrawableRoomPlaylist.cs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs index 76353323d6..566a8db608 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs @@ -147,6 +147,40 @@ namespace osu.Game.Tests.Visual.Multiplayer AddAssert("item 1 is selected", () => playlist.SelectedItem.Value == playlist.Items[1]); } + [Test] + public void KeyboardSelection() + { + createPlaylist(p => p.AllowSelection = true); + + AddStep("press down", () => InputManager.Key(Key.Down)); + AddAssert("item 0 is selected", () => playlist.SelectedItem.Value == playlist.Items[0]); + + AddStep("press down", () => InputManager.Key(Key.Down)); + AddAssert("item 1 is selected", () => playlist.SelectedItem.Value == playlist.Items[1]); + + AddStep("press up", () => InputManager.Key(Key.Up)); + AddAssert("item 0 is selected", () => playlist.SelectedItem.Value == playlist.Items[0]); + + AddUntilStep("navigate to last item via keyboard", () => + { + InputManager.Key(Key.Down); + return playlist.SelectedItem.Value == playlist.Items.Last(); + }); + AddAssert("last item is selected", () => playlist.SelectedItem.Value == playlist.Items.Last()); + AddUntilStep("last item is scrolled into view", () => + { + var drawableItem = playlist.ItemMap[playlist.Items.Last()]; + return playlist.ScreenSpaceDrawQuad.Contains(drawableItem.ScreenSpaceDrawQuad.TopLeft) + && playlist.ScreenSpaceDrawQuad.Contains(drawableItem.ScreenSpaceDrawQuad.BottomRight); + }); + + AddStep("press down", () => InputManager.Key(Key.Down)); + AddAssert("last item is selected", () => playlist.SelectedItem.Value == playlist.Items.Last()); + + AddStep("press up", () => InputManager.Key(Key.Up)); + AddAssert("second last item is selected", () => playlist.SelectedItem.Value == playlist.Items.Reverse().ElementAt(1)); + } + [Test] public void TestDownloadButtonHiddenWhenBeatmapExists() { From 0405c1c34a18ca1c06be95fa5426f7deeeaf0daa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 4 May 2022 13:15:26 +0200 Subject: [PATCH 05/10] Ensure selected playlist item is always scrolled into view --- .../Screens/OnlinePlay/DrawableRoomPlaylist.cs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs index d1b21ca1b0..fe654c4ab8 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Diagnostics; using System.Linq; using osu.Framework.Bindables; using osu.Framework.Graphics; @@ -173,6 +174,21 @@ namespace osu.Game.Screens.OnlinePlay protected virtual DrawableRoomPlaylistItem CreateDrawablePlaylistItem(PlaylistItem item) => new DrawableRoomPlaylistItem(item); + protected override void LoadComplete() + { + base.LoadComplete(); + + SelectedItem.BindValueChanged(_ => scrollToSelection(), true); + } + + private void scrollToSelection() + { + if (SelectedItem.Value == null) return; + + Debug.Assert(ItemMap.TryGetValue(SelectedItem.Value, out var drawableItem)); + ScrollContainer.ScrollIntoView(drawableItem); + } + #region Key selection logic (shared with BeatmapCarousel and RoomsContainer) public bool OnPressed(KeyBindingPressEvent e) From e6fdef2d7afbb769583bc5a4cab62eba438e5773 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 4 May 2022 13:51:47 +0200 Subject: [PATCH 06/10] Fix test failures due to selection/item collection desyncs --- osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs index fe654c4ab8..18cf454be3 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs @@ -2,7 +2,6 @@ // See the LICENCE file in the repository root for full licence text. using System; -using System.Diagnostics; using System.Linq; using osu.Framework.Bindables; using osu.Framework.Graphics; @@ -178,14 +177,19 @@ namespace osu.Game.Screens.OnlinePlay { base.LoadComplete(); - SelectedItem.BindValueChanged(_ => scrollToSelection(), true); + // schedules added as the properties may change value while the drawable items haven't been created yet. + SelectedItem.BindValueChanged(_ => Scheduler.AddOnce(scrollToSelection)); + Items.BindCollectionChanged((_, __) => Scheduler.AddOnce(scrollToSelection), true); } private void scrollToSelection() { - if (SelectedItem.Value == null) return; + // SelectedItem and ItemMap/drawable items are managed separately, + // so if the item can't be unmapped to a drawable, don't try to scroll to it. + // best effort is made to not drop any updates, by subscribing to both sources. + if (SelectedItem.Value == null || !ItemMap.TryGetValue(SelectedItem.Value, out var drawableItem)) + return; - Debug.Assert(ItemMap.TryGetValue(SelectedItem.Value, out var drawableItem)); ScrollContainer.ScrollIntoView(drawableItem); } From 464b3af5f37b23204c3c440d4d0359b670527384 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 5 May 2022 05:42:20 +0300 Subject: [PATCH 07/10] Rename local correctly --- osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs index 18cf454be3..d1b83c3749 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs @@ -220,18 +220,18 @@ namespace osu.Game.Screens.OnlinePlay if (SelectedItem.Disabled) return; - var visibleRooms = ListContainer.AsEnumerable().Where(r => r.IsPresent); + var visibleItems = ListContainer.AsEnumerable().Where(r => r.IsPresent); PlaylistItem item; if (SelectedItem.Value == null) - item = visibleRooms.FirstOrDefault()?.Model; + item = visibleItems.FirstOrDefault()?.Model; else { if (direction < 0) - visibleRooms = visibleRooms.Reverse(); + visibleItems = visibleItems.Reverse(); - item = visibleRooms.SkipWhile(r => r.Model != SelectedItem.Value).Skip(1).FirstOrDefault()?.Model; + item = visibleItems.SkipWhile(r => r.Model != SelectedItem.Value).Skip(1).FirstOrDefault()?.Model; } // we already have a valid selection only change selection if we still have a room to switch to. From f28978b85661ea1c878c27864788e0d65e68de57 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 5 May 2022 05:42:49 +0300 Subject: [PATCH 08/10] Handle against playlists which disallow selection `SelectedItem.Disabled` is also not checked against in the select-via-click flow inside `DrawableRoomPlaylistItem` (only `AllowSelection` is checked). --- .../Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs | 9 +++++++++ osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs index 566a8db608..d520384218 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs @@ -56,6 +56,9 @@ namespace osu.Game.Tests.Visual.Multiplayer AddStep("click", () => InputManager.Click(MouseButton.Left)); AddAssert("no item selected", () => playlist.SelectedItem.Value == null); + + AddStep("press down", () => InputManager.Key(Key.Down)); + AddAssert("no item selected", () => playlist.SelectedItem.Value == null); } [Test] @@ -73,6 +76,9 @@ namespace osu.Game.Tests.Visual.Multiplayer AddStep("click", () => InputManager.Click(MouseButton.Left)); AddAssert("no item selected", () => playlist.SelectedItem.Value == null); + + AddStep("press down", () => InputManager.Key(Key.Down)); + AddAssert("no item selected", () => playlist.SelectedItem.Value == null); } [Test] @@ -91,6 +97,9 @@ namespace osu.Game.Tests.Visual.Multiplayer AddStep("click", () => InputManager.Click(MouseButton.Left)); AddAssert("no item selected", () => playlist.SelectedItem.Value == null); + + AddStep("press down", () => InputManager.Key(Key.Down)); + AddAssert("no item selected", () => playlist.SelectedItem.Value == null); } [Test] diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs index d1b83c3749..840fc48613 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs @@ -217,7 +217,7 @@ namespace osu.Game.Screens.OnlinePlay private void selectNext(int direction) { - if (SelectedItem.Disabled) + if (!AllowSelection) return; var visibleItems = ListContainer.AsEnumerable().Where(r => r.IsPresent); From a26793cd654a29ada9d9d0234e796f531cd404c8 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 5 May 2022 06:12:24 +0300 Subject: [PATCH 09/10] Add missing `Test` prefix --- .../Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs index d520384218..e2b4b2870f 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs @@ -157,7 +157,7 @@ namespace osu.Game.Tests.Visual.Multiplayer } [Test] - public void KeyboardSelection() + public void TestKeyboardSelection() { createPlaylist(p => p.AllowSelection = true); From ac6342ff8d22213ac41a2df547ab69c816641a21 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 5 May 2022 10:16:16 +0300 Subject: [PATCH 10/10] Add workaround for item scrolling issue --- osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs index 840fc48613..2a72fc6eb1 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylist.cs @@ -190,7 +190,12 @@ namespace osu.Game.Screens.OnlinePlay if (SelectedItem.Value == null || !ItemMap.TryGetValue(SelectedItem.Value, out var drawableItem)) return; - ScrollContainer.ScrollIntoView(drawableItem); + // ScrollIntoView does not handle non-loaded items appropriately, delay scroll until the item finishes loading. + // see: https://github.com/ppy/osu-framework/issues/5158 + if (!drawableItem.IsLoaded) + drawableItem.OnLoadComplete += _ => ScrollContainer.ScrollIntoView(drawableItem); + else + ScrollContainer.ScrollIntoView(drawableItem); } #region Key selection logic (shared with BeatmapCarousel and RoomsContainer)