Centralize logic for handling logical children

src/Controls/src/Core/Application.cs

@@ -25,7 +25,6 @@ public partial class Application : Element, IResourcesProvider, IApplicationCont
 #pragma warning restore CS0612 // Type or member is obsolete
 
 		IAppIndexingProvider? _appIndexProvider;
-		ReadOnlyCollection<Element>? _logicalChildren;
 		bool _isStarted;
 
 		static readonly SemaphoreSlim SaveSemaphore = new SemaphoreSlim(1, 1);
@@ -111,17 +110,12 @@ public Page? MainPage
 			}
 		}
 
-		internal override IReadOnlyList<Element> LogicalChildrenInternal =>
-			_logicalChildren ??= new ReadOnlyCollection<Element>(InternalChildren);
-
 		/// <include file="../../docs/Microsoft.Maui.Controls/Application.xml" path="//Member[@MemberName='NavigationProxy']/Docs/*" />
 		[EditorBrowsable(EditorBrowsableState.Never)]
 		public NavigationProxy? NavigationProxy { get; private set; }
 
 		internal IResourceDictionary SystemResources => _systemResources.Value;
 
-		ObservableCollection<Element> InternalChildren { get; } = new ObservableCollection<Element>();
-
 		/// <include file="../../docs/Microsoft.Maui.Controls/Application.xml" path="//Member[@MemberName='SetAppIndexingProvider']/Docs/*" />
 		[EditorBrowsable(EditorBrowsableState.Never)]
 		public void SetAppIndexingProvider(IAppIndexingProvider provider)

src/Controls/src/Core/Border.cs

@@ -13,7 +13,6 @@ namespace Microsoft.Maui.Controls
 	public class Border : View, IContentView, IBorderView, IPaddingElement
 	{
 		float[]? _strokeDashPattern;
-		ReadOnlyCollection<Element>? _logicalChildren;
 
 		WeakNotifyPropertyChangedProxy? _strokeShapeProxy = null;
 		PropertyChangedEventHandler? _strokeShapeChanged;
@@ -26,11 +25,6 @@ public class Border : View, IContentView, IBorderView, IPaddingElement
 			_strokeProxy?.Unsubscribe();
 		}
 
-		internal ObservableCollection<Element> InternalChildren { get; } = new();
-
-		internal override IReadOnlyList<Element> LogicalChildrenInternal =>
-			_logicalChildren ??= new ReadOnlyCollection<Element>(InternalChildren);
-
 		/// <summary>Bindable property for <see cref="Content"/>.</summary>
 		public static readonly BindableProperty ContentProperty = BindableProperty.Create(nameof(Content), typeof(View),
 			typeof(Border), null, propertyChanged: ContentChanged);
@@ -274,17 +268,12 @@ public static void ContentChanged(BindableObject bindable, object oldValue, obje
 			{
 				if (oldValue is Element oldElement)
 				{
-					int index = border.InternalChildren.IndexOf(oldElement);
-					if (border.InternalChildren.Remove(oldElement))
-					{
-						border.OnChildRemoved(oldElement, index);
-					}
+					border.RemoveLogicalChildInternal(oldElement);
 				}
 
 				if (newValue is Element newElement)
 				{
-					border.InternalChildren.Add(newElement);
-					border.OnChildAdded(newElement);
+					border.AddLogicalChildInternal(newElement);
 				}
 			}
 

src/Controls/src/Core/Cells/ViewCell.cs

@@ -8,8 +8,6 @@ namespace Microsoft.Maui.Controls
 	[ContentProperty("View")]
 	public class ViewCell : Cell
 	{
-		ReadOnlyCollection<Element> _logicalChildren;
-
 		View _view;
 
 		/// <include file="../../../docs/Microsoft.Maui.Controls/ViewCell.xml" path="//Member[@MemberName='View']/Docs/*" />
@@ -25,7 +23,7 @@ public View View
 
 				if (_view != null)
 				{
-					OnChildRemoved(_view, 0);
+					RemoveLogicalChildInternal(_view);
 					_view.ComputedConstraint = LayoutConstraint.None;
 				}
 
@@ -34,18 +32,12 @@ public View View
 				if (_view != null)
 				{
 					_view.ComputedConstraint = LayoutConstraint.Fixed;
-					_logicalChildren = new ReadOnlyCollection<Element>(new List<Element>(new[] { View }));
-					OnChildAdded(_view);
-				}
-				else
-				{
-					_logicalChildren = null;
+					AddLogicalChildInternal(_view);
 				}
+
 				ForceUpdateSize();
 				OnPropertyChanged();
 			}
 		}
-
-		internal override IReadOnlyList<Element> LogicalChildrenInternal => _logicalChildren ?? base.LogicalChildrenInternal;
 	}
 }

src/Controls/src/Core/ContextFlyout.cs

