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

Windows 10 and above ANSI support #935

Merged
merged 6 commits into from
Aug 29, 2023
Merged

Windows 10 and above ANSI support #935

merged 6 commits into from
Aug 29, 2023

Conversation

tunaflsh
Copy link
Contributor

@tunaflsh tunaflsh commented Aug 7, 2023

This pull request is a quick fix to issue #934 with little changes to the original code.

  • Lets colorama decide whether to convert or strip the ANSI sequences.

@Delgan
Copy link
Owner

Delgan commented Aug 19, 2023

Thanks for the bug report and suggested improvement @tunaflsh.

I've been thinking about this, and although we could indeed let colorama decides whether convert and strip should be enabled, I believe it might be preferable to handle this manually within the should_wrap() function of Loguru.

Loguru implements additional checks that are not part of the Colorama package. For example, in should_colorize() Loguru detects that colors should be enabled when "PYCHARM_HOSTED" is present as a environment variable. I feel using convert=None and strip=None could cause Colorama to disable colors while Loguru wants them to be enabled.

What do you think of detecting support for VT processing to decide whether the stream should be wrapped?

diff --git a/loguru/_colorama.py b/loguru/_colorama.py
index d040e79..67d906f 100644
--- a/loguru/_colorama.py
+++ b/loguru/_colorama.py
@@ -44,6 +44,15 @@ def should_wrap(stream):
     if stream is not sys.__stdout__ and stream is not sys.__stderr__:
         return False
 
+    try:
+        from colorama.winterm import enable_vt_processing
+        has_native_ansi = enable_vt_processing(stream.fileno())
+    except Exception:
+        has_native_ansi = False
+
+    if has_native_ansi:
+        return False
+
     from colorama.win32 import winapi_test
 
     return winapi_test()

I'm not a big fan of letting Loguru mutate the console state while calling enable_vt_processing(), but there aren't many alternatives given the colorama API.

@@ -19,7 +20,7 @@
("{level.icon}", lambda r: r == "🐞"),
("{file}", lambda r: r == "test_formatting.py"),
("{file.name}", lambda r: r == "test_formatting.py"),
("{file.path}", lambda r: r == __file__),
("{file.path}", lambda r: os.path.samefile(r, __file__)),
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you think this is preferable? I feel the opposite. Checking using os.path.samefile() is less restrictive than using ==. The test must fail if {file.path} is "/home/user/../user/foo.txt" while "/home/user/foo.txt" is expected, but using os.path.samefile() won't detect that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That unit test failed on my system. The path's letter cases is inconsistent in Python on Windows. Maybe it should be os.path.normcase.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the explanation! Yes using os.path.normcase() is a good idea.

Copy link
Contributor Author

@tunaflsh tunaflsh Aug 21, 2023

Choose a reason for hiding this comment

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

Should this be resolved independently from this and #943 PRs?

@tunaflsh
Copy link
Contributor Author

tunaflsh commented Aug 21, 2023

Loguru implements additional checks that are not part of the Colorama package. For example, in should_colorize() Loguru detects that colors should be enabled when "PYCHARM_HOSTED" is present as a environment variable. I feel using convert=None and strip=None could cause Colorama to disable colors while Loguru wants them to be enabled.

I'm not sure what you are referring to exactly. I've looked into colorama source code and the stripping/conversion occurs if convert or strip are True. So setting them to None shouldn't make it more likely to disable colors than setting convert=True, unless you mean the old code could've also been affected by this issue.

In any case, enable_vt_processing should resolve the issue.

But on older Windows systems won't it still strip ANSI codes even with "PYCHARM_HOSTED" because of convert=True?

@Delgan
Copy link
Owner

Delgan commented Aug 21, 2023

I'm not sure what you are referring to exactly. I've looked into colorama source code and the stripping/conversion occurs if convert or strip are True. So setting them to None shouldn't make it more likely to disable colors than setting convert=True, unless you mean the old code could've also been affected by this issue.

Hum... Actually the current usage of convert=True and strip=False by Loguru is misleading. Both values should be set to True in the wrap() function. It works because Colorama isn't implemented to convert the colors to Win32 API calls without stripping the ansi codes from the string, so convert=True and strip=False is effectively the same than convert=True and strip=True.

Here is a table to summarize usage of these arguments:

convert strip Behavior When to use it
True True Remove ansi code from the string and convert them to Win32 API calls. Colorizing console output on old Windows version.
True False Call Win32 API to colorize output and don't remove the ansi codes. Never
False True Remove ansi code from the string but don't call Win32 API. When the output isn't TTY (when piping commands).
False False Don't remove ansi code and don't call Win32 API. Colorizing console output on new Windows version.

When convert=None and/or strip=None is used, Colorama will use some heuristics to select the appropriate arguments according to the above table. Loguru does the same. There exists edge cases: for example, using Pycharm sys.stderr.isatty() will return False. As a consequence, Colorama would strip the ansi codes from the string but would't call Win32 API to colorize the output. This is unfortunate, because actually, Pycharm output does support colors despite isatty() returning False. For this reason, Loguru uses an additional check that isn't present in Colorama to detect this special case and enable colors when it works.

If within Loguru we have should_colorize() and should_wrap() that both return True, but that we use the stream created by AnsiToWin32 when convert=None and strip=None, then the stream produced won't be colored. This is because in the source code of colorama, have_tty will be False, therefore both strip and convert will be inferred as False. The ansi codes will be stripped, but the Win32 API won't be called (while they should).

But on older Windows systems won't it still strip ANSI codes even with "PYCHARM_HOSTED" because of convert=True?

Yes, but this is the goal. We don't want ansi codes like \x1b[1;31m to appear in the output. On Linux, they're automatically converted to colors by the console. On Windows, we wan't to strip them from the string, and call Win32 API instead to change the colors.

Delgan pushed a commit that referenced this pull request Aug 28, 2023
@Delgan Delgan merged commit d37531b into Delgan:master Aug 29, 2023
@Delgan
Copy link
Owner

Delgan commented Aug 29, 2023

I rebased your branch on master and added a few unit tests to reach 100% coverage.

Thanks for your contribution!

@tunaflsh tunaflsh deleted the windows_terminal_ansi_support branch August 29, 2023 10:01
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.

2 participants