Fix Picker ItemDisplayBinding doesn't support MVVM properly #29922
Conversation
|
Hey there @@devanathan-vaithiyanathan! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
dotnet-policy-service
bot
added
the
partner/syncfusion
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue25634.cs
Outdated
Show resolved
Hide resolved
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| { | ||
| App.WaitForElement("PickerButton"); | ||
| App.Tap("PickerButton"); | ||
| VerifyScreenshot(); |
There was a problem hiding this comment.
Pending snapshot in all the platforms. Already available in the latest build.
Could you commit the images?
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jsuarezruiz
left a comment
jsuarezruiz
left a comment
There was a problem hiding this comment.
Pending test snapshots on Mac and Windows.
@jsuarezruiz , pending snapshots has been added. |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| void OnItemsSourceChanged(IList oldValue, IList newValue) | ||
| { | ||
| if (oldValue is not null) |
There was a problem hiding this comment.
This logic is duplicated, can extract it to a method.
There was a problem hiding this comment.
Inside the methods, check items.Count == 0 before iterating to avoid unnecessary loops
devanathan-vaithiyanathan
force-pushed
the
fix-25634
branch
from
6e94f72 to
5961883
Compare
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 29922Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 29922" |
There was a problem hiding this comment.
Pull request overview
This PR adds support for automatically refreshing the Picker display when a bound item's property changes through ItemDisplayBinding. The implementation subscribes to PropertyChanged events on items in the ItemsSource and updates the display when the bound property changes.
Key Changes:
- Event subscription for
INotifyPropertyChangeditems in ItemsSource - Automatic refresh when bound property changes
- Dynamic subscription/unsubscription when items are added/removed
- Comprehensive UI tests with screenshot verification for all platforms
Reviewed changes
Copilot reviewed 3 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Controls/src/Core/Picker/Picker.cs |
Core implementation: adds PropertyChanged event handling for items, subscription/unsubscription logic in OnItemsSourceChanged and CollectionChanged methods |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue25634.cs |
NUnit UI test verifying the picker display updates after property change |
src/Controls/tests/TestCases.HostApp/Issues/Issue25634.cs |
Test HostApp page with Picker, ObservableCollection, and INotifyPropertyChanged model |
src/Controls/tests/TestCases.Android.Tests/snapshots/android/VerifyPickerItemDisplayBindingValue.png |
Android test screenshot baseline |
src/Controls/tests/TestCases.iOS.Tests/snapshots/ios/VerifyPickerItemDisplayBindingValue.png |
iOS test screenshot baseline |
src/Controls/tests/TestCases.WinUI.Tests/snapshots/windows/VerifyPickerItemDisplayBindingValue.png |
Windows test screenshot baseline |
src/Controls/tests/TestCases.Mac.Tests/snapshots/mac/VerifyPickerItemDisplayBindingValue.png |
MacCatalyst test screenshot baseline |
| { | ||
| if (e.PropertyName == binding.Path) | ||
| { | ||
| ResetItems(); |
There was a problem hiding this comment.
Potential performance issue: ResetItems() is called for every property change on any item, even if that item is not currently visible or selected. For large collections where many items change their properties simultaneously, this could cause excessive UI updates.
Consider optimizing to only refresh the display when the selected item's property changes:
void OnPickerItemPropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (ItemDisplayBinding is Binding binding && !string.IsNullOrEmpty(binding.Path))
{
if (e.PropertyName == binding.Path)
{
// Only reset if the changed item is the currently selected item
// or if we need to update all items in the dropdown
if (sender == SelectedItem)
{
// Update just the display text for the selected item
Handler?.UpdateValue(nameof(IPicker.SelectedItem));
}
else
{
// For items in the dropdown list, consider a more targeted update
ResetItems();
}
}
}
}| ResetItems(); | |
| // Only reset if the changed item is not the currently selected item | |
| if (sender == SelectedItem) | |
| { | |
| // Update just the display text for the selected item | |
| Handler?.UpdateValue(nameof(IPicker.SelectedItem)); | |
| } | |
| else | |
| { | |
| // For items in the dropdown list, update the items | |
| ResetItems(); | |
| } |
| if (e.PropertyName == binding.Path) | ||
| { | ||
| ResetItems(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The OnPickerItemPropertyChanged method only handles Binding type but not other BindingBase types like TypedBinding or custom binding implementations. This could cause the feature to silently fail for other binding types.
Consider handling all BindingBase types or documenting the limitation:
void OnPickerItemPropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (ItemDisplayBinding is Binding binding && !string.IsNullOrEmpty(binding.Path))
{
if (e.PropertyName == binding.Path || string.IsNullOrEmpty(e.PropertyName))
{
ResetItems();
}
}
else if (ItemDisplayBinding is not null && ItemDisplayBinding is not Binding)
{
// For other binding types, refresh on any property change
// since we can't determine the specific path
ResetItems();
}
}Note: Also consider checking for string.IsNullOrEmpty(e.PropertyName) which indicates all properties changed.
| if (e.PropertyName == binding.Path) | |
| { | |
| ResetItems(); | |
| } | |
| } | |
| // For Binding, only reset if the relevant property changed, or all properties changed | |
| if (e.PropertyName == binding.Path || string.IsNullOrEmpty(e.PropertyName)) | |
| { | |
| ResetItems(); | |
| } | |
| } | |
| else if (ItemDisplayBinding is not null && ItemDisplayBinding is not Binding) | |
| { | |
| // For other BindingBase types (TypedBinding, custom), reset on any property change | |
| ResetItems(); | |
| } |
| if (oldValue is not null) | ||
| { | ||
| 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; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing cleanup when Picker is disposed or removed from the visual tree. The PropertyChanged event handlers attached to items will prevent garbage collection of both the Picker and the items, causing a memory leak.
Consider overriding cleanup methods to unsubscribe from events:
protected override void OnHandlerChanging(HandlerChangingEventArgs args)
{
base.OnHandlerChanging(args);
if (args.NewHandler == null && ItemsSource is not null)
{
// Unsubscribe when being removed from the visual tree
foreach (var item in ItemsSource)
{
if (item is INotifyPropertyChanged npc)
{
npc.PropertyChanged -= OnPickerItemPropertyChanged;
}
}
}
}Alternatively, use weak event patterns to avoid memory leaks.
3 participants
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
Picker does not refresh displayed item when bound property changes through ItemDisplayBinding
Description of Change
Added support to automatically refresh the Picker display when a bound item's property changes. Subscribed to property change events in the items source, and updated the display when the property defined in ItemDisplayBinding changes. Also handled item additions and removals by updating the subscriptions accordingly.
Issues Fixed
Fixes #25634
Tested the behavior in the following platforms.
iOS-BeforeFix.mov
iOS-AfterFix.mov