Extract leaderboard fetch logic from song select beatmap leaderboard drawable #32494
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…drawable RFC. Another attempt at this. - Supersedes ppy#31881 - Supersedes / closes ppy#31355 - Closes ppy#29861 This is a weird diff because I am feeling rather boxed in by all the constraints, namely that: - Leaderboard state should be global state - But the global state is essentially managed by song select and namely `BeatmapLeaderboard` itself. That's because trying to e.g. not have `BeatmapLeaderboard` pass the beatmap and the ruleset to the global leaderboard manager is worse, as it essentially introduces two parallel paths of execution that need to be somehow merged into one (as in I'd have to somehow sync `LeaderboardManager` responding to beatmap/ruleset changes with `BeatmapLeaderboard` which is inheritance hell) - Also local leaderboard fetching is data-push (as in the scores can change under the leaderboard manager), and online leaderboard fetching is data-pull (as in the scores do not change unless the leaderboard manager does something). Also online leaderboard fetching can fail. Which is why I need to still have the weird setup wherein there's a `FetchWithCriteriaAsync()` (because I need to be able to respond to online requests taking time, or failing), but also the `BeatmapLeaderboard` only uses the public `Scores` bindable to actually read the scores (because it needs to respond to new local scores arriving). - Another thing to think about here is what happens when a retrieval fails because e.g. the user requested friend leaderboards without having supporter. With how this diff is written, that special condition is handled to `BeatmapLeaderboard`, and `LeaderboardManager`'s state will remain as whatever it was before that scope change was requested, which may be considered good or it may not (I imagine it's better to show scores in gameplay than not in this case, but maybe I'm wrong?)
2 tasks
|
This looks better to me on a brief pass. I'll defer full review to @peppy. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RFC. Another attempt at this.
This is a weird diff because I am feeling rather boxed in by all the constraints, namely that:
BeatmapLeaderboarditself. That's because trying to e.g. not haveBeatmapLeaderboardpass the beatmap and the ruleset to the global leaderboard manager is worse, as it essentially introduces two parallel paths of execution that need to be somehow merged into one (as in I'd have to somehow syncLeaderboardManagerresponding to global beatmap/ruleset changes withBeatmapLeaderboardwhich is also doing the same viaRefetchScores()inheritance hell)FetchWithCriteriaAsync()(because I need to be able to respond to online requests taking time, or failing), but also theBeatmapLeaderboardonly uses the publicScoresbindable to actually read the scores (because it needs to respond to new local scores arriving).BeatmapLeaderboard, andLeaderboardManager's state will remain as whatever it was before that scope change was requested, which may be considered good or it may not (I imagine it's better to show any scores in gameplay than not in this case, but maybe I'm wrong?)