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

Bindable picker #515

Merged
merged 2 commits into from Nov 16, 2016

Conversation

Projects
None yet
9 participants
@StephaneDelcroix
Member

StephaneDelcroix commented Nov 9, 2016

Description of Change

This PR includes and replace #296 by @joacar

Please do not squash merge to preserve authors informations.

Bugs Fixed

None

API Changes

Added:

public static readonly BindableProperty Picker.ItemsSourceProperty;
public static readonly BindableProperty Picker.SelectedItemProperty;
public BindingBase Picker.ItemDisplayBinding { get; set;}
public IList ItemsSource { get; set; }
public object SelectedItem { get; set; }

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
ItemDisplayBinding.Apply(item, this, s_displayProperty);
ItemDisplayBinding.Unapply();
return (string)GetValue(s_displayProperty);

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Nov 9, 2016

Member

See how wicked this is ?

@StephaneDelcroix

StephaneDelcroix Nov 9, 2016

Member

See how wicked this is ?

This comment has been minimized.

@joacar

joacar Nov 9, 2016

An alternative way of doing it is the below code (which allows for nested properties)

// Find the property by walking the display member path to find any nested properties
            var propertyPathParts = DisplayMemberPath.Split('.');
            object propertyValue = item;
            foreach (var propertyPathPart in propertyPathParts)
            {
                var propInfo = propertyValue.GetType().GetTypeInfo().GetDeclaredProperty(propertyPathPart);
                if (propInfo == null)
                {
                    throw new ArgumentException($"No property '{propertyPathPart}' was found on '{propertyValue.GetType().FullName}'");
                }
                propertyValue = propInfo.GetValue(propertyValue);
            }
            if (propertyValue == null)
            {
                throw new ArgumentException($"No property '{DisplayMemberPath}' was found on '{item.GetType().FullName}'");
            }
            return propertyValue as string;

Don't know how it compares to BindingBase in terms of effeciency or ease to use from XAML though

@joacar

joacar Nov 9, 2016

An alternative way of doing it is the below code (which allows for nested properties)

// Find the property by walking the display member path to find any nested properties
            var propertyPathParts = DisplayMemberPath.Split('.');
            object propertyValue = item;
            foreach (var propertyPathPart in propertyPathParts)
            {
                var propInfo = propertyValue.GetType().GetTypeInfo().GetDeclaredProperty(propertyPathPart);
                if (propInfo == null)
                {
                    throw new ArgumentException($"No property '{propertyPathPart}' was found on '{propertyValue.GetType().FullName}'");
                }
                propertyValue = propInfo.GetValue(propertyValue);
            }
            if (propertyValue == null)
            {
                throw new ArgumentException($"No property '{DisplayMemberPath}' was found on '{item.GetType().FullName}'");
            }
            return propertyValue as string;

Don't know how it compares to BindingBase in terms of effeciency or ease to use from XAML though

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Nov 9, 2016

Member

Binding has the following advantages over your fully working and correct code:

  • supports converters natively (no need for converter or Func properties)
  • paths can contains indexers (I don't see this used often in Picker, but who knows)
  • Bindings can be compiled while used from Xaml
  • Bindings are fully tested
@StephaneDelcroix

StephaneDelcroix Nov 9, 2016

Member

Binding has the following advantages over your fully working and correct code:

  • supports converters natively (no need for converter or Func properties)
  • paths can contains indexers (I don't see this used often in Picker, but who knows)
  • Bindings can be compiled while used from Xaml
  • Bindings are fully tested

This comment has been minimized.

@joacar

joacar Nov 9, 2016

Awesome! Gonna try find me some good resources on BindingBase - seems like I overlooked it :)

@joacar

joacar Nov 9, 2016

Awesome! Gonna try find me some good resources on BindingBase - seems like I overlooked it :)

@joacar

This comment has been minimized.

Show comment
Hide comment
@joacar

joacar Nov 9, 2016

How is the property ItemDisplayBinding used? Similar to "DisplayMemberPath" (aligns well with the same concept in WPF controls) - i.e. finding a property to use as display value on the bounded object(s)?

joacar commented Nov 9, 2016

How is the property ItemDisplayBinding used? Similar to "DisplayMemberPath" (aligns well with the same concept in WPF controls) - i.e. finding a property to use as display value on the bounded object(s)?

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Nov 9, 2016

Member

@joacar this is how to use the ItemDisplayBinding:

 var picker = new Picker
 {
    ItemDisplayBinding = new Binding("Name"),
    ItemsSource = items
 };

or from Xaml

<Picker ItemDisplayBinding="{Binding Name}" />

Xaml is the reason why the property is not Bindable. Otherwise the parser wouldn't now if it has to set the property, or apply the Binding.

Member

StephaneDelcroix commented Nov 9, 2016

@joacar this is how to use the ItemDisplayBinding:

 var picker = new Picker
 {
    ItemDisplayBinding = new Binding("Name"),
    ItemsSource = items
 };

or from Xaml

<Picker ItemDisplayBinding="{Binding Name}" />

Xaml is the reason why the property is not Bindable. Otherwise the parser wouldn't now if it has to set the property, or apply the Binding.

