Skip to content

Commit 6b5593a

Browse files
RedthCopilot
andcommitted
Fix seven correctness findings from architecture and code review
1. Position contract. MAUI documents GetPosition(relativeTo) as "the element to use as the coordinate reference, or null for SCREEN coordinates". The dispatcher was answering null with the view-local position, which is silently wrong. Gesture events now carry both spaces (TizenGesturePosition) and resolve null -> screen, same view -> local, other element -> null. The NUI detectors populate screen coordinates from TapGesture.ScreenPoint, LongPressGesture.ScreenPoint, PanGesture.ScreenPosition, PinchGesture.ScreenCenterPoint and Touch/Hover.GetScreenPosition. A missing screen coordinate stays null rather than being faked from the local one. 2. Real pixel scaler. AddTizenNuiControlsPlatform now registers a scaler backed by DeviceInfo.ScalingFactor. Identity scaling is only correct on a 1x display; Tizen wearables and TVs are not, so every pan, swipe, pinch, tap and pointer coordinate was wrong by the display factor. The registration is exposed as AddTizenPixelScaler taking a Func<double>, which is what makes it executable on the host: only reading the factor needs a device, and that atom is parameterized rather than baked in. The factor is read lazily because DeviceInfo is unusable until the app has initialised, and a non-positive or non-finite value degrades to 1 rather than throwing during window creation. 3. Unsubscribe is detach-only. MAUI calls Unsubscribe on ordinary page churn, not only at teardown, so dismissing dialogs there cancelled a DisplayAlertAsync the app was legitimately awaiting across a page swap. Dialogs are now dismissed only in Dispose, which the container calls at window-scope teardown. 4. Late-bound window. The subscription captured PlatformWindow at construction. MAUI can create the page handler - and therefore call Subscribe - before the window handler attaches the native window, so that snapshot could be null forever and silently drop every alert for the window's lifetime. The window is now resolved per request, and an unattached window services the request rather than discarding it. 5. Button masks. Tap and pointer dispatch now carry the originating button and filter against recognizer.Buttons, so a recognizer configured for Primary never fires on a right-click. Buttons come from Touch.GetMouseButton; Tizen.NUI.Hover exposes no equivalent, so hovers report none. Touch input reports MouseButton.Invalid, which maps to Primary - as does anything unclassified, so a stray value can never fabricate a secondary click. 6. Awaited stack operations. TizenModalHost discarded the PushAsync/PopAsync tasks, swallowing faults and letting a dialog open over a stack that had not taken the placeholder. Both are awaited, and ShownBehindPage is unwound even when the push faults. 7. Cross-window page reuse. A page popped from one window and pushed modally on another kept a handler bound to the originating window's IMauiContext, realizing it into the wrong view tree. Such handlers are now disconnected and rebuilt from the target window's factory, and the target context is applied unconditionally. Testing ------- 193 tests, up from 158. Each fix was verified load-bearing by reverting it and confirming the new tests fail: 9 failures across all seven areas, no overlap. Two API constraints found by the ref-pack lane rather than at runtime: TapGestureRecognizer.SendTapped takes no button argument, so the mask is enforced by filtering; and Tizen.NUI.Hover has no GetMouseButton. Long press remains the one gesture that cannot be dispatched, and the provisional dotnet/maui#37853 modal contracts and their expiry guard are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 8c18234 commit 6b5593a

22 files changed

Lines changed: 1076 additions & 105 deletions

docs/tizen-gesture-support-matrix.md

Lines changed: 54 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -83,15 +83,59 @@ complete `TizenGestureDispatcher` — no handler, detector or lifecycle code has
8383

8484
### Position resolution
8585

86-
The new tap and pointer members take a `Func<IElement?, Point?> getPosition` rather than a
87-
plain point, so MAUI can ask for the position relative to an arbitrary element.
86+
The new tap and pointer members take a `Func<IElement?, Point?> getPosition` rather than a plain
87+
point, because MAUI documents the parameter as *"the element to use as the coordinate reference,
88+
or `null` for **screen** coordinates"*. Three distinct cases:
8889

