Fixes 4191 - Rewrite WindowsOutput #4193
Conversation
| } | ||
| else if (width == 2 && col + 1 < buffer.Cols) | ||
| { | ||
| // Double-width char: encode to UTF-16 surrogate pair and write both halves |
There was a problem hiding this comment.
If IsBmp is true are considered Basic Multilingual Plane (BMP) and they aren't composed by surrogate pairs. Some BMP may have a width of 2 columns. If IsBmp is false (non-Bmp) then it's considered as surrogate pairs which some may have a width of 2 columns and some may have a width of 1 column.
There was a problem hiding this comment.
are you suggesting new comment wording? or that code is wrong?
There was a problem hiding this comment.
The comment and the code.
In the v2_develop using cmd.exe:
In this PR branch using cmd.exe:
Don't use Windows Terminal and VSConsole because they already has virtual terminal enabled and you won't see the issue. Use cmd.exe or conhost.exe.
The Unicode Character “你” (U+4F60) is a char with a width of 2 columns but isn't a surrogate pair nor an array of chars, because "你".Length is equal to 1 which mean it's only a single char that occupies 2 columns.
There was a problem hiding this comment.
Also the second item from the array returned by rune.EncodeToUtf16 (utf16) is a null char because it's initialized with 2 elements but the char '你' has only 1 element.
There was a problem hiding this comment.
How does v2net look for you on this setup?
Its clear the current code isn't right, because you can see there is that diamond question mark showing second cell of the Rune has bad data in it on v2_develop.
This issue does not affect NetDriver/v2net
There was a problem hiding this comment.
How does v2net look for you on this setup?
It looks great using v2net:
Its clear the current code isn't right, because you can see there is that diamond question mark showing second cell of the Rune has bad data in it on v2_develop.
The question mark is a replacement char added in the AddRune method after a wide code point. Each driver will handle it accordingly to their default. In my opinion it should be a null char '\0', which Win API normally ignore it. But also a replacement char is needed to indicate thar a wide code point doesn't feet in the last column, where it's already handled in the AddRune method.
This issue does not affect NetDriver/v2net
This issue doesn't affect NetDriver/v2net because the runes are handled as string. When a wide code point is being handled the driver already has the right encoded chars and skip the next rune. With Win API drivers we need to pass each encoded char to each cell, for output the wide code point. Here, happens 2 different situations, First situation is if the wide code point is really a char, where it must be pass to the Win API drivers, follow by a null char '\0' which will be ignored. The second situation is if the wide codepoint is a surrogate pair, where each char must be pass to the Win API drivers in each cell. The real problem, is with surrogate pairs that weren't a wide code point but will occupy 2 cells, because each char should be pass to each cell, not fitting all the chars in these columns. In this case, there is needed to rethink about a way to workaround this.
There was a problem hiding this comment.
The problem with cmd.exe comes down to 'force16' colors code pathway.
public bool WriteToConsole (Size size, WindowsConsole.ExtendedCharInfo [] charInfoBuffer, WindowsConsole.Coord bufferSize, WindowsConsole.SmallRect window, bool force16Colors)
{
//Debug.WriteLine ("WriteToConsole");
//if (_screenBuffer == nint.Zero)
//{
// ReadFromConsoleOutput (size, bufferSize, ref window);
//}
var result = false;
- if (force16Colors)
+ if (false)When I make this change on my fixed PR here for v2win then all terminals render correctly:
_cmd.exe (top left) using font NSimSun with Alacritty (top right) and Windows Terminal (below)_
…lt unless user explicitly forces 16
|
Ok looks like only change needed really is to get rid of the force 16 colors. I've created PR for that here #4195 |
|
Seems the changes are a mixed bag. My change fixes Alacritty for 16 color mode but as you said above @BDisp it results in 'gap' in conhost. I think for now we can leave it with the changes in #4195 which is just to stop using And it looks like different terminals are dealing with the lack of proper surrogate pair support in that API call differently. |
tznind
changed the title
tznind
changed the title
…rogate pairs by press Ctrl+V
…onsole which is created in input handle
Fixes legacy output mode
Fix v2net issue not draw first line by forcing set cursor position
Remove force parameter
|
@tznind when do you intend to add support to these combinations? With NetDriver all the combinations with the F1 is possible as you can see in the image. // Function keys with modifiers
/* Not currently supported
yield return new object [] { "\u001b[1;2P", Key.F1.WithShift };
yield return new object [] { "\u001b[1;3Q", Key.F2.WithAlt };
yield return new object [] { "\u001b[1;5R", Key.F3.WithCtrl };
*/
|
|
Needs patterns in AnsiParser that dont collide with other existing patterns. Lets get comprehensive list of what is missing first |
|
The patterns never colide with each other. It's only needed to add the modifiers to the patterns which in EscSeqUtils is as below. So, any number from 2 to 8 before a terminator that may be considered a key input, represent the modifier. /// <summary>
/// Gets the <see cref="ConsoleModifiers"/> from the value.
/// </summary>
/// <param name="value">The value.</param>
/// <returns>The <see cref="ConsoleModifiers"/> or zero.</returns>
public static ConsoleModifiers GetConsoleModifiers (string? value)
{
return value switch
{
"2" => ConsoleModifiers.Shift,
"3" => ConsoleModifiers.Alt,
"4" => ConsoleModifiers.Shift | ConsoleModifiers.Alt,
"5" => ConsoleModifiers.Control,
"6" => ConsoleModifiers.Shift | ConsoleModifiers.Control,
"7" => ConsoleModifiers.Alt | ConsoleModifiers.Control,
"8" => ConsoleModifiers.Shift | ConsoleModifiers.Alt | ConsoleModifiers.Control,
_ => 0
};
} |
|
I have moved to seperate issue and fixed in |
|
@BDisp anything else to do on this PR or are we happy? Seems to work a lot better and be simpler code now. |
For me it's good enough. Thanks. |
|
I'm getting married this weekend. Hence my absence. I'll dive back in to TG next week-ish. |
Congratulations!
Haha no rush! Enjoy the festivities |
|
Ok... diving back in... |
| || buffer.Contents.Length != buffer.Rows * buffer.Cols | ||
| || buffer.Rows != Console.WindowHeight) | ||
| { | ||
| // return; |
| { | ||
| if (Console.WindowHeight < 1) | ||
| { | ||
| return; |
There was a problem hiding this comment.
Why is this within the loop? It can't change while this code is running, right?
Fixes
Proposed Changes/Todos
We now assume SupportsTrueColor is
trueunless user explicitly setsfalsethemselves.Not all width 2 characters are
Rune.IsBmp. The old code only ever consideredrune.GetColumns()in theIsBmppathway.Rewrite 16 color handling mode to use
SetConsoleTextAttributeandWriteConsoleWinstead of problematicWriteConsoleOutputWFix is applied only to v2win
Pull Request checklist:
CTRL-K-Dto automatically reformat your files before committing.dotnet testbefore commit///style comments)