Partially Fixes #2483 - Removes old Border and leverages LineCanvas for Frames, etc... by tig · Pull Request #2527 · gui-cs/Terminal.Gui

tig · 2023-04-11T01:05:43Z

This PR is fairly massive, but also super cool (I think).

It includes:

Partial Fix of Remove need for TileView - Use LineCanvas to draw borders and auto-join borders #2483
- Adds LineCanvas property to View. If the new UseSuperViewLineCanvas property is true (default is false) then the Border (a Frame) will use the view's SuperView's LineCanvas. This enables auto-join. But there are a bunch of edge cases for this to work smoothly that I'll tackle in a future PR that will complete Remove need for TileView - Use LineCanvas to draw borders and auto-join borders #2483.
- The LineCanvas Experiments sets UseSuperViewLineCanvas to true so you can play around with this.
Fixes 'LineCanvas has two minds about what Length == 0` means #2530
Adds new APIs to LineCanvas and does some refactoring to ease testing (Clear, Bounds, ToString, ...)
Massively expands LineCanvas unit tests
Updates Rect with a better Union method that supports Rects with negative coords; changes ToString to match .NET style; Adds unit tests
Adds a new Line class that will eventually replace LineView. For now it's only used in a "LineCanvas Experiments" for testing.
Removes ConsoleDriver.DrawWindowFrame and ConsoleDriver.DrawWindowTitle
Moves View.DrawFrame to Frame.DrawFrame (still obsolete and not long for this world once the few Views that use it get refactored).
Adds DrawTitle to Frame (temporary API)
Renames BorderFrame to Border
Adds three new TestHelpers
- SetupFakeDriverAttribute that simply sets Application.Driver = new FakeDriver() so code like LineCanvas that needs a driver can work without the overhead of AutoInitShutdown (which makes unit tests slower)
- These guys make it easy to NOT use output and the CRAZY AssserDriverContentsAre... functions which cause hair loss.

	/// <summary>
	/// Verifies two strings are equivalent. If the assert fails, output will be generated to standard 
	/// output showing the expected and actual look.
	/// </summary>
	/// <param name="output"></param>
	/// <param name="expectedLook">A string containing the expected look. Newlines should be specified as "\r\n" as
	/// they will be converted to <see cref="Environment.NewLine"/> to make tests platform independent.</param>
	/// <param name="actualLook"></param>
	public static void AssertEqual (ITestOutputHelper output, string expectedLook, string actualLook)
	{
		// Convert newlines to platform-specific newlines
		expectedLook = expectedLook.Replace ("\r\n", Environment.NewLine);
		
		// If test is about to fail show user what things looked like
		if (!string.Equals (expectedLook, actualLook)) {
			output?.WriteLine ("Expected:" + Environment.NewLine + expectedLook);
			output?.WriteLine ("But Was:" + Environment.NewLine + actualLook);
		}

		Assert.Equal (expectedLook, actualLook);
	}

	/// <summary>
	/// Verifies two strings are equivalent. If the assert fails, output will be generated to standard 
	/// output showing the expected and actual look. 
	/// </summary>
	/// <param name="output">Uses <see cref="ustring.ToString()"/> on <paramref name="actualLook"/>.</param>
	/// <param name="expectedLook">A string containing the expected look. Newlines should be specified as "\r\n" as
	/// they will be converted to <see cref="Environment.NewLine"/> to make tests platform independent.</param>
	/// <param name="actualLook"></param>
	public static void AssertEqual (ITestOutputHelper output, string expectedLook, ustring actualLook)
	{
		// Convert newlines to platform-specific newlines
		expectedLook = expectedLook.Replace ("\r\n", Environment.NewLine);

		// If test is about to fail show user what things looked like
		if (!string.Equals (expectedLook, actualLook)) {
			output?.WriteLine ("Expected:" + Environment.NewLine + expectedLook);
			output?.WriteLine ("But Was:" + Environment.NewLine + actualLook.ToString ());
		}

		Assert.Equal (expectedLook, actualLook);
	}

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

…_borders_fixes_2483

tig · 2023-04-13T08:29:36Z

In addition to the updates in the first comment of this PR...

@BDisp I broke a few unit tests having to do with popups over dialogs. I couldn't figure it out. Maybe you can see what I did? Search for BUGBUG: Broke this test with #2483

…qual

BDisp · 2023-04-13T11:24:55Z

