[vim/vim] Using highlight in statusline causes window to not react to scrolling (Issue #10952)
Yee Cheng Chin
Steps to reproduce
- Install Lightline (https://github.com/itchyny/lightline.vim). I just put in under
~/.vim/pack/...using the standard Vim package installation method. - Launch Vim.
- Make sure mouse is enabled by doing
:set mouse=a. - Enter
:put =range(1,100)to fill some text. (Or just open any file) - Enter
:bot spto open a new split at the bottom. Right now there should be two splits, with the active window at the bottom. - Position the mouse cursor on the top window (which is not active). Attempt to scroll using mouse wheel.
Expected behaviour
The window does not react to mouse wheel. However, if you click on it, or move the active window to it somehow, it will immediately be repainted with the correctly scrolled position. That's because the window was indeed scrolling, but didn't properly repaint itself.
Version of Vim
9.0.0233
Environment
OS: macOS 12.5.1
Tested on regular Vim on terminal and also in MacVim.
Logs and stack traces
No response
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
I wanted to find a more concise repro but was failing at it. The issue here is related to 65ed136 (v8.0.1164). Basically, Lightline is calling some hi link commands within statusline itself, which causes redraw_all_later to be called which tries to update must_redraw and the window's w_redr_type. The logic introduced in v8.0.1164 tries to set it back, but is a little buggy and not properly restoring the state.
In particular, the bug here is that if you search for \<curwin\> = within the function (build_stl_str_hl), you will find that inside the handling of case STL_VIM_EXPR: // '{', it temporarily sets curwin to the current window before executing, and so if that window's w_redr_type gets change, the restoration code at the bottom would not be sufficient in properly restoring the state.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
The patch that seems to fix this issue is as follows, but honestly this feels a little fragile due to how much build_stl_str_hl seems to try to prevent the recursive draw from happening but that could really happen from anywhere.
diff --git a/src/buffer.c b/src/buffer.c index 314ab64b2a..8c17584560 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -4240,6 +4240,7 @@ build_stl_str_hl( stl_hlrec_T *sp; int save_must_redraw = must_redraw; int save_redr_type = curwin->w_redr_type; + int local_save_redr_type; int save_KeyTyped = KeyTyped; if (stl_items == NULL) @@ -4653,8 +4654,21 @@ build_stl_str_hl( if (curwin != save_curwin) VIsual_active = FALSE; + local_save_redr_type = curwin->w_redr_type; + str = eval_to_string_safe(p, use_sandbox, FALSE); + // When inside update_screen we do not want redrawing a statusline, ruler, + // title, etc. to trigger another redraw, it may cause an endless loop. + // + // We already restore the cached must_redraw / curwin->w_redr_type + // at the end of the function, but here, since we change curwin + // temporarily, we have to restore it locally. + if (updating_screen) + { + curwin->w_redr_type = local_save_redr_type; + } + curwin = save_curwin; curbuf = save_curbuf; VIsual_active = save_VIsual_active;
I could submit a pull request for this, but I'm still trying to figure out how to generate a small enough repro case, and/or a test case that could regression test this (since I don't really have a smaller repro than just "install LightLine and let it do its highlight linking thing")
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
"wp" the redraw type of "curwin" is saved and restored.
I wonder what happens if the redraw type of "wp" is saved and restored
only? Or both? There is no test that covers this.
--
TALL KNIGHT: When you have found the shrubbery, then you must cut down the
mightiest tree in the forest ... with a herring.
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
I think the core issue here is that must_redraw is saved and restored at the bottom of the function. must_redraw is kind of the maximum of all the windows' w_redr_type, so if you save/restore must_redraw you technically need to save all the windows w_redr_type as well to maintain a consistent state. The current code is only saving curwin->w_redr_type for convenience, but as we can see here other windows' w_redr_type can be modified as well. I think just saving wp's w_redr_type may not be enough actually, if the statusline evaluation is doing even more funky stuff and switches window but that seems like a bad practice to begin with.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> I think the core issue here is that `must_redraw` is saved and
> restored at the bottom of the function. `must_redraw` is kind of the
> `must_redraw` you technically need to save all the windows
> `w_redr_type` as well to maintain a consistent state. The current code
> is only saving `curwin->w_redr_type` for convenience, but as we can
> see here other windows' `w_redr_type` can be modified as well. I think
> just saving `wp`'s `w_redr_type` may not be enough actually, if the
> `statusline` evaluation is doing even more funky stuff and switches
> window but that seems like a bad practice to begin with.
What happens here is a protection against it happening anyway and
avoiding endless screen updating.
Perhaps we should instead give an error when the redraw state is changed
while updating a statusline, and then disable that statusline to prevent
it from causing trouble. I wonder what would break then...
We could at least prevent w_redr_type and must_redraw from being
changed. If this causes some redraw problem, then it's bad luck, but at
least nothing is really broken.
--
ARTHUR: Go on, Bors, chop its head off.
BORS: Right. Silly little bleeder. One rabbit stew coming up.
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
Ok, your solution does sound better than just patchwork restoring the different w_redr_type (albeit more involved). And I agree that a sort of broken statusline redraw is better compared to a broken inconsistent internal state (and statusline plugin authors can always work around the recursive redraw limit).
I tested the latest commit and it fixes the bug that I reported (I can scroll and see live update on the top window now), so feel free to close this issue as the bug is resolved as far as I am concerned.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
Closed #10952 as completed.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.