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

View shown before ViewModel initialization complete #2478

Closed
2 of 7 tasks
mrtnkrstn opened this issue Dec 19, 2017 · 4 comments
Closed
2 of 7 tasks

View shown before ViewModel initialization complete #2478

mrtnkrstn opened this issue Dec 19, 2017 · 4 comments
Labels
s/deprecated Deprecated status

Comments

@mrtnkrstn
Copy link

Starting in v5.2, a View is now shown before its ViewModel's initialization is completed. One result is that, in earlier versions, showing a loader in the Initialize() method would show it in the View that you are navigation from. Now the loader is shown in the view you are navigating to.

It is easy to track down the code where this change has happened:

In v5.1.1 and earlier, MvxNavigationService in its Navigate() method called the relevant code in this order:

await viewModel.Initialize();
ViewDispatcher.ShowViewModel(request);

v5.2 does the following:

viewModel.Prepare();
ViewDispatcher.ShowViewModel(request);
await viewModel.Initialize().ConfigureAwait(false);

This is a breaking change. Is there a specific reason why this change was made?

I have fixed the issue by implementing my own NavigationService that extends MvxNavigationService and overrides the Navigate() methods, where I swop the logic around. However this is not ideal as this implementation would have to be upgraded whenever there is a new version of MvvmCross that affects this code/logic. Surely the status quo should rather be maintained?

Steps to reproduce 📜

  1. Create a project with latest version of MvvmCross

  2. Navigate to a ViewModel

  3. Notice that the View is shown before ViewModel initialization completes.

Expected behavior 🤔

ViewModel initialization should complete before View is shown. This was the behaviour before 5.2, and should be maintained.

Actual behavior 🐛

View is shown before its ViewModel initialization is complete. This is breaking behaviour.

Configuration 🔧

Version: 5.2

Platform:

  • 📱 iOS
  • 🤖 Android
  • 🏁 WPF
  • 🌎 UWP
  • 🍎 MacOS
  • 📺 tvOS
  • 🐒 Xamarin.Forms
@nmilcoff
Copy link
Contributor

I know there were some issues around that made this change necessary.

From my understanding it makes more sense to await the call after the view is shown.

However, starting with MvvmCross 5.5 (PR: #2359) Initialize is fired from MvxViewModelLoader, and it is platform agnostic. I recommend you to look at MvxViewModel.InitializeTask and the latest blog posts / documentation.

@nickrandolph
Copy link
Contributor

@mrtnkrstn is there a reason why you can't move your logic to the Prepare method?

@nmilcoff @martijn00 is there a reason why Prepare isn't async, to allow for async preparation of view model prior to navigation (ie displaying the new view)?

@mrtnkrstn
Copy link
Author

@nmilcoff thank you, I have had a look at InitializeTask, but the fact remains that the change made to the order of calling ShowViewModel() and Initialize() is a breaking change.

@nickrandolph my understanding is that Prepare() is intended for data/state set-up, while Initialize() is meant for data loading.

The main issue is that the View is shown before all the data has been retrieved, so the screen is shown as it is being assembled. Our app was created on the understanding that the View is only shown once the data has been loaded (i.e. Initialize() has completed). The fact is that this change is a breaking change and affects how all our screens have been implemented.

Is there any reason why this change has been made? Can the code not be reverted to the original behaviour?

@nmilcoff
Copy link
Contributor

nmilcoff commented Mar 2, 2018

I apologize for the unexpected inconveniences, but we won't change this back. I'd say the old behavior was actually incorrect.

The way it works now it's how it always used to be. It makes much more sense to display a loading indicator on the ViewModel that is being initialized rather than the one which triggered the navigation.

@nmilcoff nmilcoff closed this as completed Mar 2, 2018
@nmilcoff nmilcoff added the s/deprecated Deprecated status label Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s/deprecated Deprecated status
Development

No branches or pull requests

3 participants