Looking into gif above, the current window being drag should be bring to front to overlap the others. Another issue is the borders being visible inside two windows. Only the border of the window being dragged should be visible inside others windows.

tig · 2023-04-13T12:53:50Z

Looking into gif above, the current window being drag should be bring to front to overlap the others. Another issue is the borders being visible inside two windows. Only the border of the window being dragged should be visible inside others windows.

Right. These are all things that will need to be fixed before al this is done.

Related: #2537

Looking into gif above, the current window being drag should be bring to front to overlap the others.

This would bein Overalp mode only. In Tile mode the right behavior would be for it not be possible to have overlaps. This is what I meant in the spec:

Only the border of the window being dragged should be visible inside others windows.

Yes. My original plan, which is now partially implemented in this PR, was to have subviews just render to their SuperView's LineCanvas. But this causes the addition of the Lines to be done in the order opposite of how the subviews were added (their z-order). I've now realized this wont work for this reason and a few others.

Instead, we'll have the subviews render to their ownLineCanvas. The SuperView will then iterate through each subview, rendering that subview's LineCanvas in the correct order.

BDisp · 2023-04-13T15:01:26Z

Expected:
┌──────────────────┐
│                  │
│ ┌─────────────┐  │
│ │ Test        │  │
┌───────────────────
│ Select All   Ctrl+
│ Delete All   Ctrl+
│ Copy         Ctrl+
│ Cut          Ctrl+
│ Paste        Ctrl+
│ Undo         Ctrl+
│ Redo         Ctrl+
└───────────────────
│                  │
└──────────────────┘
But Was:
┌──────────────────┐
│                  │
│ ┌─────────────┐  │
│ │ Test        │  │
┌─│─────────────│───
│ └─────────────┘rl+
│ Delete All   Ctrl+
│ Copy         Ctrl+
│ Cut          Ctrl+
│ Paste        Ctrl+
│ Undo         Ctrl+
│ Redo         Ctrl+
└───────────────────
│                  │
└──────────────────┘

For sure I don´t like the current result. The question is what aspect should it has to have, since the LineCanvas is so powered? The Expected or the following?

┌──────────────────┐
│                  │
│ ┌─────────────┐  │
│ │ Test        │  │
├─┴─────────────┴──┴
│ Select All   Ctrl+
│ Delete All   Ctrl+
│ Copy         Ctrl+
│ Cut          Ctrl+
│ Paste        Ctrl+
│ Undo         Ctrl+
│ Redo         Ctrl+
├──────────────────┬
│                  │
└──────────────────┘

BDisp

@tig I'll submit a PR to this PR.

BDisp · 2023-04-13T14:04:43Z

