Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #666. Refactor ConsoleDrivers to simplify and remove duplicated code #2612

Merged
merged 120 commits into from
Aug 9, 2023

Conversation

tig
Copy link
Collaborator

@tig tig commented May 8, 2023

Fixes #666 - Include a terse summary of the change or which issue is fixed.

For v2, and as part of #2606, I'm tackling this. Todo:

  • Move() and AddStr() - there's almost no difference in each of the 4 driver's implementation, and the code is complex and hard to understand.
  • I have an idea that doing unicode wide manipulation in AddStr is the wrong place to do it and will investigate how to move that logic into UpdateScreen
  • Each driver defines _ccol and _crow and _contents the same and uses them in the same way. These should be moved into ConsoleDriver
  • Move non-driver specific color stuff to ./Drawning
  • NetDriver keyboard handling is broken (e..g. multiple ESC required to quit)
  • NetDriver under WSL does not work - (fails to draw correctly)
  • Some double clicks not moving focus (WindowsDriver)
  • CursesDriver doesn't draw when first started (until first mouse move)
  • CursesDriver hangs when moving to ArabicPresentaitonFormsB
  • Add more primitive unit tests, especially for unicode handling

These issues are NOT fixed in this PR and need to be addressed separately:

  • CursesDriver doesn't render CharMap correctly for wide chars (and other Unicode) - Curses is doing something funky with glyphs that report GetColums() of 1 yet are rendered wide. E.g. 0x2064 (invisible times) is reported as 1 column but is rendered as 2. WindowsDriver & NetDriver correctly render this as 1 column, overlapping the next cell.
  • Refactor Color/True Color
  • Add TrueColor support to CursesDriver
  • Add TrueColor support to NetDriver

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

@tig tig requested a review from migueldeicaza as a code owner May 8, 2023 19:17
@tig
Copy link
Collaborator Author

tig commented May 9, 2023

@BDisp take a look...

Right now, the only driver that's really using my new codepath is FakeDriver. When I read your message about working on the same thing, I was just about to refactor AddRune to not be virtual so there was one implementation that looks something like this:

	public override void AddRune (System.Rune systemRune)
	{
		var rune = new Rune (systemRune).MakePrintable ();
		var runeWidth = rune.GetColumnWidth ();
		var validLocation = IsValidLocation (Col, Row);

		if (validLocation) {
			if (rune.IsCombiningMark () && Col > 0) {
				// Decode the previous rune
				var previousRune = new Rune (Contents [Row, Col - 1, 0]);
				var newCombo = new StringBuilder ();
				ReadOnlySpan<char> remainingInput = previousRune.ToString ().AsSpan ();
				while (!remainingInput.IsEmpty) {
					// Decode
					OperationStatus opStatus = Rune.DecodeFromUtf16 (remainingInput, out Rune result, out int charsConsumed);

					if (opStatus == OperationStatus.DestinationTooSmall || opStatus == OperationStatus.InvalidData) {
						result = Rune.ReplacementChar;
					}

					newCombo.Append (result);
					// Slice and loop again
					remainingInput = remainingInput.Slice (charsConsumed);
				}
				newCombo.Append (rune);

				var combined = newCombo.ToString ();
				var normalized = !combined.IsNormalized () ? combined.Normalize () : combined;
				Contents [Row, Col - 1, 0] = normalized [0];// BUGBUG: This is wrong, we need to handle the case where the rune is more than one char
				Contents [Row, Col - 1, 1] = CurrentAttribute;
				Contents [Row, Col - 1, 2] = 1;
				Col--;
			} else {
				Contents [Row, Col, 0] = rune.Value;
				Contents [Row, Col, 1] = CurrentAttribute;

				if (Col > 0) {
					var left = new Rune (Contents [Row, Col - 1, 0]);
					if (left.GetColumnWidth () > 1) {
						Contents [Row, Col - 1, 0] = Rune.ReplacementChar.Value;
					}
				}

				if (runeWidth > 1) {
					Col++;
					Contents [Row, Col, 0] = Rune.ReplacementChar.Value;
					Contents [Row, Col, 1] = CurrentAttribute;
					Contents [Row, Col, 2] = 1;
				}
			}
		}
		Col++;
	}

To enable CursesDriver to render to curses at AddRune time, vs in UpdateScreen, my plan was to add new virtual methods (e.g. OnContentsChanged(col, row, Rune, Attribute, Dirty).

One thing I just learned is having Contents just be an array of tuples of ints is we'll never be able to support combining sequences that don't normalize into a single unicode char. See ContentsTests.AddStr_With_Combining_Characters for an example that will never work.

To address this, I propose we change the way Contents is defined to be a two dimensional array of structs like this:

struct ContentsCell {
   string runes;
   Attribute attr;
   bool Dirty;
}

Also, look at how I refactored things, including the System.Text.Rune extension methods.

@tig
Copy link
Collaborator Author

tig commented May 9, 2023

See #2616

@BDisp
Copy link
Collaborator

BDisp commented May 9, 2023

You are doing well. I think with your focused on the ConsoleDriver and I with focused on the System.Text.Rune will result on the right combination.

@tig tig marked this pull request as ready for review August 8, 2023 13:15
@tig
Copy link
Collaborator Author

tig commented Aug 8, 2023

@BDisp & @tznind - This huge, massive, PR is now ready for review.

See the first post in this thread for what I've decided to punt to other PRs, so we can merge this.

I want to add some more unicode unit tests too.

Copy link
Collaborator

@BDisp BDisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tig I only left a question. See bellow, thanks.

Comment on lines +44 to +47
│これは┌────────────┐です。│
│これは│ワイドルーン│です。│
│これは│ {CM.Glyphs.LeftBracket} 選ぶ {CM.Glyphs.RightBracket} │です。│
│これは└────────────┘です。│
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally when a surrogate pair hasn't enough space to display I replaced it with a space. Now you are using the replacement rune. Is this temporary or you are think let it as is now? It's a matter of personal choice but I only I'm asking if it was for a test or a definitive choice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the right thing to do is.

In some cases (e.g. when debugging), it's helpful to have the replacement char shown. But in real-world cases, it may look better to have spaces.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I'm reverting to show the replacement char

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the right thing to do is.

That was controlled on the AddRune method. I referred to surrogate pairs, but it also may be non surrogate UTF16 that has a column width of 2. Even some surrogate pairs have only one column width of 1. Well, a valid surrogate pair is composed by two surrogates (lower and high). When you are reading a surrogate pair that doesn't fit in the available space (you have only one column) you are reading the first char surrogate that isn't printable and you replace it. Before when I checked a rune that doesn't fit I replace with a space, because the user must see the right rune or none, instead of a replacement rune. The condition is the same for all drivers but each driver has it own way to replace it. Only the contents are equal.

In some cases (e.g. when debugging), it's helpful to have the replacement char shown. But in real-world cases, it may look better to have spaces.

Yes is true, I agree. The only driver that is dealing with surrogate pairs with a column width of 1 is the NetDriver because it use a String.Builder list, but I think the FakeDriver may use it too.

@tig tig merged commit 0df485a into gui-cs:v2_develop Aug 9, 2023
1 check passed
This was referenced Aug 9, 2023
@tig tig deleted the v2_fixes_666_console_driver_dupes branch April 3, 2024 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants