[RFC] Refactor Neovim to remove the side effects of `K_EVENT` #3413

tarruda · 2015-10-02T14:42:49Z

This PR is my new attempt to remove the effects of the internal K_EVENT key code, which is used to represent events in various modes that explicitly declare the ability to do so. While using a special key to signal events is relatively safe(because Vim is a state machine that only deals with keys), it has a couple of disadvantages:

Some operators can be interrupted
Mappings are affected

Previously I have attempted to remove this key by always handling events in the most inner place of Neovim loops: the input.c module, which created instability since we can't handle events in every state.

Fixing this issue is a bigger deal now because terminal redrawing is now handled as deferred events: 47cbbc0

Close #3382 (and possibly many other issues I can't find right now)

oni-link · 2015-10-04T18:56:27Z

src/nvim/normal.c

+    if (s->noexmode) {
+      return false;
+    }
+    do_exmode(exmode_active == EXMODE_VIM);


If !s->cmdwin || cmdwin_result == 0 is true then after the call do_exmode(), normal_cmd() is now always called. That was not the case in the old code.

oni-link · 2015-10-11T00:59:51Z

CursorHold is now retriggered (every 'updatetime' ms) without pressing a key:

vimrc:

set updatetime=100

let g:x=0

function! Update()
    let g:x=g:x+1
    echom "CursorHoldI ".g:x
    call jobstart('echo')
endfunction

function! Update2()
    let g:x=g:x+1
    echom "CursorHold ".g:x
    call jobstart('echo') 
endfunction

autocmd CursorHoldI * call Update()
autocmd CursorHold * call Update2()

Script used with nvim -u vimrc.

tarruda · 2015-10-12T10:16:28Z

CursorHold is now retriggered (every 'updatetime' ms) without pressing a key:

👍 Fixed

tarruda · 2015-10-19T11:47:52Z

@ZyX-I I'm having trouble determining why a certain shada test is failing in this PR, more specifically on commit tarruda@a4be270 (main: Refactor main_loop to call os_inchar directly).

The failing test can be run with:

TEST_FILTER='correctly ignores case' TEST_FILE=test/functional/shada/shada_spec.lua make test

and has the following failed assertion:

Failure → test/functional/shada/shada_spec.lua @ 189
ShaDa support code correctly ignores case with shada-r option
test/functional/shada/shada_spec.lua:200: Expected objects to be the same.
Passed in:
(table) {
  [7] = 1
  [8] = 2
  [9] = 1
  [10] = 4
 *[11] = 1 }
Expected:
(table) {
  [7] = 1
  [8] = 2
  [9] = 1
  [10] = 4
 *[11] = 2 }

That is, key 11 has a expected value of 2 but the actual value was 1. Since I'm having trouble to quickly parse the relevant shada test, can you explain what is the meaning of key 11?

ZyX-I · 2015-10-19T14:04:13Z

@tarruda This key is a number of recorded changelist items in the ShaDa file for the file given as the argument to find_file. Basically this means that one change has disappeared from change list for some reason.

