Add Win 10 native ANSI cmd support #105
Conversation
Slayther
changed the title
|
Not sure why |
Slayther
changed the title
| @@ -21,6 +21,20 @@ def is_a_tty(stream): | |||
| return hasattr(stream, 'isatty') and stream.isatty() | |||
|
|
|||
|
|
|||
| def check_win_10_ansi_support(): | |||
There was a problem hiding this comment.
Is it possible to replace the windows release check using the "platform" module? platform.win32_ver() or platform.platform()?
There was a problem hiding this comment.
I haven't found a way to check the build version using the platform module.
There was a problem hiding this comment.
@Slayther Try sys.getwindowsversion()[2].
| # should we strip ANSI sequences from our output? | ||
| if strip is None: | ||
| if strip is None and not win_10_ansi_support: |
There was a problem hiding this comment.
I prefer to add the win10 check inside the if, this way "strip" will always be True/False and not be left as None. Same for 'convert'
| @@ -66,13 +80,16 @@ def __init__(self, wrapped, convert=None, strip=None, autoreset=False): | |||
| # to support the ANSI codes. | |||
| conversion_supported = on_windows and winapi_test() | |||
|
|
|||
| # Does this version of Win 10 support ANSI? | |||
| win_10_ansi_support = on_windows and check_win_10_ansi_support() | |||
There was a problem hiding this comment.
Let's change the name to "windows_ansi_support", in case it will be more than just Windows 10 in the future :)
There was a problem hiding this comment.
Ok, also changed the function name.
|
Well, it seems that the checks assume that Windows needs strip and convert. Not sure what I should do here. @wiggin15 |
| self.strip = strip | ||
|
|
||
| # should we should convert ANSI sequences into win32 calls? | ||
| if convert is None: | ||
| convert = conversion_supported and not is_stream_closed(wrapped) and is_a_tty(wrapped) | ||
| convert = not windows_ansi_support or (conversion_supported and not is_stream_closed(wrapped) and is_a_tty(wrapped)) |
There was a problem hiding this comment.
Are you sure this should be "or"-ed and not "and"-ed? This way if the old condition with conversion_supported is True, then convert will be True even if windows_ansi_support is True, which (I think) is not the desired outcome. I think that there is the same problem with strip (but I may be mistaken). This is a difficult part.
There was a problem hiding this comment.
I'm not really sure. That's why I put the check on the if line at first. I tried several combinations, but couldn't find one that fits for all cases.
There was a problem hiding this comment.
I think we should have separate ifs, one for on_windows and one for other platforms.
| @@ -21,6 +21,23 @@ def is_a_tty(stream): | |||
| return hasattr(stream, 'isatty') and stream.isatty() | |||
|
|
|||
|
|
|||
| def check_windows_ansi_support(): | |||
| try: | |||
| import winreg | |||
There was a problem hiding this comment.
There is a problem here because winreg only exists in Python 3 but we support Python 2 too. Also, this function fails on older versions of Windows because the ReleaseID value doesn't exist in the registry. I am reluctant to pull this function - too much platform specific code has a high chance of breaking things.
There was a problem hiding this comment.
I worked with Python 2 when I encountered the error and fixed it with this check function. Though, I agree it contains a lot of platform specific code, but I don't know what else would work.
There was a problem hiding this comment.
I'm not sure why I can't import it anymore. I am sure it worked in Python 2.7.x
There was a problem hiding this comment.
The module is named _winreg on Python 2.7 (notice the leading underscore). https://docs.python.org/2/library/_winreg.html
|
I mentioned this in the review comments, but, just in case it got skipped over:
def check_windows_ansi_support():
return sys.getwindowsversion()[2] >= 1511 |
|
On Windows 7 this returns True. The check should include the major and minor, not just the build. A tuple comparison should work. |
|
This would be wonderful to have! Powershell (on Windows 10) now supports 24 bit color, but when colorama is initialized, we get dropped back to 16 color mode. Below is the same script, the first run without colorama, the second with colorama initialized.
|
|
Adding to the complexity, this new functionality only works on Powershell, and not on CMD... |
|
The build number is not the same as the ReleaseID, but they correspond: So the code should look like this: def check_windows_ansi_support():
return sys.getwindowsversion()[2] >= 10586 |
|
Hey. FYI, yesterday I created a PR to test releases before we push them to PyPI. When that is merged, I'll be more confident about resuming merges and releases. I'll try to look at this PR soon. Thank you for creating it! |
|
I think this PR is a different way of implementing the same feature as is done by #139, is that right? I have no idea how to decide which is the best way of doing it. I'll have a closer look soon, but if anyone has any insight, please tell. Apologies for ignoring this (and all other PRs) for years. :-D |
|
Also, we'd need corresponding test changes before this could merge. |
|
#139 uses a more direct/reliable strategy to handle the ANSI stuff, so closing this in favor of it |
No description provided.