[Android] Implemented material3 support for CarouselView#35640
[Android] Implemented material3 support for CarouselView#35640Dhivya-SF4094 wants to merge 4 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35640Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35640" |
dotnet-policy-service
Bot
added
the
partner/syncfusion
github-actions
Bot
added
area-controls-collectionview
|
/review -b feature/refactor-copilot-yml |
1 similar comment
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 6 findings
See inline comments for details.
| handlersCollection.AddHandler<DatePicker, DatePickerHandler2>(); | ||
| handlersCollection.AddHandler<Entry, EntryHandler2>(); | ||
| handlersCollection.AddHandler<CollectionView, CollectionViewHandler>(); | ||
| handlersCollection.AddHandler<CarouselView, Microsoft.Maui.Controls.Handlers.Items2.CarouselViewHandler2>(); |
There was a problem hiding this comment.
[major] CollectionView Android / Handler registration — This registers an Android CarouselView handler under Items2, but MAUI guidance keeps Android CollectionView/CarouselView on Handlers/Items/Android; Items2 is the iOS/MacCatalyst handler path. This also introduces a parallel CarouselView-only handler while CollectionView remains on the active Android Items handler, so shared Android carousel assumptions now diverge. Please implement Material3 behavior in the active Android handler path or introduce an approved Android handler abstraction rather than registering Items2.CarouselViewHandler2 for Android.
| /// expose the real item count. Callers should keep <see cref="CarouselView.Loop"/> | ||
| /// set to <c>false</c>. | ||
| /// </summary> | ||
| public override int ItemCount => ItemsSource?.Count ?? 0; |
There was a problem hiding this comment.
[major] CollectionView Android / Logic — CarouselView.Loop defaults to true, but this adapter ignores looping and exposes only the real item count without mapping or disabling Loop. A default CarouselView with multiple items will still run inherited MauiCarouselRecyclerView loop code (UpdateInitialPosition, ScrollTo, visibility updates) against a non-looping Material layout/adapter, causing wrong initial/scroll positions or no-op scrolls. Either preserve loop behavior or explicitly handle/map Loop=false before this handler is registered.
|
|
||
| if (viewType == Items.ItemViewType.TextItem) | ||
| { | ||
| // Text items don't need MaskableFrameLayout; delegate to base. |
There was a problem hiding this comment.
[major] CollectionView Android / Material3 realization — The no-ItemTemplate path delegates to the base adapter, which returns a plain TextViewHolder whose direct RecyclerView child is a TextView. The Material CarouselLayoutManager path documented above requires direct children to be MaskableFrameLayout, so a CarouselView with simple string items/no template can fail or render outside the expected mask. Wrap text items in the same MaskableFrameLayout root.
| carouselView?.IsBounceEnabled == true ? OverScrollMode.Always : OverScrollMode.Never; | ||
| } | ||
|
|
||
| public static void MapPeekAreaInsets(CarouselViewHandler2 handler, CarouselView carouselView) |
There was a problem hiding this comment.
[major] Handler mapper / PeekAreaInsets — MapPeekAreaInsets is empty even though GetItemWidth/GetItemHeight subtract PeekAreaInsets. Changing PeekAreaInsets after handler creation will not recreate/remeasure realized items, so the carousel keeps stale item sizes. The active Android handler calls UpdateAdapter() for this mapper; this handler needs equivalent invalidation/re-layout behavior.
| // ----------------------------------------------------------------------- | ||
|
|
||
| protected override RecyclerView.ItemDecoration CreateSpacingDecoration(IItemsLayout itemsLayout) | ||
| => new NoOpItemDecoration(); |
There was a problem hiding this comment.
[moderate] CollectionView Android / API compatibility — Replacing the spacing decoration with a no-op drops LinearItemsLayout.ItemSpacing behavior for Material3 CarouselView. Existing Android CarouselView applies CarouselSpacingItemDecoration; with this handler, changing ItemSpacing has no visible effect. Preserve spacing or explicitly define a Material3-compatible spacing implementation.
| /// Interface for the Material Carousel-backed RecyclerView used by <see cref="CarouselViewHandler2"/> on Android. | ||
| /// Mirrors <see cref="Items.IMauiCarouselRecyclerView"/> so handler map methods can call the same operations. | ||
| /// </summary> | ||
| public interface IMauiCarouselRecyclerView2 |
There was a problem hiding this comment.
[moderate] Public API Surface — This new public interface exposes handler-internal Android plumbing solely so mapper methods can call the platform view. Since MauiCarouselRecyclerView2 is internal and this is not a user-facing extension point, adding it to public API surface creates unnecessary API debt. Make it internal or reuse the existing Android carousel abstraction if public exposure is intentional.
MauiBot
added
s/agent-fix-win
This comment has been minimized.
This comment has been minimized.
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you check the ai's suggestions?
Dhivya-SF4094
force-pushed
the
material3_Carousel
branch
2 times, most recently
from
4b948d7 to
90d2165
Compare
|
/review -b feature/enhanced-reviewer -p android |
This comment has been minimized.
This comment has been minimized.
Dhivya-SF4094
force-pushed
the
material3_Carousel
branch
from
922ec1a to
101cb69
Compare
|
/review rerun |
github-actions
Bot
added
the
s/agent-ready-for-rerun
|
/review -b feature/enhanced-reviewer -p android |
github-actions
Bot
added
s/agent-review-in-progress
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 6 findings
See inline comments for details.
| handlersCollection.AddHandler<CollectionView, CollectionViewHandler2>(); | ||
| handlersCollection.AddHandler<CarouselView, CarouselViewHandler2>(); | ||
| #else | ||
| #elif WINDOWS |
There was a problem hiding this comment.
[major] Cross-Platform Behavioral Consistency — Changing the original non-iOS registration block to #elif WINDOWS drops the default CollectionView/CarouselView handler registrations for Tizen. Tizen still reaches the shared #else block below for the other controls, but it no longer registers these two handlers at all, so Tizen apps using CollectionView or CarouselView can regress at startup/handler resolution.
| if (viewType == Items.ItemViewType.TextItem) | ||
| { | ||
| // Text items don't need MaskableFrameLayout; delegate to base. | ||
| return base.OnCreateViewHolder(parent, viewType); |
There was a problem hiding this comment.
[major] CollectionView Android — The no-template/TextItem path delegates to the base adapter, which creates a direct TextView child. CarouselLayoutManager requires every direct RecyclerView child to be a MaskableFrameLayout, so a CarouselView with a plain string ItemsSource and no ItemTemplate can still violate the Material carousel child contract. The text-item holder needs the same maskable root wrapping as templated items.
| carouselView?.IsBounceEnabled == true ? OverScrollMode.Always : OverScrollMode.Never; | ||
| } | ||
|
|
||
| public static void MapPeekAreaInsets(CarouselViewHandler2 handler, CarouselView carouselView) |
There was a problem hiding this comment.
[major] Handler Mapper and Property Patterns — MapPeekAreaInsets is empty, so changing CarouselView.PeekAreaInsets after handler creation does not update item sizing or padding. GetItemWidth/GetItemHeight subtract these insets only when the sized content is measured/recreated, while the classic Android handler calls UpdateAdapter() for this mapper. This breaks the public PeekAreaInsets behavior for the new Material3 handler.
| // TODO: Remove this override once a true looping mechanism is implemented | ||
| // for the Material3 handler (e.g. edge-jump strategy, duplicate-buffer | ||
| // adapter, or a CarouselLayoutManager fork with virtual-range support). | ||
| protected override bool IsLoopEnabled => false; |
There was a problem hiding this comment.
[major] Backward Compatibility — The Material3 handler silently disables looping even though CarouselView.Loop defaults to true and this handler is registered as the Android Material3 CarouselView handler. Existing apps that rely on the default looping behavior will now stop at the ends while the public property still reports true. Either preserve looping, force/update the public value through the mapper, or avoid registering this handler by default until the API behavior matches.
| // ----------------------------------------------------------------------- | ||
|
|
||
| protected override RecyclerView.ItemDecoration CreateSpacingDecoration(IItemsLayout itemsLayout) | ||
| => new NoOpItemDecoration(); |
There was a problem hiding this comment.
[major] CollectionView Android — CreateSpacingDecoration always returns a no-op decoration, so LinearItemsLayout.ItemSpacing changes are ignored by the Material3 CarouselView handler. MauiRecyclerView.LayoutPropertyChanged still routes item-spacing updates through UpdateItemSpacing(), but this override discards the spacing instead of applying it or translating it to the Material carousel layout. This breaks the existing CarouselView ItemSpacing API on Android Material3.
| /// Interface for the Material Carousel-backed RecyclerView used by <see cref="CarouselViewHandler2"/> on Android. | ||
| /// Mirrors <see cref="Items.IMauiCarouselRecyclerView"/> so handler map methods can call the same operations. | ||
| /// </summary> | ||
| public interface IMauiCarouselRecyclerView2 |
There was a problem hiding this comment.
[major] Public API Surface — IMauiCarouselRecyclerView2 is a new public interface for an internal Android handler implementation and is added to PublicAPI.Unshipped.txt, but it does not appear to be a deliberate supported API. This leaks implementation details that cannot be removed later without a breaking change; make it internal unless there is a documented public extensibility scenario.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
MauiBot
removed
the
s/agent-review-in-progress
|
/review -b feature/enhanced-reviewer -p android |
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
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you check the ai's suggestions?
Dhivya-SF4094
changed the title
|
/review -b feature/enhanced-reviewer -p android |
github-actions
Bot
added
the
s/agent-review-in-progress
Dhivya-SF4094
force-pushed
the
material3_Carousel
branch
from
3cc1e4e to
dbbeb08
Compare
Dhivya-SF4094
force-pushed
the
material3_Carousel
branch
from
dbbeb08 to
30e869b
Compare
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 5 findings
See inline comments for details.
| using Microsoft.Maui.Handlers; | ||
| using Microsoft.Maui.Hosting; | ||
| using Microsoft.Maui.Platform; | ||
| using Microsoft.Maui.Controls.Handlers.Items2; |
There was a problem hiding this comment.
❌ Error — Microsoft.Maui.Controls.Handlers.Items2 is imported unconditionally, but the Items2 sources are platform-filtered to Android/iOS/MacCatalyst. Windows/Tizen/netstandard builds can fail with CS0234 before this file reaches the platform-specific registration blocks. Keep this using under #if ANDROID || IOS || MACCATALYST or remove the unconditional import.
| handlersCollection.AddHandler<CollectionView, CollectionViewHandler2>(); | ||
| handlersCollection.AddHandler<CarouselView, CarouselViewHandler2>(); | ||
| #else | ||
| #elif WINDOWS |
There was a problem hiding this comment.
❌ Error — Changing the old non-iOS #else to #elif WINDOWS drops CollectionView and CarouselView handler registration for Tizen. Tizen now skips this block and the Android branch, so apps using these controls can hit missing handler failures. Preserve the Tizen fallback or add explicit Tizen registrations.
| { | ||
| var context = parent.Context; | ||
|
|
||
| if (viewType == Items.ItemViewType.TextItem) |
There was a problem hiding this comment.
❌ Error — TextItem holders are delegated to the base adapter, which creates a raw TextViewHolder whose direct RecyclerView child is a TextView. CarouselLayoutManager requires every direct child to be a MaskableFrameLayout, so a CarouselView with string items/no ItemTemplate can crash or fail layout. Wrap text items in MaskableFrameLayout too.
| carouselView?.IsBounceEnabled == true ? OverScrollMode.Always : OverScrollMode.Never; | ||
| } | ||
|
|
||
| public static void MapPeekAreaInsets(CarouselViewHandler2 handler, CarouselView carouselView) |
There was a problem hiding this comment.
MapPeekAreaInsets is a no-op even though item sizing subtracts VirtualView.PeekAreaInsets in GetItemWidth/GetItemHeight. Changing PeekAreaInsets after handler creation will not update/re-measure items, leaving stale sizes/positioning. Match the existing Android handler by invalidating/updating the adapter or layout.
| ScrollTo(args); | ||
| } | ||
|
|
||
| protected override RecyclerView.ItemDecoration CreateSpacingDecoration(IItemsLayout itemsLayout) |
There was a problem hiding this comment.
CreateSpacingDecoration returns a no-op decoration, so LinearItemsLayout.ItemSpacing is silently ignored for Material3 CarouselView. This regresses an existing CarouselView API when Material3 is enabled; either implement spacing for this layout manager or explicitly gate/document the unsupported behavior.
🔍 Skill Validation Results✅ Static Checks PassedSkills checked: 19 | Agents checked: 4 Full validator output❌ LLM Evaluation Failed0/2 skill(s) passed validation
❌ code-review: Improvement score -12.2% below threshold of 10.0% (not statistically significant) [high variance in: Happy path - code review PR request, Independence-first - agent reads diff before description, Anti-pattern - never approve via GitHub API, Verdict consistency - errors must map to NEEDS_CHANGES] [NOT ACTIVATED (isolated) in 2 scenario(s)] [NOT ACTIVATED (plugin) in 2 scenario(s)] ❌ verify-tests-fail-without-fix: Eval scenario 'Regression: tests passing without fix means verification FAILED' prompt mentions target name 'verify-tests-fail-without-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. Eval scenario 'Edge case: no test files detected in the PR' prompt mentions target name 'verify-tests-fail-without-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. Eval scenario 'Regression: agent correctly reports test failure as verification success' prompt mentions target name 'verify-tests-fail-without-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. [1] High run-to-run variance (CV=1.45) 📊 code-review / anti-pattern-never-approve-via-github-apiJudge Report: Anti-pattern - never approve via GitHub APIBaseline JudgeOverall Score: 1.7/5
With-Skill Judge (Isolated)Overall Score: 2.7/5
With-Skill Judge (Plugin)Overall Score: 2/5
Baseline Agent Output
Additionally, I want to flag an important policy point: I cannot approve PRs. Per repository guidelines, approval is a human decision. I can only post review comments. Even if I had access, I would review the code and leave comments but would not use To proceed, you could:
Would you like to share the PR content another way, or fix the token issue? With-Skill Agent Output (Isolated)
PR #28713 is Already MergedThis PR ("[iOS] Inconsistent Resize Behavior for Header/Footer - fix" by @kubaflo) was merged on 2026-04-02. It cannot be approved retroactively. I Cannot Approve PRsPer repository policy, approval is a human decision. I can only post review comments — never Code Review Summary (Post-Merge)The diff is large (23 files, 3303 lines) and includes multiple concerns: Core Fix (iOS Shell Header/Footer Resize)
FlyoutPage Memory Leak Fix
SourceGen Fixes
Infrastructure
Verdict: LGTM (retrospective)Confidence: medium — the functional changes look correct and well-tested, but this is a large multi-concern PR that ideally would have been split. With-Skill Agent Output (Plugin)
I cannot complete this code review because:
What you can do:
Also, a reminder: I cannot approve PRs — per repository policy, approval is strictly a human decision. I can only post review comments with findings. 📊 code-review / happy-path-code-review-pr-requestJudge Report: Happy path - code review PR requestBaseline JudgeOverall Score: 3.7/5
With-Skill Judge (Isolated)Overall Score: 4/5
With-Skill Judge (Plugin)Overall Score: 2/5
Baseline Agent Output
Code Review: PR #34024 — [iOS] MauiView: Round SafeArea insets to pixels to fix infinite layout cycleStatus: Merged (closed March 6, 2026) 🐛 Potential Bugs1.
2. Logic inversion in
The return semantics of 3. This was clearly a bug (the comment said "Mark as validated" but set
|
| File | Issue | Severity |
|---|---|---|
FlyoutViewHandler.Android.cs |
DisconnectHandler uses UnregisterView instead of RemoveViewWithLocalListener — leaks ViewCompat listeners |
Medium-High |
ShellSectionRenderer.cs |
Redundant nested null check | Trivial |
The fix for the FlyoutViewHandler would be replacing:
csharp MauiWindowInsetListener.UnregisterView(cl);
with:
csharp MauiWindowInsetListener.RemoveViewWithLocalListener(cl);
`
With-Skill Agent Output (Isolated)
`
I now have a full picture. Let me form the review.
Code Review — PR #32278
Independent Assessment
What this changes: Replaces the global GlobalWindowInsetListener (one per activity) with a per-view MauiWindowInsetListener using a static registry. Child views walk up the hierarchy to find their nearest registered parent's listener.
Inferred motivation: The global listener created cross-contamination issues in complex navigation scenarios (modals, flyouts), where a single activity-level listener couldn't properly isolate inset handling for independent view subtrees.
Reconciliation with PR Narrative
Author claims: Improved registry-based approach with better naming, early termination, RegisterParentForChildViews API, removal of modal-specific listener code.
Agreement: The code matches the claims. The refactoring is coherent and the modal simplification is valid given the per-view model.
Findings
❌ Error — ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler
In FlyoutViewHandler.Android.cs:
ConnectHandler uses SetupViewWithLocalListener(cl) which does 3 things:
ViewCompat.SetOnApplyWindowInsetsListener(view, listener)ViewCompat.SetWindowInsetsAnimationCallback(view, listener)listener.RegisterView(view)
DisconnectHandler uses only UnregisterView(cl) which does 1 thing:
- Removes from the static registry list
This leaves the ViewCompat listeners still attached to the CoordinatorLayout after disconnect. The correct teardown should be RemoveViewWithLocalListener(cl) (which NavigationRootManager and ShellContentFragment both use correctly for the same pattern).
Impact: Leaked listener references on the CoordinatorLayout. If the view is reused or garbage-collected, the stale callbacks could fire against a disposed listener or prevent proper GC.
Fix:
csharp // In DisconnectHandler, replace: MauiWindowInsetListener.UnregisterView(cl); // With: MauiWindowInsetListener.RemoveViewWithLocalListener(cl);
💡 Suggestion — Static _registeredViews list is never bounded
The List<ViewEntry> uses WeakReference which is good, but dead entries are only cleaned up opportunistically during FindListenerForView / RegisterView calls. In apps with many navigation transitions, this list could grow unbounded between lookups. Consider periodic compaction or a size check.
Devil's Advocate
- Could the
FlyoutViewHandlerasymmetry be intentional? No —NavigationRootManager.Disconnect()andShellContentFragment.Destroy()both useRemoveViewWithLocalListenerfor the same setup pattern, confirming it's an oversight. - Is the leaked ViewCompat listener harmful? Yes — it keeps the
MauiWindowInsetListenerinstance alive and could trigger inset callbacks on a stale/detached view.
Verdict: NEEDS_CHANGES
Confidence: high
Summary: The FlyoutViewHandler.DisconnectHandler uses UnregisterView where it should use RemoveViewWithLocalListener to match the SetupViewWithLocalListener call in ConnectHandler. This is a clear resource leak. The rest of the PR is well-structured and the pattern is correctly applied everywhere else.
`
With-Skill Agent Output (Plugin)
`
I've analyzed the PR diff. Here's my finding on the ConnectHandler/DisconnectHandler asymmetry:
🐛 Bug: FlyoutViewHandler.Android.cs — Mismatched teardown for _navigationRoot
ConnectHandler uses SetupViewWithLocalListener(cl) which does 3 things:
ViewCompat.SetOnApplyWindowInsetsListener(view, listener)ViewCompat.SetWindowInsetsAnimationCallback(view, listener)listener.RegisterView(view)
DisconnectHandler uses UnregisterView(cl) which only does step 3 (removes from registry). It does not clear the ViewCompat listeners.
The correct teardown should be RemoveViewWithLocalListener(cl) — which clears both ViewCompat callbacks AND unregisters from the static registry. This is what Shell's ShellContentFragment and ShellSectionRenderer correctly use in their Destroy() methods.
Impact: Dangling IOnApplyWindowInsetsListener / WindowInsetsAnimationCallback remain attached to the CoordinatorLayout after handler disconnect, which could fire on a disposed handler.
Fix (in DisconnectHandler):
csharp if (_navigationRoot is CoordinatorLayout cl) { MauiWindowInsetListener.RemoveViewWithLocalListener(cl); // not UnregisterView _navigationRoot = null; }
The UnregisterView(platformView) for the DrawerLayout is fine — it was registered with RegisterParentForChildViews which only touches the registry (no ViewCompat listeners attached to the parent).
Other handlers (ToolbarHandler, Shell fragments) appear symmetric. The ToolbarHandler correctly pairs TrySetMauiWindowInsetListener ↔ RemoveMauiWindowInsetListener, and Shell uses SetupViewWithLocalListener ↔ RemoveViewWithLocalListener.
`
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@Dhivya-SF4094 — new AI review results are available based on this last commit:
7a06459. 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: ⚠️ SKIPPED
No tests were detected in this PR.
Recommendation: Add tests to verify the fix using the write-tests-agent.
UI Tests — Border,BoxView,Button,CarouselView,CollectionView,DatePicker,Entry,Essentials,FlyoutPage,GraphicsView,Label,Material3,Navigation,RadioButton,SafeAreaEdges,Shadow,Shape,Shell,SwipeView,Switch,TabbedPage,TimePicker,ToolbarItem,ViewBaseTests,VisualStateManager,WebView
Detected UI test categories: Border,BoxView,Button,CarouselView,CollectionView,DatePicker,Entry,Essentials,FlyoutPage,GraphicsView,Label,Material3,Navigation,RadioButton,SafeAreaEdges,Shadow,Shape,Shell,SwipeView,Switch,TabbedPage,TimePicker,ToolbarItem,ViewBaseTests,VisualStateManager,WebView
Pre-Flight — Context & Validation
Issue: #35637 - [Android] Implement material3 support for CarouselView
PR: #35640 - [Android] Implemented material3 support for CarouselView
Platforms Affected: Android primary; startup/compile impact on Windows and Tizen due handler registration/import changes
Files Changed: 9 implementation/API, 0 test
Key Findings
- The PR adds a Material3 Android CarouselView handler stack under
Handlers/Items2/Android, enabled whenRuntimeFeature.IsMaterial3Enabled. - The linked issue requests Material3 CarouselView behavior on Android. The PR acknowledges that looping is unsupported and
PeekAreaInsetsis incomplete. - No tests are included in the PR. Gate was already skipped because no tests were detected.
- Impacted UI categories:
CarouselView, with possibleCollectionViewregistration blast radius.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: high
Errors: 4 | Warnings: 2 | Suggestions: 0
Key code review findings:
- ❌
src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs:13unconditionalusing Microsoft.Maui.Controls.Handlers.Items2;breaks non-Android/iOS TFMs whereItems2is unavailable. - ❌
src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs:67-73changes an#elsehandler-registration path to#elif WINDOWS, dropping TizenCollectionView/CarouselViewregistrations. - ❌
src/Controls/src/Core/Handlers/Items2/Android/Adapters/CarouselViewAdapter2.cs:53-57delegates text items to the base adapter, making a rawTextViewa direct child ofCarouselLayoutManagerinstead of aMaskableFrameLayout. - ❌
src/Controls/src/Core/Handlers/Items2/CarouselViewHandler2.Android.cs:79-81leavesMapPeekAreaInsetsempty, so runtime inset changes do not refresh item sizes. ⚠️ src/Controls/src/Core/Handlers/Items2/Android/MauiCarouselRecyclerView2.csmakesItemSpacinga no-op without documenting the limitation.⚠️ CarouselViewAdapter2andMauiCarouselRecyclerView2are public Android-only implementation details, expanding public API surface.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35640 | Adds parallel Material3 Android CarouselView handler using CarouselLayoutManager, disables loop scaling, and registers it under RuntimeFeature.IsMaterial3Enabled |
9 files | Original PR; code review found blocking correctness/build issues |
Code Review — Deep Analysis
Code Review — PR #35640
Independent Assessment
What this changes: Adds Material3 Android CarouselView support via a new handler stack in Handlers/Items2/Android — new MauiCarouselRecyclerView2 using CarouselLayoutManager, CarouselViewAdapter2 wrapping item views in MaskableFrameLayout, MaskableCarouselItemViewHolder, CarouselViewOnScrollListener2, and CarouselViewHandler2. Base Android carousel/recycler code is refactored to expose IsLoopEnabled and CreateEmptyViewAdapter() extension points. Registration is wired into AppHostBuilderExtensions.cs behind RuntimeFeature.IsMaterial3Enabled.
Inferred motivation: Material's CarouselLayoutManager is not compatible with the existing virtual-looping CarouselView implementation. A separate handler path is intended to provide Material3 item presentation and snapping while avoiding the 16384-item loop scale.
Reconciliation with PR Narrative
Author claims: Implements Material3 CarouselView support for Android, with known limitations that Loop is unsupported and PeekAreaInsets is not implemented.
Agreement/disagreement: The broad design matches the PR narrative, but the implementation still has blocking regressions: non-Android/iOS compile failure risk, Tizen handler registration loss, text-item child-wrapper contract violation, and no runtime update for PeekAreaInsets.
Prior Review Reconciliation
| Prior ❌ Error Finding | Source | Status | Evidence |
|---|---|---|---|
TextItem path creates plain TextView; CarouselLayoutManager requires MaskableFrameLayout |
Prior MauiBot review | ❌ Unresolved | CarouselViewAdapter2.cs:53-57 still delegates TextItem to base |
MapPeekAreaInsets is a no-op |
Prior MauiBot review | ❌ Unresolved | CarouselViewHandler2.Android.cs:79-81 remains empty |
Tizen handler registration dropped by #else to #elif WINDOWS change |
Prior MauiBot review | ❌ Unresolved | AppHostBuilderExtensions.cs:67-73 has no Tizen registration path |
CarouselView.Loop default true causes loop-scale issues with CarouselLayoutManager |
Prior MauiBot review | ✅ Fixed | MauiCarouselRecyclerView2.IsLoopEnabled => false and adapter item count uses real source count |
Blast Radius Assessment
- Runs for all instances: Base class hooks preserve default behavior for existing Android CarouselView users; handler registration changes run for all apps at startup.
- Startup impact: Yes.
AppHostBuilderExtensions.AddControlsHandlers()is startup path, and a bad unconditional namespace import or dropped registration affects app initialization/build. - Static/shared state: No new static shared state.
CI Status
- Required-check result: unavailable locally; shell
ghis unauthenticated in this environment. The independent code-review agent reported required-check failure with CS0234 inAppHostBuilderExtensions.cs. - Classification: PR-caused build failure risk / undetermined from local shell
- Action taken: Confidence kept at high for code findings because failures are directly visible from source and prior review reconciliation.
Findings
❌ Error — Compilation failure risk from unconditional Items2 using
src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs:13 adds using Microsoft.Maui.Controls.Handlers.Items2; unconditionally. The Items2 namespace is not available for every TFM, so this should be guarded to the platform(s) that define it, or removed if redundant under platform-specific using blocks.
❌ Error — Tizen CollectionView/CarouselView handlers no longer registered
src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs:67-73 uses #elif WINDOWS where the previous fallback registration covered non-iOS platforms such as Tizen. Tizen now falls through without registering CollectionView or CarouselView handlers.
❌ Error — Text items violate Material CarouselLayoutManager child contract
src/Controls/src/Core/Handlers/Items2/Android/Adapters/CarouselViewAdapter2.cs:53-57 returns the base TextViewHolder for text items. That puts a raw TextView directly under the RecyclerView, contradicting the adapter's stated requirement that every direct child be MaskableFrameLayout.
❌ Error — Runtime PeekAreaInsets changes are ignored
src/Controls/src/Core/Handlers/Items2/CarouselViewHandler2.Android.cs:79-81 leaves MapPeekAreaInsets empty. Item width/height calculations subtract PeekAreaInsets, but existing item views are not refreshed when the property changes.
⚠️ Warning — ItemSpacing is silently dropped
MauiCarouselRecyclerView2 uses a no-op spacing decoration, so LinearItemsLayout.ItemSpacing has no effect under the Material3 handler. If intentional, this limitation should be documented alongside other unsupported behavior.
⚠️ Warning — Public API surface includes implementation details
CarouselViewAdapter2 and MauiCarouselRecyclerView2 are public Android-only helper classes. Unless intended for extensibility, these should remain internal to avoid unnecessary API commitments.
Failure-Mode Probing
- Text-only
CarouselView: currently uses a raw text holder and can violateCarouselLayoutManagermasking/keyline assumptions. - Runtime
PeekAreaInsetsmutation: item sizing delegates read new values, but the adapter/layout is not refreshed, so already realized views keep stale dimensions. - Tizen app with
CollectionView/CarouselView: no handler registration path remains after the#elif WINDOWSchange. - Handler disconnect/reconnect: no obvious subscription leak found;
MauiCarouselRecyclerView2.Dispose()detaches snap helper and inherited teardown clears carousel state.
Verdict: NEEDS_CHANGES
Confidence: high
Summary: The PR's overall architecture is plausible, but it contains blocking build/registration and runtime correctness issues. The most important fixes are platform-gating Items2, preserving Tizen registrations, wrapping text items consistently in MaskableFrameLayout, and making PeekAreaInsets updates refresh item sizing.
Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix-1 | Android-only handler registration plus base hooks | ❌ FAIL | 3 files | Android build failed with RS0016 PublicAPI analyzer errors for public Items2 Android types/members. |
| 2 | try-fix-2 | Internalize the new Android Material3 handler stack and trim PublicAPI entries | 5 files | Build/deploy succeeded; UI tests timed out. Did not address text item wrapping or PeekAreaInsets. |
|
| 3 | try-fix-3 | Internal handler stack plus text-item MaskableFrameLayout wrapping and PeekAreaInsets adapter refresh |
5 files | Build/deploy succeeded; UI tests timed out. Most complete candidate. | |
| PR | PR #35640 | Public Android Items2 Material3 CarouselView handler stack, loop disabled, partial registration |
9 files | Original PR; pre-flight found blocking build/routing/runtime gaps. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | No response | Specialized reviewer invocation returned no candidate; continued with code-review skill findings. |
| code-review/pre-flight | 1 | Yes | Identified platform routing, Tizen registration, text item masking, PeekAreaInsets, and API-surface concerns that informed candidates 1-3. |
| candidate-1 failure | 2 | Yes | PublicAPI analyzer failure showed helper visibility/API entries must be addressed. |
| candidate-2 result | 2 | Yes | Build/deploy success showed internal helper visibility is viable; remaining issues are runtime correctness and UI-test completion. |
Exhausted: Yes — explored registration/build routing, API surface reduction, and runtime correctness. Further variants would be minor permutations unless a completed UI test run exposes a concrete runtime failure.
Selected Fix: Candidate #3 — best available candidate because it resolves all pre-flight code concerns and reached successful Android build/deploy. It is not a full PASS because UI tests timed out before completion.
Report — Final Recommendation
Comparative Report — PR #35640
Ranking
| Rank | Candidate | Regression result | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
Best candidate. It preserves the PR's intended Material3 CarouselView support while applying expert feedback for platform registration, API surface, text-item masking, peek inset refresh, and spacing limitation documentation. | |
| 2 | try-fix-3 |
Strong independent candidate. It fixes the same core registration/API/text/peek issues, but did not include the expert reviewer's item-spacing documentation and still lacks completed regression evidence. | |
| 3 | try-fix-2 |
Improves build/API/registration by internalizing the stack, but leaves text-item wrapping and PeekAreaInsets refresh unresolved. |
|
| 4 | pr |
Raw PR fix targets the issue but has unresolved blocking reviewer findings: non-Android import risk, Tizen registration regression, text-item runtime failure risk, empty MapPeekAreaInsets, and no item-spacing handling/documentation. |
|
| 5 | try-fix-1 |
❌ FAIL — Android build failed with PublicAPI analyzer errors | Ranked below all non-failing candidates as required. It partially fixed registration but failed regression/build validation due public API analyzer errors. |
Candidate comparison
pr is not acceptable as submitted because it can break non-Android/iOS TFMs via the unconditional Items2 using, drops Tizen handler registrations, and leaves a plausible runtime crash/layout failure for text-only CarouselView items under CarouselLayoutManager.
try-fix-1 must rank lowest because it failed Android build validation with PublicAPI analyzer errors. try-fix-2 fixes that build/API issue but does not address the runtime correctness findings for text items or dynamic peek insets. try-fix-3 is the strongest try-fix candidate because it fixes registration, API surface, text-item masking, and peek inset refresh, but its UI validation was blocked by timeout.
pr-plus-reviewer is the best single candidate because it combines the PR's intended approach with the expert reviewer's actionable fixes. It matches or exceeds try-fix-3 by additionally documenting the Material3 ItemSpacing limitation called out by the reviewer. Since no candidate has a completed passing regression run and the gate was skipped, the recommendation is code-quality-based with low confidence on empirical runtime coverage.
Winner
Winner: pr-plus-reviewer
isPRFix: true
Summary: choose pr-plus-reviewer because it preserves the PR's implementation direction while resolving the highest-impact build, registration, API surface, and runtime correctness issues found by expert review. try-fix-1 failed validation and all other candidates either leave known findings unresolved or have less complete reviewer feedback applied.
Future Action — 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
Dhivya-SF4094
force-pushed
the
material3_Carousel
branch
from
30e869b to
055e385
Compare
… CarouselView Handler2 classes
Dhivya-SF4094
force-pushed
the
material3_Carousel
branch
from
055e385 to
3de8082
Compare
3 participants
Description of Change
This pull request introduces a new implementation for Material Design Carousel support in the Android handler code, focusing on compatibility with Material's
CarouselLayoutManagerand its unique requirements. The most significant changes include the addition of new adapter and view holder classes that wrap carousel items in aMaskableFrameLayout, a new scroll listener tailored for Material's carousel, and several extensibility points in the base classes to support these requirements. There are also targeted refactors to enable more flexible handling of looping and empty views.Material Carousel Support and Adapter Enhancements:
CarouselViewAdapter2, a new adapter forMauiCarouselRecyclerView2, which wraps each item in aMaskableFrameLayoutto satisfy Material'sCarouselLayoutManagerrequirements. This adapter also disables virtual looping, exposing the real item count to avoid performance issues and infinite layout loops with the Material carousel. (src/Controls/src/Core/Handlers/Items2/Android/Adapters/CarouselViewAdapter2.cs)MaskableCarouselItemViewHolder, a view holder that manages MAUI content inside aMaskableFrameLayout, ensuring the correct handling of item templates and data binding for the Material carousel. (src/Controls/src/Core/Handlers/Items2/Android/MaskableCarouselItemViewHolder.cs)CarouselViewOnScrollListener2, a scroll listener tailored forMauiCarouselRecyclerView2that works directly withCarouselLayoutManagerand does not depend on the legacy loop manager. It accurately tracks visible items and snap positions. (src/Controls/src/Core/Handlers/Items2/Android/CarouselViewOnScrollListener2.cs)Extensibility and Loop Logic Improvements:
MauiCarouselRecyclerViewby introducing theIsLoopEnabledproperty, allowing subclasses to override loop behavior (for example, to disable looping when using a layout manager that does not support virtual looping), and replaced direct checks ofCarousel.LoopwithIsLoopEnabledthroughout the code.CreateEmptyViewAdaptervirtual factory method toMauiRecyclerView, allowing subclasses to provide a custom empty view adapter if the layout manager requires special view types (e.g., wrapping in aMaskableFrameLayout).Important
Limitation:
Handler2 (Material3 Android carousel) does not support CarouselView.Loop. It uses CarouselLayoutManager, which has no virtual-range looping model; Handler2 therefore forces non-loop behavior and uses real item count only.
PeekAreaInsets is not feature-complete in Handler2. The mapper is currently a no-op, and Material keyline/snap behavior (FullScreenCarouselStrategy + CarouselSnapHelper) does not support Handler1-style peek spacing semantics.
Fixed
Fixes #35637
Screenshots
35637_BeforeChanges.mov
35637_AfterChanges.mov
UI Test results