[Windows] [Android] Added Picker Styling for BorderColor and BorderThickness#33093
[Windows] [Android] Added Picker Styling for BorderColor and BorderThickness#33093licon4812 wants to merge 22 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 33093Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 33093" |
licon4812
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds new BorderColor and BorderThickness properties to the Picker control for Windows and Android platforms, partially addressing issue #32055. The implementation adds public API surface in both Core (IPicker interface) and Controls (Picker class) layers, with platform-specific implementations for Windows (using native BorderBrush/BorderThickness) and Android (using a custom drawable approach).
Key Changes:
- New public API properties for styling Picker borders
- Platform-specific implementations for Windows and Android
- Sample demonstration in PickerPage.xaml
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Core/src/PublicAPI/*/PublicAPI.Unshipped.txt | API surface additions for IPicker.BorderColor and IPicker.BorderThickness getters across all TFMs |
| src/Core/src/Core/IPicker.cs | Interface additions for BorderColor and BorderThickness properties |
| src/Controls/src/Core/PublicAPI/*/PublicAPI.Unshipped.txt | API surface additions for Picker bindable properties and mapper methods |
| src/Controls/src/Core/Picker/Picker.cs | Implementation of BorderColor and BorderThickness bindable properties |
| src/Controls/src/Core/Picker/Picker.Windows.cs | Windows-specific mapper methods using native BorderBrush/BorderThickness |
| src/Controls/src/Core/Picker/Picker.Android.cs | Android-specific mapper methods calling CreateBorder extension |
| src/Controls/src/Core/Picker/Picker.Mapper.cs | Registration of border property mappers for Windows and Android |
| src/Controls/src/Core/Platform/Android/Extensions/PickerExtensions.cs | Android implementation using custom ThicknessDrawable for per-side border support |
| src/Controls/samples/Controls.Sample/Pages/Controls/PickerPage.xaml | Sample demonstrating BorderColor and BorderThickness usage |
| /// <include file="../../docs/Microsoft.Maui.Controls/Picker.xml" path="//Member[@MemberName='BorderColor']/Docs/*" /> | ||
| public Color BorderColor | ||
| { | ||
| get { return (Color)GetValue(BorderColorProperty); } | ||
| set { SetValue(BorderColorProperty, value); } | ||
| } | ||
|
|
||
| /// <include file="../../docs/Microsoft.Maui.Controls/Picker.xml" path="//Member[@MemberName='BorderThickness']/Docs/*" /> | ||
| public Thickness BorderThickness | ||
| { | ||
| get { return (Thickness)GetValue(BorderThicknessProperty); } | ||
| set { SetValue(BorderThicknessProperty, value); } | ||
| } |
There was a problem hiding this comment.
Missing unit tests for the new BorderColor and BorderThickness properties. The repository has comprehensive Picker tests (e.g., src/Controls/tests/Core.UnitTests/PickerTests.cs), but there are no tests verifying the new border properties' behavior, such as:
- Property getters/setters work correctly
- Default values are as expected
- Property changed notifications fire correctly
- Binding to these properties works
Please add unit tests to cover these new properties.
Tamilarasan-Paranthaman
added
platform/windows
platform/android
area-controls-picker
|
/rebase |
|
/review -b feature/refactor-copilot-yml |
|
/review -b feature/refactor-copilot-yml -p windows |
|
|
AI code review for net11.0 targetVerdict: Needs discussion (partial-platform public API + minor functional gaps) This is a non-approval, automated review comment. It is advisory only — human approval is required. What this PR doesAdds Findings
CI noteRed checks: Confidence: Medium. |
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Synthesized Review — PR #33093 (Picker BorderColor/BorderThickness)
Verdict: NEEDS_CHANGES (confidence: high)
This PR adds public Picker.BorderColor/BorderThickness API with Windows + Android rendering (iOS deferred), but the platform implementations have functional defects and the new members reshape the shipped IPicker interface. Both the Android and Windows paths are demonstrably broken in the PR's own scenarios — a thickness-only border renders blank on Android, and the default Thickness(0) strips the native border from every Windows Picker. maui-pr CI is red and the PR has a net11.0 merge conflict, so it cannot be approved as-is.
Key findings (4 inline, code-confirmed)
- Android —
CreateBorderdestroys native background / renders blank (PickerExtensions.cs:27, error): overwrites the nativeBackground(losingBackgroundColor+ caret/underline); thickness-only borders fall back toColors.Transparentand draw invisibly (the sample'sBorderThickness="5"is blank on Android); reverting thickness to 0 leaves the stale drawable instead of restoring the native background. - Android — missing density scaling (
PickerExtensions.cs:45, warning):ThicknessDrawableuses device-independent units directly as physical pixels;BorderDrawablescales byDisplayMetrics.Density. Borders render 2–3x too thin on hdpi+ devices. - Windows — mapper bugs (
Picker.Windows.cs:24, error): both mappers dereferencePlatformViewwithout?.;MapBorderColorcan't clear a previously-set brush (early-return on null);MapBorderThicknessapplies the defaultThickness(0)unconditionally, removing the native ComboBox border from every Picker. - Core —
IPickerbreaking change (IPicker.cs:46, warning):BorderColor/BorderThicknessadded as non-default interface members (breaking for external implementers;IsOpenuses a default impl to avoid this) and currently unused by any Core handler. Use default implementations or defer.
Dropped / merged (synthesis)
13 raw findings across 4 models → clustered to 4. Merged duplicates (density, background-overwrite, null-deref reported by multiple models) and folded the "mappings belong in Core handler" architectural note into finding 4. Dropped as nit: PickerExtensions/CreateBorder being public API surface. iOS/Tizen no-op coverage already raised in PR discussion and acknowledged by the author as a follow-up, so it is noted here rather than inline.
CI
maui-pr failing (Build Analysis + AOT macOS + RunOniOS Release/TrimFull jobs); PR also has a net11.0 merge conflict. Confirm the iOS Release/Trim failures are pre-existing/flaky (this PR touches no iOS code) and resolve the conflict before merge.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ions.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Re-review PR #33093 — Picker border styling (rebased; new head
|
Thanks for this. The rebase was so I could continue working on the review you left 😂. I believe I have fixed the windows null-deref error. I am currently working on the IPicker review and then I will try and do the android fixes (I am having issues building with net11.0-preview5 for android |
Re-review PR #33093 — Picker border styling (new head
|
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Verdict: NEEDS_DISCUSSION (confidence: medium) — re-review after the latest commit.
Multi-model consensus (gpt-5.5 · opus-4.8 · opus-4.6 · gemini): the new commit resolves all prior blocking concerns — the Windows mappers now null-guard PlatformView and use ClearValue() to reset; the Android CreateBorder/ThicknessDrawable now clones and draws over the preserved native background instead of discarding it; the IPicker interface additions were reverted (verified — IPicker no longer declares BorderColor/BorderThickness); and the Picker public-API entries are present across all TFMs. macOS CI is green; Windows/Helix legs are still pending for this commit.
The one item that keeps this at NEEDS_DISCUSSION is a maintainer-policy question, not a code defect: BorderColor/BorderThickness are added as cross-platform public API but are silent no-ops on iOS, MacCatalyst, and Tizen (mappers are only registered under WINDOWS/ANDROID). The author flags iOS as a follow-up — please confirm this partial-platform-API pattern is acceptable.
Key findings (see inline comments):
⚠️ Unrelated PublicAPI baseline churn (BOM additions, entry reordering, Maps/LongPress/ViewExtensions edits) — likely a merge artifact; please revert so the diff only carries the Picker API. (opus-4.8 + independent)⚠️ WindowsMapBorderThickness: set→unset viaClearValueleaves a stale native border. (gemini)⚠️ Android: nativeBackgroundTintListisn't preserved across aBorderColorchange when thickness is 0. (gpt-5.5)- 💡 Orphaned
BorderColor/BorderThicknessmembers onPickerStub(leftover from the IPicker approach). (opus-4.8)
Dropped as false positives after validating against HEAD: "Android border not density-scaled" (it is scaled — thickness * density before the drawable), "Android doesn't restore native Background" (it does — platformView.Background = nativeBackground), and "Windows MapBorderThickness dereferences PlatformView without a null check" (the guard is present). (all from one model)
Model verdicts: gpt-5.5 NEEDS_DISCUSSION · opus-4.8 NEEDS_DISCUSSION · gemini NEEDS_CHANGES · opus-4.6 NEEDS_CHANGES (findings did not survive validation).
| @@ -1,3 +1,3 @@ | |||
| #nullable enable | |||
| #nullable enable | |||
There was a problem hiding this comment.
Unrelated PublicAPI baseline churn (likely a merge artifact). This PR modifies several PublicAPI.Unshipped.txt baselines in ways unrelated to Picker border styling: a UTF-8 BOM is prepended to #nullable enable here and in the Core net-windows/netstandard/netstandard2.0/net-android baselines; src/Core/maps/.../net-windows drops nullable-oblivious IMap duplicates; and the Controls net-android baseline reorders/re-adds unrelated entries (LongPressGestureRecognizer, ViewExtensions.*Async, VisualStateManager, ShellRenderer). This usually happens when a branch absorbs a net11.0/main merge that regenerated the API files. It bloats the diff and invites conflicts. Please revert the BOM/reorder/Maps edits and keep only the four Picker entries (BorderColor, BorderThickness, BorderColorProperty, BorderThicknessProperty) plus the Map*/PickerExtensions additions. Flagged independently by 2 of 4 review models.
| if (pxThickness.IsEmpty) | ||
| { | ||
| platformView.Background = nativeBackground; | ||
| platformView.BackgroundTintList = picker.BorderColor?.ToDefaultColorStateList() ?? nativeBackgroundTintList; |
There was a problem hiding this comment.
Native BackgroundTintList isn't preserved across a BorderColor change when BorderThickness is 0. In this zero-thickness branch the view's tint is overwritten with the BorderColor tint, but GetNativeBackgroundState() only recovers the original background/tint from a ThicknessDrawable — which is not created on this path. So after setting BorderColor (with thickness 0) and then changing/clearing it, the original spinner tint read back by GetNativeBackgroundState() is already the stale BorderColor tint, and the platform default is lost. Consider capturing the original BackgroundTintList once (e.g., stash it the first time you mutate it) so the no-border path can restore the true native tint rather than whatever was last applied.
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Synthesized Review — PR #33093 (Picker BorderColor/BorderThickness)
Verdict: NEEDS_CHANGES (confidence: high) — 3rd re-review, heads ecbd1e06 → a443f369.
What changed in this commit
This re-review covers a small follow-up commit on the Picker border-styling PR (Windows + Android; iOS deferred). The author addressed two findings from the previous round: the orphaned BorderColor/BorderThickness members on PickerStub were removed, and the Windows "set→unset leaves a stale native border" issue was fixed by adding ClearValue(Control.BorderThicknessProperty) to the !IsSet branch of MapBorderThickness — exactly the fix the prior review suggested. The remaining delta is some src/Core PublicAPI.Unshipped.txt shuffling (BOM flips, *REMOVED* marker reordering).
Why NEEDS_CHANGES
That same edit deleted the if (handler.PlatformView is null) return; guard at the top of MapBorderThickness (Picker.Windows.cs:37). The guard was present at the previous head ecbd1e06 — it was even explicitly dropped as a false positive last round — so this is a genuine regression: lines 37, 43, and 47 now dereference handler.PlatformView unguarded and can NRE if the mapper fires before connect / after disconnect, while the sibling MapBorderColor still keeps its guard. All four models flagged this (3 as error), and the ecbd1e06→a443f369 diff confirms the removal. This is the lone blocking issue and forces NEEDS_CHANGES (an error-severity finding precludes LGTM). A secondary, non-blocking design note (:41): because default(Thickness) == new Thickness(0), an explicit BorderThickness="0" hits the == default branch and restores the native border rather than zeroing it — surfaced for an explicit decision, not as a defect.
Carried over / not re-posted inline (to avoid duplicate comments)
Two prior warnings remain unaddressed on files that did not change this commit, so they are noted here rather than re-commented: (a) the Android zero-thickness path overwrites BackgroundTintList without wrapping in ThicknessDrawable, so GetNativeBackgroundState can't recover the original spinner tint after a BorderColor change (PickerExtensions.cs); and (b) unrelated PublicAPI baseline churn (BOM flips plus reordered LongPressGestureRecognizer/ViewExtensions.*Async/Maps IMap entries) that inflates the diff and invites merge conflicts. The open maintainer-policy question also stands: the new properties are cross-platform public API but silent no-ops on iOS/MacCatalyst/Tizen. Dropped opus-4.8's "make PickerExtensions internal" suggestion — public platform extension classes are the dominant convention in that folder (12 public vs 4 internal). CI: maui-pr is red only on iOS Release/TrimFull and AOT-macOS integration legs, which exercise no code in this PR (consistent with pre-existing/flaky infra).
Multi-model synthesis (gpt-5.5 · opus-4.8 · opus-4.6 · gemini-3.1-pro). Comments only — not a formal approval.
| handler.PlatformView?.UpdateVerticalOptions(picker); | ||
| if (!picker.IsSet(BorderThicknessProperty)) | ||
| { | ||
| handler.PlatformView.ClearValue(Control.BorderThicknessProperty); |
There was a problem hiding this comment.
Regression: this commit removed the if (handler.PlatformView is null) return; guard from MapBorderThickness (it was present at the previous head ecbd1e06) while adding the ClearValue call here. Lines 37, 43, and 47 now dereference handler.PlatformView with no null check and will throw an NRE if the mapper runs before connect / after disconnect. MapBorderColor (line 21) still has the guard — restore if (handler.PlatformView is null) return; at the top of this method. Flagged by all 4 review models.
| return; | ||
| } | ||
|
|
||
| if (picker.BorderThickness == default) |
There was a problem hiding this comment.
default(Thickness) equals new Thickness(0), so an explicit BorderThickness="0" also satisfies this check and calls ClearValue (restoring the native border) instead of applying a zero-thickness border — there is no way to remove the border via thickness alone. If that is intended, ignore; otherwise drop the == default check and rely on IsSet.
Description of Change
Added new public API to the Picker Control. This will allow users to set border colours and thicknesses for pickers. Currently, I have only implemented Android and Windows. iOS will be a separate PR (Or chucked onto this one if required)
Windows
Android
Issues Fixed
Partial Fix
#32055
Target Branch
As this introduces an API change, I have targeted the .NET 11.0 branch