Skip to content

SwipeItems: Implement vector compatibility methods - #23885

Open
morning4coffe-dev wants to merge 4 commits into
masterfrom
agents/winui-port-swipeitems-apis
Open

SwipeItems: Implement vector compatibility methods#23885
morning4coffe-dev wants to merge 4 commits into
masterfrom
agents/winui-port-swipeitems-apis

Conversation

@morning4coffe-dev

@morning4coffe-dev morning4coffe-dev commented Jul 26, 2026

Copy link
Copy Markdown
Member

GitHub Issue: closes #23882

Fixes #23882

PR Type:

🐞 Bugfix

What changed? 🚀

Implements the throwing SwipeItems.First, GetMany, and ReplaceAll compatibility methods while hiding the non-projected APIs from IntelliSense. It also preserves SwipeMode.Execute validation and routes projected removal through the vector notification path.

PR Checklist ✅

  • 🧪 Added Runtime tests, UI tests, or a manual test sample (4 focused runtime tests pass)
  • 📚 Docs have been added/updated following the documentation template (not applicable)
  • 🖼️ Validated PR Screenshots Compare Test Run (not applicable)
  • ❗ Contains NO breaking changes
  • 👀 Reviewed 2 other open pull requests (optional)

Source fidelity status

Compared against microsoft-ui-xaml main commit b8cfb849061c00df624ebb29ac4727b9e58ea99c. WinUI still leaves First, GetMany, and ReplaceAll as native TODO stubs, and these methods are not part of the projected C# API. This PR therefore remains draft as an Uno compatibility implementation rather than a literal WinUI3 port.

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 3m 6s —— View job


Code Review

  • Gather context and read changed files
  • Review SwipeItems.h.cs changes
  • Review SwipeItems.Uno.cs changes
  • Review SwipeItemsTests.cs (new test file)
  • Post inline comments for findings
  • Update summary

Summary

The implementation is correct overall: First() / GetMany() / ReplaceAll() are wired up properly, the SwipeMode.Execute guard in the Items setter correctly prevents ReplaceAll from corrupting state (it throws before modifying m_items), and routing Remove through RemoveAt to pick up the VectorChanged notification is the right fix.

3 issues found — 1 minor bug, 2 convention gaps:

Severity Count
Minor (misleading message) 1
Style/convention 2

Details (see inline comments):

  1. [Obsolete] message on First() is misleading — "Use GetEnumerator instead." directs callers to an IEnumerator<SwipeItem>, not the first element. Suggest "Use the indexer [0] or LINQ .First() instead." (view →)

  2. Test class/method naming convention — per runtime-tests.md, the class should be Given_SwipeItems and methods should use a When_ prefix. (view →)

  3. Missing [GitHubWorkItem] — all four tests cover issue Port SwipeItems vector compatibility methods #23882 but none are annotated with [GitHubWorkItem] for traceability. (view →)

  4. ReplaceAll not tested for VectorChangedRemoveRaisesVectorChanged explicitly verifies the event for removals; the same assertion is missing after a successful ReplaceAll call. (view →)

Comment thread src/Uno.UI/UI/Xaml/Controls/SwipeControl/SwipeItems.h.cs Outdated
@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23885/wasm-skia-net9/index.html

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 3m 17s —— View job


Code Review

  • Read changed files
  • Check if prior review findings were addressed
  • Review for new issues
  • Update summary

Summary

All four findings from the previous review pass on this commit.

Prior finding Status
[Obsolete] message on First() misleading ✅ Fixed — now says "Use the indexer [0] or LINQ First() instead."
Class/method naming (Given_ / When_) ✅ Fixed — class is Given_SwipeItems, all methods use When_ prefix
Missing [GitHubWorkItem] on all 4 tests ✅ Fixed — all 4 tests carry the #23882 annotation
VectorChanged not asserted after ReplaceAll ✅ Fixed — When_CompatibilityVectorMethodsWork now subscribes before calling ReplaceAll and asserts changeCount == 1

