Adds support for reverse video and underline on Windows 10. #267

jpwroberts · 2020-07-07T14:55:29Z

Also prevents SGR codes 30-37 from appearing BRIGHT when the terminal is using a bright colour by default.

closes #265

Also prevents SGR codes 30-37 from appearing BRIGHT when the terminal is using a bright colour by default.

jpwroberts · 2020-07-15T09:23:03Z

Hi,
This is my first time contributing to an open source project, and I'm not sure about the process. Is there anything further I should be doing?
I found a paragraph starting "After you're happy with the proposed changes, you can merge the pull request", in the documentation, but it seems strange that I'd have the necessary access to do that. I'm reluctant to experiment on a production system.

wiggin15 · 2020-07-16T09:10:57Z

Hi @jpwroberts. Thanks for the pull request!
Regarding the process, your PR is ok and it needs to be reviewed and accepted, and then merged, by the project maintainers (which are @tartley and myself). You don't need to do any merges yourself.
Unfortunately, lately we had less time to go over contributions and review them, especially with recent global events. Hopefully we'll get the chance to review this soon.

jpwroberts · 2020-07-17T13:18:38Z

Thanks for clarifying. I understand, these are chaotic times.

tartley · 2020-10-13T14:05:38Z

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!

jpwroberts · 2020-10-13T15:20:40Z

Thanks for the update :-)

jtesta · 2021-02-05T22:29:28Z

@wiggin15 Any estimate as to when this PR will be merged? I maintain a project that would benefit from underlining support. Thanks in advance!

wiggin15 · 2021-02-06T15:44:47Z

CHANGELOG.rst