89-
The Tizen detectors report a position local to the view the gesture occurred on. That value is
90-
returned for the view itself and for the `null` (view-relative) request. For any *other* element
91-
the resolver returns `null`, which is how MAUI models "cannot be determined": translating between
92-
two elements needs both on-screen origins, which requires a native call per element that the
93-
Tizen platform layer does not expose to this assembly. Returning a plausible-looking but wrong
94-
coordinate would be worse, so it is not done.
90+
| `relativeTo` | Returned |
91+
|---|---|
92+
| `null` | The **screen** position |
93+
| The view the gesture occurred on | The view-local position |
94+
| Any other element | `null` — cannot be determined |
95+
96+
Answering the `null` case with a view-local coordinate is silently wrong, which is why the native
97+
detectors report both spaces: `TapGesture.ScreenPoint`, `LongPressGesture.ScreenPoint`,
98+
`PanGesture.ScreenPosition`, `PinchGesture.ScreenCenterPoint`, `Touch.GetScreenPosition` and
99+
`Hover.GetScreenPosition`.
100+
101+
When a native event carries no screen position, the screen case returns `null` rather than
102+
substituting the local one — an honest "unknown" instead of a wrong number.
103+
104+
The third row is `null` because translating into another element's space needs that element's
105+
on-screen origin, which requires a native call per element that the Tizen platform layer does not
106+
expose to this assembly.
107+
108+
### Button masks
109+
110+
`TapGestureRecognizer.Buttons` and `PointerGestureRecognizer.Buttons` let an app ask for a
111+
specific button. The dispatcher filters on them, so a recognizer configured for `Primary` never
112+
fires on a right-click and vice versa.
113+
114+
Buttons come from `Touch.GetMouseButton`. `Tizen.NUI.Hover` exposes no equivalent — a hover is
115+
pointer movement with nothing pressed — so hover transitions report no button.
116+
117+
| Native | Reported |
118+
|---|---|
119+
| `MouseButton.Primary` | `ButtonsMask.Primary` |
120+
| `MouseButton.Secondary` | `ButtonsMask.Secondary` |
121+
| `MouseButton.Tertiary` | `ButtonsMask.Primary` |
122+
| `MouseButton.Invalid` (touch) | `ButtonsMask.Primary` |
123+
124+
Touch input has no button, and NUI reports `Invalid` for it. It maps to `Primary`, matching how
125+
MAUI's own touch backends report a finger press. Anything unclassified maps to `Primary` too, so a
126+
stray value can never fabricate a right-click — the failure direction that would actually surprise
127+
a user.
128+
129+
### Pixel scaling
130+
131+
Native coordinates are device pixels; MAUI gesture events are device-independent units. The
132+
conversion factor comes from `DeviceInfo.ScalingFactor`, registered by
133+
`AddTizenNuiControlsPlatform` via `AddTizenPixelScaler`.
134+
135+
This is not cosmetic: Tizen wearables and TVs do not run at 1x, so an identity scaler makes every
136+
pan, swipe, pinch, tap and pointer coordinate wrong by the display factor. The neutral
137+
`AddTizenGestures` still registers an identity fallback with `TryAdd` so host-side tests work
138+
unconfigured, but the platform layer registers the real scaler first and therefore always wins.
95139

96140
### What upstream still needs to change
97141

@@ -228,7 +272,8 @@ factory can therefore refine this table without changing any other code.
228272
|---|---|
229273
| Gesture translation (totals, scaling, gesture identity, tap counts, pointer mapping) | `tests/Controls.UnitTests/TizenGestureTranslationTests.cs` |
230274
| Manager and detector lifecycle (attach, detach, enable, dispose, collection changes) | `tests/Controls.UnitTests/TizenGesturePlatformManagerTests.cs` |
231-
| Dispatch through real MAUI recognizers, position resolution, and the one blocked gesture | `tests/Controls.UnitTests/TizenGestureDispatcherTests.cs` |
275+
| Dispatch through real MAUI recognizers, screen/local/unknown position resolution, button masks, and the one blocked gesture | `tests/Controls.UnitTests/TizenGestureDispatcherTests.cs` |
276+
| Pixel scaler registration and lazy display-factor lookup | `tests/Controls.UnitTests/TizenServiceRegistrationTests.cs` |
232277
| DI registration and lifetimes | `tests/Controls.UnitTests/TizenServiceRegistrationTests.cs` |
233278
| NUI adapters under `Core/Platform/Nui` | Type-checked against `Samsung.Tizen.Ref.API15` and `Tizen.UIExtensions.NUI` 0.9.2 by `tests/Maui.Tizen.Controls.RefPackCompile`; behaviour needs a device |
234279

docs/tizen-modal-navigation.md

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,25 @@ Two behaviours worth calling out:
110110
- **Back button.** The handler resolves `host.CurrentPage` on every press rather than capturing it,
111111
because the current page changes as modals come and go.
112112

113+
### Cross-window page reuse
114+
115+
A page can be popped from one window and pushed modally on another. Its existing handler is bound
116+
to the *originating* window's `IMauiContext`, and reusing it would realize the page into the wrong
117+
window's view tree.
118+
119+
`TizenModalPageRealizer` therefore disconnects and discards any handler whose `MauiContext` is not
120+
the target one, and builds a fresh handler from the target window's handler factory. When the
121+
handler already belongs to the target window it is reused, but the context is re-applied
122+
unconditionally so a handler created without one — or whose context was cleared on disconnect — is
123+
always realized against the right window.
124+
125+
### Awaiting the navigation stack
126+
127+
`ITizenNavigationStack.PushAsync` and `PopAsync` are awaited, never fire-and-forget. Discarding
128+
those tasks swallows the fault and lets a dialog open over a stack that has not actually taken the
129+
placeholder, which then unbalances the pop. `TizenModalHostTests` covers both the ordering and the
130+
push/pop failure paths.
131+
113132
### Realizing a page without `ToPlatform`
114133

115134
`ModalNavigationManager.Tizen.cs` called `modal.ToPlatform(WindowMauiContext)`.
@@ -175,7 +194,8 @@ create a second, competing source of truth for back-button routing, so
175194
|---|---|
176195
| Modal page push/pop, animation flags, batch pop, back button, disposal | `tests/Controls.UnitTests/TizenModalNavigationPlatformTests.cs` |
177196
| Factory behaviour and per-window isolation | `TizenModalNavigationPlatformFactoryTests` |
178-
| Dialog placeholder balance, including the fault and buried-placeholder paths | `tests/Controls.UnitTests/TizenModalHostTests.cs` |
197+
| Dialog placeholder balance, push/pop failure propagation, async completion ordering, buried placeholder | `tests/Controls.UnitTests/TizenModalHostTests.cs` |
198+
| Cross-window page reuse and target-context application | `TizenModalNavigationPlatformTests` |
179199
| Window-scoped holder semantics | `TizenScopedWindowServiceTests` |
180200
| Provisional contract shape, namespace and expiry | `ProvisionalModalNavigationContractTests` |
181201
| DI registration and lifetimes | `tests/Controls.UnitTests/TizenServiceRegistrationTests.cs` |

src/Maui.Tizen.Controls/Core/Platform/Alerts/TizenAlertManager.cs

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -81,25 +81,34 @@ public void Subscribe()
8181
return;
8282
}
8383

