Fixes #2358 - BREAKING CHANGE: `Pos.Combine` is incorrect for scenarios involving `PosAbsolute` by tig · Pull Request #2379 · gui-cs/Terminal.Gui

tig · 2023-02-24T21:02:44Z

Fixes Pos.Combine is incorrect for scenarios involving PosAbsolute #2358
Prepares for v2 by making SetRelativeLayout less fragile
In order to fix Why does `TopLevel.Activate/Deactivate` exist? Why is this just not `Focus`? #2356 the following is part of this PR:
- SetRelativeLayout and the related methods were refactored
- Layout related unit tests were moved to Core/LayoutTests.cs
- Adds unit tests to show Pos.Combine is incorrect for scenarios involving PosAbsolute #2358
- Fixes a bug in Border where it was not checking for negative row/col values

This is a breaking change (it changes the previous behavior of SetRelativeLayout) and breaks any code that was using PosCombine but not realizing the results were invalid. For example, all of the Border related Scenarios.

We should take this breaking change because it fixes a real problem in how the API is supposed to work.

Pull Request checklist:

I've named my PR in the form of "Fixes #issue. Terse description."
My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
My code follows the Terminal.Gui library design guidelines
I ran dotnet test before commit
I have made corresponding changes to the API documentation (using /// style comments)
My changes generate no new warnings
I have checked my code and corrected any poor grammar or misspellings
I conducted basic QA to assure all features are working

… not reproduce (gui-cs#2332)" This reverts commit c85ff95.

tig · 2023-02-24T21:05:03Z

@BDisp - can you please do a CR of this?

BDisp · 2023-02-24T21:23:10Z

@tig I think you may merge the #2345 into this to leverage the frame border color not equal to the color scheme.

BDisp · 2023-02-25T00:07:12Z

@tig none of theses labels are redrawing on the Borders Comparisons, Borders on FrameView, Borders on Toplevel and Borders on Window scenarios.

var label = new Label ("I'm a Window") {
				X = Pos.Center (),
				Y = Pos.Center () - 3,
			};
var label2 = new Label ("I'm a Toplevel") {
				X = Pos.Center (),
				Y = Pos.Center () - 3,
			};
var label3 = new Label ("I'm a FrameView") {
				X = Pos.Center (),
				Y = Pos.Center () - 3,
			};

BDisp · 2023-02-25T00:19:42Z

On load theses to views are misaligned:

On the develop branch they are aligned:

I think this has to do with the Pos.Center and Pos.Absolute layout.

BDisp

I wrote some considerations about this. I hope you don´t mind.

BDisp · 2023-02-25T00:36:24Z

-		[Fact, AutoInitShutdown]
-		public void Frame_And_Labels_Does_Not_Overspill_ScrollView ()


These are also good unit to test with layout. I hope you reconsidered to not remove them.

They were moved to LayoutTest.cs (or at least I meant to move them... Or I deleted them by accident). I'll re-look.

BDisp · 2023-02-25T00:51:07Z

@@ -1109,8 +1110,15 @@ public void BringSubviewForward (View subview)
 		/// </remarks>
 		public void Clear ()


@tig you have to merge with the PR #2354 because the Clear method is wrong on this PR. It causes layout malfunction.

BDisp · 2023-02-25T01:06:21Z

-		[Fact, AutoInitShutdown]
-		public void BorderStyle_And_DrawMarginFrame_Gets_Sets ()


I hope you also maintain this unit test. I think you have some errors because of the View.Clear method.

tig · 2023-02-26T00:00:19Z

On load theses to views are misaligned:

On the develop branch they are aligned:

I think this has to do with the Pos.Center and Pos.Absolute layout.

Yep, I struggled getting them to align using Pos.Bottom(label) but Panel's Frame is at the marginframe and label's is at the Margin so it wasn't working. I decided to leave it like this so that would be obvious (but I was also tired of fixing bugs in those Scenarios that were exposed by the bug fix).

tig · 2023-02-26T00:23:13Z

@tig I think you may merge the #2345 into this to leverage the frame border color not equal to the color scheme.

Done. Thanks for noting that!

tig · 2023-02-26T00:32:17Z

@tig none of theses labels are redrawing on the Borders Comparisons, Borders on FrameView, Borders on Toplevel and Borders on Window scenarios.

var label = new Label ("I'm a Window") {
				X = Pos.Center (),
				Y = Pos.Center () - 3,
			};
var label2 = new Label ("I'm a Toplevel") {
				X = Pos.Center (),
				Y = Pos.Center () - 3,
			};
var label3 = new Label ("I'm a FrameView") {
				X = Pos.Center (),
				Y = Pos.Center () - 3,
			};

This is caused by bugs in those scenarios that I didn't notice:

			var label = new Label ("I'm a Window") {
				X = Pos.Center (),
				Y = Pos.Center () - 3,
			};

Y.PosCenter() - 3 for a 5 row tall superview puts the label outside of the Bounds.

This works better:

			var label = new Label ("I'm a Window") {
				X = Pos.Center (),
				Y = Pos.Top (button) - 1
			};

This regex search finds all instances of potential bugs that do this Pos.Center \(\) -.

tig · 2023-02-26T00:40:48Z

I just pushed several fixes to this PR that fix the issues you noted. However, one fix that SHOULD have worked has caused another problem in the 3 "Borders on XXX" scenarios:

The button isn't painting right.

"Borders on Window"

"Borders on FrameView"

"Borders on Toplevel"

Since this doesn't happen in "Borders Comparsion" I'm wondering if this is another bug in the Scenario. Debugging now...

BDisp · 2023-02-26T00:51:19Z

Since this doesn't happen in "Borders Comparsion" I'm wondering if this is another bug in the Scenario. Debugging now...

I already noted that. Try to comment in the View on the GetMaxNeedDisplay method these two lines:

+			//rect.Width += Math.Max (oldFrame.X - newFrame.X, 0);
+			//rect.Height += Math.Max (oldFrame.Y - newFrame.Y, 0);

@tig I prefer to use the Y = Pos.Center () - 2, to really test that center is working properly. Change to -2, because you fixed some existent bug on the layout.

tig · 2023-02-26T01:06:49Z

Since this doesn't happen in "Borders Comparsion" I'm wondering if this is another bug in the Scenario. Debugging now...

I already noted that. Try to comment in the View on the GetMaxNeedDisplay method these two lines:
+			//rect.Width += Math.Max (oldFrame.X - newFrame.X, 0);
+			//rect.Height += Math.Max (oldFrame.Y - newFrame.Y, 0);
@tig I prefer to use the Y = Pos.Center () - 2, to really test that center is working properly. Change to -2, because you fixed some existent bug on the layout.

The bug is here, I think:

GetNeedDisplay is {{X=0,Y=0,Width=65,Height=17}} for the label, which has AutoSize on and thus a Height of `. Thus this call to Clear is clearing more than it should:

I'm out of time for tonight. I you get a chance can you file a new issue for that?

tig · 2023-02-26T01:08:04Z

@tig I prefer to use the Y = Pos.Center () - 2, to really test that center is working properly. Change to -2, because you fixed some existent bug on the layout.

Well, that didn't work, did it? Center WASN'T working right ;-).

I'm out of time tonight so I'm going to leave it as-is for now.

tig · 2023-02-26T01:10:49Z

Now that I think about it more: Why isn't Clear honoring Driver.Clip???

BDisp · 2023-02-26T01:13:48Z

Now that I think about it more: Why isn't Clear honoring Driver.Clip???

If the Driver.Clip is equal to the current view bounds it's ok, otherwise is better maintain as is.

I'll work on this tomorrow.

BDisp · 2023-02-27T20:07:18Z

@tig I already finished and submitted the PR tig#10.

tig · 2023-02-28T01:56:22Z

I now no longer want to fix this in v1.

This bug has persisted for years and I'm the first to notice it.

I am getting worn out trying to merge v1 changes into v2. I spent 3 hours yesterday and ended up giving up because I had made such a mess.

BDisp · 2023-03-01T01:27:01Z

@tig I added the PR #2385 on the v2 that fixes the #2358. The behavior with the button redraw doesn't reproduce anymore. I hope you don't mind.

tig · 2023-03-02T16:34:27Z

Closing as a dupe of #2385

tig added 12 commits February 21, 2023 11:53

Revert "Illustrates gui-cs#2331 (Scrollview not respecting clip) does…

6bb90ed

… not reproduce (gui-cs#2332)" This reverts commit c85ff95.

Merge branch 'visual_style_manager' into v2_config_manager

2b0adf0

Merge branch 'develop' of tig:gui-cs/Terminal.Gui into develop

b5de3e6

Refactored SetRelativeLayout etc; looks good but tests fail

a8b75b6

fixed da bug; also fixed pwsh clipboard tests

1219414

Merge branch 'develop' into v1_setrelativelayout_improvments

7d36a84

merged from develop after accident

d04bf94

merged from develop after accident

c732923

fixing bugs caused by Pos.Combine bugfix

eb3af67

fixed more bugs exposed by Pos.Combine bugfix

c11b4e3

restored Generic.cs

a0c44b7

Pos.Combine unit tests

7fbece9

tig added the breaking-change For PRs that introduces a breaking change (behavior or API) label Feb 24, 2023

tig requested a review from migueldeicaza as a code owner February 24, 2023 21:02

BDisp reviewed Feb 25, 2023

View reviewed changes

Comment thread UnitTests/Core/BorderTests.cs

BDisp requested changes Feb 25, 2023

View reviewed changes

tig added 2 commits February 25, 2023 17:05

Merge branch 'develop' into v1_setrelativelayout_improvement

eaede47

re-merged changes lost from gui-cs#2345

5bb350b

tig added 2 commits February 25, 2023 17:32

Fixed Border Comparsion Pos.Center bugs

0e8cd63

Fixed Borders on XXX Pos.Center bugs

a9cd9ba

BDisp added a commit to BDisp/Terminal.Gui that referenced this pull request Feb 27, 2023

Merge gui-cs#2382 into gui-cs#2379.

301567b

BDisp added a commit to BDisp/Terminal.Gui that referenced this pull request Feb 27, 2023

Merge gui-cs#2382 into gui-cs#2379.

932aca8

tig closed this Mar 2, 2023

tig deleted the v1_setrelativelayout_improvement branch April 3, 2024 01:48

		[Fact, AutoInitShutdown]
		public void Frame_And_Labels_Does_Not_Overspill_ScrollView ()

		@@ -1109,8 +1110,15 @@ public void BringSubviewForward (View subview)
		/// </remarks>
		public void Clear ()

		[Fact, AutoInitShutdown]
		public void BorderStyle_And_DrawMarginFrame_Gets_Sets ()

Conversation

tig commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request checklist:

Uh oh!

tig commented Feb 24, 2023

Uh oh!

BDisp commented Feb 24, 2023

Uh oh!

BDisp commented Feb 25, 2023

Uh oh!

BDisp commented Feb 25, 2023

Uh oh!

Uh oh!

BDisp left a comment

Choose a reason for hiding this comment

Uh oh!

BDisp Feb 25, 2023

Choose a reason for hiding this comment

Uh oh!

tig Feb 26, 2023

Choose a reason for hiding this comment

Uh oh!

BDisp Feb 25, 2023

Choose a reason for hiding this comment

Uh oh!

BDisp Feb 25, 2023

Choose a reason for hiding this comment

Uh oh!

tig commented Feb 26, 2023

Uh oh!

tig commented Feb 26, 2023

Uh oh!

tig commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tig commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BDisp commented Feb 26, 2023

Uh oh!

tig commented Feb 26, 2023

Uh oh!

tig commented Feb 26, 2023

Uh oh!

tig commented Feb 26, 2023

Uh oh!

BDisp commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BDisp commented Feb 27, 2023

Uh oh!

tig commented Feb 28, 2023

Uh oh!

BDisp commented Mar 1, 2023

Uh oh!

tig commented Mar 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tig commented Feb 24, 2023 •

edited

Loading

tig commented Feb 26, 2023 •

edited

Loading

tig commented Feb 26, 2023 •

edited

Loading

BDisp commented Feb 26, 2023 •

edited

Loading