Fix additional scenarios with nested defined routes #13648

PureWeen · 2021-02-04T01:16:46Z

Description of Change

When defining routes like so

			Routing.RegisterRoute("page1/page2", typeof(ShellTestPage));
			Routing.RegisterRoute("page1/page2/page3", typeof(TestPage1));
			var shell = new TestShell(
				CreateShellItem(shellSectionRoute: "page1")
			);

Shell wasn't correctly collapsing page1 from the shellsectionroute into the global route because of the default route set on the Content

Issues Resolved

fixes [Bug] [Shell] [iOS] [Android] Shell Navigation (via RegisterRoute) behaves differently in 5.0 #13611

Platforms Affected

Core/XAML (all platforms)

Testing Procedure

Unit tests included to match reported issue
download the repro from the referenced issue and make sure nugets work as expected

PR Checklist

Targets the correct branch
Tests are passing (or failures are unrelated)

PureWeen · 2021-02-04T01:38:07Z

Xamarin.Forms.Core/Shell/ShellUriHandler.cs

@@ -245,21 +249,6 @@ internal static List<RouteRequestBuilder> GenerateRoutePaths(Shell shell, Uri re

 			var segments = RetrievePaths(localPath);

-			if (!relativeMatch)


This code just moved to the bottom of this method so we can try to fulfill the route before throwing an exception

PureWeen · 2021-02-04T01:38:40Z

Xamarin.Forms.Core/Shell/ShellUriHandler.cs

@@ -221,11 +230,6 @@ internal static List<RouteRequestBuilder> GenerateRoutePaths(Shell shell, Uri re
 			var routeKeys = Routing.GetRouteKeys();
 			for (int i = 0; i < routeKeys.Length; i++)
 			{
-				if (routeKeys[i] == originalRequest.OriginalString)


This code just moved to the bottom of this method so we can try to match all the nested route parts

PureWeen · 2021-02-04T01:39:58Z

Xamarin.Forms.Core/Shell/ShellUriHandler.cs

@@ -274,124 +263,207 @@ internal static List<RouteRequestBuilder> GenerateRoutePaths(Shell shell, Uri re

 			if (relativeMatch && shell?.CurrentItem != null)
 			{
-				// retrieve current location
-				var currentLocation = NodeLocation.Create(shell);


this code all just moved into a specific method ProcessRelativeRoute

PureWeen · 2021-02-08T18:13:49Z

https://dev.azure.com/xamarin/public/_releaseProgress?releaseId=2843&_a=release-pipeline-progress

# Conflicts: # Xamarin.Forms.Core.UnitTests/ShellNavigatingTests.cs

PureWeen · 2021-02-09T15:38:15Z

Test failures unrelated

PureWeen · 2021-02-09T17:08:48Z

Xamarin.Forms.Core/Shell/ShellUriHandler.cs

+					shell.CurrentItem.CurrentItem.CurrentItem.Route,
+				};
+
+				restOfPath = CollapsePath(restOfPath.ToArray(), shellRoutes, true);


New Code

This code takes into account the parts of the shell route that need to be combined with the page routes which was the main issue.

So if the shell route is

//default_item/page/default_content

and the user registers a global route of

page/page2

When they navigate to //page/page2 the uri should ne

//page/page2

PureWeen · 2021-02-09T17:16:04Z

Xamarin.Forms.Core/Shell/ShellUriHandler.cs


+			bestMatches.Clear();
+			ExpandOutGlobalRoutes(possibleRoutePaths, routeKeys);


This is new code

The ExpandOutGlobalRoutes used to just be inside the if statement for relative route searches but I moved the logic out to ExpandOutGlobalRoutes so if the user supplies an absolute route it can still try to expand out the global routes

PureWeen · 2021-02-09T17:17:48Z

Xamarin.Forms.Core/Shell/ShellUriHandler.cs

+				var url = possibleRoutePath.PathFull;
+
+				var globalRouteMatches = 
+					SearchForGlobalRoutes(


This is new code

Same description as above. Once we get here if the full route hasn't been matched then we try to fulfill the rest of the request with global registered routes.

PureWeen · 2021-02-09T17:20:05Z

Xamarin.Forms.Core/Shell/ShellUriHandler.cs

+			int walkBackCurrentStackIndex = -1;
+
+			if (paths.Count > 0)
+				walkBackCurrentStackIndex = localRouteStack.IndexOf(paths[0]);


This is new code

The idea here is to collapse the URI

If a user is at

//monkey/page/nextpage

And they have a global route that's

page/nextpage/something

Then this code will combine those two paths so they become

//monkey/page/something

rachelkang

Fixes issue referenced and LGTM for the most part!
Thank you for all the PR comments :)
Just had a few nit questions/suggestions

Xamarin.Forms.Core.UnitTests/ShellNavigatingTests.cs

Xamarin.Forms.Core/Shell/ShellUriHandler.cs

Fix additional scenarios with nested defined routes

59d2f79

PureWeen requested a review from StephaneDelcroix as a code owner February 4, 2021 01:16

PureWeen requested review from rachelkang and removed request for StephaneDelcroix February 4, 2021 01:16

PureWeen added this to To Fix in 5.0.0 SR 3 via automation Feb 4, 2021

PureWeen commented Feb 4, 2021

View reviewed changes

PureWeen moved this from To Fix to PR Needs Review in 5.0.0 SR 3 Feb 4, 2021

- pick better starting point for uri matching

dba5d7a

jsuarezruiz added a/navigation a/shell 🐚 labels Feb 4, 2021

PureWeen added 2 commits February 8, 2021 17:21

Merge branch '5.0.0' into fix_13611

fa7e108

# Conflicts: # Xamarin.Forms.Core.UnitTests/ShellNavigatingTests.cs

- fix merge

e16afda

hartez approved these changes Feb 9, 2021

View reviewed changes

PureWeen commented Feb 9, 2021

View reviewed changes

rachelkang reviewed Feb 9, 2021

View reviewed changes

- formating changes and more accurate test fix

5792f43

PureWeen merged commit cc0caa6 into 5.0.0 Feb 9, 2021

5.0.0 SR 3 automation moved this from PR Needs Review to Done Feb 9, 2021

PureWeen deleted the fix_13611 branch February 9, 2021 18:12

samhouts added this to the 5.0.0 milestone Feb 12, 2021

samhouts added 5.0.0 Regression on 5.0.0 t/bug 🐛 labels Feb 13, 2021

dependabot bot mentioned this pull request Mar 18, 2021

Bump Xamarin.Forms.Platform.WPF from 4.8.0.1269 to 5.0.0.2012 aleks-ivanov/maui#9

Open

pictos mentioned this pull request Jun 10, 2021

[Bug][Shell] Shell.Current.GoToAsync exception on certain navigation sequence #12958

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix additional scenarios with nested defined routes #13648

Fix additional scenarios with nested defined routes #13648

PureWeen commented Feb 4, 2021

PureWeen Feb 4, 2021

PureWeen Feb 4, 2021

PureWeen Feb 4, 2021

PureWeen commented Feb 8, 2021

PureWeen commented Feb 9, 2021

PureWeen Feb 9, 2021 •

edited

PureWeen Feb 9, 2021

PureWeen Feb 9, 2021

PureWeen Feb 9, 2021

rachelkang left a comment

		@@ -245,21 +249,6 @@ internal static List<RouteRequestBuilder> GenerateRoutePaths(Shell shell, Uri re

		var segments = RetrievePaths(localPath);

		if (!relativeMatch)


		bestMatches.Clear();
		ExpandOutGlobalRoutes(possibleRoutePaths, routeKeys);

Fix additional scenarios with nested defined routes #13648

Fix additional scenarios with nested defined routes #13648

Conversation

PureWeen commented Feb 4, 2021

Description of Change

Issues Resolved

Platforms Affected

Testing Procedure

PR Checklist

PureWeen Feb 4, 2021

Choose a reason for hiding this comment

PureWeen Feb 4, 2021

Choose a reason for hiding this comment

PureWeen Feb 4, 2021

Choose a reason for hiding this comment

PureWeen commented Feb 8, 2021

PureWeen commented Feb 9, 2021

PureWeen Feb 9, 2021 • edited

Choose a reason for hiding this comment

PureWeen Feb 9, 2021

Choose a reason for hiding this comment

PureWeen Feb 9, 2021

Choose a reason for hiding this comment

PureWeen Feb 9, 2021

Choose a reason for hiding this comment

rachelkang left a comment

Choose a reason for hiding this comment

PureWeen Feb 9, 2021 •

edited