MvxSetupSingleton optimizations / Fix SplashScreen initialization on Android #2746
Conversation
|
@kjeremy 👍 ! It's splitted now |
| { | ||
| public SplashScreen() | ||
| : base(Resource.Layout.SplashScreen) | ||
| { | ||
| } | ||
|
|
||
| protected override void OnResume() |
There was a problem hiding this comment.
Does this override need to be here?
MvvmCross/Core/MvxSetupSingleton.cs
Outdated
| } | ||
| else | ||
| { | ||
| MvxLog.Instance.Trace("EnsureInitialized has already been called so now waiting for completion"); | ||
| StartSetupInitialization(); |
There was a problem hiding this comment.
This beaks the behaviour of EnsureInitialized which should block until setup is completed. Currently this PR breaks more than it fixes.
There was a problem hiding this comment.
Sounds reasonable. We need to test PRs like #2713 or this one more before merging...
| @@ -86,5 +85,10 @@ protected virtual void TriggerFirstNavigate() | |||
| if (!startup.IsStarted) | |||
| startup.Start(); | |||
| } | |||
|
|
|||
| // do nothing on this override, as initial navigation is managed by TriggerFirstNavigate | |||
There was a problem hiding this comment.
Should we remove TriggerFirstNavigate?
…if cancelling monitor with wrong value
…tart only when not monitoring startup
|
Ok guys, thank you all for your feedback! Honestly I feel like this PR is actually fixing some issues that were introduced just before... We need to be more careful with merging. You'll notice I removed some calls to EnsureInitialized. This is because all Android Activities are already calling that here (that method is called from |
nmilcoff
dismissed stale reviews from kjeremy and nickrandolph
Changes were applied
|
I think this is probably one of the more complex parts of mvx to get right. Mainly because each platform has slightly different startup behaviour and we’re trying to coerce into a single pattern to make it easy for devs. |
|
A big fat high level comment on how all this works might be good.
…On Sat, Mar 31, 2018, 5:18 AM Martijn van Dijk ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2746 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEIBRD_tmFLMafni5wetJv9hdKY-rlQvks5tj0nfgaJpZM4TCDeu>
.
|
|
@martijn00 @nmilcoff I've done another pass through this PR and whilst there are some extreme edge cases where a race condition can occur, they're not easily resolvable without reworking the singleton significantly. I propose that this PR is accepted as is, and that we confirm that all platforms (inc Forms) are working well on startup. |
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Bug fix
🆕 What is the new behavior (if this is a feature change)?
Above is fixed, also made some optimizations and added some reference comments to MvxSetupSingleton.
💥 Does this PR introduce a breaking change?
Yes. TriggerFirstNavigate was removed.
🐛 Recommendations for testing
Test the Playground apps
📝 Links to relevant issues/docs
PR: #2713
🤔 Checklist before submitting