Mapping erases search count message

[Gary Johnson]

I just tried exposing the search count message by removing 'S' from
'shortmess', but I couldn't see it. I discovered that it is hidden,
erased and/or not updated by a couple of my mappings.

nnoremap <silent> n nzv:call AdjCursor()<CR>
nnoremap <silent> N Nzv:call AdjCursor()<CR>

Here is a simple experiment that demonstrates the problem. Create
a file, test.vim, that contains the following.

set shortmess-=S
nnoremap <silent> n n
help map.txt

Open a standard-sized, 80x24 terminal, and in it run

$ vim -N -u NONE -i NONE -S test.vim

Then search for "command":

/command

After hitting Enter, the cursor will be at the start of "commands"
on line 7 and the command line will contain this:

/command [1/>99]

After hitting 'n', the cursor advances to line 13 and the command
line stays the same, even showing "[1/>99]" when it should be
showing "[2/>99]".

Another 'n' advances the cursor to line 17, the screen scrolls
up so that that line is at the bottom of the window, and the command
line is empty--no search count message at all.

I would think that <silent> would prevent the mapping from
disturbing the command line, in which case this is a bug.

If it's not a bug, then is there some way of defining a mapping that
does not interfere with the search count message, or some way of
restoring that message at the end of a mapping?

Regards,
Gary

[Tony Mechelynck]

I can't reproduce the problem.

What I get by hitting n after applying your mappings is:

E117: Unknown function: AdjCursor

With no mappings, the count is of course corectly shown.


Best regards,
Tony.

[Christian Brabandt]

Is that with patch 8.1.1288 included?

Best,
Christian
--
Wer nicht mit der Zeit geht, geht mit der Zeit!

[Gary Johnson]

On 2019-08-28, Tony Mechelynck wrote:

Yes, if you were to use those first two mappings without the
AdjCursor() function defined, I would expect them to fail as you
describe. You can avoid that by defining AdjCursor() as an empty
function. Alternatively, you can use the mapping that I intended
for you to use to demonstrate the problem, the one defined in
test.vim.

Regards,
Gary

[Gary Johnson]

Sorry, I forgot to include the version information. Yes, I used the
latest version, 8.1.1933.

Regards,
Gary

[Tony Mechelynck]

On Wed, Aug 28, 2019 at 8:12 AM Gary Johnson <gary...@spocom.com> wrote:
> Yes, if you were to use those first two mappings without the
> AdjCursor() function defined, I would expect them to fail as you
> describe. You can avoid that by defining AdjCursor() as an empty
> function. Alternatively, you can use the mapping that I intended
> for you to use to demonstrate the problem, the one defined in
> test.vim.
>
> Regards,
> Gary

Ah, sorry. Well, with ":noremap <silent> n n" the count is indeed not
echoed, but with ":noremap n n" instead, it is. I suppose that the
<silent> switch avoids output from the normal-mode {rhs} and that what
the help says about the need for an additional ":silent" in the {rhs}
applies to an ex-command, which is not what we have here.

Best regards,
Tony.

[Christian Brabandt]

Hm, I need to investigate.

Best,
Christian
--
Was die Gesellschaft öffentliche Meinung nennt, heißt beim einzelnen
Menschen Vorurteil.
-- Karl Heinrich Waggerl

[Christian Brabandt]

I see what is happening. A mapping with the `<silent>` flag will set the
internal variable cmd_silent to prevent it from being output the command
line. So what your mapping does is it acts like 'n' without outputting
anything on the command line.

But this is not what you want. You want the default behaviour of n,
which does output the command to search + the new search index feature.

(See the difference on the commandline between a plain `n` and a n
mapped with `nnoremap <silent> n n`).

So the obvious fix would be to remove the `<silent>` command. While this
fixes your minimal test case, it most likely is no fix for your actual
issue, that calling the AdjCursor() function will be output in the
command line in addition (possibly overwriting the command line).

What might work (depending on the complexity of your AdjCursor()
function) is to use an expression mapping that simply returns 'n' after
having done whatever action it needs to be doing. However, this might be
a bit difficult since you want this to happen after the cursor has been
placed.

Another alternative might be a mapping like this:

nmap n nzv
nnoremap <silent> zv zv:call AdjCursor()<cr>

Best,
Christian
--
Man darf nicht das Gras wachsen hören, sonst wird man taub.
-- Gerhard Hauptmann

