feat(window/ui): add splitkeep option #19243

luukvbaal · 2022-07-05T23:50:22Z

Hi, I'm the author of a relatively popular plugin. I'm looking for feedback on whether something like this PR has any possibility of being merged with the behavior of said plugin being provided in core behind a new option.

I had originally assumed while writing the plugin that the "jarring" phenomenon of window viewports changing(scrolling), when splitting windows horizontally, was some undesired side effect of managing the windows. Now, having finally taken the time to look into the source code, it seems this is done intentionally with the following justification:

neovim/src/nvim/window.c

Lines 6193 to 6194 in 41785b1

    
                * Find a value for w_topline that shows the cursor at the same 
        
                * relative position in the window as before (more or less).

After 2 months of work on the vim patch it was finally merged. This PR now ports vim/vim@29ab524, vim/vim@5ed3917, vim/vim@470a141, vim/vim@3735f11, vim/vim@594f9e0, vim/vim@d5bc762, vim/vim@faf1d41, vim/vim@20e5856, vim/vim@7c1cbb6, vim/vim@439a2ba, vim/vim@13ece2a, vim/vim@346823d.

luukvbaal · 2022-07-06T19:29:03Z

BTW the motivation behind trying to merge it into core is that the "stabilization" done by the plugin is sometimes not fast enough in faster terminals. Thus resulting in flickering when restoring the topline. That and the overall gimmickyness of the plugin, never changing the scroll position at all is of course better than saving and restoring it.

lervag · 2022-07-09T09:47:55Z

I'm not a neovim dev, but I have some immediate thoughts:

First, this is cool, I would be happy to see it merged!
I think you should also add documentation. E.g. in runtime/doc/quickref.txt and runtime/doc/options.txt.
Would it make sense to add a test? Not sure how to write such a test, though.

luukvbaal · 2022-07-09T10:22:44Z

* I think you should also add documentation. E.g. in `runtime/doc/quickref.txt` and `runtime/doc/options.txt`.

Definitely. I refrained from doing so as I was still looking for general feedback on the feasibility of the PR. The PR turned out to be easier than I imagined though so I guess I can draft the help docs.

* Would it make sense to add a test? Not sure how to write such a test, though.

Not sure about this either.

I'm also wondering about the etiquette of opening PRs agains vim. I have no interest in doing so myself but I quickly checked and this part of core vim seems similar to nvim still.

clason · 2022-07-09T10:27:17Z

I'm also wondering about the etiquette of opening PRs agains vim. I have no interest in doing so myself but I quickly checked and this part of core vim seems similar to nvim still.

The etiquette is very much to contribute to Vim, if at all appropriate :)

In this case, you are right that this PR is directly applicable to Vim. The best course of action is then indeed to first make the PR to Vim, see what they think, and if they merge it, port that patch.

This way both projects profit from the new feature, and we avoid needless divergence in case Vim decides on slight changes to, say, the abbreviated option name.

