colorama
colorama copied to clipboard
tartley
Adds support for reverse video and underline on Windows 10.
Also prevents SGR codes 30-37 from appearing BRIGHT when the terminal is using a bright colour by default.
closes #265
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.
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.
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!
@wiggin15 Any estimate as to when this PR will be merged? I maintain a project that would benefit from underlining support. Thanks in advance!
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.
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. https://github.com/tartley/colorama/pull/267/commits/752c327260d171e4b7856f0d611583d61352ed2a There are also still conflicts - can you try to rebase your branch on top of master instead of using merge?
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.
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.
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.
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.
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.
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 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 https://github.com/tartley/colorama/pull/267#issuecomment-779188571. 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.
@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.
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!