Prevent an extra character is inserted #1940
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1940 +/- ##
==========================================
- Coverage 74.64% 74.64% -0.01%
==========================================
Files 77 77
Lines 125988 125990 +2
Branches 28199 29858 +1659
==========================================
- Hits 94044 94043 -1
- Misses 31940 31943 +3
Partials 4 4
Continue to review full report at Codecov.
|
|
Ozaki Kiichi wrote:
On GVim, An extra character is inserted to command-line mode when focus state is changed in timer callback.
* But I'm worried this patch has another side effects :(
[repro step]
* confirmed Vim 8.0.0862 on Ubuntu 16.04, Windows 10
test.vim:
```vim
if has('win32')
let s:cmd = 'path'
elseif executable('xterm')
let s:cmd = 'xterm -e "sleep 1"'
endif
fu! T(...)
call system(s:cmd)
if winnr('$') > 1
wincmd p
else
split
endif
endfu
call timer_start(0, 'T')
```
`gvim --clean -S test.vim`
* Another window shows, executes command, and closes. (*)
* And then input `/` (or `:`, to enter command-line), so `/b` at command line. Thus, an extra character `b` is appended to `/`.
I cannot reproduce this.
In addition: `FocusLost` and `FocusGained` don't happen at (*) on GVim.
[cause]
* Finishing another window, GUI focus-gained event (from system) calls `gui_focus_change` and stacks bytes (`CSI KS_EXTRA KE_FOCUSGAINED`) into `inbuf`.
* (`wincmd p` or `split`) `vgetorpeek` is called in `wincmd`. After 8.0.0670, `vgetorpeek` called in timer-callback is run beyond [if-clause](https://github.com/vim/vim/blob/master/src/getchar.c#L1987) (`if (vgetc_busy > 0 && ex_normal_busy == 0)`).
* Therefore `typebuf` retrieves `inbuf` data:
```
#0 0x00000000005e5fde in read_from_input_buf (buf=0x8ff65a <typebuf_init+58> "\233\375b~tr\t\200kb\200kb/tr\tvim_\ti\t\r/\200kb:so\200ku\r/", maxlen=3) at ui.c:1758
#1 0x00000000005e3c65 in ui_inchar (buf=0x8ff65a <typebuf_init+58> "\233\375b~tr\t\200kb\200kb/tr\tvim_\ti\t\r/\200kb:so\200ku\r/", maxlen=68, wtime=0, tb_change_cnt=68
) at ui.c:187
#2 0x00000000004babc5 in inchar (buf=0x8ff65a <typebuf_init+58> "\233\375b~tr\t\200kb\200kb/tr\tvim_\ti\t\r/\200kb:so\200ku\r/", maxlen=206, wait_time=0, tb_change_cnt=
68) at getchar.c:3065
#3 0x00000000004ba78f in vgetorpeek (advance=0) at getchar.c:2841
#4 0x00000000004b8d5c in vpeekc () at getchar.c:1842
#5 0x00000000004b8e08 in char_avail () at getchar.c:1898
#6 0x00000000004f7efb in update_mouseshape (shape_idx=-1) at misc2.c:3774
#7 0x00000000005da435 in setmouse () at term.c:3532
#8 0x00000000005ff95c in win_enter_ext (wp=0xe386e0, undo_sync=1, curwin_invalid=0, trigger_new_autocmds=0, trigger_enter_autocmds=1, trigger_leave_autocmds=1)
at window.c:4476
#9 0x00000000005ff565 in win_enter (wp=0xe386e0, undo_sync=1) at window.c:4348
#10 0x00000000005ff169 in win_goto (wp=0xe386e0) at window.c:4167
#11 0x00000000005f8b69 in do_window (nchar=119, Prenum=0, xchar=0) at window.c:266
#12 0x0000000000486cae in ex_wincmd (eap=0x7ffd88bcb380) at ex_docmd.c:9310
(snip)
Further down in the stack trace, does it have
gui_wait_for_chars_or_timer()? There it bails out:
if (typebuf.tb_change_cnt != tb_change_cnt)
Perhaps even further down in the stack this is not used?
* then:
```
typebuf.tb_len = 3
typebuf.tb_buf[typebuf.tb_off + 0] = '\233'
typebuf.tb_buf[typebuf.tb_off + 1] = '\375'
typebuf.tb_buf[typebuf.tb_off + 2] = 'b'
```
* `typebuf` is not consumed after timer-callback `T`. Then input `/`:
```
typebuf.tb_len = 3
typebuf.tb_buf[typebuf.tb_off + 0] = '/'
typebuf.tb_buf[typebuf.tb_off + 1] = '\000'
typebuf.tb_buf[typebuf.tb_off + 2] = 'b'
```
Thus `/b` is displayed.
This indicates something is put in the typebuf ignoring the change
count. Where does that happen?
[solution]
* Set `typebuf_was_filled` to `TRUE` in `vgetorpeek` if `timer_busy >
0` in order to comsume `typebuf`.
I don't think changing vgetorpeek() is the right solution.
…--
Proofread carefully to see if you any words out.
/// Bram Moolenaar -- Bram@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
changed the title
Changing GUI focus by system() in timer callback and then entering command line mode, an extra character is inserted to command line.
|
Sorry for the delayed reply. I updated the patch. [reproducible sample] test2.vim let g:focus = []
augroup test
au!
au FocusGained * let g:focus += ['gained']
au FocusLost * let g:focus += ['lost']
augroup END
function! T(...)
"echo 'Start'
sleep 5
"echo 'Finish'
call getchar(1)
endfunction
call timer_start(3000, 'T')steps:
Additionally; [the points of this issue]
In the abose case, first [solution proposal]
|
ichizok
changed the title
|
Sorry for the delayed reply. I updated the patch.
[reproducible sample]
test2.vim
```vim
let g:focus = []
augroup test
au!
au FocusGained * let g:focus += ['gained']
au FocusLost * let g:focus += ['lost']
augroup END
function! T(...)
"echo 'Start'
sleep 5
"echo 'Finish'
call getchar(1)
endfunction
call timer_start(3000, 'T')
```
steps:
1. `gvim --clean`
2. `:so test2.vim`
3. Switch GUI focus to another application before timer-callback `T` starts.
4. Return the focus to GVim during `sleep 5` (before `getchar(1)` is called).
5. After timer-callback finished, type `:`, then `:b` shows.
Additionally;
`FocusLost` event occurs by step 3, but `FocusGained` does not by step 4. (Please check `g:focus`)
[the points of this issue]
* GUI focus event caught during timer-callback called in `vgetc()` (NOT in `sleep`)
* `vpeek()` (`vgetorpeek(FALSE)`) called by timer-callback called in `vgetc()`
```
vgetorpeek(0)
-> vpeek
-> f_getchar (getchar(1))
-> check_due_timer
-> vgetorpeek(1)
-> vgetc
```
In the abose case, first `vgetorpeek(0)` moves `inbuf` contents (focus-gained byte sequence) to
`typebuf` by `read_from_input_buf`, but this contents doesn't consumed.
Therefore `typebuf.tb_buf` is overwritten partly by user-input `:` while previous contents have been
valid.
[solution proposal]
* In `check_due_timer`, set `typebuf_was_filled` to indicate `typebuf` changed in timer-callback
when `did_one` and `typebuf.tb_len > 0` are true.
Hmm, both using sleep and getchar() in a timer are unusual, at least.
But I assume the sleep here is just to be able to reproduce the problem,
it would not be there in an actual timer function.
Perhaps the problem is that tb_change_cnt is not updated? That's the
flag that makes many functions return. It is incremented in
ins_typebuf() but not when the timer calls getchar(). So when the
deepest vgetorpeek() calls inchar() then characters are added to the
typebuf, but the vgetorpeek() higher up is still pointing to the
original position in typebuf. That's where it should bail out and
return.
…--
`The Guide says there is an art to flying,' said Ford, `or at least a
knack. The knack lies in learning how to throw yourself at the ground
and miss.' He smiled weakly.
-- Douglas Adams, "The Hitchhiker's Guide to the Galaxy"
/// Bram Moolenaar -- Bram@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 ///
|
Yes, Additional (and more concise) sampletest3.vim: fu! T(...)
sleep 3
call getchar(1)
endfu
call timer_start(0, 'T')This is reproducible on CUI.
Detailsenvironment: Vim 8.0.1013 on Ubuntu 16.04
input
After timer-callback finished;
then the processing stays still in
When user-input occurs, the processing returns to
My conclusionWhen the input is peeked and not consumed in timer-callback, it needs to set the flag meant |
|
> Hmm, both using sleep and getchar() in a timer are unusual, at least.
> But I assume the sleep here is just to be able to reproduce the problem,
> it would not be there in an actual timer function.
Yes, `sleep` and `getchar` are not essential.
[additional (and more concise) sample]
test3.vim:
```vim
fu! T(...)
sleep 3
call getchar(1)
endfu
call timer_start(0, 'T')
```
This is reproducible on CUI.
1. `vim --clean -S test3.vim`
2. Input any 3 characters, e.g. `aaa`, while `sleep`
3. After timer-callback finished, input `:`, then `:a` shows
[details]
environment: Vim 8.0.1013 on Ubuntu 16.04
`vgetorpeek(FALSE)` in `getchar(1)`:
```
#0 0x00000000004b8e4e in vgetorpeek (advance=0) at getchar.c:1942
#1 0x00000000004b8d3b in vpeekc () at getchar.c:1842
#2 0x00000000004b8d9b in vpeekc_any () at getchar.c:1875
#3 0x000000000044f429 in f_getchar (argvars=0x7ffcfa5273d0, rettv=0x7ffcfa5275b0) at evalfunc.c:4319
#4 0x0000000000449046 in call_internal_func (name=0xe7f5f0 "getchar", argcount=1, argvars=0x7ffcfa5273d0, rettv=0x7ffcfa5275b0) at evalfunc.c:1008
#5 0x00000000005f4f60 in call_func (funcname=0xe7f610 "getchar", len=7, rettv=0x7ffcfa5275b0, argcount_in=1, argvars_in=0x7ffcfa5273d0, argv_func=
0x0, firstline=1, lastline=1, doesrange=0x7ffcfa527578, evaluate=1, partial=0x0, selfdict_in=0x0) at userfunc.c:1446
#6 0x00000000005f2da9 in get_func_tv (name=0xe7f610 "getchar", len=7, rettv=0x7ffcfa5275b0, arg=0x7ffcfa527580, firstline=1, lastline=1, doesrange=0x7ffcfa527578, evaluate=1, partial=0x0, selfdict=0x0) at userfunc.c:455
#7 0x00000000005f8d52 in ex_call (eap=0x7ffcfa5276d0) at userfunc.c:3082
#8 0x000000000047b587 in do_one_cmd (cmdlinep=0x7ffcfa5278f0, sourcing=1, cstack=0x7ffcfa5279e0, fgetline=0x5f918e <get_func_line>, cookie=0xe7ed10) at ex_docmd.c:2952
#9 0x000000000047803f in do_cmdline (cmdline=0x0, fgetline=0x5f918e <get_func_line>, cookie=0xe7ed10, flags=7) at ex_docmd.c:1089
#10 0x00000000005f4004 in call_user_func (fp=0xe7ec20, argcount=1, argvars=0x7ffcfa528510, rettv=0x7ffcfa528500, firstline=0, lastline=0, selfdict=0x0)
at userfunc.c:942
#11 0x00000000005f4ec6 in call_func (funcname=0x1031e60 "T", len=1, rettv=0x7ffcfa528500, argcount_in=1, argvars_in=0x7ffcfa528510, argv_func=
0x0, firstline=0, lastline=0, doesrange=0x7ffcfa5284fc, evaluate=1, partial=0x0, selfdict_in=0x0) at userfunc.c:1427
#12 0x00000000004703da in timer_callback (timer=0x1031e00) at ex_cmds2.c:1177
#13 0x000000000047059e in check_due_timer () at ex_cmds2.c:1232
#14 0x000000000053e24f in WaitForChar (msec=4000, interrupted=0x7ffcfa528654, ignore_input=0) at os_unix.c:5735
#15 0x000000000053847d in mch_inchar (buf=0x8e775d <typebuf_init+61> "", maxlen=67, wtime=-1, tb_change_cnt=8) at os_unix.c:489
#16 0x00000000005e6703 in ui_inchar (buf=0x8e775d <typebuf_init+61> "", maxlen=67, wtime=-1, tb_change_cnt=8) at ui.c:195
#17 0x00000000004baba4 in inchar (buf=0x8e775d <typebuf_init+61> "", maxlen=203, wait_time=-1, tb_change_cnt=8) at getchar.c:3065
#18 0x00000000004ba76e in vgetorpeek (advance=1) at getchar.c:2841
#19 0x00000000004b8770 in vgetc () at getchar.c:1608
#20 0x00000000004b8cb4 in safe_vgetc () at getchar.c:1804
#21 0x000000000050680f in normal_cmd (oap=0x7ffcfa528a50, toplevel=1) at normal.c:628
#22 0x0000000000644e68 in main_loop (cmdwin=0, noexmode=0) at main.c:1371
#23 0x0000000000644571 in vim_main2 () at main.c:910
#24 0x0000000000643c24 in main (argc=4, argv=0x7ffcfa528c48) at main.c:419
```
input `aaa` is stored to `typebuf` from `inbuf`.
`typebuf.tb_change_cnt` is not changed in `vgetorpeek(FALSE)`, and I think this behavior is valid.
Actually, I think it is not. WaitForChar() already has a check for
tb_change_cnt, in case feedkeys() was used. Calling getchar(1) is
similar, in that it causes typeahead to be added. feedkeys() does both:
increments tb_change_cnt and sets typebuf_was_filled.
But tb_change_cnt should not be incremented too early, then inchar()
bails out. Best is to do it in inchar(). Let me do that, please check
if everything works OK.
```
#0 0x00000000005e8a61 in read_from_input_buf (buf=0x8e775d <typebuf_init+61> "000", maxlen=3) at ui.c:1758
#1 0x00000000005384ab in mch_inchar (buf=0x8e775d <typebuf_init+61> "000", maxlen=67, wtime=0, tb_change_cnt=8) at os_unix.c:502
#2 0x00000000005e6703 in ui_inchar (buf=0x8e775d <typebuf_init+61> "000", maxlen=67, wtime=0, tb_change_cnt=8) at ui.c:195
#3 0x00000000004baba4 in inchar (buf=0x8e775d <typebuf_init+61> "000", maxlen=203, wait_time=0, tb_change_cnt=8) at getchar.c:3065
#4 0x00000000004ba76e in vgetorpeek (advance=0) at getchar.c:2841
#5 0x00000000004b8d3b in vpeekc () at getchar.c:1842
#6 0x00000000004b8d9b in vpeekc_any () at getchar.c:1875
#7 0x000000000044f429 in f_getchar (argvars=0x7ffcfa5273d0, rettv=0x7ffcfa5275b0) at evalfunc.c:4319
(snip)
```
`check_due_timer()` is called in `WaitForChar()`.
After timer-callback finished;
* input (`inbuf`) is empty
* `typebuf.tb_change_cnt` is not changed
then the processing stays still in `mch_inchar()` until next input occurs.
```
#0 0x00007ff8e3fd2573 in __select_nocancel () at ../sysdeps/unix/syscall-template.S:84
#1 0x000000000053e7e9 in RealWaitForChar (fd=0, msec=4000, check_for_gpm=0x0, interrupted=0x7ffcfa528654) at os_unix.c:6086
#2 0x000000000053e3a5 in WaitForCharOrMouse (msec=4000, interrupted=0x7ffcfa528654, ignore_input=0) at os_unix.c:5817
#3 0x000000000053e29e in WaitForChar (msec=4000, interrupted=0x7ffcfa528654, ignore_input=0) at os_unix.c:5743
#4 0x000000000053847d in mch_inchar (buf=0x8e775d <typebuf_init+61> "000", maxlen=67, wtime=-1, tb_change_cnt=8) at os_unix.c:489
#5 0x00000000005e6703 in ui_inchar (buf=0x8e775d <typebuf_init+61> "000", maxlen=67, wtime=-1, tb_change_cnt=8) at ui.c:195
#6 0x00000000004baba4 in inchar (buf=0x8e775d <typebuf_init+61> "000", maxlen=203, wait_time=-1, tb_change_cnt=8) at getchar.c:3065
#7 0x00000000004ba76e in vgetorpeek (advance=1) at getchar.c:2841
#8 0x00000000004b8770 in vgetc () at getchar.c:1608
#9 0x00000000004b8cb4 in safe_vgetc () at getchar.c:1804
#10 0x000000000050680f in normal_cmd (oap=0x7ffcfa528a50, toplevel=1) at normal.c:628
#11 0x0000000000644e68 in main_loop (cmdwin=0, noexmode=0) at main.c:1371
#12 0x0000000000644571 in vim_main2 () at main.c:910
#13 0x0000000000643c24 in main (argc=4, argv=0x7ffcfa528c48) at main.c:419
```
`typebuf` state is:
```
typebuf.tb_len = 3
typebuf.tb_buf[typebuf.tb_off + 0] = 'a'
typebuf.tb_buf[typebuf.tb_off + 1] = 'a'
typebuf.tb_buf[typebuf.tb_off + 2] = 'a'
```
When user-input occurs, the processing returns to `vgetorpeek(TRUE)` and `typebuf` has been
overwritten:
```
typebuf.tb_len = 3
typebuf.tb_buf[typebuf.tb_off + 0] = ':'
typebuf.tb_buf[typebuf.tb_off + 1] = '\000'
typebuf.tb_buf[typebuf.tb_off + 2] = 'a'
```
`del_typebuf()` changes `typebuf.tb_change_cnt` and consumes `typebuf`.
```
#0 0x00000000004b7dc9 in del_typebuf (len=1, offset=0) at getchar.c:1233
#1 0x00000000004b99f5 in vgetorpeek (advance=1) at getchar.c:2402
#2 0x00000000004b8770 in vgetc () at getchar.c:1608
#3 0x00000000004b8cb4 in safe_vgetc () at getchar.c:1804
#4 0x000000000050680f in normal_cmd (oap=0x7ffcfa528a50, toplevel=1) at normal.c:628
#5 0x0000000000644e68 in main_loop (cmdwin=0, noexmode=0) at main.c:1371
#6 0x0000000000644571 in vim_main2 () at main.c:910
#7 0x0000000000643c24 in main (argc=4, argv=0x7ffcfa528c48) at main.c:419
```
[my conclusion]
When the input is peeked and not consumed in timer-callback, it needs
to set the flag meant
`typebuf` changed (i.e. `typebuf_was_filled`), after timer-callback finishes.
Thanks for taking the time to figure this out.
Could this be turned into a test? That is, without the sleep.
…--
hundred-and-one symptoms of being an internet addict:
34. You laugh at people with a 10 Mbit connection.
/// Bram Moolenaar -- Bram@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 ///
|
https://github.com/vim/vim/compare/master...ichizok:test/feed_input.diff
|
|
pull-req. #2046 |
adizero
pushed a commit
to adizero/vim
that referenced
this pull request
May 19, 2018
Problem: When a timer calls getchar(1) input is overwritten. Solution: Increment tb_change_cnt in inchar(). (closes vim#1940)
janlazo
added a commit
to janlazo/neovim
that referenced
this pull request
Jul 10, 2018
Problem: When a timer calls getchar(1) input is overwritten. Solution: Increment tb_change_cnt in inchar(). (closes vim/vim#1940) vim/vim@0f0f230
janlazo
added a commit
to janlazo/neovim
that referenced
this pull request
Jul 20, 2018
Problem: When a timer calls getchar(1) input is overwritten. Solution: Increment tb_change_cnt in inchar(). (closes vim/vim#1940) vim/vim@0f0f230
janlazo
added a commit
to janlazo/neovim
that referenced
this pull request
Aug 22, 2018
Problem: When a timer calls getchar(1) input is overwritten. Solution: Increment tb_change_cnt in inchar(). (closes vim/vim#1940) vim/vim@0f0f230
janlazo
added a commit
to janlazo/neovim
that referenced
this pull request
Aug 24, 2018
Problem: When a timer calls getchar(1) input is overwritten. Solution: Increment tb_change_cnt in inchar(). (closes vim/vim#1940) vim/vim@0f0f230
janlazo
added a commit
to janlazo/neovim
that referenced
this pull request
Sep 4, 2018
Problem: When a timer calls getchar(1) input is overwritten. Solution: Increment tb_change_cnt in inchar(). (closes vim/vim#1940) vim/vim@0f0f230
timeyyy
pushed a commit
to timeyyy/neovim
that referenced
this pull request
Sep 23, 2018
Problem: When a timer calls getchar(1) input is overwritten. Solution: Increment tb_change_cnt in inchar(). (closes vim/vim#1940) vim/vim@0f0f230
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On GVim, An extra character is inserted to command-line mode when focus state is changed in timer callback.
[repro step]
test.vim:
gvim --clean -S test.vim/(or:, to enter command-line), so/bat command line. Thus, an extra characterbis appended to/.In addition:
FocusLostandFocusGaineddon't happen at (*) on GVim.[cause]
gui_focus_changeand stacks bytes (CSI KS_EXTRA KE_FOCUSGAINED) intoinbuf.wincmd porsplit)vgetorpeekis called inwincmd. After 8.0.0670,vgetorpeekcalled in timer-callback is run beyond if-clause (if (vgetc_busy > 0 && ex_normal_busy == 0)).typebufretrievesinbufdata:typebufis not consumed after timer-callbackT. Then input/:Thus
/bis displayed.[solution]
typebuf_was_filledtoTRUEinvgetorpeekiftimer_busy > 0in order to comsumetypebuf.Ozaki Kiichi