-
Notifications
You must be signed in to change notification settings - Fork 7.4k
fix: windows can now paste non-ascii multiline text #8774
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
Conversation
9803d17 to
2ce8cc7
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2617290692
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
c2c624f to
1d604f5
Compare
| } | ||
|
|
||
| #[test] | ||
| fn question_mark_does_not_toggle_during_paste_burst() { |
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.
exists in tui/chat_composer.rs, copying over
joshka-oai
left a comment
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.
LGTM
| const PASTE_BURST_MIN_CHARS: u16 = 3; | ||
| const PASTE_BURST_CHAR_INTERVAL: Duration = Duration::from_millis(8); | ||
| const PASTE_ENTER_SUPPRESS_WINDOW: Duration = Duration::from_millis(120); | ||
| const PASTE_BURST_ACTIVE_IDLE_TIMEOUT: Duration = Duration::from_millis(60); |
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.
Would be good to capture the reason for the differences in these intervals - are they based on testing / docs / ? Do we have more context on why we might increase or decrease? Are they likely to cause differences in operating systems / remote systems / multiplexers?
Maybe just something here that captures why we chose is enough and we can come back to the deeper questions.
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.
In Windows, if we use an 8ms interval, copying long non-ASCII text might cause the Pasted Content Block to be split into multiple chunks, indicating that the paste burst was temporarily exited midway through the paste process. Sometimes, it might even cause line breaks to be recognized as commits. I'm not sure if there's a better way.
The following images show the results caused by two different delays.
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.
Thanks - mostly I want a short summary in the code, so we know why / how to tweak it if needed.
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.
Thanks @occurrent! I added a comment here and made this platform-specific
|
@occurrent thanks again for your contribution here - we really appreciate the time you took to draft this change. I'll make sure you're credited in the next release notes for this PR |


Summary
This PR builds heavily on the work from @occurrent in #8021 - I've only added a small fix, added additional tests, and propagated the changes to tui2.
From the original PR:
Testing