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

Bug when navigating and sending a Cmd at the same time #158

Closed
TimLariviere opened this issue Aug 14, 2018 · 3 comments
Closed

Bug when navigating and sending a Cmd at the same time #158

TimLariviere opened this issue Aug 14, 2018 · 3 comments
Labels
s/ready This issue is ready to be implemented t/bug Something isn't working as expected
Milestone

Comments

@TimLariviere
Copy link
Member

Applies to: iOS and Android

Description:
One common use case in apps is to provide some kind of action when back navigating.
As an example, when you click the "Save" button on an edit page, you want to save the fields, navigate back to the previous page and update the previous page to reflect the changes as well.

The best way I found to handle that is to:

  • [Edit Page] Click Save button
  • [Edit Page] Once saved, send a back navigation message
  • [Application] Upon updating for back navigation, send a message to the previous page to trigger update
  • [Previous page] Handle update

This works, but when using NavigationPage, this also triggers 2 rounds of PopAsync.
Because for some reason, Cmd keeps its own set of previous values and when updateNavigationPage is called for the Cmd, it sees the previous values and thinks it should pop the last page a second time, which is not correct.

Repro:
https://github.com/TimLariviere/EXF_NavigationAndCmd

  1. Start the repro
  2. Click "Go to next page"
  3. Click "Go to next page"
  4. Click "Go back and update"
  5. Not working... 2 pops then corrected by 1 push

Workaround:
I have found that if I don't do navigation and Cmd at the same time the bug doesn't trigger.
Thus we can store the Cmd in the model and wait for NavigationPopped to signal that the navigation has ended

  1. Start the repro
  2. Click "Go to next page"
  3. Click "Go to next page"
  4. Click "Go back and update workaround"
  5. Working! Only 1 pop
@dsyme dsyme added the t/bug Something isn't working as expected label Sep 5, 2018
@TimLariviere
Copy link
Member Author

TimLariviere commented Oct 29, 2018

Ok, so I investigated this issue.
The root cause is simple, but the fix will prove to be quite hard to write.

NavigationPage handles navigation through asynchronous methods: PushAsync, PopAsync and PopToRootAsync
Those methods update the pages count only when done (animation completed).

Currently Fabulous is fully synchronous and works on the main thread.
Those asynchronous methods are not awaited.
https://github.com/fsprojects/Fabulous/blob/b034fd8bf4877953d3dc0f5b1a81037ef5d29e8f/src/Fabulous.Core/ViewConverters.fs#L499

When a call to the update function in the sample app returns a new model that will remove a page, along with a Cmd, the updateNavigationPages function will be called 2 times.
Once for the initial message and once for the message stored in the Cmd.

Those calls are too close in time, not letting enough time for the first PopAsync to complete.
This results in Fabulous thinking that the 2nd time (for the Cmd message) the NavigationPage has not been updated (still all the pages)

I think the way to fix this is to properly await PopAsync in updateNavigationPages.
But this means one of two things :

  • synchronously await (RunSynchronously)
  • make updateNavigationPages asynchronous

Any attempt to synchronously await will result in a deadlock, as both Fabulous and Xamarin.Forms will need the main thread to complete PopAsync.

Making updateNavigationPages asynchronous also means to make its parent async, and so on.
The whole update workflow would need to be asynchronous.

@dsyme What do you think?

@TimLariviere
Copy link
Member Author

Planning it for a v2.0 release because it would need to make the whole update loop asynchronous to support awaiting PopAsync

@TimLariviere TimLariviere added s/ready This issue is ready to be implemented and removed a/controls labels Feb 18, 2020
@TimLariviere
Copy link
Member Author

v2 fixes this bug :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s/ready This issue is ready to be implemented t/bug Something isn't working as expected
Projects
None yet
Development

No branches or pull requests

2 participants