@@ -9,11 +9,10 @@ namespace Microsoft.Maui.Controls
 
 	public partial class MenuFlyout : FlyoutBase, IMenuFlyout // Same pattern as MenuBarItem
 	{
-		ReadOnlyCastingList<Element, IMenuElement> _logicalChildren;
-		readonly ObservableCollection<IMenuElement> _menus = new ObservableCollection<IMenuElement>();
+		readonly List<IMenuElement> _menus = new List<IMenuElement>();
 
-		internal override IReadOnlyList<Element> LogicalChildrenInternal =>
-			_logicalChildren ??= new ReadOnlyCastingList<Element, IMenuElement>(_menus);
+		private protected override IList<Element> LogicalChildrenInternalBackingStore
+			=> new CastingList<Element, IMenuElement>(_menus);
 
 		public IMenuElement this[int index]
 		{
@@ -32,7 +31,7 @@ public IMenuElement this[int index]
 		public void Add(IMenuElement item)
 		{
 			var index = _menus.Count;
-			_menus.Add(item);
+			AddLogicalChildInternal((Element)item);
 			NotifyHandler(nameof(IMenuFlyoutHandler.Add), index, item);
 
 			// Take care of the Element internal bookkeeping
@@ -70,42 +69,24 @@ public int IndexOf(IMenuElement item)
 
 		public void Insert(int index, IMenuElement item)
 		{
-			_menus.Insert(index, item);
+			InsertLogicalChildInternal(index, (Element)item);
 			NotifyHandler(nameof(IMenuFlyoutHandler.Insert), index, item);
-
-			// Take care of the Element internal bookkeeping
-			if (item is Element element)
-			{
-				OnChildAdded(element);
-			}
 		}
 
 		public bool Remove(IMenuElement item)
 		{
 			var index = _menus.IndexOf(item);
-			var result = _menus.Remove(item);
+			var result = RemoveLogicalChildInternal((Element)item, index);
 			NotifyHandler(nameof(IMenuFlyoutHandler.Remove), index, item);
 
-			// Take care of the Element internal bookkeeping
-			if (item is Element element)
-			{
-				OnChildRemoved(element, index);
-			}
-
 			return result;
 		}
 
 		public void RemoveAt(int index)
 		{
 			var item = _menus[index];
-			_menus.RemoveAt(index);
+			RemoveLogicalChildInternal((Element)item, index);
 			NotifyHandler(nameof(IMenuFlyoutHandler.Remove), index, item);
-
-			// Take care of the Element internal bookkeeping
-			if (item is Element element)
-			{
-				OnChildRemoved(element, index);
-			}
 		}
 
 		IEnumerator IEnumerable.GetEnumerator()

src/Controls/src/Core/Element.cs

@@ -37,6 +37,10 @@ public abstract partial class Element : BindableObject, IElementDefinition, INam
 
 		string _styleId;
 
+		IReadOnlyList<Element> _logicalChildrenReadonly;
+
+		IList<Element> _internalChildren;
+
 		/// <include file="../../docs/Microsoft.Maui.Controls/Element.xml" path="//Member[@MemberName='AutomationId']/Docs/*" />
 		public string AutomationId
 		{
@@ -56,6 +60,7 @@ public string ClassId
 			get => (string)GetValue(ClassIdProperty);
 			set => SetValue(ClassIdProperty, value);
 		}
+
 		/// <include file="../../docs/Microsoft.Maui.Controls/Element.xml" path="//Member[@MemberName='Effects']/Docs/*" />
 		public IList<Effect> Effects
 		{
@@ -97,7 +102,119 @@ public string StyleId
 			}
 		}
 
-		internal virtual IReadOnlyList<Element> LogicalChildrenInternal => EmptyChildren;
+		// Leaving this internal for now.
+		// If users want to add/remove from this they can use
+		// AddLogicalChildren and RemoveLogicalChildren on the respective control
+		// if available.
+		//
+		// Ultimately I don't think we'll need these to be virtual but some controls (layout)
+		// are going to take a more focused effort so I'd rather just do that in a 
+		// separate PR. I don't think there's ever a scenario where a subclass needs
+		// to replace the backing store.
+		// If everyone just uses AddLogicalChildren and RemoveLogicalChildren
+		// and then overrides OnChildAdded/OnChildRemoved
+		// that should be sufficient
+		internal IReadOnlyList<Element> LogicalChildrenInternal
+		{
+			get
+			{
+				SetupChildren();
+				return _logicalChildrenReadonly;
+			}
+		}
+
+		private protected virtual IList<Element> LogicalChildrenInternalBackingStore
+		{
+			get
+			{
+				_internalChildren ??= new List<Element>();
+				return _internalChildren;
+			}
+		}
+
+		[EditorBrowsable(EditorBrowsableState.Never)]
+		[Obsolete("Do not use! This is to be removed! Just used by Hot Reload! To be replaced with IVisualTreeElement!")]
+		public ReadOnlyCollection<Element> LogicalChildren =>
+			new ReadOnlyCollection<Element>(new TemporaryWrapper(LogicalChildrenInternal));
+
+		IReadOnlyList<Element> IElementController.LogicalChildren => LogicalChildrenInternal;
+
+		void SetupChildren()
+		{
+			_logicalChildrenReadonly ??= new ReadOnlyCollection<Element>(LogicalChildrenInternalBackingStore);
+		}
+
+		internal void InsertLogicalChildInternal(int index, Element element)
+		{
+			if (element is null)
+			{
+				return;
+			}
+
+			SetupChildren();
+
+			LogicalChildrenInternalBackingStore.Insert(index, element);
+			OnChildAdded(element);
+		}
+
+		internal void AddLogicalChildInternal(Element element)
+		{
+			if (element is null)
+			{
+				return;
+			}
+
+			SetupChildren();
+
+			LogicalChildrenInternalBackingStore.Add(element);
+			OnChildAdded(element);
+		}
+
+		internal bool RemoveLogicalChildInternal(Element element)
+		{
+			if (element is null)
+			{
+				return false;
+			}
+
+			if (LogicalChildrenInternalBackingStore is null)
+				return false;
+
+			var oldLogicalIndex = LogicalChildrenInternalBackingStore.IndexOf(element);
+			if (oldLogicalIndex < 0)
+				return false;
+
+			RemoveLogicalChildInternal(element, oldLogicalIndex);
+
+			return true;
+		}
+
+		internal void ClearLogicalChildren()
+		{
+			if (LogicalChildrenInternalBackingStore is null)
+				return;
+
+			if (LogicalChildrenInternal == EmptyChildren)
+				return;
+
+			// Reverse for-loop, so children can be removed while iterating
+			for (int i = LogicalChildrenInternalBackingStore.Count - 1; i >= 0; i--)
+			{
+				RemoveLogicalChildInternal(LogicalChildrenInternalBackingStore[i], i);
+			}
+		}
+
+		/// <summary>
+		/// This doesn't validate that the oldLogicalIndex is correct, so be sure you're passing in the
+		/// correct index
+		/// </summary>
+		internal bool RemoveLogicalChildInternal(Element element, int oldLogicalIndex)
+		{
+			LogicalChildrenInternalBackingStore.Remove(element);
+			OnChildRemoved(element, oldLogicalIndex);
+
+			return true;
+		}
 
 		internal IEnumerable<Element> AllChildren
 		{
@@ -118,14 +235,6 @@ internal IEnumerable<Element> AllChildren
 		// return null by default so we don't need to foreach over an empty collection in OnPropertyChanged
 		internal virtual IEnumerable<Element> ChildrenNotDrawnByThisElement => null;
 
-		IReadOnlyList<Element> IElementController.LogicalChildren => LogicalChildrenInternal;
-
-		/// <include file="../../docs/Microsoft.Maui.Controls/Element.xml" path="//Member[@MemberName='LogicalChildren']/Docs/*" />
-		[EditorBrowsable(EditorBrowsableState.Never)]
-		[Obsolete("Do not use! This is to be removed! Just used by Hot Reload! To be replaced with IVisualTreeElement!")]
-		public ReadOnlyCollection<Element> LogicalChildren =>
-			new ReadOnlyCollection<Element>(new TemporaryWrapper(LogicalChildrenInternal));
-
 		internal bool Owned { get; set; }
 
 		internal Element ParentOverride
@@ -200,7 +309,7 @@ public Element Parent
 					((IElementDefinition)RealParent).AddResourcesChangedListener(OnParentResourcesChanged);
 				}
 
-				object context = value != null ? value.BindingContext : null;
+				object context = value?.BindingContext;
 				if (value != null)
 				{
 					value.SetChildInheritedBindingContext(this, context);
@@ -322,7 +431,8 @@ void INameScope.UnregisterName(string name)
 			base.SetDynamicResource(property, key);
 		}
 
-		IReadOnlyList<Maui.IVisualTreeElement> IVisualTreeElement.GetVisualChildren() => LogicalChildrenInternal;
+		IReadOnlyList<Maui.IVisualTreeElement> IVisualTreeElement.GetVisualChildren()
+			=> LogicalChildrenInternal;
 
 		IVisualTreeElement IVisualTreeElement.GetVisualParent() => this.Parent;
 

src/Controls/src/Core/HandlerImpl/Application/Application.Impl.cs

@@ -82,10 +82,7 @@ internal void RemoveWindow(Window window)
 
 			if (window is Element windowElement)
 			{
-				var oldIndex = InternalChildren.IndexOf(windowElement);
-				InternalChildren.RemoveAt(oldIndex);
-				windowElement.Parent = null;
-				OnChildRemoved(windowElement, oldIndex);
+				RemoveLogicalChildInternal(windowElement);
 			}
 
 			_windows.Remove(window);
@@ -134,9 +131,7 @@ void AddWindow(Window window)
 
 			if (window is Element windowElement)
 			{
-				windowElement.Parent = this;
-				InternalChildren.Add(windowElement);
-				OnChildAdded(windowElement);
+				AddLogicalChildInternal(windowElement);
 			}
 
 			if (window is NavigableElement ne)