changing curwin in callback results in scrolling for 'nosplitscroll'#11185
Closed
luukvbaal wants to merge 1 commit intovim:masterfrom
Closed
changing curwin in callback results in scrolling for 'nosplitscroll'#11185luukvbaal wants to merge 1 commit intovim:masterfrom
luukvbaal wants to merge 1 commit intovim:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11185 +/- ##
==========================================
- Coverage 81.81% 81.25% -0.56%
==========================================
Files 162 162
Lines 188698 188667 -31
Branches 42907 42888 -19
==========================================
- Hits 154374 153298 -1076
- Misses 21774 22925 +1151
+ Partials 12550 12444 -106
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
zeertzjq
reviewed
Sep 21, 2022
src/testdir/test_window_cmd.vim
Outdated
| set splitscroll& | ||
| endfunc | ||
|
|
||
| source screendump.vim |
Member
There was a problem hiding this comment.
This is usually put at the top at the file.
Contributor
|
Problem: Changing curwin in callback results in scrolling for 'nosplitscroll', missing test for terminal mode callback.
Solution: Prevent scrolling in callback, add missing test.
This fixes scrolling when the current window is changed inside a callback as I alluded to in: #11166
> Moreover, if we move the wincmd k to the callback function, the
> current fix is not enough to catch that and scrolling will still
> happen. Trying to fix it with a watchpoint on firstwin->w_topline
> and backtracing led me to trying to add workarounds in
> redraw_after_callback() or ex_docmd.c but it got quite complicated.
> Not sure if it's worth to catch this issue.
If we now catch all possible cases that could result in scrolling due
to a user script callback I'm fine with this patch. I however can´t
been 100% certain that that is the case. I did try various other
split/window commands inside the callback script and the added test
keeps passing. I'm sure some user will report this issue at some point
so up to you if you think this is a valid patch @brammool. Also adds
missing test for the terminal mode callback patch added previously.
This calls validate_botline_win() after every callback. That is an
expensive operation, it has to compute the size of every visible line.
Is there a way to add a condition, that something changed in the window
layout, cursor position, etc? Actually, the current mechanism to only
invalidate botline (w_valid flags VALID_BOTLINE and VALID_BOTLINE_AP)
should already take care of this.
If you leave out the "invalidate_botline_win()" call, then what fails?
Perhaps the solution is to call invalidate_botline() in some situation
and there check for 'splitscroll' to be false.
…--
"Computers in the future may weigh no more than 1.5 tons."
Popular Mechanics, 1949
/// 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 ///
|
Contributor
Author
It seems botline is not (in)validated before the window is entered, inside the callback. Thus, leaving it out results in win_fix_cursor() using an outdated value to determine whether topline < cursor.lnum < botline. I'm not sure if this outdated botline could be considered a bug or if it's because vim (without 'nosplitscroll') doesn't care about botline being up-to-date during a callback? |
0d3f9e6 to
965a498
Compare
luukvbaal
commented
Sep 21, 2022
Contributor
|
@luukvbaal commented on this pull request.
> @@ -6383,6 +6383,7 @@ win_fix_scroll(int resize)
wp->w_cursor.lnum = lnum;
}
invalidate_botline_win(wp);
+ wp->w_valid &= ~VALID_CROW;
Simply invalidating VALID_CROW fixes the issue. But since invalidating
it is only necessary for this callback case, I wonder if there is a
better place to invalidate it without needlessly making comp_botline()
more expensive.
Just above the cursor is moved, thus removing VALID_CROW makes a lot of
sense.
Also, if the window height changed then invalidating botline also is
correct.
What I'm trying to avoid is that when a callback returns it
unconditionally makes botline invalid. Many callbacks don't change the
screen contents at all.
This is all getting quite complicated, we should really try to use the
"w_valid" flags in a proper way, otherwise we just keep updating values
that don't need updating.
…--
Would you care for a drink? I mean, if it were, like,
disabled and you had to look after it?
/// 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 ///
|
Contributor
Author
Here we are actually just changing it temporarily(to scroll to bottom) and restoring it. So invalidating it here still doesn't quite make sense I think.
Yes, I'm not sure why the check_cursor_moved() call in comp_botline() does not invalidate it. I guess it has to do with the fact that we delay moving the cursor in a window until it is entered(such that the cursor is not needlessly moved if the window layout changes again before it is entered). I'll try to make sense of the W_VALID flags and see if I can figure out where we need to set which in win_fix_cursor/scroll. |
965a498 to
e9a5995
Compare
luukvbaal
commented
Sep 21, 2022
e9a5995 to
c8591a6
Compare
Contributor
Author
|
Latest commit lets the validate_cursor() call in win_new_height() handle the w_valid flags, except for VALID_TOPLINE because we update the cursor position in win_fix_cursor instead. I think this patch now makes sense and it prevents the scrolling that happened in the test case. |
Contributor
Author
|
Scratch that, I thought I was doing a bitwise OR. Looking into it once more... |
c8591a6 to
3ad0802
Compare
Problem: Changing curwin in callback results in scrolling for 'nosplitscroll', missing test for terminal mode callback. Solution: Invalidate w_crow for curwin, add callback tests.
3ad0802 to
35ddba8
Compare
Contributor
Author
|
This is the best I could come up with. It fixes the test case and I don't see any issues but I can't say I'm confident that w_valid flags are updated properly for 'nosplitscroll'. |
Contributor
|
This is the best I could come up with. It fixes the test case and I
don't see any issues but I can't say I'm confident that are updated
properly for 'nosplitscroll'.
I'm glad to see you found a simple solution.
…--
hundred-and-one symptoms of being an internet addict:
138. You develop a liking for cold coffee.
/// 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 ///
|
zeertzjq
added a commit
to luukvbaal/neovim
that referenced
this pull request
Oct 6, 2022
vim-patch:9.0.0445: when opening/closing window text moves up/down
Problem: When opening/closing window text moves up/down.
Solution: Add the 'splitscroll' option. When off text will keep its
position as much as possible.
vim/vim@29ab524
vim-patch:9.0.0455: a few problems with 'splitscroll'
Problem: A few problems with 'splitscroll'.
Solution: Fix 'splitscroll' problems. (Luuk van Baal, closes vim/vim#11117)
vim/vim@5ed3917
vim-patch:9.0.0461: 'scroll' is not always updated
Problem: 'scroll' is not always updated.
Solution: Call win_init_size() at the right place.
vim/vim@470a141
vim-patch:9.0.0465: cursor moves when cmdwin is closed when 'splitscroll' is off
Problem: Cursor moves when cmdwin is closed when 'splitscroll' is off.
Solution: Temporarily set 'splitscroll' when jumping back to the original
window. (closes vim/vim#11128)
vim/vim@e697d48
vim-patch:9.0.0469: cursor moves if cmdwin is closed when 'splitscroll' is off
Problem: Cursor moves if cmdwin is closed when 'splitscroll' is off.
Solution: Skip win_fix_cursor if called when cmdwin is open or closing.
(Luuk van Baal, closes vim/vim#11134)
vim/vim@3735f11
vim-patch:9.0.0478: test for 'splitscroll' takes too much time
Problem: Test for 'splitscroll' takes too much time.
Solution: Only test some of the combinations. (Luuk van Baal, closes vim/vim#11139)
vim/vim@594f9e0
vim-patch:9.0.0486: text scrolled with 'nosplitscroll', autocmd win and help
Problem: Text scrolled with 'nosplitscroll', autocmd win opened and help
window closed.
Solution: Skip win_fix_scroll() in more situations. (Luuk van Baal,
closes vim/vim#11150)
vim/vim@d5bc762
vim-patch:9.0.0505: various problems with 'nosplitscroll'
Problem: Various problems with 'nosplitscroll'.
Solution: Fix 'nosplitscroll' problems. (Luuk van Baal, closes vim/vim#11166)
vim/vim@faf1d41
vim-patch:9.0.0555: scrolling with 'nosplitscroll' in callback changing curwin
Problem: Scrolling with 'nosplitscroll' in callback changing curwin.
Solution: Invalidate w_cline_row in the right place. (Luuk van Baal,
closes vim/vim#11185)
vim/vim@20e5856
vim-patch:9.0.0603: with 'nosplitscroll' folds are not handled correctly
Problem: With 'nosplitscroll' folds are not handled correctly.
Solution: Take care of closed folds when moving the cursor. (Luuk van Baal,
closes vim/vim#11234)
vim/vim@7c1cbb6
vim-patch:9.0.0605: dump file missing
Problem: Dump file missing.
Solution: Add the missing dump file. (issue vim/vim#11234)
vim/vim@439a2ba
vim-patch:9.0.0647: the 'splitscroll' option is not a good name
Problem: The 'splitscroll' option is not a good name.
Solution: Rename 'splitscroll' to 'splitkeep' and make it a string option,
also supporting "topline". (Luuk van Baal, closes vim/vim#11258)
vim/vim@13ece2a
vim-patch:9.0.0667: ml_get error when 'splitkeep' is "screen"
Problem: ml_get error when 'splitkeep' is "screen". (Marius Gedminas)
Solution: Check the botline is not too large. (Luuk van Baal,
closes vim/vim#11293, closes vim/vim#11292)
vim/vim@346823d
zeertzjq
pushed a commit
to neovim/neovim
that referenced
this pull request
Oct 6, 2022
vim-patch:9.0.0445: when opening/closing window text moves up/down
Problem: When opening/closing window text moves up/down.
Solution: Add the 'splitscroll' option. When off text will keep its
position as much as possible.
vim/vim@29ab524
vim-patch:9.0.0455: a few problems with 'splitscroll'
Problem: A few problems with 'splitscroll'.
Solution: Fix 'splitscroll' problems. (Luuk van Baal, closes vim/vim#11117)
vim/vim@5ed3917
vim-patch:9.0.0461: 'scroll' is not always updated
Problem: 'scroll' is not always updated.
Solution: Call win_init_size() at the right place.
vim/vim@470a141
vim-patch:9.0.0465: cursor moves when cmdwin is closed when 'splitscroll' is off
Problem: Cursor moves when cmdwin is closed when 'splitscroll' is off.
Solution: Temporarily set 'splitscroll' when jumping back to the original
window. (closes vim/vim#11128)
vim/vim@e697d48
vim-patch:9.0.0469: cursor moves if cmdwin is closed when 'splitscroll' is off
Problem: Cursor moves if cmdwin is closed when 'splitscroll' is off.
Solution: Skip win_fix_cursor if called when cmdwin is open or closing.
(Luuk van Baal, closes vim/vim#11134)
vim/vim@3735f11
vim-patch:9.0.0478: test for 'splitscroll' takes too much time
Problem: Test for 'splitscroll' takes too much time.
Solution: Only test some of the combinations. (Luuk van Baal, closes vim/vim#11139)
vim/vim@594f9e0
vim-patch:9.0.0486: text scrolled with 'nosplitscroll', autocmd win and help
Problem: Text scrolled with 'nosplitscroll', autocmd win opened and help
window closed.
Solution: Skip win_fix_scroll() in more situations. (Luuk van Baal,
closes vim/vim#11150)
vim/vim@d5bc762
vim-patch:9.0.0505: various problems with 'nosplitscroll'
Problem: Various problems with 'nosplitscroll'.
Solution: Fix 'nosplitscroll' problems. (Luuk van Baal, closes vim/vim#11166)
vim/vim@faf1d41
vim-patch:9.0.0555: scrolling with 'nosplitscroll' in callback changing curwin
Problem: Scrolling with 'nosplitscroll' in callback changing curwin.
Solution: Invalidate w_cline_row in the right place. (Luuk van Baal,
closes vim/vim#11185)
vim/vim@20e5856
vim-patch:9.0.0603: with 'nosplitscroll' folds are not handled correctly
Problem: With 'nosplitscroll' folds are not handled correctly.
Solution: Take care of closed folds when moving the cursor. (Luuk van Baal,
closes vim/vim#11234)
vim/vim@7c1cbb6
vim-patch:9.0.0605: dump file missing
Problem: Dump file missing.
Solution: Add the missing dump file. (issue vim/vim#11234)
vim/vim@439a2ba
vim-patch:9.0.0647: the 'splitscroll' option is not a good name
Problem: The 'splitscroll' option is not a good name.
Solution: Rename 'splitscroll' to 'splitkeep' and make it a string option,
also supporting "topline". (Luuk van Baal, closes vim/vim#11258)
vim/vim@13ece2a
vim-patch:9.0.0667: ml_get error when 'splitkeep' is "screen"
Problem: ml_get error when 'splitkeep' is "screen". (Marius Gedminas)
Solution: Check the botline is not too large. (Luuk van Baal,
closes vim/vim#11293, closes vim/vim#11292)
vim/vim@346823d
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
changing curwin in callback results in scrolling for 'nosplitscroll'
Problem: w_valid flags are not updated accordingly for 'nosplitscroll', missing test for terminal mode callback.
Solution: Let validate_cursor handle w_valid flags except for VALID_TOPLINE, add callback tests.
This fixes scrolling when the current window is changed inside a callback as I alluded to in: #11166
If we now catch all possible cases that could result in scrolling due to a user script callback I'm fine with this patch. I however can't be 100% certain that that is the case. I did try various other split/window commands inside the callback script and the added test keeps passing. I'm sure some user will report this issue at some point so up to you if you think this is a valid patch @brammool. Also adds missing test for the terminal mode callback patch added previously.