Skip to content
This repository has been archived by the owner on Feb 16, 2023. It is now read-only.

Correct OnBackPressed return value #107

Closed

Conversation

nabond251
Copy link

@nabond251 nabond251 commented Aug 31, 2022

Seems the MAUI docs aren't abundantly clear here, but comparing with Xamarin's Page.OnBackButtonPressed Method -

Application developers can override this method to provide behavior when the back button is pressed.
...
Returns
Boolean
Whether or not the back navigation was handled by the override.

So it looks like what was actually intended in the PrismAppBuilder's handling of the back button is the reverse of the current implementation. Current behavior is when I am in a page on a NavigationPage stack and hit back, Android backs out of the entire app. Tested this fix in a workaround reconfiguration of OnBackPressed and pressing back behaves as expected.

@nabond251
Copy link
Author

Looking through the issues, I believe this fixes PrismLibrary/Prism#2815

@nabond251
Copy link
Author

@dansiegel you able to give this a look?

@nabond251
Copy link
Author

Ok, been using this some - seems you can't back out of the app with it.

@nabond251
Copy link
Author

Ok, hooked the source up to my app to walk through the sequence of events backing out of the app. Ultimately hit the following branch in PageNavigationService.cs:

            page = GetCurrentPage();
            if (IsRoot(GetPageFromWindow(), page))
                throw new NavigationException(NavigationException.CannotPopApplicationMainPage, page);

As of my current checkout, this is lines 139-141. Latest commit here gives PrismAppBuilder some copies of IsRoot and GetPageFromWindow to replicate this check and leave handling the back-press to Android if so. Not super happy with this, it works but it's not very DRY. OTOH not sure how to otherwise get at this functionality, and it's all of 33 lines copied or so. Happy to have someone suggest an alternative, but going with this for now.

@nabond251
Copy link
Author

Ach, we're still not done here. Current symptom is original, but potentially for a different reason - scenario is PrismNavigationPage > MainPage > child page; attempting to hardware back out of child page ought to go to MainPage, but instead it backs out of the entire app. This is because by the time OnBackPressed fires in PrismAppBuilder's configured lifecycle events, HandleNavigationPageGoBack has already called GoBackAsync so the current page in the lifecycle handler is already MainPage. So it seems we're double-dipping on handling the hardware back event, both in PrismNavigationPage.OnBackButtonPressed and in PrismAppBuilder's android.OnBackPressed handlers. My initial thoughts are that it might be best to handle it exclusively by the latter, since that's closer to the Android OS and that's the only place this actually matters..?

@dansiegel
Copy link
Member

@nabond251 sorry I haven't gotten to this yet. I've been focused more on some net7 stuff and will be to give this a review once I've addressed the net7 bits.

@nabond251
Copy link
Author

@dansiegel you're good man, I'm still puzzling on it. Thanks for getting back.

@nabond251
Copy link
Author

nabond251 commented Nov 5, 2022

Ok, tentatively stating this at least seems to work... Not sure how much I like it. There are a few things that make this whole animal difficult. First, we have two events that occur when the Android back button is hit, the MAUI cross-platform back event followed by the corresponding Android back lifecycle event. These should be unified, but they are separate. Second, and dual to the first, we have the one MAUI back event for both the hardware back key and the software back button. These should be distinct, but they are confused. Third, the Android back event handler is synchronous, but the Prism logic for determining if the back navigation can occur is async. Granted, this might not be strictly the case - the main logic is still just whether the page is root, which is a synchronous determination - but even then, this determination happens in the cross-platform logic, notifying the Android handler seems to be an async operation, and again it's a sync handler. The previous attempt of duplicating the logic didn't work, since by the time the Android handler rolled around the nav had already happened so the state was different.

All this being the case, this proposed solution effectively discards any use of the Android API in the case of normal back-navigation, and instead handles this more fully in the PrismNavigationPage - specifically the aspect of backing out of the app if at root. Some potential issues -

  • changed the public API for MvvmHelpers.HandleNavigationPageGoBack from Task to Task<INavigationResult> (so that PrismNavigationPage can use the result; if this is a problem, could have the latter call an internal version of the former instead)
  • PrismNavigationPage will now Application.Current.Quit the app if back navigation fails due to NavigationException.CannotPopApplicationMainPage; this is to mirror normal Android behavior of backing out of app
  • Kind of forces the user to do a little more heavy lifting to support backing out of the app if they are not using the PrismNavigationPage
  • Not quitting app for CannotGoBackFromRoot; sounds potentially relevant, but don't understand the use case enough to know

First and third items could maybe be dealt with by having a built-in observer of the navigation result that quits the application instead of making PrismNavigationPage do it, but I don't necessarily like this since it basically forces a response to this failure; whereas the way this is currently proposed, if they wanted a different response to back nav failure at root, they could use their custom nav page.

My current test is along the simple lines of

  • Start app; hardware back quits app
  • Start app; navigate to child page; hardware back navigates to MainPage; hardware back again quits app
  • Start app; navigate to child page; software back navigates to MainPage; hardware back again quits app
  • Haven't tested with IConfirmNavigationReturnedFalse or on iOS; don't have a Mac and test app isn't complex enough yet

I don't know; whole thing's a bit hairy, not much for it. I think I'm going to use this for now, but there very well might be a less-hairy more-elegant solution out there yet.

Comment on lines +31 to +38
try
{
var result = await MvvmHelpers.HandleNavigationPageGoBack(this).ConfigureAwait(false);
}
catch (NavigationException ex)
{
backingOut = ex.Message == NavigationException.CannotPopApplicationMainPage;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should never occur as the NavigationService is meant to catch any exception it encounters and wrap it in the INavigationResult. If we are aren't catching this for some reason it is a bug. Also this wouldn't be where we want to handle the logic to back out as the Navigation Stack may not contain a PrismNavigationPage as the root Page.

@dansiegel
Copy link
Member

I am closing this PR as we have moved the Prism.Maui code to the Prism repo and this repo is being archived. If you would like to pick this back up and submit a PR there we would love to try to get your update into the codebase!

@dansiegel dansiegel closed this Feb 16, 2023
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.

2 participants