Skip to content

Commit 58776f0

Browse files
RedthCopilot
andcommitted
Address review: reset container on disconnect, make validation hook protected
Two defects found in review of the SetContainerView extensibility point. 1. Disconnecting a handler that still had an active container left both ContainerView and HasContainer set. ElementHandler.DisconnectHandler() nulls PlatformView and never touched container state, so a later reconnect hit the HasContainer equality short-circuit in the setter, skipped SetupContainer(), and kept serving the stale wrapper that still held the previous platform view. The teardown has to run while PlatformView is still reachable: the generic PlatformView getter throws when null, so calling RemoveContainer() after the null would throw on Android and Tizen. Add a private protected virtual ElementHandler.OnDisconnecting() hook that runs before PlatformView is cleared, and override it in ViewHandler to flip HasContainer off (which unwinds the container through the platform's own RemoveContainer()) and clear ContainerView. RemoveContainer() is therefore invoked under exactly the precondition it already runs under during normal operation, so every existing platform override stays on its supported path. 2. ValidateContainerView was private protected, so an external subclass that shadows ContainerView with its own wrapper type could not enforce that type and the broad setter could corrupt its strongly typed getter. Promote it to protected virtual with XML docs, and update the iOS/MacCatalyst and Tizen overrides accordingly. The external backend test handler now overrides ValidateContainerView to require its own wrapper type. Note the override is declared with `object` because on the neutral TFM the PlatformView alias is System.Object. New regression tests (5), all verified to fail without the fixes: DisconnectingWithAnActiveContainerTearsTheContainerDown, ReconnectingAfterDisconnectWithAnActiveContainerRebuildsTheContainer, SetContainerViewRejectsAContainerOfTheWrongType, SetContainerViewRejectionLeavesAnExistingContainerIntact, SetContainerViewStillAcceptsNullAfterValidationIsOverridden. Also applies two reviewer nits: scope the SetupContainer/RemoveContainer remarks to overrides outside this assembly (in-box handlers still use the private protected setter), and restore `using System;` first in ViewHandlerOfT.iOS.cs to match the other platform partials. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent b33290c commit 58776f0

14 files changed

Lines changed: 213 additions & 8 deletions

File tree

src/Core/src/Handlers/Element/ElementHandler.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,10 @@ void IElementHandler.DisconnectHandler()
141141
{
142142
if (PlatformView != null && VirtualView != null)
143143
{
144+
// Give derived handlers a chance to tear down state that needs the platform view
145+
// while it is still reachable through the PlatformView property.
146+
OnDisconnecting();
147+
144148
// We set the PlatformView to null so no one outside of this handler tries to access
145149
// PlatformView. PlatformView access should be isolated to the instance passed into
146150
// DisconnectHandler
@@ -151,5 +155,10 @@ void IElementHandler.DisconnectHandler()
151155

152156
_handlerState = ElementHandlerState.Disconnected;
153157
}
158+
159+
// Runs while PlatformView is still set, so overrides can use it to unwind platform state.
160+
private protected virtual void OnDisconnecting()
161+
{
162+
}
154163
}
155164
}

src/Core/src/Handlers/View/ViewHandler.cs

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -145,14 +145,17 @@ public virtual bool NeedsContainer
145145
/// Constructs the <see cref="ContainerView"/> and adds <see cref="PlatformView"/> to a container.
146146
/// </summary>
147147
/// <remarks>This method is called when <see cref="HasContainer"/> is set to <see langword="true"/>.
148-
/// Overrides should call <see cref="SetContainerView(PlatformView?)"/> to publish the container they created.</remarks>
148+
/// An override that cannot reach the <see cref="ContainerView"/> setter - that is, one declared outside of this
149+
/// assembly - should publish the container it created by calling <see cref="SetContainerView(PlatformView?)"/>.</remarks>
149150
protected abstract void SetupContainer();
150151

151152
/// <summary>
152153
/// Deconstructs the <see cref="ContainerView"/> and removes <see cref="PlatformView"/> from its container.
153154
/// </summary>
154155
/// <remarks>This method is called when <see cref="HasContainer"/> is set to <see langword="false"/>.
155-
/// Overrides should call <see cref="SetContainerView(PlatformView?)"/> with <see langword="null"/> to clear the container they removed.</remarks>
156+
/// An override that cannot reach the <see cref="ContainerView"/> setter - that is, one declared outside of this
157+
/// assembly - should clear the container it removed by calling <see cref="SetContainerView(PlatformView?)"/> with
158+
/// <see langword="null"/>.</remarks>
156159
protected abstract void RemoveContainer();
157160

158161
/// <summary>
@@ -232,10 +235,55 @@ protected void SetContainerView(PlatformView? containerView)
232235
ContainerView = containerView;
233236
}
234237

