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.
Kind of a big oversight this. In wanting to get the leaderboard
refactors to move forward I sort of didn't realise the fact that all of
the error handling related to online status and such in
`BeatmapLeaderboard` kind of... can't stay there...
It's also an all-or-nothing business too - moving this stuff can't
really be done only in part.
Not sure whether tests are warranted if it's more or less moving logic
across?
This is a prerequisite for supporting skinning of leaderboards.
- New `IGameplayLeaderboardProvider` and `IGameplayLeaderboardScore`
interfaces are introduced. They are strictly concerned with supplying
leaderboard data.
- Logic of managing display, which was previously jammed into the
inheritance hierarchy of `GameplayLeaderboard`, is now moved into
`IGameplayLeaderboardProvider` implementations. Solo play,
multiplayer, and multiplayer spectator get their own implementation of
the interface.
- The inheritance hierarchy of `GameplayLeaderboard` and per-player
overriding of the implementation of the gameplay leaderboard is gone.
Only one drawable class (renamed to `DrawableGameplayLeaderboard`) is
allowed to display the leaderboards, across all modes of play.
Saves having this defined in 20+ places. If we ever make any changes to
shear, it's 100% going to need to be applied to every usage (there will
never be a case of multiple different shears in the game).
Also fixes a mismatching definition in `ShearedNub`.
Resolves one part of
https://github.com/ppy/osu/discussions/32568#discussioncomment-12612928
A few caveats:
- Layout is slightly different than web intentionally. Web does things
that I think will be difficult to reproduce or just plain look bad in
client, such as:
- On web, the metadata info box has 200px min height and 300px max
height. I just hardcoded 300 units.
- On web, user tags and mapper tags are individually scrollable, and
the amount of space taken up by each is calculated in a way that
is - as far as I can tell - indeterminate, and probably influenced
by some flexbox magic. I just made the entire thing scrollable
instead.
- Because song select shares controls with the beatmap set overlay, now
song select says "Mapper Tags" in the header instead of just "Tags"
too. I think this is fine, because people asked for user tags to be
shown in song select too.
- Search query syntax lifted from
https://github.com/ppy/osu-web/pull/12047.
- Using hardcoded English strings for now, will update to the
translations after the next osu-resources localisations update.
RFC. Another attempt at this.
- Supersedes https://github.com/ppy/osu/pull/31881
- Supersedes / closes https://github.com/ppy/osu/pull/31355
- Closes https://github.com/ppy/osu/issues/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?)
Reported at https://osu.ppy.sh/community/forums/topics/2015478?n=1.
Would you believe it that this button that has been there for literal
years never did anything?
Implemented at a per-beatmap level. Also additionally added to context
menu (at @peppy's suggestion), and also copy reworded from "Delete from
unplayed" to "Mark as played" because double negation hurt my tiny
brain.
The intention here is to make things more testable going forward.
Specifically, to remove the "back-door" entrance into `BeatmapCarousel`
where `BeatmapSets` can be set by tests and bypas/block realm retrieval.