Skip to content

Conversation

@bdach
Copy link
Collaborator

@bdach bdach commented Jul 29, 2025

This is the low effort attempt at fixing #23804 in line with the suggestion from #34112 (comment).

Why low effort?

  • It doesn't fix the fact that the same API request is done by two wedges independently despite them being right next to each other (which was the case before I started touching any of this).

    • I could add a caching layer to RealmPopulatingOnlineLookupSource but that would imply that this cache now has to be purged at some point. At what point? Good question. Probably on re-entering song select? Or maybe never (...with the exception of unit tests)?
    • Or I could pull out this online beatmap set fetch to a higher level and pass only the result of it.
  • It does a local realm refetch of the status. This is primarily required because of Fix clicking beatmap header causing leaderboard to refresh #33515 - the guard added there prevents the global working beatmap from being refetched with the updated BeatmapInfo via beatmap carousel / detached beatmap store.

  • It doesn't address Update button may not show under certain scenarios #20159 because that case concerns difficulties that are present online but missing locally. Resolving that would likely require a new database flag which would feed into

    public bool AllBeatmapsUpToDate => Beatmaps.All(b => b.MatchesOnlineVersion);

  • I've tested this manually because that's frankly easiest if you have realm studio working (open realm studio, select a beatmap in game, go to realm, change its status to something random, then go back to game, change to another set and change back - the status should revert to what it was). I can add automated tests but it's likely that will be much more annoying to set up.

Please advise how much of the above you'd see addressed and how. PRing as draft pending answers to these concerns.

bdach added 2 commits July 29, 2025 14:06
This sort of guarantees that the wedge displays the latest online status
of the map.
@bdach bdach self-assigned this Jul 29, 2025
@bdach bdach changed the title Update beatmap online status when the set is selected in song select Update beatmap online statuses when the set is selected in song select Jul 29, 2025
@peppy peppy self-requested a review August 4, 2025 09:35
@peppy
Copy link
Member

peppy commented Aug 4, 2025

I've read through the concerns and they all seem valid, but not blockers for getting this in in the current state. Nothing looks critically bad in the diff.

Is that something you could get behind?

@bdach
Copy link
Collaborator Author

bdach commented Aug 4, 2025

if you're ok with the caveats described in the OP I'm ok with undrafting this as-is.

@bdach bdach marked this pull request as ready for review August 4, 2025 09:57
Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work

@peppy peppy merged commit 5a9c8da into ppy:master Aug 4, 2025
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants