Merge pull request #2600 from cwensley/curtis/reparent-tablerow-and-t…

…ablecell

Reparent TableRow and TableCell when added from one table to another

src/Eto/Forms/Container.cs

@@ -295,9 +295,10 @@ protected void RemoveParent(Control child)
 				child.TriggerUnLoad(EventArgs.Empty);
 			}
 			child.VisualParent = null;
-			if (ReferenceEquals(child.InternalLogicalParent, this))
-				child.InternalLogicalParent = null;
 		}
+
+		if (ReferenceEquals(child.InternalLogicalParent, this))
+			child.InternalLogicalParent = null;
 	}
 
 	/// <summary>
@@ -397,7 +398,7 @@ protected void SetParent(Control child, Action assign = null, Control previousCh
 			// Detach so parent can remove from controls collection if necessary.
 			// prevents UnLoad from being called more than once when containers think a control is still a child
 			// no-op if there is no parent (handled in detach)
-			child.Detach();
+			child.Detach(); //.VisualParent?.Remove(child);
 
 			if (ReferenceEquals(child.InternalLogicalParent, null))
 				SetLogicalParent(child);

src/Eto/Forms/Layout/TableCell.cs

@@ -7,8 +7,10 @@ namespace Eto.Forms;
 [sc.TypeConverter(typeof(TableCellConverter))]
 public class TableCell
 {
-	Control control;
-	TableLayout layout;
+	Control _control;
+	TableCellCollection _cells;
+
+	TableLayout ParentTable => _cells?._layout;
 
 	/// <summary>
 	/// Gets or sets a value indicating whether this <see cref="Eto.Forms.TableCell"/> will scale its width
@@ -31,23 +33,24 @@ public class TableCell
 	/// <value>The control.</value>
 	public Control Control
 	{
-		get { return control; }
+		get { return _control; }
 		set
 		{
-			if (control != value)
+			if (_control != value)
 			{
 				value?.Detach();
-				control?.Detach();
-				layout?.InternalRemoveLogicalParent(control);
-				control = value;
-				layout?.InternalSetLogicalParent(control);
+				_control?.Detach();
+				var layout = ParentTable;
+				layout?.InternalRemoveLogicalParent(_control);
+				_control = value;
+				layout?.InternalSetLogicalParent(_control);
 			}
 		}
 	}
 
 	internal void SetControl(Control control)
 	{
-		this.control = control;
+		_control = control;
 	}
 
 	/// <summary>
@@ -119,20 +122,29 @@ public static implicit operator TableCell(Image image)
 		return new TableCell(new ImageView { Image = image });
 	}
 
-	internal void SetLayout(TableLayout layout)
+	internal void SetLayout(TableCellCollection cells, bool shouldRemove, TableLayout oldLayout = null)
 	{
-		if (!ReferenceEquals(this.layout, layout))
+		if (!ReferenceEquals(_cells, cells))
+		{
+			// remove from old cells collection
+			if (shouldRemove)
+				_cells?.RemoveItemWithoutLayoutUpdate(this);
+
+			ParentTable?.InternalRemoveLogicalParent(_control);
+			_cells = cells;
+			ParentTable?.InternalSetLogicalParent(_control);
+		}
+		else if (!ReferenceEquals(oldLayout, ParentTable))
 		{
-			this.layout?.InternalRemoveLogicalParent(control);
-			this.layout = layout;
-			layout?.InternalSetLogicalParent(control);
+			oldLayout?.InternalRemoveLogicalParent(_control);
+			ParentTable?.InternalSetLogicalParent(_control);
 		}
 	}
 }
 
 class TableCellCollection : Collection<TableCell>, IList
 {
-	TableLayout layout;
+	internal TableLayout _layout;
 
 	public TableCellCollection()
 	{
@@ -142,26 +154,45 @@ public TableCellCollection(IEnumerable<TableCell> list)
 		: base(list.Select(r => r ?? new TableCell { ScaleWidth = true }).ToList())
 	{
 	}
-
-	internal void SetLayout(TableLayout layout)
+	
+	internal void SetLayout(TableRow row, TableLayout layout, bool shouldRemove)
 	{
-		this.layout = layout;
+		if (ReferenceEquals(layout, _layout))
+			return;
+
+		var oldLayout = _layout;
+		if (shouldRemove && _layout?.Rows is TableRowCollection oldLayoutRows)
+		{
+			// switching layouts, remove from old layout without triggering a new SetLayout
+			// This really shouldn't be done and should throw an exception
+			oldLayoutRows.RemoveItemWithoutLayoutUpdate(row);
+		}
+		_layout = layout;
+
 		foreach (var cell in this)
-			cell.SetLayout(layout);
+			cell.SetLayout(this, true, oldLayout);
 	}
+
+	internal void RemoveItemWithoutLayoutUpdate(TableCell cell)
+	{
+		var index = IndexOf(cell);
+		if (index >= 0)
+			base.RemoveItem(index);
+	}
+
 
 	protected override void RemoveItem(int index)
 	{
 		var item = this[index];
-		item?.SetLayout(null);
+		item?.SetLayout(null, false);
 		base.RemoveItem(index);
 	}
 
 	protected override void ClearItems()
 	{
 		foreach (var item in this)
 		{
-			item?.SetLayout(null);
+			item?.SetLayout(null, false);
 		}
 		base.ClearItems();
 	}
@@ -170,17 +201,17 @@ protected override void InsertItem(int index, TableCell item)
 	{
 		if (item == null)
 			item = new TableCell { ScaleWidth = true };
-		item.SetLayout(layout);
+		item.SetLayout(this, true);
 		base.InsertItem(index, item);
 	}
 
 	protected override void SetItem(int index, TableCell item)
 	{
 		var old = this[index];
-		old?.SetLayout(null);
+		old?.SetLayout(null, false);
 		if (item == null)
 			item = new TableCell { ScaleWidth = true };
-		item.SetLayout(layout);
+		item.SetLayout(this, true);
 		base.SetItem(index, item);
 	}
 

src/Eto/Forms/Layout/TableRow.cs

@@ -155,57 +155,64 @@ public static implicit operator TableCell(TableRow row)
 		return new TableCell(new TableLayout(row));
 	}
 
-	internal void SetLayout(TableLayout layout)
+	internal void SetLayout(TableLayout layout, bool shouldRemove)
 	{
-		((TableCellCollection)Cells).SetLayout(layout);
+		((TableCellCollection)Cells).SetLayout(this, layout, shouldRemove);
 	}
 }
 
 class TableRowCollection : Collection<TableRow>, IList
 {
-	TableLayout layout;
+	internal TableLayout _layout;
 	public TableRowCollection(TableLayout layout)
 	{
-		this.layout = layout;
+		_layout = layout;
 	}
 
 	public TableRowCollection(TableLayout layout, IEnumerable<TableRow> list)
 		: base(list.Select(r => r ?? new TableRow { ScaleHeight = true }).ToList())
 	{
-		this.layout = layout;
+		_layout = layout;
 		foreach (var item in this)
-			item?.SetLayout(layout);
+			item?.SetLayout(layout, true);
+	}
+
+	internal void RemoveItemWithoutLayoutUpdate(TableRow row)
+	{
+		var index = IndexOf(row);
+		if (index >= 0)
+			base.RemoveItem(index);
 	}
 
 	protected override void RemoveItem(int index)
 	{
 		var item = this[index];
-		item?.SetLayout(null);
+		item?.SetLayout(null, false);
 		base.RemoveItem(index);
 	}
 
 	protected override void ClearItems()
 	{
 		foreach (var item in this)
-			item?.SetLayout(null);
+			item?.SetLayout(null, false);
 		base.ClearItems();
 	}
 
 	protected override void InsertItem(int index, TableRow item)
 	{
 		if (item == null)
 			item = new TableRow { ScaleHeight = true };
-		item?.SetLayout(layout);
+		item?.SetLayout(_layout, true);
 		base.InsertItem(index, item);
 	}
 
 	protected override void SetItem(int index, TableRow item)
 	{
 		var old = this[index];
-		old?.SetLayout(null);
+		old?.SetLayout(null, false);
 		if (item == null)
 			item = new TableRow { ScaleHeight = true };
-		item?.SetLayout(layout);
+		item?.SetLayout(_layout, true);
 		base.SetItem(index, item);
 	}
 

test/Eto.Test/UnitTests/Forms/Layout/TableLayoutTests.cs

@@ -189,6 +189,88 @@ public void LabelInAutoSizedColumnShouldHaveCorrectWidth()
 				Assert.Greater(label.Height, 0, "Label didn't get correct height!");
 			});
 		}