No new issues found. Implementation logic is correct:

  • First() delegates to GetAt(0), which throws IndexOutOfRangeException on empty — test verified.
  • GetMany boundary check (startIndex > Count throws, startIndex == Count returns 0) is WinRT-correct.
  • ReplaceAll routes through the Items setter, which guards the Execute+multi-item invariant before mutating m_items — so a rejected call leaves state and event count unchanged.
  • Remove now routes through RemoveAt to pick up the VectorChanged notification — the new When_RemoveRaisesVectorChanged test covers this.

Ready to ship.

@morning4coffe-dev morning4coffe-dev changed the title SwipeItems: Complete vector compatibility methods SwipeItems: Implement vector compatibility methods Jul 27, 2026
@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23885/wasm-skia-net9/index.html

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@morning4coffe-dev
morning4coffe-dev marked this pull request as ready for review July 29, 2026 11:01
Copilot AI review requested due to automatic review settings July 29, 2026 11:01
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 implements Uno-only compatibility implementations for SwipeItems.First, GetMany, and ReplaceAll (previously throwing), while hiding them from IntelliSense and ensuring vector-change notifications are raised consistently (notably for Remove and ReplaceAll). It also adds focused runtime-test coverage for these compatibility behaviors.

Changes:

  • Implement SwipeItems.First, GetMany, and ReplaceAll as Uno compatibility methods and hide them via [EditorBrowsable(Never)] + [Obsolete].
  • Route SwipeItems.Remove(SwipeItem) through RemoveAt to ensure VectorChanged is raised.
  • Add runtime tests validating method hiding, functional behavior, Execute-mode validation behavior, and vector change notifications.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/Uno.UI/UI/Xaml/Controls/SwipeControl/SwipeItems.Uno.cs Updates Remove to trigger vector notifications via RemoveAt.
src/Uno.UI/UI/Xaml/Controls/SwipeControl/SwipeItems.h.cs Implements and hides compatibility vector methods (First, GetMany, ReplaceAll).
src/Uno.UI.RuntimeTests/MUX/Windows_UI_Xaml_Controls/SwipeControl/SwipeItemsTests.cs Adds runtime tests for compatibility APIs, including visibility and notification behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +4 to +9
using System;
using System.ComponentModel;
using System.Linq;
using System.Reflection;
using Microsoft.UI.Xaml.Controls;

@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23885/wasm-skia-net9/index.html

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 9m 38s —— View job


Code Review

  • Gather context and read changed files
  • Review SwipeItems.h.cs changes
  • Review SwipeItems.Uno.cs changes
  • Review SwipeItemsTests.cs test file
  • Verify Copilot finding about missing using Private.Infrastructure;
  • Update summary

Summary

Verdict: ready to ship. All prior review findings were addressed and no new issues are found.

The implementation is correct:

  • First() delegates to GetAt(0), throws IndexOutOfRangeException on empty — correct WinRT-style behavior.
  • GetMany boundary check (startIndex > Count throws, startIndex == Count returns 0) is WinRT-correct. The uint/int comparison promotes both operands to long, so there's no overflow risk after the bounds check validates startIndex ≤ m_items.Count ≤ int.MaxValue.
  • ReplaceAll routes through the Items setter which: (1) validates the Execute-mode guard before mutating state, (2) replaces m_items, (3) fires VectorChanged exactly once — matching the test assertion.
  • Remove now routes through RemoveAt(uint) to pick up the VectorChanged notification instead of calling m_items.Remove directly.

Regarding the Copilot inline comment (line 9 of SwipeItemsTests.cs) claiming a missing using Private.Infrastructure;:

This is a false positive. Private.Infrastructure contains only TestServices (with WindowHelper, helpers for UI-thread dispatch, etc.) — it does not define any attributes. [GitHubWorkItem], [PlatformCondition], and RuntimeTestPlatforms all live in Microsoft.VisualStudio.TestTools.UnitTesting (MSTest 4.2.1), which is globally imported via GlobalUsings.cs. [RunsOnUIThread] is in Uno.UI.RuntimeTests, accessible as the parent namespace of the file's own namespace. Comparable files such as Given_Ellipse.cs use [GitHubWorkItem] without using Private.Infrastructure; and compile correctly — the same applies here.

