From e9187cc3cf3192b3559b23b7c4cf5a60d4b1832a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 15 Dec 2021 16:13:12 +0900 Subject: [PATCH 1/7] Add failing test showing expanded state being unexpectedly lost --- .../Visual/Beatmaps/TestSceneBeatmapCard.cs | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/osu.Game.Tests/Visual/Beatmaps/TestSceneBeatmapCard.cs b/osu.Game.Tests/Visual/Beatmaps/TestSceneBeatmapCard.cs index f835d21603..0b9857486a 100644 --- a/osu.Game.Tests/Visual/Beatmaps/TestSceneBeatmapCard.cs +++ b/osu.Game.Tests/Visual/Beatmaps/TestSceneBeatmapCard.cs @@ -6,12 +6,12 @@ using System.Collections.Generic; using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; -using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Shapes; using osu.Framework.Testing; using osu.Game.Beatmaps; +using osu.Game.Beatmaps.Drawables; using osu.Game.Beatmaps.Drawables.Cards; using osu.Game.Graphics.Containers; using osu.Game.Online.API; @@ -19,11 +19,10 @@ using osu.Game.Online.API.Requests; using osu.Game.Online.API.Requests.Responses; using osu.Game.Overlays; using osuTK; -using APIUser = osu.Game.Online.API.Requests.Responses.APIUser; namespace osu.Game.Tests.Visual.Beatmaps { - public class TestSceneBeatmapCard : OsuTestScene + public class TestSceneBeatmapCard : OsuManualInputManagerTestScene { /// /// All cards on this scene use a common online ID to ensure that map download, preview tracks, etc. can be tested manually with online sources. @@ -253,14 +252,32 @@ namespace osu.Game.Tests.Visual.Beatmaps public void TestNormal() { createTestCase(beatmapSetInfo => new BeatmapCard(beatmapSetInfo)); + } - AddToggleStep("toggle expanded state", expanded => - { - var card = this.ChildrenOfType().Last(); - if (!card.Expanded.Disabled) - card.Expanded.Value = expanded; - }); - AddToggleStep("disable/enable expansion", disabled => this.ChildrenOfType().ForEach(card => card.Expanded.Disabled = disabled)); + [Test] + public void TestHoverState() + { + AddStep("create cards", () => Child = createContent(OverlayColourScheme.Blue, s => new BeatmapCard(s))); + + AddStep("Hover card", () => InputManager.MoveMouseTo(firstCard())); + AddWaitStep("wait for potential state change", 5); + AddAssert("card is not expanded", () => !firstCard().Expanded.Value); + + AddStep("Hover spectrum display", () => InputManager.MoveMouseTo(firstCard().ChildrenOfType().Single())); + AddUntilStep("card is expanded", () => firstCard().Expanded.Value); + + AddStep("Hover difficulty content", () => InputManager.MoveMouseTo(firstCard().ChildrenOfType().Single())); + AddWaitStep("wait for potential state change", 5); + AddAssert("card is still expanded", () => firstCard().Expanded.Value); + + AddStep("Hover main content again", () => InputManager.MoveMouseTo(firstCard())); + AddWaitStep("wait for potential state change", 5); + AddAssert("card is still expanded", () => firstCard().Expanded.Value); + + AddStep("Hover away", () => InputManager.MoveMouseTo(this.ChildrenOfType().Last())); + AddUntilStep("card is not expanded", () => !firstCard().Expanded.Value); + + BeatmapCard firstCard() => this.ChildrenOfType().First(); } } } From 41e6c24dad8750edd2115c12ecd6857252036b46 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 15 Dec 2021 15:51:47 +0900 Subject: [PATCH 2/7] Expose `Expanded` state of `BeatmapCardContent` as read-only bindable This is just to reduce complexity of these interactions by ensuring that the expanded state can only be changed by the class itself. --- .../Beatmaps/Drawables/Cards/BeatmapCardContent.cs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs index 681f09c658..e353e61b71 100644 --- a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs +++ b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs @@ -31,7 +31,9 @@ namespace osu.Game.Beatmaps.Drawables.Cards set => dropdownScroll.Child = value; } - public Bindable Expanded { get; } = new BindableBool(); + public IBindable Expanded => expanded; + + private readonly BindableBool expanded = new BindableBool(); private readonly Box background; private readonly Container content; @@ -128,7 +130,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards scheduledExpandedChange = Scheduler.AddDelayed(() => { if (!Expanded.Disabled) - Expanded.Value = true; + expanded.Value = true; }, 100); } @@ -141,7 +143,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards scheduledExpandedChange = Scheduler.AddDelayed(() => { if (!Expanded.Disabled) - Expanded.Value = false; + expanded.Value = false; }, 500); } @@ -154,7 +156,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards return; scheduledExpandedChange?.Cancel(); - Expanded.Value = false; + expanded.Value = false; } private void keep() @@ -163,7 +165,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards return; scheduledExpandedChange?.Cancel(); - Expanded.Value = true; + expanded.Value = true; } private void updateState() From ef4ab74565a843d23da2e05bc01a7ace7478a353 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 15 Dec 2021 16:19:47 +0900 Subject: [PATCH 3/7] Also only expose `Expanded` state of `BeatmapCard` as read-only --- osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs | 6 ++++-- osu.Game/Screens/Play/SoloSpectator.cs | 5 +---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs index d93ac841ab..435a8227bd 100644 --- a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs +++ b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs @@ -33,7 +33,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards public const float TRANSITION_DURATION = 400; public const float CORNER_RADIUS = 10; - public Bindable Expanded { get; } = new BindableBool(); + public IBindable Expanded { get; } private const float width = 408; private const float height = 100; @@ -64,9 +64,11 @@ namespace osu.Game.Beatmaps.Drawables.Cards [Resolved] private OverlayColourProvider colourProvider { get; set; } = null!; - public BeatmapCard(APIBeatmapSet beatmapSet) + public BeatmapCard(APIBeatmapSet beatmapSet, bool allowExpansion = true) : base(HoverSampleSet.Submit) { + Expanded = new BindableBool { Disabled = !allowExpansion }; + this.beatmapSet = beatmapSet; favouriteState = new Bindable(new BeatmapSetFavouriteState(beatmapSet.HasFavourited, beatmapSet.FavouriteCount)); downloadTracker = new BeatmapDownloadTracker(beatmapSet); diff --git a/osu.Game/Screens/Play/SoloSpectator.cs b/osu.Game/Screens/Play/SoloSpectator.cs index 3918dbe8fc..ba5663bfa3 100644 --- a/osu.Game/Screens/Play/SoloSpectator.cs +++ b/osu.Game/Screens/Play/SoloSpectator.cs @@ -228,10 +228,7 @@ namespace osu.Game.Screens.Play onlineBeatmapRequest.Success += beatmapSet => Schedule(() => { this.beatmapSet = beatmapSet; - beatmapPanelContainer.Child = new BeatmapCard(this.beatmapSet) - { - Expanded = { Disabled = true } - }; + beatmapPanelContainer.Child = new BeatmapCard(this.beatmapSet, allowExpansion: false); checkForAutomaticDownload(); }); From 7a9db22c5240a600b87a40b74cc6416cc0d9122f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 15 Dec 2021 16:02:43 +0900 Subject: [PATCH 4/7] Tidy up method naming and structure for expanded state changes --- .../Beatmaps/Drawables/Cards/BeatmapCard.cs | 6 +-- .../Drawables/Cards/BeatmapCardContent.cs | 52 +++++-------------- 2 files changed, 15 insertions(+), 43 deletions(-) diff --git a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs index 435a8227bd..2e39fafa05 100644 --- a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs +++ b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs @@ -284,7 +284,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards { Hovered = _ => { - content.ScheduleShow(); + content.ExpandAfterDelay(); return false; }, Unhovered = _ => @@ -292,7 +292,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards // This hide should only trigger if the expanded content has not shown yet. // ie. if the user has not shown intent to want to see it (quickly moved over the info row area). if (!Expanded.Value) - content.ScheduleHide(); + content.CollapseAfterDelay(); } } } @@ -368,7 +368,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards protected override void OnHoverLost(HoverLostEvent e) { - content.ScheduleHide(); + content.CollapseAfterDelay(); updateState(); base.OnHoverLost(e); diff --git a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs index e353e61b71..148372786a 100644 --- a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs +++ b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs @@ -56,7 +56,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards AutoSizeAxes = Axes.Y, CornerRadius = BeatmapCard.CORNER_RADIUS, Masking = true, - Unhovered = _ => checkForHide(), + Unhovered = _ => collapseIfNotHovered(), Children = new Drawable[] { background = new Box @@ -78,10 +78,10 @@ namespace osu.Game.Beatmaps.Drawables.Cards Alpha = 0, Hovered = _ => { - keep(); + queueExpandedStateChange(true); return true; }, - Unhovered = _ => checkForHide(), + Unhovered = _ => collapseIfNotHovered(), Child = dropdownScroll = new ExpandedContentScrollContainer { RelativeSizeAxes = Axes.X, @@ -121,51 +121,23 @@ namespace osu.Game.Beatmaps.Drawables.Cards private ScheduledDelegate? scheduledExpandedChange; - public void ScheduleShow() - { - scheduledExpandedChange?.Cancel(); - if (Expanded.Disabled || Expanded.Value) - return; + public void ExpandAfterDelay() => queueExpandedStateChange(true, 100); - scheduledExpandedChange = Scheduler.AddDelayed(() => - { - if (!Expanded.Disabled) - expanded.Value = true; - }, 100); + public void CollapseAfterDelay() => queueExpandedStateChange(false, 500); + + private void collapseIfNotHovered() + { + if (!content.IsHovered && !dropdownContent.IsHovered) + queueExpandedStateChange(false); } - public void ScheduleHide() - { - scheduledExpandedChange?.Cancel(); - if (Expanded.Disabled || !Expanded.Value) - return; - - scheduledExpandedChange = Scheduler.AddDelayed(() => - { - if (!Expanded.Disabled) - expanded.Value = false; - }, 500); - } - - private void checkForHide() - { - if (Expanded.Disabled) - return; - - if (content.IsHovered || dropdownContent.IsHovered) - return; - - scheduledExpandedChange?.Cancel(); - expanded.Value = false; - } - - private void keep() + private void queueExpandedStateChange(bool newState, int delay = 0) { if (Expanded.Disabled) return; scheduledExpandedChange?.Cancel(); - expanded.Value = true; + scheduledExpandedChange = Scheduler.AddDelayed(() => expanded.Value = newState, delay); } private void updateState() From 94d1a2aacae0a6472e06eeb004004a984feb69c4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 15 Dec 2021 16:37:44 +0900 Subject: [PATCH 5/7] Remove unnecessary collapse call from `BeatmapCard` This is already handled at the `BeatmapCardContent` level. This call actually causes the buggy behaviour reported in https://github.com/ppy/osu/discussions/16085. --- osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs index 2e39fafa05..be7119be36 100644 --- a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs +++ b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs @@ -368,8 +368,6 @@ namespace osu.Game.Beatmaps.Drawables.Cards protected override void OnHoverLost(HoverLostEvent e) { - content.CollapseAfterDelay(); - updateState(); base.OnHoverLost(e); } From 6a1f535257c49720f9f81ca80d9942d44889456d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 15 Dec 2021 16:38:19 +0900 Subject: [PATCH 6/7] Refactor cancellation of expand to be more explicit --- osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs | 6 +++--- osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs index be7119be36..1e24501426 100644 --- a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs +++ b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCard.cs @@ -289,10 +289,10 @@ namespace osu.Game.Beatmaps.Drawables.Cards }, Unhovered = _ => { - // This hide should only trigger if the expanded content has not shown yet. - // ie. if the user has not shown intent to want to see it (quickly moved over the info row area). + // Handles the case where a user has not shown explicit intent to view expanded info. + // ie. quickly moved over the info row area but didn't remain within it. if (!Expanded.Value) - content.CollapseAfterDelay(); + content.CancelExpand(); } } } diff --git a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs index 148372786a..0739f7328f 100644 --- a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs +++ b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs @@ -123,7 +123,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards public void ExpandAfterDelay() => queueExpandedStateChange(true, 100); - public void CollapseAfterDelay() => queueExpandedStateChange(false, 500); + public void CancelExpand() => scheduledExpandedChange?.Cancel(); private void collapseIfNotHovered() { From ad430a627701afa2fcd7c538b99e232d4f54a693 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 15 Dec 2021 16:44:58 +0900 Subject: [PATCH 7/7] Centralise hover state handling (and fix back-to-front conditionals) --- .../Beatmaps/Drawables/Cards/BeatmapCardContent.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs index 0739f7328f..286e03e700 100644 --- a/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs +++ b/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs @@ -56,7 +56,7 @@ namespace osu.Game.Beatmaps.Drawables.Cards AutoSizeAxes = Axes.Y, CornerRadius = BeatmapCard.CORNER_RADIUS, Masking = true, - Unhovered = _ => collapseIfNotHovered(), + Unhovered = _ => updateFromHoverChange(), Children = new Drawable[] { background = new Box @@ -78,10 +78,10 @@ namespace osu.Game.Beatmaps.Drawables.Cards Alpha = 0, Hovered = _ => { - queueExpandedStateChange(true); + updateFromHoverChange(); return true; }, - Unhovered = _ => collapseIfNotHovered(), + Unhovered = _ => updateFromHoverChange(), Child = dropdownScroll = new ExpandedContentScrollContainer { RelativeSizeAxes = Axes.X, @@ -125,11 +125,8 @@ namespace osu.Game.Beatmaps.Drawables.Cards public void CancelExpand() => scheduledExpandedChange?.Cancel(); - private void collapseIfNotHovered() - { - if (!content.IsHovered && !dropdownContent.IsHovered) - queueExpandedStateChange(false); - } + private void updateFromHoverChange() => + queueExpandedStateChange(content.IsHovered || dropdownContent.IsHovered, 100); private void queueExpandedStateChange(bool newState, int delay = 0) {