[Bram Moolenaar]

The <silent> argument means that the command won't be echoed. But it
does not suppress the output of the command like the ":silent" modifier
does.

How about removing the check for cmd_silent in search.c, where
search_stat() is called?

--
hundred-and-one symptoms of being an internet addict:
131. You challenge authority and society by portnuking people

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

[Andy Wokula]

This is what <script> mappings are for.

> nmap n nzv
> nnoremap <silent> zv zv:call AdjCursor()<cr>

:nmap n n<SID>(adj-cursor)
:nnoremap <silent> <SID>(adj-cursor) zv:call AdjCursor()<CR>


" first command can be written as:
:nmap <script> n n<SID>(adj-cursor)
:nnoremap <script> n n<SID>(adj-cursor)

--
Andy

[Gary Johnson]

Thanks for looking further into this, Christian.

I don't understand how that first mapping in your alternative
mappings works. I thought that using nmap (not nnoremap) to map
n to a rhs including n would cause an infinite recursion, but it
doesn't.

Those mappings solve part of the problem. That is, if AdjCursor()
is an empty function, they work fine--the search count message is
always visible. But if AdjCursor() is the actual function (which
scrolls the window when needed to keep the cursor at least two lines
from the top and bottom), then whenever the window is scrolled, the
message disappears.

In fact, removing all the mappings and just executing Ctrl-E or
Ctrl-Y to scroll the window after a search erases the search count
message. I think that's a bug. I can see no reason why scrolling
should erase that message unless scrolling moves the cursor.

Further, certain motion commands such as j, k and gg _don't_ erase
the search count message, even though it would make sense for them
to do so. It's weird to jump from the bottom of a buffer to the top
with gg and still see the last search count message in the command
line.

The purpose of AdjCursor () is to scroll the window after a search
moves the cursor near the top or bottom of the window so as to
provide at least two lines of context around the cursor. (It should
really be named AdjWindow().) It behaves like scrolloff=2, but only
after certain commands. I don't want 'scrolloff' on all the time.

That gave me an idea, a different solution to the problem:
temporarily enable 'scrolloff' instead of scrolling the window.
Here is what I just came up with and it seems to work well.

nmap <silent> n :call ScrolloffCmd('n')<cr>
nmap <silent> N :call ScrolloffCmd('N')<cr>
function! ScrolloffCmd(cmd)
set scrolloff=2
try
exe 'normal!' a:cmd
catch
echohl ErrorMsg
echomsg matchstr(v:exception, ':\zs.*')
echohl NONE
endtry
set scrolloff=0
endfunction

Regards,
Gary

[Christian Brabandt]

On Do, 29 Aug 2019, Bram Moolenaar wrote:

> The <silent> argument means that the command won't be echoed. But it
> does not suppress the output of the command like the ":silent" modifier
> does.

Yeah, but it felt natural to me, to only show the search index feature,
if the command is echoed. I actually played with a simple patch
yesterday, but then thought that <silent> works as expected.

> How about removing the check for cmd_silent in search.c, where
> search_stat() is called?

It's a bit more convoluted, since msgbuf needs to be properly allocated
and initialized.

The attached patch does it. Let me know if you think that is okay. I can
write a test then.

Mit freundlichen Grüßen
Christian
--
Fragt der Arzt:
"Rauchen Sie?"
"Nein."
"Trinken Sie?"
"Nein."
Darauf der Arzt:
"Grinsen Sie nicht so blöd, ich find schon noch was!"

[Christian Brabandt]

On Do, 29 Aug 2019, Gary Johnson wrote:

> In fact, removing all the mappings and just executing Ctrl-E or
> Ctrl-Y to scroll the window after a search erases the search count
> message. I think that's a bug. I can see no reason why scrolling
> should erase that message unless scrolling moves the cursor.
>
> Further, certain motion commands such as j, k and gg _don't_ erase
> the search count message, even though it would make sense for them
> to do so. It's weird to jump from the bottom of a buffer to the top
> with gg and still see the last search count message in the command
> line.

Hm, yeah I can reproduce it. I am not sure yet, what causes the extra
redraw of the command line.

Best,
Christian
--
Wußten Sie schon...
... daß ein Knall schneller als der Schall sein kann?

[Christian Brabandt]

On Do, 29 Aug 2019, 'Andy Wokula' via vim_use wrote:

