Preserve the correct binding context for menu items (#7358)

* Rework parenting for MenuBarItems * - sync menu bar items when collection changes * - clear parent * - fix spelling Co-authored-by: Shane Neuville <shneuvil@microsoft.com>
dotnet · Oct 4, 2022 · 71739c2 · 71739c2
1 parent e75b91d
commit 71739c2
Show file tree

Hide file tree

Showing 5 changed files with 166 additions and 52 deletions.
diff --git a/src/Controls/src/Core/MenuBar.cs b/src/Controls/src/Core/MenuBar.cs
@@ -16,12 +16,8 @@ public bool IsEnabled
 			set { SetValue(IsEnabledProperty, value); }
 		}
 
-		ReadOnlyCastingList<Element, IMenuBarItem> _logicalChildren;
 		readonly ObservableCollection<IMenuBarItem> _menus = new ObservableCollection<IMenuBarItem>();
 
-		internal override IReadOnlyList<Element> LogicalChildrenInternal =>
-			_logicalChildren ??= new ReadOnlyCastingList<Element, IMenuBarItem>(_menus);
-
 		public IMenuBarItem this[int index]
 		{
 			get { return _menus[index]; }
@@ -41,19 +37,24 @@ public void Add(IMenuBarItem item)
 			var index = _menus.Count;
 			_menus.Add(item);
 			NotifyHandler(nameof(IMenuBarHandler.Add), index, item);
+		}
 
-			// Take care of the Element internal bookkeeping
-			if (item is Element element)
+		internal void SyncMenuBarItemsFromPages(IList<MenuBarItem> menuBarItems)
+		{
+			for (int i = 0; i < menuBarItems.Count; i++)
 			{
-				OnChildAdded(element);
+				var menuBarItem = menuBarItems[i];
+				if (this.Count > i && this[i] == menuBarItem)
+					continue;
+
+				if (this.Contains(menuBarItem))
+					Remove(menuBarItem);
+
+				Insert(i, menuBarItem);
 			}
-		}
 
-		internal void ReplaceWith(IList<MenuBarItem> menuBarItems)
-		{
-			Clear();
-			foreach (var menuItem in menuBarItems)
-				Add(menuItem);
+			while (this.Count > menuBarItems.Count)
+				RemoveAt(this.Count - 1);
 		}
 
 		public void Clear()
@@ -86,12 +87,6 @@ public void Insert(int index, IMenuBarItem item)
 		{
 			_menus.Insert(index, item);
 			NotifyHandler(nameof(IMenuBarHandler.Insert), index, item);
-
-			// Take care of the Element internal bookkeeping
-			if (item is Element element)
-			{
-				OnChildAdded(element);
-			}
 		}
 
 		public bool Remove(IMenuBarItem item)
@@ -100,11 +95,8 @@ public bool Remove(IMenuBarItem item)
 			var result = _menus.Remove(item);
 			NotifyHandler(nameof(IMenuBarHandler.Remove), index, item);
 
-			// Take care of the Element internal bookkeeping
-			if (item is Element element)
-			{
-				OnChildRemoved(element, index);
-			}
+			if (item is Element e)
+				e.Parent = null;
 
 			return result;
 		}
@@ -119,11 +111,8 @@ public void RemoveAt(int index)
 			_menus.RemoveAt(index);
 			NotifyHandler(nameof(IMenuBarHandler.Remove), index, item);
 
-			// Take care of the Element internal bookkeeping
-			if (item is Element element)
-			{
-				OnChildRemoved(element, index);
-			}
+			if (item is Element e)
+				e.Parent = null;
 		}
 
 		IEnumerator IEnumerable.GetEnumerator()

diff --git a/src/Controls/src/Core/MenuBarTracker.cs b/src/Controls/src/Core/MenuBarTracker.cs
@@ -25,16 +25,10 @@ public MenuBarTracker(Element parent, string handlerProperty)
 
 		void OnMenuBarItemCollectionChanged(object sender, EventArgs e)
 		{
+			_menuBar.SyncMenuBarItemsFromPages(ToolbarItems);
+
 			if (_handlerProperty != null)
 			{
-				// For now we just reset the entire menu if users modify the menubar
-				// collection
-				//if (_parent is IMenuBarElement mbe &&
-				//	mbe.MenuBar?.Handler != null)
-				//{
-				//	mbe.MenuBar.Handler.DisconnectHandler();
-				//}
-
 				_parent?.Handler?.UpdateValue(_handlerProperty);
 			}
 		}
@@ -47,7 +41,7 @@ public MenuBar MenuBar
 				if (menuBarItems.Count == 0)
 					return null;
 
-				_menuBar.ReplaceWith(ToolbarItems);
+				_menuBar.SyncMenuBarItemsFromPages(ToolbarItems);
 
 				return _menuBar;
 			}

diff --git a/src/Controls/src/Core/Page.cs b/src/Controls/src/Core/Page.cs
@@ -565,10 +565,17 @@ void OnPageBusyChanged()
 
 		void OnToolbarItemsCollectionChanged(object sender, NotifyCollectionChangedEventArgs args)
 		{
-			if (args.Action != NotifyCollectionChangedAction.Add)
-				return;
-			foreach (IElementDefinition item in args.NewItems)
-				item.Parent = this;
+			if (args.NewItems != null)
+			{
+				foreach (IElementDefinition item in args.NewItems)
+					item.Parent = this;
+			}
+
+			if (args.OldItems != null)
+			{
+				foreach (IElementDefinition item in args.OldItems)
+					item.Parent = null;
+			}
 		}
 
 		bool ShouldLayoutChildren()

