Removing and Re-Adding a MenuBar breaks Width

[tznind]

Describe the bug

Removing and Re-Adding a View should not change any of it's size/position parameters.

But for MenuBar it does, it changes Dim.Fill(0) to Dim.Absolute(0)

Strangely it manifests only for the second menu bar and not the first. Do not understand why but is consistent. I first noticed this in gui-cs/TerminalGuiDesigner#287 .

TerminalGuiDesigner will sometimes remove and re-add a View to a parent. This sometimes is for sensible reasons (e.g. dragging into a new view container). Also sometimes to avoid Exceptions being thrown for changing some properties while mounted. It is important that removing and re-adding does not modify properties (other than parent :) )

[Fact]
public void RemoveAndThenAddMenuBar_ShouldNotChangeWidth ()
{
	MenuBar menuBar;
	MenuBar menuBar2;

	var w = new Window ();
	menuBar2 = new Terminal.Gui.MenuBar ();
	menuBar = new Terminal.Gui.MenuBar ();
	w.Width = Dim.Fill (0);
	w.Height = Dim.Fill (0);
	w.X = 0;
	w.Y = 0;

	w.Visible = true;
	w.Modal = false;
	w.Title = "";
	menuBar.Width = Dim.Fill (0);
	menuBar.Height = 1;
	menuBar.X = 0;
	menuBar.Y = 0;
	menuBar.Visible = true;
	w.Add (menuBar);

	menuBar2.Width = Dim.Fill (0);
	menuBar2.Height = 1;
	menuBar2.X = 0;
	menuBar2.Y = 4;
	menuBar2.Visible = true;
	w.Add (menuBar2);


	var menuBars = w.Subviews.OfType<MenuBar> ().ToArray ();
	Assert.Equal (2, menuBars.Length);

	Assert.Equal (Dim.Fill (0), menuBars [0].Width);
	Assert.Equal (Dim.Fill (0), menuBars [1].Width);

	// Goes wrong here
	w.Remove (menuBar);
	w.Remove (menuBar2);

	w.Add (menuBar);
	w.Add (menuBar2);

	// These assertions fail
	Assert.Equal (Dim.Fill (0), menuBars [0].Width);
	Assert.Equal (Dim.Fill (0), menuBars [1].Width);
}

  Message: 
Assert.Equal() Failure: Values differ
Expected: Absolute(0)
Actual:   Fill(0)

  Stack Trace: 
MenuBarTests.RemoveAndThenAddMenuBar_ShouldNotChangeWidth() line 2806
RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

[tig]

This is likely related to the spaghetti code that is "Toplevel".

See the issues listed here: #3135

[tig]

I've added your test to #3135 and changed it to this in order to pass for now (use View instead of Window)

		var w = new View ();
		menuBar2 = new Terminal.Gui.MenuBar ();
		menuBar = new Terminal.Gui.MenuBar ();
		w.Width = Dim.Fill (0);
		w.Height = Dim.Fill (0);
		w.X = 0;
		w.Y = 0;

		w.Visible = true;
		//w.Modal = false;

[BDisp]

The affecting code is the RemoveMenuStatusBar method. Commenting the MenuBar?.Dispose (); and the StatusBar?.Dispose (); will not fail. A super-view must only dispose a sub-view if the super-view is being disposed.

	internal void RemoveMenuStatusBar (View view)
	{
		if (view is MenuBar) {
			//MenuBar?.Dispose ();
			MenuBar = null;
		}
		if (view is StatusBar) {
			//StatusBar?.Dispose ();
			StatusBar = null;
		}
	}

The Toplevel.Remove is also wrong because of if (this is Toplevel Toplevel && (Toplevel.MenuBar != null) {. So this is always Toplevel and must call RemoveMenuStatusBar no matter MenuBar != null or StatusBar != null.

	///<inheritdoc/>
	public override void Remove (View view)
	{
		RemoveMenuStatusBar (view);
		base.Remove (view);
	}

Unit test for the StatusBar:

		[Fact]
		public void RemoveAndThenAddStatusBar_ShouldNotChangeWidth ()
		{
			StatusBar statusBar;
			StatusBar statusBar2;

			var w = new Window ();
			statusBar2 = new StatusBar () { Id = "statusBar2" };
			statusBar = new StatusBar () { Id = "statusBar" };
			w.Width = Dim.Fill (0);
			w.Height = Dim.Fill (0);
			w.X = 0;
			w.Y = 0;

			w.Visible = true;
			w.Modal = false;
			w.Title = "";
			statusBar.Width = Dim.Fill (0);
			statusBar.Height = 1;
			statusBar.X = 0;
			statusBar.Y = 0;
			statusBar.Visible = true;
			w.Add (statusBar);
			Assert.Equal (w.StatusBar, statusBar);

			statusBar2.Width = Dim.Fill (0);
			statusBar2.Height = 1;
			statusBar2.X = 0;
			statusBar2.Y = 4;
			statusBar2.Visible = true;
			w.Add (statusBar2);
			Assert.Equal (w.StatusBar, statusBar2);

			var menuBars = w.Subviews.OfType<StatusBar> ().ToArray ();
			Assert.Equal (2, menuBars.Length);

			Assert.Equal (Dim.Fill (0), menuBars [0].Width);
			Assert.Equal (Dim.Fill (0), menuBars [1].Width);

			// Goes wrong here
			w.Remove (statusBar);
			w.Remove (statusBar2);

			w.Add (statusBar);
			w.Add (statusBar2);

			// These assertions fail
			Assert.Equal (Dim.Fill (0), menuBars [0].Width);
			Assert.Equal (Dim.Fill (0), menuBars [1].Width);
		}

[BDisp]

I think that some of what is stated here has already been implemented, right?
Another solution that could help while the MenuBar is being built and dragged, is to create two methods for suspending (allows manipulating objects with new values ​​without performing any layout) and resuming (allows resetting manipulated objects with their original values ​​and summarizing the layout immediately ) the layout, as already discussed.

[tig]

I just added the RemoveAndThenAddStatusBar_ShouldNotChangeWidth you proposed above to #3533 and it passes.

The menu test is already in v2_develop. I think this can be closed once #3533 is done.