gh-131878: Fix input of unicode characters with two or more code points in new pyrepl on Windows#131901
Conversation
- with two codepoints or more
Sorry, something went wrong.
|
LGTM. I cannot do a thorough review, but at least confirm, that with this PR I can enter |
Sorry, something went wrong.
tomasr8
left a comment
There was a problem hiding this comment.
Tested with a Czech keyboard and everything works. Thanks!
Sorry, something went wrong.
Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
…epl-for-long-unicode-chars
Had some time now: this needs fixes for Linux. Otherwise looks good, thanks @sergey-miryanov! |
Sorry, something went wrong.
Since this wasn't caught by the tests, we should also probably add a test for that. |
Sorry, something went wrong.
|
I have suggested the |
Sorry, something went wrong.
Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
|
@chris-eibl Please take a look! I would like to keep tests for paste mode to be sure that escape sequences not mixed with input if error occurred. But I'm OK to remove them if you think it is not necessary. |
Sorry, something went wrong.
|
LGTM. Sure, more tests are always better. Though, AFAIU, there is nothing special about paste mode here:
IMHO, both are reasonable, but "paste mode" makes them look too special about pasting? So I'd rename them and use arbitrary "one byte chars" in there. |
Sorry, something went wrong.
|
Sequence of "one byte chars" is a control command to enable "paste mode". Originally in gh-131878 problem aroise from case where the wrongly passed unicode string puts to the buffer and mixed with following control command that disables paste mode and asserted in those code path. So, I want to check that we don't "corrupt" chars buffer. |
Sorry, something went wrong.
|
Yupp, I know. But as said, there is nothing special about bracketed pasting here. From the perspective of the eventqueue test, these are just arbitrary bytes. Whether bracketed pasting is working correctly, is tested in cpython/Lib/test/test_pyrepl/test_pyrepl.py Line 1201 in 1b7470f If you'd like to showcase that sequences of "one byte chars" interleaved with "multi byte chars" work correctly via "paste mode", then maybe just add a comment? And small nit: cpython/Lib/test/test_pyrepl/test_pyrepl.py Lines 1227 to 1228 in 1b7470f To me, those two tests have merit, but are too special about "paste mode". |
Sorry, something went wrong.
|
@chris-eibl It seems that I did not correctly assume how paste-mode works (shame on me!). I removed one test because the same code path is covered by |
Sorry, something went wrong.
|
I don't have a better name - the comment in there clarifies what we intend to test (not much tbh). Otherwise LGTM. Thanks for bearing with me! |
Sorry, something went wrong.
Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
chris-eibl
left a comment
There was a problem hiding this comment.
Thank you @sergey-miryanov!
Sorry, something went wrong.
tomasr8
left a comment
There was a problem hiding this comment.
Tested again after the simplifications. WFM on both Windows and Linux :)
Sorry, something went wrong.
Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
ambv
left a comment
There was a problem hiding this comment.
Gentlemen, excellent work. Not only does it fix the bug, but it makes the codebase simpler and more elegant. If not for the new test, the net number of lines added here would be negative. Impressive!
Sorry, something went wrong.
|
Thanks @sergey-miryanov for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
|
Sorry, @sergey-miryanov and @ambv, I could not cleanly backport this to |
Sorry, something went wrong.
|
@ambv Please take a look - this is the same problem with backport as in earlier PR - #130805 (comment) |
Sorry, something went wrong.
…ore code points in new pyrepl on Windows (pythongh-131901) (cherry picked from commit 0c5151b) Co-authored-by: Sergey Miryanov <sergey.miryanov@gmail.com> Co-authored-by: Tomas R. <tomas.roun8@gmail.com> Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot AMD64 Debian root 3.13 (tier-1) has failed when building commit 891232f. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/#/builders/1441/builds/1169 Summary of the results of the build (if available): Click to see traceback logsremote: Enumerating objects: 26, done.
remote: Counting objects: 3% (1/26)
remote: Counting objects: 7% (2/26)
remote: Counting objects: 11% (3/26)
remote: Counting objects: 15% (4/26)
remote: Counting objects: 19% (5/26)
remote: Counting objects: 23% (6/26)
remote: Counting objects: 26% (7/26)
remote: Counting objects: 30% (8/26)
remote: Counting objects: 34% (9/26)
remote: Counting objects: 38% (10/26)
remote: Counting objects: 42% (11/26)
remote: Counting objects: 46% (12/26)
remote: Counting objects: 50% (13/26)
remote: Counting objects: 53% (14/26)
remote: Counting objects: 57% (15/26)
remote: Counting objects: 61% (16/26)
remote: Counting objects: 65% (17/26)
remote: Counting objects: 69% (18/26)
remote: Counting objects: 73% (19/26)
remote: Counting objects: 76% (20/26)
remote: Counting objects: 80% (21/26)
remote: Counting objects: 84% (22/26)
remote: Counting objects: 88% (23/26)
remote: Counting objects: 92% (24/26)
remote: Counting objects: 96% (25/26)
remote: Counting objects: 100% (26/26)
remote: Counting objects: 100% (26/26), done.
remote: Compressing objects: 33% (1/3)
remote: Compressing objects: 66% (2/3)
remote: Compressing objects: 100% (3/3)
remote: Compressing objects: 100% (3/3), done.
remote: Total 14 (delta 12), reused 12 (delta 11), pack-reused 0 (from 0)
From https://github.com/python/cpython
* branch 3.13 -> FETCH_HEAD
Note: switching to '891232f3386dd8b20a216a473954c1b01cede7ec'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:
git switch -c <new-branch-name>
Or undo this operation with:
git switch -
Turn off this advice by setting config variable advice.detachedHead to false
HEAD is now at 891232f3386 [3.13] gh-131878: Fix input of unicode characters with two or more code points in new pyrepl on Windows (gh-131901) (gh-133468)
Switched to and reset branch '3.13'
configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)
configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
make: [Makefile:3116: clean-retain-profile] Error 1 (ignored) |
Sorry, something went wrong.
…e points in new pyrepl on Windows (pythongh-131901) Co-authored-by: Tomas R. <tomas.roun8@gmail.com> Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
If unicode characters with two or more codepoints (ñ or é) typed or pasted, then it should be converted to bytes before passing to
eventqueue.push.Also fixed handling of exceptions while decoding buffer. If exception occurred then buffer should be flushed to prevent mixing it with following control commands (for example "\x1b[201").