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

Conversion of Segments to Windows Console API calls on legacy Windows platform #1993

Merged
merged 27 commits into from
Mar 9, 2022

Conversation

darrenburns
Copy link
Member

@darrenburns darrenburns commented Feb 22, 2022

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

Currently we depend on Colorama, which converts ANSI to Windows Console API calls in the context of legacy Windows terminals. This change removes the Colorama dependency, and has Rich directly call the Windows Console API.

  • This should speed things up a little on older systems, since Colorama is no longer parsing ANSI codes.
  • This change is implemented at the Segment level - so the calls to the API are done slightly earlier than before.
  • We support a larger surface of the Console API now. For example, control sequences for showing and hiding the cursor are now handled where they previously weren't (we may see less flickering as a result), and the title of the terminal can be set.

Closes #1991

Comparing new vs old

rich.syntax - New on left, old (Colorama) on right

image

rich.color - left column is this branch, right column is master

image
image

rich.palette - left this branch, right master

image

rich.progress - left this branch, right master - no difference in the amount of flickering, however, on master the cursor occasionally flashes on screen.
image

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2022

Codecov Report

Merging #1993 (862416a) into master (75f4637) will decrease coverage by 0.26%.
The diff coverage is 91.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1993      +/-   ##
==========================================
- Coverage   99.81%   99.54%   -0.27%     
==========================================
  Files          71       73       +2     
  Lines        7058     7294     +236     
==========================================
+ Hits         7045     7261     +216     
- Misses         13       33      +20     
Flag Coverage Δ
unittests 99.54% <91.73%> (-0.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rich/segment.py 99.34% <ø> (ø)
rich/console.py 99.63% <88.00%> (-0.37%) ⬇️
rich/_win32_console.py 90.22% <90.22%> (ø)
rich/_windows_renderer.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa4546a...862416a. Read the comment docs.

@USLTD
Copy link
Contributor

USLTD commented Feb 25, 2022

@darrenburns I have a question.
Why do you define wrappers over Win32 API calls? I don't think it is needed

@darrenburns
Copy link
Member Author

@UltraStudioLTD The wrapper abstracts legacy Windows console API calls behind an easier to understand and more Pythonic interface, so that the rest of Rich doesn't need to know anything about legacy Windows APIs. The purpose of this PR is to drop the dependency on Colorama, and handle older Windows systems in a slightly more performative way.

@darrenburns darrenburns marked this pull request as ready for review March 1, 2022 11:45
@darrenburns darrenburns requested a review from willmcgugan March 1, 2022 11:45
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

Great! Just a few requests.

rich/_win32_console.py Outdated Show resolved Hide resolved
rich/_win32_console.py Outdated Show resolved Hide resolved
rich/_win32_console.py Outdated Show resolved Hide resolved
rich/_win32_console.py Outdated Show resolved Hide resolved
@willmcgugan
Copy link
Collaborator

@darrenburns Just looking in to styles a bit more.

According to the colorama docs, it will interpret bold as bright, and also dim as normal intensity. We should probably do the same. Colorama doesn't support the "reverse" style, but I think we could do that quite easily by swapping "fore" and "back". Would you mind making those changes?

@darrenburns
Copy link
Member Author

@willmcgugan Made those changes in #2019

Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Replace Colorama with internal solution for windows legacy mode
4 participants