Conversation
There was a problem hiding this comment.
Pull request overview
This PR ports WinUI’s ItemCollectionTransition* type system into Uno, adding the core transition descriptor/progress/provider APIs plus a runtime test suite to validate expected add/remove/move transition behavior.
Changes:
- Added
ItemCollectionTransition, related enums, progress type, completed-event args, and the baseItemCollectionTransitionProviderimplementation underUI/Xaml/Controls/Repeater/. - Updated generated stubs to defer to the new handwritten implementations.
- Added runtime tests validating provider callbacks, filtering behavior, and
TransitionCompletedsemantics.
Reviewed changes
Copilot reviewed 7 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionTriggers.cs | Adds [Flags] triggers enum used to describe why transitions occur. |
| src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionOperation.cs | Adds operation enum (Add/Remove/Move) for transition classification. |
| src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransition.cs | Adds transition descriptor object + Start()/HasStarted. |
| src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProgress.cs | Adds progress object used to signal completion back to the provider. |
| src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionCompletedEventArgs.cs | Adds event args for the provider’s TransitionCompleted event. |
| src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProvider.cs | Adds provider batching logic using CompositionTarget.Rendering and keep-alive timers. |
| src/Uno.UI/Generated/3.0.0.0/Microsoft.UI.Xaml.Controls/ItemCollectionTransition*.cs | Updates generated stubs to skip members now implemented in Uno sources. |
| src/Uno.UI.RuntimeTests/Tests/Microsoft_UI_Xaml_Controls/Given_ItemCollectionTransitionProvider.cs | Adds runtime tests for transition provider call patterns and completion event behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProvider.cs
Outdated
Show resolved
Hide resolved
src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProvider.cs
Outdated
Show resolved
Hide resolved
src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProvider.cs
Outdated
Show resolved
Hide resolved
src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProvider.cs
Outdated
Show resolved
Hide resolved
src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProvider.cs
Outdated
Show resolved
Hide resolved
src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProvider.cs
Outdated
Show resolved
Hide resolved
src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProgress.cs
Outdated
Show resolved
Hide resolved
src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProgress.cs
Outdated
Show resolved
Hide resolved
...o.UI.RuntimeTests/Tests/Microsoft_UI_Xaml_Controls/Given_ItemCollectionTransitionProvider.cs
Show resolved
Hide resolved
src/Uno.UI/UI/Xaml/Controls/Repeater/ItemCollectionTransitionProvider.cs
Outdated
Show resolved
Hide resolved
morning4coffe-dev
left a comment
There was a problem hiding this comment.
LGTM, lets just recheck with WinUI3 source please
a51ff01 to
a171406
Compare
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-22960/docs/index.html |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (usesNewTransitionsBatch) | ||
| { | ||
| // Allocating a new array of ItemCollectionTransition with animations for this new batch. | ||
| _transitionsWithAnimationsMap[_transitionsBatch] = new List<ItemCollectionTransition>(); | ||
| } | ||
|
|
||
| _transitionsWithAnimationsMap[_transitionsBatch].Add(transition); |
There was a problem hiding this comment.
QueueTransition can throw KeyNotFoundException when the first transition in a new batch is not animated: _transitionsWithAnimationsMap[_transitionsBatch] is only initialized when usesNewTransitionsBatch is true and the current transition is animated. If the first transition(s) are filtered out by ShouldAnimate, a later animated transition will attempt to .Add into a missing list. Initialize the list on-demand (e.g., TryGetValue/create) before adding, independent of usesNewTransitionsBatch.
| if (usesNewTransitionsBatch) | |
| { | |
| // Allocating a new array of ItemCollectionTransition with animations for this new batch. | |
| _transitionsWithAnimationsMap[_transitionsBatch] = new List<ItemCollectionTransition>(); | |
| } | |
| _transitionsWithAnimationsMap[_transitionsBatch].Add(transition); | |
| if (!_transitionsWithAnimationsMap.TryGetValue(_transitionsBatch, out var transitionsWithAnimations)) | |
| { | |
| // Allocating a new array of ItemCollectionTransition with animations for this batch. | |
| transitionsWithAnimations = new List<ItemCollectionTransition>(); | |
| _transitionsWithAnimationsMap[_transitionsBatch] = transitionsWithAnimations; | |
| } | |
| transitionsWithAnimations.Add(transition); |
| // Uno does not support cleanup via finalizers; consumers should call Dispose() or stop | ||
| // using the provider when done. The timers will be collected when the provider is GC'd. |
There was a problem hiding this comment.
The #if HAS_UNO note mentions consumers should call Dispose(), but ItemCollectionTransitionProvider does not implement IDisposable or expose Dispose. This is misleading API guidance; either remove the Dispose() mention or implement deterministic cleanup (stop timers + detach handlers).
| // Uno does not support cleanup via finalizers; consumers should call Dispose() or stop | |
| // using the provider when done. The timers will be collected when the provider is GC'd. | |
| // Uno does not support this destructor-style cleanup. When the provider is no longer used, | |
| // any associated timers become eligible for collection when the provider is GC'd. |
| private readonly ItemCollectionTransition _transition; | ||
|
|
||
| /// <summary> | ||
| /// Gets the ItemCollectionTransition whose animations are in progress. | ||
| /// </summary> | ||
| public ItemCollectionTransition Transition => _transition; |
There was a problem hiding this comment.
_transition is declared readonly, so it can never be cleared. This prevents making Complete() single-shot and also means ItemCollectionTransitionProgress will keep the transition/provider/element graph alive as long as the progress object is referenced. If you intend Complete() to be idempotent and release references, _transition needs to be mutable (or guarded via a separate completion flag + clearable reference).
| private readonly ItemCollectionTransition _transition; | |
| /// <summary> | |
| /// Gets the ItemCollectionTransition whose animations are in progress. | |
| /// </summary> | |
| public ItemCollectionTransition Transition => _transition; | |
| // The reference is intentionally mutable so completion logic can clear it and release the transition graph. | |
| private ItemCollectionTransition? _transition; | |
| /// <summary> | |
| /// Gets the ItemCollectionTransition whose animations are in progress. | |
| /// </summary> | |
| public ItemCollectionTransition? Transition => _transition; |
| public void Complete() | ||
| { | ||
| if (_transition is not null) | ||
| { | ||
| var transitionProvider = _transition.OwningProvider(); | ||
| transitionProvider?.NotifyTransitionCompleted(_transition); | ||
| } |
There was a problem hiding this comment.
Complete() is not idempotent: calling it multiple times will raise TransitionCompleted multiple times because _transition is never cleared/guarded. Add a single-shot guard (e.g., set the stored transition reference to null after the first call, or track a _completed flag) so completion is raised at most once and references can be released.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22960/wasm-skia-net9/index.html |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-22960/docs/index.html |
GitHub Issue: closes #22904
PR Type: ✨ Feature
What is the current behavior? 🤔
Support for
ItemCollectionTransitionmissingWhat is the new behavior? 🚀
Support for
ItemCollectionTransitionaddedPR Checklist ✅
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Runresults.Other information ℹ️