[win32] Use initial cursor size when hiding or showing it #2339
Conversation
olivierphi
force-pushed
the
windows-dont-override-cursor-info
branch
from
386e4fe to
49aa015
Compare
Codecov Report
@@ Coverage Diff @@
## master #2339 +/- ##
==========================================
- Coverage 98.88% 98.71% -0.18%
==========================================
Files 73 73
Lines 7629 7704 +75
==========================================
+ Hits 7544 7605 +61
- Misses 85 99 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
By doing this the users get their original cursor style back when Rich gives control back to the terminal, rather than a "full size" one
olivierphi
force-pushed
the
windows-dont-override-cursor-info
branch
from
49aa015 to
a7124ea
Compare
| ) | ||
| def test_cursor_position(_): | ||
| @pytest.fixture | ||
| def win32_console_getters(): |
There was a problem hiding this comment.
I replaced the 20+ repeated occurrences of @patch.object(_win32_console, "GetConsoleScreenBufferInfo", return_value=StubScreenBufferInfo) with a Pytest fixture, which should scale better if we add more patching-that-is-common-to-most-tests in the future .
At the moment this fixture is in charge of patching the previous GetConsoleScreenBufferInfo, but also the new GetConsoleCursorInfo 🙂
| ) as GetConsoleScreenBufferInfo, mock.patch.object( | ||
| _win32_console, "GetConsoleCursorInfo", side_effect=stub_console_cursor_info | ||
| ) as GetConsoleCursorInfo: | ||
| yield { |
There was a problem hiding this comment.
the fixture yields the created mocks, which is useful for tests that few need to override the default behaviour :-)
rich/_win32_console.py
Outdated
| @@ -340,6 +364,12 @@ def __init__(self, file: "IO[str]") -> None: | |||
| default_text = GetConsoleScreenBufferInfo(handle).wAttributes | |||
| self._default_text = default_text | |||
|
|
|||
| # Store the current cursor size | |||
| # @link https://docs.microsoft.com/en-us/windows/console/console-cursor-info-str | |||
| initial_cursor_info = CONSOLE_CURSOR_INFO() | |||
There was a problem hiding this comment.
I don't think we should be storing the cursor size in the constructor. Consider if the user changes the cursor size after this object has been created (hardly likely, but possible). Now if they call hide_cursor or show_cursor the cursor size may be incorrect.
We should add this to hide_cursor and show_cursor. i.e. get the cursor info first, modify the visible flag, and then set the cursor info in a single operation.
olivierphi
force-pushed
the
windows-dont-override-cursor-info
branch
from
3d7cea4 to
08ea255
Compare
By doing this the users get their original cursor style back when Rich gives control back to the terminal, rather than a "full size" one
Fixes #2333
Type of changes
Checklist
Description
Before the fix:
We can see the cursor is no longer the same (converted to a "thick" one) after Rich gives control back to the terminal:
Command.Prompt.2022-06-14.15-39-25.mp4
After the fix:
Cursor-after-Rich is well and truly the same than cursor-before-Rich:
Command.Prompt.2022-06-14.15-38-47.mp4
(I also tested with Powershell, and the fix looks to work there too :-)