-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
vim-patch:8.0.0683 #9897
vim-patch:8.0.0683 #9897
Conversation
|
ui_call_visual_bell(); | ||
} else { | ||
ui_call_bell(); | ||
static uint64_t start_time = 0; |
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 we added a counter here like static int beeps = 0
, then we could be a bit smarter about when the beep is skipped. Then we can avoid the delay in the tests. If, say, more than 3 beeps occur within 500ms, then do the delay. And reset the counter if some number of seconds have passed since the last beep.
I'm not really convinced that Nvim has any risk of "freezing" or whatever the original report was about. So adding 1 second to our test runs is kind of annoying.
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.
There are assert_beep
patches that seem to not rely on this patch. Do you prefer making this N/A if the other patches pass? I don't use this bell feature myself.
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.
Actually, is this a problem in the nvim core or the client UI only? Vim didn't decouple theirs so it's easier to do it here.
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.
I think the problem was TUI only, but I agree it's easier to do here and better to keep the behavior consistent across UIs. And I guess it could apply to Nvim TUI like the Vim TUI, so might as well include it, but I think the "counting" approach described above is worth it to avoid time delays in the tests.
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.
I'm not sure I like hard-coding a TUI-specific restriction (that even hasn't been confirmed in nvim TUI) in the abstract UI layer. GUI:s might have various ways of indicating a beep some of which would work well even with more frequent beeps. Though there would be a way of making this workaround more palatable: reset start_time
every time raw input is handled (input_enqueue[_mouse]
). Then if the user presses three or five invalid keys within a second they will still hear a beep after every key, while beeps from macros and such will be throttled.
Problem: When using a visual bell there is no delay, causing the flash to be very short, possibly unnoticeable. Also, the flash and the beep can lockup the UI when repeated often. Solution: Do the delay in Vim or flush the output before the delay. Limit the bell to once per half a second. (Ozaki Kiichi, closes vim/vim#1789) vim/vim@2e147ca
vim-patch:8.0.0683: visual bell flashes too quickly
Problem: When using a visual bell there is no delay, causing the flash to
be very short, possibly unnoticeable. Also, the flash and the
beep can lockup the UI when repeated often.
Solution: Do the delay in Vim or flush the output before the delay. Limit the
bell to once per half a second. (Ozaki Kiichi, closes vim/vim#1789)
vim/vim@2e147ca