Skip to content

Fix Picker ItemDisplayBinding doesn't support MVVM properly #29922

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
53 changes: 53 additions & 0 deletions src/Controls/src/Core/Picker/Picker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,27 @@ static void OnItemsSourceChanged(BindableObject bindable, object oldValue, objec

void OnItemsSourceChanged(IList oldValue, IList newValue)
{
if (oldValue is not null)
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is duplicated, can extract it to a method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Inside the methods, check items.Count == 0 before iterating to avoid unnecessary loops

{
foreach (var item in oldValue)
{
if (item is INotifyPropertyChanged npc)
{
npc.PropertyChanged -= OnPickerItemPropertyChanged;
}
}
}
if (newValue is not null && ItemDisplayBinding is not null)
{
foreach (var item in newValue)
{
if (item is INotifyPropertyChanged npc)
{
npc.PropertyChanged += OnPickerItemPropertyChanged;
}
}
}

var oldObservable = oldValue as INotifyCollectionChanged;
if (oldObservable != null)
oldObservable.CollectionChanged -= CollectionChanged;
Expand All @@ -287,8 +308,40 @@ void OnItemsSourceChanged(IList oldValue, IList newValue)
}
}

void OnPickerItemPropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (ItemDisplayBinding is Binding binding && !string.IsNullOrEmpty(binding.Path))
{
if (e.PropertyName == binding.Path)
{
ResetItems();
}
}
}

void CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
{
if (e.OldItems is not null)
{
foreach (var item in e.OldItems)
{
if (item is INotifyPropertyChanged npc)
{
npc.PropertyChanged -= OnPickerItemPropertyChanged;
}
}
}
if (e.NewItems is not null && ItemDisplayBinding is not null)
{
foreach (var item in e.NewItems)
{
if (item is INotifyPropertyChanged npc)
{
npc.PropertyChanged += OnPickerItemPropertyChanged;
}
}
}

switch (e.Action)
{
case NotifyCollectionChangedAction.Add:
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
94 changes: 94 additions & 0 deletions src/Controls/tests/TestCases.HostApp/Issues/Issue25634.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
using System.Collections.ObjectModel;
using System.ComponentModel;
using System.Runtime.CompilerServices;
using Microsoft.Maui.Controls;

namespace Maui.Controls.Sample.Issues;

[Issue(IssueTracker.Github, 25634, "Picker ItemDisplayBinding doesn't support MVVM properly", PlatformAffected.All)]
public class Issue25634 : ContentPage
{
public ObservableCollection<Issue25634_Model> People { get; private set; } = new()
{
new Issue25634_Model { FirstName = "John", LastName = "Doe" },
new Issue25634_Model { FirstName = "Jane", LastName = "Smith" },
new Issue25634_Model { FirstName = "Sam", LastName = "Johnson" }
};

Picker picker;

public Issue25634()
{
BindingContext = this;

picker = new Picker();
picker.SetBinding(Picker.ItemsSourceProperty, nameof(People));
picker.ItemDisplayBinding = new Binding("LastName");

var button = new Button
{
Text = "Click",
AutomationId = "PickerButton"
};
button.Clicked += Button_Clicked;

var stackLayout = new VerticalStackLayout
{
Padding = new Thickness(30, 0),
Spacing = 25
};
stackLayout.Children.Add(picker);
stackLayout.Children.Add(button);

Content = stackLayout;

picker.SelectedItem = People[0];
}

void Button_Clicked(object sender, EventArgs e)
{
if (People.Count > 0)
{
People[0].LastName = "Alice";
}
}
}

public class Issue25634_Model : INotifyPropertyChanged
{
private string firstName = "FirstName";
private string lastName = "LastName";

public string FirstName
{
get => firstName;
set
{
if (firstName != value)
{
firstName = value;
OnPropertyChanged();
}
}
}

public string LastName
{
get => lastName;
set
{
if (lastName != value)
{
lastName = value;
OnPropertyChanged();
}
}
}

public event PropertyChangedEventHandler PropertyChanged;

protected void OnPropertyChanged([CallerMemberName] string name = "")
{
PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(name));
}
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using NUnit.Framework;
using UITest.Appium;
using UITest.Core;

namespace Microsoft.Maui.TestCases.Tests.Issues;
public class Issue25634 : _IssuesUITest
{
public Issue25634(TestDevice testDevice) : base(testDevice)
{
}

public override string Issue => "Picker ItemDisplayBinding doesn't support MVVM properly";

[Test]
[Category(UITestCategories.Picker)]
public void VerifyPickerItemDisplayBindingValue()
{
App.WaitForElement("PickerButton");
App.Tap("PickerButton");
VerifyScreenshot();
Copy link
Contributor

Choose a reason for hiding this comment

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

Pending snapshot in all the platforms. Already available in the latest build.
image
Could you commit the images?

}
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading