Preserve collection moves behind a XamlChangeId opt-in - #11887
Open
Sergio Pedri (Sergio0694) wants to merge 4 commits into
Open
Sergio Pedri (Sergio0694) wants to merge 4 commits into
Sergio Pedri (Sergio0694) wants to merge 4 commits into
Conversation
Register CollectionMoveNotifications as an independent, default-off XamlChangeId for GitHub issue #1503. Document the notification contract and cover flag isolation and locking. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Project progressive single-item and range removals and insertions from the already-updated source without copying or mutating it. Route vector views and ItemCollection enumeration through the adapter, reject reentrant writes, reconcile source changes, and preserve callback failures and adapter lifetime. Cover projection ranges, bounds, and intermediate reads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add managed regressions for single and range moves, notification-time reads and retained iterators, duplicates, lazy sources, reentrancy, and callback failure recovery. Cover unchanged containers, selection, bindings, focus, editors, and viewport moves in ListView and GridView. Register explicit per-test opt-in without changing test defaults. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Override inherited UAP-only hosting metadata on the new managed tests so the existing WPF CI job selects them. Make the reentrant-source case assert the invalidation error propagated by CollectionView while updating the removed current item. Verified actual CI discovery selects all 13 managed regressions and 7 optional-change API tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
microsoft-github-policy-service
Bot
added
the
needs-triage
Sergio Pedri (Sergio0694)
marked this pull request as ready for review
September 14, 2026 23:06
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Member
Author
|
/azp run |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
Mike Crider (codendone)
requested changes
Sep 15, 2026
Mike Crider (codendone)
left a comment
Mike Crider (codendone)
left a comment
Contributor
There was a problem hiding this comment.
Added a few comments. I haven't reviewed the new tests.
| VERIFY_IS_TRUE(!!locked); | ||
| } | ||
|
|
||
| void XamlOptionalChangesTests::CollectionMoveNotificationsHasIndependentOptIn() |
Contributor
There was a problem hiding this comment.
This test has minimal utility. I suggest removing it.
| } | ||
| else if (_wcsicmp(name.c_str(), L"CollectionMoveNotifications") == 0) | ||
| { | ||
| changeId = xaml_settings::XamlChangeId_CollectionMoveNotifications; |
Contributor
There was a problem hiding this comment.
EnableAllXamlOptionalChanges in APITestBase.cs should also be updated.
| optionalChangesStatics->EnableChange(xaml_settings::XamlChangeId_IconNoGridOptimization, &mutated); | ||
| optionalChangesStatics->EnableChange(xaml_settings::XamlChangeId_OptimizeApplyStyles, &mutated); | ||
| optionalChangesStatics->EnableChange(xaml_settings::XamlChangeId_DefaultStyleOptimizations, &mutated); | ||
| optionalChangesStatics->EnableChange(xaml_settings::XamlChangeId_DeferContextFlyoutInit, &mutated); |
Contributor
There was a problem hiding this comment.
The new opt-in should also be added here.
| { | ||
| ctl::ComPtr<IInspectable> item; | ||
| bool equal = false; | ||
| IFC_RETURN(GetAt(current, &item)); |
Contributor
There was a problem hiding this comment.
It looks like this raw GetAt() call will result in the repeated CheckMoveSourceUnchanged() checks and range checks of the new GetAt() method. Should this directly call BindableVectorWrapper::GetAt() for better efficiency?
|
|
||
| IFACEMETHODIMP BindableObservableVectorWrapper::First(_Outptr_ wfc::IIterator<IInspectable *> **value) | ||
| { | ||
| if (!m_tpINCC || !OptionalChangeState::IsCollectionMoveNotificationsEnabled()) |
Contributor
There was a problem hiding this comment.
Why the !m_tpINCC check? It isn't obvious to me why this is good/needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #1503
PR Type
Description
Current Behavior
BindableObservableVectorWrappertranslatesNotifyCollectionChangedAction.Moveinto a whole-vector Reset. Moving an item can consequently rebuild unaffected ListView/GridView rows and discard their state.New Behavior
Add the independently disabled-by-default
XamlChangeId.CollectionMoveNotificationsopt-in. With it enabled:The existing Move-to-Reset path and ordinary Add/Remove/Replace/Reset handling remain unchanged when the opt-in is disabled. The spec documents configuration and compatibility semantics.
Customer Impact
Apps can opt in before XAML initialization:
This avoids whole-list invalidation and preserves unaffected containers, bindings, selection, focus, and pending edits. Moved items still undergo removal/insertion; their own containers, selection, and focus are not guaranteed to survive.
Regression Potential
The changed event sequence and intermediate view are intentionally opt-in. Shared collection consumers, range indices, reentrancy, and virtualization are the main regression surfaces.
How Has This Been Tested?
Product, native client, isolated collection tests, managed Enterprise.Moco, and External.Framework builds succeeded. All five isolated collection tests passed, including the four new projection tests.
Added 13 managed regressions for notification-time reads, ranges, duplicates/nulls, lazy sources, reentrancy, error recovery, ListView/GridView item state, and viewport moves, plus an independent opt-in API test.
Integration validation is pending CI. Local execution of the 13 managed and seven optional-change API cases stopped in test-harness setup before test bodies ran. These are not claimed as passing locally.
Verified discovery using the actual CI Desktop/WPF/Integration/OS filter: 13/13 managed and 7/7 API tests are selected. The new managed methods explicitly select WPF rather than inheriting their class's UAP-only metadata.
build/WinUI-GitHub-PR.ymlenables full validation by default and includes the RunTests stage. Its DevTestSuite selects bothMicrosoft.UI.Xaml.Tests.Managed.*.dllandMicrosoft.UI.Xaml.Tests.External.*.dllfor WPF. The WinUI-GitHub-PR (OneBranch) check is the relevant runtime gate; the lightweight GitHub Actions PR Build jobs do not run these tests.Screenshots (if appropriate)
No visual recording captured. Regression coverage checks notification sequences and unaffected control state directly.