An exception should be thrown if `SetRelativeLayout` detects a `Pos.View` with `Target == this.Superview`

[BDisp]

Describe the bug

Running this unit test will fail because the ContentBottomRightCorner isn't draw.

	[Fact, AutoInitShutdown]
	public void ContentBottomRightCorner_Draw ()
	{
		var size = new Size { Width = 20, Height = 10};
		var sv = new ScrollView {
			X = 1,
			Y = 1,
			Width = 10,
			Height = 5,
			ContentSize = size
		};
		string text = null;
		for (int i = 0; i < size.Height; i++) {
			text += "*".Repeat (size.Width);
			if (i < size.Height) {
				text += '\n';
			}
		}
		var view = new View { Width = size.Width, Height = size.Height, ColorScheme = Colors.ColorSchemes ["Error"], AutoSize = true, Text = text };
		sv.Add (view);
		Application.Top.Add (sv);
		Application.Begin (Application.Top);

		TestHelpers.AssertDriverContentsWithFrameAre (@"
 *********▲
 *********┬
 *********┴
 *********▼
 ◄├──┤░░░► ", _output);

		var attrs = new Attribute [] { Colors.ColorSchemes ["Toplevel"].Normal, Colors.ColorSchemes ["Toplevel"].Focus, Colors.ColorSchemes ["Error"].Normal };

		TestHelpers.AssertDriverAttributesAre (@"
000000000000
022222222210
022222222210
022222222210
022222222210
011111111110
000000000000", null, attrs);

	}

Changing the code in the ScrollBarView as following the test pass and I will fix in the #3181.

			if (_hosted) {
				Host.SuperView.Add (_contentBottomRightCorner);
				_contentBottomRightCorner.X = Pos.Right (Host) - 1;
				_contentBottomRightCorner.Y = Pos.Bottom (Host) - 1;
			} else {
				Host.Add (_contentBottomRightCorner);
				_contentBottomRightCorner.X = Pos.AnchorEnd (1);
				_contentBottomRightCorner.Y = Pos.AnchorEnd (1);
			}

-	void ContentBottomRightCorner_DrawContent (object sender, DrawEventArgs e) => Driver.SetAttribute (Host.HasFocus ? ColorScheme.Focus : GetNormalColor ());
+	void ContentBottomRightCorner_DrawContent (object sender, DrawEventArgs e)
+	{
+		Driver.SetAttribute (Host.HasFocus ? ColorScheme.Focus : GetNormalColor ());
+		// I'm forced to do this here because the Clear method is
+		// changing the color attribute and is different of this one
+		Driver.FillRect (Driver.Clip);
+		e.Cancel = true;
+	}

What surprises me is that Pos.Right(Host) - 1 should be the same as Pos.AnchorEnd(1). I also tried it with just Pos.Right (Host) without success. I noticed that the ClipToBounds method sets the Driver.Clip to the correct location but with an empty size (0,0). I could be wrong but something is strange here, perhaps with the call Driver.Clip = Rect.Intersect (previous, BoundsToScreen (Bounds));. In the v1 this was working. https://github.com/gui-cs/Terminal.Gui/blob/fcad17853e78bc537650c40e8e273462a963ca0a/Terminal.Gui/Views/ScrollBarView.cs#L114-L115 I would appreciate your opinion on this please.

[tig]

I'm confused. The test just tests ScrollBar. ScrollBar has no dependencies (that I'm aware of) on ScrollBarView. How can changing code in ScrollBarView make a test that never references ScrollBarView pass/fail?

I suggest you focus exclusively on ScrollBar and ignore ScrollBarView (unless I misunderstood you).

[BDisp]

The test use a ScrollView which also use ScrollBarView. I didn't understanded about the ScrollBar.

[tig]

The test use a ScrollView which also use ScrollBarView. I didn't understanded about the ScrollBar.

My bad. I got ScrollBarView and ScrollView backwards. Nevermind.

Ignore me for now. What i typed before if you saw it was incorrect. I'm going to slow down and dive deep.

[BDisp]

Perhaps it should be but I don't think it is today. There's no code in SetRelativeLayout that factors in the fact that Host may have Adornments.

Right?

I'm not sure. It is not a problem with SetRelativeLayout because it returns the correct dimension (1,1). The BoundsToScreen (Bounds) method is also returning the right location and the dimension remains as (1,1,). The Rect.Intersect method returns the same correct location and the empty dimension (0,0). Therefore, my question is, if the correct location is found in the superview area, then the Rect.Intersect method, which in turn calls the Rect.FromLTRB method, should return the dimension of (1,1). Am I mistaken?

[tig]

FWIW, here's a version of your test that does not depend on Application and is a bit simpler. It passes/fails just as the one you described.

	[Fact]
	[SetupFakeDriver]
	public void ContentBottomRightCorner_Draw ()
	{
		((FakeDriver)Application.Driver).SetBufferSize (30, 30);

		var top = new View {
			Width = 30,
			Height = 30,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Blue, Color.Yellow)
			}
		};

		var size = new Size { Width = 20, Height = 10 };
		var sv = new ScrollView {
			X = 1,
			Y = 1,
			Width = 10,
			Height = 5,
			ContentSize = size,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Red, Color.Green)
			}
		};

		var view = new View {
			Width = size.Width,
			Height = size.Height,
			ColorScheme = top.ColorScheme
		};
		sv.Add (view);

		sv.AutoHideScrollBars = false;
		sv.ShowVerticalScrollIndicator = true;
		sv.ShowHorizontalScrollIndicator = true;

		top.Add (sv);
		top.BeginInit ();
		top.EndInit ();

		top.LayoutSubviews ();

		top.Draw ();


		TestHelpers.AssertDriverContentsWithFrameAre (@"
          ▲
          ┬
          ┴
          ▼
 ◄├──┤░░░► ", _output);

		var attrs = new [] { Attribute.Default, new Attribute (Color.Red, Color.Green), new Attribute (Color.Blue, Color.Yellow),
		};

		TestHelpers.AssertDriverAttributesAre (@"
00000000000
02222222221
02222222221
02222222221
02222222221
01111111111", null, attrs);
	}

[tig]

I think the issue is related to the fact that Pos objects cannot reference a superview (or, in this case the superview's superview). Dim is ok. But Pos is not because Frame != Bounds.

We need a focused Pos unit test that proves/disproves this.

[BDisp]

This unit tests doesn't depended of the ScrollView and I think it's better to debug. When two rectangles touch each other, the last one that draws is the one that prevails when the latter is not a subview of the first, I think. But reducing one unit from right to left is still within the previous rectangle, that is, within the superview and should have a dimension of 1.

	[Fact]
	[SetupFakeDriver]
	public void AnchorEnd_Equal_Right ()
	{
		((FakeDriver)Application.Driver).SetBufferSize (30, 30);

		var top = new View {
			Width = 30,
			Height = 30,
			ColorScheme = new ColorScheme { Normal = Attribute.Default }
		};

		var super1 = new View {
			Id = "super1",
			X = 1,
			Y = 1,
			Width = 4,
			Height = 1,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Red, Color.Green)
			},
			Text = "123"
		};
		var view1 = new View {
			Id = "view1",
			X = Pos.AnchorEnd (1),
			Width = 1,
			Height = 1,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Blue, Color.Yellow)
			},
			Text = "4"
		};
		super1.Add (view1);
		var super2 = new View {
			Id = "super2",
			X = 1,
			Y = 2,
			Width = 4,
			Height = 1,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Red, Color.Green)
			},
			Text = "123"
		};
		var view2 = new View {
			Id = "view2",
			X = Pos.Right (super2) - 1,
			Width = 1,
			Height = 1,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Blue, Color.Yellow)
			},
			Text = "4"
		};
		super2.Add (view2);

		top.Add (super1, super2);
		top.BeginInit ();
		top.EndInit ();

		top.LayoutSubviews ();

		top.Draw ();

		TestHelpers.AssertDriverContentsWithFrameAre (@"
 1234
 1234", _output);

		var attrs = new [] {
			Attribute.Default, new Attribute (Color.Red, Color.Green),
			new Attribute (Color.Blue, Color.Yellow)
		};

		TestHelpers.AssertDriverAttributesAre (@"
000000
011120
011120
000000", null, attrs);
	}

[tig]

This unit tests doesn't depended of the ScrollView and I think it's better to debug. When two rectangles touch each other, the last one that draws is the one that prevails when the latter is not a subview of the first, I think. But reducing one unit from right to left is still within the previous rectangle, that is, within the superview and should have a dimension of 1.

	[Fact]
	[SetupFakeDriver]
	public void AnchorEnd_Equal_Right ()
	{
		((FakeDriver)Application.Driver).SetBufferSize (30, 30);

		var top = new View {
			Width = 30,
			Height = 30,
			ColorScheme = new ColorScheme { Normal = Attribute.Default }
		};

		var super1 = new View {
			Id = "super1",
			X = 1,
			Y = 1,
			Width = 4,
			Height = 1,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Red, Color.Green)
			},
			Text = "123"
		};
		var view1 = new View {
			Id = "view1",
			X = Pos.AnchorEnd (1),
			Width = 1,
			Height = 1,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Blue, Color.Yellow)
			},
			Text = "4"
		};
		super1.Add (view1);
		var super2 = new View {
			Id = "super2",
			X = 1,
			Y = 2,
			Width = 4,
			Height = 1,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Red, Color.Green)
			},
			Text = "123"
		};
		var view2 = new View {
			Id = "view2",
			X = Pos.Right (super2) - 1,
			Width = 1,
			Height = 1,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Blue, Color.Yellow)
			},
			Text = "4"
		};
		super2.Add (view2);

		top.Add (super1, super2);
		top.BeginInit ();
		top.EndInit ();

		top.LayoutSubviews ();

		top.Draw ();

		TestHelpers.AssertDriverContentsWithFrameAre (@"
 1234
 1234", _output);

		var attrs = new [] {
			Attribute.Default, new Attribute (Color.Red, Color.Green),
			new Attribute (Color.Blue, Color.Yellow)
		};

		TestHelpers.AssertDriverAttributesAre (@"
000000
011120
011120
000000", null, attrs);
	}

The issue should be reproducible using just three views (maybe just two). You are way over complicating this by using colors etc...

Let's think about this a bit.

What does view.X = Pos.Right (superView) - 1 mean?

• Have the X Pos of view to track the right side of the superView minus 1 column.
• The "right side" is defined as superView.Frame.Right which is superView.Frame.X + superView.Frame.Width. - The -1 is odd because if superView.Frame.X = 0 and superView.Frame.Width = 1 then superView.Frame.X + superView.Frame.Width = 1, so subtracting 1 would give 0, which is NOT the right side of the superView.

In the face of Adornments, given there's no existing code (it would be in SetRelativeLayout) that deals with GetAdornmentsThickness using Pos.Right (superView) (with a - 1 or not) makes no sense.

But, I'm still not sure where the bug is here, because if my thinking above is correct, this would work, but it doesn't:

var view2 = new View {
			Id = "view2",
			X = Pos.Right (super2),
			Width = 1,
			Height = 1,
			ColorScheme = new ColorScheme () {
				Normal = new Attribute (Color.Blue, Color.Yellow)
			},
			Text = "4"
		};
		super2.Add (view2);

[tig]

Here's a set of minimal tests:

	[Theory]
	[SetupFakeDriver]
	[InlineData (0, 0)]
	[InlineData (1, 1)]
	public void Pos_AnchorEnd_1_Offset_Super (int offsetX, int offsetY)
	{
		var super = new View {
			Id = "super",
			X = offsetX,
			Y = offsetY,
			Width = 2,
			Height = 2,
		};
		var view = new View {
			Id = "subView",
			X = Pos.AnchorEnd (1), // Pos.Right(super) - 1,
			Width = 1,
			Height = 1,
		};
		super.Add (view);

		super.BeginInit ();
		super.EndInit ();

		Assert.Equal (new Rect (offsetX, offsetY, 2, 2), super.Frame);
		Assert.Equal (new Rect (1, 0, 1, 1), view.Frame);
	}

	[Theory]
	[SetupFakeDriver]
	[InlineData (0, 0)]
	[InlineData (1, 1)]
	public void Pos_Right_Minus_1_Offset_Super (int offsetX, int offsetY)
	{
		var super = new View {
			Id = "super",
			X = offsetX,
			Y = offsetY,
			Width = 2,
			Height = 2,
		};
		var view = new View {
			Id = "subView",
			X = Pos.Right (super) - 1,
			Width = 1,
			Height = 1,
		};
		super.Add (view);

		super.BeginInit ();
		super.EndInit ();

		Assert.Equal (new Rect (offsetX, offsetY, 2, 2), super.Frame);
		Assert.Equal (new Rect (1, 0, 1, 1), view.Frame);
	}

[tig]

• super.Frame = {(1,1,2,2)}
• super.Frame.Right = 3 (X + Width/1 + 2 = 3)
• Thus view.X, which is Pos.Right - 1 is PosView.Anchor(2) - 1
• PosView.Anchor(2) is therefore 3. Thus PosView.Anchor(2) - 1 is 2, which is the Actual above.

There's no way for a Pos object to know that the Target view is a Superview. Thus:

• Pos.View does not support Target being Superview.

It is unclear why this worked in v1. But it will not work in v2.

[dodexahedron]

The new title is concerning.

Property getters are typically not supposed to throw exceptions unless there is a really really good reason for doing so.

From the property design guidelines:

❌ AVOID throwing exceptions from property getters. Property getters should be simple operations and should not have any preconditions. If a getter can throw an exception, it should probably be redesigned to be a method. Notice that this rule does not apply to indexers, where we do expect exceptions as a result of validating the arguments.

[tig]

The new title is concerning.

Property getters are typically not supposed to throw exceptions unless there is a really really good reason for doing so.

From the property design guidelines:

❌ AVOID throwing exceptions from property getters. Property getters should be simple operations and should not have any preconditions. If a getter can throw an exception, it should probably be redesigned to be a method. Notice that this rule does not apply to indexers, where we do expect exceptions as a result of validating the arguments.

I didn't say we'd implement the exception throw on a property-getter. But, fixed the title anyway.

[tig]

I implemented a POC:

case Pos.PosView posView:
	if (posView.Target == SuperView) {
		throw new InvalidOperationException ();
	}
	newLocation = pos?.Anchor (superviewDimension) ?? 0;
	newDimension = Math.Max (GetNewDimension (dim, newLocation, superviewDimension, autosizeDimension), 0);
	break;

The good news is the only library code that appears to violate this is ScrollBarView. The bad news is a BUNCH of unit tests violate it. And some Scenarios.

That code all function correctly because, by accident, the SuperViews in question have .X == .Y == 0.

[dodexahedron]

The new title is concerning. Property getters are typically not supposed to throw exceptions unless there is a really really good reason for doing so. From the property design guidelines:

❌ AVOID throwing exceptions from property getters. Property getters should be simple operations and should not have any preconditions. If a getter can throw an exception, it should probably be redesigned to be a method. Notice that this rule does not apply to indexers, where we do expect exceptions as a result of validating the arguments.

I didn't say we'd implement the exception throw on a property-getter. But, fixed the title anyway.

Whoops my bad. I assumed for both get/set.

[dodexahedron]

I implemented a POC:

case Pos.PosView posView:
	if (posView.Target == SuperView) {
		throw new InvalidOperationException ();
	}
	newLocation = pos?.Anchor (superviewDimension) ?? 0;
	newDimension = Math.Max (GetNewDimension (dim, newLocation, superviewDimension, autosizeDimension), 0);
	break;

The good news is the only library code that appears to violate this is ScrollBarView. The bad news is a BUNCH of unit tests violate it. And some Scenarios.

That code all function correctly because, by accident, the SuperViews in question have .X == .Y == 0.

Hm.

Maybe a good idea to tag those tests with a TODO or a [Trait] or whatever, if they are still otherwise valid tests, so they can be tackled when I make the pass over the unit tests?

[dodexahedron]

Unless, of course, you intend to fix it now or otherwise before that.

[tig]

Unless, of course, you intend to fix it now or otherwise before that.

They should be fixed as part of whatever PR is created to fix this Issue.

[BDisp]

They should be fixed as part of whatever PR is created to fix this Issue.

@ tig do you want I fix this on the #3181 with a commit referencing this issue?

[BDisp]

They should be fixed as part of whatever PR is created to fix this Issue.

@ tig do you want I fix this on the #3181 with a commit referencing this issue?

I already fixed the ScrollBarView in this commit https://github.com/BDisp/Terminal.Gui/commit/e95f50b3c544276d6ec0a540b017c9aa10f12fd1. It can be up to you to throw the exception if you don't mind, thank you.

[tig]

Let's do it separately. #3181 is already "epic" enough ;-)

[dodexahedron]

Let's do it separately. #3181 is already "epic" enough ;-)

Now you've given me flashbacks of dealing with Jira. 😨

[BDisp]

@tig do you have intentions of throwing the exception or this issue can be closed?

[tig]

Yes I do. Please leave it open.

[tig]

This is no longer relevant. Closing.