+
+		[Test]
+		public void DestroyingChildShouldRemoveFromParent()
+		{
+			TableLayout layout = null;
+			Label child = null;
+			Shown(form =>
+			{
+				child = new Label { Text = "I should not be shown" };
+				layout = new TableLayout
+				{
+					Rows = {
+						new TableRow(child)
+					}
+				};
+				child.Dispose();
+				form.Content = layout;
+			}, () =>
+			{
+				Assert.IsTrue(child.IsDisposed);
+				Assert.IsNull(child.Parent);
+				CollectionAssert.DoesNotContain(layout.Children, child);
+			});
+		}
+
+		[Test, ManualTest]
+		public void CopyingRowsShouldReparentChildren()
+		{
+			ManualForm("Label above should show", form =>
+			{
+				var child = new Label { Text = "I should be shown!" };
+
+				var layout1 = new TableLayout { ID = "layout1" };
+				layout1.Rows.Add(child);
+				Assert.AreEqual(child.Parent, layout1, "#1.1 Child's parent should now be the 2nd table");
+				Assert.AreEqual(child.LogicalParent, layout1, "#1.2 Child's logical parent should now be the 2nd table");
+
+				// copy rows to a new layout
+				var layout2 = new TableLayout { ID = "layout2" };
+				foreach (var row in layout1.Rows.ToList())
+					layout2.Rows.Add(row);
+
+				Assert.AreEqual(0, layout1.Rows.Count, "#2.1 All rows should now be in the 2nd table");
+				Assert.AreEqual(child.Parent, layout2, "#2.2 Child's parent should now be the 2nd table");
+				Assert.AreEqual(child.LogicalParent, layout2, "#2.3 Child's logical parent should now be the 2nd table");
+				return layout2;
+			});
+		}
+
+		[Test, ManualTest]
+		public void CopyingCellsShouldReparentChildren()
+		{
+			ManualForm("Label above should show", form =>
+			{
+				var child = new Label { Text = "I should be shown!" };
+
+				var layout1 = new TableLayout { ID = "layout1" };
+				var cell1 = new TableCell(child);
+				var row1 = new TableRow(child);
+				layout1.Rows.Add(row1);
+				Assert.AreEqual(child.Parent, layout1, "#1.1 Child's parent should now be the 2nd table");
+				Assert.AreEqual(child.LogicalParent, layout1, "#1.2 Child's logical parent should now be the 2nd table");
+
+				// copy rows to a new layout
+				var layout2 = new TableLayout { ID = "layout2" };
+				var row2 = new TableRow();
+				foreach (var row in layout1.Rows.ToList())
+				{
+					foreach (var cell in row.Cells.ToList())
+						row2.Cells.Add(cell);
+				}
+				layout2.Rows.Add(row2);
+
+				Assert.AreEqual(1, layout1.Rows.Count, "#2.1 Should still have a single row");
+				Assert.AreEqual(0, layout1.Rows[0].Cells.Count, "#2.2 Cells should be removed from old table");
+				Assert.AreEqual(1, layout2.Rows.Count, "#2.3 2nd table should have a single row");
+				Assert.AreEqual(1, layout2.Rows[0].Cells.Count, "#2.4 All cells should now be in the 2nd table");
+				Assert.AreEqual(child.Parent, layout2, "#2.5 Child's parent should now be the 2nd table");
+				Assert.AreEqual(child.LogicalParent, layout2, "#2.6 Child's logical parent should now be the 2nd table");
+				return layout2;
+			});
+		}
 	}
 }