+  * ANSI codes CSI [ 0-37 m no longer render as BRIGHT if the default terminal
+    foreground color is BRIGHT.
+  * The demo01.py file now demonstrates reverse video and underlining effects.
+  * winterm_test.py updated to assert on term._default_style rather than _style.


I'm not sure this change is prominent enough to appear in the changelog. It's more "internal".

I may have been a bit over-zealous there. Definitely agree about the last two not being in the changelog. The first two result in a change in behaviour, especially no longer getting a BRIGHT foreground colour with CSI [ 30-37 m. That should be documented somewhere, but I'm happy to remove it from the changelog.

README.rst

wiggin15 · 2021-02-06T16:01:21Z

README.rst

-    Style: DIM, NORMAL, BRIGHT, RESET_ALL
+    Style: DIM, NORMAL, BRIGHT, BRIGHT_OFF, REVERSE, UNDERLINE, RESET_ALL
+
+``Style.REVERSE_OFF`` and ``Style.UNDERLINE_OFF`` are provided to allow intent


If I understand correctly, regular ANSI-compliant terminals do have a special code for REVERSE_OFF and UNDERLINE_OFF, but Windows doesn't have an attribute to "clear" these.
Does this mean that we'll have different behaviors on Windows/Linux? e.g. the following two calls
print(colorama.Style.REVERSE_OFF + colorama.Back.GREEN + "off?" + colorama.Style.RESET_ALL)
and
print(colorama.Style.REVERSE_OFF + colorama.Style.REVERSE_OFF + colorama.Back.GREEN + "off?" + colorama.Style.RESET_ALL)
will have "REVERSE" off in both on Linux, but will have "REVERSE" on in the first call on Windows?
Maybe we can call a different function, or use a special value in WinStyle to make sure OFF means OFF?

That's the behaviour I expected (and intended) but there seems to be a problem there. I'll work on that (after rebasing to the current version).
I think the Windows behaviour can be fixed by tracking the current state of REVERSE and UNDERLINE. I'll make sure OFF really means off (and ON doesn't mean OFF if the attribute is already set).

README.rst

wiggin15 · 2021-02-06T16:31:58Z

Thanks for nudging me, @jtesta .
This PR is pretty good. I left some comments that I think may need attention. There is also a small conflict with the latest code, so it needs to be rebased / updated. Once these issues are addressed I can merge the PR.

wiggin15 · 2021-02-14T17:36:31Z

Thanks for updating the PR. If I understand correctly, this is a different implementation that before, and we don't use the Win32 calls any more?

This commit contains changes from the already-current master branch (looks like it's a merge commit) - is it supposed to look like this? It's harder to review and I don't know what merging it like this will mean. 752c327
There are also still conflicts - can you try to rebase your branch on top of master instead of using merge?

colorama/ansitowin32.py

wiggin15 · 2021-02-14T17:44:49Z

colorama/ansitowin32.py

+            old_mode = wintypes.DWORD()
+
+            stdout_handle = windll.kernel32.GetStdHandle(STDOUT)
+            # If the next call failed, we may be running on Windows pre 10.0.10586.


If I understand correctly, this call isn't supposed to fail (GetConsoleMode existed in prior versions), but it will not return the ENABLE_VIRTUAL_TERMINAL_PROCESSING flag which only exists on later versions. The "set" call with the flag may fail, though. Please update the comments to reflect this.

Actually, I think there's more than just the comment to change. The only way to be sure that we've enabled the virtual terminal is to call GetConsoleMode again, and check. Only if that test passes should I set conversion_supported to False.
To be safe, I need to find an old version of Windows 10 to test on. I'll also test on Cygwin.

wiggin15 · 2021-02-14T17:46:08Z

colorama/ansitowin32.py

+                        print("Stored old_mode_value = " + str(old_mode_value))
+                    # Only enable if it's not already enabled
+                    print("enabling virtual terminal for " + str(self.stream))
+                    if windll.kernel32.SetConsoleMode(stdout_handle, old_mode_value | ENABLE_VIRTUAL_TERMINAL_PROCESSING):


I am a bit reluctant to do this here. Normally "init" itself doesn't change any console properties, and everything is controllable with the init flags. This call is now made implicitly and in any case.

I can't think of a cleaner way to do it, unfortunately. We're basically putting Windows into the same mode as Linux, and avoiding win32 calls. As far as I can see, that has to be done here.

wiggin15 · 2021-02-14T17:54:30Z

colorama/ansitowin32.py

        on_windows = os.name == 'nt'
        # We test if the WinAPI works, because even if we are on Windows
        # we may be using a terminal that doesn't support the WinAPI
        # (e.g. Cygwin Terminal). In this case it's up to the terminal
        # to support the ANSI codes.
-        conversion_supported = on_windows and winapi_test()
+        if on_windows and platform.release().startswith('10'):


The comment above this line mismatches the code now. We do need to call winapi_test before using the other WinAPI functions inside the "if".

wiggin15 · 2021-02-14T17:56:23Z

colorama/ansitowin32.py

+            # We turn virtual terminal support back on, then set
+            # `conversion_supported` to False. Behaviour will then be the same
+            # as for Linux - colorama doesn't use win32 calls.
+            from ctypes import wintypes, byref  # import wintypes now we know we're on Windows.


Please move relevant code to win32.py, that's where we check the imports and define the ctypes functions.

After reading #300 I checked through the outstanding PRs. #139 by @segevfiner has used win32.py. That PR also enables vt via a function in winterm.py (as you suggested I should do). Once I've set up appropriate test environments (old Windows 10, Cygwin, etc), I'll run the same tests using #139. That may be a better candidate for implementing this functionality. @segevfiner identified vt as a more appropriate solution while I was still trying to make win32 calls behave.

jpwroberts · 2021-02-14T23:55:17Z

Thanks for updating the PR. If I understand correctly, this is a different implementation that before, and we don't use the Win32 calls any more?

This commit contains changes from the already-current master branch (looks like it's a merge commit) - is it supposed to look like this? It's harder to review and I don't know what merging it like this will mean. 752c327
There are also still conflicts - can you try to rebase your branch on top of master instead of using merge?

It looks like I messed up there, sorry.
That's correct about not using Win32 calls on Windows 10 versions that support a virtual terminal. As the changes I'm implementing aren't supported on Windows versions that don't have a vt, it makes no sense to try to use Win32 calls to provide the functionality that the vt gives us.

wiggin15 · 2021-02-15T07:31:26Z

After thinking about this further, I think the new approach is less than ideal, because it leaves older operating systems unsupported for specific new codes. colorama's purpose is to add support for terminals that don't support ANSI codes using Win32 API calls.

segevfiner · 2021-02-15T07:36:28Z

This might mean you need both. As the Win 10 VT support enables support for additional escape sequences that aren't supported by the WinAPI such as 24-bit colors.

jpwroberts · 2021-02-15T12:22:54Z

After thinking about this further, I think the new approach is less than ideal, because it leaves older operating systems unsupported for specific new codes. colorama's purpose is to add support for terminals that don't support ANSI codes using Win32 API calls.

That's what I started off doing, then it became apparent that older Windows versions just don't have the necessary API calls to support the new codes I wanted to add (reverse video and underline).
This approach doesn't prevent adding new functionality to older operating systems - nothing has changed for them. We're just effectively treating Windows 10 the same as Linux.
I agree that we shouldn't do anything that disadvantages the older operating systems. If, for example, we discover a way to support strikethrough on Windows 7, that can be added to colorama in the usual way - regardless of this change.

jpwroberts · 2021-02-15T21:54:18Z

It looks like I have a lot to learn about using github. I've just managed to accidentally close this PR while trying to delete all my previous commits.
I've made the changes suggested by @wiggin15 including moving appropriate code into functions in win32.py. But I'm really struggling to rebase this PR on top of master.
Unless someone is able to correctly rebase it, I think my best option is to open a new PR. I have local copies of all my proposed changes.
I've tested the change successfully on an old version of Windows 10 (10586) and also on Cygwin.

tartley · 2021-10-07T04:00:54Z

Hi folks. I'm looking at Colorama stuff after many many months (years?) away. @jpwroberts, would you like some help getting this PR into shape? Do you think the basic approach is sound, and can solve (or already has solved) the issues @wiggin15 raised here? Or have you given up on it? Thoughts appreciated.

jpwroberts · 2021-10-07T07:08:13Z

Hi folks. I'm looking at Colorama stuff after many many months (years?) away. @jpwroberts, would you like some help getting this PR into shape? Do you think the basic approach is sound, and can solve (or already has solved) the issues @wiggin15 raised here? Or have you given up on it? Thoughts appreciated.

Hi,
Great :-).
Yes, I believe my new approach addresses most of the issues. I've got code that works for Windows 10 versions that support the virtual terminal (enabling it if it's supported but disabled). Other operating systems and Windows versions work as before (which means there's limited support on Windows 8 or earlier). @wiggin15 expressed uncertainty about this approach, and I replied in #267 (comment).
My problem is unfamiliarity with pull requests. I managed to make a mess of this one, and am unsure how to rebase it on top of master as @wiggin15 requested. I have a lot to learn about using github.
I can provide my code in some other way (a zip file perhaps) if that will help. Or if anyone has suggestions how we can get this PR into the right shape, I can continue from there.

tartley · 2021-10-07T15:02:04Z

@jpwroberts OK, thanks for the info. I'm sure I can help out with git/github and this PR. I'll take a look at it later. Many thanks for your efforts and your patience.

tartley · 2022-10-16T20:39:19Z

Hi. This PR has a laudable goal but cannot be merged in its current state (it contains diffs for many disparate changes and a large number of conflicts.) I'm closing it. Apologies for the extent to which my inattention has contributed to the difficulty of sorting this out. Hugs!

jpwroberts added 3 commits July 7, 2020 16:53

Adds support for reverse video and underline on Windows 10.

6ee65e5

Also prevents SGR codes 30-37 from appearing BRIGHT when the terminal is using a bright colour by default.

Incorrectly left BRIGHT_OFF in the documentation.

e0f6ea3

Updated screenshots (plus minor formatting changes)

7eb3dcb

jpwroberts mentioned this pull request Jul 14, 2020

Colors STOPPED showing in Windows 10, latest update. #265

Open

jtesta mentioned this pull request Feb 5, 2021

Man Page on Windows jtesta/ssh-audit#82

Closed

wiggin15 requested changes Feb 6, 2021

View reviewed changes

wiggin15 mentioned this pull request Feb 10, 2021

Better colorama maintenance & support is needed #300

Open

jpwroberts added 2 commits February 14, 2021 00:13

Changed to use Windows virtual terminal on Windows 10

752c327

Left diagnostic print statements in the code

6ee11c1

wiggin15 requested changes Feb 14, 2021

View reviewed changes

jpwroberts closed this Feb 15, 2021

jpwroberts deleted the reverse-and-underline branch February 15, 2021 21:32

jpwroberts restored the reverse-and-underline branch February 15, 2021 21:52

jpwroberts reopened this Feb 15, 2021

tartley added the requires-thought label Oct 7, 2021

tartley mentioned this pull request Oct 7, 2021

Support foreground/background reversing #129

Open

tartley closed this Oct 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for reverse video and underline on Windows 10. #267

Adds support for reverse video and underline on Windows 10. #267

jpwroberts commented Jul 7, 2020 •

edited

jpwroberts commented Jul 15, 2020

wiggin15 commented Jul 16, 2020

jpwroberts commented Jul 17, 2020

tartley commented Oct 13, 2020 •

edited

jpwroberts commented Oct 13, 2020

jtesta commented Feb 5, 2021

wiggin15 Feb 6, 2021

jpwroberts Feb 7, 2021

wiggin15 Feb 6, 2021 •

edited

jpwroberts Feb 7, 2021

wiggin15 commented Feb 6, 2021

wiggin15 commented Feb 14, 2021

wiggin15 Feb 14, 2021

jpwroberts Feb 14, 2021

wiggin15 Feb 14, 2021

jpwroberts Feb 14, 2021

wiggin15 Feb 14, 2021

wiggin15 Feb 14, 2021

jpwroberts Feb 14, 2021

jpwroberts commented Feb 14, 2021 •

edited

wiggin15 commented Feb 15, 2021

segevfiner commented Feb 15, 2021

jpwroberts commented Feb 15, 2021

jpwroberts commented Feb 15, 2021 •

edited

tartley commented Oct 7, 2021

jpwroberts commented Oct 7, 2021

tartley commented Oct 7, 2021

tartley commented Oct 16, 2022

Adds support for reverse video and underline on Windows 10. #267

Adds support for reverse video and underline on Windows 10. #267

Conversation

jpwroberts commented Jul 7, 2020 • edited

jpwroberts commented Jul 15, 2020

wiggin15 commented Jul 16, 2020

jpwroberts commented Jul 17, 2020

tartley commented Oct 13, 2020 • edited

jpwroberts commented Oct 13, 2020

jtesta commented Feb 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiggin15 Feb 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiggin15 commented Feb 6, 2021

wiggin15 commented Feb 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpwroberts commented Feb 14, 2021 • edited

wiggin15 commented Feb 15, 2021

segevfiner commented Feb 15, 2021

jpwroberts commented Feb 15, 2021

jpwroberts commented Feb 15, 2021 • edited

tartley commented Oct 7, 2021

jpwroberts commented Oct 7, 2021

tartley commented Oct 7, 2021

tartley commented Oct 16, 2022

jpwroberts commented Jul 7, 2020 •

edited

tartley commented Oct 13, 2020 •

edited

wiggin15 Feb 6, 2021 •

edited

jpwroberts commented Feb 14, 2021 •

edited

jpwroberts commented Feb 15, 2021 •

edited