+	public static void AssertEqual (ITestOutputHelper output, string expectedLook, string actualLook)
+	{
+		// Convert newlines to platform-specific newlines
+		expectedLook = expectedLook.Replace ("\r\n", Environment.NewLine);


@tig it seems that you are only testing on Linux because this will throw an exception if testing on Windows. You have to add the follow line:

if (Environment.NewLine.Length == 2 && !expectedLook.Contains ("\r\n")) { expectedLook = expectedLook.Replace ("\n", Environment.NewLine); } else if (Environment.NewLine.Length == 1) { expectedLook = expectedLook.Replace ("\r\n", Environment.NewLine); }

BDisp · 2023-04-13T14:06:14Z

+	public static void AssertEqual (ITestOutputHelper output, string expectedLook, ustring actualLook)
+	{
+		// Convert newlines to platform-specific newlines
+		expectedLook = expectedLook.Replace ("\r\n", Environment.NewLine);


@tig it seems that you are only testing on Linux because this will throw an exception if testing on Windows. You have to add the follow line:

if (Environment.NewLine.Length == 2 && !expectedLook.Contains ("\r\n")) { expectedLook = expectedLook.Replace ("\n", Environment.NewLine); } else { expectedLook = expectedLook.Replace ("\r\n", Environment.NewLine); }

BDisp · 2023-04-13T14:09:02Z

+		expectedLook = expectedLook.Replace ("\r\n", Environment.NewLine);
+
+		// If test is about to fail show user what things looked like
+		if (!string.Equals (expectedLook, actualLook)) {


To verify the equality you have to add ToString() for the ustring,

if (!string.Equals (expectedLook, actualLook.ToString ())) {

…rs them

tig · 2023-04-13T15:38:22Z

Expected:
┌──────────────────┐
│                  │
│ ┌─────────────┐  │
│ │ Test        │  │
┌───────────────────
│ Select All   Ctrl+
│ Delete All   Ctrl+
│ Copy         Ctrl+
│ Cut          Ctrl+
│ Paste        Ctrl+
│ Undo         Ctrl+
│ Redo         Ctrl+
└───────────────────
│                  │
└──────────────────┘
But Was:
┌──────────────────┐
│                  │
│ ┌─────────────┐  │
│ │ Test        │  │
┌─│─────────────│───
│ └─────────────┘rl+
│ Delete All   Ctrl+
│ Copy         Ctrl+
│ Cut          Ctrl+
│ Paste        Ctrl+
│ Undo         Ctrl+
│ Redo         Ctrl+
└───────────────────
│                  │
└──────────────────┘

For sure I don´t like the current result. The question is what aspect should it has to have, since the LineCanvas is so powered? The Expected or the following?

┌──────────────────┐
│                  │
│ ┌─────────────┐  │
│ │ Test        │  │
├─┴─────────────┴──┴
│ Select All   Ctrl+
│ Delete All   Ctrl+
│ Copy         Ctrl+
│ Cut          Ctrl+
│ Paste        Ctrl+
│ Undo         Ctrl+
│ Redo         Ctrl+
├──────────────────┬
│                  │
└──────────────────┘

Expected:
┌──────────────────┐
│                  │
│ ┌─────────────┐  │
│ │ Test        │  │
┌───────────────────
│ Select All   Ctrl+
│ Delete All   Ctrl+
│ Copy         Ctrl+
│ Cut          Ctrl+
│ Paste        Ctrl+
│ Undo         Ctrl+
│ Redo         Ctrl+
└───────────────────
│                  │
└──────────────────┘
But Was:
┌──────────────────┐
│                  │
│ ┌─────────────┐  │
│ │ Test        │  │
┌─│─────────────│───
│ └─────────────┘rl+
│ Delete All   Ctrl+
│ Copy         Ctrl+
│ Cut          Ctrl+
│ Paste        Ctrl+
│ Undo         Ctrl+
│ Redo         Ctrl+
└───────────────────
│                  │
└──────────────────┘

For sure I don´t like the current result. The question is what aspect should it has to have, since the LineCanvas is so powered? The Expected or the following?

┌──────────────────┐
│                  │
│ ┌─────────────┐  │
│ │ Test        │  │
├─┴─────────────┴──┴
│ Select All   Ctrl+
│ Delete All   Ctrl+
│ Copy         Ctrl+
│ Cut          Ctrl+
│ Paste        Ctrl+
│ Undo         Ctrl+
│ Redo         Ctrl+
├──────────────────┬
│                  │
└──────────────────┘

Expected.

V2 autojoin borders fixes 2483 fix

tig · 2023-04-13T19:40:53Z

For giggles, I just wasted a bunch of time making this happen:

BDisp · 2023-04-13T20:00:27Z

In addition to the updates in the first comment of this PR...

* @BDisp I broke a few unit tests having to do with popups over dialogs. I couldn't figure it out. Maybe you can see what I did? Search for `BUGBUG: Broke this test with #2483`

@tig I already had fixed this but when I merged from your repo to submit a PR, I get a bunch of errors again.

BDisp · 2023-04-13T18:30:11Z

 			// Invoke DrawContentCompleteEvent
 			OnDrawContentComplete (bounds);

+			OnDrawFrames ();


OnDrawFrames must be the first call on a Draw, otherwise the frames will always overlap the other views frames.

tig and others added 5 commits April 10, 2023 19:04

POC

afc8d97

View.DrawFrame now uses LineCanvas

9da0678

Fixes gui-cs#2531. Toplevel should redraw only if it's needed.

a06a345

Fix toplevel when mdi is enabled preventing clear the screen twice.

970f0dc

Massive LineCanvis updates

3afddc5

tig mentioned this pull request Apr 12, 2023

Add additional LineStyles #2533

Merged

8 tasks

tig changed the title ~~Fxes #2483 - Modifying~~ Fxes #2483 - Removes old Border and leverages LineCanvas Apr 12, 2023

tig changed the title ~~Fxes #2483 - Removes old Border and leverages LineCanvas~~ Fxes #2483 - Auto-join Borders - Removes old Border and leverages LineCanvas Apr 12, 2023

BDisp and others added 6 commits April 13, 2023 04:51

Fixes gui-cs#2534. Bounds isn't updating when the Frame is changed.

abd7eba

Almost everything works!

2f66e7b

Had to disable a few tests but all unit test now pass again

9958fcb

Deleted ConsoleDriver.DrawWindowFrame; hacked ProgressBar

9a222ed

Deleted ConsoleDriver.DrawWindowTitle; moved to Frame.DrawTitle

c41581a

Merge branch 'v2_develop' of tig:gui-cs/Terminal.Gui into v2_autojoin…

a57ffed

…_borders_fixes_2483

tig marked this pull request as ready for review April 13, 2023 07:38

tig requested a review from migueldeicaza as a code owner April 13, 2023 07:38

tig added 2 commits April 13, 2023 01:54

Renames BorderFrame to Border

d8b2fef

Removed old commented code

3f9059d

tig changed the title ~~Fxes #2483 - Auto-join Borders - Removes old Border and leverages LineCanvas~~ Fxes #2483 - Removes old Border and leverages LineCanvas for Frames, etc... Apr 13, 2023

tig changed the title ~~Fxes #2483 - Removes old Border and leverages LineCanvas for Frames, etc...~~ Partially Fixes #2483 - Removes old Border and leverages LineCanvas for Frames, etc... Apr 13, 2023

Tweaked scenario

0508d55

Added auto convert \r\n to Enviornment.NewLine in TestHelpers.AssertE…

3348848

…qual

BDisp added 2 commits April 13, 2023 12:34

Resolving merge conflicts.

43458d5

Resolving merge conflicts.

27efe26

BDisp added 2 commits April 13, 2023 14:45

Fix merge errors.

ea8322f

Fix AssertEqual newlines to platform-specific.

0d4b1df

BDisp reviewed Apr 13, 2023

View reviewed changes

Refactored frames drawing; view adds to its lineview, superview rende…

1ad43b2

…rs them

Merge pull request #12 from BDisp/v2_autojoin_borders_fixes_2483-fix

cf00926

V2 autojoin borders fixes 2483 fix

BDisp reviewed Apr 13, 2023

View reviewed changes

tig added 3 commits April 13, 2023 14:31

New titlebar style based on Border.Top size; fixed bugs

6a4a970

wzard bug

8bbbc0c

Merged bdisp's latest menu pr; hacked stuff to make work

baddc34

tig merged commit 8c59e82 into gui-cs:v2_develop Apr 13, 2023

This was referenced Apr 13, 2023

Fixes menu frame and unit test on the #2483. #2542

Merged

Remove need for TileView - Use LineCanvas to draw borders and auto-join borders #2483

Closed

tig deleted the v2_autojoin_borders_fixes_2483 branch April 3, 2024 01:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partially Fixes #2483 - Removes old Border and leverages LineCanvas for Frames, etc...#2527

Partially Fixes #2483 - Removes old Border and leverages LineCanvas for Frames, etc...#2527
tig merged 24 commits into
gui-cs:v2_developfrom
tig:v2_autojoin_borders_fixes_2483

tig commented Apr 11, 2023 •

edited

Loading

Uh oh!

tig commented Apr 13, 2023 •

edited

Loading

Uh oh!

BDisp commented Apr 13, 2023

Uh oh!

tig commented Apr 13, 2023

Uh oh!

BDisp commented Apr 13, 2023

Uh oh!

BDisp left a comment

Uh oh!

BDisp Apr 13, 2023

Uh oh!

BDisp Apr 13, 2023

Uh oh!

BDisp Apr 13, 2023

Uh oh!

tig commented Apr 13, 2023

Uh oh!

tig commented Apr 13, 2023

Uh oh!

BDisp commented Apr 13, 2023 •

edited

Loading

Uh oh!

BDisp Apr 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Pull Request checklist:

Uh oh!

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

Uh oh!

Uh oh!

BDisp commented Apr 13, 2023

Uh oh!

tig commented Apr 13, 2023

Uh oh!

BDisp commented Apr 13, 2023

Uh oh!

BDisp left a comment

Choose a reason for hiding this comment

Uh oh!

BDisp Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

BDisp Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

BDisp Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

tig commented Apr 13, 2023

Uh oh!

tig commented Apr 13, 2023

Uh oh!

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

Uh oh!

Uh oh!

BDisp Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tig commented Apr 11, 2023 •

edited

Loading

tig commented Apr 13, 2023 •

edited

Loading

BDisp commented Apr 13, 2023 •

edited

Loading