[vim/vim] feat(move): allow scrolloff to be satisfied using screen rows past EOF (PR #19040)
mcauley-penney
Add a new window-local option 'scrolloffpad' that lets 'scrolloff' remain effective at the end of a buffer by treating EOF filler rows as usable screen space.
When enabled, cursor movements and jumps (e.g. j, G) keep the cursor line centered according to 'scrolloff' even past the last buffer line. When disabled, existing scrolloff behavior is preserved.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/19040
Commit Summary
- bc03504 feat(move): allow scrolloff to be satisfied using screen rows past EOF
File Changes
(10 files)
- M src/channel.c (2)
- M src/move.c (41)
- M src/normal.c (4)
- M src/option.c (26)
- M src/option.h (2)
- M src/optiondefs.h (4)
- M src/popupwin.c (1)
- M src/proto/move.pro (2)
- M src/structs.h (2)
- M src/window.c (1)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- 4f1d2e9 docs(options): init scrolloffpad option docs
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- 3e4f519 feat(options): add wiring for 'scrolloffpad' option
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- c856028 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- bbd8a56 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- 12cbb36 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 2 commits.
- 4e6a098 feat(options): add wiring for 'scrolloffpad' option
- 35d14e4 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 2 commits.
- ac71fdf feat(options): add wiring for 'scrolloffpad' option
- 1e5654e feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 2 commits.
- 1ef0b53 feat(options): add wiring for 'scrolloffpad' option
- 42c0ba8 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 3 commits.
- 09581ea docs(options): init scrolloffpad option docs
- a22cddc feat(options): add wiring for 'scrolloffpad' option
- 9f666b0 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- 1cd4276 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- 0b64ca9 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 3 commits.
- e96fec2 docs(options): init scrolloffpad option docs
- 882f91b feat(options): add wiring for 'scrolloffpad' option
- 6dbd639 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 3 commits.
- bd8a2d0 docs(options): init scrolloffpad option docs
- 2762631 feat(options): add wiring for 'scrolloffpad' option
- 2275bfc feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 2 commits.
- 38fa28c feat(options): add wiring for 'scrolloffpad' option
- 2a76ebf feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- bced57e feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 3 commits.
- f0280ab docs(options): init scrolloffpad option docs
- 570f9b3 feat(options): add wiring for 'scrolloffpad' option
- c17251e feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 3 commits.
- e63841c docs(options): init scrolloffpad option docs
- 012dbbe feat(options): add wiring for 'scrolloffpad' option
- c927f47 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- 670e072 test(scrolloffpad): make terminal sizes deterministic
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- 856bab2 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 3 commits.
- 5fa2df3 docs(options): init scrolloffpad option docs
- 0e18520 feat(options): add wiring for 'scrolloffpad' option
- 3e0d6a0 feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- 292532c feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 3 commits.
- 7ffeaad docs(options): init scrolloffpad option docs
- 97220ff feat(options): add wiring for 'scrolloffpad' option
- 8eedfce feat(move): implement scrolloffpad functionality
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19040)Thanks. I’m wondering, do we really need a new option for this? Perhaps we should just implement the missing functionality. Or would that be too intrusive a behavioral change for users who already use the 'scrolloff' option?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Copilot
@Copilot commented on this pull request.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
In src/move.c:
> + } + if (n >= *so_ptr && !eof_pressure) + // sufficient context, no need to scroll + check_botline = FALSE; } - if (n >= *so_ptr) + else // sufficient context, no need to scroll check_botline = FALSE;
The comment 'sufficient context, no need to scroll' appears twice with identical meaning. The duplicate 'else' branch at line 518-520 can be consolidated with the condition at line 514-516 to reduce redundancy and improve clarity.
In src/testdir/test_cursor_func.vim:
> + let winbar_height = get(wininfo, 'winbar', 0)
+ call assert_equal({'row': winrow + wininfo.height - 1 + winbar_height,
The variable 'winbar_height' is misleading because it retrieves the 'winbar' value from wininfo, which is not necessarily a height. Consider renaming to 'winbar_offset' or documenting what the 'winbar' property actually represents.
⬇️ Suggested change- let winbar_height = get(wininfo, 'winbar', 0)
- call assert_equal({'row': winrow + wininfo.height - 1 + winbar_height,
+ let winbar_offset = get(wininfo, 'winbar', 0)
+ call assert_equal({'row': winrow + wininfo.height - 1 + winbar_offset,
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
mcauley-penney
mcauley-penney left a comment (vim/vim#19040)Thanks. I’m wondering, do we really need a new option for this? Perhaps we should just implement the missing functionality. Or would that be too intrusive a behavioral change for users who already use the 'scrolloff' option?
I think that the Vim community generally doesn't like changes to longstanding behavior and that a bigger discussion would be needed, with more users, before something like that.
I also think that this isn't necessarily a case of missing functionality, because scrolloff without this new option does have a benefit. It allows the user to see a lot of context around the edges of the buffer. When the user is at the top or bottom, they can see below or above, respectively. I view this option as implementing a tradeoff instead of adding missing functionality: it centers the bottom edge of the buffer for ease of viewing (kind of like what all of the Zen mode plugins do) but sacrifices context.
So, I think that users may like to select one or the other.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19040)is this ready?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
mcauley-penney
mcauley-penney left a comment (vim/vim#19040)Let me take one last look at it right now and ping you. I will
- resolve the conflicts,
- address the two comments from Copilot, and
- do a last pass over it.
I left it alone for a bit just in case there was further discussion about it
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- fceb056 Merge branch 'master' into feat-scrolloffpad
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney commented on this pull request.
In src/move.c:
> + } + if (n >= *so_ptr && !eof_pressure) + // sufficient context, no need to scroll + check_botline = FALSE; } - if (n >= *so_ptr) + else // sufficient context, no need to scroll check_botline = FALSE;
This branch is not duplicated. The comment text is, but the else condition at 518 is not redundant logic
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney commented on this pull request.
In src/testdir/test_cursor_func.vim:
> + let winbar_height = get(wininfo, 'winbar', 0)
+ call assert_equal({'row': winrow + wininfo.height - 1 + winbar_height,
In getwininfo(), winbar is documented as a 0/1 presence flag. This test adds it to wininfo.height, which excludes winbar, as a one-line row adjustment, so the current naming is intentional
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 2 commits.
- d20e4b8 Merge branch 'master' into feat-scrolloffpad
- 63fa971 test(scrolloffpad): fix fragile diff mode test
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
mcauley-penney
mcauley-penney left a comment (vim/vim#19040)is this ready?
This is ready, though I'm not sure about the lowered codecov score
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
dkearns
@dkearns commented on this pull request.
In src/move.c:
> @@ -279,6 +279,31 @@ update_topline_redraw(void)
update_screen(0);
}
+/*
+ * Return TRUE when 'scrolloffpad' may augment 'scrolloff'.
+ * This only applies to automatic cursor visibility correction.
+ * For now 'scrolloffpad' is treated as boolean: 0 disables, > 0 enables.
+ */
+ static int
+use_scrolloffpad(void)
+{
+ return get_scrolloff_value() > 0 && get_scrolloffpad_value() > 0;
+}
+
+/*
+ * Return TRUE when there are not enough real buffer lines below "lnum" to
+ * satisfy the requested "so" context.
⬇️ Suggested change
- * satisfy the requested "so" context. + * satisfy the requested 'so' context.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
dkearns
dkearns left a comment (vim/vim#19040)With regards to this becoming a non-optional default, I'm chiming in to say that as a user I dislike this 'scrolloffpad' behaviour.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
@zeertzjq commented on this pull request.
In src/move.c:
> @@ -279,6 +279,31 @@ update_topline_redraw(void)
update_screen(0);
}
+/*
+ * Return TRUE when 'scrolloffpad' may augment 'scrolloff'.
+ * This only applies to automatic cursor visibility correction.
+ * For now 'scrolloffpad' is treated as boolean: 0 disables, > 0 enables.
+ */
+ static int
+use_scrolloffpad(void)
+{
+ return get_scrolloff_value() > 0 && get_scrolloffpad_value() > 0;
+}
+
+/*
+ * Return TRUE when there are not enough real buffer lines below "lnum" to
+ * satisfy the requested "so" context.
This so is an argument name, not an option name.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
dkearns
@dkearns commented on this pull request.
In src/move.c:
> @@ -279,6 +279,31 @@ update_topline_redraw(void)
update_screen(0);
}
+/*
+ * Return TRUE when 'scrolloffpad' may augment 'scrolloff'.
+ * This only applies to automatic cursor visibility correction.
+ * For now 'scrolloffpad' is treated as boolean: 0 disables, > 0 enables.
+ */
+ static int
+use_scrolloffpad(void)
+{
+ return get_scrolloff_value() > 0 && get_scrolloffpad_value() > 0;
+}
+
+/*
+ * Return TRUE when there are not enough real buffer lines below "lnum" to
+ * satisfy the requested "so" context.
Oops
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19040)thanks
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19040)I was just trying to merge it, but for me the newly added test fails:
1 FAILED:
Found errors in Test_scrolloffpad_diff_eof_filler_behavior():
command line..script /mnt/home/chrisbra/code/vim-upstream/src/testdir/runtest.vim[636]..function RunTheTest[63]..Test_scrolloffpad_diff_eof_filler_behavior line 67: Expected True but got 0
command line..script /mnt/home/chrisbra/code/vim-upstream/src/testdir/runtest.vim[636]..function RunTheTest[63]..Test_scrolloffpad_diff_eof_filler_behavior line 68: Expected True but got 0
any idea?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- 2b645cc test(scrolloffpad): stabilize diff-mode tests across layouts
You are receiving this because you are subscribed to this thread.
mcauley-penney
mcauley-penney left a comment (vim/vim#19040)I was just trying to merge it, but for me the newly added test fails:
1 FAILED: Found errors in Test_scrolloffpad_diff_eof_filler_behavior(): command line..script /mnt/home/chrisbra/code/vim-upstream/src/testdir/runtest.vim[636]..function RunTheTest[63]..Test_scrolloffpad_diff_eof_filler_behavior line 67: Expected True but got 0 command line..script /mnt/home/chrisbra/code/vim-upstream/src/testdir/runtest.vim[636]..function RunTheTest[63]..Test_scrolloffpad_diff_eof_filler_behavior line 68: Expected True but got 0any idea?
I think that what was going on is that the assertions at those lines made assumptions that were not always valid. The test would create two diff windows, move the "shorter" one, and assert qualities in the viewport state. The assumption was that topline and winline would always change in certain directions (e.g. lower and higher), but it is possible for that to not be true and for the functionality to work correctly.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19040)Hm, I typically run this in a full screen putty windows, I just checked, it has 74 lines and 204 columns.
So to make this robust, how about to first create a 20 lines high new window like this :20new and from there you create your diff windows?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19040)could you make this suggested change?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
mcauley-penney
mcauley-penney left a comment (vim/vim#19040)I apologize for the wait. I'm currently traveling and will make the requested changes one week from today.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19040)ah no worries. I just wasn't sure you received the last comment. Take as much time as you need.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
mcauley-penney
@mcauley-penney pushed 1 commit.
- c5637d6 test(scrolloffpad): stabilize diff-mode tests across layouts
You are receiving this because you are subscribed to this thread.
mcauley-penney
mcauley-penney left a comment (vim/vim#19040)ah no worries. I just wasn't sure you received the last comment. Take as much time as you need.
I apologize for the wait. I traveled longer than I expected to, got really sick, then had a bit of a major life change that came up out of nowhere.
I made the requested change and included it in the last commit, which was related.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19040)No worries. Thanks for your work. I made a few minor changes (changing the function signature to use bool instead of int, fixed a bit of indentation issues and updated documentation. Please verify it works as expected. Thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.