Simplify `LayoutStyle` - Make it a computed/read-only property ensure correctness #3127

tig · 2024-01-05T15:24:14Z

I've confused myself (again) around ComputedLayout.

I noticed this morning we throw an exception of Width or Height is absolute and ComputedLayout is set.

Does this actually make sense?

Did I do this?

Why?

Why can't someone force a view to a fixed size even when using ComputedLayout?

Maybe I just need more coffee?

BDisp · 2024-01-05T15:45:00Z

If you saw my comment #3019 (comment) you'll understand why. Absolute layout must be respected if frame is provided. When a view is added to another view the Pos/Dim properties must be set even to a view that has LayoutStyle.Absolute to automatically position with other views. Thus they don't to have run the SetRelativeLayout method but others views with LayoutStyle.Computed can access to his Pos/Dim to calculate the right location and dimension.
So LayoutStyte.Computed can have absolute location and dimension, the diference is in this case SetRelativeLayout can be called and with LayoutStyte.Absolute can't.

tig · 2024-01-05T17:02:33Z

If you saw my comment #3019 (comment) you'll understand why. Absolute layout must be respected if frame is provided. When a view is added to another view the Pos/Dim properties must be set even to a view that has LayoutStyle.Absolute to automatically position with other views. Thus they don't to have run the SetRelativeLayout method but others views with LayoutStyle.Computed can access to his Pos/Dim to calculate the right location and dimension. So LayoutStyte.Computed can have absolute location and dimension, the diference is in this case SetRelativeLayout can be called and with LayoutStyte.Absolute can't.

I'm having a really hard time with your English. It is NOT your fault, but I struggle to parse your writing sometimes and instead of digging in I just give up. This is my bad as I should be more patient.

I'm going to try to echo back the above as clearly as I can...

(Ignoring AutoSize use-cases)

`SetRelativeLayout`

You assert that if view.LayoutStyle == LayoutStyle.Absolute the SetRelativeLayout method should not be called. I disagree. it can and should work if view.LayoutStyle == LayoutStyle.Absolute as long as X, Y, Width, and Height match Frame and are Dim/PosAbsolute.

In the code right now, when Frame is set, we do NOT set these properties (even though the API docs say we do):

/// <summary>
/// Gets or sets location and size of the view. The frame is relative to the <see cref="SuperView"/>'s
/// <see cref="Bounds"/>.
/// </summary>
/// <value>
/// The rectangle describing the location and size of the view, in coordinates relative to the
/// <see cref="SuperView"/>.
/// </value>
/// <remarks>
///         <para>
///         Change the Frame when using the <see cref="LayoutStyle.Absolute"/> layout style to move or resize views.
///         </para>
///         <para>
///         Altering the Frame will change <see cref="LayoutStyle"/> to <see cref="LayoutStyle.Absolute"/>.
///         Additionally, <see cref="X"/>, <see cref="Y"/>, <see cref="Width"/>, and <see cref="Height"/> will be set
///         to the values of the Frame (using <see cref="Pos.PosAbsolute"/> and <see cref="Dim.DimAbsolute"/>).
///         </para>
///         <para>
///         Altering the Frame will eventually (when the view is next drawn) cause the
///         <see cref="LayoutSubview(View, Rect)"/>
///         and <see cref="OnDrawContent(Rect)"/> methods to be called.
///         </para>
/// </remarks>
public virtual Rect Frame {
	get => _frame;
	set {
		_frame = new Rect (value.X, value.Y, Math.Max (value.Width, 0), Math.Max (value.Height, 0));
		//X = _frame.X;
		//Y = _frame.Y;
		//Width = _frame.Width;
		//Height = _frame.Height;
		if (IsInitialized || LayoutStyle == LayoutStyle.Absolute) {
			LayoutFrames ();
			TextFormatter.Size = GetTextFormatterSizeNeededForTextAndHotKey ();
			SetNeedsLayout ();
			SetNeedsDisplay ();
		}
	}
}

I think we should uncomment that code, and change the unit tests as needed.

I also think we should change the X, Y, Height, and Width setters to NOT throw if view.LayoutStyle == LayoutStyle.Computed and they are set to Pos/DimAbsolute as there is nothing wrong with doing so.

If view.LayoutStyle == LayoutStyle.Absolute and X, Y, Height, or Width are set to NON-Absolute values, we should either throw an exception or force view.LayoutStyle to be LayoutStyle.Computed.

Do you agree?

tznind · 2024-01-05T17:14:05Z

Coul LayoutStyle be get only? I.e. Return x.isRelative or y.IsRelative or width.IsRelative etc?

…

On Fri, 5 Jan 2024, 17:02 Tig, ***@***.***> wrote: If you saw my comment #3019 (comment) <#3019 (comment)> you'll understand why. Absolute layout must be respected if frame is provided. When a view is added to another view the Pos/Dim properties must be set even to a view that has LayoutStyle.Absolute to automatically position with other views. Thus they don't to have run the SetRelativeLayout method but others views with LayoutStyle.Computed can access to his Pos/Dim to calculate the right location and dimension. So LayoutStyte.Computed can have absolute location and dimension, the diference is in this case SetRelativeLayout can be called and with LayoutStyte.Absolute can't. I'm having a really hard time with your English. It is NOT your fault, but I struggle to parse your writing sometimes and instead of digging in I just give up. This is my bad as I should be more patient. I'm going to try to echo back the above as clearly as I can... (Ignoring AutoSize use-cases) SetRelativeLayout You assert that if view.LayoutStyle == LayoutStyle.Absolute the SetRelativeLayout method should not be called. I disagree. it can and should work if view.LayoutStyle == LayoutStyle.Absolute *as long as* X, Y, Width, and Height match Frame and are Dim/PosAbsolute. In the code right now, when Frame is set, we do NOT set these properties (even though the API docs say we do): /// <summary>/// Gets or sets location and size of the view. The frame is relative to the <see cref="SuperView"/>'s/// <see cref="Bounds"/>./// </summary>/// <value>/// The rectangle describing the location and size of the view, in coordinates relative to the/// <see cref="SuperView"/>./// </value>/// <remarks>/// <para>/// Change the Frame when using the <see cref="LayoutStyle.Absolute"/> layout style to move or resize views./// </para>/// <para>/// Altering the Frame will change <see cref="LayoutStyle"/> to <see cref="LayoutStyle.Absolute"/>./// Additionally, <see cref="X"/>, <see cref="Y"/>, <see cref="Width"/>, and <see cref="Height"/> will be set/// to the values of the Frame (using <see cref="Pos.PosAbsolute"/> and <see cref="Dim.DimAbsolute"/>)./// </para>/// <para>/// Altering the Frame will eventually (when the view is next drawn) cause the/// <see cref="LayoutSubview(View, Rect)"/>/// and <see cref="OnDrawContent(Rect)"/> methods to be called./// </para>/// </remarks>public virtual Rect Frame { get => _frame; set { _frame = new Rect (value.X, value.Y, Math.Max (value.Width, 0), Math.Max (value.Height, 0)); //X = _frame.X; //Y = _frame.Y; //Width = _frame.Width; //Height = _frame.Height; if (IsInitialized || LayoutStyle == LayoutStyle.Absolute) { LayoutFrames (); TextFormatter.Size = GetTextFormatterSizeNeededForTextAndHotKey (); SetNeedsLayout (); SetNeedsDisplay (); } }} I think we should uncomment that code, and change the unit tests as needed. I also think we should change the X, Y, Height, and Width setters to NOT throw if view.LayoutStyle == LayoutStyle.Computed and they are set to Pos/DimAbsolute as there is nothing wrong with doing so. If view.LayoutStyle == LayoutStyle.Absolute and X, Y, Height, or Width are set to NON-Absolute values, we should either throw an exception or force view.LayoutStyle to be LayoutStyle.Computed. Do you agree? — Reply to this email directly, view it on GitHub <#3127 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHO3C5F6R5NOVWJV4PHYGE3YNAW35AVCNFSM6AAAAABBOSSLT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZYHE4DOOJZGQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

BDisp · 2024-01-05T17:43:49Z

I'm having a really hard time with your English. It is NOT your fault, but I struggle to parse your writing sometimes and instead of digging in I just give up. This is my bad as I should be more patient.

Your problem, not mine. I also have to be very patient when I expose my point of view in vain and much time later you sometimes end up to give me raison.

I'm going to try to echo back the above as clearly as I can...

(Ignoring AutoSize use-cases)

SetRelativeLayout

You assert that if view.LayoutStyle == LayoutStyle.Absolute the SetRelativeLayout method should not be called. I disagree. it can and should work if view.LayoutStyle == LayoutStyle.Absolute as long as X, Y, Width, and Height match Frame and are Dim/PosAbsolute.

