refactor(android): Make Skia-Android multi-window-ready - #24042
refactor(android): Make Skia-Android multi-window-ready#24042MartinZikmund wants to merge 9 commits into
Conversation
github-actions
Bot
added
platform/android 🤖
|
Claude finished @MartinZikmund's task in 9m 24s —— View job Review complete
Overall verdict:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // Update the native layer host clip path | ||
| ApplicationActivity.NativeLayerHost!.Path = nativeClipPath; | ||
| _activity.NativeLayerHost!.Path = nativeClipPath; |
There was a problem hiding this comment.
The canvas view (UnoSKCanvasView) was just fixed in this same PR to null-guard NativeLayerHost (replacing ApplicationActivity.NativeLayerHost!.Path with if (_activity.NativeLayerHost is { } nativeLayerHost)), but this Vulkan view still retains the null-forgiving !. The Vulkan render thread starts when SurfaceCreated fires, which in theory can happen before _activity.NativeLayerHost is initialized in OnStart. Apply the same guard to be consistent and safe:
if (_activity.NativeLayerHost is { } nativeLayerHost)
{
nativeLayerHost.Path = nativeClipPath;
}
| @@ -328,7 +328,23 @@ protected override void OnDestroy() | |||
|
|
|||
| partial void InnerDestroy(); | |||
|
|
|||
There was a problem hiding this comment.
The PR updates ContextHelper.Current semantics to "foreground-activity scoped", but the BaseActivity constructors (lines 132 and 138) still call ContextHelper.Current = this before the activity enters its lifecycle. An activity under construction is not yet foreground — it only becomes foreground in OnResume.
On a configuration-change recreation, Android constructs the new activity before calling OnPause on the old one. The constructor's early assignment means ContextHelper.Current re-points to the uninitialized new activity while the old activity is still the visible foreground. Code running in the old activity's OnPause teardown path (e.g. any callers of ContextHelper.Current) will silently get the wrong activity.
The assignment in both constructors is also redundant: SetAsCurrent() (called from InnerCreate, InnerStart, InnerResume) already drives ContextHelper.Current = this from the correct lifecycle points. Consider removing the constructor assignments and letting SetAsCurrent() be the sole writer.
| } | ||
|
|
||
| typeof(ContextHelper) | ||
| .Log() |
There was a problem hiding this comment.
The PR makes _current explicitly Android.Content.Context? and the docs say the value "may be null before any activity is created", but the public Current property still returns Android.Content.Context (non-nullable) via the ! null-forgiveness operator. Any external caller who trusts the type signature (or uses #nullable enable) will get a silent null-dereference rather than an explicit failure.
Now that TryGetCurrent is the safe internal path, consider either:
- Making the public return type
Android.Content.Context?(breaking change, but honest about the contract), or - Throwing
InvalidOperationExceptionexplicitly here instead of returning_current!, so callers get a clear message rather than a NullReferenceException at the call site.
At a minimum, the XML doc <returns> on the property should be updated to reflect that the getter can return null (or that callers should use TryGetCurrent/ApplicationContext instead).
| { | ||
| var windowMetrics = (ContextHelper.Current as Activity)?.WindowManager?.CurrentWindowMetrics; | ||
| var windowMetrics = activity.WindowManager?.CurrentWindowMetrics; | ||
| displaySize = new Size(windowMetrics.Bounds.Width(), windowMetrics.Bounds.Height()); |
There was a problem hiding this comment.
The nullable access chain activity.WindowManager?.CurrentWindowMetrics can yield null (e.g. when WindowManager is null), yet windowMetrics.Bounds is called on the next line without any null check — a latent NullReferenceException. The PR touched line 232 (switching from (ContextHelper.Current as Activity)? to activity) and is a good opportunity to complete the null guard:
var windowMetrics = activity.WindowManager?.CurrentWindowMetrics;
if (windowMetrics is null) return default;
displaySize = new Size(windowMetrics.Bounds.Width(), windowMetrics.Bounds.Height());
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-24042/docs/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24042/wasm-skia-net9/index.html |
|
The build 226956 found UI Test snapshots differences: Details
|
github-actions
Bot
added
the
area/build
|
Claude finished @MartinZikmund's task in 8m 3s —— View job Re-review: 4 new commits since first pass
What the 4 new commits addressed
The The new Remaining issues (2 inline comments posted)HIGH — Vulkan LOW — Note on
|
|
|
||
| // Update the native layer host clip path | ||
| ApplicationActivity.NativeLayerHost!.Path = nativeClipPath; | ||
| _activity.NativeLayerHost!.Path = nativeClipPath; |
There was a problem hiding this comment.
The null-forgiving ! here was flagged in the first review pass and is still not fixed. The canvas view (UnoSKCanvasView) received the defensive guard in commit a2e944b2:
if (_activity.NativeLayerHost is { } nativeLayerHost)
{
nativeLayerHost.Path = nativeClipPath;
}Please apply the same pattern here. Although a lifecycle analysis shows _nativeLayerHost is non-null while the Vulkan render thread is running (it is initialized in OnStart before the surface is added to the window, and TeardownRenderer joins the thread before OnDestroy nulls it), the ! is a silent crash if that ordering ever shifts and is inconsistent with the canvas view. Both backends should use the same null-safe pattern.
| _activity.NativeLayerHost!.Path = nativeClipPath; | |
| if (_activity.NativeLayerHost is { } nativeLayerHost) | |
| { | |
| nativeLayerHost.Path = nativeClipPath; | |
| } |
| { | ||
| var windowMetrics = (ContextHelper.Current as Activity)?.WindowManager?.CurrentWindowMetrics; | ||
| var windowMetrics = activity.WindowManager?.CurrentWindowMetrics; | ||
| displaySize = new Size(windowMetrics.Bounds.Width(), windowMetrics.Bounds.Height()); |
There was a problem hiding this comment.
Still unfixed from the first review pass. activity.WindowManager?.CurrentWindowMetrics can return null when WindowManager is null, but windowMetrics.Bounds is called unconditionally on the next line:
| displaySize = new Size(windowMetrics.Bounds.Width(), windowMetrics.Bounds.Height()); | |
| var windowMetrics = activity.WindowManager?.CurrentWindowMetrics; | |
| if (windowMetrics is null) return default; | |
| displaySize = new Size(windowMetrics.Bounds.Width(), windowMetrics.Bounds.Height()); |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24042/wasm-skia-net9/index.html |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-24042/docs/index.html |
|
The build 227003 found UI Test snapshots differences: Details
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SBqEtwCdGHuvjapkaNdAP
Add ContextHelper.ApplicationContext for app-scoped callers, make Current fall back to the application context when no activity is in the foreground, and repoint the foreground context on activity destroy so a torn-down activity is never left as "current". Groundwork for multi-window (#13827). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SBqEtwCdGHuvjapkaNdAP
Converge Skia-on-Android onto the per-window ownership pattern the desktop and iOS runtimes already use, so the architecture is multi-window-ready (#13827). SupportsMultipleWindows stays false. - NativeWindowWrapper is no longer a process-wide Lazy singleton; it is bound to its window and tracks the activity currently driving it (CurrentActivity), which the managed Window outlives across activity re-creation. - ApplicationActivity's render stack (render view, native-layer host, root layout) and the Instance singleton become per-activity instance state; a new activity rebuilds its surface and re-attaches on re-creation. - AndroidSkiaXamlRootHost is per-window: it resolves its own RootElement and the driving activity via the wrapper, and registers in XamlRootMap so consumers (native element hosting, TextBox notifications, IME) resolve the owning activity from a XamlRoot instead of a global. - The window factory creates the host per window and binds the activity's wrapper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SBqEtwCdGHuvjapkaNdAP
Pointer and keyboard input sources are no longer process singletons. Each window's wrapper owns its own sources, resolved by that window's InputManager via its IXamlRootHost (matching the Win32 runtime), and fed by the driving activity's native event dispatch. Completes the multi-window-ready de-singleton (#13827). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SBqEtwCdGHuvjapkaNdAP
- ContextHelper.Current no longer falls back to the application context: it stays activity-scoped (foreground activity or null) so existing (Activity)Current hard-casts and Current == null guards keep their semantics; the app-context split is the separate ApplicationContext accessor. - Only repoint the foreground context on destroy when another live activity can take over, preserving last-activity behavior for single-window. - Raise the managed window Closing only when the activity is finishing, so a configuration-change re-creation doesn't spuriously close the surviving window. - Make OnSystemUiVisibilityChangeListener internal (no external consumer). - Tag the remaining foreground-activity fallbacks with the #13827 follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SBqEtwCdGHuvjapkaNdAP
Align UnoSKCanvasView with the Vulkan backend: skip the frame when the window's root/composition target isn't ready (e.g. mid teardown during activity re-creation) instead of dereferencing with a null-forgiving bang. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SBqEtwCdGHuvjapkaNdAP
Review panel findings on the per-activity render stack. - Skipping a frame when the window isn't ready stranded rendering: only OnNativePlatformFrameRequested clears CompositionTarget.RenderRequested, so a bare return made every later RequestNewFrame a no-op and the window never repainted. Both backends now re-arm on the skip path. - Build the render stack before base.OnStart(): that call synchronously reaches OnLaunched -> CreateWindow, after which InvalidateRender() can run against RelativeLayout. Harmless while the stack was static (null only on first launch), but it is per-activity now, so it was null on every re-creation. - Release the GL/Vulkan context in OnDestroy via IUnoSkiaRenderView.TeardownRenderer; the peer finalizer never runs the managed dispose path, so each re-creation stranded a GRContext (a VkDevice on Vulkan). - Unsubscribe the InputPane and orientation-sensor handlers, which are on process-wide singletons and were rooting every destroyed activity. - Gate the managed window Closing on IsChangingConfigurations and wrapper ownership: IsFinishing is not the complement of "being re-created", so the StartActivity/Finish restart idiom raised Closing on the successor's window. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015yi8oUA7TT2ArHyBqzwJz6
Current is activity-scoped and can be null, but the file's new #nullable enable left it annotated non-null while returning _current!, which told every consumer null was unreachable. Typing it Context? restores the signal; runtime behaviour is unchanged, since the null-forgiving getter already handed back the same null. Making it honest surfaced 13 call sites that dereference it unguarded. They are left explicit with `!` rather than repointed at ApplicationContext: several read Resources, WindowManager or assembly identity, where the activity and application contexts are not interchangeable. Threading an owning-window context through these callers is the documented follow-up. The native WebView provider does resolve its owning activity from the ContentPresenter's XamlRoot, since one is available there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015yi8oUA7TT2ArHyBqzwJz6
- Record the two public-surface changes that were not additive: the OnSystemUiVisibilityChangeListener narrowing (PackageDiffIgnore + migration notes) and the ContextHelper.Current nullability and foreground-tracking changes. - Correct the plan: Current has no application-context fallback, and the wrapper lands per-activity rather than strictly per-Window, since the activity still adopts it through the ambient current window. - Add Given_AndroidSkiaXamlRootHost. The plan claimed this logic was only exercisable on-device, but CI runs an Android Skia runtime-test lane, so host registration, activity resolution and per-window input-source identity are assertable there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015yi8oUA7TT2ArHyBqzwJz6
9dc62c3 to
d12374d
Compare
|
Claude finished @MartinZikmund's task in 3m 8s —— View job Re-review pass 3 (commits
|
| Commit | Topic | Status |
|---|---|---|
2542a0c9 |
Re-arm skipped frames, TeardownRenderer, unsubscribe singletons, IsChangingConfigurations gate |
✅ Sound — no new issues |
080f5262 |
Type ContextHelper.Current as Android.Content.Context?; 13 call sites updated with explicit ! |
✅ Fixed (MEDIUM from prior passes) |
d12374d1 |
Docs: migration guide additions, PackageDiffIgnore.xml, Given_AndroidSkiaXamlRootHost runtime test, spec corrections |
✅ Sound |
The new Given_AndroidSkiaXamlRootHost runtime test is well-formed: [PlatformCondition(ConditionMode.Include, RuntimeTestPlatforms.SkiaAndroid)] correctly gates it to the Android Skia CI lane; the three synchronous void methods don't await anything so the void return type is correct; reflection-based access avoids a compile-time dependency on the Android assembly.
Verdict: fix-first — 2 inline findings still open
The same two issues from the first two review passes remain unaddressed:
HIGH — UnoSKVulkanView.cs:225 — _activity.NativeLayerHost!.Path = nativeClipPath still uses the null-forgiving !. The canvas view received this fix in commit a2e944b2; the Vulkan view did not. See inline comment.
LOW — NativeWindowWrapper.Android.cs:233 — windowMetrics.Bounds is called unconditionally when windowMetrics can be null. See inline comment.
Neither the re-arm commit (2542a0c9) nor either of the two subsequent commits (080f5262, d12374d1) touched these lines. Both carry a one-line suggestion in their inline comments.
|
|
||
| // Update the native layer host clip path | ||
| ApplicationActivity.NativeLayerHost!.Path = nativeClipPath; | ||
| _activity.NativeLayerHost!.Path = nativeClipPath; |
There was a problem hiding this comment.
HIGH — still unfixed after three review passes. This null-forgiving ! was called out in the first pass, re-flagged in the second pass, and the three new commits (2542a0c9, 080f5262, d12374d1) did not address it.
The canvas view received the fix in commit a2e944b2 (line 237–240 of UnoSKCanvasView.cs):
if (_activity.NativeLayerHost is { } nativeLayerHost)
{
nativeLayerHost.Path = nativeClipPath;
}Please apply the identical pattern here so both backends are consistent.
| _activity.NativeLayerHost!.Path = nativeClipPath; | |
| if (_activity.NativeLayerHost is { } nativeLayerHost) | |
| { | |
| nativeLayerHost.Path = nativeClipPath; | |
| } |
| { | ||
| var windowMetrics = (ContextHelper.Current as Activity)?.WindowManager?.CurrentWindowMetrics; | ||
| var windowMetrics = activity.WindowManager?.CurrentWindowMetrics; | ||
| displaySize = new Size(windowMetrics.Bounds.Width(), windowMetrics.Bounds.Height()); |
There was a problem hiding this comment.
LOW — still unfixed after three review passes. activity.WindowManager?.CurrentWindowMetrics can return null when WindowManager is null (e.g. before the activity window is attached), but windowMetrics.Bounds is called unconditionally on the next line.
| displaySize = new Size(windowMetrics.Bounds.Width(), windowMetrics.Bounds.Height()); | |
| var windowMetrics = activity.WindowManager?.CurrentWindowMetrics; | |
| if (windowMetrics is null) return default; | |
| displaySize = new Size(windowMetrics.Bounds.Width(), windowMetrics.Bounds.Height()); |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24042/wasm-skia-net9/index.html |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-24042/docs/index.html |
|
The build 227800 found UI Test snapshots differences: Details
|
3 participants
GitHub Issue: #8341
PR Type:
🔄 Refactoring (no functional changes, no api changes)
What changed? 🚀
Skia-on-Android baked "one window" into three process-wide static anchors. This PR
de-singletons them so Android converges onto the same per-window ownership pattern the
Skia desktop runtimes (Win32/X11/macOS) and the Apple UIKit runtime already use — making
the architecture multi-window-ready.
SupportsMultipleWindowsdeliberately staysfalse. The definition of done here isper-window instances everywhere, and zero single-window regressions — not a live second window.
The three anchors that went away
NativeWindowWrapper.Instance— was a process-wideLazy<>singleton returned byAndroidSkiaWindowFactory.CreateWindowfor every window. It is now an instance bound toits window, tracking the activity currently driving it (
CurrentActivity), since the managedWindowoutlives individual activities across re-creation.ApplicationActivity.Instance+ thestaticrender stack (render view, native-layer host,root layout) — now per-activity instance state. A new activity rebuilds its surface and
re-attaches on re-creation.
ContextHelper.Current— split into an explicit app-globalApplicationContextand anactivity-scoped
Currentthat tracks the foreground activity via the existingBaseActivityregistry, instead of today's sticky "last-ever-set" behaviour.
Supporting changes
AndroidSkiaXamlRootHostis per-window: it resolves its ownRootElementand drivingactivity, and registers in
XamlRootMapso consumers (native element hosting, TextBoxnotifications, IME) resolve the owning activity from a
XamlRootrather than a global.registered via
ApiExtensibility.Register<IXamlRootHost>(…), matching the Win32 runtime. Eachwindow's
InputManagerresolves its own sources through its host.Closingis raised only when the activity isactually finishing, so a configuration-change re-creation no longer spuriously closes the
surviving window.
UnoSKCanvasViewnow skips a frame when the window'sroot/composition target isn't ready (e.g. mid-teardown during activity re-creation), matching
the Vulkan backend, instead of dereferencing with
!.API surface
Not additive — this PR contains two deliberate public-surface breaks, both recorded in
doc/articles/migrating-to-uno-7.mdandbuild/PackageDiffIgnore.xml:Uno.UI.ContextHelper.Currentis re-typedAndroid.Content.Context?. It could always benullbefore any activity exists, but was annotated non-null and returned_current!, so thecompiler never surfaced it. Typing it honestly changed no runtime behaviour and immediately
exposed 13 unguarded dereferences in
Uno.UWP. Its semantics also change: it now tracks theforeground activity rather than the last one ever assigned.
Uno.UI.OnSystemUiVisibilityChangeListeneris narrowed tointernal. It is constructed bythe host with the activity owning the window; app code had no way to supply one.
ContextHelper.ApplicationContextis new and additive. Everything else in this PR isinternal.Design notes
specs/053-android-multiwindow-ready/plan.mddocuments the architecture, the phase breakdown,and the deliberate follow-ups — chiefly flipping
SupportsMultipleWindowstotrue(needsActivity↔Window lifecycle orchestration and on-device validation, mirroring how iOS staged its
own multi-window behind scene adoption) and threading an explicit owning-window
Contextthroughthe remaining ambient
ContextHelper.Currentconsumers.Validation
Uno.UI.Runtime.Skia.Androidbuilds clean fornet10.0-android(0 errors; the 2
XA0101warnings are pre-existing and unrelated). This is the only assemblythat compiles the
__ANDROID__Skia code.this change. Reviewers/CI should smoke-test single-window behaviour:
cd src/SamplesApp/SamplesApp.Skia.netcoremobile/Android && dotnet run -f net10.0-android,exercising launch/render, touch + text input (soft keyboard, IME composition), rotation,
background→foreground, and a config-change re-creation (system font-size/locale change).
Kept as a draft until that on-device pass is done.
PR Checklist ✅
Given_AndroidSkiaXamlRootHostruns on the Android Skia CI lane and asserts host registration, activity resolution and per-window input-source identity. The foreground-repoint logic itself lives onBaseActivity : AppCompatActivity, which the net-basedUno.UI.UnitTestscannot instantiate.migrating-to-uno-7.md; the in-repo spec covers the architecture.Screenshots Compare Test Runresults — pending CI.Known follow-ups
Carried from the review panel, deliberately not in this PR:
Window— the activity still adopts it throughthe ambient current window. An explicit activity⇄window binding needs the lifecycle
orchestration that lands with the live second activity. Marked
TODO #13827.ContextHelper.ApplicationContextstill has no consumers; migrating the app-scopedUno.UWPcallers offCurrentis the documented follow-up (several readResources,WindowManageror assembly identity, where the two contexts are not interchangeable).NativeWindowWrapperand the new lifecyclebranches emit no trace;
OnDrawFramelacks the try/catch its Vulkan sibling has.GetWindowSizedereferencesWindowManager?.CurrentWindowMetricsunconditionally._contentViewAttachedToWindowsurvives re-creation, so a new activity's first pre-drawshort-circuits; the fix must move the
ContentViewAttachedToWindowsubscription out of theWasShown-gatedShowCorerather than just resetting the flag.BaseActivityprunes_instancesinDisposerather thanOnDestroy, so the foregroundrepoint can select an already-destroyed activity.
TextInputPlugin.NotifyValueChangedlogs full text at Debug, andPasswordBox : TextBox, so passwords can reach logcat.