Area Status
First / GetMany / ReplaceAll implementation ✅ Correct
[EditorBrowsable(Never)] + [Obsolete] decoration ✅ Correct
RemoveRemoveAt routing for VectorChanged ✅ Correct
Execute-mode guard preserved across ReplaceAll ✅ Correct
Test naming (Given_ / When_) ✅ Correct
[GitHubWorkItem] on all 4 tests ✅ Correct
VectorChanged asserted after ReplaceAll ✅ Correct
Missing using Private.Infrastructure; (Copilot) ✅ False positive — not needed

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 838452d3-d864-4283-8690-7260d18c0672
Copilot AI review requested due to automatic review settings July 30, 2026 19:47
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @morning4coffe-dev's task in 3m 16s —— View job


Code Review

  • Gather context
  • Read changed files
  • Review implementation
  • Review tests (including 2 new tests in this commit)
  • Verify no new issues
  • Update summary

Summary

Verdict: ready to ship. This commit adds two additional tests (When_ReplaceAll_Then_OutstandingEnumeratorIsInvalidated and When_ReplaceAllWithEmptyArray_Then_EnumeratorIsInvalidatedAndResetRaised) that are correct and valuable.

New tests verified

Both new tests cover important enumerator-invalidation semantics of ReplaceAll:

  • When_ReplaceAll_Then_OutstandingEnumeratorIsInvalidated — verifies that calling ReplaceAll while an enumerator is in flight (mid-MoveNext) correctly throws InvalidOperationException on the next MoveNext call. This works because ReplaceAll mutates the underlying ObservableCollection<T> in-place (via m_items.Clear()), which increments the internal List<T>._version counter and invalidates all outstanding List<T>.Enumerator instances. ✓
  • When_ReplaceAllWithEmptyArray_Then_EnumeratorIsInvalidatedAndResetRaised — verifies that ReplaceAll([]) (empty array) invalidates enumerators and still fires VectorChanged with CollectionChange.Reset. ✓

Both new tests carry [GitHubWorkItem], [RunsOnUIThread], and [PlatformCondition(ConditionMode.Exclude, NativeWinUI)]. All assertions are correctly inside #if HAS_UNO. ✓

All prior findings remain addressed

Area Status
First / GetMany / ReplaceAll implementation ✅ Correct
[EditorBrowsable(Never)] + [Obsolete] decoration ✅ Correct
RemoveRemoveAt routing for VectorChanged ✅ Correct
Execute-mode guard preserved and tested ✅ Correct
VectorChangedEventArgs accessibility (internal via InternalsVisibleTo) ✅ Correct
Test naming (Given_ / When_) ✅ Correct
[GitHubWorkItem] on all 6 tests ✅ Correct
VectorChanged asserted after ReplaceAll ✅ Correct
Enumerator invalidation by ReplaceAll ✅ Correct (new tests)
Missing using Private.Infrastructure; (Copilot, prior review) ✅ False positive — confirmed not needed

No new issues found.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@unodevops

Copy link
Copy Markdown
Contributor

🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23885/wasm-skia-net9/index.html

@@ -0,0 +1,185 @@
// Copyright (c) Microsoft Corporation. All rights reserved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wait, are these tests actually part of winui code?

namespace Uno.UI.RuntimeTests.MUX.Windows_UI_Xaml_Controls.SwipeControl;

[TestClass]
public class Given_SwipeItems

@Xiaoy312 Xiaoy312 Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

pick a lane: typename and filename shouild be consistent

[PlatformCondition(ConditionMode.Exclude, RuntimeTestPlatforms.NativeWinUI)]
public void When_CompatibilityVectorMethodsAreHidden()
{
#if HAS_UNO

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

instead of doing this on every method
maybe rename file to SwipeItemsTests.Compatibility.cs (& make the class partial)
and just wrap the entire file in #if HAS_UNO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Port SwipeItems vector compatibility methods

4 participants