235-
// Lets a platform-specific handler reject container views that would break the strongly typed
236-
// ContainerView property it shadows (for example the WrapperView-typed property on iOS and Tizen).
237-
private protected virtual void ValidateContainerView(PlatformView containerView)
238+
/// <summary>
239+
/// Validates that <paramref name="containerView"/> is a container type this handler supports, throwing if it is not.
240+
/// </summary>
241+
/// <param name="containerView">The candidate container view. This is never <see langword="null"/>.</param>
242+
/// <remarks>
243+
/// <para>Called by <see cref="SetContainerView(PlatformView?)"/> before the value is stored. Override this when a derived
244+
/// handler shadows <see cref="ContainerView"/> with a more specific type, so that an unsupported container is rejected
245+
/// where the mistake is made instead of surfacing later as an <see cref="System.InvalidCastException"/> from the
246+
/// shadowing getter.</para>
247+
/// <para>The .NET MAUI iOS, Mac Catalyst and Tizen handlers use this to require a
248+
/// <see cref="Platform.WrapperView"/>. An external backend that shadows <see cref="ContainerView"/> with its own
249+
/// wrapper type should do the same.</para>
250+
/// <example>
251+
/// <code language="csharp">
252+
/// public new MyWrapperView? ContainerView => (MyWrapperView?)base.ContainerView;
253+
///
254+
/// protected override void ValidateContainerView(MyPlatformView containerView)
255+
/// {
256+
/// if (containerView is not MyWrapperView)
257+
/// {
258+
/// throw new ArgumentException(
259+
/// $"The container view must be a {nameof(MyWrapperView)}.",
260+
/// nameof(containerView));
261+
/// }
262+
///
263+
/// base.ValidateContainerView(containerView);
264+
/// }
265+
/// </code>
266+
/// </example>
267+
/// </remarks>
268+
/// <exception cref="System.ArgumentException">Thrown by overrides when <paramref name="containerView"/> is not supported.</exception>
269+
protected virtual void ValidateContainerView(PlatformView containerView)
270+
{
271+
}
272+
273+
private protected override void OnDisconnecting()
238274
{
275+
// The platform view is still reachable here, which is the same precondition RemoveContainer()
276+
// runs under during normal operation, so existing overrides stay on their supported path.
277+
// Without this, ContainerView and HasContainer would survive the disconnect and a later
278+
// reconnect would skip SetupContainer() and keep pointing at the stale container.
279+
if (HasContainer)
280+
{
281+
HasContainer = false;
282+
}
283+
284+
ContainerView = null;
285+
286+
base.OnDisconnecting();
239287
}
240288

241289
object? IViewHandler.ContainerView => ContainerView;

src/Core/src/Handlers/View/ViewHandlerOfT.Tizen.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,17 @@ public abstract partial class ViewHandler<TVirtualView, TPlatformView> : IPlatfo
2121
protected set => base.ContainerView = value;
2222
}
2323

24-
private protected override void ValidateContainerView(NView containerView)
24+
/// <inheritdoc/>
25+
protected override void ValidateContainerView(NView containerView)
2526
{
2627
if (containerView is not WrapperView)
2728
{
2829
throw new ArgumentException(
2930
$"The container view must be a {nameof(WrapperView)} because {GetType().Name} exposes {nameof(ContainerView)} as a {nameof(WrapperView)}.",
3031
nameof(containerView));
3132
}
33+
34+
base.ValidateContainerView(containerView);
3235
}
3336

3437
~ViewHandler()

src/Core/src/Handlers/View/ViewHandlerOfT.iOS.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
using Microsoft.Maui.Graphics;
21
using System;
2+
using Microsoft.Maui.Graphics;
33
using UIKit;
44

55
namespace Microsoft.Maui.Handlers
@@ -12,14 +12,17 @@ public partial class ViewHandler<TVirtualView, TPlatformView> : IPlatformViewHan
1212
protected set => base.ContainerView = value;
1313
}
1414

15-
private protected override void ValidateContainerView(UIView containerView)
15+
/// <inheritdoc/>
16+
protected override void ValidateContainerView(UIView containerView)
1617
{
1718
if (containerView is not WrapperView)
1819
{
1920
throw new ArgumentException(
2021
$"The container view must be a {nameof(WrapperView)} because {GetType().Name} exposes {nameof(ContainerView)} as a {nameof(WrapperView)}.",
2122
nameof(containerView));
2223
}
24+
25+
base.ValidateContainerView(containerView);
2326
}
2427

2528
public UIViewController? ViewController { get; set; }

