From c6d303a5b44ba2f0d0c221178e9abab50dc2fbaa Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 21:16:29 +0900 Subject: [PATCH 01/33] Add xmldoc to `Leaderboard` class --- osu.Game/Online/Leaderboards/Leaderboard.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 515cc6fd73..e2a52b5db9 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -23,6 +23,12 @@ using osuTK.Graphics; namespace osu.Game.Online.Leaderboards { + /// + /// A leaderboard which displays a scrolling list of top scores, along with a single "user best" + /// for the local user. + /// + /// The scope of the leaderboard (ie. global or local). + /// The score model class. public abstract class Leaderboard : Container { private const double fade_duration = 300; From aee93934d5c42792b41ef3b13c7baa748b7c3ee0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 21:22:09 +0900 Subject: [PATCH 02/33] Rename methods to make more sense (and always run through `AddOnce`) --- .../Visual/UserInterface/TestSceneDeleteLocalScore.cs | 2 +- osu.Game/Online/Leaderboards/Leaderboard.cs | 10 +++++----- .../OnlinePlay/Match/Components/MatchLeaderboard.cs | 2 +- .../OnlinePlay/Playlists/PlaylistsRoomSubScreen.cs | 2 +- .../Screens/Select/Leaderboards/BeatmapLeaderboard.cs | 10 +++++----- osu.Game/Screens/Select/PlayBeatmapDetailArea.cs | 2 +- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs index 4826d2fb33..4938fd1271 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs @@ -132,7 +132,7 @@ namespace osu.Game.Tests.Visual.UserInterface leaderboard.FinishTransforms(true); // After setting scores, we may be waiting for transforms to expire drawables leaderboard.BeatmapInfo = beatmapInfo; - leaderboard.RefreshScores(); // Required in the case that the beatmap hasn't changed + leaderboard.RefetchScores(); // Required in the case that the beatmap hasn't changed }); [SetUpSteps] diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index e2a52b5db9..cd67fc7301 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -132,7 +132,7 @@ namespace osu.Game.Online.Leaderboards return; scope = value; - RefreshScores(); + RefetchScores(); } } @@ -160,7 +160,7 @@ namespace osu.Game.Online.Leaderboards case PlaceholderState.NetworkFailure: replacePlaceholder(new ClickablePlaceholder(@"Couldn't fetch scores!", FontAwesome.Solid.Sync) { - Action = RefreshScores + Action = RefetchScores }); break; @@ -272,15 +272,15 @@ namespace osu.Game.Online.Leaderboards case APIState.Online: case APIState.Offline: if (IsOnlineScope) - RefreshScores(); + RefetchScores(); break; } }); - public void RefreshScores() => Scheduler.AddOnce(UpdateScores); + public void RefetchScores() => Scheduler.AddOnce(refetchScores); - protected void UpdateScores() + private void refetchScores() { // don't display any scores or placeholder until the first Scores_Set has been called. // this avoids scope changes flickering a "no scores" placeholder before initialisation of song select is finished. diff --git a/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs b/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs index 134e083c42..e4ec3ac4a5 100644 --- a/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs +++ b/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs @@ -26,7 +26,7 @@ namespace osu.Game.Screens.OnlinePlay.Match.Components return; Scores = null; - UpdateScores(); + RefetchScores(); }, true); } diff --git a/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomSubScreen.cs b/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomSubScreen.cs index 4114a5e9a0..542851cb0f 100644 --- a/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Playlists/PlaylistsRoomSubScreen.cs @@ -220,7 +220,7 @@ namespace osu.Game.Screens.OnlinePlay.Playlists protected override Screen CreateGameplayScreen() => new PlayerLoader(() => new PlaylistsPlayer(Room, SelectedItem.Value) { - Exited = () => leaderboard.RefreshScores() + Exited = () => leaderboard.RefetchScores() }); } } diff --git a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs index 31cbe91f5c..02e4e162dd 100644 --- a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs @@ -53,7 +53,7 @@ namespace osu.Game.Screens.Select.Leaderboards Scores = null; if (IsOnlineScope) - UpdateScores(); + RefetchScores(); else { if (IsLoaded) @@ -77,7 +77,7 @@ namespace osu.Game.Screens.Select.Leaderboards filterMods = value; - UpdateScores(); + RefetchScores(); } } @@ -96,11 +96,11 @@ namespace osu.Game.Screens.Select.Leaderboards [BackgroundDependencyLoader] private void load() { - ruleset.ValueChanged += _ => UpdateScores(); + ruleset.ValueChanged += _ => RefetchScores(); mods.ValueChanged += _ => { if (filterMods) - UpdateScores(); + RefetchScores(); }; } @@ -127,7 +127,7 @@ namespace osu.Game.Screens.Select.Leaderboards (_, changes, ___) => { if (!IsOnlineScope) - RefreshScores(); + RefetchScores(); }); } diff --git a/osu.Game/Screens/Select/PlayBeatmapDetailArea.cs b/osu.Game/Screens/Select/PlayBeatmapDetailArea.cs index b8b8e3e4bc..09f75b7658 100644 --- a/osu.Game/Screens/Select/PlayBeatmapDetailArea.cs +++ b/osu.Game/Screens/Select/PlayBeatmapDetailArea.cs @@ -53,7 +53,7 @@ namespace osu.Game.Screens.Select { base.Refresh(); - Leaderboard.RefreshScores(); + Leaderboard.RefetchScores(); } protected override void OnTabChanged(BeatmapDetailAreaTabItem tab, bool selectedMods) From b9dac6c3b2d312bc79fefff4d5abb1c24d78c00c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 21:33:22 +0900 Subject: [PATCH 03/33] Reorder and tidy up bindable flows --- osu.Game/Online/Leaderboards/Leaderboard.cs | 81 ++++++++++++--------- 1 file changed, 45 insertions(+), 36 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index cd67fc7301..af448b7d7b 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -46,6 +46,18 @@ namespace osu.Game.Online.Leaderboards private bool scoresLoadedOnce; + private APIRequest getScoresRequest; + private ScheduledDelegate getScoresRequestCallback; + + protected abstract bool IsOnlineScope { get; } + + [Resolved(CanBeNull = true)] + private IAPIProvider api { get; set; } + + private ScheduledDelegate pendingUpdateScores; + + private readonly IBindable apiState = new Bindable(); + private readonly Container content; protected override Container Content => content; @@ -239,44 +251,34 @@ namespace osu.Game.Online.Leaderboards protected virtual void Reset() { - getScoresRequest?.Cancel(); - getScoresRequest = null; + cancelPendingWork(); + Scores = null; } - [Resolved(CanBeNull = true)] - private IAPIProvider api { get; set; } - - private ScheduledDelegate pendingUpdateScores; - - private readonly IBindable apiState = new Bindable(); - - [BackgroundDependencyLoader] - private void load() + protected override void LoadComplete() { + base.LoadComplete(); + if (api != null) - apiState.BindTo(api.State); - - apiState.BindValueChanged(onlineStateChanged, true); - } - - private APIRequest getScoresRequest; - private ScheduledDelegate getScoresRequestCallback; - - protected abstract bool IsOnlineScope { get; } - - private void onlineStateChanged(ValueChangedEvent state) => Schedule(() => - { - switch (state.NewValue) { - case APIState.Online: - case APIState.Offline: - if (IsOnlineScope) - RefetchScores(); + apiState.BindTo(api.State); + apiState.BindValueChanged(state => + { + switch (state.NewValue) + { + case APIState.Online: + case APIState.Offline: + if (IsOnlineScope) + RefetchScores(); - break; + break; + } + }); } - }); + + RefetchScores(); + } public void RefetchScores() => Scheduler.AddOnce(refetchScores); @@ -286,13 +288,8 @@ namespace osu.Game.Online.Leaderboards // this avoids scope changes flickering a "no scores" placeholder before initialisation of song select is finished. if (!scoresLoadedOnce) return; - getScoresRequest?.Cancel(); - getScoresRequest = null; + cancelPendingWork(); - getScoresRequestCallback?.Cancel(); - getScoresRequestCallback = null; - - pendingUpdateScores?.Cancel(); pendingUpdateScores = Schedule(() => { PlaceholderState = PlaceholderState.Retrieving; @@ -319,6 +316,18 @@ namespace osu.Game.Online.Leaderboards }); } + private void cancelPendingWork() + { + getScoresRequest?.Cancel(); + getScoresRequest = null; + + getScoresRequestCallback?.Cancel(); + getScoresRequestCallback = null; + + pendingUpdateScores?.Cancel(); + pendingUpdateScores = null; + } + /// /// Performs a fetch/refresh of scores to be displayed. /// From 64925b3feaafa353bbfb87af39e13ee9e762a8bb Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 21:38:23 +0900 Subject: [PATCH 04/33] Remove unused `Content` override --- osu.Game/Online/Leaderboards/Leaderboard.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index af448b7d7b..785c2e2ecf 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -29,7 +29,7 @@ namespace osu.Game.Online.Leaderboards /// /// The scope of the leaderboard (ie. global or local). /// The score model class. - public abstract class Leaderboard : Container + public abstract class Leaderboard : CompositeDrawable { private const double fade_duration = 300; @@ -58,10 +58,6 @@ namespace osu.Game.Online.Leaderboards private readonly IBindable apiState = new Bindable(); - private readonly Container content; - - protected override Container Content => content; - private ICollection scores; public ICollection Scores @@ -231,7 +227,7 @@ namespace osu.Game.Online.Leaderboards }, new Drawable[] { - content = new Container + new Container { AutoSizeAxes = Axes.Y, RelativeSizeAxes = Axes.X, From 17aa9f304000505d5aff1d04239f5fa828873871 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 21:47:28 +0900 Subject: [PATCH 05/33] Remove pointless level of schedule/cancel logic --- osu.Game/Online/Leaderboards/Leaderboard.cs | 39 ++++++++------------- 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 785c2e2ecf..7ab3d0343a 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -54,8 +54,6 @@ namespace osu.Game.Online.Leaderboards [Resolved(CanBeNull = true)] private IAPIProvider api { get; set; } - private ScheduledDelegate pendingUpdateScores; - private readonly IBindable apiState = new Bindable(); private ICollection scores; @@ -248,7 +246,6 @@ namespace osu.Game.Online.Leaderboards protected virtual void Reset() { cancelPendingWork(); - Scores = null; } @@ -286,30 +283,27 @@ namespace osu.Game.Online.Leaderboards cancelPendingWork(); - pendingUpdateScores = Schedule(() => + PlaceholderState = PlaceholderState.Retrieving; + loading.Show(); + + getScoresRequest = FetchScores(scores => getScoresRequestCallback = Schedule(() => { - PlaceholderState = PlaceholderState.Retrieving; - loading.Show(); + Scores = scores.ToArray(); + PlaceholderState = Scores.Any() ? PlaceholderState.Successful : PlaceholderState.NoScores; + })); - getScoresRequest = FetchScores(scores => getScoresRequestCallback = Schedule(() => - { - Scores = scores.ToArray(); - PlaceholderState = Scores.Any() ? PlaceholderState.Successful : PlaceholderState.NoScores; - })); + if (getScoresRequest == null) + return; - if (getScoresRequest == null) + getScoresRequest.Failure += e => getScoresRequestCallback = Schedule(() => + { + if (e is OperationCanceledException) return; - getScoresRequest.Failure += e => getScoresRequestCallback = Schedule(() => - { - if (e is OperationCanceledException) - return; - - PlaceholderState = PlaceholderState.NetworkFailure; - }); - - api?.Queue(getScoresRequest); + PlaceholderState = PlaceholderState.NetworkFailure; }); + + api?.Queue(getScoresRequest); } private void cancelPendingWork() @@ -319,9 +313,6 @@ namespace osu.Game.Online.Leaderboards getScoresRequestCallback?.Cancel(); getScoresRequestCallback = null; - - pendingUpdateScores?.Cancel(); - pendingUpdateScores = null; } /// From c5486586623acd425be534fced770e456493c85c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 21:49:52 +0900 Subject: [PATCH 06/33] Remove move unused pieces --- osu.Game/Online/Leaderboards/Leaderboard.cs | 29 ++++++++------------- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 7ab3d0343a..a426d2b448 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -82,7 +82,13 @@ namespace osu.Game.Online.Leaderboards // ensure placeholder is hidden when displaying scores PlaceholderState = PlaceholderState.Successful; - var scoreFlow = CreateScoreFlow(); + var scoreFlow = new FillFlowContainer + { + RelativeSizeAxes = Axes.X, + AutoSizeAxes = Axes.Y, + Spacing = new Vector2(0f, 5f), + Padding = new MarginPadding { Top = 10, Bottom = 5 }, + }; scoreFlow.ChildrenEnumerable = scores.Select((s, index) => CreateDrawableScore(s, index + 1)); // schedule because we may not be loaded yet (LoadComponentAsync complains). @@ -118,15 +124,6 @@ namespace osu.Game.Online.Leaderboards } } - protected virtual FillFlowContainer CreateScoreFlow() - => new FillFlowContainer - { - RelativeSizeAxes = Axes.X, - AutoSizeAxes = Axes.Y, - Spacing = new Vector2(0f, 5f), - Padding = new MarginPadding { Top = 10, Bottom = 5 }, - }; - private TScope scope; public TScope Scope @@ -345,9 +342,6 @@ namespace osu.Game.Online.Leaderboards currentPlaceholder = placeholder; } - protected virtual bool FadeBottom => true; - protected virtual bool FadeTop => false; - protected override void UpdateAfterChildren() { base.UpdateAfterChildren(); @@ -366,22 +360,21 @@ namespace osu.Game.Online.Leaderboards float topY = c.ToSpaceOfOtherDrawable(Vector2.Zero, scrollFlow).Y; float bottomY = topY + LeaderboardScore.HEIGHT; - bool requireTopFade = FadeTop && topY <= fadeTop; - bool requireBottomFade = FadeBottom && bottomY >= fadeBottom; + bool requireBottomFade = bottomY >= fadeBottom; - if (!requireTopFade && !requireBottomFade) + if (!requireBottomFade) c.Colour = Color4.White; else if (topY > fadeBottom + LeaderboardScore.HEIGHT || bottomY < fadeTop - LeaderboardScore.HEIGHT) c.Colour = Color4.Transparent; else { - if (bottomY - fadeBottom > 0 && FadeBottom) + if (bottomY - fadeBottom > 0) { c.Colour = ColourInfo.GradientVertical( Color4.White.Opacity(Math.Min(1 - (topY - fadeBottom) / LeaderboardScore.HEIGHT, 1)), Color4.White.Opacity(Math.Min(1 - (bottomY - fadeBottom) / LeaderboardScore.HEIGHT, 1))); } - else if (FadeTop) + else { c.Colour = ColourInfo.GradientVertical( Color4.White.Opacity(Math.Min(1 - (fadeTop - topY) / LeaderboardScore.HEIGHT, 1)), From b85b2c01fb248db8a3deb773ba1ed5da930d07c2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 21:53:12 +0900 Subject: [PATCH 07/33] Reorder based on accessibility and add regions --- osu.Game/Online/Leaderboards/Leaderboard.cs | 268 ++++++++++---------- 1 file changed, 138 insertions(+), 130 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index a426d2b448..d843dcb2bb 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -139,6 +139,139 @@ namespace osu.Game.Online.Leaderboards } } + protected Leaderboard() + { + InternalChildren = new Drawable[] + { + new OsuContextMenuContainer + { + RelativeSizeAxes = Axes.Both, + Masking = true, + Child = new GridContainer + { + RelativeSizeAxes = Axes.Both, + RowDimensions = new[] + { + new Dimension(), + new Dimension(GridSizeMode.AutoSize), + }, + Content = new[] + { + new Drawable[] + { + scrollContainer = new OsuScrollContainer + { + RelativeSizeAxes = Axes.Both, + ScrollbarVisible = false, + } + }, + new Drawable[] + { + new Container + { + AutoSizeAxes = Axes.Y, + RelativeSizeAxes = Axes.X, + Child = topScoreContainer = new UserTopScoreContainer(CreateDrawableTopScore) + }, + }, + }, + }, + }, + loading = new LoadingSpinner(), + placeholderContainer = new Container + { + RelativeSizeAxes = Axes.Both + }, + }; + } + + protected override void LoadComplete() + { + base.LoadComplete(); + + if (api != null) + { + apiState.BindTo(api.State); + apiState.BindValueChanged(state => + { + switch (state.NewValue) + { + case APIState.Online: + case APIState.Offline: + if (IsOnlineScope) + RefetchScores(); + + break; + } + }); + } + + RefetchScores(); + } + + public void RefetchScores() => Scheduler.AddOnce(refetchScores); + + protected virtual void Reset() + { + cancelPendingWork(); + Scores = null; + } + + /// + /// Performs a fetch/refresh of scores to be displayed. + /// + /// A callback which should be called when fetching is completed. Scheduling is not required. + /// An responsible for the fetch operation. This will be queued and performed automatically. + protected abstract APIRequest FetchScores(Action> scoresCallback); + + protected abstract LeaderboardScore CreateDrawableScore(TScoreInfo model, int index); + + protected abstract LeaderboardScore CreateDrawableTopScore(TScoreInfo model); + + private void refetchScores() + { + // don't display any scores or placeholder until the first Scores_Set has been called. + // this avoids scope changes flickering a "no scores" placeholder before initialisation of song select is finished. + if (!scoresLoadedOnce) return; + + cancelPendingWork(); + + PlaceholderState = PlaceholderState.Retrieving; + loading.Show(); + + getScoresRequest = FetchScores(scores => getScoresRequestCallback = Schedule(() => + { + Scores = scores.ToArray(); + PlaceholderState = Scores.Any() ? PlaceholderState.Successful : PlaceholderState.NoScores; + })); + + if (getScoresRequest == null) + return; + + getScoresRequest.Failure += e => getScoresRequestCallback = Schedule(() => + { + if (e is OperationCanceledException) + return; + + PlaceholderState = PlaceholderState.NetworkFailure; + }); + + api?.Queue(getScoresRequest); + } + + private void cancelPendingWork() + { + getScoresRequest?.Cancel(); + getScoresRequest = null; + + getScoresRequestCallback?.Cancel(); + getScoresRequestCallback = null; + } + + #region Placeholder handling + + private Placeholder currentPlaceholder; + private PlaceholderState placeholderState; /// @@ -194,133 +327,6 @@ namespace osu.Game.Online.Leaderboards } } - protected Leaderboard() - { - InternalChildren = new Drawable[] - { - new OsuContextMenuContainer - { - RelativeSizeAxes = Axes.Both, - Masking = true, - Child = new GridContainer - { - RelativeSizeAxes = Axes.Both, - RowDimensions = new[] - { - new Dimension(), - new Dimension(GridSizeMode.AutoSize), - }, - Content = new[] - { - new Drawable[] - { - scrollContainer = new OsuScrollContainer - { - RelativeSizeAxes = Axes.Both, - ScrollbarVisible = false, - } - }, - new Drawable[] - { - new Container - { - AutoSizeAxes = Axes.Y, - RelativeSizeAxes = Axes.X, - Child = topScoreContainer = new UserTopScoreContainer(CreateDrawableTopScore) - }, - }, - }, - }, - }, - loading = new LoadingSpinner(), - placeholderContainer = new Container - { - RelativeSizeAxes = Axes.Both - }, - }; - } - - protected virtual void Reset() - { - cancelPendingWork(); - Scores = null; - } - - protected override void LoadComplete() - { - base.LoadComplete(); - - if (api != null) - { - apiState.BindTo(api.State); - apiState.BindValueChanged(state => - { - switch (state.NewValue) - { - case APIState.Online: - case APIState.Offline: - if (IsOnlineScope) - RefetchScores(); - - break; - } - }); - } - - RefetchScores(); - } - - public void RefetchScores() => Scheduler.AddOnce(refetchScores); - - private void refetchScores() - { - // don't display any scores or placeholder until the first Scores_Set has been called. - // this avoids scope changes flickering a "no scores" placeholder before initialisation of song select is finished. - if (!scoresLoadedOnce) return; - - cancelPendingWork(); - - PlaceholderState = PlaceholderState.Retrieving; - loading.Show(); - - getScoresRequest = FetchScores(scores => getScoresRequestCallback = Schedule(() => - { - Scores = scores.ToArray(); - PlaceholderState = Scores.Any() ? PlaceholderState.Successful : PlaceholderState.NoScores; - })); - - if (getScoresRequest == null) - return; - - getScoresRequest.Failure += e => getScoresRequestCallback = Schedule(() => - { - if (e is OperationCanceledException) - return; - - PlaceholderState = PlaceholderState.NetworkFailure; - }); - - api?.Queue(getScoresRequest); - } - - private void cancelPendingWork() - { - getScoresRequest?.Cancel(); - getScoresRequest = null; - - getScoresRequestCallback?.Cancel(); - getScoresRequestCallback = null; - } - - /// - /// Performs a fetch/refresh of scores to be displayed. - /// - /// A callback which should be called when fetching is completed. Scheduling is not required. - /// An responsible for the fetch operation. This will be queued and performed automatically. - protected abstract APIRequest FetchScores(Action> scoresCallback); - - private Placeholder currentPlaceholder; - private void replacePlaceholder(Placeholder placeholder) { if (placeholder != null && placeholder.Equals(currentPlaceholder)) @@ -342,6 +348,10 @@ namespace osu.Game.Online.Leaderboards currentPlaceholder = placeholder; } + #endregion + + #region Fade handling + protected override void UpdateAfterChildren() { base.UpdateAfterChildren(); @@ -384,8 +394,6 @@ namespace osu.Game.Online.Leaderboards } } - protected abstract LeaderboardScore CreateDrawableScore(TScoreInfo model, int index); - - protected abstract LeaderboardScore CreateDrawableTopScore(TScoreInfo model); + #endregion } } From 661fec7c8abd1fba873a5003092d9f5f45da475c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 21:59:29 +0900 Subject: [PATCH 08/33] Make score setter private --- .../Visual/SongSelect/TestSceneBeatmapLeaderboard.cs | 5 ++++- .../Visual/UserInterface/TestSceneDeleteLocalScore.cs | 3 --- osu.Game/Online/Leaderboards/Leaderboard.cs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs index e31be1d51a..7ffab2eebc 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Collections.Generic; using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; @@ -100,7 +101,7 @@ namespace osu.Game.Tests.Visual.SongSelect public void TestGlobalScoresDisplay() { AddStep(@"Set scope", () => leaderboard.Scope = BeatmapLeaderboardScope.Global); - AddStep(@"New Scores", () => leaderboard.Scores = generateSampleScores(null)); + AddStep(@"New Scores", () => leaderboard.SetScores(generateSampleScores(null))); } [Test] @@ -422,6 +423,8 @@ namespace osu.Game.Tests.Visual.SongSelect { PlaceholderState = state; } + + public void SetScores(ICollection scores) => Scores = scores; } } } diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs index 4938fd1271..985ff6f251 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs @@ -128,9 +128,6 @@ namespace osu.Game.Tests.Visual.UserInterface scoreManager.Undelete(r.All().Where(s => s.DeletePending).ToList()); }); - leaderboard.Scores = null; - leaderboard.FinishTransforms(true); // After setting scores, we may be waiting for transforms to expire drawables - leaderboard.BeatmapInfo = beatmapInfo; leaderboard.RefetchScores(); // Required in the case that the beatmap hasn't changed }); diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index d843dcb2bb..0c58faa178 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -61,7 +61,7 @@ namespace osu.Game.Online.Leaderboards public ICollection Scores { get => scores; - set + protected set { scores = value; From a700ad38499ed2fbaed093e5de5570b27f5cd0c3 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 22:04:34 +0900 Subject: [PATCH 09/33] Remove `scoresLoadedOnce` weirdness --- osu.Game/Online/Leaderboards/Leaderboard.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 0c58faa178..238e883d24 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -44,8 +44,6 @@ namespace osu.Game.Online.Leaderboards private ScheduledDelegate showScoresDelegate; private CancellationTokenSource showScoresCancellationSource; - private bool scoresLoadedOnce; - private APIRequest getScoresRequest; private ScheduledDelegate getScoresRequestCallback; @@ -65,8 +63,6 @@ namespace osu.Game.Online.Leaderboards { scores = value; - scoresLoadedOnce = true; - scrollFlow?.FadeOut(fade_duration, Easing.OutQuint).Expire(); scrollFlow = null; @@ -230,10 +226,6 @@ namespace osu.Game.Online.Leaderboards private void refetchScores() { - // don't display any scores or placeholder until the first Scores_Set has been called. - // this avoids scope changes flickering a "no scores" placeholder before initialisation of song select is finished. - if (!scoresLoadedOnce) return; - cancelPendingWork(); PlaceholderState = PlaceholderState.Retrieving; From c48e9f2bbdc2dba7360e5d0b7ef9e4dad12256e4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 22:13:52 +0900 Subject: [PATCH 10/33] Remove more unnecessary schedule/cancel logic --- osu.Game/Online/Leaderboards/Leaderboard.cs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 238e883d24..e8fc2d126e 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -3,10 +3,12 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Threading; using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Framework.Development; using osu.Framework.Extensions.Color4Extensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Colour; @@ -41,7 +43,6 @@ namespace osu.Game.Online.Leaderboards private readonly LoadingSpinner loading; - private ScheduledDelegate showScoresDelegate; private CancellationTokenSource showScoresCancellationSource; private APIRequest getScoresRequest; @@ -61,15 +62,16 @@ namespace osu.Game.Online.Leaderboards get => scores; protected set { + Debug.Assert(ThreadSafety.IsUpdateThread); + scores = value; scrollFlow?.FadeOut(fade_duration, Easing.OutQuint).Expire(); scrollFlow = null; - showScoresDelegate?.Cancel(); showScoresCancellationSource?.Cancel(); - if (scores == null || !scores.Any()) + if (scores?.Any() != true) { loading.Hide(); return; @@ -84,11 +86,11 @@ namespace osu.Game.Online.Leaderboards AutoSizeAxes = Axes.Y, Spacing = new Vector2(0f, 5f), Padding = new MarginPadding { Top = 10, Bottom = 5 }, + ChildrenEnumerable = scores.Select((s, index) => CreateDrawableScore(s, index + 1)) }; - scoreFlow.ChildrenEnumerable = scores.Select((s, index) => CreateDrawableScore(s, index + 1)); // schedule because we may not be loaded yet (LoadComponentAsync complains). - showScoresDelegate = Schedule(() => LoadComponentAsync(scoreFlow, _ => + LoadComponentAsync(scoreFlow, _ => { scrollContainer.Add(scrollFlow = scoreFlow); @@ -100,9 +102,9 @@ namespace osu.Game.Online.Leaderboards s.Show(); } - scrollContainer.ScrollTo(0f, false); + scrollContainer.ScrollToStart(false); loading.Hide(); - }, (showScoresCancellationSource = new CancellationTokenSource()).Token)); + }, (showScoresCancellationSource = new CancellationTokenSource()).Token); } } @@ -253,6 +255,9 @@ namespace osu.Game.Online.Leaderboards private void cancelPendingWork() { + showScoresCancellationSource?.Cancel(); + showScoresCancellationSource = null; + getScoresRequest?.Cancel(); getScoresRequest = null; From 13f445ddd5c49b2be0ccf2efeeff72c4703b1b8b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 22:28:13 +0900 Subject: [PATCH 11/33] Move score update code into own method --- osu.Game/Online/Leaderboards/Leaderboard.cs | 91 +++++++++++---------- 1 file changed, 48 insertions(+), 43 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index e8fc2d126e..0302568b34 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -33,6 +33,8 @@ namespace osu.Game.Online.Leaderboards /// The score model class. public abstract class Leaderboard : CompositeDrawable { + protected abstract bool IsOnlineScope { get; } + private const double fade_duration = 300; private readonly OsuScrollContainer scrollContainer; @@ -48,8 +50,6 @@ namespace osu.Game.Online.Leaderboards private APIRequest getScoresRequest; private ScheduledDelegate getScoresRequestCallback; - protected abstract bool IsOnlineScope { get; } - [Resolved(CanBeNull = true)] private IAPIProvider api { get; set; } @@ -62,49 +62,9 @@ namespace osu.Game.Online.Leaderboards get => scores; protected set { - Debug.Assert(ThreadSafety.IsUpdateThread); - scores = value; - scrollFlow?.FadeOut(fade_duration, Easing.OutQuint).Expire(); - scrollFlow = null; - - showScoresCancellationSource?.Cancel(); - - if (scores?.Any() != true) - { - loading.Hide(); - return; - } - - // ensure placeholder is hidden when displaying scores - PlaceholderState = PlaceholderState.Successful; - - var scoreFlow = new FillFlowContainer - { - RelativeSizeAxes = Axes.X, - AutoSizeAxes = Axes.Y, - Spacing = new Vector2(0f, 5f), - Padding = new MarginPadding { Top = 10, Bottom = 5 }, - ChildrenEnumerable = scores.Select((s, index) => CreateDrawableScore(s, index + 1)) - }; - - // schedule because we may not be loaded yet (LoadComponentAsync complains). - LoadComponentAsync(scoreFlow, _ => - { - scrollContainer.Add(scrollFlow = scoreFlow); - - int i = 0; - - foreach (var s in scrollFlow.Children) - { - using (s.BeginDelayedSequence(i++ * 50)) - s.Show(); - } - - scrollContainer.ScrollToStart(false); - loading.Hide(); - }, (showScoresCancellationSource = new CancellationTokenSource()).Token); + updateScoresDrawables(); } } @@ -324,6 +284,51 @@ namespace osu.Game.Online.Leaderboards } } + private void updateScoresDrawables() + { + Debug.Assert(ThreadSafety.IsUpdateThread); + + scrollFlow?.FadeOut(fade_duration, Easing.OutQuint).Expire(); + scrollFlow = null; + + showScoresCancellationSource?.Cancel(); + + if (scores?.Any() != true) + { + loading.Hide(); + return; + } + + // ensure placeholder is hidden when displaying scores + PlaceholderState = PlaceholderState.Successful; + + var scoreFlow = new FillFlowContainer + { + RelativeSizeAxes = Axes.X, + AutoSizeAxes = Axes.Y, + Spacing = new Vector2(0f, 5f), + Padding = new MarginPadding { Top = 10, Bottom = 5 }, + ChildrenEnumerable = scores.Select((s, index) => CreateDrawableScore(s, index + 1)) + }; + + // schedule because we may not be loaded yet (LoadComponentAsync complains). + LoadComponentAsync(scoreFlow, _ => + { + scrollContainer.Add(scrollFlow = scoreFlow); + + int i = 0; + + foreach (var s in scrollFlow.Children) + { + using (s.BeginDelayedSequence(i++ * 50)) + s.Show(); + } + + scrollContainer.ScrollToStart(false); + loading.Hide(); + }, (showScoresCancellationSource = new CancellationTokenSource()).Token); + } + private void replacePlaceholder(Placeholder placeholder) { if (placeholder != null && placeholder.Equals(currentPlaceholder)) From 3d59bab7c61cea8d71b362bd21f9bc183c3baf7d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 22:47:45 +0900 Subject: [PATCH 12/33] Remove fetch callback logic completely --- osu.Game/Online/Leaderboards/Leaderboard.cs | 25 +--- .../Match/Components/MatchLeaderboard.cs | 6 +- .../Select/Leaderboards/BeatmapLeaderboard.cs | 130 ++++++++---------- 3 files changed, 67 insertions(+), 94 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 0302568b34..eeb814b4cb 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -3,12 +3,11 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.Linq; using System.Threading; +using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; -using osu.Framework.Development; using osu.Framework.Extensions.Color4Extensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Colour; @@ -178,9 +177,9 @@ namespace osu.Game.Online.Leaderboards /// /// Performs a fetch/refresh of scores to be displayed. /// - /// A callback which should be called when fetching is completed. Scheduling is not required. /// An responsible for the fetch operation. This will be queued and performed automatically. - protected abstract APIRequest FetchScores(Action> scoresCallback); + [CanBeNull] + protected abstract APIRequest FetchScores(); protected abstract LeaderboardScore CreateDrawableScore(TScoreInfo model, int index); @@ -193,11 +192,7 @@ namespace osu.Game.Online.Leaderboards PlaceholderState = PlaceholderState.Retrieving; loading.Show(); - getScoresRequest = FetchScores(scores => getScoresRequestCallback = Schedule(() => - { - Scores = scores.ToArray(); - PlaceholderState = Scores.Any() ? PlaceholderState.Successful : PlaceholderState.NoScores; - })); + getScoresRequest = FetchScores(); if (getScoresRequest == null) return; @@ -240,11 +235,6 @@ namespace osu.Game.Online.Leaderboards get => placeholderState; set { - if (value != PlaceholderState.Successful) - { - Reset(); - } - if (value == placeholderState) return; @@ -284,10 +274,8 @@ namespace osu.Game.Online.Leaderboards } } - private void updateScoresDrawables() + private void updateScoresDrawables() => Scheduler.Add(() => { - Debug.Assert(ThreadSafety.IsUpdateThread); - scrollFlow?.FadeOut(fade_duration, Easing.OutQuint).Expire(); scrollFlow = null; @@ -296,6 +284,7 @@ namespace osu.Game.Online.Leaderboards if (scores?.Any() != true) { loading.Hide(); + PlaceholderState = PlaceholderState.NoScores; return; } @@ -327,7 +316,7 @@ namespace osu.Game.Online.Leaderboards scrollContainer.ScrollToStart(false); loading.Hide(); }, (showScoresCancellationSource = new CancellationTokenSource()).Token); - } + }, false); private void replacePlaceholder(Placeholder placeholder) { diff --git a/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs b/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs index e4ec3ac4a5..5b60f64160 100644 --- a/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs +++ b/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using System; -using System.Collections.Generic; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Game.Online.API; @@ -32,7 +30,7 @@ namespace osu.Game.Screens.OnlinePlay.Match.Components protected override bool IsOnlineScope => true; - protected override APIRequest FetchScores(Action> scoresCallback) + protected override APIRequest FetchScores() { if (roomId.Value == null) return null; @@ -41,7 +39,7 @@ namespace osu.Game.Screens.OnlinePlay.Match.Components req.Success += r => { - scoresCallback?.Invoke(r.Leaderboard); + Scores = r.Leaderboard; TopScore = r.UserScore; }; diff --git a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs index 02e4e162dd..898b894541 100644 --- a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs @@ -25,12 +25,6 @@ namespace osu.Game.Screens.Select.Leaderboards { public Action ScoreSelected; - [Resolved] - private RulesetStore rulesets { get; set; } - - [Resolved] - private RealmAccess realm { get; set; } - private BeatmapInfo beatmapInfo; public BeatmapInfo BeatmapInfo @@ -52,13 +46,7 @@ namespace osu.Game.Screens.Select.Leaderboards beatmapInfo = value; Scores = null; - if (IsOnlineScope) - RefetchScores(); - else - { - if (IsLoaded) - refreshRealmSubscription(); - } + RefetchScores(); } } @@ -93,6 +81,14 @@ namespace osu.Game.Screens.Select.Leaderboards [Resolved] private IAPIProvider api { get; set; } + [Resolved] + private RulesetStore rulesets { get; set; } + + [Resolved] + private RealmAccess realm { get; set; } + + private IDisposable scoreSubscription; + [BackgroundDependencyLoader] private void load() { @@ -104,33 +100,6 @@ namespace osu.Game.Screens.Select.Leaderboards }; } - protected override void LoadComplete() - { - base.LoadComplete(); - - refreshRealmSubscription(); - } - - private IDisposable scoreSubscription; - - private void refreshRealmSubscription() - { - scoreSubscription?.Dispose(); - scoreSubscription = null; - - if (beatmapInfo == null) - return; - - scoreSubscription = realm.RegisterForNotifications(r => - r.All() - .Filter($"{nameof(ScoreInfo.BeatmapInfo)}.{nameof(BeatmapInfo.ID)} = $0", beatmapInfo.ID), - (_, changes, ___) => - { - if (!IsOnlineScope) - RefetchScores(); - }); - } - protected override void Reset() { base.Reset(); @@ -141,13 +110,11 @@ namespace osu.Game.Screens.Select.Leaderboards private CancellationTokenSource loadCancellationSource; - protected override APIRequest FetchScores(Action> scoresCallback) + protected override APIRequest FetchScores() { loadCancellationSource?.Cancel(); loadCancellationSource = new CancellationTokenSource(); - var cancellationToken = loadCancellationSource.Token; - var fetchBeatmapInfo = BeatmapInfo; if (fetchBeatmapInfo == null) @@ -158,31 +125,7 @@ namespace osu.Game.Screens.Select.Leaderboards if (Scope == BeatmapLeaderboardScope.Local) { - realm.Run(r => - { - var scores = r.All() - .AsEnumerable() - // TODO: update to use a realm filter directly (or at least figure out the beatmap part to reduce scope). - .Where(s => !s.DeletePending && s.BeatmapInfo.ID == fetchBeatmapInfo.ID && s.Ruleset.ShortName == ruleset.Value.ShortName); - - if (filterMods && !mods.Value.Any()) - { - // we need to filter out all scores that have any mods to get all local nomod scores - scores = scores.Where(s => !s.Mods.Any()); - } - else if (filterMods) - { - // otherwise find all the scores that have *any* of the currently selected mods (similar to how web applies mod filters) - // we're creating and using a string list representation of selected mods so that it can be translated into the DB query itself - var selectedMods = mods.Value.Select(m => m.Acronym); - scores = scores.Where(s => s.Mods.Any(m => selectedMods.Contains(m.Acronym))); - } - - scores = scores.Detach(); - - scoreManager.OrderByTotalScoreAsync(scores.ToArray(), cancellationToken) - .ContinueWith(ordered => scoresCallback?.Invoke(ordered.GetResultSafely()), TaskContinuationOptions.OnlyOnRanToCompletion); - }); + subscribeToLocalScores(); return null; } @@ -217,13 +160,13 @@ namespace osu.Game.Screens.Select.Leaderboards req.Success += r => { - scoreManager.OrderByTotalScoreAsync(r.Scores.Select(s => s.CreateScoreInfo(rulesets, fetchBeatmapInfo)).ToArray(), cancellationToken) + scoreManager.OrderByTotalScoreAsync(r.Scores.Select(s => s.CreateScoreInfo(rulesets, fetchBeatmapInfo)).ToArray(), loadCancellationSource.Token) .ContinueWith(task => Schedule(() => { - if (cancellationToken.IsCancellationRequested) + if (loadCancellationSource.IsCancellationRequested) return; - scoresCallback?.Invoke(task.GetResultSafely()); + Scores = task.GetResultSafely(); TopScore = r.UserScore?.CreateScoreInfo(rulesets, fetchBeatmapInfo); }), TaskContinuationOptions.OnlyOnRanToCompletion); }; @@ -241,10 +184,53 @@ namespace osu.Game.Screens.Select.Leaderboards Action = () => ScoreSelected?.Invoke(model) }; + private void subscribeToLocalScores() + { + scoreSubscription?.Dispose(); + scoreSubscription = null; + + if (beatmapInfo == null) + return; + + scoreSubscription = realm.RegisterForNotifications(r => + r.All().Filter($"{nameof(ScoreInfo.BeatmapInfo)}.{nameof(BeatmapInfo.ID)} == $0" + + $" AND {nameof(ScoreInfo.Ruleset)}.{nameof(RulesetInfo.ShortName)} == $1" + + $" AND {nameof(ScoreInfo.DeletePending)} == false" + , beatmapInfo.ID, ruleset.Value.ShortName), localScoresChanged); + } + + private void localScoresChanged(IRealmCollection sender, ChangeSet changes, Exception exception) + { + if (IsOnlineScope) + return; + + var scores = sender.AsEnumerable(); + + if (filterMods && !mods.Value.Any()) + { + // we need to filter out all scores that have any mods to get all local nomod scores + scores = scores.Where(s => !s.Mods.Any()); + } + else if (filterMods) + { + // otherwise find all the scores that have *any* of the currently selected mods (similar to how web applies mod filters) + // we're creating and using a string list representation of selected mods so that it can be translated into the DB query itself + var selectedMods = mods.Value.Select(m => m.Acronym); + scores = scores.Where(s => s.Mods.Any(m => selectedMods.Contains(m.Acronym))); + } + + scores = scores.Detach(); + + scoreManager.OrderByTotalScoreAsync(scores.ToArray(), loadCancellationSource.Token) + .ContinueWith(ordered => + { + Scores = ordered.GetResultSafely(); + }, TaskContinuationOptions.OnlyOnRanToCompletion); + } + protected override void Dispose(bool isDisposing) { base.Dispose(isDisposing); - scoreSubscription?.Dispose(); } } From daea13f49158b5c464f20c4b395a85bd46206991 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 23:14:26 +0900 Subject: [PATCH 13/33] Simplify flow of cancellation token --- osu.Game/Online/Leaderboards/Leaderboard.cs | 42 ++++++------ .../Match/Components/MatchLeaderboard.cs | 6 +- .../Select/Leaderboards/BeatmapLeaderboard.cs | 65 +++++++++---------- 3 files changed, 55 insertions(+), 58 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index eeb814b4cb..0d430f4903 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Threading; using JetBrains.Annotations; @@ -13,7 +14,6 @@ using osu.Framework.Graphics; using osu.Framework.Graphics.Colour; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Sprites; -using osu.Framework.Threading; using osu.Game.Graphics.Containers; using osu.Game.Graphics.Cursor; using osu.Game.Graphics.UserInterface; @@ -44,10 +44,9 @@ namespace osu.Game.Online.Leaderboards private readonly LoadingSpinner loading; - private CancellationTokenSource showScoresCancellationSource; + private CancellationTokenSource currentFetchCancellationSource; - private APIRequest getScoresRequest; - private ScheduledDelegate getScoresRequestCallback; + private APIRequest fetchScoresRequest; [Resolved(CanBeNull = true)] private IAPIProvider api { get; set; } @@ -62,7 +61,6 @@ namespace osu.Game.Online.Leaderboards protected set { scores = value; - updateScoresDrawables(); } } @@ -177,9 +175,10 @@ namespace osu.Game.Online.Leaderboards /// /// Performs a fetch/refresh of scores to be displayed. /// + /// /// An responsible for the fetch operation. This will be queued and performed automatically. [CanBeNull] - protected abstract APIRequest FetchScores(); + protected abstract APIRequest FetchScores(CancellationToken cancellationToken); protected abstract LeaderboardScore CreateDrawableScore(TScoreInfo model, int index); @@ -187,37 +186,36 @@ namespace osu.Game.Online.Leaderboards private void refetchScores() { - cancelPendingWork(); + Reset(); PlaceholderState = PlaceholderState.Retrieving; loading.Show(); - getScoresRequest = FetchScores(); + currentFetchCancellationSource = new CancellationTokenSource(); - if (getScoresRequest == null) + fetchScoresRequest = FetchScores(currentFetchCancellationSource.Token); + + if (fetchScoresRequest == null) return; - getScoresRequest.Failure += e => getScoresRequestCallback = Schedule(() => + fetchScoresRequest.Failure += e => Schedule(() => { - if (e is OperationCanceledException) + if (e is OperationCanceledException || currentFetchCancellationSource.IsCancellationRequested) return; PlaceholderState = PlaceholderState.NetworkFailure; }); - api?.Queue(getScoresRequest); + api?.Queue(fetchScoresRequest); } private void cancelPendingWork() { - showScoresCancellationSource?.Cancel(); - showScoresCancellationSource = null; + currentFetchCancellationSource?.Cancel(); + currentFetchCancellationSource = null; - getScoresRequest?.Cancel(); - getScoresRequest = null; - - getScoresRequestCallback?.Cancel(); - getScoresRequestCallback = null; + fetchScoresRequest?.Cancel(); + fetchScoresRequest = null; } #region Placeholder handling @@ -279,8 +277,6 @@ namespace osu.Game.Online.Leaderboards scrollFlow?.FadeOut(fade_duration, Easing.OutQuint).Expire(); scrollFlow = null; - showScoresCancellationSource?.Cancel(); - if (scores?.Any() != true) { loading.Hide(); @@ -288,6 +284,8 @@ namespace osu.Game.Online.Leaderboards return; } + Debug.Assert(!currentFetchCancellationSource.IsCancellationRequested); + // ensure placeholder is hidden when displaying scores PlaceholderState = PlaceholderState.Successful; @@ -315,7 +313,7 @@ namespace osu.Game.Online.Leaderboards scrollContainer.ScrollToStart(false); loading.Hide(); - }, (showScoresCancellationSource = new CancellationTokenSource()).Token); + }, currentFetchCancellationSource.Token); }, false); private void replacePlaceholder(Placeholder placeholder) diff --git a/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs b/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs index 5b60f64160..1945899a11 100644 --- a/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs +++ b/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System.Threading; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Game.Online.API; @@ -30,7 +31,7 @@ namespace osu.Game.Screens.OnlinePlay.Match.Components protected override bool IsOnlineScope => true; - protected override APIRequest FetchScores() + protected override APIRequest FetchScores(CancellationToken cancellationToken) { if (roomId.Value == null) return null; @@ -39,6 +40,9 @@ namespace osu.Game.Screens.OnlinePlay.Match.Components req.Success += r => { + if (cancellationToken.IsCancellationRequested) + return; + Scores = r.Leaderboard; TopScore = r.UserScore; }; diff --git a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs index 898b894541..388395c9f9 100644 --- a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs @@ -108,13 +108,8 @@ namespace osu.Game.Screens.Select.Leaderboards protected override bool IsOnlineScope => Scope != BeatmapLeaderboardScope.Local; - private CancellationTokenSource loadCancellationSource; - - protected override APIRequest FetchScores() + protected override APIRequest FetchScores(CancellationToken cancellationToken) { - loadCancellationSource?.Cancel(); - loadCancellationSource = new CancellationTokenSource(); - var fetchBeatmapInfo = BeatmapInfo; if (fetchBeatmapInfo == null) @@ -125,7 +120,7 @@ namespace osu.Game.Screens.Select.Leaderboards if (Scope == BeatmapLeaderboardScope.Local) { - subscribeToLocalScores(); + subscribeToLocalScores(cancellationToken); return null; } @@ -160,10 +155,10 @@ namespace osu.Game.Screens.Select.Leaderboards req.Success += r => { - scoreManager.OrderByTotalScoreAsync(r.Scores.Select(s => s.CreateScoreInfo(rulesets, fetchBeatmapInfo)).ToArray(), loadCancellationSource.Token) + scoreManager.OrderByTotalScoreAsync(r.Scores.Select(s => s.CreateScoreInfo(rulesets, fetchBeatmapInfo)).ToArray(), cancellationToken) .ContinueWith(task => Schedule(() => { - if (loadCancellationSource.IsCancellationRequested) + if (cancellationToken.IsCancellationRequested) return; Scores = task.GetResultSafely(); @@ -184,7 +179,7 @@ namespace osu.Game.Screens.Select.Leaderboards Action = () => ScoreSelected?.Invoke(model) }; - private void subscribeToLocalScores() + private void subscribeToLocalScores(CancellationToken cancellationToken) { scoreSubscription?.Dispose(); scoreSubscription = null; @@ -197,35 +192,35 @@ namespace osu.Game.Screens.Select.Leaderboards + $" AND {nameof(ScoreInfo.Ruleset)}.{nameof(RulesetInfo.ShortName)} == $1" + $" AND {nameof(ScoreInfo.DeletePending)} == false" , beatmapInfo.ID, ruleset.Value.ShortName), localScoresChanged); - } - private void localScoresChanged(IRealmCollection sender, ChangeSet changes, Exception exception) - { - if (IsOnlineScope) - return; - - var scores = sender.AsEnumerable(); - - if (filterMods && !mods.Value.Any()) + void localScoresChanged(IRealmCollection sender, ChangeSet changes, Exception exception) { - // we need to filter out all scores that have any mods to get all local nomod scores - scores = scores.Where(s => !s.Mods.Any()); - } - else if (filterMods) - { - // otherwise find all the scores that have *any* of the currently selected mods (similar to how web applies mod filters) - // we're creating and using a string list representation of selected mods so that it can be translated into the DB query itself - var selectedMods = mods.Value.Select(m => m.Acronym); - scores = scores.Where(s => s.Mods.Any(m => selectedMods.Contains(m.Acronym))); - } + if (IsOnlineScope || cancellationToken.IsCancellationRequested) + return; - scores = scores.Detach(); + var scores = sender.AsEnumerable(); - scoreManager.OrderByTotalScoreAsync(scores.ToArray(), loadCancellationSource.Token) - .ContinueWith(ordered => - { - Scores = ordered.GetResultSafely(); - }, TaskContinuationOptions.OnlyOnRanToCompletion); + if (filterMods && !mods.Value.Any()) + { + // we need to filter out all scores that have any mods to get all local nomod scores + scores = scores.Where(s => !s.Mods.Any()); + } + else if (filterMods) + { + // otherwise find all the scores that have *any* of the currently selected mods (similar to how web applies mod filters) + // we're creating and using a string list representation of selected mods so that it can be translated into the DB query itself + var selectedMods = mods.Value.Select(m => m.Acronym); + scores = scores.Where(s => s.Mods.Any(m => selectedMods.Contains(m.Acronym))); + } + + scores = scores.Detach(); + + scoreManager.OrderByTotalScoreAsync(scores.ToArray(), cancellationToken) + .ContinueWith(ordered => + { + Scores = ordered.GetResultSafely(); + }, TaskContinuationOptions.OnlyOnRanToCompletion); + } } protected override void Dispose(bool isDisposing) From 0293d95f829f7b3596e472244e42ed040ee62717 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 28 Jan 2022 23:17:06 +0900 Subject: [PATCH 14/33] Simplify `IsOnlineScope` usage --- osu.Game/Online/Leaderboards/Leaderboard.cs | 3 +++ osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 0d430f4903..6c2f0d9425 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -32,6 +32,9 @@ namespace osu.Game.Online.Leaderboards /// The score model class. public abstract class Leaderboard : CompositeDrawable { + /// + /// Whether the current scope should refetch in response to changes in API connectivity state. + /// protected abstract bool IsOnlineScope { get; } private const double fade_duration = 300; diff --git a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs index 388395c9f9..e55bd56e62 100644 --- a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs @@ -195,7 +195,7 @@ namespace osu.Game.Screens.Select.Leaderboards void localScoresChanged(IRealmCollection sender, ChangeSet changes, Exception exception) { - if (IsOnlineScope || cancellationToken.IsCancellationRequested) + if (cancellationToken.IsCancellationRequested) return; var scores = sender.AsEnumerable(); From d0b74a91fba148d56fbd0ac586e5f6e1159e2a92 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 29 Jan 2022 02:12:36 +0900 Subject: [PATCH 15/33] Fix edge cases with score drawable loading --- osu.Game/Online/Leaderboards/Leaderboard.cs | 45 ++++++++++--------- .../Select/Leaderboards/BeatmapLeaderboard.cs | 1 - 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 6c2f0d9425..a1f07c469a 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.Linq; using System.Threading; using JetBrains.Annotations; @@ -43,11 +42,12 @@ namespace osu.Game.Online.Leaderboards private readonly Container placeholderContainer; private readonly UserTopScoreContainer topScoreContainer; - private FillFlowContainer scrollFlow; + private FillFlowContainer scoreFlowContainer; private readonly LoadingSpinner loading; private CancellationTokenSource currentFetchCancellationSource; + private CancellationTokenSource currentScoresAsyncLoadCancellationSource; private APIRequest fetchScoresRequest; @@ -64,7 +64,7 @@ namespace osu.Game.Online.Leaderboards protected set { scores = value; - updateScoresDrawables(); + Scheduler.AddOnce(updateScoresDrawables); } } @@ -239,6 +239,8 @@ namespace osu.Game.Online.Leaderboards if (value == placeholderState) return; + loading.Hide(); + switch (placeholderState = value) { case PlaceholderState.NetworkFailure: @@ -275,40 +277,41 @@ namespace osu.Game.Online.Leaderboards } } - private void updateScoresDrawables() => Scheduler.Add(() => + private void updateScoresDrawables() { - scrollFlow?.FadeOut(fade_duration, Easing.OutQuint).Expire(); - scrollFlow = null; + currentScoresAsyncLoadCancellationSource?.Cancel(); + currentScoresAsyncLoadCancellationSource = new CancellationTokenSource(); + + scoreFlowContainer? + .FadeOut(fade_duration, Easing.OutQuint) + .Expire(); + scoreFlowContainer = null; + + loading.Hide(); if (scores?.Any() != true) { - loading.Hide(); PlaceholderState = PlaceholderState.NoScores; return; } - Debug.Assert(!currentFetchCancellationSource.IsCancellationRequested); - // ensure placeholder is hidden when displaying scores PlaceholderState = PlaceholderState.Successful; - var scoreFlow = new FillFlowContainer + LoadComponentAsync(new FillFlowContainer { RelativeSizeAxes = Axes.X, AutoSizeAxes = Axes.Y, Spacing = new Vector2(0f, 5f), Padding = new MarginPadding { Top = 10, Bottom = 5 }, ChildrenEnumerable = scores.Select((s, index) => CreateDrawableScore(s, index + 1)) - }; - - // schedule because we may not be loaded yet (LoadComponentAsync complains). - LoadComponentAsync(scoreFlow, _ => + }, newFlow => { - scrollContainer.Add(scrollFlow = scoreFlow); + scrollContainer.Add(scoreFlowContainer = newFlow); int i = 0; - foreach (var s in scrollFlow.Children) + foreach (var s in scoreFlowContainer.Children) { using (s.BeginDelayedSequence(i++ * 50)) s.Show(); @@ -316,8 +319,8 @@ namespace osu.Game.Online.Leaderboards scrollContainer.ScrollToStart(false); loading.Hide(); - }, currentFetchCancellationSource.Token); - }, false); + }, currentScoresAsyncLoadCancellationSource.Token); + } private void replacePlaceholder(Placeholder placeholder) { @@ -354,12 +357,12 @@ namespace osu.Game.Online.Leaderboards if (!scrollContainer.IsScrolledToEnd()) fadeBottom -= LeaderboardScore.HEIGHT; - if (scrollFlow == null) + if (scoreFlowContainer == null) return; - foreach (var c in scrollFlow.Children) + foreach (var c in scoreFlowContainer.Children) { - float topY = c.ToSpaceOfOtherDrawable(Vector2.Zero, scrollFlow).Y; + float topY = c.ToSpaceOfOtherDrawable(Vector2.Zero, scoreFlowContainer).Y; float bottomY = topY + LeaderboardScore.HEIGHT; bool requireBottomFade = bottomY >= fadeBottom; diff --git a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs index e55bd56e62..31868624bb 100644 --- a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs @@ -121,7 +121,6 @@ namespace osu.Game.Screens.Select.Leaderboards if (Scope == BeatmapLeaderboardScope.Local) { subscribeToLocalScores(cancellationToken); - return null; } From 6f54f8ad7897cc9bfa31e3ad00660eff3efeef59 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 29 Jan 2022 12:18:34 +0900 Subject: [PATCH 16/33] Add more safety around `CancellationToken` usage --- osu.Game/Online/Leaderboards/Leaderboard.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index a1f07c469a..cc6d7a7b62 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -3,11 +3,13 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Threading; using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Framework.Development; using osu.Framework.Extensions.Color4Extensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Colour; @@ -189,6 +191,8 @@ namespace osu.Game.Online.Leaderboards private void refetchScores() { + Debug.Assert(ThreadSafety.IsUpdateThread); + Reset(); PlaceholderState = PlaceholderState.Retrieving; @@ -215,10 +219,8 @@ namespace osu.Game.Online.Leaderboards private void cancelPendingWork() { currentFetchCancellationSource?.Cancel(); - currentFetchCancellationSource = null; - + currentScoresAsyncLoadCancellationSource?.Cancel(); fetchScoresRequest?.Cancel(); - fetchScoresRequest = null; } #region Placeholder handling @@ -280,7 +282,6 @@ namespace osu.Game.Online.Leaderboards private void updateScoresDrawables() { currentScoresAsyncLoadCancellationSource?.Cancel(); - currentScoresAsyncLoadCancellationSource = new CancellationTokenSource(); scoreFlowContainer? .FadeOut(fade_duration, Easing.OutQuint) @@ -319,7 +320,7 @@ namespace osu.Game.Online.Leaderboards scrollContainer.ScrollToStart(false); loading.Hide(); - }, currentScoresAsyncLoadCancellationSource.Token); + }, (currentScoresAsyncLoadCancellationSource = new CancellationTokenSource()).Token); } private void replacePlaceholder(Placeholder placeholder) From a915b9cd3093bd2ede73c3d6ef9470ff12967dc1 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 29 Jan 2022 23:12:57 +0900 Subject: [PATCH 17/33] Fix occasional failures in `TestSceneDeleteLocalScore` --- .../Visual/UserInterface/TestSceneDeleteLocalScore.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs index 985ff6f251..da4cf9c6e3 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs @@ -135,11 +135,9 @@ namespace osu.Game.Tests.Visual.UserInterface [SetUpSteps] public void SetupSteps() { - // Ensure the leaderboard has finished async-loading drawables - AddUntilStep("wait for drawables", () => leaderboard.ChildrenOfType().Any()); - // Ensure the leaderboard items have finished showing up AddStep("finish transforms", () => leaderboard.FinishTransforms(true)); + AddUntilStep("wait for drawables", () => leaderboard.ChildrenOfType().Any()); } [Test] From 9861c50b33f8fa036e8bdf62ff1f646cab55c8ef Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 00:03:22 +0900 Subject: [PATCH 18/33] Remove pointless tests that no longer show anything valid --- .../Visual/SongSelect/TestSceneBeatmapLeaderboard.cs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs index 7ffab2eebc..b4b66e8afa 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs @@ -122,13 +122,6 @@ namespace osu.Game.Tests.Visual.SongSelect AddStep(@"None selected", () => leaderboard.SetRetrievalState(PlaceholderState.NoneSelected)); } - [Test] - public void TestBeatmapStates() - { - foreach (BeatmapOnlineStatus status in Enum.GetValues(typeof(BeatmapOnlineStatus))) - AddStep($"{status} beatmap", () => showBeatmapWithStatus(status)); - } - private void showPersonalBestWithNullPosition() { leaderboard.TopScore = new ScoreInfo From 51acf79935724e5786ecd1b515f07657071b65fd Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 02:29:51 +0900 Subject: [PATCH 19/33] Change test exposure to property instead of method --- .../Visual/SongSelect/TestSceneBeatmapLeaderboard.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs index b4b66e8afa..91ec1de3ad 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs @@ -101,7 +101,7 @@ namespace osu.Game.Tests.Visual.SongSelect public void TestGlobalScoresDisplay() { AddStep(@"Set scope", () => leaderboard.Scope = BeatmapLeaderboardScope.Global); - AddStep(@"New Scores", () => leaderboard.SetScores(generateSampleScores(null))); + AddStep(@"New Scores", () => leaderboard.Scores = generateSampleScores(null)); } [Test] @@ -417,7 +417,11 @@ namespace osu.Game.Tests.Visual.SongSelect PlaceholderState = state; } - public void SetScores(ICollection scores) => Scores = scores; + public new ICollection Scores + { + get => base.Scores; + set => base.Scores = value; + } } } } From 3d771c0fc7ed0f292ec28c0257ff80c122bcd058 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 02:34:31 +0900 Subject: [PATCH 20/33] Remove unnecessary `loading` hide call from `PlaceholderState_Set` and add more assertiveness --- osu.Game/Online/Leaderboards/Leaderboard.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index cc6d7a7b62..e20c8b5007 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -241,9 +241,11 @@ namespace osu.Game.Online.Leaderboards if (value == placeholderState) return; - loading.Hide(); + placeholderState = value; - switch (placeholderState = value) + Debug.Assert(placeholderState != PlaceholderState.Successful || scores?.Any() == true); + + switch (placeholderState) { case PlaceholderState.NetworkFailure: replacePlaceholder(new ClickablePlaceholder(@"Couldn't fetch scores!", FontAwesome.Solid.Sync) From d3cb910cf82cb208bf3ae6c3059e8b851107770e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 02:36:50 +0900 Subject: [PATCH 21/33] Convert inline math to not so inline to make operation more explicit --- osu.Game/Online/Leaderboards/Leaderboard.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index e20c8b5007..460869fa54 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -312,12 +312,14 @@ namespace osu.Game.Online.Leaderboards { scrollContainer.Add(scoreFlowContainer = newFlow); - int i = 0; + double delay = 0; foreach (var s in scoreFlowContainer.Children) { - using (s.BeginDelayedSequence(i++ * 50)) + using (s.BeginDelayedSequence(delay)) s.Show(); + + delay += 50; } scrollContainer.ScrollToStart(false); From d21464ea61b8efcf53eb867970b8bf4a5013a109 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 02:54:51 +0900 Subject: [PATCH 22/33] Fix assertions to work in both directions --- .../Visual/SongSelect/TestSceneBeatmapLeaderboard.cs | 1 + osu.Game/Online/Leaderboards/Leaderboard.cs | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs index 91ec1de3ad..31b7b9fa8d 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs @@ -414,6 +414,7 @@ namespace osu.Game.Tests.Visual.SongSelect { public void SetRetrievalState(PlaceholderState state) { + Scores = null; PlaceholderState = state; } diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 460869fa54..7b596d8381 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -243,11 +243,10 @@ namespace osu.Game.Online.Leaderboards placeholderState = value; - Debug.Assert(placeholderState != PlaceholderState.Successful || scores?.Any() == true); - switch (placeholderState) { case PlaceholderState.NetworkFailure: + Debug.Assert(scores?.Any() != true); replacePlaceholder(new ClickablePlaceholder(@"Couldn't fetch scores!", FontAwesome.Solid.Sync) { Action = RefetchScores @@ -255,26 +254,32 @@ namespace osu.Game.Online.Leaderboards break; case PlaceholderState.NoneSelected: + Debug.Assert(scores?.Any() != true); replacePlaceholder(new MessagePlaceholder(@"Please select a beatmap!")); break; case PlaceholderState.Unavailable: + Debug.Assert(scores?.Any() != true); replacePlaceholder(new MessagePlaceholder(@"Leaderboards are not available for this beatmap!")); break; case PlaceholderState.NoScores: + Debug.Assert(scores?.Any() != true); replacePlaceholder(new MessagePlaceholder(@"No records yet!")); break; case PlaceholderState.NotLoggedIn: + Debug.Assert(scores?.Any() != true); replacePlaceholder(new LoginPlaceholder(@"Please sign in to view online leaderboards!")); break; case PlaceholderState.NotSupporter: + Debug.Assert(scores?.Any() != true); replacePlaceholder(new MessagePlaceholder(@"Please invest in an osu!supporter tag to view this leaderboard!")); break; default: + Debug.Assert(scores?.Any() == true); replacePlaceholder(null); break; } From 9b573fbc2bdc044d0702179fa9b94a82a0fe0648 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 02:58:53 +0900 Subject: [PATCH 23/33] Add missing entries to `switch` statement and guard against out of range --- osu.Game/Online/Leaderboards/Leaderboard.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 7b596d8381..4134046320 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -278,10 +278,18 @@ namespace osu.Game.Online.Leaderboards replacePlaceholder(new MessagePlaceholder(@"Please invest in an osu!supporter tag to view this leaderboard!")); break; - default: + case PlaceholderState.Retrieving: + Debug.Assert(scores?.Any() != true); + replacePlaceholder(null); + break; + + case PlaceholderState.Successful: Debug.Assert(scores?.Any() == true); replacePlaceholder(null); break; + + default: + throw new ArgumentOutOfRangeException(); } } } From 06660ff96019000f7b8302d24fdd0cf28176f460 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 03:02:56 +0900 Subject: [PATCH 24/33] Fix null beatmap in test scene --- osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs index 31b7b9fa8d..77fe3be933 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs @@ -101,7 +101,7 @@ namespace osu.Game.Tests.Visual.SongSelect public void TestGlobalScoresDisplay() { AddStep(@"Set scope", () => leaderboard.Scope = BeatmapLeaderboardScope.Global); - AddStep(@"New Scores", () => leaderboard.Scores = generateSampleScores(null)); + AddStep(@"New Scores", () => leaderboard.Scores = generateSampleScores(new BeatmapInfo())); } [Test] From dad9cc931524f6627c507d5831c9cbd3a79ba6fb Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 03:06:29 +0900 Subject: [PATCH 25/33] Ensure `Reset`/`Scores_Set` run inline where possible --- osu.Game/Online/Leaderboards/Leaderboard.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 4134046320..0ad9f46130 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -66,7 +66,7 @@ namespace osu.Game.Online.Leaderboards protected set { scores = value; - Scheduler.AddOnce(updateScoresDrawables); + Scheduler.Add(updateScoresDrawables, false); } } From b434e29a7c4f4883cb55498678933dc44b4ec185 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 03:10:01 +0900 Subject: [PATCH 26/33] Move loading hide operation inside early return to ensure not hidden too early It should only be hidden after the async load completes. --- osu.Game/Online/Leaderboards/Leaderboard.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 0ad9f46130..8cf20ea8d1 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -303,10 +303,9 @@ namespace osu.Game.Online.Leaderboards .Expire(); scoreFlowContainer = null; - loading.Hide(); - if (scores?.Any() != true) { + loading.Hide(); PlaceholderState = PlaceholderState.NoScores; return; } From c401629dd84e8b91a9389fc85c16cc84f55aa013 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 03:37:57 +0900 Subject: [PATCH 27/33] Also refactor `placeholder` logic to make more sense --- .../SongSelect/TestSceneBeatmapLeaderboard.cs | 27 +-- osu.Game/Online/Leaderboards/Leaderboard.cs | 155 +++++++++--------- ...olderState.cs => LeaderboardErrorState.cs} | 5 +- .../Select/Leaderboards/BeatmapLeaderboard.cs | 15 +- 4 files changed, 86 insertions(+), 116 deletions(-) rename osu.Game/Online/Leaderboards/{PlaceholderState.cs => LeaderboardErrorState.cs} (82%) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs index 77fe3be933..969bafa98b 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs @@ -114,12 +114,12 @@ namespace osu.Game.Tests.Visual.SongSelect [Test] public void TestPlaceholderStates() { - AddStep(@"Empty Scores", () => leaderboard.SetRetrievalState(PlaceholderState.NoScores)); - AddStep(@"Network failure", () => leaderboard.SetRetrievalState(PlaceholderState.NetworkFailure)); - AddStep(@"No supporter", () => leaderboard.SetRetrievalState(PlaceholderState.NotSupporter)); - AddStep(@"Not logged in", () => leaderboard.SetRetrievalState(PlaceholderState.NotLoggedIn)); - AddStep(@"Unavailable", () => leaderboard.SetRetrievalState(PlaceholderState.Unavailable)); - AddStep(@"None selected", () => leaderboard.SetRetrievalState(PlaceholderState.NoneSelected)); + AddStep(@"Empty Scores", () => leaderboard.SetErrorState(LeaderboardErrorState.NoScores)); + AddStep(@"Network failure", () => leaderboard.SetErrorState(LeaderboardErrorState.NetworkFailure)); + AddStep(@"No supporter", () => leaderboard.SetErrorState(LeaderboardErrorState.NotSupporter)); + AddStep(@"Not logged in", () => leaderboard.SetErrorState(LeaderboardErrorState.NotLoggedIn)); + AddStep(@"Unavailable", () => leaderboard.SetErrorState(LeaderboardErrorState.Unavailable)); + AddStep(@"None selected", () => leaderboard.SetErrorState(LeaderboardErrorState.NoneSelected)); } private void showPersonalBestWithNullPosition() @@ -401,22 +401,9 @@ namespace osu.Game.Tests.Visual.SongSelect }; } - private void showBeatmapWithStatus(BeatmapOnlineStatus status) - { - leaderboard.BeatmapInfo = new BeatmapInfo - { - OnlineID = 1113057, - Status = status, - }; - } - private class FailableLeaderboard : BeatmapLeaderboard { - public void SetRetrievalState(PlaceholderState state) - { - Scores = null; - PlaceholderState = state; - } + public new void SetErrorState(LeaderboardErrorState errorState) => base.SetErrorState(errorState); public new ICollection Scores { diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 8cf20ea8d1..2e552f9774 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -53,6 +53,8 @@ namespace osu.Game.Online.Leaderboards private APIRequest fetchScoresRequest; + private LeaderboardErrorState errorState; + [Resolved(CanBeNull = true)] private IAPIProvider api { get; set; } @@ -169,14 +171,37 @@ namespace osu.Game.Online.Leaderboards RefetchScores(); } + /// + /// Perform a full refetch of scores using current criteria. + /// public void RefetchScores() => Scheduler.AddOnce(refetchScores); + /// + /// Reset the leaderboard into an empty state. + /// protected virtual void Reset() { cancelPendingWork(); Scores = null; } + /// + /// Call when a retrieval or display failure happened to show a relevant message to the user. + /// + /// The state to display. + protected void SetErrorState(LeaderboardErrorState errorState) + { + switch (errorState) + { + case LeaderboardErrorState.NoError: + throw new InvalidOperationException($"State {errorState} cannot be set by a leaderboard implementation."); + } + + Debug.Assert(scores?.Any() != true); + + setErrorState(errorState); + } + /// /// Performs a fetch/refresh of scores to be displayed. /// @@ -195,7 +220,7 @@ namespace osu.Game.Online.Leaderboards Reset(); - PlaceholderState = PlaceholderState.Retrieving; + setErrorState(LeaderboardErrorState.NoError); loading.Show(); currentFetchCancellationSource = new CancellationTokenSource(); @@ -210,7 +235,7 @@ namespace osu.Game.Online.Leaderboards if (e is OperationCanceledException || currentFetchCancellationSource.IsCancellationRequested) return; - PlaceholderState = PlaceholderState.NetworkFailure; + SetErrorState(LeaderboardErrorState.NetworkFailure); }); api?.Queue(fetchScoresRequest); @@ -223,77 +248,6 @@ namespace osu.Game.Online.Leaderboards fetchScoresRequest?.Cancel(); } - #region Placeholder handling - - private Placeholder currentPlaceholder; - - private PlaceholderState placeholderState; - - /// - /// Update the placeholder visibility. - /// Setting this to anything other than PlaceholderState.Successful will cancel all existing retrieval requests and hide scores. - /// - protected PlaceholderState PlaceholderState - { - get => placeholderState; - set - { - if (value == placeholderState) - return; - - placeholderState = value; - - switch (placeholderState) - { - case PlaceholderState.NetworkFailure: - Debug.Assert(scores?.Any() != true); - replacePlaceholder(new ClickablePlaceholder(@"Couldn't fetch scores!", FontAwesome.Solid.Sync) - { - Action = RefetchScores - }); - break; - - case PlaceholderState.NoneSelected: - Debug.Assert(scores?.Any() != true); - replacePlaceholder(new MessagePlaceholder(@"Please select a beatmap!")); - break; - - case PlaceholderState.Unavailable: - Debug.Assert(scores?.Any() != true); - replacePlaceholder(new MessagePlaceholder(@"Leaderboards are not available for this beatmap!")); - break; - - case PlaceholderState.NoScores: - Debug.Assert(scores?.Any() != true); - replacePlaceholder(new MessagePlaceholder(@"No records yet!")); - break; - - case PlaceholderState.NotLoggedIn: - Debug.Assert(scores?.Any() != true); - replacePlaceholder(new LoginPlaceholder(@"Please sign in to view online leaderboards!")); - break; - - case PlaceholderState.NotSupporter: - Debug.Assert(scores?.Any() != true); - replacePlaceholder(new MessagePlaceholder(@"Please invest in an osu!supporter tag to view this leaderboard!")); - break; - - case PlaceholderState.Retrieving: - Debug.Assert(scores?.Any() != true); - replacePlaceholder(null); - break; - - case PlaceholderState.Successful: - Debug.Assert(scores?.Any() == true); - replacePlaceholder(null); - break; - - default: - throw new ArgumentOutOfRangeException(); - } - } - } - private void updateScoresDrawables() { currentScoresAsyncLoadCancellationSource?.Cancel(); @@ -305,13 +259,14 @@ namespace osu.Game.Online.Leaderboards if (scores?.Any() != true) { + SetErrorState(LeaderboardErrorState.NoScores); loading.Hide(); - PlaceholderState = PlaceholderState.NoScores; return; } // ensure placeholder is hidden when displaying scores - PlaceholderState = PlaceholderState.Successful; + setErrorState(LeaderboardErrorState.NoError); + loading.Show(); LoadComponentAsync(new FillFlowContainer { @@ -339,25 +294,61 @@ namespace osu.Game.Online.Leaderboards }, (currentScoresAsyncLoadCancellationSource = new CancellationTokenSource()).Token); } - private void replacePlaceholder(Placeholder placeholder) + #region Placeholder handling + + private Placeholder placeholder; + + private void setErrorState(LeaderboardErrorState errorState) { - if (placeholder != null && placeholder.Equals(currentPlaceholder)) + if (errorState == this.errorState) return; - currentPlaceholder?.FadeOut(150, Easing.OutQuint).Expire(); + this.errorState = errorState; + + placeholder?.FadeOut(150, Easing.OutQuint).Expire(); + + placeholder = getPlaceholderFor(errorState); if (placeholder == null) - { - currentPlaceholder = null; return; - } placeholderContainer.Child = placeholder; placeholder.ScaleTo(0.8f).Then().ScaleTo(1, fade_duration * 3, Easing.OutQuint); placeholder.FadeInFromZero(fade_duration, Easing.OutQuint); + } - currentPlaceholder = placeholder; + private Placeholder getPlaceholderFor(LeaderboardErrorState errorState) + { + switch (errorState) + { + case LeaderboardErrorState.NetworkFailure: + return new ClickablePlaceholder(@"Couldn't fetch scores!", FontAwesome.Solid.Sync) + { + Action = RefetchScores + }; + + case LeaderboardErrorState.NoneSelected: + return new MessagePlaceholder(@"Please select a beatmap!"); + + case LeaderboardErrorState.Unavailable: + return new MessagePlaceholder(@"Leaderboards are not available for this beatmap!"); + + case LeaderboardErrorState.NoScores: + return new MessagePlaceholder(@"No records yet!"); + + case LeaderboardErrorState.NotLoggedIn: + return new LoginPlaceholder(@"Please sign in to view online leaderboards!"); + + case LeaderboardErrorState.NotSupporter: + return new MessagePlaceholder(@"Please invest in an osu!supporter tag to view this leaderboard!"); + + case LeaderboardErrorState.NoError: + return null; + + default: + throw new ArgumentOutOfRangeException(); + } } #endregion diff --git a/osu.Game/Online/Leaderboards/PlaceholderState.cs b/osu.Game/Online/Leaderboards/LeaderboardErrorState.cs similarity index 82% rename from osu.Game/Online/Leaderboards/PlaceholderState.cs rename to osu.Game/Online/Leaderboards/LeaderboardErrorState.cs index 297241fa73..17f47bb557 100644 --- a/osu.Game/Online/Leaderboards/PlaceholderState.cs +++ b/osu.Game/Online/Leaderboards/LeaderboardErrorState.cs @@ -3,10 +3,9 @@ namespace osu.Game.Online.Leaderboards { - public enum PlaceholderState + public enum LeaderboardErrorState { - Successful, - Retrieving, + NoError, NetworkFailure, Unavailable, NoneSelected, diff --git a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs index 31868624bb..b0ba830076 100644 --- a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs @@ -33,19 +33,12 @@ namespace osu.Game.Screens.Select.Leaderboards set { if (beatmapInfo == null && value == null) - { - // always null scores to ensure a correct initial display. - // see weird `scoresLoadedOnce` logic in base implementation. - Scores = null; return; - } if (beatmapInfo?.Equals(value) == true) return; beatmapInfo = value; - Scores = null; - RefetchScores(); } } @@ -114,7 +107,7 @@ namespace osu.Game.Screens.Select.Leaderboards if (fetchBeatmapInfo == null) { - PlaceholderState = PlaceholderState.NoneSelected; + SetErrorState(LeaderboardErrorState.NoneSelected); return null; } @@ -126,19 +119,19 @@ namespace osu.Game.Screens.Select.Leaderboards if (api?.IsLoggedIn != true) { - PlaceholderState = PlaceholderState.NotLoggedIn; + SetErrorState(LeaderboardErrorState.NotLoggedIn); return null; } if (fetchBeatmapInfo.OnlineID <= 0 || fetchBeatmapInfo.Status <= BeatmapOnlineStatus.Pending) { - PlaceholderState = PlaceholderState.Unavailable; + SetErrorState(LeaderboardErrorState.Unavailable); return null; } if (!api.LocalUser.Value.IsSupporter && (Scope != BeatmapLeaderboardScope.Global || filterMods)) { - PlaceholderState = PlaceholderState.NotSupporter; + SetErrorState(LeaderboardErrorState.NotSupporter); return null; } From acc1199addc044c2673516c1738586e16a058a87 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 16:16:00 +0900 Subject: [PATCH 28/33] Consolidate flows of `Set` operations, either result or error --- .../SongSelect/TestSceneBeatmapLeaderboard.cs | 17 ++--- osu.Game/Online/Leaderboards/Leaderboard.cs | 63 ++++++++----------- .../Match/Components/MatchLeaderboard.cs | 5 +- .../Select/Leaderboards/BeatmapLeaderboard.cs | 11 +--- 4 files changed, 37 insertions(+), 59 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs index 969bafa98b..e9a518b2fd 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs @@ -101,7 +101,7 @@ namespace osu.Game.Tests.Visual.SongSelect public void TestGlobalScoresDisplay() { AddStep(@"Set scope", () => leaderboard.Scope = BeatmapLeaderboardScope.Global); - AddStep(@"New Scores", () => leaderboard.Scores = generateSampleScores(new BeatmapInfo())); + AddStep(@"New Scores", () => leaderboard.SetScores(generateSampleScores(new BeatmapInfo()))); } [Test] @@ -124,7 +124,7 @@ namespace osu.Game.Tests.Visual.SongSelect private void showPersonalBestWithNullPosition() { - leaderboard.TopScore = new ScoreInfo + leaderboard.SetScores(leaderboard.Scores, new ScoreInfo { Rank = ScoreRank.XH, Accuracy = 1, @@ -142,12 +142,12 @@ namespace osu.Game.Tests.Visual.SongSelect FlagName = @"ES", }, }, - }; + }); } private void showPersonalBest() { - leaderboard.TopScore = new ScoreInfo + leaderboard.SetScores(leaderboard.Scores, new ScoreInfo { Position = 999, Rank = ScoreRank.XH, @@ -166,7 +166,7 @@ namespace osu.Game.Tests.Visual.SongSelect FlagName = @"ES", }, }, - }; + }); } private void loadMoreScores(Func beatmapInfo) @@ -404,12 +404,7 @@ namespace osu.Game.Tests.Visual.SongSelect private class FailableLeaderboard : BeatmapLeaderboard { public new void SetErrorState(LeaderboardErrorState errorState) => base.SetErrorState(errorState); - - public new ICollection Scores - { - get => base.Scores; - set => base.Scores = value; - } + public new void SetScores(IEnumerable scores, ScoreInfo userScore = default) => base.SetScores(scores, userScore); } } } diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 2e552f9774..9c46739443 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -33,6 +33,11 @@ namespace osu.Game.Online.Leaderboards /// The score model class. public abstract class Leaderboard : CompositeDrawable { + /// + /// The currently displayed scores. + /// + public IEnumerable Scores => scores; + /// /// Whether the current scope should refetch in response to changes in API connectivity state. /// @@ -42,7 +47,7 @@ namespace osu.Game.Online.Leaderboards private readonly OsuScrollContainer scrollContainer; private readonly Container placeholderContainer; - private readonly UserTopScoreContainer topScoreContainer; + private readonly UserTopScoreContainer userScoreContainer; private FillFlowContainer scoreFlowContainer; @@ -62,30 +67,6 @@ namespace osu.Game.Online.Leaderboards private ICollection scores; - public ICollection Scores - { - get => scores; - protected set - { - scores = value; - Scheduler.Add(updateScoresDrawables, false); - } - } - - public TScoreInfo TopScore - { - get => topScoreContainer.Score.Value; - set - { - topScoreContainer.Score.Value = value; - - if (value == null) - topScoreContainer.Hide(); - else - topScoreContainer.Show(); - } - } - private TScope scope; public TScope Scope @@ -133,7 +114,7 @@ namespace osu.Game.Online.Leaderboards { AutoSizeAxes = Axes.Y, RelativeSizeAxes = Axes.X, - Child = topScoreContainer = new UserTopScoreContainer(CreateDrawableTopScore) + Child = userScoreContainer = new UserTopScoreContainer(CreateDrawableTopScore) }, }, }, @@ -176,15 +157,6 @@ namespace osu.Game.Online.Leaderboards /// public void RefetchScores() => Scheduler.AddOnce(refetchScores); - /// - /// Reset the leaderboard into an empty state. - /// - protected virtual void Reset() - { - cancelPendingWork(); - Scores = null; - } - /// /// Call when a retrieval or display failure happened to show a relevant message to the user. /// @@ -202,6 +174,24 @@ namespace osu.Game.Online.Leaderboards setErrorState(errorState); } + /// + /// Call when score retrieval is ready to be displayed. + /// + /// The scores to display. + /// The user top score, if any. + protected void SetScores(IEnumerable scores, TScoreInfo userScore = default) + { + this.scores = scores?.ToList(); + userScoreContainer.Score.Value = userScore; + + if (userScore == null) + userScoreContainer.Hide(); + else + userScoreContainer.Show(); + + Scheduler.Add(updateScoresDrawables, false); + } + /// /// Performs a fetch/refresh of scores to be displayed. /// @@ -218,7 +208,8 @@ namespace osu.Game.Online.Leaderboards { Debug.Assert(ThreadSafety.IsUpdateThread); - Reset(); + cancelPendingWork(); + SetScores(null); setErrorState(LeaderboardErrorState.NoError); loading.Show(); diff --git a/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs b/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs index 1945899a11..ea7de917e2 100644 --- a/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs +++ b/osu.Game/Screens/OnlinePlay/Match/Components/MatchLeaderboard.cs @@ -24,7 +24,7 @@ namespace osu.Game.Screens.OnlinePlay.Match.Components if (id.NewValue == null) return; - Scores = null; + SetScores(null); RefetchScores(); }, true); } @@ -43,8 +43,7 @@ namespace osu.Game.Screens.OnlinePlay.Match.Components if (cancellationToken.IsCancellationRequested) return; - Scores = r.Leaderboard; - TopScore = r.UserScore; + SetScores(r.Leaderboard, r.UserScore); }; return req; diff --git a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs index b0ba830076..3521a2ef78 100644 --- a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs @@ -93,12 +93,6 @@ namespace osu.Game.Screens.Select.Leaderboards }; } - protected override void Reset() - { - base.Reset(); - TopScore = null; - } - protected override bool IsOnlineScope => Scope != BeatmapLeaderboardScope.Local; protected override APIRequest FetchScores(CancellationToken cancellationToken) @@ -153,8 +147,7 @@ namespace osu.Game.Screens.Select.Leaderboards if (cancellationToken.IsCancellationRequested) return; - Scores = task.GetResultSafely(); - TopScore = r.UserScore?.CreateScoreInfo(rulesets, fetchBeatmapInfo); + SetScores(task.GetResultSafely(), r.UserScore?.CreateScoreInfo(rulesets, fetchBeatmapInfo)); }), TaskContinuationOptions.OnlyOnRanToCompletion); }; @@ -210,7 +203,7 @@ namespace osu.Game.Screens.Select.Leaderboards scoreManager.OrderByTotalScoreAsync(scores.ToArray(), cancellationToken) .ContinueWith(ordered => { - Scores = ordered.GetResultSafely(); + SetScores(ordered.GetResultSafely()); }, TaskContinuationOptions.OnlyOnRanToCompletion); } } From 04dbb5d3c6cc9c8285fa5d95ac23621761e4a57c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 16:16:33 +0900 Subject: [PATCH 29/33] Disallow setting "NoScores" externally as it is handled internally --- .../Visual/SongSelect/TestSceneBeatmapLeaderboard.cs | 3 ++- osu.Game/Online/Leaderboards/Leaderboard.cs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs index e9a518b2fd..f91339e060 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs @@ -114,7 +114,8 @@ namespace osu.Game.Tests.Visual.SongSelect [Test] public void TestPlaceholderStates() { - AddStep(@"Empty Scores", () => leaderboard.SetErrorState(LeaderboardErrorState.NoScores)); + AddStep("ensure no scores displayed", () => leaderboard.SetScores(null)); + AddStep(@"Network failure", () => leaderboard.SetErrorState(LeaderboardErrorState.NetworkFailure)); AddStep(@"No supporter", () => leaderboard.SetErrorState(LeaderboardErrorState.NotSupporter)); AddStep(@"Not logged in", () => leaderboard.SetErrorState(LeaderboardErrorState.NotLoggedIn)); diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 9c46739443..9fb0fe0e3b 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -165,6 +165,7 @@ namespace osu.Game.Online.Leaderboards { switch (errorState) { + case LeaderboardErrorState.NoScores: case LeaderboardErrorState.NoError: throw new InvalidOperationException($"State {errorState} cannot be set by a leaderboard implementation."); } @@ -250,7 +251,7 @@ namespace osu.Game.Online.Leaderboards if (scores?.Any() != true) { - SetErrorState(LeaderboardErrorState.NoScores); + setErrorState(LeaderboardErrorState.NoScores); loading.Hide(); return; } From 1cec76df74df950db682ecde9de14ceeb9c1de8a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 30 Jan 2022 23:18:40 +0900 Subject: [PATCH 30/33] Fix weird reading xmldoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bartłomiej Dach --- osu.Game/Online/Leaderboards/Leaderboard.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 9fb0fe0e3b..e96b12bc5d 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -176,7 +176,7 @@ namespace osu.Game.Online.Leaderboards } /// - /// Call when score retrieval is ready to be displayed. + /// Call when retrieved scores are ready to be displayed. /// /// The scores to display. /// The user top score, if any. From f8939af5e6d165d97ad733d0b39634ef6c5b1723 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 31 Jan 2022 01:12:03 +0900 Subject: [PATCH 31/33] Track loading via state as well --- .../SongSelect/TestSceneBeatmapLeaderboard.cs | 12 ++-- osu.Game/Online/Leaderboards/Leaderboard.cs | 66 ++++++++++--------- ...boardErrorState.cs => LeaderboardState.cs} | 5 +- .../Select/Leaderboards/BeatmapLeaderboard.cs | 8 +-- 4 files changed, 48 insertions(+), 43 deletions(-) rename osu.Game/Online/Leaderboards/{LeaderboardErrorState.cs => LeaderboardState.cs} (82%) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs index f91339e060..48230ff9e9 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapLeaderboard.cs @@ -116,11 +116,11 @@ namespace osu.Game.Tests.Visual.SongSelect { AddStep("ensure no scores displayed", () => leaderboard.SetScores(null)); - AddStep(@"Network failure", () => leaderboard.SetErrorState(LeaderboardErrorState.NetworkFailure)); - AddStep(@"No supporter", () => leaderboard.SetErrorState(LeaderboardErrorState.NotSupporter)); - AddStep(@"Not logged in", () => leaderboard.SetErrorState(LeaderboardErrorState.NotLoggedIn)); - AddStep(@"Unavailable", () => leaderboard.SetErrorState(LeaderboardErrorState.Unavailable)); - AddStep(@"None selected", () => leaderboard.SetErrorState(LeaderboardErrorState.NoneSelected)); + AddStep(@"Network failure", () => leaderboard.SetErrorState(LeaderboardState.NetworkFailure)); + AddStep(@"No supporter", () => leaderboard.SetErrorState(LeaderboardState.NotSupporter)); + AddStep(@"Not logged in", () => leaderboard.SetErrorState(LeaderboardState.NotLoggedIn)); + AddStep(@"Unavailable", () => leaderboard.SetErrorState(LeaderboardState.Unavailable)); + AddStep(@"None selected", () => leaderboard.SetErrorState(LeaderboardState.NoneSelected)); } private void showPersonalBestWithNullPosition() @@ -404,7 +404,7 @@ namespace osu.Game.Tests.Visual.SongSelect private class FailableLeaderboard : BeatmapLeaderboard { - public new void SetErrorState(LeaderboardErrorState errorState) => base.SetErrorState(errorState); + public new void SetErrorState(LeaderboardState state) => base.SetErrorState(state); public new void SetScores(IEnumerable scores, ScoreInfo userScore = default) => base.SetScores(scores, userScore); } } diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index e96b12bc5d..7ac05fb0c0 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -58,7 +58,7 @@ namespace osu.Game.Online.Leaderboards private APIRequest fetchScoresRequest; - private LeaderboardErrorState errorState; + private LeaderboardState state; [Resolved(CanBeNull = true)] private IAPIProvider api { get; set; } @@ -160,19 +160,20 @@ namespace osu.Game.Online.Leaderboards /// /// Call when a retrieval or display failure happened to show a relevant message to the user. /// - /// The state to display. - protected void SetErrorState(LeaderboardErrorState errorState) + /// The state to display. + protected void SetErrorState(LeaderboardState state) { - switch (errorState) + switch (state) { - case LeaderboardErrorState.NoScores: - case LeaderboardErrorState.NoError: - throw new InvalidOperationException($"State {errorState} cannot be set by a leaderboard implementation."); + case LeaderboardState.NoScores: + case LeaderboardState.Retrieving: + case LeaderboardState.Success: + throw new InvalidOperationException($"State {state} cannot be set by a leaderboard implementation."); } Debug.Assert(scores?.Any() != true); - setErrorState(errorState); + setState(state); } /// @@ -212,8 +213,7 @@ namespace osu.Game.Online.Leaderboards cancelPendingWork(); SetScores(null); - setErrorState(LeaderboardErrorState.NoError); - loading.Show(); + setState(LeaderboardState.Retrieving); currentFetchCancellationSource = new CancellationTokenSource(); @@ -227,7 +227,7 @@ namespace osu.Game.Online.Leaderboards if (e is OperationCanceledException || currentFetchCancellationSource.IsCancellationRequested) return; - SetErrorState(LeaderboardErrorState.NetworkFailure); + SetErrorState(LeaderboardState.NetworkFailure); }); api?.Queue(fetchScoresRequest); @@ -251,15 +251,10 @@ namespace osu.Game.Online.Leaderboards if (scores?.Any() != true) { - setErrorState(LeaderboardErrorState.NoScores); - loading.Hide(); + setState(LeaderboardState.NoScores); return; } - // ensure placeholder is hidden when displaying scores - setErrorState(LeaderboardErrorState.NoError); - loading.Show(); - LoadComponentAsync(new FillFlowContainer { RelativeSizeAxes = Axes.X, @@ -269,6 +264,8 @@ namespace osu.Game.Online.Leaderboards ChildrenEnumerable = scores.Select((s, index) => CreateDrawableScore(s, index + 1)) }, newFlow => { + setState(LeaderboardState.Success); + scrollContainer.Add(scoreFlowContainer = newFlow); double delay = 0; @@ -282,7 +279,6 @@ namespace osu.Game.Online.Leaderboards } scrollContainer.ScrollToStart(false); - loading.Hide(); }, (currentScoresAsyncLoadCancellationSource = new CancellationTokenSource()).Token); } @@ -290,16 +286,21 @@ namespace osu.Game.Online.Leaderboards private Placeholder placeholder; - private void setErrorState(LeaderboardErrorState errorState) + private void setState(LeaderboardState state) { - if (errorState == this.errorState) + if (state == this.state) return; - this.errorState = errorState; + if (state == LeaderboardState.Retrieving) + loading.Show(); + else + loading.Hide(); + + this.state = state; placeholder?.FadeOut(150, Easing.OutQuint).Expire(); - placeholder = getPlaceholderFor(errorState); + placeholder = getPlaceholderFor(state); if (placeholder == null) return; @@ -310,32 +311,35 @@ namespace osu.Game.Online.Leaderboards placeholder.FadeInFromZero(fade_duration, Easing.OutQuint); } - private Placeholder getPlaceholderFor(LeaderboardErrorState errorState) + private Placeholder getPlaceholderFor(LeaderboardState state) { - switch (errorState) + switch (state) { - case LeaderboardErrorState.NetworkFailure: + case LeaderboardState.NetworkFailure: return new ClickablePlaceholder(@"Couldn't fetch scores!", FontAwesome.Solid.Sync) { Action = RefetchScores }; - case LeaderboardErrorState.NoneSelected: + case LeaderboardState.NoneSelected: return new MessagePlaceholder(@"Please select a beatmap!"); - case LeaderboardErrorState.Unavailable: + case LeaderboardState.Unavailable: return new MessagePlaceholder(@"Leaderboards are not available for this beatmap!"); - case LeaderboardErrorState.NoScores: + case LeaderboardState.NoScores: return new MessagePlaceholder(@"No records yet!"); - case LeaderboardErrorState.NotLoggedIn: + case LeaderboardState.NotLoggedIn: return new LoginPlaceholder(@"Please sign in to view online leaderboards!"); - case LeaderboardErrorState.NotSupporter: + case LeaderboardState.NotSupporter: return new MessagePlaceholder(@"Please invest in an osu!supporter tag to view this leaderboard!"); - case LeaderboardErrorState.NoError: + case LeaderboardState.Retrieving: + return null; + + case LeaderboardState.Success: return null; default: diff --git a/osu.Game/Online/Leaderboards/LeaderboardErrorState.cs b/osu.Game/Online/Leaderboards/LeaderboardState.cs similarity index 82% rename from osu.Game/Online/Leaderboards/LeaderboardErrorState.cs rename to osu.Game/Online/Leaderboards/LeaderboardState.cs index 17f47bb557..75e2c6e6db 100644 --- a/osu.Game/Online/Leaderboards/LeaderboardErrorState.cs +++ b/osu.Game/Online/Leaderboards/LeaderboardState.cs @@ -3,9 +3,10 @@ namespace osu.Game.Online.Leaderboards { - public enum LeaderboardErrorState + public enum LeaderboardState { - NoError, + Success, + Retrieving, NetworkFailure, Unavailable, NoneSelected, diff --git a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs index 3521a2ef78..48eb33cc05 100644 --- a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs @@ -101,7 +101,7 @@ namespace osu.Game.Screens.Select.Leaderboards if (fetchBeatmapInfo == null) { - SetErrorState(LeaderboardErrorState.NoneSelected); + SetErrorState(LeaderboardState.NoneSelected); return null; } @@ -113,19 +113,19 @@ namespace osu.Game.Screens.Select.Leaderboards if (api?.IsLoggedIn != true) { - SetErrorState(LeaderboardErrorState.NotLoggedIn); + SetErrorState(LeaderboardState.NotLoggedIn); return null; } if (fetchBeatmapInfo.OnlineID <= 0 || fetchBeatmapInfo.Status <= BeatmapOnlineStatus.Pending) { - SetErrorState(LeaderboardErrorState.Unavailable); + SetErrorState(LeaderboardState.Unavailable); return null; } if (!api.LocalUser.Value.IsSupporter && (Scope != BeatmapLeaderboardScope.Global || filterMods)) { - SetErrorState(LeaderboardErrorState.NotSupporter); + SetErrorState(LeaderboardState.NotSupporter); return null; } From 610eb9f6a45e8f3f762bac4f25a814c7169d8010 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 31 Jan 2022 13:45:49 +0900 Subject: [PATCH 32/33] Remove unnecessary container level --- osu.Game/Online/Leaderboards/Leaderboard.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/osu.Game/Online/Leaderboards/Leaderboard.cs b/osu.Game/Online/Leaderboards/Leaderboard.cs index 7ac05fb0c0..5dd3e46b4a 100644 --- a/osu.Game/Online/Leaderboards/Leaderboard.cs +++ b/osu.Game/Online/Leaderboards/Leaderboard.cs @@ -110,12 +110,7 @@ namespace osu.Game.Online.Leaderboards }, new Drawable[] { - new Container - { - AutoSizeAxes = Axes.Y, - RelativeSizeAxes = Axes.X, - Child = userScoreContainer = new UserTopScoreContainer(CreateDrawableTopScore) - }, + userScoreContainer = new UserTopScoreContainer(CreateDrawableTopScore) }, }, }, From 9c9fda84f3cf54073c7d15b62946251a3a27d328 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 31 Jan 2022 13:50:53 +0900 Subject: [PATCH 33/33] Add schedule and cancellation check to score ordering step --- osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs index 48eb33cc05..907a2c9bda 100644 --- a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs @@ -201,10 +201,13 @@ namespace osu.Game.Screens.Select.Leaderboards scores = scores.Detach(); scoreManager.OrderByTotalScoreAsync(scores.ToArray(), cancellationToken) - .ContinueWith(ordered => + .ContinueWith(ordered => Schedule(() => { + if (cancellationToken.IsCancellationRequested) + return; + SetScores(ordered.GetResultSafely()); - }, TaskContinuationOptions.OnlyOnRanToCompletion); + }), TaskContinuationOptions.OnlyOnRanToCompletion); } }