84+
// The window is resolved per request rather than snapshotted here: MAUI can create the
85+
// page handler - and therefore call Subscribe - before the window handler has attached
86+
// the native window, and a null snapshot taken in that order would silently drop every
87+
// alert for the window's lifetime.
8488
_subscription = new TizenAlertManagerSubscription(
85-
_windowContext.PlatformWindow,
89+
() => _windowContext.PlatformWindow,
8690
_dialogs,
8791
_modalHost,
8892
_windowProvider);
8993
}
9094

9195
/// <inheritdoc/>
9296
/// <remarks>
93-
/// Unsubscribing dismisses any dialog that is still on screen. Unsubscribing while not
94-
/// subscribed is a safe no-op, and a later <see cref="Subscribe"/> creates a fresh
95-
/// subscription bound to the window's current native window.
97+
/// <para>
98+
/// Unsubscribing is <b>detach only</b>: the subscription is dropped so no further requests
99+
/// are serviced, but dialogs already on screen are left alone and their awaiting callers
100+
/// keep waiting.
101+
/// </para>
102+
/// <para>
103+
/// This matters because .NET MAUI calls <c>Unsubscribe</c> on ordinary page churn - the
104+
/// window's page handler changing, or the page being replaced - not only at teardown.
105+
/// Dismissing dialogs here would cancel a <c>DisplayAlertAsync</c> that the application is
106+
/// legitimately awaiting across a page swap. Dialogs are dismissed only in
107+
/// <see cref="Dispose"/>, which the DI container calls when the window scope is torn down.
108+
/// </para>
96109
/// </remarks>
97-
public void Unsubscribe()
98-
{
99-
var subscription = _subscription;
110+
public void Unsubscribe() =>
100111
_subscription = null;
101-
subscription?.Dispose();
102-
}
103112

104113
/// <inheritdoc/>
105114
public void RequestAlert(Page page, AlertArguments arguments) =>
@@ -126,9 +135,15 @@ public void RequestPageBusy(Page page, bool isBusy) =>
126135
#pragma warning restore CS0618
127136

128137
/// <summary>
129-
/// Unsubscribes and releases the manager. Called by the DI container when the window scope
130-
/// is disposed.
138+
/// Releases the manager and dismisses any dialog still on screen. Called by the DI
139+
/// container when the window scope is disposed.
131140
/// </summary>
141+
/// <remarks>
142+
/// This is the only place dialogs are dismissed. Native NUI popups stay on screen until
143+
/// explicitly closed, so a window teardown that merely dropped the reference would leave an
144+
/// orphaned modal overlay and a caller pending forever. Disposing cancels the pending
145+
/// dialogs, completing those callers with the documented cancellation result.
146+
/// </remarks>
132147
public void Dispose()
133148
{
134149
if (_disposed)
@@ -137,7 +152,10 @@ public void Dispose()
137152
}
138153

139154
_disposed = true;
140-
Unsubscribe();
155+
156+
var subscription = _subscription;
157+
_subscription = null;
158+
subscription?.Dispose();
141159
}
142160
}
143161
}

src/Maui.Tizen.Controls/Core/Platform/Alerts/TizenAlertManagerSubscription.cs

Lines changed: 47 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public sealed class TizenAlertManagerSubscription : IAlertManagerSubscription, I
2828
readonly ITizenAlertDialogFactory _dialogs;
2929
readonly ITizenModalHost _modalHost;
3030
readonly ITizenPlatformWindowProvider _windowProvider;
31-
readonly object? _window;
31+
readonly Func<object?> _resolveWindow;
3232
readonly List<ITizenAlertDialog> _openDialogs = new();
3333
readonly object _sync = new();
3434

@@ -37,30 +37,53 @@ public sealed class TizenAlertManagerSubscription : IAlertManagerSubscription, I
3737
bool _disposed;
3838

