[vim] :tabedit clears command-line (#630)

[Miroslav Koskar]

When doing :edit file, :split file, and :vsplit file last message
is always present in command-line but that is not the case for
:tabedit file where message blinks and disappears.

Same goes for :tab ... e.g., :tab help.

It seems like switching to new tab in those cases does equivalent of
:redraw! where :redraw might be just enough and not cause
command-line to be cleared.

—
Reply to this email directly or view it on GitHub.

[Christian Brabandt]

can you please give a reproducible example, starting with vim -u NONE -N ?

[Miroslav Koskar]

$ touch test.txt
$ vim -u NONE -N -c 'tabedit test.txt'
$ vim -u NONE -N -c 'tab help'

Notice "test.txt" 0L, 0C / "help.txt" [readonly] 225L, 8382C in command-line.

$ vim -u NONE -N
:edit test.txt
:split test.txt
:vsplit test.txt

Notice "test.txt" 0L, 0C in command-line.

:tabedit test.txt
:tab help

Command-line is empty

[Miroslav Koskar]

Interesting, I don't see those.

Well, I've retested now and figured out that when I open new urxvt for the first time I don't see it, but then subsequent invocations I do. Interesting, indeed.

Do you see it on :mess? I suppose, there is a redraw happing sometime, which prevents the output.

Yes I do see it there, with my full VIM config I see it even flashing there so yes, there is an equivalent of :redraw! happening it seems. Question is if just :redraw wouldn't be enough in this case.

[Jason Franklin]

I noticed this just the other day, and I was motivated to fix it.

The problem here is pretty easy to find:

The error in timing occurs in the ex_splitview() function. Ultimately, the win_new_tabpage() function invokes redraw_all_later() with the CLEAR redraw type. This will remove the fileinfo() message from the screen before we complete the main loop.

The patch here fixes the problem:

diff --git a/src/window.c b/src/window.c
index b9617f037..da93e25e9 100644
--- a/src/window.c
+++ b/src/window.c
@@ -3702,7 +3702,7 @@ win_new_tabpage(int after)
        entering_window(curwin);
 #endif
 
-       redraw_all_later(CLEAR);
+       redraw_all_later(NOT_VALID);
        apply_autocmds(EVENT_WINNEW, NULL, NULL, FALSE, curbuf);
        apply_autocmds(EVENT_WINENTER, NULL, NULL, FALSE, curbuf);
        apply_autocmds(EVENT_TABNEW, NULL, NULL, FALSE, curbuf);

Here, we use the same redraw type as is used in the win_split_ins() function. This will make :tabedit and the like leave the same message as the :split command.

I also discovered that the NeoVim project changed this exact line in the same way. You can view the commit message here.

I tried to formulate a test for this problem to prove that it was fixed, but redrawing operations are suppressed within functions to the point that I could not make the test fail as one would expect from interactive use. My attempt at a test is below:

diff --git a/src/testdir/test_tabpage.vim b/src/testdir/test_tabpage.vim
index add9b3d7c..efaf22767 100644
--- a/src/testdir/test_tabpage.vim
+++ b/src/testdir/test_tabpage.vim
@@ -547,4 +547,24 @@ func Test_tabs()
   bw!
 endfunc
 
+func Test_new_tabpage_fileinfo()
+  %bw
+
+  let sp_fileinfo   = ''
+  let tabe_fileinfo = ''
+
+  sp foo
+  redraw
+  for i in range(1, &columns)
+    let sp_fileinfo .= nr2char(screenchar(&lines - &cmdheight + 1, i))
+  endfor
+  %bw | redraw!
+  tabe foo
+  redraw
+  for i in range(1, &columns)
+    let tabe_fileinfo .= nr2char(screenchar(&lines - &cmdheight + 1, i))
+  endfor
+  call assert_equal(sp_fileinfo, tabe_fileinfo)
+endfunc
+
 " vim: shiftwidth=2 sts=2 expandtab

@brammool, is this kind of change acceptable without a test? Do you think this test could be made to work?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub

[Bram Moolenaar]

Jason Franklin wrote:

> I noticed this just the other day, and I was motivated to fix it.
>
> The problem here is pretty easy to find:
>
> The error in timing occurs in the `ex_splitview()` function. Ultimately, the `win_new_tabpage()` function invokes `redraw_all_later()` with the `CLEAR` redraw type. This will remove the `fileinfo()` message from the screen before we complete the main loop.
>
> The patch here fixes the problem:
>

> ```patch

> diff --git a/src/window.c b/src/window.c
> index b9617f037..da93e25e9 100644
> --- a/src/window.c
> +++ b/src/window.c
> @@ -3702,7 +3702,7 @@ win_new_tabpage(int after)
> entering_window(curwin);
> #endif
>
> - redraw_all_later(CLEAR);
> + redraw_all_later(NOT_VALID);
> apply_autocmds(EVENT_WINNEW, NULL, NULL, FALSE, curbuf);
> apply_autocmds(EVENT_WINENTER, NULL, NULL, FALSE, curbuf);
> apply_autocmds(EVENT_TABNEW, NULL, NULL, FALSE, curbuf);
> ```
>
> Here, we use the same redraw type as is used in the `win_split_ins()`
> function. This will make `:tabedit` and the like leave the same
> message as the `:split` command.

Thanks, that looks like a good change.

> I also discovered that the NeoVim project changed this exact line in the same way. You can view the commit message [here](https://github.com/neovim/neovim/commit/e598811e76a4ee6666b680545097fb06b0ba59aa).

>
> I tried to formulate a test for this problem to prove that it was fixed, but redrawing operations are suppressed within functions to the point that I could not make the test fail as one would expect from interactive use. My attempt at a test is below:
>

> ```patch

It would be nice to have a test, but it quickly becomes a lot of work.
Since we don't know what part of the screen would not be updated
properly, and the suppresion of redraw while executing a script, it
would require a screenshot test. But then we have to be careful that
it's portable (e.g., no absolute path name must be shown, file size must
always be the same, etc.).

I'm fine with doing this without a test. The reasoning from Bjorn that
clearing the screen isn't necessarily faster than updating all the text
makes sense, I'll change some more CLEAR to NOT_VALID.

--
FIXME and XXX are two common keywords used to mark broken or incomplete code
not only since XXX as a sex reference would grab everybody's attention but
simply due to the fact that Vim would highlight these words.
-- Hendrik Scholz

/// 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 ///

[Bram Moolenaar]

Closed #630 via bf3250a.