Remove fetching logic from leaderboard

[Wleter]

As was said in #27861 I tried to refactor fetching logic out of the Leaderboard into LeaderboardScoresProvider.

The flow is slightly altered, however as I tested it was fine, now LeaderboardScoresProvider have a LeaderboardState and Leaderboard responses to changes of this state.

I hope it is somewhat close to what peppy had in mind.

[Wleter]

I think this could now be removed, as while testing I didn't encounter such incorrect behaviour.

[bdach]

Absolutely delighted to see yet another XXL diff dropped in my lap, with instant merge conflicts to boot...

(To clarify: that was sarcasm. Please fix merge conflicts. And maybe split this if possible.)

[Wleter]

I added scores provider for gameplay leaderboard while spectating solo gameplay, maybe unnecessary for this PR, if so I can remove that sorry.

[Wleter]

Now leaderboard reacts to LeaderboardState change so it is now needed.

[Wleter]

Merge conflict was thankfully a 1 line of code. I can't really split up PR as most of it is just moving fetching logic of leaderboard to its own class. I can only remove 25 lines that are not really a refactor.

Sorry for XXL, but I wrote in #27861, that it can be large and can I really be the one to try and do it.

[bdach]

not feeling it 🙈

[bdach]

This looks like a very forced structure. Two generic type parameters, a whole bunch of members with no documentations that look very overfitted to the two implementations of this abstract that you have there.

I think this abstract class should not exist. I'm not sure this class is a good abstraction or even achieving much of code duplication reduction, it mainly looks to be obfuscating logic. Its responsibilities can and should be handled by its current derivative classes.

[bdach]

The fact that this class has both local leaderboard and online leaderboard retrieval looks similarly off to me. All of this is doing too much magic tracking for my liking. (This is an extremely similar criticism to #27128 (comment), by the way.)

The way I imagine this working is that you'd have separate "leaderboard providers" for separate "scopes":

• one for local leaderboard, with the realm subscription logic inside
• one for global leaderboard, with the online lookups
• one for country leaderboard

each with only a simple API to retrieve scores for the given scope and no tracking of external anything (api state, current ruleset, whatever - all of that stuff should be accepted by the provider as method parameters or similar). All of them should essentially wrap the lookup logic / be caches. At that point:

• Switching between leaderboard scopes is as simple as replacing the provider with another.
• Using the same scores in song select and in player is as simple as using one and the same provider in both places (or even passing it forward).

[Wleter]

Following your suggestions I prototyped from zero https://github.com/ppy/osu/compare/master...Wleter:osu:Leaderboad-refactor-v2?expand=1.

It is a prototype so I didn't really cared about some stuff and I didn't changed tests yet, however
if this is more acceptable solution and have a future should I close and open another PR or revert back the changes and push it to this PR?

I am also thinking about adding something like BeatmapLeaderboardWatcher that have bindable leaderboard scores and provider that can be switched. Then this BeatmapLeaderboardWatcher could be in the PlaySongSelect and bind to gameplayleaderboard instead of taking the binding from the BeatmapLeaderboard as it is now.

I also wanted to handle LeaderboardState from the ScoresProvider by returning some kind of result type Result<APIRequest, ErrorLeaderboardState> but I refrained from that for now. Would it be a good idea?

[bdach]

Following your suggestions I prototyped from zero https://github.com/ppy/osu/compare/master...Wleter:osu:Leaderboad-refactor-v2?expand=1.

I dunno this is still weird...

• BeatmapLeaderboardProvider.RequireSupporter() should not exist
• Why does the "leaderboard provider" return APIRequests but also have an OnScoresFetched event? And judging by source... the user is supposed to manually enqueue the returned APIRequest so that the callback fires? That is turboweird.

This "leaderboard provider" must have a simple API. Ask it for scores, get scores back. There should be no extra frills about "supporter required", or quirks like "having to enqueue the API request yourself". You're abstracting retrieval of scores. That's the only part that matters.

[Wleter]

Simplified it, at the cost of adding on failure delegate, however what I still don't really understand is why do we want provider for every scope.

I find it difficulty to work with such structure because when the scope is changed callbacks (or bindables if we were to use them) have to be added again. However I can't think of better way to do it.

In my opinion it makes sense with how currently Leaderboard, ResultsScreen, GameplayLeaderboard are constructed to have a more global component with callbacks(or bindables) to scores.

This provider could change scope by replacing those components, something like this:

public partial class BeatmapLeaderboardProvider : IBeatmapLeaderboardProvider
{
    public Action<LeaderboardScores<ScoreInfo>>? OnScoresFetched;

    public Action<Exception, CancellationToken>? OnFetchedFailure;

    public override BeatmapLeaderboardScope Scope => provider.Scope;

    private ScopedBeatmapLeaderboardScope provider;

    public BeatmapLeaderboardProvider(ScopedBeatmapLeaderboardScope scope)
    {
        provider = GetProviderForScope(scope);
        provider.OnFetchedFailure += OnFetchedFailure;
        provider.OnScoresFetched += OnScoresFetched;
    }

    public void SetScope(ScopedBeatmapLeaderboardScope scope)
    {
        if (Scope == scope)
            return;

        provider = GetProviderForScope(scope);
        provider.OnFetchedFailure += OnFetchedFailure;
        provider.OnScoresFetched += OnScoresFetched;
    }

    void FetchScores(...) => provider.FetchScores(...)
}

Which in my opinion is better and I would just use this everywhere, but maybe I am wrong.

Or maybe I just completely misunderstand your intentions...