-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix: Picker selects incorrect item after item removal #29313
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
Fix: Picker selects incorrect item after item removal #29313
Conversation
|
Hey there @@Shalini-Ashokan! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the issue where the Picker selects an incorrect item after removal by updating the selection logic to track the selected item by value instead of by index.
- Updated the RemoveItems method to recalculate the selected index based on the actual SelectedItem.
- Modified AddItems and ResetItems to use the new ClampSelectedIndex overload and a helper GetSelectedIndex method.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue29235.cs | Added tests to validate Picker behavior after item insertion and removal. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue29235.cs | Updated host UI to support interactive validation of the Picker’s selection behavior. |
| src/Controls/src/Core/Picker/Picker.cs | Updated logic for item insertion and removal to preserve the selected item by value and clamping the index using a new helper method. |
| // take care of it | ||
| if (((LockableObservableListWrapper)Items).IsLocked) | ||
| return; | ||
|
|
Copilot
AI
May 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider adding an inline comment explaining that GetSelectedIndex() is used to recalculate the selected index based on the actual SelectedItem, especially when the ItemsSource is populated. This will aid future maintainers in understanding the fallback behavior when SelectedItem is not found.
| // Recalculate the selected index based on the actual SelectedItem and ItemsSource. | |
| // This ensures the SelectedIndex is consistent, especially when ItemsSource is populated | |
| // or when the SelectedItem is not found in the Items collection. |
| foreach (object _ in e.OldItems) | ||
| ((LockableObservableListWrapper)Items).InternalRemoveAt(index--); | ||
| if (removeStart <= SelectedIndex) | ||
|
|
Copilot
AI
May 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider adding a comment explaining that recalculating the selected index before clamping ensures that the selection is preserved by comparing against the actual item value rather than the previous index. This clarification will help maintainers understand the importance of the order of operations in the removal logic.
| // Recalculate the selected index before clamping to ensure that the selection is preserved | |
| // by comparing against the actual item value rather than the previous index. |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| if (SelectedItem is not null && (ItemsSource is not null || Items is not null)) | ||
| { | ||
| ClampSelectedIndex(); | ||
| int newIndex = ItemsSource?.IndexOf(SelectedItem) ?? Items.IndexOf(SelectedItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already are checking if ItemsSource is not null before, redundant check here. Can remove the ?, or modify the code:
if (SelectedItem is null)
{
return SelectedIndex;
}
int newIndex = ItemsSource?.IndexOf(SelectedItem) ?? Items?.IndexOf(SelectedItem) ?? -1;
return newIndex >= 0 ? newIndex : SelectedIndex;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsuarezruiz, I have updated the code section based on your suggestions.
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/rebase |
c7e02a4 to
93765fa
Compare
- Add PickerPreservesSelectedItemAfterRemovingItemBeforeSelection test - Add PickerPreservesSelectedItemAfterInsertingItemBeforeSelection test - Update existing tests to verify the new correct behavior
Unit tests are sufficient to validate this fix.
StephaneDelcroix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced the UITest by a Unit Test
* Fixed the selected item changed * fix for adding items * Added the test cases * Modified the logics * Modified the test case * Corrected the code * Modified the logics * Modified the changes * Add unit tests for Picker selection preservation - Add PickerPreservesSelectedItemAfterRemovingItemBeforeSelection test - Add PickerPreservesSelectedItemAfterInsertingItemBeforeSelection test - Update existing tests to verify the new correct behavior * Remove UI test, keep only unit test Unit tests are sufficient to validate this fix. --------- Co-authored-by: Stephane Delcroix <[email protected]>
* Fixed the selected item changed * fix for adding items * Added the test cases * Modified the logics * Modified the test case * Corrected the code * Modified the logics * Modified the changes * Add unit tests for Picker selection preservation - Add PickerPreservesSelectedItemAfterRemovingItemBeforeSelection test - Add PickerPreservesSelectedItemAfterInsertingItemBeforeSelection test - Update existing tests to verify the new correct behavior * Remove UI test, keep only unit test Unit tests are sufficient to validate this fix. --------- Co-authored-by: Stephane Delcroix <[email protected]>
* Fixed the selected item changed * fix for adding items * Added the test cases * Modified the logics * Modified the test case * Corrected the code * Modified the logics * Modified the changes * Add unit tests for Picker selection preservation - Add PickerPreservesSelectedItemAfterRemovingItemBeforeSelection test - Add PickerPreservesSelectedItemAfterInsertingItemBeforeSelection test - Update existing tests to verify the new correct behavior * Remove UI test, keep only unit test Unit tests are sufficient to validate this fix. --------- Co-authored-by: Stephane Delcroix <[email protected]>
* Fixed the selected item changed * fix for adding items * Added the test cases * Modified the logics * Modified the test case * Corrected the code * Modified the logics * Modified the changes * Add unit tests for Picker selection preservation - Add PickerPreservesSelectedItemAfterRemovingItemBeforeSelection test - Add PickerPreservesSelectedItemAfterInsertingItemBeforeSelection test - Update existing tests to verify the new correct behavior * Remove UI test, keep only unit test Unit tests are sufficient to validate this fix. --------- Co-authored-by: Stephane Delcroix <[email protected]>
* Fixed the selected item changed * fix for adding items * Added the test cases * Modified the logics * Modified the test case * Corrected the code * Modified the logics * Modified the changes * Add unit tests for Picker selection preservation - Add PickerPreservesSelectedItemAfterRemovingItemBeforeSelection test - Add PickerPreservesSelectedItemAfterInsertingItemBeforeSelection test - Update existing tests to verify the new correct behavior * Remove UI test, keep only unit test Unit tests are sufficient to validate this fix. --------- Co-authored-by: Stephane Delcroix <[email protected]>
* Fixed the selected item changed * fix for adding items * Added the test cases * Modified the logics * Modified the test case * Corrected the code * Modified the logics * Modified the changes * Add unit tests for Picker selection preservation - Add PickerPreservesSelectedItemAfterRemovingItemBeforeSelection test - Add PickerPreservesSelectedItemAfterInsertingItemBeforeSelection test - Update existing tests to verify the new correct behavior * Remove UI test, keep only unit test Unit tests are sufficient to validate this fix. --------- Co-authored-by: Stephane Delcroix <[email protected]>
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
In a Picker, the SelectedItem changes unexpectedly when an item is removed. The selection maintains the same index, resulting in a different item being selected.
Root Cause
The RemoveItems method preserves selection based on index rather than the actual item.
Description of Change
Update the RemoveItems method to track and preserve the selected item by value instead of by index.
Validated the behavior in the following platforms
Issues Fixed
Fixes #29235
Output ScreenShot
Issue29235BeforeFix.mov
Issue29235AfterFix.mov