From a very high level point of view, Vim/Nvim can be described as state machines following these instructions in a loop: - Read user input - Peform some action. The action is determined by the current state and can switch states. - Possibly display some feedback to the user. This is not immediately visible because these instructions spread across dozens of nested loops and function calls, making it very hard to modify the state machine(to support more event types, for example). So far, the approach Nvim has taken to allow more events is this: - At the very core function that blocks for input, poll for arbitrary events. - If the event received from the OS is user input, just return it normally to the callers. - If the event is not direct result of user input(possibly a vimscript function call coming from a msgpack-rpc socket or a job control callback), return a special key code(`K_EVENT`) that is handled by callers where it is safer to perform arbitrary actions. One problem with this approach is that the `K_EVENT` signal is being sent across multiple states that may be unaware of it. This was partially fixed with the `input_enable_events`/`input_disable_events` functions, which were added as a mechanism that the upper layers can use to tell the core input functions that it is ready to accept `K_EVENT`. Another problem is that the mapping engine is implemented in getchar.c which is called from every state, but the mapping engine is not aware of `K_EVENT` so events can break mappings. While it is theoretically possible to modify getchar.c to make it aware of `K_EVENT`, this commit fixes the problem with a different approach: Model Nvim as a pushdown automaton(https://en.wikipedia.org/wiki/Pushdown_automaton). This design has many advantages which include: - Decoupling the event loop from the states reponsible for handling events. - Better control of state transition with less dependency on global variable hacks(eg: 'restart_edit' global variable). - Easier removal of global variables and function splitting. That is because many variables are for state-specific information, and probably ended up being global to simplify communication between functions, which we fix by storing state-specific information in specialized structures. The final goal is to let Nvim have a single top-level event loop represented by the following pseudo-code: ``` while not quitting let event = read_event current_state(event) update_screen() ``` This closely mirrors the state machine description above and makes it easier to understand, extend and debug the program. Note that while the pseudo code suggests an explicit stack of states that doesn't rely on return addresses(as suggested by the principles of automata-based programming: https://en.wikipedia.org/wiki/Automata-based_programming), for now we'll use the call stack as a structure to manage state transitioning as it would be very difficult to refactor Nvim to use an explicit stack of states, and the benefits would be small. While this change may seem like an endless amount of work, it is possible to do it incrementally as was shown in the previous commits. The general procedure is: 1- Find a blocking `vgetc()`(or derivatives) call. This call represents an implicit state of the program. 2- Split the code before and after the `vgetc()` call into functions that match the signature of `state_check_callback` and `state_execute_callback. Only `state_execute_callback` is required. 3- Create a `VimState` "subclass" and a initializer function that sets the function pointers and performs any other required initialization steps. If the state has no local variables, just use `VimState` without subclassing. 4- Instead of calling the original function containing the `vgetc()`, initialize a stack-allocated `VimState` subclass, then call `state_enter` to begin processing events in the state. 5- The check/execute callbacks can return 1 to continue normally, 0 to break the loop or -1 to skip to the next iteration. These callbacks contain code that execute before and after the old `vgetc()` call. The functions created in step 2 may contain other `vgetc()` calls. These represent implicit sub-states of the current state, but it is fine to remove them later in smaller steps since we didn't break compatibility with existing code.

Split most code in `normal_check` in: - `normal_check_stuff_buffer` - `normal_check_interrupt` - `normal_check_cursor_moved` - `normal_check_text_changed` - `normal_check_folds` - `normal_redraw`

- `normal_handle_special_visual_command` - `normal_need_aditional_char` - `normal_get_additional_char` - `normal_invert_horizontal`

- `normal_need_redraw_mode_message` - `normal_redraw_mode_message`

Begin refactoring edit() into a state that can be managed by the `state_enter()`: - Move local variables into a local InsertState structure - Fix code style in the entire function.

Refactor insert mode to use `state_enter` as an event loop: - Move o_lnum(static variable) outside function - Move code before the insert mode loop into `insert_enter` - Move code before `safe_vgetc()` call into `insert_check` - Move code after `safe_vgetc()` call into `insert_execute` - Remove doESCkey label and handle insert mode repeating in the `insert_enter` function - Remove do_intr label(this is not the place for platform-specific interrupt charts)

- `insert_handle_key`: Contains the big insert mode switch statement. - `insert_do_complete`: Code that used to be in the `docomplete` label. - `insert_do_cindent`: Code that used to be in the `force_cindent` label. Also move some code after the switch statement into the beginning of `insert_check`.

- Create `TerminalState` structure containing data used in terminal mode - Extract `terminal_execute` from `terminal_enter` and use it with `state_enter`.

Begin refactoring getcmdline() into a state that can be managed by the `state_enter()`: - Move local variables into a local CommandLineState structure - Fix code style in the entire function.

Split `getcmdline()` into command_line_{enter,check,execute}`

oni-link · 2015-10-26T14:06:37Z

The use of intr_char inside of a #ifdef UNIXwas removed in some cases in the refactoring. Do we still need it? Can this global variable be removed?

tarruda · 2015-10-26T14:08:23Z

The use of intr_char inside of a #ifdef UNIXwas removed in some cases in the refactoring. Do we still need it? Can this global variable be removed?

The global variable is not set and always has a value of 0, so I guess it can be removed safely

tarruda · 2015-10-26T14:09:43Z

In any case, it's not like we want to have platform-specific code in core functions. This translation should happen in the TUI(if it should happen at all)

justinmk · 2015-10-26T14:48:01Z

The real issue is that in some situations, no other information about what is on the screen is kept except for what is already on ScreenLines and friends.

Understood, though in the case of nnoremap gl :ls<CR>:buffer<Space> where the pager is showing with Press-Enter prompt, I believe we do have flags that can already be checked.

For reference, this issue is covered by #3416

marvim added the WIP label Oct 2, 2015

tarruda force-pushed the remove-kevent branch 2 times, most recently from be6bf2d to 503a19f Compare October 2, 2015 19:02

justinmk mentioned this pull request Oct 3, 2015

RPC request (or other K_EVENT) causes redraw #3416

Open

tarruda force-pushed the remove-kevent branch from 503a19f to 9986e7b Compare October 3, 2015 14:09

tarruda added this to the 0.1-first-public-release milestone Oct 4, 2015

tarruda force-pushed the remove-kevent branch 2 times, most recently from d8bfa57 to 30dddcf Compare October 4, 2015 15:09

oni-link reviewed Oct 4, 2015
View reviewed changes

tarruda force-pushed the remove-kevent branch 9 times, most recently from 42d31aa to 3f063f8 Compare October 6, 2015 14:15

tarruda force-pushed the remove-kevent branch from 26fdc61 to 3a3813b Compare October 12, 2015 10:16

tarruda force-pushed the remove-kevent branch 5 times, most recently from 8aadcc8 to 4ff7fb3 Compare October 19, 2015 11:04

tarruda added 15 commits October 26, 2015 10:52

normal: Split normal_check into multiple functions

fa83b03

Split most code in `normal_check` in: - `normal_check_stuff_buffer` - `normal_check_interrupt` - `normal_check_cursor_moved` - `normal_check_text_changed` - `normal_check_folds` - `normal_redraw`

normal: Extract some functions from normal_execute

0f0fae5

- `normal_handle_special_visual_command` - `normal_need_aditional_char` - `normal_get_additional_char` - `normal_invert_horizontal`

normal: Extract normal_get_command_count from normal_execute

5abd25f

normal: Extract normal_finish_command from normal_execute

4c6e417

normal: Extract some functions from normal_finish_command

f1bc3e4

- `normal_need_redraw_mode_message` - `normal_redraw_mode_message`

edit: Extract local variables from edit() and fix code style

f5b333f

Begin refactoring edit() into a state that can be managed by the `state_enter()`: - Move local variables into a local InsertState structure - Fix code style in the entire function.

terminal: Refactor to use state_enter

344bd64

- Create `TerminalState` structure containing data used in terminal mode - Extract `terminal_execute` from `terminal_enter` and use it with `state_enter`.

ex_getln: Extract local variables from getcmdline() and fix code style

52d4978

Begin refactoring getcmdline() into a state that can be managed by the `state_enter()`: - Move local variables into a local CommandLineState structure - Fix code style in the entire function.

ex_getln: refactor command line mode to use the state_enter loop

0701e1b

Split `getcmdline()` into command_line_{enter,check,execute}`

ex_getln: Convert cmdline_{not_,}changed labels into functions

c0c562f

ex_getln: Extract command_line_handle_key from command_line_execute

ed985d0

Start documenting code

b4c82db

tarruda force-pushed the remove-kevent branch from 1aec658 to 9aa8ab6 Compare October 26, 2015 13:52

edit.c: Fix one clint warning

1726c7d

tarruda force-pushed the remove-kevent branch from 9aa8ab6 to 1726c7d Compare October 26, 2015 14:06

tarruda merged commit 1726c7d into neovim:master Oct 26, 2015

tarruda added a commit that referenced this pull request Oct 26, 2015

Merge PR #3413 'Refactor Neovim to remove the side effects of K_EVENT'

29d64a9

jszakmeister removed the RFC label Oct 26, 2015

This was referenced Oct 26, 2015

vim_input changed? #3282

Closed

Leader keys and some other key sequences not working with showcmd rogual/neovim-dot-app#112

Closed

tarruda mentioned this pull request Oct 26, 2015

operator-pending mode interrupted by redraw or job/event #3382

Closed

justinmk mentioned this pull request Oct 28, 2015

"Autotrigger" should be optional SirVer/ultisnips#593

Closed

critiqjo mentioned this pull request Nov 18, 2015

Vim-command output disappears when a terminal buffer gets updated #2503

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Refactor Neovim to remove the side effects of `K_EVENT` #3413

[RFC] Refactor Neovim to remove the side effects of `K_EVENT` #3413

tarruda commented Oct 2, 2015

oni-link Oct 4, 2015

tarruda Oct 5, 2015

oni-link commented Oct 11, 2015

tarruda commented Oct 12, 2015

tarruda commented Oct 19, 2015

ZyX-I commented Oct 19, 2015

oni-link commented Oct 26, 2015

tarruda commented Oct 26, 2015

tarruda commented Oct 26, 2015

justinmk commented Oct 26, 2015

[RFC] Refactor Neovim to remove the side effects of K_EVENT #3413

[RFC] Refactor Neovim to remove the side effects of K_EVENT #3413

Conversation

tarruda commented Oct 2, 2015

oni-link Oct 4, 2015

Choose a reason for hiding this comment

tarruda Oct 5, 2015

Choose a reason for hiding this comment

oni-link commented Oct 11, 2015

tarruda commented Oct 12, 2015

tarruda commented Oct 19, 2015

ZyX-I commented Oct 19, 2015

oni-link commented Oct 26, 2015

tarruda commented Oct 26, 2015

tarruda commented Oct 26, 2015

justinmk commented Oct 26, 2015

[RFC] Refactor Neovim to remove the side effects of `K_EVENT` #3413

[RFC] Refactor Neovim to remove the side effects of `K_EVENT` #3413