[Android] Fix for CurrentItem is updated incorrectly when the CarouselView is bound to an ObservableCollection with Loop = false - #36243
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36243Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36243" |
dotnet-policy-service
Bot
added
the
partner/syncfusion
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
area-controls-collectionview
GitHub Actions
Inactive
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
MauiBot
added
s/agent-gate-passed
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
MauiBot
removed
the
s/agent-review-in-progress
I've reviewed the AI summary comments. Please find the following details.
|
BagavathiPerumal
changed the title
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
MauiBot
removed
the
s/agent-review-in-progress
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
The test failures mentioned in the AI summary are not present in the latest CI run. The only remaining CollectionView-related failure reported in the latest CI could not be reproduced locally and is not related to this fix. |
… -1 when an unequal-count Replace operation empties the collection. This prevents an invalid negative position from being passed to UpdatePosition and ScrollToPosition, ensuring a valid carousel state.
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@BagavathiPerumal — new AI review results are available based on this last commit:
2ea358c. To request a fresh review after new comments or commits, comment/review rerun.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ✅ PASSED
Platform: ANDROID · Base: main · Merge base: 0395a53b
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🖥️ Issue35643 Issue35643 |
✅ FAIL — 2522s | ✅ PASS — 586s |
🔴 Without fix — 🖥️ Issue35643: FAIL ✅ · 2522s
Error-relevant lines (filtered from the build log):
/home/vsts/work/1/s/.dotnet/packs/Microsoft.Android.Sdk.Linux/36.1.2/tools/Xamarin.Android.Common.Debugging.targets(333,5): error ADB0010: Mono.AndroidTools.InstallFailedException: Unexpected install output: cmd: Failure calling service package: Broken pipe (32) [/home/vsts/work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-android]
/home/vsts/work/1/s/.dotnet/packs/Microsoft.Android.Sdk.Linux/36.1.2/tools/Xamarin.Android.Common.Debugging.targets(333,5): error ADB0010: [/home/vsts/work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-android]
/home/vsts/work/1/s/.dotnet/packs/Microsoft.Android.Sdk.Linux/36.1.2/tools/Xamarin.Android.Common.Debugging.targets(333,5): error ADB0010: at Mono.AndroidTools.Internal.AdbOutputParsing.CheckInstallSuccess(String output, String packageName) [/home/vsts/work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-android]
/home/vsts/work/1/s/.dotnet/packs/Microsoft.Android.Sdk.Linux/36.1.2/tools/Xamarin.Android.Common.Debugging.targets(333,5): error ADB0010: at Mono.AndroidTools.AndroidDevice.<>c__DisplayClass105_0.<InstallPackage>b__0(Task`1 t) [/home/vsts/work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-android]
/home/vsts/work/1/s/.dotnet/packs/Microsoft.Android.Sdk.Linux/36.1.2/tools/Xamarin.Android.Common.Debugging.targets(333,5): error ADB0010: at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) [/home/vsts/work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-android]
/home/vsts/work/1/s/.dotnet/packs/Microsoft.Android.Sdk.Linux/36.1.2/tools/Xamarin.Android.Common.Debugging.targets(333,5): error ADB0010: --- End of stack trace from previous location --- [/home/vsts/work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-android]
/home/vsts/work/1/s/.dotnet/packs/Microsoft.Android.Sdk.Linux/36.1.2/tools/Xamarin.Android.Common.Debugging.targets(333,5): error ADB0010: at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread) [/home/vsts/work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-android]
/home/vsts/work/1/s/.dotnet/packs/Microsoft.Android.Sdk.Linux/36.1.2/tools/Xamarin.Android.Common.Debugging.targets(333,5): error ADB0010: at AndroidDeviceExtensions.PushAndInstallPackageAsync(AndroidDevice device, PushAndInstallCommand command, CancellationToken token) [/home/vsts/work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-android]
/home/vsts/work/1/s/.dotnet/packs/Microsoft.Android.Sdk.Linux/36.1.2/tools/Xamarin.Android.Common.Debugging.targets(333,5): error ADB0010: at Xamarin.Android.Tasks.FastDeploy.InstallPackage(Boolean installed) [/home/vsts/work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-android]
/home/vsts/work/1/s/.dotnet/packs/Microsoft.Android.Sdk.Linux/36.1.2/tools/Xamarin.Android.Common.Debugging.targets(333,5): error ADB0010: at Xamarin.Android.Tasks.FastDeploy.RunInstall() [/home/vsts/work/1/s/src/Controls/tests/TestCases.HostApp/Controls.TestCases.HostApp.csproj::TargetFramework=net10.0-android]
Build FAILED.
at Microsoft.Maui.TestCases.Tests.Issues.Issue35643.CurrentItemShouldUpdateWhenCurrentItemIsReplaced() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35643.cs:line 31
at Microsoft.Maui.TestCases.Tests.Issues.Issue35643.CurrentItemShouldUpdateWhenCurrentItemIsReplacedInLoopMode() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35643.cs:line 56
🟢 With fix — 🖥️ Issue35643: PASS ✅ · 586s
(no coded error found; showing last 1200 chars)
Tests.dll
NUnit3TestExecutor discovered 2 of 2 NUnit test cases using Current Discovery mode, Non-Explicit run
>>>>> 07/15/2026 16:25:39 FixtureSetup for Issue35643(Android)
>>>>> 07/15/2026 16:25:42 CurrentItemShouldUpdateWhenCurrentItemIsReplaced Start
>>>>> 07/15/2026 16:25:48 CurrentItemShouldUpdateWhenCurrentItemIsReplaced Stop
Passed CurrentItemShouldUpdateWhenCurrentItemIsReplaced [6 s]
>>>>> 07/15/2026 16:25:49 CurrentItemShouldUpdateWhenCurrentItemIsReplacedInLoopMode Start
>>>>> 07/15/2026 16:25:53 CurrentItemShouldUpdateWhenCurrentItemIsReplacedInLoopMode Stop
Passed CurrentItemShouldUpdateWhenCurrentItemIsReplacedInLoopMode [4 s]
NUnit Adapter 4.5.0.0: Test execution complete
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.13] Discovering: Controls.TestCases.Android.Tests
[xUnit.net 00:00:00.50] Discovered: Controls.TestCases.Android.Tests
Results File: /home/vsts/work/1/s/CustomAgentLogsTmp/UITests/TestResults/Issue35643.trx
Test Run Successful.
Total tests: 2
Passed: 2
Total time: 25.4401 Seconds
>>> TRX_RESULT_FILE: /home/vsts/work/1/s/CustomAgentLogsTmp/UITests/TestResults/Issue35643.trx
📁 Fix files reverted (1 files)
src/Controls/src/Core/Handlers/Items/Android/MauiCarouselRecyclerView.cs
📱 UI Tests — CarouselView,CollectionView
Detected UI test categories: CarouselView,CollectionView
CarouselView, CollectionView) was interrupted before producing results — most often the per-category time budget (a very long-running category) was exhausted, or an emulator/Appium hang. This is usually an infrastructure/timeout issue rather than a code problem; re-run the review to try again. See the build-output.log in the drop-deep-uitests artifact.
📋 Pre-Flight — Context & Validation
Issue: #35643 - CurrentItem is updated incorrectly on Android when the CarouselView is bound to an ObservableCollection with Loop = false
PR: #36243 - [Android] Fix for CurrentItem is updated incorrectly when the CarouselView is bound to an ObservableCollection with Loop = false
Platforms Affected: Android
Files Changed: 1 implementation, 2 test
Key Findings
- Issue #35643 is a verified Android regression introduced in 9.0.60: replacing the currently selected
CarouselViewitem while also updatingCurrentItemresets Android non-loop selection to item 0 instead of the replacement item. - PR #36243 adds a dedicated Android equal-count
Replacepath inMauiCarouselRecyclerView.CollectionItemsSourceChanged, plus UI tests for loop and non-loopCarouselViewreplacement. - The PR's implementation avoids falling through to removal/scroll-mode logic and includes loop-mode visible virtual-cell rebinding, but code review found a deferred-dispatch teardown race in the new callbacks.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: low
Errors: 1 | Warnings: 1 | Suggestions: 0
Key code review findings:
- ✗
src/Controls/src/Core/Handlers/Items/Android/MauiCarouselRecyclerView.cs:568and:606— dispatched Replace callbacks can run after handler teardown and dereferenceCarouselthroughSetCurrentItem,UpdatePosition, orUpdateVisualStateswithout revalidating lifecycle state. - ⚠
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35643.cs— tests cover the main equal-count single-item Replace path, but not adjacent unequal-count, indexless Replace, or teardown/disconnect safety branches.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #36243 | Dedicated Android equal-count Replace handler preserving position/current item, plus loop-mode visible virtual-cell rebind without adapter rebuild | ✅ PASSED (Gate) | src/Controls/src/Core/Handlers/Items/Android/MauiCarouselRecyclerView.cs, src/Controls/tests/TestCases.HostApp/Issues/Issue35643.cs, src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue35643.cs |
Original PR; gate result supplied by caller |
🔬 Code Review — Deep Analysis
Code Review — PR #36243
Independent Assessment
What this changes: Adds Android CarouselView handling for equal-count NotifyCollectionChangedAction.Replace, preserving Position/CurrentItem and rebinding visible loop-mode virtual cells without a full adapter rebuild. Adds Android UI coverage for loop and non-loop replacement.
Inferred motivation: Replacing the selected item currently looks like removal because the old CurrentItem no longer exists, causing position/current item reset behavior.
Reconciliation with PR Narrative
Author claims: Android-only fix for issue #35643; preserves replace position/current item, with special loop-mode virtual-cell refresh.
Agreement/disagreement: The intent matches the code. However, the new dispatched callbacks still have handler teardown races despite the PR’s lifecycle guard.
Prior Review Reconciliation
No prior ❌ Error findings found.
Notable prior [major]/[moderate] findings were mostly addressed:
- Missing loop-mode test: ✅ fixed.
- Range replace rebind: ✅ fixed.
- Unequal replace clamp: ✅ fixed.
- Indexless replace: ✅ mostly addressed.
- Latest UI-thread adapter-notification comments: likely obsolete because
ObservableItemsSource.CollectionChangeddispatches via_container.Dispatcher.DispatchIfRequired(...)before raisingCollectionItemsSourceChanged.
Blast Radius Assessment
- Runs for all instances: No; only Android
CarouselViewcollection changes. - Startup impact: No.
- Static/shared state: No.
- Handler/platform plumbing: Yes — confidence capped at medium, then CI red caps to low.
CI Status
- Required-check result:
gh pr checks --requiredunavailable due unauthenticatedgh; fallback GitHub Checks API showsmaui-prfailed. - Classification: CI failure appears PR-unrelated infra. AzDO build
1510132failed inRunOniOS_BlazorDebug ARM64becauseInstall Simulator Runtimestimed out while downloading iOS 26.0 simulator; unrelated to Android CarouselView code. - Action taken: Invoked
azdo-build-investigator; confidence capped low because CI is red/tool fallback was required.
Findings
❌ Error — Deferred Replace callbacks can still run after handler teardown
CollectionItemsSourceChanged guards Carousel?.Handler?.MauiContext before entering HandleReplaceAction, but both new dispatcher callbacks dereference Carousel later without revalidating lifecycle state.
Affected lines:
src/Controls/src/Core/Handlers/Items/Android/MauiCarouselRecyclerView.cs:568src/Controls/src/Core/Handlers/Items/Android/MauiCarouselRecyclerView.cs:606
If TearDownOldElement/handler disconnect happens after Dispatch(...) queues but before the callback executes, ItemsView/Carousel can be null. Then SetCurrentItem, UpdatePosition, or UpdateVisualStates can throw. Re-check Carousel?.Handler?.MauiContext inside each dispatched lambda before touching carousel state, while still resetting _isInternalPositionUpdate/_noNeedForScroll in finally.
⚠️ Warning — Adjacent Replace edge cases remain untested
The tests cover the main equal-count single-item replace path, including loop mode visible-cell rebind. They do not cover adjacent branches added by this PR: unequal-count replace clamping, indexless replace fallback, or teardown/disconnect safety. Given the handler complexity, at least one lighter adjacent test would reduce regression risk.
Failure-Mode Probing
- Handler disconnect after dispatch: currently unsafe; callback can dereference
Carouselafter teardown. - Dispatch refuses to queue: flags are reset; handled.
- Empty collection after unequal replace: clamped to
0; avoidsScrollToPosition(-1). - Multi-item equal replace in loop mode: iterates replaced range; addressed.
- Background collection change:
ObservableItemsSourcemarshals via dispatcher before raising this handler event.
Verdict: NEEDS_CHANGES
Confidence: low
Summary: The fix addresses the reported behavior and prior review concerns, but the new deferred callbacks still have a concrete Android handler lifecycle race. CI is also currently red, though the observed failure appears infrastructure-related rather than PR-caused.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix-1 / maui-expert-reviewer | Synchronous count-preserving Replace reconciliation; no deferred dispatch; NotifyDataSetChanged() only for loop-mode virtual cell refresh |
✅ PASS | 1 file | Simpler than PR fix and eliminates the lifecycle race found in pre-flight review |
| PR | PR #36243 | Dedicated equal-count Replace handler with deferred callbacks and targeted visible loop virtual-cell rebind | ✅ PASSED (Gate) | 3 files | Original PR; gate result supplied by caller |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Proposed synchronous count-preserving Replace reconciliation as a different root-cause/implementation approach |
Exhausted: No — stopped after Candidate #1 because it passed the Android regression tests and is demonstrably simpler while removing the PR fix's deferred-callback lifecycle race.
Selected Fix: Candidate #1 — passes the same target tests, changes only the Android implementation file, removes the queued dispatch teardown hazard, and has a substantially smaller implementation. Tradeoff: loop-mode refresh is coarser than the PR's targeted virtual-cell notification.
Environment notes: gh is unauthenticated in this environment, so GitHub CLI PR queries were replaced with public GitHub API reads. EstablishBrokenBaseline.ps1 could not establish a broken baseline because the repository already contained unrelated dirty .github/eng files; the attempt continued by changing only src/Controls/src/Core/Handlers/Items/Android/MauiCarouselRecyclerView.cs and capturing the diff separately.
📝 Recommended PR Title & Description
Assessment: ✏️ Recommend updating — the current description is strong, but the winning fix adds deferred-callback lifecycle revalidation that is not reflected, and the title can be made shorter and more component-focused.
Recommended title
[Android] CarouselView: Preserve CurrentItem when replacing the selected item
Recommended description
### Issue details
The issue occurs on Android when replacing an item in the CarouselView.ItemsSource while Loop is set to false and simultaneously updating the CurrentItem. If the replaced item is the currently selected item, CarouselView incorrectly resets the selection to the first item instead of preserving the updated item as the current selection.
### Root Cause
When ItemsUpdatingScrollMode support was added to CarouselView collection change handling on Android, the change correctly implemented scroll-mode logic for Add, Remove, and Reset actions but did not add a dedicated path for the Replace action. This caused Replace to fall through into the KeepItemsInView/KeepLastItemInView scroll logic.
GetPosition(CurrentItem) always returns -1 after a Replace (old item gone from source), making removingCurrentElement = true and triggering a position reset to 0.
### Description of Change
The fix involves intercepting the Replace action at the top of CollectionItemsSourceChanged — before any Remove/Insert flags are computed — and delegating to a dedicated HandleReplaceAction method, ensuring Replace never reaches the KeepItemsInView reset logic.
For non-loop mode, SetCurrentItem and UpdatePosition are dispatched at the preserved position with no scroll. For loop mode, a new RebindVisibleLoopItem helper surgically rebinds only the visible virtual cells that map to the replaced real index, avoiding a full adapter rebuild that previously caused a flash to position 0 and spurious events. The deferred Replace callbacks also re-check Carousel.Handler.MauiContext before applying state updates so handler teardown between dispatch and callback execution does not dereference a disconnected CarouselView. All existing Remove, Reset, and Insert behavior is fully preserved.
The Android UI tests cover both Loop=false and Loop=true replacement. The loop-mode test verifies the visible carousel cell is rebound to the replacement item and scrolls the LoopCarouselView back into view before querying the cell AutomationId so RecyclerView does not recycle the target cell off-screen.
### Regression PR
PR https://github.com/dotnet/maui/pull/26608
Tested the behavior in the following platforms.
- [x] Android
- [ ] Mac
- [ ] iOS
- [ ] Windows
### Issues Fixed
Fixes https://github.com/dotnet/maui/issues/35643
### Output
|Platform|Before|After|
|--|--|--|
|Android|<video src="https://github.com/user-attachments/assets/093a0324-9102-400f-944e-56a11565acc4"> |<video src="https://github.com/user-attachments/assets/08ebeb5c-59c3-4640-af15-530053c0cebb">|
🏁 Report — Final Recommendation
Comparative Report — PR #36243
Candidates compared
| Rank | Candidate | Regression result | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
Not separately re-run; preserves the raw PR's passing regression path and only adds lifecycle guards inside the new Replace callbacks plus a test scroll-back before asserting the visible loop cell | Best overall. Keeps the PR's comprehensive equal-count Replace handling, loop-mode targeted visible-cell rebind, and Android UI tests, while fixing the actionable deferred-dispatch teardown race and small-screen test reliability issue. |
| 2 | try-fix-1 |
✅ PASS | Strong alternative. Simpler synchronous count-preserving Replace reconciliation removes the deferred callback race entirely and passed the Issue35643 Android UI tests. Ranked below pr-plus-reviewer because it is coarser for loop mode (NotifyDataSetChanged) and only handles equal-count Replace when the current position is inside the replaced range, whereas the PR path consistently short-circuits equal-count Replace and preserves current carousel state. |
| 3 | pr |
✅ PASSED gate | Functionally addresses the reported regression and includes good UI coverage, but remains below the other passing candidates because expert review identified an actionable lifecycle race in the newly queued Replace callbacks and a loop-mode test reliability issue after scrolling to the button. |
No candidate failed the supplied regression gate. Per the ranking rule, a regression-failing candidate would be ranked below all passing candidates; that condition did not apply here.
Winning candidate
pr-plus-reviewer
Rationale
The raw PR fix correctly identifies the root cause: Android equal-count Replace was falling through into removal / scroll-mode logic because the old CurrentItem no longer exists in the source. pr-plus-reviewer preserves that fix and the PR's loop-mode virtual-cell rebind strategy, while applying the expert reviewer's concrete lifecycle feedback by re-checking Carousel?.Handler?.MauiContext inside each deferred Replace callback before touching carousel state. It also applies the reviewer test feedback by scrolling LoopCarouselView back into view before asserting the visible rebound cell.
Compared with try-fix-1, pr-plus-reviewer has slightly more implementation complexity, but it retains the PR's more precise loop-mode updates and broader equal-count Replace handling. The added reviewer changes are surgical and do not alter the successful regression behavior except in unsafe/disconnected handler states or when stabilizing the UI test's visibility precondition.
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
MauiBot
removed
the
s/agent-review-in-progress
The deep UI test loop previously bounded itself at a 210-min internal hard stop under a 220-min AzDO task timeout / 240-min job timeout. For heavy, genuinely long-running categories (e.g. CarouselView + CollectionView on PR #36243, or a lone ~391-test CollectionView) the per-category fair-share slice was too small — two heavy categories got only ~105 min each and were cut off mid-run, then mis-reported as an infra timeout ("interrupted before producing results"). Raise the whole budget stack to the Microsoft-hosted-agent maximum (a 360-min per-job ceiling that Android/Windows Azure-Pipelines agents cannot exceed): - deep JOB timeoutInMinutes 240 -> 360 (the platform hard ceiling) - per-category-loop STEP timeout 220 -> 320 - $budgetMin (stop starting new) 185 -> 270 - $hardStopMin (whole-loop stop) 210 -> 300 - $perCatCapMin (single-cat cap) 150 -> 290 (~4.8 h for a lone category) - $perCatIdleMin (hang-killer) 25 -> 25 (unchanged) Effect on the per-category fair-share math (min(perCatCap, max(min(50, remain), floor(remain / catsRemaining)))): 1 category -> 290 min (~4.8 h) 2 categories-> 150 min each (was ~105 — the #36243 case) 3 categories-> 100 min each 6/12 cats -> 50 min floor each The hierarchy stays safe: loop hard stop 300 < step 320 < job 360, and the non-loop headroom (pre-loop workload install / PR-merge + post-loop publish) actually GROWS from 30 to 60 min, so partial results always flush before any hard cancel. The 25-min idle no-progress tree-kill is deliberately left unchanged: a genuinely hung build/run is still killed within 25 min of silence, so the larger wall-clock ceiling only ever benefits a category that is actively producing test output — it can never let a hang waste hours. Note: Android/Windows use Microsoft-hosted agents and cannot exceed 360 min; only the self-hosted iOS/Mac (AcesShared) pools could ever go higher, which would require templating the job timeout per-platform. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15d2af20-e4ab-4e88-9011-cfbd83513bc0
… and non-Loop Replace dispatched callbacks to prevent a crash during a teardown race.
I've reviewed all the outstanding concerns from the AI review and addressed them as follows:
|
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
Tests Failure Analysis
Test Failure Review: Not ready - click to expandOverall verdict: Not ready. Of 11 distinct failures, 2 legs are deterministic regressions vs the base branch (
Coverage: 162 checks · 155 passing · 7 failing · 0 pending · 0 inaccessible · 1 unmapped · 13 unexplained build legs · 0 unaccounted failing checks · 2 aborted failing checks · 0 canceled-build checks · 0 device-test unverified · 9 unattributed · 2 regressed-vs-base. Deterministic ceiling: Not ready — 2 legs regressed vs base plus unexplained/aborted legs and unattributed failures. Builds (this PR): maui-pr-uitests 1512303, maui-pr-devicetests 1512304. Base sampling (main, 5 recent builds per definition): 1503618, 1503332, 1503036. Recommended actionHave a human inspect the |
I have reviewed the reported failures and confirmed that they are unrelated to this fix. |
…lView is bound to an ObservableCollection with Loop = false (#36243) > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Issue details The issue occurs on Android when replacing an item in the CarouselView.ItemsSource while Loop is set to false and simultaneously updating the CurrentItem. If the replaced item is the currently selected item, CarouselView incorrectly resets the selection to the first item instead of preserving the updated item as the current selection. ### Root Cause When ItemsUpdatingScrollMode support was added to CarouselView collection change handling on Android, the change correctly implemented scroll-mode logic for Add, Remove, and Reset actions but did not add a dedicated path for the Replace action. This caused Replace to fall through into the KeepItemsInView/KeepLastItemInView scroll logic. GetPosition(CurrentItem) always returns -1 after a Replace (old item gone from source), making removingCurrentElement = true and triggering a position reset to 0. ### Description of Change The fix involves intercepting the Replace action at the top of CollectionItemsSourceChanged — before any Remove/Insert flags are computed — and delegating to a dedicated HandleReplaceAction method, ensuring Replace never reaches the KeepItemsInView reset logic. For non-loop mode, SetCurrentItem and UpdatePosition are dispatched at the preserved position with no scroll. For loop mode, a new RebindVisibleLoopItem helper surgically rebinds only the visible virtual cells that map to the replaced real index, avoiding a full adapter rebuild that previously caused a flash to position 0 and spurious events. All existing Remove, Reset, and Insert behavior is fully preserved. ### Regression PR PR #26608 Tested the behavior in the following platforms. - [x] Android - [ ] Mac - [ ] iOS - [ ] Windows ### Issues Fixed Fixes #35643 ### Output |Platform|Before|After| |--|--|--| |Android|<video src="https://github.com/user-attachments/assets/093a0324-9102-400f-944e-56a11565acc4"> |<video src="https://github.com/user-attachments/assets/08ebeb5c-59c3-4640-af15-530053c0cebb">|
…lView is bound to an ObservableCollection with Loop = false (#36243) > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Issue details The issue occurs on Android when replacing an item in the CarouselView.ItemsSource while Loop is set to false and simultaneously updating the CurrentItem. If the replaced item is the currently selected item, CarouselView incorrectly resets the selection to the first item instead of preserving the updated item as the current selection. ### Root Cause When ItemsUpdatingScrollMode support was added to CarouselView collection change handling on Android, the change correctly implemented scroll-mode logic for Add, Remove, and Reset actions but did not add a dedicated path for the Replace action. This caused Replace to fall through into the KeepItemsInView/KeepLastItemInView scroll logic. GetPosition(CurrentItem) always returns -1 after a Replace (old item gone from source), making removingCurrentElement = true and triggering a position reset to 0. ### Description of Change The fix involves intercepting the Replace action at the top of CollectionItemsSourceChanged — before any Remove/Insert flags are computed — and delegating to a dedicated HandleReplaceAction method, ensuring Replace never reaches the KeepItemsInView reset logic. For non-loop mode, SetCurrentItem and UpdatePosition are dispatched at the preserved position with no scroll. For loop mode, a new RebindVisibleLoopItem helper surgically rebinds only the visible virtual cells that map to the replaced real index, avoiding a full adapter rebuild that previously caused a flash to position 0 and spurious events. All existing Remove, Reset, and Insert behavior is fully preserved. ### Regression PR PR #26608 Tested the behavior in the following platforms. - [x] Android - [ ] Mac - [ ] iOS - [ ] Windows ### Issues Fixed Fixes #35643 ### Output |Platform|Before|After| |--|--|--| |Android|<video src="https://github.com/user-attachments/assets/093a0324-9102-400f-944e-56a11565acc4"> |<video src="https://github.com/user-attachments/assets/08ebeb5c-59c3-4640-af15-530053c0cebb">|
…lView is bound to an ObservableCollection with Loop = false (#36243) > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Issue details The issue occurs on Android when replacing an item in the CarouselView.ItemsSource while Loop is set to false and simultaneously updating the CurrentItem. If the replaced item is the currently selected item, CarouselView incorrectly resets the selection to the first item instead of preserving the updated item as the current selection. ### Root Cause When ItemsUpdatingScrollMode support was added to CarouselView collection change handling on Android, the change correctly implemented scroll-mode logic for Add, Remove, and Reset actions but did not add a dedicated path for the Replace action. This caused Replace to fall through into the KeepItemsInView/KeepLastItemInView scroll logic. GetPosition(CurrentItem) always returns -1 after a Replace (old item gone from source), making removingCurrentElement = true and triggering a position reset to 0. ### Description of Change The fix involves intercepting the Replace action at the top of CollectionItemsSourceChanged — before any Remove/Insert flags are computed — and delegating to a dedicated HandleReplaceAction method, ensuring Replace never reaches the KeepItemsInView reset logic. For non-loop mode, SetCurrentItem and UpdatePosition are dispatched at the preserved position with no scroll. For loop mode, a new RebindVisibleLoopItem helper surgically rebinds only the visible virtual cells that map to the replaced real index, avoiding a full adapter rebuild that previously caused a flash to position 0 and spurious events. All existing Remove, Reset, and Insert behavior is fully preserved. ### Regression PR PR #26608 Tested the behavior in the following platforms. - [x] Android - [ ] Mac - [ ] iOS - [ ] Windows ### Issues Fixed Fixes #35643 ### Output |Platform|Before|After| |--|--|--| |Android|<video src="https://github.com/user-attachments/assets/093a0324-9102-400f-944e-56a11565acc4"> |<video src="https://github.com/user-attachments/assets/08ebeb5c-59c3-4640-af15-530053c0cebb">|
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
The issue occurs on Android when replacing an item in the CarouselView.ItemsSource while Loop is set to false and simultaneously updating the CurrentItem. If the replaced item is the currently selected item, CarouselView incorrectly resets the selection to the first item instead of preserving the updated item as the current selection.
Root Cause
When ItemsUpdatingScrollMode support was added to CarouselView collection change handling on Android, the change correctly implemented scroll-mode logic for Add, Remove, and Reset actions but did not add a dedicated path for the Replace action. This caused Replace to fall through into the KeepItemsInView/KeepLastItemInView scroll logic.
GetPosition(CurrentItem) always returns -1 after a Replace (old item gone from source), making removingCurrentElement = true and triggering a position reset to 0.
Description of Change
The fix involves intercepting the Replace action at the top of CollectionItemsSourceChanged — before any Remove/Insert flags are computed — and delegating to a dedicated HandleReplaceAction method, ensuring Replace never reaches the KeepItemsInView reset logic.
For non-loop mode, SetCurrentItem and UpdatePosition are dispatched at the preserved position with no scroll. For loop mode, a new RebindVisibleLoopItem helper surgically rebinds only the visible virtual cells that map to the replaced real index, avoiding a full adapter rebuild that previously caused a flash to position 0 and spurious events. All existing Remove, Reset, and Insert behavior is fully preserved.
Regression PR
PR #26608
Tested the behavior in the following platforms.
Issues Fixed
Fixes #35643
Output
35643-BeforeFix-Android.mov
35643-AfterFix-Android.mov