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

Update notification text when import is paused due to gameplay #30402

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

peppy
Copy link
Member

@peppy peppy commented Oct 23, 2024

Addresses #30388.

osu.2024-10-23.at.08.50.25.mp4

@peppy peppy added the area:import dealing with importing of data from stable or file formats label Oct 23, 2024
@tovc
Copy link

tovc commented Oct 23, 2024

more of a point regarding design, but the spinning circle doesn't change, which might, at a glance, imply that the import isn't paused. something like a yellow pause icon might be more appropriate?

actually, for that matter, there is already the yellow spinning circle design, might as well just reuse that


try
{
pauseIfNecessary(parameters, notification.CancellationToken);
Copy link
Contributor

@smoogipoo smoogipoo Oct 24, 2024

Choose a reason for hiding this comment

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

It's a little bit scary that this is the first entrypoint to the async context (there's no await prior to this line). For example if Import is ever called from the update/bdl thread, then this is going to potentially deadlock the game.

I suggest either await Task.Yield() before the pause, or wrapping the entire thing in a Task.Run().

Or.... if it can, use await Task.Delay() instead of Thread.Sleep(). Doesn't look like it can though.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a fair point. I started by duplicating the method and making an async version, but then ended up refactoring things a bit to not require this.

Of note, the Parallel.ForEachAsync change was required because without it, the previous Select loop would seem to cause all imports to get past the pause check and run up to the Import part which would mean they never actually pause.

The whole import async logic is a bit of a mess but as far as I can tell it's working as expected.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:import dealing with importing of data from stable or file formats size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants