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

[Feature] Bind Navigation Parameters directly to ViewModel #1746

Closed
aritchie opened this issue Mar 21, 2019 · 13 comments · Fixed by #1779
Closed

[Feature] Bind Navigation Parameters directly to ViewModel #1746

aritchie opened this issue Mar 21, 2019 · 13 comments · Fixed by #1779
Labels

Comments

@aritchie
Copy link

Description

Add the ability to "bind" parameters from the INavigationParameters without having to pull them out yourself. This can save quite a bit of code in heavy parameterized navigation strategies.

Example

public class MyViewModel : INavigatingAware
{
        public void OnNavigatedTo(INavigationParameters parameters)
        {
            parameters.BindTo(this);
        }

        public string Id { get; set; } // If Id is passed in parameters, this is set 
        public AnotherObject MyObject { get; set; }
}

Expected Behavior

INavigationParameters.BindTo will loop through all parameters that were passed, find an equivalently named property and set it. This saves the user having to do:

Id = parameters.GetValue<string>("Id");
MyObject = parameters.GetValue<AnotherObject>("MyObject");

Potential Additional Downstream Features

  • Could add [NavigationParameter] that allows additional control over the binding mechanism similar to how Xamarin Forms Shell [QueryParameter] works
  • Could lead to a type of strongly typed navigation "from" another viewmodel
  • May not have to use navigation interfaces at all to accept the navigation parameters

#WeQuit - yes, I can do the tests, docs, & PR 👍 Let me know if you're interested

Here's a rough implementation to start the thinking process

public static void BindTo(this INavigationParameters parameters, object model)
{
    var props = model
        .GetType()
        .GetProperties(BindingFlags.Instance | BindingFlags.Public)
        .Where(x => x.CanWrite)
        .ToList();

    foreach (var parameter in parameters)
    {
        var prop = props.FirstOrDefault(x => x.Name.Equals(parameter.Key));
        if (prop != null)
        {
            // TODO: watch type
            prop.SetValue(model, parameter.Value);
        }
    }
}
@dansiegel dansiegel added the XF label Mar 22, 2019
@dansiegel
Copy link
Member

LMAO @aritchie I love this issue if for no other reason than that hashtag!

So just a thought it's pretty common to camelCase NavigationParameter keys... with a TitleCased property name... so should this be doing an equals or equals ignore case?

@Pretasoc
Copy link

This is an interesting proposal. And isn't limited to XF. If this feature gets implemented i'd suggest to introduce some attributes to control the binding.

[IgnoreBind] - Properties marked with this Attribute aren't bindet.
[ParameterName(string)] - Overwrites the name of the navigation parameter.

I'm not sure which of the following attempts is the better one, but there may be properties which aren't always present in the navigation parameters.

The first option could be the [System.ComponentModel-DataAnnotations.Required] attribute to mark some properties as required. If a required property has no value in the navigation parameters an exception is thrown. All unannotated properties are implicitly optional.

The second options is exactly the other way around. Optional Properties are marked with an [Optional] attribute and all other properties are implicitly required, if not annotated with an [IgnoreBind] attribute.

@dansiegel To solve the camelCase, TitleCase issue: an overload acceptiong an IEqualityComparer<string> could be provided, which in the default case would ignore cases for the first letter.

@aritchie
Copy link
Author

I think the nav parameter matching should be case sensitive when using “by convention/no parameter attribute” binding.

A next evolutionary step with by to do a BindFrom - find all public get properties from previous view model and bindto navigating viewmodel. These prop names should match 1-1.

As for the attributes, I don’t think ignore is a good idea. Perhaps a secondary argument for implicit by convention binding vs explicit attribute only would be better (opt in/opt out style). [NavParameter(Name=“optional”, Required=true/false)] should accomplish a well rounded set of goals for the initial release.

@dansiegel
Copy link
Member

have to admit this is growing on me... just not as proposed... unless I've misunderstood your goal... your goal seems to be to quickly initialize your ViewModel based on the Navigation Parameters which brings us into the realm of issue #1724

What I'm thinking is this might look instead more like:

public class ViewAViewModel : IAbracadabra
{
    [AutoProperty("title")]
    public string Title { get; set; }
}

The idea here is that we instead introduce an empty marker interface to ensure it's opt-in... then the NavigationService can automatically set the ViewModel the same as what you have proposed... thoughts?

@aritchie
Copy link
Author

You got it. Parameter name should be implied if not set (name optional). Also, a IsRequired=bool.

Though, The problem with the attribute opt-in effect is that you aren’t really saving code. You’re trading apples to apples.

@dansiegel
Copy link
Member

@aritchie so you're thinking this should throw exceptions if you have an IsRequired and it doesn't exist?

@brianlagunas
Copy link
Member

brianlagunas commented Mar 22, 2019

I would say that if you use the IAbracadabra it will just work assuming you have the parameter names cased property and matching exactly with the property names. If they don't match, you should add the attribute to map them.

Maybe something like

[NavigationParameter("title")]
public string Title { ... }

@aritchie
Copy link
Author

In cases where attributes are used yes. My big thing is only using attributes in required scenarios otherwise you end up trading

Parameter.GetValue for [AutoProperty]

@brianlagunas
Copy link
Member

Correct. Out of the box it just works when this condition is true (no attribute needed)

  • Both the parameter name and the property name match

This condition would pass:
parameters.Add("Title", "My Title") with pubic string Title {...}

This condition would fail:
parameters.Add("title", "My Title") with pubic string Title {...}

Use of attributes required when

  • Property is required - throw exception if not found
  • need to map param name to property name

Given:

[NavigationParameter("title")]
public string Title { ... }

This condition would pass:
parameters.Add("title", "My Title") with pubic string Title {...}

How does that look?

@aritchie
Copy link
Author

Dead sexy

@powerdude
Copy link

powerdude commented Mar 23, 2019

why make it case-sensitive? Url parameters are usually camel-cased, or hyphenated, so why make it required to have an attribute in order for camel-cased, or hypen-cased, parameters to be mapped to proper-cased(title-cased) property names?

@brianlagunas
Copy link
Member

brianlagunas commented Mar 23, 2019

@powerdude I actually have the same feeling, but it's a tough decision as you never know how parameters and properties are being used and named in an app. It's a hard choice to make.

@lock
Copy link

lock bot commented Jan 28, 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 28, 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.

5 participants