src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,3 +364,4 @@ Microsoft.Maui.ISwipeItemMenuItemIconColor
364364
Microsoft.Maui.ISwipeItemMenuItemIconColor.IconColor.get -> Microsoft.Maui.Graphics.Color?
365365
static Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.MapIconColor(Microsoft.Maui.Handlers.ISwipeItemMenuItemHandler! handler, Microsoft.Maui.ISwipeItemMenuItem! view) -> void
366366
Microsoft.Maui.Handlers.ViewHandler.SetContainerView(Android.Views.View? containerView) -> void
367+
virtual Microsoft.Maui.Handlers.ViewHandler.ValidateContainerView(Android.Views.View! containerView) -> void

src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,5 @@ Microsoft.Maui.ISwipeItemMenuItemIconColor
6464
Microsoft.Maui.ISwipeItemMenuItemIconColor.IconColor.get -> Microsoft.Maui.Graphics.Color?
6565
static Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.MapIconColor(Microsoft.Maui.Handlers.ISwipeItemMenuItemHandler! handler, Microsoft.Maui.ISwipeItemMenuItem! view) -> void
6666
Microsoft.Maui.Handlers.ViewHandler.SetContainerView(UIKit.UIView? containerView) -> void
67+
virtual Microsoft.Maui.Handlers.ViewHandler.ValidateContainerView(UIKit.UIView! containerView) -> void
68+
override Microsoft.Maui.Handlers.ViewHandler<TVirtualView, TPlatformView>.ValidateContainerView(UIKit.UIView! containerView) -> void

src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,3 +61,5 @@ Microsoft.Maui.ISwipeItemMenuItemIconColor
6161
Microsoft.Maui.ISwipeItemMenuItemIconColor.IconColor.get -> Microsoft.Maui.Graphics.Color?
6262
static Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.MapIconColor(Microsoft.Maui.Handlers.ISwipeItemMenuItemHandler! handler, Microsoft.Maui.ISwipeItemMenuItem! view) -> void
6363
Microsoft.Maui.Handlers.ViewHandler.SetContainerView(UIKit.UIView? containerView) -> void
64+
virtual Microsoft.Maui.Handlers.ViewHandler.ValidateContainerView(UIKit.UIView! containerView) -> void
65+
override Microsoft.Maui.Handlers.ViewHandler<TVirtualView, TPlatformView>.ValidateContainerView(UIKit.UIView! containerView) -> void

src/Core/src/PublicAPI/net-tizen/PublicAPI.Unshipped.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,5 @@ Microsoft.Maui.ISwipeItemMenuItemIconColor
2222
Microsoft.Maui.ISwipeItemMenuItemIconColor.IconColor.get -> Microsoft.Maui.Graphics.Color?
2323
static Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.MapIconColor(Microsoft.Maui.Handlers.ISwipeItemMenuItemHandler! handler, Microsoft.Maui.ISwipeItemMenuItem! view) -> void
2424
Microsoft.Maui.Handlers.ViewHandler.SetContainerView(Tizen.NUI.BaseComponents.View? containerView) -> void
25+
virtual Microsoft.Maui.Handlers.ViewHandler.ValidateContainerView(Tizen.NUI.BaseComponents.View! containerView) -> void
26+
override Microsoft.Maui.Handlers.ViewHandler<TVirtualView, TPlatformView>.ValidateContainerView(Tizen.NUI.BaseComponents.View! containerView) -> void

src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,3 +49,4 @@ Microsoft.Maui.ISwipeItemMenuItemIconColor
4949
Microsoft.Maui.ISwipeItemMenuItemIconColor.IconColor.get -> Microsoft.Maui.Graphics.Color?
5050
static Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.MapIconColor(Microsoft.Maui.Handlers.ISwipeItemMenuItemHandler! handler, Microsoft.Maui.ISwipeItemMenuItem! view) -> void
5151
Microsoft.Maui.Handlers.ViewHandler.SetContainerView(Microsoft.UI.Xaml.FrameworkElement? containerView) -> void
52+
virtual Microsoft.Maui.Handlers.ViewHandler.ValidateContainerView(Microsoft.UI.Xaml.FrameworkElement! containerView) -> void

src/Core/src/PublicAPI/net/PublicAPI.Unshipped.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,4 @@ static Microsoft.Maui.Handlers.ShapeViewHandler.MapFlowDirection(Microsoft.Maui.
3232
static Microsoft.Maui.Handlers.SearchBarHandler.MapCursorPosition(Microsoft.Maui.Handlers.ISearchBarHandler! handler, Microsoft.Maui.ISearchBar! searchBar) -> void
3333
static Microsoft.Maui.Handlers.SearchBarHandler.MapSelectionLength(Microsoft.Maui.Handlers.ISearchBarHandler! handler, Microsoft.Maui.ISearchBar! searchBar) -> void
3434
Microsoft.Maui.Handlers.ViewHandler.SetContainerView(object? containerView) -> void
35+
virtual Microsoft.Maui.Handlers.ViewHandler.ValidateContainerView(object! containerView) -> void

0 commit comments

Comments
 (0)