> > nmap n nzv
> > nnoremap <silent> zv zv:call AdjCursor()<cr>
>
> :nmap n n<SID>(adj-cursor)
> :nnoremap <silent> <SID>(adj-cursor) zv:call AdjCursor()<CR>
>
>
> " first command can be written as:
> :nmap <script> n n<SID>(adj-cursor)
> :nnoremap <script> n n<SID>(adj-cursor)

Ah yes, I always forget about that possibility.

Best,
Christian
--
Hätten die Neandertaler Atomkraftwerke besessen, wir hätten heute noch
damit zu tun.
-- Jürgen Trittin

[Bram Moolenaar]

Christian wrote:

> On Do, 29 Aug 2019, Gary Johnson wrote:
>
> > In fact, removing all the mappings and just executing Ctrl-E or
> > Ctrl-Y to scroll the window after a search erases the search count
> > message. I think that's a bug. I can see no reason why scrolling
> > should erase that message unless scrolling moves the cursor.
> >
> > Further, certain motion commands such as j, k and gg _don't_ erase
> > the search count message, even though it would make sense for them
> > to do so. It's weird to jump from the bottom of a buffer to the top
> > with gg and still see the last search count message in the command
> > line.
>
> Hm, yeah I can reproduce it. I am not sure yet, what causes the extra
> redraw of the command line.

It's probably just the scrolling. And it's probably tricky to keep the
counts when moving around in the file. I don't mind too much, so long
as it shows right after the search navigation.

--
"The sun oozed over the horizon, shoved aside darkness, crept along the
greensward, and, with sickly fingers, pushed through the castle window,
revealing the pillaged princess, hand at throat, crown asunder, gaping
in frenzied horror at the sated, sodden amphibian lying beside her,
disbelieving the magnitude of the frog's deception, screaming madly,
"You lied!"
- Winner of the Bulwer-Lytton contest (San Jose State University),
wherein one writes only the first line of a bad novel

[Bram Moolenaar]

Christian wrote:

> On Do, 29 Aug 2019, Bram Moolenaar wrote:
>
> > The <silent> argument means that the command won't be echoed. But it
> > does not suppress the output of the command like the ":silent" modifier
> > does.
>
> Yeah, but it felt natural to me, to only show the search index feature,
> if the command is echoed. I actually played with a simple patch
> yesterday, but then thought that <silent> works as expected.
>
> > How about removing the check for cmd_silent in search.c, where
> > search_stat() is called?
>
> It's a bit more convoluted, since msgbuf needs to be properly allocated
> and initialized.
>
> The attached patch does it. Let me know if you think that is okay. I can
> write a test then.

Thanks. Yes, I think we should do this. But the allocation should
probably be done differently, it looks like with cmd_silent set it still
computes the size of the command. This will require some more "if"
statements, but makes the size computation more accurate.

--
Birthdays are healthy. The more you have them, the longer you live.

[Christian Brabandt]

On Fr, 30 Aug 2019, Bram Moolenaar wrote:

> Thanks. Yes, I think we should do this. But the allocation should
> probably be done differently, it looks like with cmd_silent set it still
> computes the size of the command. This will require some more "if"
> statements, but makes the size computation more accurate.

Well, yeah I thought this wouldn't hurt.

So how about the attached patch then? It will simply use all available
space in the command line. Not sure this is correct however.

Best,
Christian
--
Wie man sein Kind nicht nennen sollte:
Carmen Bert

[Bram Moolenaar]

Christian wrote:

> On Fr, 30 Aug 2019, Bram Moolenaar wrote:
>
> > Thanks. Yes, I think we should do this. But the allocation should
> > probably be done differently, it looks like with cmd_silent set it still
> > computes the size of the command. This will require some more "if"
> > statements, but makes the size computation more accurate.
>
> Well, yeah I thought this wouldn't hurt.
>
> So how about the attached patch then? It will simply use all available
> space in the command line. Not sure this is correct however.

It's tricky with all the conditions. But it looks OK. All tests pass.
Let's include this and check that nothing goes wrong. Can we cover this
with a test?

--
"Hit any key to continue" is very confusing when you have two keyboards.

[Christian Brabandt]

On Mo, 02 Sep 2019, Bram Moolenaar wrote:

