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

Fix special ansi sequences not ignored and producing wrong colors #222

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Delgan
Copy link
Contributor

@Delgan Delgan commented May 25, 2019

Hi.

This change is intended to fix #217.

On Linux, special coloration sequences may be initialized with 38 (foreground) and 48 (bakground) ansi codes. 8-bit looks like \x1b[38;5;46m and 24-bit looks like \x1b[38;2;120;33;255m. These colors are not supported by Windows terminal as far as I know (maybe PowerShell console supports it?), so they should be ignored by colorama, as it is the case for others special styles.

Notes about my implementation:

  • I tried to optimize for the main use case which happens when there is no such special sequence. The overhead is just one check against skip truthy value most of the time.
  • There is some edge cases like \x1b[38;0;31m or \x1b[38;48;5;46m. According to my observations on Linux, if the ansi code folowing 38 is not 2 nor 5, the sequence should be ignored (but another one can start immediatly).

Feel free to suggest improvements. 👍

@Delgan Delgan force-pushed the fix_217 branch 2 times, most recently from cfa0ce1 to caaba84 Compare May 25, 2019 10:04
@pfmoore
Copy link

pfmoore commented Jan 27, 2020

hese colors are not supported by Windows terminal as far as I know

Recent versions of the Windows 10 terminal do support them

@tartley
Copy link
Owner

tartley commented Oct 13, 2020

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!

@tartley
Copy link
Owner

tartley commented Oct 7, 2021

Apologies for being AWOL for years, but I'm rounding up PRs that should get merged.

This sounds AMAZING, and I'd like to merge it. Huge thanks! But I can't merge it without corresponding tests (How will future developers know if their changes break your code?)

@Delgan
Copy link
Contributor Author

Delgan commented Nov 28, 2021

Hey @tartley, I just updated the PR by adding unit tests (and slightly modifying the implementation to make it more readable).

Please let me know what you think of it!

@Delgan Delgan force-pushed the fix_217 branch 3 times, most recently from f3b71d1 to 7796819 Compare November 28, 2021 11:33
@tartley tartley added 0.4.6 Get merged for 0.4.6. release and removed 0.4.5 Get merged for 0.4.5 release labels Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.4.6 Get merged for 0.4.6. release requires-thought
Projects
None yet
Development

Successfully merging this pull request may close these issues.

24 bits ANSI codes are wrongly converted instead of being stripped
3 participants