(If Vim rejects the feature, we can reopen this PR, now with the knowledge that we don't need to take upstream into account.)

luukvbaal · 2022-07-09T10:38:09Z

The etiquette is very much to contribute to Vim,

I guess I'll do that then.

justinmk · 2022-07-09T19:40:02Z

This is definitely wanted and will likely become a Nvim default, so thank you.
Also makes sense to see if upstream Vim wants this, please let us know the result of that.

Adding a screen test is easy, see test/functional/example_spec.lua for example. You can run the test like this:

TEST_FILE=test/functional/example_spec.lua make functionaltest

I didn't find an existing test file where this would obviously fit, but maybe one of these:

test/functional/ui/screen.lua
test/functional/ui/screen_basic_spec.lua
test/functional/ui/multigrid_spec.lua

else we might need a new file, test/functional/ui/window_spec.lua.

lewis6991 · 2022-07-09T19:45:34Z

Are you sure we would want this as default? Isn't the point of the scrolling to ensure the split doesn't cover the last position of the cursor?

justinmk · 2022-07-09T20:35:27Z

Isn't the point of the scrolling to ensure the split doesn't cover the last position of the cursor?

The current :split behavior scrolls to keep the cursor at the same relative position, e.g. 30% or 50%.

I would expect 'splitscroll=false' to scroll if the cursor position would be hidden, since there is not yet an internal concept of a window with an off-screen cursor. This PR needs tests for both cases in any case.

(btw, does this imply we need a number or flags option instead of a boolean? because there are several possible behaviors.)

lewis6991 · 2022-07-09T20:47:57Z

Hmm, sounds like the current behaviour is naive/simple and now we just want to make it "smart". I'd contemplate not even making an option for this and just change the current behaviour, unless there's a good argument I'm not realising.

(btw, does this imply we need a number or flags option instead of a boolean? because there are several possible behaviors.)

Can you list these?

Also note we need to make sure we always respect 'scrolloff'.

baodrate · 2022-07-09T20:53:30Z

Hmm, sounds like the current behaviour is naive/simple and now we just want to make it "smart"

Is this PR not just allowing currently existing "smart" behavior to be disabled?

luukvbaal · 2022-07-09T20:59:00Z

(btw, does this imply we need a number or flags option instead of a boolean? because there are several possible behaviors.)

Yes this makes sense, the option was also available in the plugin. There it was named force as in "force stabilizing even when cursor is behind newly opened window". Not sure what an apt flag name would be if the option name stays splitscroll. Perhaps it should be reconsidered?

justinmk · 2022-07-09T21:15:17Z

Not sure what an apt flag name would be if the option name stays splitscroll. Perhaps it should be reconsidered?

could just be a number option, see 'laststatus'.
future: also consider vertical scroll

(btw, does this imply we need a number or flags option instead of a boolean? because there are several possible behaviors.)

Can you list these?

scroll to maintain relative position (current behavior)
don't scroll except as needed to keep cursor row visible (and subject to 'scrolloff')
never scroll, even if cursor row will be hidden or 'scrolloff' would be violated

luukvbaal · 2022-07-09T21:18:09Z

Are you sure we would want this as default? Isn't the point of the scrolling to ensure the split doesn't cover the last position of the cursor? I'd contemplate not even making an option for this and just change the current behaviour

Even when the cursor would not be hidden behind the split, currently scrolling happens just to "keep the same relative position". To me this is unnecessary but I guess it could be up to personal preference. Hence the option, but perhaps not necessary if we think the current default is undesired.

justinmk · 2022-07-09T21:28:32Z

Keeping the relative position is a valid use-case. Could avoid a new option by changing the core behavior, then users who want "keep relative position" could use a WinScroll (which apparently also means "WinResize"...?) event, but idk if WinScroll/WinResize gives the necessary info in v:event yet.

But that doesn't resolve the question about letting the cursor row get hidden: that does not seem like a good default behavior, and raises a lot of questions. If we are going to entertain the idea of letting the cursor row get hidden, that would need to be a third choice.

luukvbaal · 2022-07-10T12:03:50Z

I thought it could look something like the latest commit. Perhaps the solution is too naive though, I never use scrolloff so I'm not sure what to consider.

luukvbaal · 2022-07-10T14:34:06Z

Vim upstream will consider the feature if we can agree on what it will do. They will require a test as well, testdir/test_window_cmd.vim seems appropriate. Will adding a test there be sufficient or are the legacy tests deprecated here?

zeertzjq · 2022-07-10T21:29:06Z

Will adding a test there be sufficient

Yes

lewis6991 · 2022-07-11T08:30:53Z

Keeping the relative position is a valid use-case. Could avoid a new option by changing the core behavior, then users who want "keep relative position" could use a WinScroll (which apparently also means "WinResize"...?) event, but idk if WinScroll/WinResize gives the necessary info in v:event yet.

Ok, let's just introduce an option.

But that doesn't resolve the question about letting the cursor row get hidden: that does not seem like a good default behavior, and raises a lot of questions. If we are going to entertain the idea of letting the cursor row get hidden, that would need to be a third choice.

I'm working on the assumption that any new behaviour we add, doesn't break 'scrolloff', and, never hides (or moves) the cursor. For that reason I don't think we should add a 3rd form of behaviour.

In terms of how we do it, I think we should just add 'splitoptions', even if it only contains one option.

I thought it could look something like the latest commit. Perhaps the solution is too naive though, I never use scrolloff so I'm not sure what to consider.

We need to check if the cursor will remain in the window and obeys 'scrolloff'. If it doesn't then we need to scroll appropriately, and not necessarily the old scroll behaviour, but instead the minimum amount.

luukvbaal · 2022-07-11T13:00:16Z

For that reason I don't think we should add a 3rd form of behaviour.

I don't see any harm in adding the third option for those who prefer it. It was the default for my plugin, and never got any complaints. I agree that the cursor position should never violate scrolloff for the active window. However, I think delaying the scrolling to when the cursor is moved back the window where scrolling would be required to honor scrolloff (current behavior of this PR for splitscroll=2), results in better UX.

but instead the minimum amount.

Yeah I considered this too and I agree it would probably be better behavior but I wasn't sure it would be worth the hassle.

luukvbaal · 2022-07-11T13:09:58Z

I think I'll stop updating this PR until some movement happens in vim/vim#10682. Furthermore it seems like we still haven't agreed on what the option should be so perhaps updating either PR is futile but I thought I would cross post https://github.com/vim/vim/pull/10682/files#r917474316 to see if there's any feedback here.

luukvbaal · 2022-09-24T14:44:46Z

Any particular reason this was moved to the 0.9 milestone btw @clason? Is it too late in the 0.8 cycle? Is anything else needed for this PR? I can keep updating it with patches as vim bug reports/improvements keep coming in but 0.9 seems far away.

clason · 2022-09-24T14:46:34Z

Yes, it's (much) too late; we've been in feature freeze for a week already.

luukvbaal · 2022-09-30T00:26:39Z

Yes, it's (much) too late; we've been in feature freeze for a week already.

Whoops, totally missed that. Haven't really kept up with neovim development while working on the vim patch😅

Cross posting from vim/vim#10682 (comment):

Adding the option to keep the topline the same might actually be simpler to implement.

I think it might amount to simply guarding this clause behind the option. If users really want this I could look into it but the option shouldn't get needlessly complicated imo.

So yeah circling back to this after some initial user bug reports/improvements to the feature, I really do think providing this option is as simple as guarding the winrow != prev_winrow clause behind an alternate option value. We are then simply (in)validating botline for each window, and correcting the cursor position for curwin.

To reiterate, instead of having the previously proposed options for scroll behavior depending on whether or not the cursor would be valid ({"normal", "noscroll", "neverscroll"}, we dropped this because changing the cursor position was considered not to be that egregious when splitting a window, and as a compromise we add the previous cursor position to the jumplist).

Now the proposal is the have 3 separate scroll behaviors for 'splitscroll' because user(s)(@ychin) might prefer one over the other:

"normal": default behavior, keep the same relative cursor position in both new and old split.
"counterscroll": keep the text that was already visible in a window on the same screenlines for each window that is resized/moved by a split command, effectively "counter-scrolling" the topline to keep the text stable. (current 'nosplitscroll' behavior).
"noscroll": keep the same topline in all windows, effectively only keeping the text stable in the uppermost split.

Not sure "counterscroll" is very telling, can anyone think of a better name for these option values? Do any more users see the need for the third option? See vim/vim#11258 for the proposal.

zeertzjq · 2022-10-06T05:28:22Z

src/nvim/testdir/test_window_cmd.vim

+    let &laststatus = (run % 3)
+    let &splitbelow = (run % 3)
+    let &equalalways = (run % 2)
+    "let wsb = (run % 2) && &splitbelow


Is this because in Vim the splitted window does not have a winbar, while in Nvim it does?

Oh yes. I just noticed that this variable used to be called winbar_sb.

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

LGTM. I pushed a fixup commit with some small changes.

luukvbaal · 2022-10-06T09:15:14Z

LGTM. I pushed a fixup commit with some small changes.

Yay! I see I missed some of my own (style) changes from the vim patches. So thanks for the fixup and merge!

luukvbaal · 2022-10-07T11:21:35Z

Unfortunately maintainers weren't able to review this before the 0.8 release. I would be willing to make a backport PR but AFAIK 0.x.y releases are only for bug fixes.

luukvbaal force-pushed the splitscroll branch from e21ece0 to 2619b6a Compare July 5, 2022 23:57

luukvbaal changed the title ~~window/ui: add splitscroll option~~ feat(window/ui): add splitscroll option Jul 6, 2022

luukvbaal force-pushed the splitscroll branch from 2619b6a to 95c8c5c Compare July 9, 2022 02:31

clason added needs:discussion issue needs attention from an expert, or PR proposes significant changes to architecture or API core Nvim core functionality or code options configuration, settings labels Jul 9, 2022

luukvbaal force-pushed the splitscroll branch 2 times, most recently from e44f2d4 to 6534eea Compare July 9, 2022 10:51

luukvbaal mentioned this pull request Jul 9, 2022

Add new option splitscroll to avoid scrolling vim/vim#10682

Closed

justinmk added the has:vim-patch issue is fixed in vim and patch needs to be ported label Jul 9, 2022

luukvbaal force-pushed the splitscroll branch from b2d4b05 to 520708b Compare September 28, 2022 10:01

luukvbaal mentioned this pull request Oct 2, 2022

row-wise scrolling #11014

Closed

luukvbaal force-pushed the splitscroll branch from 520708b to c0d986a Compare October 3, 2022 17:29

luukvbaal changed the title ~~feat(window/ui): add splitscroll option~~ feat(window/ui): add splitkeep option Oct 3, 2022

luukvbaal force-pushed the splitscroll branch 3 times, most recently from b119175 to f581406 Compare October 5, 2022 21:22

feat(window/ui): add splitkeep option

7e32d6b

zeertzjq reviewed Oct 6, 2022

View reviewed changes

zeertzjq force-pushed the splitscroll branch from f581406 to 669867a Compare October 6, 2022 06:33

zeertzjq approved these changes Oct 6, 2022

View reviewed changes

zeertzjq merged commit 5acf52e into neovim:master Oct 6, 2022

github-actions bot removed the request for review from bfredl October 6, 2022 06:58

This comment was marked as off-topic.

Sign in to view

CharlesChiuGit mentioned this pull request Oct 6, 2022

New options from Nvim core ayamir/nvimdots#251

Closed

neovim deleted a comment from tricktux Oct 7, 2022

crivotz added a commit to crivotz/nv-ide that referenced this pull request Oct 10, 2022

Removed stabilize. Added to Neovim core neovim/neovim#19243

89043c6

dhruvinsh mentioned this pull request Oct 10, 2022

Future Stable build to look for (0.9.0) dhruvinsh/nvim#11

Closed

pidgeon777 mentioned this pull request Oct 10, 2022

Take space from current window instead of pushing window to the other side stevearc/aerial.nvim#134

Open

loqusion mentioned this pull request Oct 25, 2022

Files open in notification window nvim-neo-tree/neo-tree.nvim#575

Closed

sisrfeng mentioned this pull request Nov 6, 2022

wf4PR sisrfeng/neovim#1

Closed

This comment was marked as off-topic.

Sign in to view

This comment was marked as outdated.

Sign in to view

neovim locked as resolved and limited conversation to collaborators Nov 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(window/ui): add splitkeep option #19243

feat(window/ui): add splitkeep option #19243

luukvbaal commented Jul 5, 2022 •

edited

Loading

luukvbaal commented Jul 6, 2022 •

edited

Loading

lervag commented Jul 9, 2022

luukvbaal commented Jul 9, 2022

clason commented Jul 9, 2022

luukvbaal commented Jul 9, 2022

justinmk commented Jul 9, 2022

lewis6991 commented Jul 9, 2022

justinmk commented Jul 9, 2022 •

edited

Loading

lewis6991 commented Jul 9, 2022

baodrate commented Jul 9, 2022

luukvbaal commented Jul 9, 2022 •

edited

Loading

justinmk commented Jul 9, 2022 •

edited

Loading

luukvbaal commented Jul 9, 2022 •

edited

Loading

justinmk commented Jul 9, 2022 •

edited

Loading

luukvbaal commented Jul 10, 2022

luukvbaal commented Jul 10, 2022

zeertzjq commented Jul 10, 2022

lewis6991 commented Jul 11, 2022

luukvbaal commented Jul 11, 2022 •

edited

Loading

luukvbaal commented Jul 11, 2022

luukvbaal commented Sep 24, 2022

clason commented Sep 24, 2022

luukvbaal commented Sep 30, 2022 •

edited

Loading

zeertzjq Oct 6, 2022

zeertzjq Oct 6, 2022

zeertzjq left a comment •

edited

Loading

luukvbaal commented Oct 6, 2022

This comment was marked as off-topic.

luukvbaal commented Oct 7, 2022

This comment was marked as off-topic.

This comment was marked as outdated.

	* Find a value for w_topline that shows the cursor at the same
	* relative position in the window as before (more or less).

feat(window/ui): add splitkeep option #19243

feat(window/ui): add splitkeep option #19243

Conversation

luukvbaal commented Jul 5, 2022 • edited Loading

luukvbaal commented Jul 6, 2022 • edited Loading

lervag commented Jul 9, 2022

luukvbaal commented Jul 9, 2022

clason commented Jul 9, 2022

luukvbaal commented Jul 9, 2022

justinmk commented Jul 9, 2022

lewis6991 commented Jul 9, 2022

justinmk commented Jul 9, 2022 • edited Loading

lewis6991 commented Jul 9, 2022

baodrate commented Jul 9, 2022

luukvbaal commented Jul 9, 2022 • edited Loading

justinmk commented Jul 9, 2022 • edited Loading

luukvbaal commented Jul 9, 2022 • edited Loading

justinmk commented Jul 9, 2022 • edited Loading

luukvbaal commented Jul 10, 2022

luukvbaal commented Jul 10, 2022

zeertzjq commented Jul 10, 2022

lewis6991 commented Jul 11, 2022

luukvbaal commented Jul 11, 2022 • edited Loading

luukvbaal commented Jul 11, 2022

luukvbaal commented Sep 24, 2022

clason commented Sep 24, 2022

luukvbaal commented Sep 30, 2022 • edited Loading

zeertzjq Oct 6, 2022

Choose a reason for hiding this comment

zeertzjq Oct 6, 2022

Choose a reason for hiding this comment

zeertzjq left a comment • edited Loading

Choose a reason for hiding this comment

luukvbaal commented Oct 6, 2022

This comment was marked as off-topic.

luukvbaal commented Oct 7, 2022

This comment was marked as off-topic.

This comment was marked as outdated.

luukvbaal commented Jul 5, 2022 •

edited

Loading

luukvbaal commented Jul 6, 2022 •

edited

Loading

justinmk commented Jul 9, 2022 •

edited

Loading

luukvbaal commented Jul 9, 2022 •

edited

Loading

justinmk commented Jul 9, 2022 •

edited

Loading

luukvbaal commented Jul 9, 2022 •

edited

Loading

justinmk commented Jul 9, 2022 •

edited

Loading

luukvbaal commented Jul 11, 2022 •

edited

Loading

luukvbaal commented Sep 30, 2022 •

edited

Loading

zeertzjq left a comment •

edited

Loading