I meant that if view.LayoutStyle == LayoutStyle.Absolute it always have a fixed frame, but of course we can't avoid to set the X, Y, Width, and Height (at OnAdded or at EndInit) to allow that other views can use his Pos/Dim (location/dimension).

In the code right now, when Frame is set, we do NOT set these properties (even though the API docs say we do):

I think there are tons of code like if (view.Frame.X == intValue) and others which probe that the Frame is used a lot.

I think we should uncomment that code, and change the unit tests as needed.

I also think we should change the X, Y, Height, and Width setters to NOT throw if view.LayoutStyle == LayoutStyle.Computed and they are set to Pos/DimAbsolute as there is nothing wrong with doing so.

I absolutely agree. I think that is an absurd.

If view.LayoutStyle == LayoutStyle.Absolute and X, Y, Height, or Width are set to NON-Absolute values, we should either throw an exception or force view.LayoutStyle to be LayoutStyle.Computed.

Do you agree?

Yes I agree. This isn't so greave as the opposite by force LayoutStyle.Computed to be LayoutStyle.Absolute which will depend of the ValidatePosDim property.

BDisp · 2024-01-05T17:50:21Z

Coul LayoutStyle be get only? I.e. Return x.isRelative or y.IsRelative or width.IsRelative etc?

I.e.

Return x.isRelative or y.IsRelative or width.IsRelative etc?

Well I think that can be possible. Remember the v2 is completely a new version and many though can be changed. I only saw your comment now and it could be acceptable, if @tig also agree. In this case we will rely on Pos/Dim which mustn't be null after it's initialized. But if a user set Pos/Dim to null? What the API should do?

tig · 2024-01-05T18:04:41Z

Coul LayoutStyle be get only? I.e. Return x.isRelative or y.IsRelative or width.IsRelative etc?

I actually tried that in #2467, but backed it out. Some of the code is still there commented out because I think it may be the right thing to do:

public LayoutStyle LayoutStyle {
	get => _layoutStyle;
	//if ((X == null || X is Pos.PosAbsolute) && (Y == null || Y is Pos.PosAbsolute) &&
	//(Width == null || Width is Dim.DimAbsolute) && (Height == null || Height is Dim.DimAbsolute)) {
	//	return LayoutStyle.Absolute;
	//} else {
	//	return LayoutStyle.Computed;
	//}
	set {
		_layoutStyle = value;
		//switch (_layoutStyle) {
		//case LayoutStyle.Absolute:
		//	X = Frame.X;
		//	Y = Frame.Y;
		//	Width = Frame.Width;
		//	Height = Frame.Height;
		//	break;

		//case LayoutStyle.Computed:
		//	X ??= Frame.X;
		//	Y ??= Frame.Y;
		//	Width ??= Frame.Width;
		//	Height ??= Frame.Height;
		//	break;
		//}
		SetNeedsLayout ();
	}
}

BDisp · 2024-01-05T18:10:24Z

I also think it's be better solution, using only the get. If the user set the Pos/Dim to null the SetRelativeLayout know what to do, which is using the Frame property.

tig · 2024-01-05T18:13:03Z

In this case we will rely on Pos/Dim which mustn't be null after it's initialized. But if a user set Pos/Dim to null? What the API should do?

I think you're asking "if X, Y, Width or Height is set to null, what should the API do?

