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

OnNavigatedTo Contains Extra Parameter #1225

Closed
mrbiggred opened this issue Oct 25, 2017 · 11 comments · Fixed by #1249
Closed

OnNavigatedTo Contains Extra Parameter #1225

mrbiggred opened this issue Oct 25, 2017 · 11 comments · Fixed by #1249
Labels

Comments

@mrbiggred
Copy link

Package info

  • Platform: Windows, Visual Studio 2017
  • Prism version: Dryloc Forms 7.0.0.168-pre
  • Xamarin version (if applicable): Xamarin Forms 2.4.0.282
  • Other version info:

Repro steps

When OnNaviagedTo is called there is an extra parameter with a key of __NavigationMode. The problem is the extra parameter is messing up my parameter checking. For example, I have logic in the OnNavigatedTo that checks if a certain number of parameters are provided.

public void OnNavigatedTo(NavigationParameters parameters)
{
    // Should only be one parameter.
    if (parameters.Count != 1)
        throw new ArgumentException("Should only be one parameter", nameof(parameters));

    // Rest of method....
}

I propose that instead of having the __NavigationMode added to the parameters collection it is a separate property. That way it is still available but won't mess up my parameter checking logic. It would be nice to assume all the parameters in the collection are ones I put there and not ones that magically show up.

If you like the above idea I can try implement it and do a pull request. If you anyone has any better idea please let me know.

Thank you.

@brianlagunas
Copy link
Member

I think you have a valid point. The issue is that we need a way to pass information about what is going on within the navigation service. These should not be used directly which is why they start with "__".

Maybe we can create an internal "property bag" that we can then use extension methods to expose instead of properties. We can't add properties to the NavigationParameters class as the long term vision is to add that class to the Prism.Core which will be shared across all platforms.

@brianlagunas
Copy link
Member

I'd be interested in seeing your idea to implement this while keeping our long term goal in mind.

@mrbiggred
Copy link
Author

@brianlagunas thanks for explaining the reason you can't just add a property. I'll post any solutions I come up with in the next couple of days.

@brianlagunas brianlagunas added the XF label Nov 1, 2017
@mrbiggred
Copy link
Author

@brianlagunas any reason we couldn't use inheritance? For example, the long term solution might look like:

namespace Prism.Core
{
  public class NavigationParametersBase : IEnumerable<KeyValuePair<string, object>>
  {
    // Existing Prism.Forms.Navigation.NavigationParameters class
  }
}

namespace Prism.Forms
{
  public class NavigationParameters : NavigationParametersBase
  {
    NavigationMode PageNavigationMode { get; set; }
  }
}

Now each platform can extend the navigation parameters as needed. Until the NavigationParameters is moved to Prism.Core we could leave everything in the Prism.Forms project and, hopefully, not break anything:

namespace Prism.Forms
{
  internal class NavigationParametersBase : IEnumerable<KeyValuePair<string, object>>
  {
    // Existing Prism.Forms.Navigation.NavigationParameters class.  Will eventually be moved
    // to Prism.Core.
  } 

  public class NavigationParameters : NavigationParametersBase
  {
    NavigationMode PageNavigationMode { get; set; }
  }
}

@brianlagunas
Copy link
Member

That would break the sharing of the NavigationParameters class in the interfaces.

@mrbiggred
Copy link
Author

Is the plan to move all or most of the navigation classes from Prism.Forms project to the Prism core? If so is the goal to have a common navigation interfaces for each platform? Would there be any platform specific navigation settings that need to be exposed just to that one platform?

Is there a road map or design document for the future of Prism?

Thanks.

@brianlagunas
Copy link
Member

brianlagunas commented Nov 3, 2017

Yes, the long term plan is to move as many of the navigation interfaces into a shared assembly for all platforms. This will make code sharing across various platforms with Prism much easier. There will also most likely be platform specific navigation settings that will need to be exposed.

There is no published roadmap or design document.

@mrbiggred
Copy link
Author

Thank you for the information and your patience with my questions. I'll be on vacation for the next two weeks so feel free to assign this issue to someone else. On the plus side I have a 10+ hour flight to think about this problem.

@brianlagunas
Copy link
Member

Vacations are the best time to work on OSS. Especially on features you want :)

brianlagunas added a commit that referenced this issue Nov 10, 2017
Added a collection to the NavigationParameetrs class that can be used to track internal parameter infromation. This fixes #1225
@mrbiggred
Copy link
Author

@brianlagunas I'm glad you found a solution to the issue. I look forward to trying it out the fix later this week. Sorry I couldn't be more help.

@lock
Copy link

lock bot commented Jan 29, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants