A view referencing a View that's not a peer (same superview) in Dim/Pos should not be allowed

[tig]

I was debugging tests and came across this test:

https://github.com/gui-cs/Terminal.Gui/blob/33c9a5fad44256ab1ade0d9ac478dab6af89b9b6/UnitTests/Types/DimTests.cs#LL541C3-L585C4

		public void DimCombine_Do_Not_Throws ()
		{
			Application.Init (new FakeDriver ());


			var t = Application.Top;


			var w = new Window ("w") {
				Width = Dim.Width (t) - 2,
				Height = Dim.Height (t) - 2
			};
			var f = new FrameView ("f");
			var v1 = new View ("v1") {
				Width = Dim.Width (w) - 2,
				Height = Dim.Height (w) - 2
			};
			var v2 = new View ("v2") {
				Width = Dim.Width (v1) - 2,
				Height = Dim.Height (v1) - 2
			};


			f.Add (v1, v2);
			w.Add (f);
			t.Add (w);


			f.Width = Dim.Width (t) - Dim.Width (v2);
			f.Height = Dim.Height (t) - Dim.Height (v2);


			t.Ready += (s, e) => {
				Assert.Equal (80, t.Frame.Width);
				Assert.Equal (25, t.Frame.Height);
				Assert.Equal (78, w.Frame.Width);
				Assert.Equal (23, w.Frame.Height);
				Assert.Equal (6, f.Frame.Width);
				Assert.Equal (6, f.Frame.Height);
				Assert.Equal (76, v1.Frame.Width);
				Assert.Equal (21, v1.Frame.Height);
				Assert.Equal (74, v2.Frame.Width);
				Assert.Equal (19, v2.Frame.Height);
			};


			Application.Iteration += () => Application.RequestStop ();


			Application.Run ();
			Application.Shutdown ();
		}

I have decided this test is totally bogus because views are referencing superviews.

This code would guard against this:

		internal void CollectPos (Pos pos, View from, ref HashSet<View> nNodes, ref HashSet<(View, View)> nEdges)
		{
			switch (pos) {
			case Pos.PosView pv:
				if (!from.InternalSubviews.Contains (pv.Target)) {
					throw new InvalidOperationException ($"View {pv.Target} is not a subview of {from}");
				}
				if (pv.Target != this) {
					nEdges.Add ((pv.Target, from));
				}
				foreach (var v in from.InternalSubviews) {
					CollectAll (v, ref nNodes, ref nEdges);
				}
				return;
			case Pos.PosCombine pc:
				foreach (var v in from.InternalSubviews) {
					CollectPos (pc.left, from, ref nNodes, ref nEdges);
					CollectPos (pc.right, from, ref nNodes, ref nEdges);
				}
				break;
			}
		}

		internal void CollectDim (Dim dim, View from, ref HashSet<View> nNodes, ref HashSet<(View, View)> nEdges)
		{
			switch (dim) {
			case Dim.DimView dv:
				if (!from.InternalSubviews.Contains (dv.Target)) {
					throw new InvalidOperationException ($"View {dv.Target} is not a subview of {from}");
				}
				if (dv.Target != this) {
					nEdges.Add ((dv.Target, from));
				}
				foreach (var v in from.InternalSubviews) {
					CollectAll (v, ref nNodes, ref nEdges);
				}
				return;
			case Dim.DimCombine dc:
				foreach (var v in from.InternalSubviews) {
					CollectDim (dc.left, from, ref nNodes, ref nEdges);
					CollectDim (dc.right, from, ref nNodes, ref nEdges);
				}
				break;
			}
		}

And this test would verify:

		[Fact]
		public void Dim_Referencing_SuperView_Throws ()
		{
			var super = new View ("super") {
				Width = 10,
				Height = 10
			};
			var view = new View ("view") {
				Width = Dim.Width (super),	// this is not allowed
				Height = Dim.Height (super),    // this is not allowed
			};

			super.Add (view);
			super.BeginInit ();
			super.EndInit ();
			Assert.Throws<InvalidOperationException> (() => super.LayoutSubviews ());
		}

This breaks a lot of existing code (e.g. TileView does this internally). So I'm not sure I'm correct.

Please debate ;-).

Thanks.

[BDisp]

The test is very confuse and I purpose did it. If you look better all them belong to the same hierarchy (t, w, f, v1 and v2). For this to work I did changes to the TopologicalSort to not throw exception if they belong to the same hierarchy and in the LayoutSubviews to process them later, but you didn't take them on account.

[tig]

The test is very confuse and I purpose did it. If you look better all them belong to the same hierarchy (t, w, f, v1 and v2). For this to work I did changes to the TopologicalSort to not throw exception if they belong to the same hierarchy and in the LayoutSubviews to process them later, but you didn't take them on account.

Ahhh.... Now it makes much more sense to me.

Do you think it would be a good idea to NOT allow these funky scenarios?

[BDisp]

Ok I agree to avoid steaming heads :-)
I only demonstrated that is possible :-)

[tig]

Separate from the work I'm doing now, I'm going to implement this and see how much actually breaks.

I'm guessing I may find some use cases where referencing a superview may be very useful... Or not.

[BDisp]

Well for the TileView it help :-)

[tig]

1. I think it's possible (but not 100% sure) for TileView to be refactored to not do what it's doing.
2. I still believe my new View design will completely negate the need for a TileView class. 💪

[BDisp]

On layout, a inner subview can reference a outer sujperview, but a superview never can reference a subview. See #2504.

[tig]

On layout, a inner subview can reference a outer sujperview, but a superview never can reference a subview. See #2504.

Can you give an example of a useful scenario where a subview references it's superview (or superview's superview)? I tried to think of one but couldn't.

[BDisp]

Can you give an example of a useful scenario where a subview references it's superview (or superview's superview)? I tried to think of one but couldn't.

Sure, here is a small example:

			Application.Init ();

			var top = Application.Top;
			var win1 = new Window ("win1");
			var win2 = new Window (new Rect (0, 0, 60, 20), "win2");
			var win3 = new Window (new Rect (0, 0, 30, 10), "win3");
			var leftLabel = new Label ("move to right") { X = Pos.Left (win2) };
			var rightLabel = new Label ("move to left") { X = 90 - 14 - Pos.Right (win2), Y = Pos.AnchorEnd (1) };
			win3.Add (leftLabel, rightLabel);
			win2.Add (win3);
			win1.Add (win2);
			top.Add (win1);

			Application.Run ();
			Application.Shutdown ();

Maybe we can leverage this for scrolling, what do think?

@tig see the dragging is exceeds the right side and the bottom side of the superview. Also when I start the drag by pressing the mouse the frame move down one line and the mouse isn't aligned at the top.

[BDisp]

@tig the above only works with the pr #2504.

[BDisp]

@tig the numeration is broke again. We need to create a unit test for this doesn't happens again.

[BDisp]

@tig the numeration is broke again. We need to create a unit test for this doesn't happens again.

The reason is you removed the code which pass the containerBounds parameter to the TextFormatter.Draw. You are passing the scroll view width and height, instead of passing the scroll view container size.

[tig]

var leftLabel = new Label ("move to right") { X = Pos.Left (win2) };

This effect is cool, but I'm not sure why it's useful.

I'm pushing on this because I want us to simplify the Terminal.Gui for developers and my intuition (based on my experience, and the fact that I'm not a very smart programmers) is that Pos/Dim are powerful-yet-confusing. By making the rule "Pos/Dim can only reference peer-Views" we can make it easier to understand.

If some developer comes along later with a REAL and useful scenario that is not possible with this limitation, we can back it out. In the meantime, developers will make fewer mistakes.

I'm COMPLETELY willing to have my mind changed on this. I have a feeling you have a real scenario in mind, but you've just not been able to articulate it yet. So please do.

Note this is not something we need to decide right now! We can let it stew ;-).

[tig]

@tig the numeration is broke again. We need to create a unit test for this doesn't happens again.

The reason is you removed the code which pass the containerBounds parameter to the TextFormatter.Draw. You are passing the scroll view width and height, instead of passing the scroll view container size.

Right. The documentation for TextFormatter is confusing. We need to fix that because I've made this mistake before.

[BDisp]

@tig the scrolling issue wasn't related with I said before, sorry. The main cause was the PosView and DimView was collection more than the necessary, by processing all the subviews on the CollectPos and CollectDim, respectively.

[BDisp]

@tig with the last fix in the #2504, the rightLabel now have the right Pos.AnchorEnd (1) instead of Pos.AnchorEnd (13).

[tig]

I'm going to close this issue. I think @BDisp shows an interesting use-case.