-
Notifications
You must be signed in to change notification settings - Fork 441
Provide an extension method to detect if popup was previous page #2767
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
base: main
Are you sure you want to change the base?
Provide an extension method to detect if popup was previous page #2767
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds an extension method to NavigatedToEventArgs
that allows developers to detect if the previous page was a Toolkit popup.
- Introduces
WasPreviousPageAToolkitPopup
extension. - Checks
args.PreviousPage is PopupPage
. - Adds new file under
Extensions
namespace.
Comments suppressed due to low confidence (2)
src/CommunityToolkit.Maui/Extensions/NavigatedToEventArgsExtensions.shared.cs:11
- The XML comment references but the method checks for PopupPage; update the cref to for consistency.
/// Determines whether the previous page was a Community Toolkit <see cref="Popup"/>.
src/CommunityToolkit.Maui/Extensions/NavigatedToEventArgsExtensions.shared.cs:15
- Consider adding unit tests covering the cases where the previous page is a PopupPage and when it's not to ensure the extension behaves correctly.
public static bool WasPreviousPageAToolkitPopup(this NavigatedToEventArgs args) => args.PreviousPage is PopupPage;
Thanks for the approval @pictos @jfversluis do you have any understanding on whether dotnet/maui#28384 will make it into .NET 10.0? |
Asked @PureWeen or maybe @jsuarezruiz knows |
While this seems useful, I'd need the same extension on private static void PreventPrematureNavigation(object sender, ShellNavigatingEventArgs e)
{
if (e.CanCancel)
{// If we can cancel the navigation, do so if it is not for a popup
if (e.Source == ShellNavigationSource.Push && e.Target.Location.OriginalString.Contains("Popup"))
{
// Don't cancel if the navigation is to a popup
RecordMsg("Splash Page: Allowed Navigation to " + e.Target.Location.OriginalString);
}
else if (e.Source == ShellNavigationSource.PopToRoot && e.Current.Location.OriginalString.Contains("Popup"))
{
// Don't cancel if the navigation is from a popup
RecordMsg("Splash Page: Allowed Navigation from " + e.Current.Location.OriginalString);
}
else
{
e.Cancel();
RecordMsg("Splash Page: Navigation canceled because it was not for a popup");
}
}
else
{
// If we can't cancel the navigation, just log it
RecordMsg("Splash Page: Navigation not canceled because CanCancel was false");
}
} |
@david-maw what's your use case for preventing navigation? I know we've discussed it somewhere but I don't recall the exact reason. |
Annoyingly, Play Store testing seems to be able to cause navigation even when there's no UI to do it (it gets to various pages before initialization has completed, which is a Bad Thing). I suspect it may be a timing window that I have not been able to duplicate, and I originally solved it by prohibiting all navigation while initialization was in process. That's no longer sufficient because popups now use navigation, so I need an explicit exception for them. |
Hmm yeah I'd try to get to the bottom of why it's happening rather than try to cancel things in this way. One way might be to collect analytics if it only happens in a released version. Can you reproduce in a released version without going through the play store? |
I have not been able to reproduce it outside Play Store testing which is why I suspect some sort of timing window. |
Of course once I had a workaround I went on to more pressing issues. Still, the need to create an exception for popup is reason enough to take another run at it. Who knows, in the intervening couple of years the problem, whatever it is, could have gone away and then there would be no need for the workaround. Also, pigs might fly. |
I should add that current turnaround time for Play Store submissions varies between an hour and several days and this only ever failed one time in ten or so, meaning it might be a while before I learn anything. |
Description of Change
This introduces the ability for developers to determine whether the previous page displayed in their app was a
PopupPage
as belowThe reason for the extension method over allowing a developer to just check the
PreviousPage
property is twofold:PreviousPage
is currentlyinternal
PopupPage
is alsointernal
I understand that internal access to MAUI will be removed from MAUI for .NET 10.0 but it also appears that this will be introduced dotnet/maui#28384. If that is the case the code in this PR will continue to function when .NET 10.0 ships.
Linked Issues
PR Checklist
approved
(bug) orChampioned
(feature/proposal)NavigatedToEventArgs
is internal so it would require some reflection to construct something to test.main
at time of PRAdditional information