I have considered (and I think I actually implemented but backed it out). Note `// BUGBUG:

public Pos X {
	get => VerifyIsInitialized (_x, nameof (X));
	set {
		// BUGBUG: null is the sames a Pos.Absolute(0). Should we be explicit and set it?
		
		if (ValidatePosDim && LayoutStyle == LayoutStyle.Computed) {
			CheckAbsolute (nameof (X), _x, value);
		}

		_x = value;

		OnResizeNeeded ();
	}
}

Proposed version:

public Pos X {
	get => VerifyIsInitialized (_x, nameof (X));
	set {
		if (ValidatePosDim && LayoutStyle == LayoutStyle.Computed) {
			CheckAbsolute (nameof (X), _x, value);
		}
		_x = value ?? Pos.At (0);
		OnResizeNeeded ();
	}
}

tig · 2024-01-05T18:14:52Z

I also think it's be better solution, using only the get. If the user set the Pos/Dim to null the SetRelativeLayout know what to do, which is using the Frame property.

Can you explain this further? My understanding is that if X = Pos.At (0) then SetRelativeLayout will just work as-is, regardless of whether LayoutStyle is Comptued or Absolute.

tig · 2024-01-05T18:16:56Z

public Pos X {
	get => VerifyIsInitialized (_x, nameof (X));
	set {
		if (ValidatePosDim && LayoutStyle == LayoutStyle.Computed) {
			CheckAbsolute (nameof (X), _x, value);
		}
		_x = value ?? Pos.At (0);
		OnResizeNeeded ();
	}
}

Actually, given all of this discussion the new version would be:

public Pos X {
	get => VerifyIsInitialized (_x, nameof (X));
	set {
		_x = value ?? Pos.At (0);
		OnResizeNeeded ();
	}
}

BDisp · 2024-01-05T18:19:29Z

I think we shouldn't set it as 0. The user may want them as null to force the app use the Frame property. The SetRelativeLayout always use the Frame value when some of the Pos/Dim is null. If the user want to set some of them to 0, then he must explicitly set =0, after it's initialized, of course.

tig · 2024-01-05T18:23:49Z

I meant that if view.LayoutStyle == LayoutStyle.Absolute it always have a fixed frame, but of course we can't avoid to set the X, Y, Width, and Height (at OnAdded or at EndInit) to allow that other views can use his Pos/Dim (location/dimension).

EndInit does not set these properties, so I'm not sure what you mean by that.

OnAdded:

/// <summary>
/// Method invoked when a subview is being added to this view.
/// </summary>
/// <param name="e">Event where <see cref="ViewEventArgs.View"/> is the subview being added.</param>
public virtual void OnAdded (SuperViewChangedEventArgs e)
{
	var view = e.Child;
	view.IsAdded = true;
	view.OnResizeNeeded ();
	view._x ??= view._frame.X;
	view._y ??= view._frame.Y;
	view._width ??= view._frame.Width;
	view._height ??= view._frame.Height;

	view.Added?.Invoke (this, e);
}

If we force X to never be null per the above, we can remove this code, right?

/// <summary>
/// Method invoked when a subview is being added to this view.
/// </summary>
/// <param name="e">Event where <see cref="ViewEventArgs.View"/> is the subview being added.</param>
public virtual void OnAdded (SuperViewChangedEventArgs e)
{
	var view = e.Child;
	view.IsAdded = true;
	view.OnResizeNeeded ();
	view.Added?.Invoke (this, e);
}

Can you expand on this: "allow that other views can use his Pos/Dim (location/dimension)."?

tig · 2024-01-05T18:26:26Z

I think we shouldn't set it as 0. The user may want them as null to force the app use the Frame property. The SetRelativeLayout always use the Frame value when some of the Pos/Dim is null. If the user want to set some of them to 0, then he must explicitly set =0, after it's initialized, of course.

X = Pos.At (0) is the same as X = 0.

I do not see where SetRelativeLayout checks for null. Am I not seeing something?

tig · 2024-01-05T18:32:05Z

The user may want them as null to force the app use the Frame property.

Can you explain why someone would want to do this? What is the use-case?

BDisp · 2024-01-05T18:32:34Z

X = Pos.At (0) is the same as X = 0.

I know. What I meant it shouldn't set to 0 anyway.

I do not see where SetRelativeLayout checks for null. Am I not seeing something?

Here

tig · 2024-01-05T18:36:33Z

X = Pos.At (0) is the same as X = 0.

I know. What I meant it shouldn't set to 0 anyway.

I do not see where SetRelativeLayout checks for null. Am I not seeing something?

Here

Ahhh. Right. Note the comment though: dim == null is the same as dim == Dim.FIll (0)

If we forced Height and Width to be Dim.Fill (0) if ever set to null, then that case statement could be removed and the case Dim.DimFill would just do the right thing.

Likewise for the Pos case (which is already coded such that PosAbsoulte and null are the same:

BDisp · 2024-01-05T18:36:58Z

The user may want them as null to force the app use the Frame property.

Can you explain why someone would want to do this? What is the use-case?

On the UICatalog:

tig · 2024-01-05T18:48:54Z

The user may want them as null to force the app use the Frame property.

Can you explain why someone would want to do this? What is the use-case?

On the UICatalog:

A better search:

etc...

If we made the change, all of these would be needed to be cleaned up, but nothing works differently.

E.g. this code in ComboBox

Is already testing for both (Height == null) and (Height is Dim.DimAbsolute)...

BDisp · 2024-01-05T18:50:55Z

Ahhh. Right. Note the comment though: dim == null is the same as dim == Dim.FIll (0)

Before were considered the Frame property when Pos/Dim were null. But ok I'll not talked more about this.

If we forced Height and Width to be Dim.Fill (0) if ever set to null, then that case statement could be removed and the case Dim.DimFill would just do the right thing.

If forcing is to be used then is better using the Dim.Auto.

Likewise for the Pos case (which is already coded such that PosAbsoulte and null are the same:

Using your idea, yes.

Another thinking I had is the LayoutStyle.get only returns Layout.Absolute if all the Pos/Dim properties are null. In your mind the LayoutStyle.get should return Layout.Absolute if all the Pos/Dim properties are Pos/Dim Absolute?

BDisp · 2024-01-05T18:58:25Z

EndInit does not set these properties, so I'm not sure what you mean by that.

No needed. See bellow.

If we force X to never be null per the above, we can remove this code, right?

Yes and the Pos/Dim properties should return 0 from the getter if they are null.

Can you expand on this: "allow that other views can use his Pos/Dim (location/dimension)."?

This has no sense now since the SetRelativeLayout will always be called, no matter the LayoutStyle used.

BDisp · 2024-01-05T19:01:25Z

None of those SET Frame, except in ASCIICustomButton.cs. I don't understand why it's doing that, but I suspect there is a cleaner way for it to be doing what it's doing.

Also, none set them to null

But I never said the Frame is set to null but only the Pos/Dim properties.

tig · 2024-01-05T19:09:28Z

A PR for this stuff...

Fixes #3127 - View Layout improvements - Redefines how LayoutStyle works. Fixes AutoSize etc... #3130

So far i just fixed the CheckAbsoulte issue.

tig · 2024-01-05T19:13:39Z

Another thinking I had is the LayoutStyle.get only returns Layout.Absolute if all the Pos/Dim properties are null. In your mind the LayoutStyle.get should return Layout.Absolute if all the Pos/Dim properties are Pos/Dim Absolute?

I think so. Like this:

public LayoutStyle LayoutStyle {
	get {
		if (X is Pos.PosAbsolute && Y is Pos.PosAbsolute && Width is Dim.DimAbsolute && Height is Dim.DimAbsolute) {
			return LayoutStyle.Absolute;
		} else {
			return LayoutStyle.Computed;
		}
	}
	set {
		X = Frame.X;
		Y = Frame.Y;
		Width = Frame.Width;
		Height = Frame.Height;
		SetNeedsLayout ();
	}
}

BDisp · 2024-01-05T19:21:42Z

I think so. Like this:

public LayoutStyle LayoutStyle {
	get {
		if (X is Pos.PosAbsolute && Y is Pos.PosAbsolute && Width is Dim.DimAbsolute && Height is Dim.DimAbsolute) {
			return LayoutStyle.Absolute;
		} else {
			return LayoutStyle.Computed;
		}
	}
	set {
		X = Frame.X;
		Y = Frame.Y;
		Width = Frame.Width;
		Height = Frame.Height;
		SetNeedsLayout ();
	}
}

I don't understands the needed of the LayoutStyle.set to set all the Pos/Dim. Imagine they have other values not Absolute, then they will be overridden with Absolute values. It's bad.

BDisp · 2024-01-05T19:34:06Z

	_x ??= _frame.X;
	_y ??= _frame.Y;
	_width ??= _frame.Width;
	_height ??= _frame.Height;

This code can also be used at BeginInit or EndInit for the views that weren't been added, so Superview == null.

tig · 2024-01-05T19:36:31Z

How's this?

[Fact]
[TestRespondersDisposed]
public void AbsoluteLayout_Constructor ()
{
	var frame = Rect.Empty;
	var v = new View (frame);
	Assert.True (v.LayoutStyle == LayoutStyle.Absolute);
	Assert.Equal (frame,                                      v.Frame);
	Assert.Equal (new Rect (0, 0, frame.Width, frame.Height), v.Bounds); // With Absolute Bounds *is* deterministic before Layout
	Assert.Equal (Pos.At (0),                                 v.X);
	Assert.Equal (Pos.At (0),                                 v.Y);
	Assert.Equal (Dim.Sized (0),                              v.Width);
	Assert.Equal (Dim.Sized (0),                              v.Height);
	v.Dispose ();

	frame = new Rect (1, 2, 3, 4);
	v = new View (frame);
	Assert.True (v.LayoutStyle == LayoutStyle.Absolute);
	Assert.Equal (frame,                                      v.Frame);
	Assert.Equal (new Rect (0, 0, frame.Width, frame.Height), v.Bounds); // With Absolute Bounds *is* deterministic before Layout
	Assert.Equal (Pos.At (1),                                 v.X);
	Assert.Equal (Pos.At (2),                                 v.Y);
	Assert.Equal (Dim.Sized (3),                              v.Width);
	Assert.Equal (Dim.Sized (4),                              v.Height);
	v.Dispose ();

	v = new View (frame, "v");
	Assert.True (v.LayoutStyle == LayoutStyle.Absolute);
	Assert.Equal (frame,                                      v.Frame);
	Assert.Equal (new Rect (0, 0, frame.Width, frame.Height), v.Bounds); // With Absolute Bounds *is* deterministic before Layout
	Assert.Equal (Pos.At (1),                                 v.X);
	Assert.Equal (Pos.At (2),                                 v.Y);
	Assert.Equal (Dim.Sized (3),                              v.Width);
	Assert.Equal (Dim.Sized (4),                              v.Height);
	v.Dispose ();

	v = new View (frame.X, frame.Y, "v");
	Assert.True (v.LayoutStyle == LayoutStyle.Absolute);
	// BUGBUG: v2 - I think the default size should be 0,0 not 1,1
	Assert.Equal (new Rect (frame.X, frame.Y, 1, 1), v.Frame);
	Assert.Equal (new Rect (0,       0,       1, 1), v.Bounds); // With Absolute Bounds *is* deterministic before Layout
	Assert.Equal (Pos.At (1),                        v.X);
	Assert.Equal (Pos.At (2),                        v.Y);
	Assert.Equal (Dim.Sized (1),                     v.Width);
	Assert.Equal (Dim.Sized (1),                     v.Height);
	v.Dispose ();

}

This seems correct to me. You?

Related note // BUGBUG: v2 - I think the default size should be 0,0 not 1,1 - what do y'all think of that?

tig · 2024-01-05T19:39:45Z

I think so. Like this:

public LayoutStyle LayoutStyle {
	get {
		if (X is Pos.PosAbsolute && Y is Pos.PosAbsolute && Width is Dim.DimAbsolute && Height is Dim.DimAbsolute) {
			return LayoutStyle.Absolute;
		} else {
			return LayoutStyle.Computed;
		}
	}
	set {
		X = Frame.X;
		Y = Frame.Y;
		Width = Frame.Width;
		Height = Frame.Height;
		SetNeedsLayout ();
	}
}

I don't understands the needed of the LayoutStyle.set to set all the Pos/Dim. Imagine they have other values not Absolute, then they will be overridden with Absolute values. It's bad.

Good point. If value == LayoutStyle.Computed we should not change them as I do, for sure.

But if value == LayoutStyle.Absolute? By defn, if X == Pos.Center() and the layout style is changed to Absolute, then X is no longer valid. What should we do in this case?

a) Ignore it.
b) Change _x to Frame.X.
c) throw an exception

BDisp · 2024-01-05T19:51:06Z

This seems correct to me. You?

Yes it's correct. Also I'll have more headaches to me resolve more merge conflicts.

Related note // BUGBUG: v2 - I think the default size should be 0,0 not 1,1 - what do y'all think of that?

Well as you know that is the text auto size "v" in the View constructor. Is discussable if that must be the default. Let's hear the @tznind as well about his opinion about this.

BDisp · 2024-01-05T19:58:57Z

Good point. If value == LayoutStyle.Computed we should not change them as I do, for sure.

But if value == LayoutStyle.Absolute? By defn, if X == Pos.Center() and the layout style is changed to Absolute, then X is no longer valid. What should we do in this case?

Only handle this on their own properties only when the user change them. The point is I'm not understands why you have a setter on the LayoutStyle property. Remember that was proposed only a read only LayoutStyle property.

a) Ignore it.
b) Change _x to Frame.X.
c) throw an exception

If LayoutStyle property is read only none of this make sense to me. Am I missing something?

BDisp · 2024-01-05T21:18:39Z

Coul LayoutStyle be get only? I.e. Return x.isRelative or y.IsRelative or width.IsRelative etc?

@tznind LayoutStyle be get only, I understood but the X.IsRelative, Y.IsRelative, Width.IsRelative and Height.IsRelative make me confusion because they seem as Pos/Dim properties and not as LayoutStyle enum. Do you mean to change LayoutStyle enum as well?

tig · 2024-01-05T22:24:40Z

The point is I'm not understands why you have a setter on the LayoutStyle property. Remember that was proposed only a read only LayoutStyle property.

I will make it readonly. I'm leaving the getter for now so compiling still works. For now it's:

	public LayoutStyle LayoutStyle {
		get {
			if (_x is Pos.PosAbsolute && _y is Pos.PosAbsolute && _width is Dim.DimAbsolute && _height is Dim.DimAbsolute) {
				return LayoutStyle.Absolute;
			} else {
				return LayoutStyle.Computed;
			}
		}
		set {
			throw new InvalidOperationException ("LayoutStyle is read-only.");
		}
	}

BDisp · 2024-01-05T22:30:35Z

I will make it readonly. I'm leaving the getter for now so compiling still works. For now it's:

That the way I think i should be, but I'm curious about the @tznind comment 😃

tig · 2024-01-05T22:32:11Z

I've decided we should throw an exception of someone tries to set X, Y, Width, and Height to null.

This simplifies a ton and I really don't like setters that do automatic stuff.

	public Pos Y {
		get => VerifyIsInitialized (_y, nameof (Y));
		set {
			_y = value ?? throw new ArgumentNullException (nameof (value), !@$"{nameof (Y)} cannot be null");
			OnResizeNeeded ();
		}
	}

tznind · 2024-01-05T23:39:42Z

Coul LayoutStyle be get only? I.e. Return x.isRelative or y.IsRelative or width.IsRelative etc?

@tznind LayoutStyle be get only, I understood

That was really the only bit I was trying to get across

but the X.IsRelative, Y.IsRelative, Width.IsRelative and Height.IsRelative make me confusion because they seem as Pos/Dim properties and not as LayoutStyle enum. Do you mean to change LayoutStyle enum as well?

I guess I was thinking that each Pos/Dim is either absolute or relative. So a view will also have its layout be relative if any of its dimensions is relative.

I also think that we should not allow null pos/dim or treat them as 0. To me they that just makes the most sense for a view.

BDisp · 2024-01-05T23:46:00Z

I guess I was thinking that each Pos/Dim is either absolute or relative. So a view will also have its layout be relative if any of its dimensions is relative.

But the Pos/Dim already define what is absolute or relative. PosAbsolute and DimAbsolute are absolute and PosView and DimView or other than PosAbsolute or DimAbsolute are relative. Do you really think that is needed?

I also think that we should not allow null pos/dim or treat them as 0. To me they that just makes the most sense for a view.

I agree.

…works. Fixes `AutoSize` etc... (#3130) * Removes CheckAbsoulte and updates unit tests to match * Fixed code that was dependent on ToString behavior vs. direct test for null * Dim/Pos != null WIP * Moved AutoSize specific tests out of Pos/Dim tests * Broke out AutoSize = false tests to new file * Commented test TODOs * New test * Removed unused API and cleaned up code * Removed unused API and cleaned up code * Cleaned up code * Cleaned up code * reorg'd Toplevel tests * Fixed Create and related unit tests * Added test from #3136 * Removed TopLevel.Create * Fixed SetCurrentOverlappedAsTop * Updated pull request template * Updated pull request template * Revert "Updated pull request template" This reverts commit d807190. * reverting * re-reverting * Fixed every thing but autosize scenarios?? * Fixed hexview * Fixed contextmenu * Fixed more minor issues in tests * Fixed more minor issues in tests * Debugging Dialog test failure * Fixed bad Dialog test. Was cleary invalid * Fixed OnResizeNeeded bug * Fixed OnResizeNeeded bug * Fixed UICatalog to not eat exceptions * Fixed TextView * Removed Frame overrides * Made Frame non-virtual * Fixed radioGroup * Fixed TabView * Hcked ScrolLBarView unit tests to pass * All AutoSize tests pass! * All tests pass!!!!!!! * Updated API docs. Cleaned up code. * Fixed ColorPicker * Added 'Bounds =' unit tests * Refactored TextFormatter.Size setting logic * Cleaned up OnResizeNeeded (api docs and usages) * Merges in #3019 changes. Makes OnResizeNeeded non-virtual. If we find a use-case where someone wants to override it we can change this back. * Fixed FileDialog bounds warning * Removed resharper settings from editorconfig * Added Pos.Center test to AllViewsTests.cs. Modernized RadioGroup. Fixed ProgressBar. * Reverted formatting * Reverted formatting * Reverted formatting * Reverted formatting * Reverted formatting * Reverted formatting * Reverted formatting * Code cleanup * Code cleanup * Code cleanup * Code cleanup * Code cleanup * Code cleanup * Code cleanup * Code cleanup * Reverted formatting

tig mentioned this issue Jan 5, 2024

Fixes #3127 - View Layout improvements - Redefines how LayoutStyle works. Fixes AutoSize etc... #3130

Merged

23 tasks

tig mentioned this issue Jan 7, 2024

Toplevel is confused about dimension - Should use Computed and Dim.Fill #3135

Closed

5 tasks

tig added bug design Issues regarding Terminal.Gui design (bugs, guidelines, debates, etc...) labels Jan 8, 2024

tig added this to Terminal.Gui V2 Beta Jan 8, 2024

tig mentioned this issue Jan 8, 2024

Fixes #3017. View TextDirection returns incorrect size on a vertical direction instance with AutoSize as false. #3019

Closed

8 tasks

tig changed the title ~~View.CheckAbsolute?~~ LayoutStyle is complex, confusing, and not really "Correct". E.g. View.CheckAbsolute. Jan 11, 2024

tig changed the title ~~LayoutStyle is complex, confusing, and not really "Correct". E.g. View.CheckAbsolute.~~ Simplify LayoutStyle - Make it a computed/read-only property and simplify Jan 11, 2024

tig changed the title ~~Simplify LayoutStyle - Make it a computed/read-only property and simplify~~ Simplify LayoutStyle - Make it a computed/read-only property ensure correctness Jan 11, 2024

tig closed this as completed Jan 14, 2024

github-project-automation bot moved this to ✅ Done in Terminal.Gui V2 Beta Jan 14, 2024

Simplify LayoutStyle - Make it a computed/read-only property ensure correctness #3127

Simplify LayoutStyle - Make it a computed/read-only property ensure correctness #3127

Comments

tig commented Jan 5, 2024

BDisp commented Jan 5, 2024 • edited Loading

tig commented Jan 5, 2024

SetRelativeLayout

tznind commented Jan 5, 2024 via email

BDisp commented Jan 5, 2024

SetRelativeLayout

BDisp commented Jan 5, 2024 • edited Loading

tig commented Jan 5, 2024 • edited Loading

BDisp commented Jan 5, 2024

tig commented Jan 5, 2024 • edited Loading

tig commented Jan 5, 2024

tig commented Jan 5, 2024

BDisp commented Jan 5, 2024 • edited Loading

tig commented Jan 5, 2024

tig commented Jan 5, 2024

tig commented Jan 5, 2024

BDisp commented Jan 5, 2024

tig commented Jan 5, 2024

BDisp commented Jan 5, 2024

tig commented Jan 5, 2024

BDisp commented Jan 5, 2024

BDisp commented Jan 5, 2024

BDisp commented Jan 5, 2024

tig commented Jan 5, 2024

tig commented Jan 5, 2024 • edited Loading

BDisp commented Jan 5, 2024

BDisp commented Jan 5, 2024

tig commented Jan 5, 2024

tig commented Jan 5, 2024

BDisp commented Jan 5, 2024 • edited Loading

BDisp commented Jan 5, 2024

BDisp commented Jan 5, 2024

tig commented Jan 5, 2024

BDisp commented Jan 5, 2024

tig commented Jan 5, 2024

tznind commented Jan 5, 2024

BDisp commented Jan 5, 2024 • edited Loading

Simplify `LayoutStyle` - Make it a computed/read-only property ensure correctness #3127

Simplify `LayoutStyle` - Make it a computed/read-only property ensure correctness #3127

BDisp commented Jan 5, 2024 •

edited

Loading

`SetRelativeLayout`

`SetRelativeLayout`

BDisp commented Jan 5, 2024 •

edited

Loading

tig commented Jan 5, 2024 •

edited

Loading

tig commented Jan 5, 2024 •

edited

Loading

BDisp commented Jan 5, 2024 •

edited

Loading

tig commented Jan 5, 2024 •

edited

Loading

BDisp commented Jan 5, 2024 •

edited

Loading

BDisp commented Jan 5, 2024 •

edited

Loading