3939
/// <summary>
40-
/// Initializes a new subscription bound to <paramref name="platformWindow"/>.
40+
/// Initializes a new subscription whose window is resolved lazily.
4141
/// </summary>
42-
/// <param name="platformWindow">
43-
/// The native window this subscription serves. Requests from pages in other windows are ignored.
42+
/// <param name="resolveWindow">
43+
/// Returns the native window this subscription serves, or <see langword="null"/> when the
44+
/// window has not been attached yet. Evaluated on every request rather than captured.
4445
/// </param>
4546
/// <param name="dialogs">Creates the dialogs used to service requests.</param>
4647
/// <param name="modalHost">Coordinates dialogs with the Tizen modal navigation stack.</param>
4748
/// <param name="windowProvider">Resolves the native window that a requesting page belongs to.</param>
49+
/// <remarks>
50+
/// The window is resolved per request, not captured at construction. .NET MAUI can create
51+
/// the page handler before the window handler has attached the native window, and a
52+
/// snapshot taken in that order would be <see langword="null"/> forever, silently dropping
53+
/// every alert for the window's lifetime.
54+
/// </remarks>
4855
public TizenAlertManagerSubscription(
49-
object? platformWindow,
56+
Func<object?> resolveWindow,
5057
ITizenAlertDialogFactory dialogs,
5158
ITizenModalHost modalHost,
5259
ITizenPlatformWindowProvider windowProvider)
5360
{
54-
_window = platformWindow;
61+
_resolveWindow = resolveWindow ?? throw new ArgumentNullException(nameof(resolveWindow));
5562
_dialogs = dialogs ?? throw new ArgumentNullException(nameof(dialogs));
5663
_modalHost = modalHost ?? throw new ArgumentNullException(nameof(modalHost));
5764
_windowProvider = windowProvider ?? throw new ArgumentNullException(nameof(windowProvider));
5865
}
5966

67+
/// <summary>
68+
/// Initializes a new subscription bound to a known native window.
69+
/// </summary>
70+
/// <param name="platformWindow">The native window this subscription serves.</param>
71+
/// <param name="dialogs">Creates the dialogs used to service requests.</param>
72+
/// <param name="modalHost">Coordinates dialogs with the Tizen modal navigation stack.</param>
73+
/// <param name="windowProvider">Resolves the native window that a requesting page belongs to.</param>
74+
public TizenAlertManagerSubscription(
75+
object? platformWindow,
76+
ITizenAlertDialogFactory dialogs,
77+
ITizenModalHost modalHost,
78+
ITizenPlatformWindowProvider windowProvider)
79+
: this(() => platformWindow, dialogs, modalHost, windowProvider)
80+
{
81+
}
82+
6083
/// <summary>
6184
/// Gets the native window this subscription serves.
6285
/// </summary>
63-
public object? PlatformWindow => _window;
86+
public object? PlatformWindow => _resolveWindow();
6487

6588
/// <inheritdoc/>
6689
public void OnAlertRequested(Page sender, AlertArguments arguments)
@@ -202,8 +225,23 @@ bool ShouldHandle(Page sender)
202225
return PageIsInThisWindow(sender);
203226
}
204227

205-
bool PageIsInThisWindow(IView sender) =>
206-
Equals(_windowProvider.GetPlatformWindow(sender.Handler?.MauiContext), _window);
228+
bool PageIsInThisWindow(IView sender)
229+
{
230+
var window = _resolveWindow();
231+
var senderWindow = _windowProvider.GetPlatformWindow(sender.Handler?.MauiContext);
232+
233+
if (window is null)
234+
{
235+
// The window handler has not attached the native window yet. Treat the request as
236+
// ours rather than dropping it: this subscription belongs to exactly one window
237+
// scope, and silently swallowing an alert because of handler ordering is the worst
238+
// possible outcome. If the requesting page already knows its window, that is
239+
// authoritative and is compared normally once we know ours.
240+
return true;
241+
}
242+
243+
return Equals(senderWindow, window);
244+
}
207245

208246
async Task ShowAsync<TResult>(
209247
ITizenAlertDialog<TResult> dialog,

0 commit comments

Comments
 (0)