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

Implement NavigateFromAsync method without recreating a page. #3182

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

niimima
Copy link
Contributor

@niimima niimima commented Jul 5, 2024

Description of Change

Adding NavigateFromAsync methods without recreating a page.

Bugs Fixed

API Changes

List all API changes here (or just put None),

Added:

  • Task INavigationService.NavigateFromAsync(string viewName, Uri route, INavigationParameters parameters);
  • Task INavigationServiceExtensions.NavigateFromAsync(this INavigationService navigationService, string viewName, Uri route)

Behavioral Changes

No changes. Only additions of features.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard

@niimima
Copy link
Contributor Author

niimima commented Jul 5, 2024

I have a concern regarding the pull request I've created. When searching for a page using viewName, if it is not found, what would be the recommended behavior?
https://github.com/PrismLibrary/Prism/pull/3182/files#diff-8ae9340c7f10510f59a045175866a15bdb8ed35b541c8256cf73252dafadc343R350-R356

In the current implementation, pages specified by route appears directly under the window.

{
if (page is not null && ViewModelLocator.GetNavigationName(page) == viewName)
break;
page = page.GetParentPage();
Copy link
Contributor

Choose a reason for hiding this comment

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

Parent page? Shouldn't this be the page before the current page in the stack and if it is the root page then we get the parent page?

Copy link
Member

Choose a reason for hiding this comment

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

@AmrAlSayed0 is correct. While we do care about the parent in some scenarios, what we really need is the previous page in the navigation stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are still some issues remaining, but I have made corrections to use the NavigationStack as described in the following link.

// Find a page that matches the viewName.
var currentPage = GetCurrentPage();
var navigationPages = currentPage.Navigation.NavigationStack.ToList();
navigationPages.Reverse();
var foundPage = navigationPages.FirstOrDefault(page => ViewModelLocator.GetNavigationName(page) == viewName);
if (foundPage is null)
{
// Find a page from parents.
var page = currentPage;
while (page != null)
{
if (page is not null && ViewModelLocator.GetNavigationName(page) == viewName)
break;
page = page.GetParentPage();
}
currentPage = page;
}
else
{
// Insert RemovePageSegment.
var removePageCount = navigationPages.IndexOf(foundPage);
var tempQueue = new Queue<string>();
for (int i = 0; i < removePageCount; i++)
{
tempQueue.Enqueue(RemovePageSegment);
}
while(navigationSegments.Count > 0)
{
tempQueue.Enqueue(navigationSegments.Dequeue());
}
navigationSegments = tempQueue;
}

@brianlagunas
Copy link
Member

This PR is not performing the actions required of the issue. Per the issue

This API should then effectively do the same as ../../SomePage where the ../.. is replaced by going back until we found the View that was specified.

This means you must first find the page, while keeping track of the number of times we must navigate back until we reach this page, and then begin navigating forward from there.

Decisions must also be made as to whether this can be supported in both a modal and non-modal navigation stack. While also considering the page's parent such as a TabbedPage or FlyoutPage

@dansiegel dansiegel added this to the vNext milestone Jul 19, 2024
@niimima niimima requested a review from dansiegel as a code owner July 29, 2024 02:22
@niimima
Copy link
Contributor Author

niimima commented Jul 29, 2024

@brianlagunas

This PR is not performing the actions required of the issue. Per the issue

This API should then effectively do the same as ../../SomePage where the ../.. is replaced by going back until we found the View that was specified.

This means you must first find the page, while keeping track of the number of times we must navigate back until we reach this page, and then begin navigating forward from there.

Decisions must also be made as to whether this can be supported in both a modal and non-modal navigation stack. While also considering the page's parent such as a TabbedPage or FlyoutPage

Thank you for your feedback. Firstly, I am working on ensuring that the navigation works correctly with both TabbedPage and FlyoutPage. The FlyoutPage seems to be working well, but I need to confirm the specifications for the TabbedPage.

In the existing NavigateAsync method, you can use KnownNavigationParameters to either create or select a tab. For the NavigateFromAsync method, which approach would be more appropriate? Or, would it be better to dynamically switch between creating and selecting tabs based on the situation?

Copy link
Member

@dansiegel dansiegel left a comment

Choose a reason for hiding this comment

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

This really should work across both Modal and Non-Modal Navigation Stacks. I would suggest looking at the helpers we have that walk the NavigationStack today. That should help you to build something that can go the reverse direction to locate a given view.

Comment on lines +115 to +116
public static Task<INavigationResult> NavigateFromAsync(this INavigationService navigationService, string viewName, Uri route) =>
navigationService.NavigateFromAsync(viewName, route, new NavigationParameters());
Copy link
Member

Choose a reason for hiding this comment

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

This would need some overloads that take a string for the route as well.


try
{
if (route.IsAbsoluteUri) throw new NavigationException(NavigationException.UnsupportedAbsoluteUri);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (route.IsAbsoluteUri) throw new NavigationException(NavigationException.UnsupportedAbsoluteUri);
if (route.IsAbsoluteUri)
throw new NavigationException(NavigationException.UnsupportedAbsoluteUri);

/// <summary>
/// The <see cref="NavigationException"/> Message returned when an absolute path is specified but not supported.
/// </summary>
public const string UnsupportedAbsoluteUri = "An unsupported absolute uri. Please use a relative URI.";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public const string UnsupportedAbsoluteUri = "An unsupported absolute uri. Please use a relative URI.";
public const string UnsupportedAbsoluteUri = "Absolute Uri's are not supported when Navigated from a named source View. Please use a relative Uri.";

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

Successfully merging this pull request may close these issues.

[Enhancement] NavigateFrom
4 participants