Add IViewScreenshot for third-party screenshot extensibility#34350
Add IViewScreenshot for third-party screenshot extensibility#34350Redth wants to merge 8 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34350Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34350" |
There was a problem hiding this comment.
Pull request overview
Adds a new runtime-extensible element/window screenshot path so third-party platform backends (without #if PLATFORM support) can participate in view.CaptureAsync() / window.CaptureAsync() via DI.
Changes:
- Introduces a new public
Microsoft.Maui.Media.IViewScreenshotinterface for platform-agnostic view-level capture (CaptureViewAsync(object platformView)). - Updates built-in platform
ScreenshotImplementationtypes (Android/iOS/Windows/Tizen) to implementIViewScreenshot. - Updates Core
ViewExtensions.CaptureAsync/WindowExtensions.CaptureAsyncnon-PLATFORMpaths to resolveIScreenshotfromMauiContext.Servicesand invokeIViewScreenshotwhen available.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Essentials/src/Screenshot/Screenshot.shared.cs | Adds the new IViewScreenshot public API contract. |
| src/Essentials/src/Screenshot/Screenshot.android.cs | Implements IViewScreenshot on Android by delegating to existing view capture. |
| src/Essentials/src/Screenshot/Screenshot.ios.cs | Implements IViewScreenshot on iOS by delegating to existing UIView capture. |
| src/Essentials/src/Screenshot/Screenshot.windows.cs | Implements IViewScreenshot on Windows by delegating to existing UIElement capture. |
| src/Essentials/src/Screenshot/Screenshot.tizen.cs | Implements IViewScreenshot on Tizen (currently throws for supported capture paths). |
| src/Essentials/src/PublicAPI/netstandard/PublicAPI.Unshipped.txt | Declares the new IViewScreenshot API for netstandard. |
| src/Essentials/src/PublicAPI/net/PublicAPI.Unshipped.txt | Declares the new IViewScreenshot API for net. |
| src/Essentials/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt | Declares the new IViewScreenshot API for net-windows. |
| src/Essentials/src/PublicAPI/net-tizen/PublicAPI.Unshipped.txt | Declares the new IViewScreenshot API for net-tizen. |
| src/Essentials/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt | Declares the new IViewScreenshot API for net-maccatalyst. |
| src/Essentials/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt | Declares the new IViewScreenshot API for net-ios. |
| src/Essentials/src/PublicAPI/net-android/PublicAPI.Unshipped.txt | Declares the new IViewScreenshot API for net-android. |
| src/Core/src/ViewExtensions.cs | Adds DI-based fallback in non-PLATFORM builds to use IViewScreenshot. |
| src/Core/src/WindowExtensions.cs | Adds DI-based fallback in non-PLATFORM builds to use IViewScreenshot. |
This comment has been minimized.
This comment has been minimized.
🧪 PR Test EvaluationOverall Verdict: The unit tests are well-structured and cover the core
📊 Expand Full EvaluationPR Test Evaluation ReportPR: #34350 — Screenshot extensibility via Overall VerdictThe unit tests cover the 1. Fix Coverage —
|
| Test scenario | View |
Window |
|---|---|---|
| Null handler | ✅ | ✅ |
| Null platform view | ✅ | ❌ Missing |
| Screenshot service not registered | ✅ | ❌ Missing |
| IsCaptureSupported = false | ✅ | ❌ Missing |
| IScreenshot doesn't implement IViewScreenshot | ✅ | ✅ |
| Happy path | ✅ | ✅ |
2. Edge Cases & Gaps — ⚠️
Covered:
- Null handler for both
IViewandIWindow - Null platform view for
IView - Service not registered in DI for
IView IsCaptureSupported = falseforIViewIScreenshotnot implementingIViewScreenshotfor both- Happy path returning
IScreenshotResultfor bothIViewandIWindow
Missing:
CaptureAsync_Window_ReturnsNull_WhenPlatformViewIsNull— theWindowExtensionscode has the same null-check asViewExtensionsbut it's untestedCaptureAsync_Window_ReturnsNull_WhenScreenshotServiceNotRegistered— same gapCaptureAsync_Window_ReturnsNull_WhenCaptureNotSupported— same gap- No test for when
IViewScreenshot.CaptureViewAsyncitself returnsnull(e.g., on Android:platformView is not Android.Views.View→ returns null) — this is the failure path within the platform implementation
3. Test Type Appropriateness — ✅
Current: Unit Tests (xUnit)
Recommendation: Same — appropriate choice.
The non-platform #else branch being tested is pure DI/service-resolution logic with no platform context needed. Unit tests with mocks (NSubstitute) are the correct lightweight approach for this code path.
The platform IViewScreenshot implementations could benefit from device tests, but those would be significantly heavier and the one-liner delegate to existing CaptureAsync methods is low-risk.
4. Convention Compliance — ✅
No convention issues found:
- 9
[Fact]methods (xUnit) ✅ - Appropriate
[System.ComponentModel.Category]attribute ✅ - Proper use of
NSubstitutemocks ✅ async Taskreturn types on all async tests ✅
5. Flakiness Risk — ✅ Low
Pure synchronous mocking with NSubstitute. No timing dependencies, no platform interaction, no async race conditions. These tests should be stable in CI.
6. Duplicate Coverage — ✅ No duplicates
No similar existing tests found for ViewExtensions.CaptureAsync or WindowExtensions.CaptureAsync in the unit test project. This is new test coverage for new functionality.
7. Platform Scope — ⚠️
The fix modifies 5 platform-specific files (Android, iOS, Windows, Tizen, and shared), but the unit tests only cover the platform-agnostic #else branch. The platform-specific IViewScreenshot.CaptureViewAsync implementations — each with their own type-cast logic — have no tests:
Screenshot.android.cs:platformView is View view ? CaptureAsync(view)! : Task.FromResult(IScreenshotResult?)(null)Screenshot.ios.cs: Similar cast patternScreenshot.windows.cs: Similar cast patternScreenshot.tizen.cs: Similar cast pattern
The case where the cast fails (wrong platform view type passed) returns null but is not tested in any project. Device tests would be ideal here, but given the simplicity of the implementations, this is a minor concern.
8. Assertion Quality — ✅
Assertions are precise and meaningful:
Assert.Null(result)— correctly verifies null-return pathsAssert.Same(expectedResult, result)— verifies the exact mock instance is returned, catching any wrapping or substitution bugs
9. Fix-Test Alignment — ✅
The test class directly maps to the two changed extension files: it tests both IView.CaptureAsync() (from ViewExtensions.cs) and IWindow.CaptureAsync() (from WindowExtensions.cs). The tested code paths are exactly the branches added by the PR.
Recommendations
-
Add 3 missing
Windowtests to match theViewtest coverage:CaptureAsync_Window_ReturnsNull_WhenPlatformViewIsNull,CaptureAsync_Window_ReturnsNull_WhenScreenshotServiceNotRegistered, andCaptureAsync_Window_ReturnsNull_WhenCaptureNotSupported. These are straightforward copies of the existingViewequivalents withIWindow/IElementHandlersubstitutes. -
Consider a test for
CaptureViewAsyncreturning null — e.g., register aIViewScreenshotmock that returnsnullfromCaptureViewAsyncand verifyCaptureAsyncpropagates it. This would catch any future wrapping or non-null coercion added to the extension methods. -
Platform device tests are optional but welcome — the platform
CaptureViewAsyncimplementations are simple one-liners, but if this feature is important to validate end-to-end, a device test for each platform (Android, iOS, Windows) that callsview.CaptureAsync()and asserts the result is non-null would provide confidence.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
dc.services.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "dc.services.visualstudio.com"See Network Configuration for more information.
Note
🔒 Integrity filtering filtered 2 items
Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
- pr:Add IViewScreenshot for third-party screenshot extensibility #34350 (
pull_request_read: Resource 'pr:Add IViewScreenshot for third-party screenshot extensibility #34350' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.) - pr:Add IViewScreenshot for third-party screenshot extensibility #34350 (
pull_request_read: Resource 'pr:Add IViewScreenshot for third-party screenshot extensibility #34350' has lower integrity than agent requires. Agent would need to drop integrity tags [approved:all unapproved:all] to trust this resource.)
🧪 Test evaluation by Evaluate PR Tests
📋 Review Summary — PR #34350Status: Your previous commit successfully addressed the 8 Copilot inline comments:
However, a comprehensive three-model code review identified 2 MODERATE issues and several MINOR issues that need attention: 🔴 MODERATE Issues1. Tizen
|
| Finding | Opus | Sonnet | Codex | Consensus |
|---|---|---|---|---|
| Tizen throws | MODERATE | MODERATE | Flagged | All 3 agree |
| ContainerView bypass | MODERATE | MINOR | Noted | All 3 noted |
| Window test gaps | MINOR | MINOR | MODERATE | All 3 agree |
| IsCaptureSupported throws (Tizen) | MINOR | MODERATE | Noted | All 3 noted |
Nullable ! operator |
MINOR | MINOR | — | Acceptable |
✅ What Was Done Well
- ✅ Nullable annotations correctly fixed (changed to
annotations-only mode) - ✅ Null-forgiving operators justified and safe
- ✅ DI pattern is clean and extensible
- ✅ Architecture supports third-party backends elegantly
- ✅ All original 8 Copilot comments addressed
- ✅ Core and Edge unit tests pass
🎯 Action Items
Before merge:
- Fix Tizen
CaptureViewAsyncto return null instead of throwing (5-10 min) - Add ContainerView documentation to
IViewScreenshotXML doc (2-5 min) - Optional: Add 3 missing Window unit tests for edge cases (10-15 min)
Post-merge (if desired):
- Add DI registration example to
IViewScreenshotXML docs - Consider extracting
IViewScreenshotto its own file (cosmetic)
📋 CI Status
- ✅ Core unit tests: All pass
⚠️ Build analysis failing (pre-existing infra issue, not PR-specific)⚠️ macOS pack failing (pre-existing infra issue, not PR-specific)
Recommendation
CaptureViewAsync throw issue and add ContainerView documentation. The three-model review consensus is that these are real issues that should be addressed before merge. The Window test gap is optional but recommended.
|
@Redth should this one be targeted to net11? |
…ackends (#35096) <!-- 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! Fixes #34266 ## Problem `ViewExtensions.CaptureAsync(IView)` and `WindowExtensions.CaptureAsync(IWindow)` are gated by `#if PLATFORM`. On any TFM that isn't one of MAUI's built-in platform targets (Android, iOS/MacCatalyst, Windows, Tizen) they return `null`. That blocks third-party platform backends — for example macOS AppKit or Linux/GTK — from plugging into `VisualDiagnostics.CaptureAsPngAsync(view)` or the public view-level capture API. ## Relationship to #34350 PR #34350 solves the same problem by adding a **new public `IViewScreenshot` interface**. Because that is a public API addition, it can only ship in .NET 11. This PR takes a complementary approach with **zero public API additions**, so it can ship in **.NET 10**. The two PRs coexist: when #34350 lands, its `is IViewScreenshot` fast path runs *before* the keyed-DI fallback introduced here. Backends that register under .NET 10 via this mechanism continue to work unchanged in .NET 11. ## How it works A new internal `ScreenshotDispatch` helper routes the non-`PLATFORM` `#else` branches of the two extension methods through a keyed DI lookup. The contract uses only BCL types: ```csharp Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.ViewCapture" Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.WindowCapture" ``` The lambda receives `handler.PlatformView` and returns an `IScreenshotResult?`. When no hook is registered (or `PlatformView` is `null`, or services aren't keyed) the call resolves to `null` — same as before. ### Backend registration A third-party platform backend registers the hook once during builder configuration. For example, a hypothetical AppKit backend: ```csharp builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.ViewCapture", (_, _) => platformView => ((AppKit.NSView)platformView).CaptureAsync()); builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.WindowCapture", (_, _) => platformView => ((AppKit.NSWindow)platformView).CaptureAsync()); ``` ## Why keyed DI (vs. reflection) An earlier sketch considered convention-based reflection dispatch against `IScreenshot.CaptureAsync(TPlatformView)`. Keyed DI was chosen because it is strictly safer for trimming and AOT: - **No reflection.** No `MethodInfo.Invoke`, no `Expression.Compile`, no `CreateDelegate`. - **No `[DynamicDependency]` burden on the backend.** The typed capture method is reached via normal lambda-closure reachability. - Microsoft.Extensions.DependencyInjection keyed services are already used elsewhere in MAUI Core (e.g. `KeyedWrappedServiceProvider`, `GetKeyedService<IDispatcher>`), so nothing new is taken on. ## Scope - ✅ The `PLATFORM` code path (Android, iOS, MacCatalyst, Windows, Tizen) is **untouched** — zero behavior change on built-in platforms. - ✅ Zero new public API surface. `ScreenshotDispatch` is `internal`; the key strings are documented in XML docs. - ✅ Essentials (`IPlatformScreenshot`, `ScreenshotImplementation`) is **untouched**. ## Tests 11 new unit tests in `Core.UnitTests.Extensions.ScreenshotDispatchTests` (targets `net10.0`, i.e. the exact non-`PLATFORM` path exercised by this change): - hook registered → invoked with `PlatformView`, result propagated - hook not registered → `null` - `PlatformView` null → `null` - handler null / `IView` null → `null` - hook registered under wrong key → `null` - hook returns null task → `null` propagated - view and window hooks coexist without interference All pass locally. ## Files changed - `src/Core/src/ScreenshotDispatch.cs` — new internal helper + key constants - `src/Core/src/ViewExtensions.cs` — `#else` branch delegates to dispatcher; XML docs updated with registration example - `src/Core/src/WindowExtensions.cs` — same treatment as `ViewExtensions` - `src/Core/tests/UnitTests/Extensions/ScreenshotDispatchTests.cs` — 11 tests --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ackends (#35096) <!-- 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! Fixes #34266 ## Problem `ViewExtensions.CaptureAsync(IView)` and `WindowExtensions.CaptureAsync(IWindow)` are gated by `#if PLATFORM`. On any TFM that isn't one of MAUI's built-in platform targets (Android, iOS/MacCatalyst, Windows, Tizen) they return `null`. That blocks third-party platform backends — for example macOS AppKit or Linux/GTK — from plugging into `VisualDiagnostics.CaptureAsPngAsync(view)` or the public view-level capture API. ## Relationship to #34350 PR #34350 solves the same problem by adding a **new public `IViewScreenshot` interface**. Because that is a public API addition, it can only ship in .NET 11. This PR takes a complementary approach with **zero public API additions**, so it can ship in **.NET 10**. The two PRs coexist: when #34350 lands, its `is IViewScreenshot` fast path runs *before* the keyed-DI fallback introduced here. Backends that register under .NET 10 via this mechanism continue to work unchanged in .NET 11. ## How it works A new internal `ScreenshotDispatch` helper routes the non-`PLATFORM` `#else` branches of the two extension methods through a keyed DI lookup. The contract uses only BCL types: ```csharp Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.ViewCapture" Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.WindowCapture" ``` The lambda receives `handler.PlatformView` and returns an `IScreenshotResult?`. When no hook is registered (or `PlatformView` is `null`, or services aren't keyed) the call resolves to `null` — same as before. ### Backend registration A third-party platform backend registers the hook once during builder configuration. For example, a hypothetical AppKit backend: ```csharp builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.ViewCapture", (_, _) => platformView => ((AppKit.NSView)platformView).CaptureAsync()); builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.WindowCapture", (_, _) => platformView => ((AppKit.NSWindow)platformView).CaptureAsync()); ``` ## Why keyed DI (vs. reflection) An earlier sketch considered convention-based reflection dispatch against `IScreenshot.CaptureAsync(TPlatformView)`. Keyed DI was chosen because it is strictly safer for trimming and AOT: - **No reflection.** No `MethodInfo.Invoke`, no `Expression.Compile`, no `CreateDelegate`. - **No `[DynamicDependency]` burden on the backend.** The typed capture method is reached via normal lambda-closure reachability. - Microsoft.Extensions.DependencyInjection keyed services are already used elsewhere in MAUI Core (e.g. `KeyedWrappedServiceProvider`, `GetKeyedService<IDispatcher>`), so nothing new is taken on. ## Scope - ✅ The `PLATFORM` code path (Android, iOS, MacCatalyst, Windows, Tizen) is **untouched** — zero behavior change on built-in platforms. - ✅ Zero new public API surface. `ScreenshotDispatch` is `internal`; the key strings are documented in XML docs. - ✅ Essentials (`IPlatformScreenshot`, `ScreenshotImplementation`) is **untouched**. ## Tests 11 new unit tests in `Core.UnitTests.Extensions.ScreenshotDispatchTests` (targets `net10.0`, i.e. the exact non-`PLATFORM` path exercised by this change): - hook registered → invoked with `PlatformView`, result propagated - hook not registered → `null` - `PlatformView` null → `null` - handler null / `IView` null → `null` - hook registered under wrong key → `null` - hook returns null task → `null` propagated - view and window hooks coexist without interference All pass locally. ## Files changed - `src/Core/src/ScreenshotDispatch.cs` — new internal helper + key constants - `src/Core/src/ViewExtensions.cs` — `#else` branch delegates to dispatcher; XML docs updated with registration example - `src/Core/src/WindowExtensions.cs` — same treatment as `ViewExtensions` - `src/Core/tests/UnitTests/Extensions/ScreenshotDispatchTests.cs` — 11 tests --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ackends (#35096) <!-- 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! Fixes #34266 ## Problem `ViewExtensions.CaptureAsync(IView)` and `WindowExtensions.CaptureAsync(IWindow)` are gated by `#if PLATFORM`. On any TFM that isn't one of MAUI's built-in platform targets (Android, iOS/MacCatalyst, Windows, Tizen) they return `null`. That blocks third-party platform backends — for example macOS AppKit or Linux/GTK — from plugging into `VisualDiagnostics.CaptureAsPngAsync(view)` or the public view-level capture API. ## Relationship to #34350 PR #34350 solves the same problem by adding a **new public `IViewScreenshot` interface**. Because that is a public API addition, it can only ship in .NET 11. This PR takes a complementary approach with **zero public API additions**, so it can ship in **.NET 10**. The two PRs coexist: when #34350 lands, its `is IViewScreenshot` fast path runs *before* the keyed-DI fallback introduced here. Backends that register under .NET 10 via this mechanism continue to work unchanged in .NET 11. ## How it works A new internal `ScreenshotDispatch` helper routes the non-`PLATFORM` `#else` branches of the two extension methods through a keyed DI lookup. The contract uses only BCL types: ```csharp Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.ViewCapture" Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.WindowCapture" ``` The lambda receives `handler.PlatformView` and returns an `IScreenshotResult?`. When no hook is registered (or `PlatformView` is `null`, or services aren't keyed) the call resolves to `null` — same as before. ### Backend registration A third-party platform backend registers the hook once during builder configuration. For example, a hypothetical AppKit backend: ```csharp builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.ViewCapture", (_, _) => platformView => ((AppKit.NSView)platformView).CaptureAsync()); builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.WindowCapture", (_, _) => platformView => ((AppKit.NSWindow)platformView).CaptureAsync()); ``` ## Why keyed DI (vs. reflection) An earlier sketch considered convention-based reflection dispatch against `IScreenshot.CaptureAsync(TPlatformView)`. Keyed DI was chosen because it is strictly safer for trimming and AOT: - **No reflection.** No `MethodInfo.Invoke`, no `Expression.Compile`, no `CreateDelegate`. - **No `[DynamicDependency]` burden on the backend.** The typed capture method is reached via normal lambda-closure reachability. - Microsoft.Extensions.DependencyInjection keyed services are already used elsewhere in MAUI Core (e.g. `KeyedWrappedServiceProvider`, `GetKeyedService<IDispatcher>`), so nothing new is taken on. ## Scope - ✅ The `PLATFORM` code path (Android, iOS, MacCatalyst, Windows, Tizen) is **untouched** — zero behavior change on built-in platforms. - ✅ Zero new public API surface. `ScreenshotDispatch` is `internal`; the key strings are documented in XML docs. - ✅ Essentials (`IPlatformScreenshot`, `ScreenshotImplementation`) is **untouched**. ## Tests 11 new unit tests in `Core.UnitTests.Extensions.ScreenshotDispatchTests` (targets `net10.0`, i.e. the exact non-`PLATFORM` path exercised by this change): - hook registered → invoked with `PlatformView`, result propagated - hook not registered → `null` - `PlatformView` null → `null` - handler null / `IView` null → `null` - hook registered under wrong key → `null` - hook returns null task → `null` propagated - view and window hooks coexist without interference All pass locally. ## Files changed - `src/Core/src/ScreenshotDispatch.cs` — new internal helper + key constants - `src/Core/src/ViewExtensions.cs` — `#else` branch delegates to dispatcher; XML docs updated with registration example - `src/Core/src/WindowExtensions.cs` — same treatment as `ViewExtensions` - `src/Core/tests/UnitTests/Extensions/ScreenshotDispatchTests.cs` — 11 tests --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review -b feature/refactor-copilot-yml |
…ackends (#35096) <!-- 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! Fixes #34266 ## Problem `ViewExtensions.CaptureAsync(IView)` and `WindowExtensions.CaptureAsync(IWindow)` are gated by `#if PLATFORM`. On any TFM that isn't one of MAUI's built-in platform targets (Android, iOS/MacCatalyst, Windows, Tizen) they return `null`. That blocks third-party platform backends — for example macOS AppKit or Linux/GTK — from plugging into `VisualDiagnostics.CaptureAsPngAsync(view)` or the public view-level capture API. ## Relationship to #34350 PR #34350 solves the same problem by adding a **new public `IViewScreenshot` interface**. Because that is a public API addition, it can only ship in .NET 11. This PR takes a complementary approach with **zero public API additions**, so it can ship in **.NET 10**. The two PRs coexist: when #34350 lands, its `is IViewScreenshot` fast path runs *before* the keyed-DI fallback introduced here. Backends that register under .NET 10 via this mechanism continue to work unchanged in .NET 11. ## How it works A new internal `ScreenshotDispatch` helper routes the non-`PLATFORM` `#else` branches of the two extension methods through a keyed DI lookup. The contract uses only BCL types: ```csharp Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.ViewCapture" Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.WindowCapture" ``` The lambda receives `handler.PlatformView` and returns an `IScreenshotResult?`. When no hook is registered (or `PlatformView` is `null`, or services aren't keyed) the call resolves to `null` — same as before. ### Backend registration A third-party platform backend registers the hook once during builder configuration. For example, a hypothetical AppKit backend: ```csharp builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.ViewCapture", (_, _) => platformView => ((AppKit.NSView)platformView).CaptureAsync()); builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.WindowCapture", (_, _) => platformView => ((AppKit.NSWindow)platformView).CaptureAsync()); ``` ## Why keyed DI (vs. reflection) An earlier sketch considered convention-based reflection dispatch against `IScreenshot.CaptureAsync(TPlatformView)`. Keyed DI was chosen because it is strictly safer for trimming and AOT: - **No reflection.** No `MethodInfo.Invoke`, no `Expression.Compile`, no `CreateDelegate`. - **No `[DynamicDependency]` burden on the backend.** The typed capture method is reached via normal lambda-closure reachability. - Microsoft.Extensions.DependencyInjection keyed services are already used elsewhere in MAUI Core (e.g. `KeyedWrappedServiceProvider`, `GetKeyedService<IDispatcher>`), so nothing new is taken on. ## Scope - ✅ The `PLATFORM` code path (Android, iOS, MacCatalyst, Windows, Tizen) is **untouched** — zero behavior change on built-in platforms. - ✅ Zero new public API surface. `ScreenshotDispatch` is `internal`; the key strings are documented in XML docs. - ✅ Essentials (`IPlatformScreenshot`, `ScreenshotImplementation`) is **untouched**. ## Tests 11 new unit tests in `Core.UnitTests.Extensions.ScreenshotDispatchTests` (targets `net10.0`, i.e. the exact non-`PLATFORM` path exercised by this change): - hook registered → invoked with `PlatformView`, result propagated - hook not registered → `null` - `PlatformView` null → `null` - handler null / `IView` null → `null` - hook registered under wrong key → `null` - hook returns null task → `null` propagated - view and window hooks coexist without interference All pass locally. ## Files changed - `src/Core/src/ScreenshotDispatch.cs` — new internal helper + key constants - `src/Core/src/ViewExtensions.cs` — `#else` branch delegates to dispatcher; XML docs updated with registration example - `src/Core/src/WindowExtensions.cs` — same treatment as `ViewExtensions` - `src/Core/tests/UnitTests/Extensions/ScreenshotDispatchTests.cs` — 11 tests --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review -b feature/refactor-copilot-yml |
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions?
|
/review -b feature/enhanced-reviewer -p android |
This comment has been minimized.
This comment has been minimized.
|
|
🤖 Multimodal code review summaryRan an independent multi-model review with Claude Opus 4.8, GPT-5.5, and Gemini 3.1 Pro, plus a CI triage. 🔧 Finding to address —
|
kubaflo
left a comment
There was a problem hiding this comment.
🤖 Round-2 review — ViewExtensions parity fix verified
I implemented and verified the fix for the one substantive round-1 finding (the container-view parity gap), and confirmed it against the non-platform unit tests.
What I changed (prepared on kubaflo:pr34350-viewscreenshot-containerview-fix, commit a5d9432):
ViewExtensions.CaptureAsync#elsenow resolves(view.Handler as IViewHandler)?.ContainerView ?? handler.PlatformView, so container-level effects (clip/shadow/border) are captured — matching the#if PLATFORMpath'sview.ToPlatform()intent.- Added 3 tests: container view is captured when present; null
MauiContextreturns null; a nullIViewScreenshotresult propagates as null.
Why ContainerView ?? PlatformView rather than view.ToPlatform(): I first tried view.ToPlatform() for exact parity, but the unit tests caught that ToPlatform() throws InvalidOperationException when it can't resolve (ElementExtensions.cs:121). That would import throw-on-failure into this deliberately graceful, null-returning path. The ContainerView ?? PlatformView form keeps the container-preference fix while preserving the "return null when unavailable" contract. (It doesn't unwrap IReplaceableView like ToPlatform() does — a minor edge case I judged not worth the throw risk here; happy to revisit.)
All 13 ViewExtensionsCaptureTests pass (10 existing + 3 new) on net11.0.
Still open for your call (from round 1, not changed): the IsCaptureSupported gate semantics and whether window capture warrants a distinct CaptureWindowAsync(object) — see the summary comment above.
CI remains the pre-existing net11.0 HybridWebView AOT-warnings baseline drift (same failures on #35901/#35892/#35891/#35870), unrelated to this PR.
Inline suggestion below if you'd like to apply it directly. 👇
Multimodal review (Opus 4.8, GPT-5.5, Gemini 3.1 Pro) found that the new non-platform #else path in ViewExtensions.CaptureAsync passed handler.PlatformView directly, while the #if PLATFORM path resolves the view via view.ToPlatform() (ContainerView ?? PlatformView). For a third-party backend that uses MAUI's container/WrapperView mechanism, the inner view would be captured and container-level effects (clip, shadow, border) omitted. Prefer (view.Handler as IViewHandler)?.ContainerView ?? handler.PlatformView, mirroring the platform path while staying graceful (no throw) to preserve the "return null when unavailable" contract (view.ToPlatform() throws on failure, which this defensive path must avoid). Adds tests: container view is captured when present, null MauiContext returns null, and a null IViewScreenshot result propagates as null. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fleet code review — PR #34350
Independent assessmentThe new commit fixes the previously flagged Findings by severity❌ ErrorsNone found in the new commit delta.
|
✅ Final multimodal merge-readiness review — unanimous
|
| Model | Verdict |
|---|---|
| Claude Opus 4.8 | MERGEABLE |
| Claude Opus 4.6 | MERGEABLE |
| Gemini 3.1 Pro | MERGEABLE |
| GPT‑5.5 | MERGEABLE |
Fix landed earlier this loop (0a8691d8c3): the non‑platform #else DI fallback now resolves the container view with (view.Handler as IViewHandler)?.ContainerView ?? handler.PlatformView, mirroring ToPlatform() (ContainerView ?? PlatformView) exactly so visual‑layer parity with the #if PLATFORM path is preserved.
What all four models independently confirmed:
- API shape is correct — a separate
IViewScreenshot(rather than extending the shippedIScreenshot) avoids a binary‑breaking change for existing implementers;object platformViewis required because Core'snetTFM can't reference platform view types; nullableTask<IScreenshotResult?>documents the "unsupported view" contract. #elsefallback is safe — nullHandler/PlatformView/MauiContext/service andIsCaptureSupported == falseall returnnullgracefully; the cast toIViewScreenshotpreserves prior null behavior for providers that don't implement it.- Public now is justified, not speculative — the repo itself consumes the interface via the
#elsepath (the real ship path for third‑partynet‑TFM backends), and issueViewExtensions.CaptureAsync(IView)andIPlatformScreenshotneed extensibility for third-party platform backends #34266 documents the macOS AppKit / Linux GTK use case.PublicAPI.Unshipped.txtentries are correct and identical across all 7 TFMs. - Tests are adequate — 13 scenarios across both View and Window cover every null branch, the container‑view preference, and the happy path.
CI note: the red AOT‑macOS / RunOniOS legs are the pre‑existing net11 HybridWebViewHandler baseline‑drift warnings shared by sibling PRs (not produced by this change); Build / Pack / Helix Unit Tests are green.
@Redth — from a code‑correctness standpoint this is ready to merge.
|
/review tests |
This comment has been minimized.
This comment has been minimized.
…ackends (dotnet#35096) <!-- 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! Fixes dotnet#34266 ## Problem `ViewExtensions.CaptureAsync(IView)` and `WindowExtensions.CaptureAsync(IWindow)` are gated by `#if PLATFORM`. On any TFM that isn't one of MAUI's built-in platform targets (Android, iOS/MacCatalyst, Windows, Tizen) they return `null`. That blocks third-party platform backends — for example macOS AppKit or Linux/GTK — from plugging into `VisualDiagnostics.CaptureAsPngAsync(view)` or the public view-level capture API. ## Relationship to dotnet#34350 PR dotnet#34350 solves the same problem by adding a **new public `IViewScreenshot` interface**. Because that is a public API addition, it can only ship in .NET 11. This PR takes a complementary approach with **zero public API additions**, so it can ship in **.NET 10**. The two PRs coexist: when dotnet#34350 lands, its `is IViewScreenshot` fast path runs *before* the keyed-DI fallback introduced here. Backends that register under .NET 10 via this mechanism continue to work unchanged in .NET 11. ## How it works A new internal `ScreenshotDispatch` helper routes the non-`PLATFORM` `#else` branches of the two extension methods through a keyed DI lookup. The contract uses only BCL types: ```csharp Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.ViewCapture" Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.WindowCapture" ``` The lambda receives `handler.PlatformView` and returns an `IScreenshotResult?`. When no hook is registered (or `PlatformView` is `null`, or services aren't keyed) the call resolves to `null` — same as before. ### Backend registration A third-party platform backend registers the hook once during builder configuration. For example, a hypothetical AppKit backend: ```csharp builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.ViewCapture", (_, _) => platformView => ((AppKit.NSView)platformView).CaptureAsync()); builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.WindowCapture", (_, _) => platformView => ((AppKit.NSWindow)platformView).CaptureAsync()); ``` ## Why keyed DI (vs. reflection) An earlier sketch considered convention-based reflection dispatch against `IScreenshot.CaptureAsync(TPlatformView)`. Keyed DI was chosen because it is strictly safer for trimming and AOT: - **No reflection.** No `MethodInfo.Invoke`, no `Expression.Compile`, no `CreateDelegate`. - **No `[DynamicDependency]` burden on the backend.** The typed capture method is reached via normal lambda-closure reachability. - Microsoft.Extensions.DependencyInjection keyed services are already used elsewhere in MAUI Core (e.g. `KeyedWrappedServiceProvider`, `GetKeyedService<IDispatcher>`), so nothing new is taken on. ## Scope - ✅ The `PLATFORM` code path (Android, iOS, MacCatalyst, Windows, Tizen) is **untouched** — zero behavior change on built-in platforms. - ✅ Zero new public API surface. `ScreenshotDispatch` is `internal`; the key strings are documented in XML docs. - ✅ Essentials (`IPlatformScreenshot`, `ScreenshotImplementation`) is **untouched**. ## Tests 11 new unit tests in `Core.UnitTests.Extensions.ScreenshotDispatchTests` (targets `net10.0`, i.e. the exact non-`PLATFORM` path exercised by this change): - hook registered → invoked with `PlatformView`, result propagated - hook not registered → `null` - `PlatformView` null → `null` - handler null / `IView` null → `null` - hook registered under wrong key → `null` - hook returns null task → `null` propagated - view and window hooks coexist without interference All pass locally. ## Files changed - `src/Core/src/ScreenshotDispatch.cs` — new internal helper + key constants - `src/Core/src/ViewExtensions.cs` — `#else` branch delegates to dispatcher; XML docs updated with registration example - `src/Core/src/WindowExtensions.cs` — same treatment as `ViewExtensions` - `src/Core/tests/UnitTests/Extensions/ScreenshotDispatchTests.cs` — 11 tests --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ackends (dotnet#35096) <!-- 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! Fixes dotnet#34266 ## Problem `ViewExtensions.CaptureAsync(IView)` and `WindowExtensions.CaptureAsync(IWindow)` are gated by `#if PLATFORM`. On any TFM that isn't one of MAUI's built-in platform targets (Android, iOS/MacCatalyst, Windows, Tizen) they return `null`. That blocks third-party platform backends — for example macOS AppKit or Linux/GTK — from plugging into `VisualDiagnostics.CaptureAsPngAsync(view)` or the public view-level capture API. ## Relationship to dotnet#34350 PR dotnet#34350 solves the same problem by adding a **new public `IViewScreenshot` interface**. Because that is a public API addition, it can only ship in .NET 11. This PR takes a complementary approach with **zero public API additions**, so it can ship in **.NET 10**. The two PRs coexist: when dotnet#34350 lands, its `is IViewScreenshot` fast path runs *before* the keyed-DI fallback introduced here. Backends that register under .NET 10 via this mechanism continue to work unchanged in .NET 11. ## How it works A new internal `ScreenshotDispatch` helper routes the non-`PLATFORM` `#else` branches of the two extension methods through a keyed DI lookup. The contract uses only BCL types: ```csharp Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.ViewCapture" Func<object, Task<IScreenshotResult?>> // key: "Microsoft.Maui.WindowCapture" ``` The lambda receives `handler.PlatformView` and returns an `IScreenshotResult?`. When no hook is registered (or `PlatformView` is `null`, or services aren't keyed) the call resolves to `null` — same as before. ### Backend registration A third-party platform backend registers the hook once during builder configuration. For example, a hypothetical AppKit backend: ```csharp builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.ViewCapture", (_, _) => platformView => ((AppKit.NSView)platformView).CaptureAsync()); builder.Services.AddKeyedSingleton<Func<object, Task<IScreenshotResult?>>>( "Microsoft.Maui.WindowCapture", (_, _) => platformView => ((AppKit.NSWindow)platformView).CaptureAsync()); ``` ## Why keyed DI (vs. reflection) An earlier sketch considered convention-based reflection dispatch against `IScreenshot.CaptureAsync(TPlatformView)`. Keyed DI was chosen because it is strictly safer for trimming and AOT: - **No reflection.** No `MethodInfo.Invoke`, no `Expression.Compile`, no `CreateDelegate`. - **No `[DynamicDependency]` burden on the backend.** The typed capture method is reached via normal lambda-closure reachability. - Microsoft.Extensions.DependencyInjection keyed services are already used elsewhere in MAUI Core (e.g. `KeyedWrappedServiceProvider`, `GetKeyedService<IDispatcher>`), so nothing new is taken on. ## Scope - ✅ The `PLATFORM` code path (Android, iOS, MacCatalyst, Windows, Tizen) is **untouched** — zero behavior change on built-in platforms. - ✅ Zero new public API surface. `ScreenshotDispatch` is `internal`; the key strings are documented in XML docs. - ✅ Essentials (`IPlatformScreenshot`, `ScreenshotImplementation`) is **untouched**. ## Tests 11 new unit tests in `Core.UnitTests.Extensions.ScreenshotDispatchTests` (targets `net10.0`, i.e. the exact non-`PLATFORM` path exercised by this change): - hook registered → invoked with `PlatformView`, result propagated - hook not registered → `null` - `PlatformView` null → `null` - handler null / `IView` null → `null` - hook registered under wrong key → `null` - hook returns null task → `null` propagated - view and window hooks coexist without interference All pass locally. ## Files changed - `src/Core/src/ScreenshotDispatch.cs` — new internal helper + key constants - `src/Core/src/ViewExtensions.cs` — `#else` branch delegates to dispatcher; XML docs updated with registration example - `src/Core/src/WindowExtensions.cs` — same treatment as `ViewExtensions` - `src/Core/tests/UnitTests/Extensions/ScreenshotDispatchTests.cs` — 11 tests --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR #34350 — Add
|
…reenshot net11.0 (#35096) shipped an internal keyed-DI screenshot hook for the same issue (#34266). Reconcile by keeping this PR's public IViewScreenshot contract as the extensibility surface and reworking ScreenshotDispatch to resolve the typed service instead of a magic-string keyed Func, so both CaptureAsync extension methods share one graceful dispatch. The view path now prefers the container view (clip/shadow/border) to match the #if PLATFORM ToPlatform() path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # src/Core/src/ViewExtensions.cs
Rebased onto net11.0 + reconciled with the already-merged keyed-DI approachHeads up reviewers: while resolving the merge conflict I found that #35096 ("Add keyed-DI screenshot extensibility", merged to Rather than discard either, I reconciled them:
Net effect: one public extensibility mechanism ( |
✅ LGTM — no blocking issues foundRe-review PR #34350 — Add
|
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/review tests |
Tests Failure Analysis
Test Failure Review: Likely unrelated - click to expandOverall verdict: Likely unrelated All 27 unique failures are in areas unrelated to this PR's changes (screenshot extensibility in Core/Essentials). The base branch
Recommended actionNo action needed from the PR author — all identified failures are pre-existing on the Evidence detailsPR scope: 17 changed files, all in Base branch health: All 5 most recent
macOS build log excerpt: iOS integration test log excerpt: Device tests: Helix job IDs found (7e5ca066, d4ad5fb4, e1445588, 38498525) but Helix API returned 404 for all; device test hidden failures could not be verified. AzDO was accessed anonymously (no AZDO_TOKEN); authenticated test-run APIs were skipped. AzDO build references: maui-pr 1466584 | maui-pr-devicetests 1466586 | maui-pr-uitests 1466585 |
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
ViewExtensions.CaptureAsync(IView)andWindowExtensions.CaptureAsync(IWindow)are gated by#if PLATFORM, making element-level screenshots impossible for third-party platform backends (e.g. macOS AppKit, Linux/GTK). This PR adds a runtime-extensible path via a newIViewScreenshotinterface.Changes
New
IViewScreenshotinterface (Screenshot.shared.cs)Task<IScreenshotResult?> CaptureViewAsync(object platformView)— allows any backend to implement view-level capture without compile-time platform knowledgePlatform implementations (iOS, Android, Windows, Tizen)
ScreenshotImplementationnow also implementsIViewScreenshot, delegating to its existing typedCaptureAsyncmethodDI-based fallback in non-platform paths
ViewExtensions.CaptureAsync#elsepath now resolvesIScreenshotfrom the handler'sMauiContext.Services, checks forIViewScreenshot, and callsCaptureViewAsyncWindowExtensions.CaptureAsyncgets the same treatmentHow third-party backends use this
A custom platform backend registers its
IScreenshotimplementation (which also implementsIViewScreenshot) in DI:Then
view.CaptureAsync()andVisualDiagnostics.CaptureAsPngAsync(view)just work.Impact on existing platforms
#if PLATFORMpath is unchangedScreenshot.Defaultwould previously returnnullFixes #34266