Make SetDefault() public on Essentials types for third-party platform backends - #34229
Make SetDefault() public on Essentials types for third-party platform backends#34229Redth wants to merge 10 commits into
Conversation
…form backends Fixes #34100 Third-party platform backends (e.g. Linux/GTK) cannot wire their Essentials implementations into the static Default properties without using private reflection. This change makes the internal SetDefault() method public on all 33 Essentials types, enabling custom backends to register implementations directly: Preferences.SetDefault(new LinuxPreferences()); FilePicker.SetDefault(new LinuxFilePicker()); Changes: - Changed SetDefault() from internal to public on all Essentials types - Added XML documentation to all SetDefault() methods - Updated PublicAPI.Unshipped.txt for all TFMs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR broadens .NET MAUI Essentials extensibility by making the SetDefault(...) registration hook public across all Essentials APIs that expose a static Default implementation, enabling third-party platform backends (e.g., Linux/GTK) to wire their implementations without reflection.
Changes:
- Changed
SetDefault(...)frominternaltopublicacross the EssentialsDefault/SetDefaultpattern types. - Added XML documentation to the newly-public
SetDefault(...)methods (including null-to-reset behavior). - Updated
PublicAPI.Unshipped.txtacross TFMs to reflect the new public surface area.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Essentials/src/WebAuthenticator/WebAuthenticator.shared.cs | Makes SetDefault(IWebAuthenticator?) public and documents it. |
| src/Essentials/src/WebAuthenticator/AppleSignInAuthenticator.shared.cs | Makes SetDefault(IAppleSignInAuthenticator?) public and documents it. |
| src/Essentials/src/Vibration/Vibration.shared.cs | Makes SetDefault(IVibration?) public and documents it. |
| src/Essentials/src/VersionTracking/VersionTracking.shared.cs | Makes SetDefault(IVersionTracking?) public and documents it. |
| src/Essentials/src/TextToSpeech/TextToSpeech.shared.cs | Makes SetDefault(ITextToSpeech?) public and documents it. |
| src/Essentials/src/Sms/Sms.shared.cs | Makes SetDefault(ISms?) public and documents it. |
| src/Essentials/src/Share/Share.shared.cs | Makes SetDefault(IShare?) public and documents it. |
| src/Essentials/src/SemanticScreenReader/SemanticScreenReader.shared.cs | Makes SetDefault(ISemanticScreenReader?) public and documents it. |
| src/Essentials/src/SecureStorage/SecureStorage.shared.cs | Makes SetDefault(ISecureStorage?) public and documents it. |
| src/Essentials/src/Screenshot/Screenshot.shared.cs | Makes SetDefault(IScreenshot?) public and documents it. |
| src/Essentials/src/Preferences/Preferences.shared.cs | Makes SetDefault(IPreferences?) public and documents it. |
| src/Essentials/src/Platform/ActivityStateManager.android.cs | Makes Android ActivityStateManager.SetDefault(...) public and documents it. |
| src/Essentials/src/Platform/WindowStateManager.ios.cs | Makes iOS WindowStateManager.SetDefault(...) public and documents it. |
| src/Essentials/src/Platform/windowstateManager.windows.cs | Makes Windows WindowStateManager.SetDefault(...) public and documents it. |
| src/Essentials/src/PhoneDialer/PhoneDialer.shared.cs | Makes SetDefault(IPhoneDialer?) public and documents it. |
| src/Essentials/src/OrientationSensor/OrientationSensor.shared.cs | Makes SetDefault(IOrientationSensor?) public and documents it. |
| src/Essentials/src/MediaPicker/MediaPicker.shared.cs | Makes SetDefault(IMediaPicker?) public and documents it. |
| src/Essentials/src/Map/Map.shared.cs | Makes SetDefault(IMap?) public and documents it. |
| src/Essentials/src/Magnetometer/Magnetometer.shared.cs | Makes SetDefault(IMagnetometer?) public and documents it. |
| src/Essentials/src/Launcher/Launcher.shared.cs | Makes SetDefault(ILauncher?) public and documents it. |
| src/Essentials/src/HapticFeedback/HapticFeedback.shared.cs | Makes SetDefault(IHapticFeedback?) public and documents it. |
| src/Essentials/src/Gyroscope/Gyroscope.shared.cs | Makes SetDefault(IGyroscope?) public and documents it. |
| src/Essentials/src/Geolocation/Geolocation.shared.cs | Makes SetDefault(IGeolocation?) public and documents it. |
| src/Essentials/src/Flashlight/Flashlight.shared.cs | Makes SetDefault(IFlashlight?) public and documents it. |
| src/Essentials/src/FilePicker/FilePicker.shared.cs | Makes SetDefault(IFilePicker?) public and documents it. |
| src/Essentials/src/Email/Email.shared.cs | Makes SetDefault(IEmail?) public and documents it. |
| src/Essentials/src/Contacts/Contacts.shared.cs | Makes SetDefault(IContacts?) public and documents it. |
| src/Essentials/src/Compass/Compass.shared.cs | Makes SetDefault(ICompass?) public and documents it. |
| src/Essentials/src/Clipboard/Clipboard.shared.cs | Makes SetDefault(IClipboard?) public and documents it. |
| src/Essentials/src/Browser/Browser.shared.cs | Makes SetDefault(IBrowser?) public and documents it. |
| src/Essentials/src/Battery/Battery.shared.cs | Makes SetDefault(IBattery?) public and documents it. |
| src/Essentials/src/Barometer/Barometer.shared.cs | Makes SetDefault(IBarometer?) public and documents it. |
| src/Essentials/src/Accelerometer/Accelerometer.shared.cs | Makes SetDefault(IAccelerometer?) public and documents it. |
| src/Essentials/src/PublicAPI/netstandard/PublicAPI.Unshipped.txt | Adds public API entries for the newly-public SetDefault(...) methods (netstandard). |
| src/Essentials/src/PublicAPI/net/PublicAPI.Unshipped.txt | Adds public API entries for the newly-public SetDefault(...) methods (net). |
| src/Essentials/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt | Adds public API entries including Windows-specific WindowStateManager.SetDefault(...). |
| src/Essentials/src/PublicAPI/net-tizen/PublicAPI.Unshipped.txt | Adds public API entries for the newly-public SetDefault(...) methods (Tizen). |
| src/Essentials/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt | Adds public API entries including WindowStateManager.SetDefault(...) (MacCatalyst). |
| src/Essentials/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt | Adds public API entries including WindowStateManager.SetDefault(...) (iOS). |
| src/Essentials/src/PublicAPI/net-android/PublicAPI.Unshipped.txt | Adds public API entries including Android-specific ActivityStateManager.SetDefault(...). |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34229 | Make SetDefault(...) public on all Essentials types using the static Default/SetDefault pattern, add XML docs, and update PublicAPI files across TFMs |
⏳ PENDING (Gate) | 40 files | Original PR |
🚦 Gate — Test Verification
Gate Result: ⚠️ SKIPPED
Platform: android
Mode: Full Verification
- Tests FAIL without fix: ❌
- Tests PASS with fix: ❌
Reason: PR #34229 does not add or modify any test files. The changed file set is 33 implementation files plus 7 PublicAPI.Unshipped.txt files, so there is no PR-supplied test to run through fail-without-fix / pass-with-fix verification.
Recommendation from Gate: Add targeted coverage (likely Essentials unit tests) to prove an external caller can replace Default via the newly-public SetDefault(...) methods.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-opus-4.6) | Add public Geocoding.SetDefault(...), keep internal SetCurrent(...), and add focused Geocoding unit tests |
✅ PASS | Geocoding.shared.cs, Geocoding_Tests.cs, 7 PublicAPI files |
Additive, backward-compatible fix for the omitted API |
| 2 | try-fix (claude-sonnet-4.6) | Rename Geocoding.SetCurrent(...) directly to public SetDefault(...) and add focused Geocoding unit tests |
✅ PASS | Geocoding.shared.cs, Geocoding_Tests.cs, 7 PublicAPI files |
Cleanest naming consistency; no internal callers of SetCurrent remain |
| 3 | try-fix (gpt-5.3-codex) | Add public SetDefault(...) and expose public SetCurrent(...) as a forwarding compatibility alias |
✅ PASS | Geocoding.shared.cs, Geocoding_Tests.cs, 7 PublicAPI files |
Works, but broadens public API more than the issue requires |
| 4 | try-fix (gemini-3-pro-preview) | Make Geocoding.Default publicly settable and add focused unit tests |
✅ PASS | Geocoding.shared.cs, Geocoding_Tests.cs, 7 PublicAPI files |
Functional, but changes the public shape more than other Essentials APIs |
| 5 | try-fix (gpt-5.3-codex round 2) | Add public ConfigureDefault(Func<IGeocoding?>?) resolver hook with lazy caching, plus focused tests |
✅ PASS | Geocoding.shared.cs, Geocoding_Tests.cs, 7 PublicAPI files |
Materially different, but introduces a one-off API pattern unlike the rest of Essentials |
| 6 | try-fix (claude-opus-4.6 round 2) | Add public SetServiceProvider(IServiceProvider?) so Default resolves IGeocoding from DI first |
✅ PASS | Geocoding.shared.cs, Geocoding_Tests.cs, 7 PublicAPI files |
Architecturally distinct, but much broader than the PR and not aligned with other Essentials APIs |
| 7 | try-fix (gemini-3-pro-preview round 2) | Add ConfigureDefault(...) in Essentials plus a MauiAppBuilder.ConfigureGeocoding(...) integration path in Core |
✅ PASS | Geocoding.shared.cs, Geocoding_Tests.cs, Core hosting/API files, PublicAPI files |
Works, but expands a one-file PR gap into a multi-assembly design change |
| PR | PR #34229 | Make existing SetDefault(...) methods public on 33 Essentials types and update docs/PublicAPI |
40 files | No PR-supplied tests to verify fail-without-fix / pass-with-fix; still misses Geocoding |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 1 | Yes | Add a public SetDefault(...) wrapper specifically for Geocoding and validate with targeted unit tests. |
| claude-sonnet-4.6 | 1 | Yes | Replace SetCurrent(...) with public SetDefault(...) for Geocoding and validate with targeted unit tests. |
| gpt-5.3-codex | 1 | Yes | Expose both public names, with SetCurrent(...) forwarding to SetDefault(...), and validate with targeted unit tests. |
| gemini-3-pro-preview | 1 | Yes | Make Geocoding.Default settable instead of adding another method. |
| claude-opus-4.6 | 2 | Yes | Resolve IGeocoding from DI/services via public SetServiceProvider(IServiceProvider?). |
| claude-sonnet-4.6 | 2 | No | NO NEW IDEAS |
| gpt-5.3-codex | 2 | Yes | Add a public resolver/factory hook ConfigureDefault(Func<IGeocoding?>?) with lazy caching. |
| gemini-3-pro-preview | 2 | Yes | Add a MauiAppBuilder.ConfigureGeocoding(IGeocoding) extension method instead of changing Geocoding directly. |
| claude-opus-4.6 | 3 | No | NO NEW IDEAS |
| claude-sonnet-4.6 | 3 | Yes | Generic cross-Essentials registry idea — broader than this PR and not attempted because round limit reached. |
| gpt-5.3-codex | 3 | Yes | Implicit DI via IPlatformApplication.Current.Services — broader than this PR and not attempted because round limit reached. |
| gemini-3-pro-preview | 3 | Yes | Event-based pull registration — broader than this PR and not attempted because round limit reached. |
Exhausted: Yes — final allowed cross-pollination round reached
Selected Fix: Candidate #2 — smallest fix that fully addresses the missed API while matching the exact pattern already used by the other Essentials types in this PR
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Linked issue #34100, file classification, and PR discussion gathered. |
| Gate | PR adds no tests, so fail-without-fix / pass-with-fix verification could not run. | |
| Try-Fix | ✅ COMPLETE | 7 attempts, 7 passing candidates; best candidate is not the PR as written. |
| Report | ✅ COMPLETE |
Summary
PR #34229 correctly publicizes SetDefault(...) on 33 Essentials types, but it leaves one relevant API behind: Geocoding. Geocoding still exposes public static IGeocoding Default while only providing internal static void SetCurrent(IGeocoding? implementation), so third-party backends still cannot register a custom Geocoding implementation without reflection.
Because of that omission, the PR does not fully satisfy issue #34100. It also ships without targeted tests proving the new public registration hooks work.
Root Cause
The PR appears to have enumerated only the existing Default/SetDefault pattern and missed the one inconsistent outlier: src/Essentials/src/Geocoding/Geocoding.shared.cs, which uses Default plus SetCurrent(...) instead. That naming inconsistency caused Geocoding to be excluded from the mechanical update, leaving the issue only partially solved.
Fix Quality
The existing PR changes are mechanically consistent for the 33 touched APIs, and the PublicAPI updates for those APIs look correct. The problem is completeness: the same user-facing extensibility hole remains for Geocoding.
The strongest alternative from Try-Fix was candidate #2: rename Geocoding.SetCurrent(...) directly to public SetDefault(...), add XML documentation, update the 7 PublicAPI.Unshipped.txt files, and add focused Geocoding_Tests coverage. That option passed targeted unit tests, matches the exact pattern already used by the other Essentials types in this PR, and is smaller/cleaner than the broader DI or builder-based variants.
MauiBot
added
s/agent-changes-requested
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34229Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34229" |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…#34265) <!-- Please let the below note in for people that find this PR --> > [!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! ## Summary Adds a standalone `code-review` skill for reviewing PR code changes with MAUI-specific domain knowledge, modeled after [dotnet/android's android-reviewer skill](https://github.com/dotnet/android/tree/main/.github/skills/android-reviewer). ### What's included **`.github/skills/code-review/SKILL.md`** (196 lines) — Review workflow: - Independence-first methodology (read code before PR description to avoid anchoring bias) - 6-step workflow: gather context → load rules → independent assessment → reconcile with PR narrative → check CI → devil's advocate verdict - Three verdicts: `LGTM`, `NEEDS_CHANGES`, `NEEDS_DISCUSSION` - Optional multi-model review for diverse perspectives - Output constraints: don't pile on, don't flag what CI catches **`.github/skills/code-review/references/review-rules.md`** (345 lines) — Domain knowledge: - **22 sections** of review rules distilled from real FTE code reviews - **138 PR citations** linking each rule to the PR where the pattern was identified - Sourced from **366 PRs**: top 142 most-discussed PRs (2,883 review comments), 30 reverted PRs, 50 candidate-branch failures, 64 regression fixes - Covers: handler lifecycle, memory leaks, safe area, layout, platform code, Android/iOS/Windows specifics, navigation, CollectionView, threading, XAML, testing, performance, error handling, API design, images, gestures, build, accessibility - §21 Regression Prevention — lessons from reverts and candidate failures - §22 Component Risk Table — ranking the most regression-prone components ### Design decisions - **Separation of concerns**: SKILL.md = workflow, review-rules.md = knowledge (follows Android's pattern) - **Standalone**: Can be invoked directly by users or by other agents. No coupling to the PR agent pipeline. - **Data-driven**: Every rule traces to a real PR where a senior maintainer flagged the pattern. No generic advice. ### Changes to copilot-instructions.md - Added "Opening PRs" section with required NOTE block template - Added code-review skill to the skills listing - Minor reformatting of existing documentation sections --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
- Disable parallel execution for SetDefault_Tests via a dedicated xUnit collection to avoid shared static Default implementation races across tests. - Remove unused System.Threading using. - Fix MockFilePicker.PickMultipleAsync signature nullability to match IFilePicker. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed today’s new Copilot suggestion by adding a guard-rail reflection test. ✅ Added
✅ Validation run
✅ Push
|
- Add AllEssentialsTypes_ExposePublicSetDefault test to ensure every static Essentials type with a public interface-typed Default property also exposes a matching public static void SetDefault(<interface>) method. - Uses reflection over the Essentials assembly and fails with a specific missing method list to prevent future accidental API pattern regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
…rloads 34 types coverage) - Add double-set test verifying second SetDefault call overrides first - Fix MockPreferences: remove superfluous single-param overloads not on IPreferences interface - Add mocks: MockClipboard, MockSecureStorage, MockBattery with delegation verification Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
… type categories - Add null-reset tests for Clipboard, SecureStorage, Battery - Add IsUsed behavioral tests for Preferences and FilePicker - Add full test coverage for Share, Launcher, Accelerometer (sensor/media/app types) - All 9 tested types now have consistent 3-test pattern: set, null-reset, is-used Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
🧪 PR Test EvaluationOverall Verdict: The unit tests are well-structured and use the right test type, but behavioral coverage exists for only 9 of 31 changed API types. The reflection guardrail catches API surface issues, but doesn't verify the plumbing actually works for the remaining 22 types.
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34229 — Essentials Overall VerdictThe test file is well-designed with a smart reflection guardrail and the right test type (unit tests). However, behavioral tests cover only 9 of the 31 types that gained a public 1. Fix Coverage —
|
Multi-Model Code Review — PR #34229PR: Make 🟡 MODERATE — Target branch should be
|
|
Ignoring the target-branch discussion, here's my take on the remaining points:
|
|
Closing in favour of #35068 after some discussion about moving more towards obsoleting the |
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!
Description
Fixes #34100
Third-party platform backends (e.g. Linux/GTK) cannot wire their Essentials implementations into the static
Defaultproperties (FilePicker.Default,Preferences.Default,SecureStorage.Default, etc.) without using private reflection. The internalSetDefault()method is currently the only way to set these, but it is not accessible to external code.This PR makes
SetDefault()public on all 33 Essentials types that follow theDefault/SetDefaultpattern, enabling custom backends to register their implementations directly:Changes
SetDefault()frominternaltopublicon all 33 Essentials types:SetDefault()methodsPublicAPI.Unshipped.txtfor all 7 TFMs (net, net-android, net-ios, net-maccatalyst, net-windows, net-tizen, netstandard)Affected Types
All types under
Microsoft.Maui.Storage,Microsoft.Maui.ApplicationModel,Microsoft.Maui.Devices,Microsoft.Maui.Media,Microsoft.Maui.Accessibility, andMicrosoft.Maui.Authenticationthat expose a staticDefaultproperty backed by aSetDefault()method.