Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix results screen fetching scores twice when scrolled to edge #29059

Merged
merged 3 commits into from
Aug 5, 2024

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Jul 25, 2024

This one is not necessarily tied to daily challenge but it's something I noticed and it didn't feel well.

The fix here is, well, a schedule... Here's the order of how things happen:

  1. ResultsScreen.Update notices user is scrolled to beginning/end and fetches scores, blocking further fetches with lastFetchCompleted
  2. The scheduled logic in fetchScoresCallback is invoked, resetting lastFetchCompleted
  3. ResultsScreen.Update is called with lastFetchCompleted state reset, therefore incorrectly fetching more scores.

The fix is to schedule resetting lastFetchCompleted just one frame after adding the scores, so that the scroll container's state is updated before checking whether it's scrolled to beginning/end.

Before:

CleanShot.2024-07-25.at.09.16.46.mp4

After:

CleanShot.2024-07-25.at.09.17.48.mp4

@bdach
Copy link
Collaborator

bdach commented Jul 25, 2024

You know what I think of fixing things via schedules...

Is using UpdateAfterChildren() or something not an option here?

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

Let's go with this for now. I don't like using schedule to fix stuff like this either, but I think using UpdateAfterChildren() is going to be even more complex/uglier/error-prone.

At the end of the day, this is an o!f layout issue and not an easy one to solve at that.

@smoogipoo smoogipoo merged commit 136cdcf into ppy:master Aug 5, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants