From b6296f4e84217adaa3326c715d4e2c82a105bc39 Mon Sep 17 00:00:00 2001 From: Scott Prager Date: Fri, 3 Apr 2015 18:44:26 -0400 Subject: [PATCH 1/4] terminal: Handle loss of focus in event loop. While in a terminal and insert mode, if an event caused loss of focus, nvim would stay in the terminal event loop causing an inconsistent view of internal state and/or segfault. Remove the "term" argument from terminal_enter() as it only makes sense to call it with curbuf->terminal. Terminate the loop when switched to a different buffer. fixes #2301 --- src/nvim/edit.c | 2 +- src/nvim/terminal.c | 15 ++++++--- test/functional/terminal/buffer_spec.lua | 40 +++++++++++++++++++++++- 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/src/nvim/edit.c b/src/nvim/edit.c index 8b2ac1943f..c60d987ddd 100644 --- a/src/nvim/edit.c +++ b/src/nvim/edit.c @@ -246,7 +246,7 @@ edit ( ) { if (curbuf->terminal) { - terminal_enter(curbuf->terminal, true); + terminal_enter(true); return false; } diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index daba7b943f..9f4d81be19 100644 --- a/src/nvim/terminal.c +++ b/src/nvim/terminal.c @@ -353,8 +353,11 @@ void terminal_resize(Terminal *term, uint16_t width, uint16_t height) invalidate_terminal(term, -1, -1); } -void terminal_enter(Terminal *term, bool process_deferred) +void terminal_enter(bool process_deferred) { + Terminal *term = curbuf->terminal; + assert(term && "should only be called when curbuf has a terminal"); + checkpcmark(); setpcmark(); int save_state = State; @@ -373,7 +376,7 @@ void terminal_enter(Terminal *term, bool process_deferred) int c; bool close = false; - for (;;) { + while (term->buf == curbuf) { if (process_deferred) { event_enable_deferred(); } @@ -431,7 +434,7 @@ end: invalidate_terminal(term, term->cursor.row, term->cursor.row + 1); mapped_ctrl_c = save_mapped_ctrl_c; unshowmode(true); - redraw(false); + redraw(term->buf != curbuf); ui_busy_stop(); if (close) { term->opts.close_cb(term->opts.data); @@ -1018,6 +1021,11 @@ static void refresh_screen(Terminal *term) static void redraw(bool restore_cursor) { + Terminal *term = curbuf->terminal; + if (!term) { + restore_cursor = true; + } + int save_row, save_col; if (restore_cursor) { // save the current row/col to restore after updating screen when not @@ -1040,7 +1048,6 @@ static void redraw(bool restore_cursor) showruler(false); - Terminal *term = curbuf->terminal; if (term && is_focused(term)) { curwin->w_wrow = term->cursor.row; curwin->w_wcol = term->cursor.col + win_col_off(curwin); diff --git a/test/functional/terminal/buffer_spec.lua b/test/functional/terminal/buffer_spec.lua index 857679c4b3..0756508a4c 100644 --- a/test/functional/terminal/buffer_spec.lua +++ b/test/functional/terminal/buffer_spec.lua @@ -2,7 +2,9 @@ local helpers = require('test.functional.helpers') local Screen = require('test.functional.ui.screen') local thelpers = require('test.functional.terminal.helpers') local feed, clear, nvim = helpers.feed, helpers.clear, helpers.nvim -local wait, execute, eq = helpers.wait, helpers.execute, helpers.eq +local wait = helpers.wait +local eval, execute, source = helpers.eval, helpers.execute, helpers.source +local eq, neq = helpers.eq, helpers.neq describe('terminal buffer', function() @@ -155,5 +157,41 @@ describe('terminal buffer', function() :bnext | ]]) end) + + it('handles loss of focus gracefully', function() + -- Temporarily change the statusline to avoid printing the file name, which + -- varies be where the test is run. + nvim('set_option', 'statusline', '==========') + execute('set laststatus=0') + + -- Save the buffer number of the terminal for later testing. + local tbuf = eval('bufnr("%")') + + source([[ + function! SplitWindow() + new + endfunction + + startinsert + call jobstart(['sh', '-c', 'exit'], {'on_exit': function("SplitWindow")}) + ]]) + + -- We should be in a new buffer now. + screen:expect([[ + ^ | + ~ | + ========== | + rows: 2, cols: 50 | + {2: } | + {1:========== }| + | + ]]) + + neq(tbuf, eval('bufnr("%")')) + execute('quit') -- Should exit the new window, not the terminal. + eq(tbuf, eval('bufnr("%")')) + + execute('set laststatus=1') -- Restore laststatus to the default. + end) end) From b8ae09b3cf990c9a374a46c644abe0191e09f2f8 Mon Sep 17 00:00:00 2001 From: Scott Prager Date: Sun, 5 Apr 2015 13:20:08 -0400 Subject: [PATCH 2/4] term: after , resume normal input loop Pressing and then a mouse click will insert the click into the terminal as if a keyboard button had been pressed. Keep track of whether the last input was and only call terminal_send_key() if the next input is a key press. --- src/nvim/terminal.c | 20 ++++++++++++-------- test/functional/terminal/buffer_spec.lua | 7 +++++-- test/functional/terminal/mouse_spec.lua | 14 ++++++++++++++ 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index 9f4d81be19..ed1a8f32b3 100644 --- a/src/nvim/terminal.c +++ b/src/nvim/terminal.c @@ -376,6 +376,8 @@ void terminal_enter(bool process_deferred) int c; bool close = false; + bool got_bs = false; // True if the last input was + while (term->buf == curbuf) { if (process_deferred) { event_enable_deferred(); @@ -388,14 +390,6 @@ void terminal_enter(bool process_deferred) } switch (c) { - case Ctrl_BSL: - c = safe_vgetc(); - if (c == Ctrl_N) { - goto end; - } - terminal_send_key(term, c); - break; - case K_LEFTMOUSE: case K_LEFTDRAG: case K_LEFTRELEASE: @@ -416,12 +410,22 @@ void terminal_enter(bool process_deferred) event_process(); break; + case Ctrl_N: + if (got_bs) { + goto end; + } + default: + if (c == Ctrl_BSL && !got_bs) { + got_bs = true; + break; + } if (term->closed) { close = true; goto end; } + got_bs = false; terminal_send_key(term, c); } } diff --git a/test/functional/terminal/buffer_spec.lua b/test/functional/terminal/buffer_spec.lua index 0756508a4c..ffdfec4428 100644 --- a/test/functional/terminal/buffer_spec.lua +++ b/test/functional/terminal/buffer_spec.lua @@ -170,15 +170,18 @@ describe('terminal buffer', function() source([[ function! SplitWindow() new + call feedkeys("iabc\") endfunction startinsert call jobstart(['sh', '-c', 'exit'], {'on_exit': function("SplitWindow")}) + call feedkeys("\", 't') " vim will expect , but be exited out of + " the terminal before it can be entered. ]]) -- We should be in a new buffer now. screen:expect([[ - ^ | + ab^c | ~ | ========== | rows: 2, cols: 50 | @@ -188,7 +191,7 @@ describe('terminal buffer', function() ]]) neq(tbuf, eval('bufnr("%")')) - execute('quit') -- Should exit the new window, not the terminal. + execute('quit!') -- Should exit the new window, not the terminal. eq(tbuf, eval('bufnr("%")')) execute('set laststatus=1') -- Restore laststatus to the default. diff --git a/test/functional/terminal/mouse_spec.lua b/test/functional/terminal/mouse_spec.lua index b8f6214f8f..4def4dd7f8 100644 --- a/test/functional/terminal/mouse_spec.lua +++ b/test/functional/terminal/mouse_spec.lua @@ -50,6 +50,20 @@ describe('terminal mouse', function() ]]) end) + it('will exit focus after , then scrolled', function() + feed('') + feed('<0,0>') + screen:expect([[ + line23 | + line24 | + line25 | + line26 | + line27 | + ^line28 | + | + ]]) + end) + describe('with mouse events enabled by the program', function() before_each(function() thelpers.enable_mouse() From 013bd4461d95341fbd7c2f742844666a5e966f94 Mon Sep 17 00:00:00 2001 From: Scott Prager Date: Sun, 5 Apr 2015 16:07:54 -0400 Subject: [PATCH 3/4] term: use window col offset to calculate width fixes #2317 --- src/nvim/terminal.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index ed1a8f32b3..136aa0e401 100644 --- a/src/nvim/terminal.c +++ b/src/nvim/terminal.c @@ -338,7 +338,7 @@ void terminal_resize(Terminal *term, uint16_t width, uint16_t height) // terminal in the current tab. FOR_ALL_WINDOWS_IN_TAB(wp, curtab) { if (!wp->w_closing && wp->w_buffer == term->buf) { - width = (uint16_t)MIN(width, (uint16_t)wp->w_width); + width = (uint16_t)MIN(width, (uint16_t)(wp->w_width - win_col_off(wp))); height = (uint16_t)MIN(height, (uint16_t)wp->w_height); } } @@ -358,6 +358,9 @@ void terminal_enter(bool process_deferred) Terminal *term = curbuf->terminal; assert(term && "should only be called when curbuf has a terminal"); + // Ensure the terminal is properly sized. + terminal_resize(term, 0, 0); + checkpcmark(); setpcmark(); int save_state = State; From 8cac2eea751379e0195b5160f9d14d19f8866e71 Mon Sep 17 00:00:00 2001 From: Scott Prager Date: Mon, 6 Apr 2015 17:47:08 -0400 Subject: [PATCH 4/4] term: ensure term->buf is valid The fallowing test (reduced), submitted by @mhinz may free term->buf, leaving the pointer dangling. ```vim let s:buf = -1 function! s:exit_handler() execute 'bdelete!' s:buf endfunction vnew let s:buf = bufnr('%') let id = termopen('sleep 1', { 'on_exit': function('s:exit_handler') }) call s:test() ``` When the buffer is known to be closing, set term->buf to NULL, and check buf_valid() in on_refresh(). Helped-by: Marco Hinz (@mhinz) --- src/nvim/terminal.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index 136aa0e401..d603a6a877 100644 --- a/src/nvim/terminal.c +++ b/src/nvim/terminal.c @@ -307,9 +307,12 @@ void terminal_close(Terminal *term, char *msg) term->forward_mouse = false; term->closed = true; if (!msg || exiting) { - // If no msg was given, this was called by close_buffer(buffer.c) so we - // should not wait for the user to press a key. Also cannot wait if - // `exiting == true` + // If no msg was given, this was called by close_buffer(buffer.c). Or if + // exiting, we must inform the buffer the terminal no longer exists so that + // close_buffer() doesn't call this again. + term->buf->terminal = NULL; + term->buf = NULL; + // We should not wait for the user to press a key. term->opts.close_cb(term->opts.data); } else { terminal_receive(term, msg, strlen(msg)); @@ -451,7 +454,9 @@ end: void terminal_destroy(Terminal *term) { - term->buf->terminal = NULL; + if (term->buf) { + term->buf->terminal = NULL; + } term->buf = NULL; pmap_del(ptr_t)(invalidated_terminals, term); for (size_t i = 0 ; i < term->sb_current; i++) { @@ -930,8 +935,13 @@ static void on_refresh(Event event) // be used act on terminal output. block_autocmds(); map_foreach(invalidated_terminals, term, stub, { - if (!term->buf) { + // TODO(SplinterOfChaos): Find the condition that makes term->buf invalid. + bool valid = true; + if (!term->buf || !(valid = buf_valid(term->buf))) { // destroyed by `close_buffer`. Dont do anything else + if (!valid) { + term->buf = NULL; + } continue; } bool pending_resize = term->pending_resize;