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

Navigation RemovePageInstruction not working correctly #3047

Closed
lucacivale opened this issue Jan 11, 2024 · 7 comments · Fixed by #3053
Closed

Navigation RemovePageInstruction not working correctly #3047

lucacivale opened this issue Jan 11, 2024 · 7 comments · Fixed by #3053
Assignees
Milestone

Comments

@lucacivale
Copy link
Contributor

Description

If I have the navigation stack MainPage/PageA/PageB/PageC and want to go back to MainPage without popping each page I should use Prisms RemovePageInstruction and do a navigation on PageC like NavigateAsync("../../../"). This worked in Xamarin but it's currently broken. For reference see also #2818.

If you navigate with NavigateAsync("../../../") the app won't navigate and remains in a broken state.

If you instead do NavigateAsync("../../../PageA") the navigation works as expected and the new navigation stack is MainPage/PageA but a new PageA instance was created.

Am I missing something or is this a bug?

Repro project
Prism.Navigation.zip

Steps to Reproduce

See attached repro project

Platform with bug

.NET MAUI

Affected platforms

iOS, Android

Did you find any workaround?

If you instead do NavigateAsync("../../../PageA") the navigation works as expected and the new navigation stack is MainPage/PageA but a new PageA instance was created.

Relevant log output

No response

@bkaankose
Copy link

For me even "../NewPage" does not work. I want existing page to be removed from the stack, and new NewPage is pushed to the stack.

NavigationService simply ignores all 'remove from the stack' requests and just pushes a new NewPage on top of the current stack.

@lucacivale
Copy link
Contributor Author

For me even "../NewPage" does not work. I want existing page to be removed from the stack, and new NewPage is pushed to the stack.

NavigationService simply ignores all 'remove from the stack' requests and just pushes a new NewPage on top of the current stack.

In my repro that seems to work. Can you verify that?

@bkaankose
Copy link

bkaankose commented Jan 18, 2024

For me even "../NewPage" does not work. I want existing page to be removed from the stack, and new NewPage is pushed to the stack.
NavigationService simply ignores all 'remove from the stack' requests and just pushes a new NewPage on top of the current stack.

In my repro that seems to work. Can you verify that?

Yes it works on your repro. I guess my situation is different since I have a complex page construction.

1 - FlyoutPage/NavigationPage/TabbedPage (tabs generated dynamically using KnownNavigationParameters.CreateTab)
2 - One of the tabs inside TabbedPage navigates to PageA
3 - Then PageA goes to PageB by (../PageB).

What works in Xamarin is on PageB go back takes me directly to TabbedPage. In MAUI we go back to PageA, which should've been removed from the stack on step 3.

@Alex-Dobrynin
Copy link

Alex-Dobrynin commented Jan 19, 2024

@bkaankose @lucacivale

For me even "../NewPage" does not work. I want existing page to be removed from the stack, and new NewPage is pushed to the stack.

You should take into consideration that if your root is not NavigationPage then you don't have navigation stack. To have an ability do ../PageB when you have root PageA, your PageA should be the root of NavigationPage. In other words, you should have NavigationPage/PageA if you want to do ../PageB

Also you need to remember, that by the logic your NavigationPage cannot be empty (when it does not have root page). Because it does not have any sense in such implementation. And whenever you need an empty NavigationPage you have to think about your architecture and may be smth wrong with it

@bkaankose
Copy link

bkaankose commented Jan 19, 2024

@bkaankose @lucacivale

For me even "../NewPage" does not work. I want existing page to be removed from the stack, and new NewPage is pushed to the stack.

You should take into consideration that if your root is not NavigationPage then you don't have navigation stack. To have an ability do ../PageB when you have root PageA, your PageA should be the root of NavigationPage. In other words, you should have NavigationPage/PageA if you want to do ../PageB

Also you need to remember, that by the logic your NavigationPage cannot be empty (when it does not have root page). Because it does not have any sense in such implementation. And whenever you need an empty NavigationPage you have to think about your architecture and may be smth wrong with it

My navigation stack is inside the Flyout's content. So there is a stack, even though the app does not start with NavigationPage, but instead starts with FlyoutPage/NavigationPage.

This exact code used to work with Prism.Forms btw. This is a %100 migration work. This clearly shows that something changed in PageNavigationService and uri parser that it ignores this remove instruction segment.

I haven't find time to debug myself yet tough unfortunately. I'll check myself when I do.

@PrismLibrary PrismLibrary deleted a comment from Alex-Dobrynin Jan 19, 2024
@lucacivale
Copy link
Contributor Author

I debugged this issue and think I found the issue.
The navigation won't work because NavigateAsync and GoBackAsync consume a request in the SemaphoreSlim and therefore the GoBackAsnyc won't finish if the navigation tries to remove pages and go back.
As far as I understand the GoBackAsync doesn't need to wait for the Semaphore in this context. Can we split the GoBackAsync in two separate methods? The GoBackAsync will wait for the Semaphore and call the GoBackInternalAsync. GoBackInternalAsync processes the navigation. The RemoveAndGoBack could call the GoBackInternalAsync and the issue should be resolved and it's ensured that only one navigation is done simultaneously.
I could create a PR for this if I get some feedback from @dansiegel or @brianlagunas

@bkaankose couldn't repro your case. I created the Tab with the NavigationBuilder and the ../PageB worked properly.

@dansiegel
Copy link
Member

@lucacivale thanks for stepping up to help out. We would be more than happy to get your PR. A couple of keys to consider here...

  1. The SemaphoreSlim was added due to solve the issue that if you have something like a 'LoadingPage' where you might evaluate the state of the user and you might then navigate to the 'LoginPage' or the 'HomePage' from the OnNavigatedTo. This would fail because the Navigation to the 'LoadingPage' did not yet complete.
  2. We will require one or more tests as makes sense for the scenario(s) you've uncovered. It would be good to have them start with Issue3047_ so that we can always track the issue back to this GitHub Issue.
  3. You may want to start with adding the tests first to validate that they are failing now and that your changes fix the issue. If you have any additional questions let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants