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

Make Screen.Push asynchronous when necessary #1490

Merged
merged 18 commits into from Apr 20, 2018
Merged

Conversation

@peppy
Copy link
Member

@peppy peppy commented Mar 30, 2018

Also changes behaviour of LoadComponentAsync to make it more versatile.

Will be adding tests to Screen behaviour as it is quite important we get things right.

peppy added 3 commits Mar 30, 2018
This allows more complex logic patterns, such as preloading but also avoiding a synchronous load when later using the (potentially) preloaded component.
This ensures no stutters when pushing to an existing screen.
@@ -138,7 +138,7 @@ private void dispose(bool isDisposing)
internal Task LoadAsync(Game game, Drawable target, Action onLoaded = null)
{
if (loadState != LoadState.NotLoaded)
throw new InvalidOperationException($@"{nameof(LoadAsync)} may not be called more than once on the same Drawable.");

This comment has been minimized.

@Tom94

Tom94 Mar 30, 2018
Collaborator

What happens when loadTask is null due to the drawable having been loaded synchronously?

This comment has been minimized.

@peppy

peppy Mar 30, 2018
Author Member

What would you expect to happen here? Maybe return a completed task?

I'll add proper tests for this change and screens in general, please leave open for now.

This comment has been minimized.

@Tom94

Tom94 Mar 30, 2018
Collaborator

Perhaps returning a completed task makes most sense. Not sure how async/await normally work in such a situation, but I think we should try following the paradigms as closely as possible.

@peppy peppy added the blocked label Mar 30, 2018
@peppy peppy added this to the April 2018 milestone Mar 30, 2018
@peppy peppy added the area:UI label Mar 30, 2018
@smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Apr 2, 2018

As discussed - won't be merging this until tests are in-place.

@peppy
Copy link
Member Author

@peppy peppy commented Apr 17, 2018

Remaining test failures will be fixed once #1524 is merged.

peppy added 2 commits Apr 18, 2018
@peppy peppy requested a review from smoogipoo Apr 18, 2018
smoogipoo and others added 3 commits Apr 18, 2018
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Looks fine to me

@smoogipoo smoogipoo merged commit 1059792 into ppy:master Apr 20, 2018
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@peppy peppy deleted the peppy:async-push branch Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.