Joakim Carselind and others added some commits Nov 9, 2016

Squashed commit of the following:
commit 8d784ec
Author: Joakim Carselind <joakim.carselind@cub.se>
Date:   Tue Aug 23 00:30:25 2016 +0200

    Added DisplayConverter property of type IValueConverter to perform conversion from object to string

commit afb606f
Author: Joakim Carselind <joakim.carselind@cub.se>
Date:   Tue Aug 23 00:07:55 2016 +0200

    Use IsValueType

commit 4742c22
Author: Joakim Carselind <joakim.carselind@cub.se>
Date:   Fri Aug 19 18:58:40 2016 +0200

    Fixed bug with nested property expression

commit 70a121e
Author: Joakim Carselind <joakim.carselind@cub.se>
Date:   Fri Aug 19 18:43:14 2016 +0200

    Added more tests

commit 49c7876
Author: Joakim Carselind <joakim.carselind@cub.se>
Date:   Thu Aug 18 13:28:36 2016 +0200

    Siimplified setting SelectedItem. Added property to provide full control over how to display the objects by DisplayFunc. Added tests

commit 5c1d5e1
Author: Joakim Carselind <joakim.carselind@cub.se>
Date:   Thu Aug 11 17:15:36 2016 +0200

    Trying to fix formatting with tabs instead of spaces

commit d64663c
Author: Joakim Carselind <joakim.carselind@cub.se>
Date:   Thu Aug 11 17:10:39 2016 +0200

    Formatting. Handle Reset,Move,Replace collection changed action equal by re binding Items collection

commit 8d46418
Author: Joakim Carselind <joakim.carselind@cub.se>
Date:   Thu Aug 11 16:33:03 2016 +0200

    Removed inline documentation. Fixed formatting

commit 28010a1
Author: Joakim Carselind <joakim.carselind@cub.se>
Date:   Thu Aug 11 16:29:37 2016 +0200

    Removed SelectedValue since SelectedIndex and SelectedItem make it redundant

commit ac9d658
Author: Joakim Carselind <joakim.carselind@cub.se>
Date:   Thu Aug 11 14:51:20 2016 +0200

    Initial attempt on bindable picker
@joacar

This comment has been minimized.

Show comment
Hide comment
@joacar

joacar Nov 10, 2016

Perhaps change all explicit type conversions of LockableObservableListWrapper on Items and introduce a private field

LockableObservableListWrapper lockableList;
...
public Picker()
{
    ....
    lockableList =(LockableObservableListWrapper)Items;
}

joacar commented Nov 10, 2016

Perhaps change all explicit type conversions of LockableObservableListWrapper on Items and introduce a private field

LockableObservableListWrapper lockableList;
...
public Picker()
{
    ....
    lockableList =(LockableObservableListWrapper)Items;
}
@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Nov 10, 2016

Member

direct casts are free

Member

StephaneDelcroix commented Nov 10, 2016

direct casts are free

@jassmith jassmith merged commit e5af21f into master Nov 16, 2016

3 of 6 checks passed

iOS10-UITests Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle8 (EZ Test) :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests f…
Details
iOS8-UITests Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle8 (EZ Test) :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests fa…
Details
iOS9-UITests Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle9 (EZ Test) :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests f…
Details
Android-UITests Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle9 (EZ Test) :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passe…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle9 (EZ Test) :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle9 (EZ Test) :: Windows Debug : Tests passed: 3479, ignored: 8
Details
return;
OnPropertyChanging();
var oldValue = value;

This comment has been minimized.

@lsuarez5280

lsuarez5280 Jan 10, 2017

Noticing (long after the fact) oldValue is in fact assigned the newly set value. Won't have any impact the way the code doesn't make use of these values in OnItemDisplayBindingChanged, but may cause future issues.

@lsuarez5280

lsuarez5280 Jan 10, 2017

Noticing (long after the fact) oldValue is in fact assigned the newly set value. Won't have any impact the way the code doesn't make use of these values in OnItemDisplayBindingChanged, but may cause future issues.

@xrkolovos

This comment has been minimized.

Show comment
Hide comment
@xrkolovos

xrkolovos Mar 10, 2017

Do you know when 2.3.4 is going to be stable?

xrkolovos commented Mar 10, 2017

Do you know when 2.3.4 is going to be stable?

@bill2004158

This comment has been minimized.

Show comment
Hide comment
@bill2004158

bill2004158 May 4, 2017

any idea why I got this error when build:
No property, bindable property, or event found for 'ItemsSource'

ps:
I am using id="Xamarin.Forms" version="2.3.4.231"

bill2004158 commented May 4, 2017

any idea why I got this error when build:
No property, bindable property, or event found for 'ItemsSource'

ps:
I am using id="Xamarin.Forms" version="2.3.4.231"

@Magic73

This comment has been minimized.

Show comment
Hide comment
@Magic73

Magic73 Aug 30, 2017

I'm using 2.4.. and it works.

Magic73 commented Aug 30, 2017

I'm using 2.4.. and it works.

@samhouts samhouts added this to the 2.3.4 milestone Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment