Ensuring Start completed before initializing Xamarin Forms #2674
Conversation
… StartAsync Ensure Application.Current is set whenever the forms application is referenced
MvvmCross/ViewModels/IMvxAppStart.cs
Outdated
| namespace MvvmCross.ViewModels | ||
| { | ||
| public interface IMvxAppStart | ||
| { | ||
| void Start(object hint = null); | ||
|
|
||
| Task WaitForStart(); |
There was a problem hiding this comment.
I would rather make void Start(object hint = null); a Task. That might be difficult to make async all the way, so maybe another idea is to create a IMvxFormsAppStart for now and make it a Task there or add WaitForStart there. Also we could cast it to make it available.
…start to complete with error handling
|
@martijn00 can you take another look - somehow in making changes I ended up re-surfacing the nullreferenceexception when calling LoadApplication for Xamarin Forms on Android. Any suggestions on why this might be happening? |
There was a problem hiding this comment.
I'm not really sure this is the best approach to solve the issue... We're plugging into the framework some async void methods in critical places. Wouldn't it be better to just mention that you shouldn't run any long operations at startup? Because the same recommendation applies to traditional Xamarin.
MvvmCross/ViewModels/MvxAppStart.cs
Outdated
| try { | ||
| await NavigationService.Navigate<TViewModel>(); | ||
| _startTaskNotifier = MvxNotifyTask.Create(async ()=> await NavigationService.Navigate<TViewModel>()); |
There was a problem hiding this comment.
The try/catch here won't work, as MvxNotifyTask doesn't re-throw exceptions. You can use the newly added onException callback for that.
| @@ -31,7 +31,6 @@ private MvxNotifyTask(Task task, Action<Exception> onException) | |||
| { | |||
| try | |||
| { | |||
| await Task.Yield(); | |||
There was a problem hiding this comment.
Why was this Task.Yield removed? It was added in purpose.
There was a problem hiding this comment.
Two things:
- my understanding is that calling Yield will cause an unnecessary latency in the task being invoked
- if it is there for a reason (eg best practice or some other guidance) we should add it back with an appropriate comment and/or reference
There was a problem hiding this comment.
Is the point of the Yield call to force the code to run asynchronously? If so, this should be an option that's exposed when creating the MvxNotifyTask, rather than being the default.
There was a problem hiding this comment.
It was added here #2642 (also I don't see any mention to this change on this PR other than the change in the source code).
You're right about why is it there. The problem is that if the code isn't run asynchronously, there is a (quite high) chance the exception will be thrown before MvxNotifyTask starts monitoring the task. This is highly unexpected when you are delegating all error handling responsibilities to the object.
I agree this adds a (very) small latency to all operations running inside of it, but MvxNotifyTask is not just common async/await.
| InitializeForms(bundle); | ||
| } | ||
|
|
||
| public virtual void InitializeForms(Bundle bundle) | ||
| protected virtual async Task StartSetup() |
There was a problem hiding this comment.
Maybe this method should be called something like RunAppStart, the Setup keyword is a bit confusing in this context
There was a problem hiding this comment.
Good call. Will fix
| @@ -48,6 +49,8 @@ public IMvxViewModel ViewModel | |||
| } | |||
| } | |||
|
|
|||
| private MvxNotifyTask _startTaskNotifier; | |||
There was a problem hiding this comment.
This member isn't used anywhere.
There was a problem hiding this comment.
Thanks. Will clean it up now that it’s no longer used
|
@nmilcoff The point of these changes are to ensure that exceptions aren't lost deep within mvx where developers can't intercept them. We also want to give developers a known point where they can run code after the first navigation has completed. Both of these scenarios are difficult to accomplish as the Start method is async void with no way of capturing exceptions, nor waiting for it to complete. |
| @@ -82,6 +82,8 @@ where type.IsConventional() | |||
| { | |||
| try | |||
| { | |||
| if (pair.Type.ContainsGenericParameters) continue; | |||
There was a problem hiding this comment.
It prevents exceptions being raised when the Type is a generic - In the playground forms droid sample it is throwing an exception.
MvvmCross/ViewModels/MvxAppStart.cs
Outdated
| try { | ||
| await NavigationService.Navigate<TViewModel>(); | ||
| NavigationService.Navigate<TViewModel>().Wait(); |
There was a problem hiding this comment.
I think using GetAwaiter().GetResult() is prefered over .Wait()
| return base.FinishedLaunching(app, options); | ||
| } | ||
|
|
||
| protected override MvxIosSetup CreateSetup(IMvxApplicationDelegate applicationDelegate, UIWindow window) |
There was a problem hiding this comment.
We should probably move this into the normal appdelegate as well.
| @@ -31,11 +33,45 @@ public override UIWindow Window | |||
|
|
|||
| public override bool FinishedLaunching(UIApplication uiApplication, NSDictionary launchOptions) | |||
| { | |||
| Window = new UIWindow(UIScreen.MainScreen.Bounds); | |||
|
|
|||
| UINavigationBar.Appearance.BarTintColor = UIColor.FromRGB(65, 105, 225); | |||
There was a problem hiding this comment.
no - I've removed these lines
| { | ||
| base.OnCreate(bundle); | ||
|
|
||
| InternalOnCreate(bundle); |
There was a problem hiding this comment.
InternalOnCreate sounds a bit weird as name
Make setup work for all platforms
| @@ -82,8 +82,7 @@ protected virtual void OnViewModelSet() | |||
|
|
|||
| protected override void AttachBaseContext(Context @base) | |||
| { | |||
| if (this is IMvxAndroidSplashScreenActivity) | |||
| { | |||
| if (this is IMvxAndroidSplashScreenActivity) { | |||
There was a problem hiding this comment.
{ should be placed in a new line
| _formsApplication = _formsApplication ?? CreateFormsApplication(); | ||
| _formsApplication = CreateFormsApplication(); | ||
| } | ||
| if (Application.Current != _formsApplication) { |
There was a problem hiding this comment.
{ should be placed in a new line
| @@ -84,8 +84,7 @@ public override void SetContentView(int layoutResId) | |||
|
|
|||
| protected override void AttachBaseContext(Context @base) | |||
| { | |||
| if (this is IMvxAndroidSplashScreenActivity) | |||
| { | |||
| if (this is IMvxAndroidSplashScreenActivity) { | |||
There was a problem hiding this comment.
{ should be placed in a new line
| @@ -53,8 +54,7 @@ protected Application FormsApplication | |||
| { | |||
| get | |||
| { | |||
| if (_formsApplication == null) | |||
| { | |||
| if (_formsApplication == null) { | |||
There was a problem hiding this comment.
{ should be placed in a new line
| public virtual void InitializeForms(Bundle bundle) | ||
| { | ||
| if (FormsApplication.MainPage != null) | ||
| { | ||
| if (!Xamarin.Forms.Forms.IsInitialized) { |
There was a problem hiding this comment.
{ should be placed in a new line
MvvmCross/Core/MvxSetupExtensions.cs
Outdated
| public static TSetup CreateSetup<TSetup>(Assembly assembly, params object[] parameters) where TSetup : MvxSetup | ||
| { | ||
| var setupType = FindSetupType<TSetup>(assembly); | ||
| if (setupType == null) { |
There was a problem hiding this comment.
{ should be placed in a new line
MvvmCross/Core/MvxSetupExtensions.cs
Outdated
| throw new MvxException("Could not find a Setup class for application"); | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
{ should be placed in a new line
| { | ||
| if (rootFrame.Content == null) { | ||
| Setup = CreateSetup(rootFrame, activationArgs, nameof(Suspend)); | ||
| if (RootFrame.Content == null) { |
There was a problem hiding this comment.
{ should be placed in a new line
MvvmCross/ViewModels/MvxAppStart.cs
Outdated
| { | ||
| // Check whether Start has commenced, and return if it has | ||
| if (Interlocked.CompareExchange(ref startHasCommenced, 1, 0) == 1) return; | ||
|
|
||
| if (hint != null) { |
There was a problem hiding this comment.
{ should be placed in a new line
MvvmCross/ViewModels/MvxAppStart.cs
Outdated
| if (hint != null) { | ||
| MvxLog.Instance.Trace("Hint ignored in default MvxAppStart"); | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
{ should be placed in a new line
|
|
||
| protected virtual void CreateSetup(IMvxApplicationDelegate applicationDelegate, UIWindow window) | ||
| { | ||
| var setupType = FindSetupType(); |
There was a problem hiding this comment.
We should not be doing this - reflection is slow. We can have this as a fallback but the need the ability for the setup class to be specified (see UWP)
There was a problem hiding this comment.
This is outdated code. Look at the whole PR.
… scanning of assemblies
Updated Android to support new setup logic
|
For the most part Setup was treated as a singleton from what I can see. Particularly on Android where it was wrapped in a singleton class to call initialise. I should have sometime over the weekend if any further changes are needed |
…ric class with same name
…hen generic class with same name" This reverts commit c65cdbe.
|
@martijn00 do you think this PR is ready, or are there other adjustments to be made? It'd be great to get this merged so we can get a beta out with these changes. I'll then start to work through the issues to see what else we can get resolved before v6 is done |
There was a problem hiding this comment.
I really like the new shape Setup has! I think this after all this it will be easier to make Mvx work on iOS extensions for example.
I left some comments around, please check them.
Also we'll need to update docs & some samples for all this before hitting stable
| public abstract class MvxAppCompatSetup<TApplication> : MvxAppCompatSetup | ||
| where TApplication : IMvxApplication, new() | ||
| { | ||
| protected override IMvxApplication CreateApp() => Mvx.IocConstruct<TApplication>(); |
There was a problem hiding this comment.
@nickrandolph are you sure this will work? At first glance looks like this won't be ready at this point
There was a problem hiding this comment.
This ran up ok for me on Android - this is late executed, so the IoC container has initialized by the time this is called.
| { | ||
| if (_setup == null) | ||
| _setup = CreateSetup(this, MainWindow); | ||
| return _setup; |
There was a problem hiding this comment.
Can we make Mac work the same way as Android and iOS? (use a InitializePlatform method instead of ctor arguments) It seems cleaner to me to unify all setup changes on the same PR
There was a problem hiding this comment.
Done - I've made the changes but can someone please test both Mac and Mac Forms as I don't have access to a Mac this weekend.
| get | ||
| { | ||
| if (_setup == null) | ||
| _setup = CreateSetup(this, Window); |
There was a problem hiding this comment.
Can we make tvOS work the same way as Android and iOS? (use a InitializePlatform method instead of ctor arguments) It seems cleaner to me to unify all setup changes on the same PR
There was a problem hiding this comment.
Done - as per mac, can someone please test to make sure my changes work
| get | ||
| { | ||
| if (_setup == null) | ||
| _setup = CreateSetup(Dispatcher, MainWindow); |
There was a problem hiding this comment.
Can we make WPF work the same way as Android and iOS? (use a InitializePlatform method instead of ctor arguments) It seems cleaner to me to unify all setup changes on the same PR
Changed setup initialize methods to allow for multiple calls
|
Ok, I think I've covered all the platforms with the updated setup structure. I'd be great if someone could test the iOS, iOS Forms, Mac, Mac Forms, TvOS builds |
|
This is a huge improvement for new developers starting on MvvmCross! We might need to fix up some more Activity's in Android but for now this will be good to get merged! |
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
bug fix
Start calls asynchronous navigation method. If this method invokes a method that takes some time to complete, it is possible for XF to crash because the page attempts to load XF before MainPage is set
🆕 What is the new behavior (if this is a feature change)?
Setup blocks after calling Start until the async navigation completes
💥 Does this PR introduce a breaking change?
No
🐛 Recommendations for testing
Run playground forms
📝 Links to relevant issues/docs
Related PR (#2665) - this PR should no longer be required after the changes in this PR which ensure the current application is set whenever the FormsApplicaiton property is accessed.
Related Issues (these need to be tested against these changes)
#2622
#2624
🤔 Checklist before submitting