sdispater / clikit

CliKit is a group of utilities to build beautiful and testable command line interfaces.
MIT License
72 stars 17 forks source link

ANSI support is not detected on Windows 10 #35

Open cgudrian opened 3 years ago

cgudrian commented 3 years ago

The StreamOutputStream examines the current console mode, sets the ENABLE_VIRTUAL_TERMINAL_PROCESSING flag if it has not been set and then returns True.

It returns False however, if that flag has already been set (which is the case in my setup): https://github.com/sdispater/clikit/blob/e60ef864db1b938ec4c66b9fdf6793ef5241e947/src/clikit/io/output_stream/stream_output_stream.py#L90-L96

Shouldn't the method return True as well in that case?

TBBle commented 3 years ago

I just came here to report exactly this. Perhaps it is a mistake or refactoring issue, and the False is supposed to be for when the SetConsoleMode fails, not when it's not needed.

With PowerShell 7.1.0 under Windows Terminal 1.4.3243.0 on Windows 10.0.19041.685, ENABLE_VIRTUAL_TERMINAL_PROCESSING is already set. Using Poetry, there's an --ansi command-line override to bypass this detection, which appears to work fine, but is annoying to always have to use.

As a workaround, you can set the ANSICON env-var to something non-empty to force ANSI support.

If the logic here is correct, i.e. if the flag is already set, that means we shouldn't use ANSI by default (due to some historical issues or incompatibility), perhaps Windows Terminal can be allow-listed for the WSLENV env-var, as some other terminals are earlier in the file.


Edit: I just checked, and this code was as-is in the very first commit to this repo in 2018, and wasn't there in the last version of stream_output.py in cleo from which it was forked.

So it probably should read something like

 if (mode.value & ENABLE_VIRTUAL_TERMINAL_PROCESSING) != 0: 
     return True

 return kernel32.SetConsoleMode( 
      h, mode.value | ENABLE_VIRTUAL_TERMINAL_PROCESSING 
  ) != 0
KotlinIsland commented 1 year ago

If this code isn't even here anymore, is the issue still valid?

TBBle commented 1 year ago

The code is still in this repo.

The history is a little confusing: AFAICT this function moved from cleo to clikit (so cleo depended on clikit, and the code in cleo was removed) in 2018, and then in January 2021 cleo was refactored to no longer depend on clikit, and the code was copied back, and now exists in two places.

This ticket was raised between those two events, and not touched since before the latter.

Poetry is using cleo, so at the time I commented, it was using clikit transitively, which is what brought me here. That is no longer the case (and it's being addressed in cleo so Poetry will soon be fixed) but any other users of clikit would benefit from the same fix as cleo.

It's possible there were no direct users except cleo, since this repo hasn't been touched since cleo stopped depending on it. That's a different question though, not specific to this issue.