-
-
Notifications
You must be signed in to change notification settings - Fork 34k
gh-144259: Fix Windows EOL wrap by syncing real console cursor #144297
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
base: main
Are you sure you want to change the base?
Conversation
Windows VT terminals do not consistently wrap the cursor when a line exactly fills the terminal width. Previously we assumed a wrap always happened, which could desynchronize the logical cursor from the real console cursor and break subsequent cursor movement. This change queries the real cursor position and updates posxy accordingly, and adds regression tests for both wrap and no-wrap cases. Signed-off-by: Yongtao Huang <yongtaoh2022@gmail.com>
Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
|
Thanks for the review! I agree with your point, so I’ve removed the VT-specific references and updated the tests to exercise both VT and legacy console modes. |
Fall back gracefully when querying the console cursor fails with an invalid handle, instead of raising an exception. Failed link: https://github.com/python/cpython/actions/runs/21563046527/job/62130029686?pr=144297
|
As you've seen, many Windows tests fail now, because in headless mode Instead of swallowing the error, maybe introduce something like Then, we can simply mock this for most of the tests. And you can also write an explicit test for |
|
Thanks for the suggestion — these failed cases were indeed quite tricky. I decided to follow your proposed approach to fix the failing cases. |
Lib/_pyrepl/windows_console.py
Outdated
| if not GetConsoleScreenBufferInfo(OutHandle, info): | ||
| err = get_last_error() | ||
| if err == 6: # ERROR_INVALID_HANDLE | ||
| return None | ||
| raise WinError(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if not GetConsoleScreenBufferInfo(OutHandle, info): | |
| err = get_last_error() | |
| if err == 6: # ERROR_INVALID_HANDLE | |
| return None | |
| raise WinError(err) | |
| if not GetConsoleScreenBufferInfo(OutHandle, info): | |
| raise WinError(get_last_error()) |
Let's raise again? We can handle failing CI like for
| console._getscrollbacksize = MagicMock(42) |
by adding
console._has_wrapped_to_next_row = MagicMock(False)
That's what I had in mind in #144297 (comment)
Instead of swallowing the error [...] we can simply mock this for most of the tests.
In case you like this suggestion, please also adapt the annotation and docstring. test_returns_none_on_invalid_handle and test_raises_on_unexpected_error are then no longer needed.
Thanks for bearing with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem at all, thanks for the clarification and your help — I’ll keep working on these over the next day or two.
Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
Long log:
Windows VT terminals do not consistently wrap the cursor when a line
exactly fills the terminal width.
Previously we assumed a wrap always happened, which could desynchronize
the logical cursor from the real console cursor and break subsequent
cursor movement.
This change queries the real cursor position and updates posxy
accordingly, and adds regression tests for both wrap and no-wrap cases.
Video with the patch
fix-issue-144259.mp4
Signed-off-by: Yongtao Huang yongtaoh2022@gmail.com