preserve window height when 'winminheight' is 0 (follow-up)

[lacygoill]

I'm sorry but the PR #149 I sent did not fix the reported issue entirely.

Here is another case where the height of the window is not preserved:

$ vim -Nu <(cat <<'EOF'
set wmh=0
set rtp^=~/.vim/plugged/vim-dirvish
au VimEnter * call Set_layout()
fu Set_layout()
    bo sp
    helpg foobar
    close|copen
    wincmd p|1wincmd w|2wincmd w
    wincmd _
    call win_execute(win_getid(3), 'resize 10')
endfu
EOF
)

From this layout, if :Dirvish is run, the height of the window decreases by 1.

The issue disappears if 1wincmd w|2wincmd w is removed; its purpose is to set the previous window. So I guess that the issue lies somewhere in the dirvish code which tries to preserve the number of the previous window.

It may seem as a contrived example, but I found it in a real usage scenario (using a plugin which automatically sets the height of entered windows).

The previous fix ran :resize +1 to increase the size of the window by 1; but this time, the lost line is above the window, not below. So, in this case, the right fix is :1resize 0.

Btw:

• the fact that :resize accepts a window number as a prefix count is not documented (at least I could not find anything about it at :h :resize) Edit: It has just been documented today.

• one can use win_execute() to run a command in a squashed window without increasing its height to 1 line (try set wmh=0|bo sp|call win_execute(win_getid(1), 'set ft=python')); so one could think that set wmh=0|bo sp|call win_execute(win_getid(1), 'resize 0') would work; it does not; Vim resizes the height of the first window to 1 line (so it seems that win_execute() needs 1 line to draw the cursor somewhere when it runs :resize, but not when it runs any other Ex command; no idea why this need is specific to :resize)

• to fix the issue, one could be tempted to run :1resize -1 instead of :1resize 0, but it does not work; I'm not even sure what :resize does when it receives a prefix count and a relative number as the suffix count; try this:

   $ vim -Nu NONE +'bo sp|wincmd _'
   :1resize -5
   :1resize -5

  Notice how the first :1resize -5 makes the first window occupy the whole terminal minus 5 lines, and how the second :1resize -5 squashes it again. That's not what :resize -5 does in the current window.

All of this lead me to believe that :resize is unreliable/too complex to preserve the height of the dirvish window.

I think a simpler and more reliable way to preserve it, is to save the layout with winrestcmd(), then restore it with :exe at the end of s:bufwin_do(). Concretely, it would mean that this line:

https://github.com/justinmk/vim-dirvish/blob/79ffe5c6cf0a56d693659b6b98e92a82daf105f4/autoload/dirvish.vim#L321

would be replaced by:

let [curtab, curwin, curwinalt, curheight, curwidth, winrestcmd] = [tabpagenr(), winnr(), winnr('#'), winheight(0), winwidth(0), winrestcmd()]

And this line:

https://github.com/justinmk/vim-dirvish/blob/79ffe5c6cf0a56d693659b6b98e92a82daf105f4/autoload/dirvish.vim#L334

by:

if (&winminheight == 0 && curheight != winheight(0)) || (&winminwidth == 0 && curwidth != winwidth(0))
  exe s:noau winrestcmd
endif

These two changes fix the issue reported in the PR #149 and the one reported at the top of this post. They also fix the case of a window squashed vertically instead of horizontally ('winminwidth' set to 0), which the previous PR didn't.

---

There are two other pitfalls, one can be fixed, but not the other one. However, this is only an issue if s:bufwin_do() runs a command which changes the windows layout.

I've looked at the code, and s:bufwin_do() is not used to change the windows layout, but to save and restore the view in a window. So I think the function is not affected, and I did not try to fix the pitfalls to not overcomplicate the code for no immediate gain. Although, I did document them in a wrapper function I use to try to emulate win_execute() in Nvim.

Note that win_execute() is not affected by those pitfalls.

---

Also, since you said – in the PR #149 – that you were interested in using win_execute(), I thought it would be a good idea to give it a try.

Currently, s:bufwin_do() and s:tab_win_do() contain 23 lines of code. I think you could do without s:tab_win_do() and get the same result in only 1 line of code, using win_execute():

function! s:bufwin_do(cmd, bname) abort
  call map(filter(getwininfo(), {_,v -> a:bname ==# bufname(v.bufnr)}), {_,v -> win_execute(v.winid, s:noau.' '.a:cmd)})
endfunction

Note that getwininfo() was added in 7.4.1557. As for lambda expressions, they were added in 7.4.2044. But these two versions are older than 8.1.1418 where win_execute() was added to Vim; so I think it's fair to assume that if someone has win_execute(), they also have getwininfo() and lambda expressions.

The refactoring is based on the old code of s:bufwin_do(): https://github.com/justinmk/vim-dirvish/commit/db33349b29250f4b31e172acd756ffe56db8fd38

Since then, the code has become more complex, but I think this complexity is not necessary when win_execute() is used. In particular, the previous command invoking win_execute() should only target windows whose buffer name is a:bname, so I don't think it could cause the screen to flicker or degrade the performance; although, I don't know how to test this properly.

---

I made sure that the PR fixes the two reported issues in Vim and in Nvim, but I haven't used it for very long (just a few days). So you may want to take your time before merging, in case you/I find other issues.

---

Whatever you don't like in the code, let me know and I will try to fix it.

[justinmk]

I think a simpler and more reliable way to preserve it, is to save the layout with winrestcmd(), then restore it with :exe at the end of s:bufwin_do().

Yeah, the reason I didn't use winrestcmd already is because it's janky. Need to think about this.

Thanks for your attention to detail!

[justinmk]

since the win_execute codepath is much nicer, I'm ok with the old codepath being hideous :)

[lacygoill]

I found another case where the height of an unfocused window is reset to 1; it only affects :windo, not win_execute().

I won't try to submit yet another PR to fix it because it seems pointless to continue playing whack-a-mole. Since the PR did add some ugly code, if you want to revert it, and just keep the win_execute() invocation, that's fine by me.

If someone else complains about this issue (doubtful, many other plugins suffer from the same issue, and I didn't find anyone complaining), you could always tell them to use a more recent version of (N)Vim; one which includes win_execute().

Anyway, thank you very much for merging the PR.