Skip to content

Conversation

@RLittlesII
Copy link
Member

What kind of change does this PR introduce?

@garyng provided a solution for fixing the RoutedViewHost Xamarin.Forms implementation.

What is the current behavior?

fixes: #2317

What is the new behavior?

The stack values do not get distorted after the navigation pattern provided.

What might this PR break?

Xamarin.Forms navigation

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:
I tested this in a sample and saw that the NavigationStack seems to be correct. I am going to look into creating some unit tests for this behavior so we can make sure it continues to function as expected.

@RLittlesII RLittlesII requested a review from a team January 26, 2020 19:51
@codecov
Copy link

codecov bot commented Jan 26, 2020

Codecov Report

Merging #2331 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2331   +/-   ##
=======================================
  Coverage   54.73%   54.73%           
=======================================
  Files         113      113           
  Lines        4334     4334           
  Branches      660      660           
=======================================
  Hits         2372     2372           
  Misses       1797     1797           
  Partials      165      165

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcb2692...877ddce. Read the comment docs.

@garyng
Copy link
Contributor

garyng commented Jan 26, 2020

@RLittlesII Is your original fix of adding .ObserveOn(Router.Scheduler) still required? After thinking about it, the navigation changed notification might fire on any thread, and Pop*Async needs to be on the UI thread right?

I am not sure what is the default value for Router.Scheduler though...

@RLittlesII
Copy link
Member Author

@garyng I am not sure if its required. I honestly thought my original fix resolved it. Default for Router.Scheduler is RxApp.MainThreadScheduler but any consumer can override that I believe.

That was my thought that the notifications were not returning to the UI thread thus causing the issue.

@garyng
Copy link
Contributor

garyng commented Jan 26, 2020

Ohh okay! I asked because I bumped into the issue where all the page-poppings are executed on other non-ui thread - it weirdly swallowed all the exceptions.

@RLittlesII
Copy link
Member Author

Well. The code changed in the PR works with the previous scenario we were testing. So you're thinking the Popped notifications should happen on the MainThreadScheduler?

@garyng
Copy link
Contributor

garyng commented Jan 26, 2020

I mean this chunk of code:

try
{
if (popToRoot)
{
await PopToRootAsync(true);
}
else if (!popToRootPending)
{
for (var i = 0; i < x.Delta; ++i)
{
await PopAsync(i == x.Delta - 1);
}
}

Should be executed on the main ui thread right? Since all the Pop*Asyncs are all ui operations.

@RLittlesII
Copy link
Member Author

Yes. Those are the Xamarin.Forms navigation methods.

@RLittlesII RLittlesII merged commit 5a0b5df into master Jan 27, 2020
@RLittlesII RLittlesII deleted the feature/2317 branch January 27, 2020 02:11
@lock lock bot locked and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Router.NavigateBack is still broken in some cases

4 participants