diff --git a/src/Controls/tests/Core.UnitTests/Menu/MenuBarTests.cs b/src/Controls/tests/Core.UnitTests/Menu/MenuBarTests.cs
@@ -37,6 +37,77 @@ MenuBarHandler CreateMenuBarHandler(Action<string, IMenuBarHandler, IMenuBar, Me
 			return handler;
 		}
 
+		[Fact]
+		public void UsingWindowDoesNotReAssignParents()
+		{
+			MenuFlyoutItem flyout;
+			MenuBarItem menuItem;
+
+			var page = new ContentPage
+			{
+				MenuBarItems =
+				{
+					(menuItem = new MenuBarItem
+					{
+						(flyout = new MenuFlyoutItem { })
+					})
+				}
+			};
+
+			Assert.Equal(menuItem, flyout.Parent);
+			Assert.Equal(page, menuItem.Parent);
+
+			var window = new Window(page);
+
+			Assert.Equal(menuItem, flyout.Parent);
+			Assert.Equal(page, menuItem.Parent);
+			Assert.Equal(window, page.Parent);
+
+			var menubar = (window as IMenuBarElement).MenuBar;
+			Assert.NotNull(menubar);
+
+			Assert.Equal(menuItem, flyout.Parent);
+			Assert.Equal(page, menuItem.Parent);
+			Assert.Equal(window, page.Parent);
+		}
+
+		[Fact]
+		public void UsingWindowDoesNotReAssignBindingContext()
+		{
+			var bindingContext = new
+			{
+				Name = "Matthew"
+			};
+
+			MenuFlyoutItem flyout;
+			MenuBarItem menuItem;
+
+			var page = new ContentPage
+			{
+				BindingContext = bindingContext,
+				MenuBarItems =
+				{
+					(menuItem = new MenuBarItem
+					{
+						(flyout = new MenuFlyoutItem { })
+					})
+				}
+			};
+
+			flyout.SetBinding(MenuFlyoutItem.TextProperty, new Binding(nameof(bindingContext.Name)));
+
+			Assert.Equal(bindingContext.Name, flyout.Text);
+
+			var window = new Window(page);
+
+			Assert.Equal(bindingContext.Name, flyout.Text);
+
+			var menubar = (window as IMenuBarElement).MenuBar;
+			Assert.NotNull(menubar);
+
+			Assert.Equal(bindingContext.Name, flyout.Text);
+		}
+
 		class NonThrowingMenuBarHandler : MenuBarHandler
 		{
 			public NonThrowingMenuBarHandler(IPropertyMapper mapper, CommandMapper commandMapper)

diff --git a/src/Controls/tests/Core.UnitTests/Menu/MenuTestBase.cs b/src/Controls/tests/Core.UnitTests/Menu/MenuTestBase.cs
@@ -30,13 +30,40 @@ public void UsingIndexUpdatesParent()
 
 			menuBar.Add(child0);
 
-			Assert.Same(menuBar, child0.Parent);
+			// Menu Bar Items only get parented to pages currently
+			if (typeof(TChildType) == typeof(MenuBarItem))
+			{
+				var cp = new ContentPage();
+				cp.MenuBarItems.Add(child0 as MenuBarItem);
+				Assert.Same(cp, child0.Parent);
+			}
+			else
+			{
+				Assert.Same(menuBar, child0.Parent);
+			}
+
 			Assert.Null(child1.Parent);
 
-			menuBar[0] = child1;
+			// Menu Bar Items only get parented to pages currently
+			if (typeof(TChildType) == typeof(MenuBarItem))
+			{
+				(child0.Parent as ContentPage)!.MenuBarItems[0] = child1 as MenuBarItem;
+			}
+			else
+			{
+				menuBar[0] = child1;
+			}
 
 			Assert.Null(child0.Parent);
-			Assert.Same(menuBar, child1.Parent);
+
+			if (typeof(TChildType) == typeof(MenuBarItem))
+			{
+				Assert.True(child1.Parent is ContentPage);
+			}
+			else
+			{
+				Assert.Same(menuBar, child1.Parent);
+			}
 		}
 
 		[Fact]
@@ -47,13 +74,39 @@ public void ClearUpdatesParent()
 			var child0 = new TChildType();
 			var child1 = new TChildType();
 
-			menuBar.Add(child0);
-			menuBar.Add(child1);
-
-			Assert.Same(menuBar, child0.Parent);
-			Assert.Same(menuBar, child1.Parent);
-
-			menuBar.Clear();
+			// Menu Bar Items only get parented to pages currently
+			if (typeof(TChildType) == typeof(MenuBarItem))
+			{
+				// this sets up the MenuBarTracker
+				var cp = new ContentPage();
+				_ = new Window()
+				{
+					Page = cp
+				};
+
+				cp.MenuBarItems.Add(child0 as MenuBarItem);
+				cp.MenuBarItems.Add(child1 as MenuBarItem);
+
+				Assert.Same(cp, child0.Parent);
+				Assert.Same(cp, child1.Parent);
+			}
+			else
+			{
+				menuBar.Add(child0);
+				menuBar.Add(child1);
+
+				Assert.Same(menuBar, child0.Parent);
+				Assert.Same(menuBar, child1.Parent);
+			}
+
+			if (typeof(TChildType) == typeof(MenuBarItem))
+			{
+				(child0.Parent as ContentPage)!.MenuBarItems.Clear();
+			}
+			else
+			{
+				menuBar.Clear();
+			}
 
 			Assert.Null(child0.Parent);
 			Assert.Null(child1.Parent);