[vim/vim] Fix: 'visualbell' doesn't work (#1789)
ichizok
repro steps
test-environments:
- macOS 10.12.5, Terminal.app
- Ubuntu 16.04, gnome-terminal
vim -Nu NONE -c 'set vb'
and input h, but expected visual-bell (flashing screen) doesn't happen.
details
Vim sends terminal-control-sequences at visualbell event when t_vb is ^[[?5h$<100/>^[[?5l:
expected: CSI ?5h (reverse video) → 100ms delay → CSI ?5l (normal video)
actual: 100ms delay → CSI ?5h (reverse video) → CSI ?5l (normal video)
cause
At visualbell event, tputs() in out_str(T_VB) is called,
and out_char_nf() is called by every characters of T_VB except $<100/> (terminfo string).
$<100/> is interpreted and executed in tputs() in advance of calling out_char_nf().
in tputs():
[100ms delay by "$<100/>"]
out_char_nf('\e')
out_char_nf('[')
out_char_nf('?')
out_char_nf('5')
out_char_nf('h')
out_char_nf('\e')
out_char_nf('[')
out_char_nf('?')
out_char_nf('5')
out_char_nf('l')
proposal of fix
T_VB has to be flushed, at least, before $<...>.
desirable sequence:
tputs("\e[?5h")
out_flush()
tputs("$<100/>\e[?5l")
This patch adds out_str_cf() to check $< in T_VB and to insert flushing.
Ozaki Kiichi
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/1789
Commit Summary
- Add out_str_cf, to fix visualbell
- Flush before delay command
File Changes
- M src/misc1.c (2)
- M src/proto/term.pro (1)
- M src/term.c (49)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
Codecov
Codecov Report
Merging #1789 into master will decrease coverage by
0.02%.
The diff coverage is0%.
@@ Coverage Diff @@ ## master #1789 +/- ## ========================================== - Coverage 75.08% 75.06% -0.03% ========================================== Files 76 76 Lines 125067 125085 +18 ========================================== - Hits 93907 93895 -12 - Misses 31160 31190 +30
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/term.c | 52.41% <0%> (-0.57%) |
⬇️ |
| src/misc1.c | 82.68% <0%> (ø) |
⬆️ |
| src/if_xcmdsrv.c | 85.18% <0%> (-0.93%) |
⬇️ |
| src/gui_gtk_x11.c | 47.39% <0%> (-0.11%) |
⬇️ |
| src/ex_cmds2.c | 79.6% <0%> (-0.11%) |
⬇️ |
| src/channel.c | 83.8% <0%> (-0.1%) |
⬇️ |
| src/evalfunc.c | 81.6% <0%> (-0.03%) |
⬇️ |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 4670490...9ae7892. Read the comment docs.
ichizok
Sorry, I wrote an incorrect content of "cause". I have revised it.
Bram Moolenaar
Ozaki Kiichi wrote:
> ### repro steps
>
> test-environments:
>
> * macOS 10.12.5, Terminal.app
> * Ubuntu 16.04, gnome-terminal
>
> ```
> vim -Nu NONE -c 'set vb'
> ```
>
>
> ### details
>
> Vim sends terminal-control-sequences at visualbell event when `t_vb` is `^[[?5h$<100/>^[[?5l`:
>
> actual: 100ms delay → `CSI ?5h` (reverse video) → `CSI ?5l` (normal video)
>
> ### cause
>
> At visualbell event, `tputs()` in `out_str(T_VB)` is called,
> and `out_char_nf()` is called by every characters of `T_VB` except `$<100/>` (terminfo string).
>
>
> ```
> in tputs():
>
> [100ms delay by "$<100/>"]
> out_char_nf('\e')
> out_char_nf('[')
> out_char_nf('?')
> out_char_nf('5')
> out_char_nf('h')
> out_char_nf('\e')
> out_char_nf('[')
> out_char_nf('?')
> out_char_nf('5')
> out_char_nf('l')
This may depend on the implementation. For me it appears there is no
delay at all.
> ### proposal of fix
>
> `T_VB` has to be flushed, at least, before `$<...>`.
>
> desirable sequence:
>
> ```
> tputs("\e[?5h")
> out_flush()
> tputs("$<100/>\e[?5l")
> ```
>
> This patch adds `out_str_cf()` to check `$<` in `T_VB` and to insert flushing.
making it flash for 200 msec would be a problem. Especially when it
repeats.
How about this solution, using your patch but changing out_str_cf():
void
out_str_cf(char_u *s)
{
#ifdef ELAPSED_FUNC
static int did_init = FALSE;
static ELAPSED_TYPE start_tv;
#endif
if (s != NULL && *s)
{
char_u *p;
#ifdef FEAT_GUI
/* Don't use tputs() when GUI is used, ncurses crashes. */
if (gui.in_use)
{
out_str_nf(s);
return;
}
#endif
if (out_pos > OUT_SIZE - 20)
out_flush();
#ifdef HAVE_TGETENT
for (p = s; *s; ++s)
{
/* flush just before delay command */
if (*s == '$' && *(s + 1) == '<')
{
char_u save_c = *s;
*s = NUL;
tputs((char *)p, 1, TPUTSFUNCAST out_char_nf);
*s = save_c;
# ifdef ELAPSED_FUNC
/* Only sleep once per second, otherwise a sequence of beeps
* would freeze Vim. */
if (!did_init || ELAPSED_FUNC(start_tv) > 1000)
{
do_sleep(100);
ELAPSED_INIT(start_tv);
did_init = TRUE;
}
p = vim_strchr(s, '>');
if (p == NULL)
p = s;
else
++p;
# else
/* Rely on the terminal library to sleep. */
out_flush();
p = s;
# endif
break;
}
}
tputs((char *)p, 1, TPUTSFUNCAST out_char_nf);
#else
while (*s)
out_char_nf(*s++);
#endif
/* For testing we write one string at a time. */
if (p_wd)
out_flush();
}
}
This way we can make the delay configurable with a option, instead of a
weird terminal-dependend escape code.
--
hundred-and-one symptoms of being an internet addict:
85. Choice between paying Compuserve bill and paying for kids education
is a no brainer -- although a bit painful for your kids.
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
ichizok
I updated patch.
- Add
do_visualbell()to adjust the interval of visualbell flashing
It appears this doesn't work for me. I actually liked the tiny flash,
making it flash for 200 msec would be a problem. Especially when it
repeats.
I agree, but I think it would be better not to include specific process for visual-bell into out_str_cf() and better to add new function to control.
I'd like to make out_str_cf() purely handle terminal-strings.
I set 200msec intervals of visualbell. I felt 1sec intervals are less responsiveness of flashing to input (e.g. at key-repeat).
Bram Moolenaar
Thanks. I think we should apply the restriction of once per second also to the normal bell. I don't think anyone counts on lots of bells :-)
Bram Moolenaar
Ozaki Kiichi wrote:
> I updated patch.
>
> * Add `do_visualbell()` to adjust the interval of visualbell flashing
>
> > It appears this doesn't work for me. I actually liked the tiny flash,
> > making it flash for 200 msec would be a problem. Especially when it
> > repeats.
>
> I agree, but I think it would be better not to include specific
> process for visual-bell into `out_str_cf()` and better to add new
> function to control. I'd like to make `out_str_cf()` purely handle
> terminal-strings.
that turn it into a visual bell and then one has to wait for the
flashing to finish.
> I set 200msec intervals of visualbell. I felt 1sec intervals are less
> responsiveness of flashing to input (e.g. at key-repeat).
Half a second seems like a good compromise.
I submitted the change, please check if it works OK now.
--
You can't have everything. Where would you put it?
-- Steven Wright
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
fghsgh
I don't think anyone counts on lots of bells :-)
I do actually, and it took me quite some time to figure out that this was the cause of this (reason for this is below). I am currently using a recompiled version of Vim (with a part of misc1.c commented out) but I would appreciate an option to adjust the half a second delay to, say, 0.
OK, but I think we should also limit the normal bell. I know terminals that turn it into a visual bell and then one has to wait for the flashing to finish.
That is a good reasoning, but even in that case, some other terminals do audible bells. VX Connectbot (Android) even uses vibrate.
In any case, the 200 ms delay for the visual bell could also be configurable. If the first two people who write to this issue are already disagreeing on the length of time, other people will want it differently, too.
In case anyone is interested in the reason why I sometimes hold down a key to hear the bell repeating: the bell sound I use is kind of soothing and repeating it adds rhythm to that. It lets me relax in between the programming work :-). Plus, it's always good to know that no programs are currently slowing down the computer.
John Little
The reason given in the comments for the one per second limit is that some systems or terminals stall while playing a beep, so repeating some action or motion by holding a key down can cause a hang while a key buffer-full of beeps play. I wonder if any such systems are still used or supported. With libcanberra repeating a sound just restarts it and sounds play asynchronously anyway.
Regards, John Little