>
> Christian wrote:
>
> > On Fr, 30 Aug 2019, Bram Moolenaar wrote:
> >
> > > Thanks. Yes, I think we should do this. But the allocation should
> > > probably be done differently, it looks like with cmd_silent set it still
> > > computes the size of the command. This will require some more "if"
> > > statements, but makes the size computation more accurate.
> >
> > Well, yeah I thought this wouldn't hurt.
> >
> > So how about the attached patch then? It will simply use all available
> > space in the command line. Not sure this is correct however.
>
> It's tricky with all the conditions. But it looks OK. All tests pass.
> Let's include this and check that nothing goes wrong. Can we cover this
> with a test?

Apologizes, the last patch was wrong and caused a strtrunc message
('...')

Perhaps we don't even need the added condition `|| cmd_silent` at all?

Here is a fix including a test.

diff --git a/src/search.c b/src/search.c
index 758c4ef1a..ee66052a9 100644
--- a/src/search.c
+++ b/src/search.c
@@ -1391,7 +1391,7 @@ do_search(
// search stat. Use all the space available, so that the
// search state is right aligned. If there is not enough space
// msg_strtrunc() will shorten in the middle.
- if (msg_scrolled != 0 || cmd_silent)
+ if (msg_scrolled != 0 && !cmd_silent)
// Use all the columns.
len = (int)(Rows - msg_row) * Columns - 1;
else
diff --git a/src/testdir/test_search_stat.vim b/src/testdir/test_search_stat.vim
index cf36f3214..f23952915 100644
--- a/src/testdir/test_search_stat.vim
+++ b/src/testdir/test_search_stat.vim
@@ -160,7 +160,27 @@ func! Test_search_stat()
let stat = '\[1/2\]'
call assert_notmatch(pat .. stat, g:a)

- " close the window
+ " normal, n comes from a silent mapping
+ " First test a normal mapping, then a silent mapping
+ call cursor(1,1)
+ nnoremap n n
+ let @/ = 'find this'
+ let pat = '/find this\s\+'
+ let g:a = execute(':unsilent :norm n')
+ let g:b = split(g:a, "\n")[-1]
+ let stat = '\[1/2\]'
+ call assert_match(pat .. stat, g:b)
+ nnoremap <silent> n n
+ call cursor(1,1)
+ let g:a = execute(':unsilent :norm n')
+ let g:b = split(g:a, "\n")[-1]
+ let stat = '\[1/2\]'
+ call assert_notmatch(pat .. stat, g:b)
+ call assert_match(stat, g:b)
+ unmap n
+
+ " Clean up
set shortmess+=S
+ " close the window
bwipe!
endfunc


Best,
Christian
--
Sitzt eine Spinne auf dem Klo, wird man des Lebens nicht mehr froh.

[Bram Moolenaar]

Christian wrote:

> > > On Fr, 30 Aug 2019, Bram Moolenaar wrote:
> > >
> > > > Thanks. Yes, I think we should do this. But the allocation should
> > > > probably be done differently, it looks like with cmd_silent set it still
> > > > computes the size of the command. This will require some more "if"
> > > > statements, but makes the size computation more accurate.
> > >
> > > Well, yeah I thought this wouldn't hurt.
> > >
> > > So how about the attached patch then? It will simply use all available
> > > space in the command line. Not sure this is correct however.
> >
> > It's tricky with all the conditions. But it looks OK. All tests pass.
> > Let's include this and check that nothing goes wrong. Can we cover this
> > with a test?
>
> Apologizes, the last patch was wrong and caused a strtrunc message
> ('...')
>
> Perhaps we don't even need the added condition `|| cmd_silent` at all?
>
> Here is a fix including a test.

Thanks. The test doesn't fail without the fix though.

--
hundred-and-one symptoms of being an internet addict:

172. You join listservers just for the extra e-mail.

[Christian Brabandt]

On Di, 03 Sep 2019, Bram Moolenaar wrote:

>
> Christian wrote:
>
> > > > On Fr, 30 Aug 2019, Bram Moolenaar wrote:
> > > >
> > > > > Thanks. Yes, I think we should do this. But the allocation should
> > > > > probably be done differently, it looks like with cmd_silent set it still
> > > > > computes the size of the command. This will require some more "if"
> > > > > statements, but makes the size computation more accurate.
> > > >
> > > > Well, yeah I thought this wouldn't hurt.
> > > >
> > > > So how about the attached patch then? It will simply use all available
> > > > space in the command line. Not sure this is correct however.
> > >
> > > It's tricky with all the conditions. But it looks OK. All tests pass.
> > > Let's include this and check that nothing goes wrong. Can we cover this
> > > with a test?
> >
> > Apologizes, the last patch was wrong and caused a strtrunc message
> > ('...')
> >
> > Perhaps we don't even need the added condition `|| cmd_silent` at all?
> >
> > Here is a fix including a test.
>
> Thanks. The test doesn't fail without the fix though.

Yeah, I did not update the test, I tried, but the message is not
truncated when running the test. I think this happens because the output
of `execute()` is actually scrolled. Not sure.

This patch should do it, but only works when run interactively.

diff --git a/src/testdir/test_search_stat.vim b/src/testdir/test_search_stat.vim
index f23952915..33c3858bc 100644
--- a/src/testdir/test_search_stat.vim
+++ b/src/testdir/test_search_stat.vim
@@ -176,7 +176,9 @@ func! Test_search_stat()

let g:b = split(g:a, "\n")[-1]

let stat = '\[1/2\]'
call assert_notmatch(pat .. stat, g:b)
- call assert_match(stat, g:b)
+ " Test that the message is not truncated
+ " it would insert '...' into the output.
+ call assert_match('^\s\+' .. stat, g:b)
unmap n

" Clean up

Mit freundlichen Grüßen
Christian
--

Das Werk sollte immer ein wenig schlauer sein als der Autor.
-- Vaclav Havel

[Bram Moolenaar]

Hmm, since nobody runs the tests manually that doesn't help much.
How about using a screendump? That makes these things a lot easier to
write.

--
I'm so disorganized my keyboard isn't even in alphabetical order!

[Christian Brabandt]

On Mi, 04 Sep 2019, Bram Moolenaar wrote:

> Hmm, since nobody runs the tests manually that doesn't help much.
> How about using a screendump? That makes these things a lot easier to
> write.

Okay, how about the attached patch then? That fails with v8.1.1965 and
should work with v8.1.1970

Best,
Christian
--
Sargdeckel fällt - die Witwe kichert, der Bauer war wohl gut versichert.

[Christian Brabandt]

On Mi, 04 Sep 2019, Christian Brabandt wrote:

> Okay, how about the attached patch then? That fails with v8.1.1965 and
> should work with v8.1.1970

Small update:

[...]
> + call delete('XscriptMatchCommon')

that should of course be

call delete('Xsearchstat')


Best,
Christian
--
Ein schönes, herrliches Weib, das unvermählt bleibt, ist eine stille
und doch laute Anklage gegen alle Männer.
-- Bogumil Goltz

[Bram Moolenaar]

Christian wrote:

> On Mi, 04 Sep 2019, Bram Moolenaar wrote:
>
> > Hmm, since nobody runs the tests manually that doesn't help much.
> > How about using a screendump? That makes these things a lot easier to
> > write.
>
> Okay, how about the attached patch then? That fails with v8.1.1965 and
> should work with v8.1.1970

Yes, this works, thanks!

--
hundred-and-one symptoms of being an internet addict:

178. You look for an icon to double-click to open your bedroom window.

[Gary Johnson]

On 2019-09-04, Bram Moolenaar wrote:
> Christian wrote:
>
> > On Mi, 04 Sep 2019, Bram Moolenaar wrote:
> >
> > > Hmm, since nobody runs the tests manually that doesn't help much.
> > > How about using a screendump? That makes these things a lot easier to
> > > write.
> >
> > Okay, how about the attached patch then? That fails with v8.1.1965 and
> > should work with v8.1.1970
>
> Yes, this works, thanks!

This is very nice! Thank you both. I just updated to 8.1.1987.
I had made some changes to my search mappings that improved the
behavior of the search count message but it still wasn't right in
all cases. Now the message behaves as I expect it to in all the
cases I've tested.

Regards,
Gary

[Bram Moolenaar]

Looking through older problems...

I tried the example with remapping "n", but it works OK for me.
Was this problem fixed already? Or is only that example fixed and there
a remaining problem?

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

[Gary Johnson]

There was a patch that fixed part of the problem, and I rewrote my
function to avoid another part of the problem. It's been working
fine since early September. I don't think there is any more that
you need to do. Thank you and Christian again.

Regards,
Gary