From f7d1809cb7a59d458ab652231aaa7f94ebcb59a4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 22 Apr 2025 19:00:45 +0900 Subject: [PATCH] Remove `LeaderboardManager` return value and simplify flow further The rationale for this change is that the return value was mostly useless, and at worst, misleading. When using `LeaderboardManager`, it's assumed that a consumer will bind to the global `Scores` list to ensure they receive updates for things like local score changes via the internal realm subscription. If one decides to instead use the return value of the task, it will be a static snapshot that potentially becomes stale in the future. I fell into this trap when refactoring the new leaderboard component (while attempting to assert correctness that the values we are displaying were in fact from the fetch operation we requested). In the interest of keeping things simple, removing the return value seems to be the best path forward. --- .../Online/Leaderboards/LeaderboardManager.cs | 50 +++++++++++++++---- osu.Game/OsuGame.cs | 7 +-- .../Select/Leaderboards/BeatmapLeaderboard.cs | 44 ++++++++-------- 3 files changed, 63 insertions(+), 38 deletions(-) diff --git a/osu.Game/Online/Leaderboards/LeaderboardManager.cs b/osu.Game/Online/Leaderboards/LeaderboardManager.cs index cd77a28893..75f2972f29 100644 --- a/osu.Game/Online/Leaderboards/LeaderboardManager.cs +++ b/osu.Game/Online/Leaderboards/LeaderboardManager.cs @@ -9,6 +9,7 @@ using System.Threading.Tasks; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Graphics; +using osu.Framework.Logging; using osu.Game.Beatmaps; using osu.Game.Database; using osu.Game.Extensions; @@ -43,10 +44,14 @@ namespace osu.Game.Online.Leaderboards [Resolved] private RulesetStore rulesets { get; set; } = null!; - public Task FetchWithCriteriaAsync(LeaderboardCriteria newCriteria) + /// + /// Fetch leaderboard content with the new criteria specified in the background. + /// On completion, will be updated with the results from this call (unless a more recent call with a different criteria has completed). + /// + public void FetchWithCriteria(LeaderboardCriteria newCriteria) { if (CurrentCriteria?.Equals(newCriteria) == true && lastFetchCompletionSource?.Task.IsFaulted == false) - return lastFetchCompletionSource?.Task ?? Task.FromResult(Scores.Value); + return; CurrentCriteria = newCriteria; localScoreSubscription?.Dispose(); @@ -55,7 +60,10 @@ namespace osu.Game.Online.Leaderboards scores.Value = null; if (newCriteria.Beatmap == null || newCriteria.Ruleset == null) - return Task.FromResult(scores.Value = LeaderboardScores.Failure(LeaderboardFailState.NoneSelected)); + { + scores.Value = LeaderboardScores.Failure(LeaderboardFailState.NoneSelected); + return; + } switch (newCriteria.Scope) { @@ -70,25 +78,40 @@ namespace osu.Game.Online.Leaderboards + $" AND {nameof(ScoreInfo.Ruleset)}.{nameof(RulesetInfo.ShortName)} == $1" + $" AND {nameof(ScoreInfo.DeletePending)} == false" , newCriteria.Beatmap.ID, newCriteria.Ruleset.ShortName), localScoresChanged); - return localFetchCompletionSource.Task; + return; } default: { if (!api.IsLoggedIn) - return Task.FromResult(scores.Value = LeaderboardScores.Failure(LeaderboardFailState.NotLoggedIn)); + { + scores.Value = LeaderboardScores.Failure(LeaderboardFailState.NotLoggedIn); + return; + } if (!newCriteria.Ruleset.IsLegacyRuleset()) - return Task.FromResult(scores.Value = LeaderboardScores.Failure(LeaderboardFailState.RulesetUnavailable)); + { + scores.Value = LeaderboardScores.Failure(LeaderboardFailState.RulesetUnavailable); + return; + } if (newCriteria.Beatmap.OnlineID <= 0 || newCriteria.Beatmap.Status <= BeatmapOnlineStatus.Pending) - return Task.FromResult(scores.Value = LeaderboardScores.Failure(LeaderboardFailState.BeatmapUnavailable)); + { + scores.Value = LeaderboardScores.Failure(LeaderboardFailState.BeatmapUnavailable); + return; + } if ((newCriteria.Scope.RequiresSupporter(newCriteria.ExactMods != null)) && !api.LocalUser.Value.IsSupporter) - return Task.FromResult(scores.Value = LeaderboardScores.Failure(LeaderboardFailState.NotSupporter)); + { + scores.Value = LeaderboardScores.Failure(LeaderboardFailState.NotSupporter); + return; + } if (newCriteria.Scope == BeatmapLeaderboardScope.Team && api.LocalUser.Value.Team == null) - return Task.FromResult(scores.Value = LeaderboardScores.Failure(LeaderboardFailState.NoTeam)); + { + scores.Value = LeaderboardScores.Failure(LeaderboardFailState.NoTeam); + return; + } var onlineFetchCompletionSource = new TaskCompletionSource(); lastFetchCompletionSource = onlineFetchCompletionSource; @@ -119,9 +142,14 @@ namespace osu.Game.Online.Leaderboards if (onlineFetchCompletionSource.TrySetResult(result)) scores.Value = result; }; - newRequest.Failure += _ => onlineFetchCompletionSource.TrySetResult(LeaderboardScores.Failure(LeaderboardFailState.NetworkFailure)); + newRequest.Failure += ex => + { + Logger.Log($@"Failed to fetch leaderboards when displaying results: {ex}", LoggingTarget.Network); + onlineFetchCompletionSource.TrySetResult(LeaderboardScores.Failure(LeaderboardFailState.NetworkFailure)); + }; + api.Queue(inFlightOnlineRequest = newRequest); - return onlineFetchCompletionSource.Task; + break; } } } diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs index 0c6a06a8fc..cbb2d44a9a 100644 --- a/osu.Game/OsuGame.cs +++ b/osu.Game/OsuGame.cs @@ -801,12 +801,7 @@ namespace osu.Game var newLeaderboard = currentLeaderboard != null ? currentLeaderboard with { Beatmap = databasedBeatmap, Ruleset = databasedScore.ScoreInfo.Ruleset } : new LeaderboardCriteria(databasedBeatmap, databasedScore.ScoreInfo.Ruleset, BeatmapLeaderboardScope.Global, null); - LeaderboardManager.FetchWithCriteriaAsync(newLeaderboard) - .ContinueWith(t => - { - if (t.Exception != null) - Logger.Log($@"Failed to fetch leaderboards when displaying results: {t.Exception}", LoggingTarget.Network); - }); + LeaderboardManager.FetchWithCriteria(newLeaderboard); } switch (presentType) diff --git a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs index 61abe3bd86..1c62499162 100644 --- a/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/BeatmapLeaderboard.cs @@ -62,7 +62,7 @@ namespace osu.Game.Screens.Select.Leaderboards } } - private readonly Bindable fetchedScores = new Bindable(); + private readonly IBindable fetchedScores = new Bindable(); [Resolved] private IBindable ruleset { get; set; } = null!; @@ -82,9 +82,10 @@ namespace osu.Game.Screens.Select.Leaderboards if (filterMods) RefetchScores(); }; - ((IBindable)fetchedScores).BindTo(leaderboardManager.Scores); } + private bool initialFetchComplete; + protected override bool IsOnlineScope => Scope != BeatmapLeaderboardScope.Local; protected override APIRequest? FetchScores(CancellationToken cancellationToken) @@ -96,30 +97,31 @@ namespace osu.Game.Screens.Select.Leaderboards if (fetchBeatmapInfo == null) return null; - leaderboardManager.FetchWithCriteriaAsync(new LeaderboardCriteria(fetchBeatmapInfo, fetchRuleset, Scope, filterMods ? mods.Value.ToArray() : null)) - .ContinueWith(t => - { - if (t.Exception != null && !t.IsCanceled) - { - Schedule(() => SetErrorState(LeaderboardState.NetworkFailure)); - return; - } + leaderboardManager.FetchWithCriteria(new LeaderboardCriteria(fetchBeatmapInfo, fetchRuleset, Scope, filterMods ? mods.Value.ToArray() : null)); - fetchedScores.UnbindEvents(); - fetchedScores.BindValueChanged(scores => - { - if (scores.NewValue == null) return; - - if (scores.NewValue.FailState == null) - Schedule(() => SetScores(scores.NewValue.TopScores, scores.NewValue.UserScore)); - else - Schedule(() => SetErrorState((LeaderboardState)scores.NewValue.FailState)); - }, true); - }, cancellationToken); + if (!initialFetchComplete) + { + // only bind this after the first fetch to avoid reading stale scores. + fetchedScores.BindTo(leaderboardManager.Scores); + fetchedScores.BindValueChanged(_ => updateScores(), true); + initialFetchComplete = true; + } return null; } + private void updateScores() + { + var scores = fetchedScores.Value; + + if (scores == null) return; + + if (scores.FailState == null) + Schedule(() => SetScores(scores.TopScores, scores.UserScore)); + else + Schedule(() => SetErrorState((LeaderboardState)scores.FailState)); + } + protected override LeaderboardScore CreateDrawableScore(ScoreInfo model, int index) => new LeaderboardScore(model, index, IsOnlineScope, Scope != BeatmapLeaderboardScope.Friend) { Action = () => ScoreSelected?.Invoke(model)