vezel-dev / cathode

A terminal-centric replacement for the .NET console APIs.
https://docs.vezel.dev/cathode
BSD Zero Clause License
91 stars 7 forks source link

Writing to Terminal with \n inconsistent on Windows vs Linux and vs other libraries #156

Closed scottbilas closed 3 months ago

scottbilas commented 3 months ago

On Windows, Terminal.Out/OutLine will not do a carriage return when receiving a string with embedded \n in it. Given this code:

Vezel.Cathode.Terminal.OutLine("Line 1\nLine 2");

On Windows the output is:

Line 1
      Line 2

If I make that \n into a \r\n, then it does the right thing.

Line 1
Line 2

If I run the \n version on Linux, then it also is correct. Only on Windows is it wrong.

Other libraries also do the right thing. I tested with both Spectre and .NET BCL. On Windows and Linux, with just \n, either of these is also correct:

Spectre.Console.AnsiConsole.WriteLine("Line 1\nLine 2");

System.Console.WriteLine("Line 1\nLine 2");

scottbilas commented 3 months ago

I also noticed that if I change the System.Console test to include a bit of Cathode, it starts doing the wrong thing.

var _ = Vezel.Cathode.Terminal.System;
System.Console.WriteLine("Line 1\nLine 2");

I know the analyzer blocks this because they don't play well together, but simply causing the WindowsVirtualTerminal to get constructed alters the meaning of \n for the system console. Am I missing an option I can set..?

scottbilas commented 3 months ago

Ok it looks like the issue is that DISABLE_NEWLINE_AUTO_RETURN is being set in the native driver.

Since this is deliberate, I'm wondering why?

I can work around the issue for myself by flipping the bit back off after ensuring the native console has been initialized, but what might I break when I do this? (Guess I'll find out!)

alexrp commented 3 months ago

Can you confirm whether clearing the DISABLE_NEWLINE_AUTO_RETURN flag actually helps? That would be surprising to me since it's a flag meant to improve compatibility with Unix terminals in regards to writing in the bottom-rightmost cell of the screen.

alexrp commented 3 months ago

I was actually aware of this issue since the beginning of the project. I recall that I concluded that it was an unfixable platform difference. How or why I concluded that, though, I don't exactly remember. But let me try...

I think it comes down to the fact that CR and LF have distinct functions in a terminal: CR moves the cursor to the leftmost cell without changing its vertical position, and LF moves the cursor down a cell without changing its horizontal position. It just so happens that on Unix platforms, the ONLCR flag is set as part of the default cooked mode, so LF gets translated to CR + LF by the kernel terminal driver, letting you get away with just LF (but only in cooked mode!). The Windows console does the same translation by default, but I believe that changes once ENABLE_VIRTUAL_TERMINAL_PROCESSING is in play. Unfortunately, that flag (and its input equivalent) is a weird mix of cooked mode and raw mode, and I believe there wasn't a good way to enable both VT100 support and retain the LF -> CR + LF translation.

(The reason ENABLE_VIRTUAL_TERMINAL_PROCESSING and ENABLE_VIRTUAL_TERMINAL_INPUT work in such a weird way is that the developers, perhaps rightly, expected most folks to want raw mode for most use cases, so that's where development and testing efforts were focused. The result is that you can tweak those flags down to proper Unix-like raw mode by disabling some other flags, but you cannot do the inverse and tweak them up to proper cooked mode (beyond what's already provided today) by enabling other flags. It's a bit unfortunate.)

... But I could also just be completely misremembering. Yep.

An easy workaround is to just use Environment.NewLine, or if you're not in control of constructing the original string, just call str.ReplaceLineEndings(). This is arguably the right thing to do for portability anyway.

scottbilas commented 3 months ago

This code indeed works around the problem for me:

        using var handle = Win32_PInvoke.CreateFile(
            "CONOUT$",
            (uint)(GENERIC_ACCESS_RIGHTS.GENERIC_READ | GENERIC_ACCESS_RIGHTS.GENERIC_WRITE),
            FILE_SHARE_MODE.FILE_SHARE_READ | FILE_SHARE_MODE.FILE_SHARE_WRITE,
            null,
            FILE_CREATION_DISPOSITION.OPEN_EXISTING,
            0,
            null);

        Win32_PInvoke.GetConsoleMode(handle, out var consoleMode);
        consoleMode &= ~CONSOLE_MODE.DISABLE_NEWLINE_AUTO_RETURN;
        Win32_PInvoke.SetConsoleMode(handle, consoleMode);

I can go find everywhere I'm using the Terminal specific streams, or one of the Terminal 'out' functions, and run them through helper functions instead that do the newline replacement. But I'd rather just set the mode once, as I did above. It's much less error prone.

Indeed this is a strange situation. I've written loads of little cli tools over the years for Windows Mac and Linux, and have always (*) used plain old \n. All my tooling, the company VCS's, etc. have been set to unix EOL. I normally strip \r's and avoid direct/indirect use of Environment.NewLine, because those \r's cause portability annoyances in practice. But here, I need to add them back in to work right!

Philosophy aside, the main argument I have is that it's different from System.Console and Spectre, and therefore surprising. Also only shows up with embedded newlines, which may be a less common case than people hitting OutLine (this is how I came across the issue today). I'm imagining the Mac user who is now trying out their cool little cli tool on Windows, only to find the most surprising behavior..

(* Ok maybe only in the last 15 years, when Windows dev tooling became more unix friendly.)

scottbilas commented 3 months ago

One clarification:

it's different from System.Console and Spectre, and therefore surprising

I don't mean to say that it shouldn't have the ability to be different. Just that the default behavior is different. If it's cooked because that is what people expect (I think I saw that in a comment in your code, and agree), then I'd say that people also generally expect that \n will end up on with an implicit \r as well.

alexrp commented 3 months ago

OK, the fact that DISABLE_NEWLINE_AUTO_RETURN causes this is actually super surprising to me and not at all what I would have expected.

I wonder if this is a bug in the console host. :thinking:

I'll need to do some more investigation and see what can be done. I do agree that it isn't ideal; consistency between platforms is preferable.

alexrp commented 3 months ago

OK. Yeah. The docs for that flag are just completely super wildly wrong. And the funny part is that I filed a bug for it! I just completely forgot, apparently.

The TL;DR of that bug tail seems to be that we should set this flag for raw mode and clear it for cooked mode, similar to how we set ONLCR for cooked mode and clear it for raw mode in the Unix driver.

alexrp commented 3 months ago

@scottbilas v0.13.25 has the fix and should appear on NuGet soon™.

scottbilas commented 3 months ago

Amazing, at multiple levels. :D

scottbilas commented 3 months ago

Got the latest version, I can confirm it fixes the issue for me and I have removed my local workaround. Thank you!