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

Avoid waiting 10ms after normal key events #655

Closed
wants to merge 2 commits into from

Conversation

sunfishcode
Copy link
Contributor

Avoid waiting 10ms after normal key events, so that we process normal key events quickly when they arrive.

To handle pastes and resize bursts, introduce a "busy" state. When we see events immediately ready after an event, assume it's a paste, and enter the "busy" state. Similarly, after a resize, enter the "busy" state. When the paste or resize burst appears to be done, leave the "busy" state and resume processing normal key events quickly. See the comments in the code for more details.

Also, bump POLL_WAIT to 100ms. The previous change means we're not waiting for POLL_WAIT after normal keyboard input events, so we can afford to make this a little longer, and empirically, 100ms seems to be needed to avoid processing events during a resize burst on my machine.

And, remove the EVENTS_THRESHOLD, which now apppears to be redundant.

Fixes #643.

Avoid waiting 10ms after normal key events, so that we process normal key
events quickly when they arrive.

To handle pastes and resize bursts, introduce a "busy" state. When we see
events immediately ready after an event, assume it's a paste, and enter
the "busy" state. Similarly, after a resize, enter the "busy" state. When
the paste or resize burst appears to be done, leave the "busy" state and
resume processing normal key events quickly. See the comments in the
code for more details.

Also, bump `POLL_WAIT` to 100ms. The previous change means we're not
waiting for `POLL_WAIT` after normal keyboard input events, so we can afford
to make this a little longer, and empirically, 100ms seems to be needed to
avoid processing events during a resize burst on my machine.

And, remove the `EVENTS_THRESHOLD`, which now apppears to be redundant.

Fixes nushell#643.
@sunfishcode
Copy link
Contributor Author

This is a more subtle change than #651, and will take more care to confirm that it works reliably for everyone.

So far, I've tested this on Linux a few different terminals. My understanding of the platform APIs used by crossterm on macOS and Windows is that this should work on them too, however I have not tested them.

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #655 (fdb9955) into main (60b3b62) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #655      +/-   ##
==========================================
- Coverage   49.21%   49.21%   -0.01%     
==========================================
  Files          43       43              
  Lines        7914     7915       +1     
==========================================
  Hits         3895     3895              
- Misses       4019     4020       +1     
Files Coverage Δ
src/engine.rs 5.13% <0.00%> (-0.01%) ⬇️

@fdncred
Copy link
Collaborator

fdncred commented Nov 2, 2023

What's the best way to test this. I have a Mac and can help test.

@fdncred
Copy link
Collaborator

fdncred commented Nov 2, 2023

I tried pasting 16 lines and it's super-fast and worked great. However, when I pasted 165 lines the last 31 lines showed up in the repl and lines before that disappeared. Also, I couldn't get out of the paste mode. I hit return about 10 times and still had the multiline indicator. Seems like it wouldn't accept the line.

@sunfishcode
Copy link
Contributor Author

Yeah, for testing, the main things to test are: simple typing, small and large (> 1024 bytes, which is crossterm's buffer size) pastes, and window resizing. Ideally they should all feel fast.

In theory, the PR here is just changing the timing when events are processed and when repaints happen. It's possible that this is uncovering a pre-existing bug somewhere.

@fdncred fdncred marked this pull request as draft November 3, 2023 12:25
@fdncred
Copy link
Collaborator

fdncred commented Nov 3, 2023

Marking this as draft until the large paste issue is resolved.

@fdncred
Copy link
Collaborator

fdncred commented Dec 6, 2023

Any more thoughts on this @sunfishcode?

@Feel-ix-343
Copy link

Any updates here?

@sunfishcode
Copy link
Contributor Author

No; I'm not likely to get back to this for a while now, so I'll close this. If anyone else wants to continue this, feel free to.

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.

POLL_WAIT hurts user experience
3 participants