neoscroll.nvim icon indicating copy to clipboard operation
neoscroll.nvim copied to clipboard

feat: implement support for arbitrary winids

Open alfaix opened this issue 2 years ago • 7 comments

Added neoscroll.*_win functions that have the same signature, but take an additional winid parameter.

That required some refactoring:

  1. Most util functions now take a winid parameter
  2. State is now stored in WindowState[winid] instead of module-level variables

Unfortunately, some functions do not have a per-winid alternatives (namely, winline, foldclosed, foldclosedend) - or at least I'm not aware of them. For these functions, I do vim.api.nvim_win_call(fn), which may (or may not) impact performance. For scrolling current window, nothing changed.

I didn't check the performance impact of calling a bunch of functions in nvim_win_call - if it's large, it may make sense to just call the whole callback inside of it and work with winid=0 everywhere else. I don't know if being inside nvim_win_call for a long time may have other side effects though.

I did some manual testing, everything seems to work the same way it did. Attaching videos (one recorded with the master branch, one recorded with this PR). Apologies for potato quality, GitHub doesn't like big files, and my Firefox doesn't like HEVC. Also tested closing the window mid-scroll, though that's not in the recording.

If there any other tests/benchmarks I can run (automatic or manual), please let me know.

https://user-images.githubusercontent.com/18747677/174565698-576a61b0-07e8-4dfd-ba84-4f0fbb61f525.mp4

https://user-images.githubusercontent.com/18747677/174565208-54563c7e-ae79-404a-997d-9a13fd40629e.mp4

alfaix avatar Jun 20 '22 09:06 alfaix

Can you rebase to upstream master? I fixed some issues and added some tests.

karb94 avatar Jul 20 '22 22:07 karb94

I did rebase, though I'm afraid some indents and formatting are inconsistent which has led to an inflated diff. If you're using an autoformatter, could you please share its configuration, so I can run it? Fixing stuff manually doesn't sound very appealing.

Second, I'm not really sure how to run the tests, so some of them may be failing. I reproduced the stuff in my own videos post-rebase, though tests may still break. Could you please explain how to run the tests?

Thanks!

alfaix avatar Jul 21 '22 12:07 alfaix

Sorry about the formatting mess. Apparently I forgot to enable my autoformatter at some point so thank you for pointing that out. I use StyLua with the following stylua.toml file:

column_width = 100
line_endings = "Unix"
indent_type = "Spaces"
indent_width = 2
quote_style = "AutoPreferDouble"
call_parentheses = "Always"

I will add the stylua.toml file to the repo once we merge this.

Regarding the tests, I run the following command from the root directory of the project:

nvim --headless -c "PlenaryBustedDirectory lua/tests {minimal_init = 'lua/tests/minimal_init.vim'}"

Let me know if you have any problems running the tests (it's my first time setting up tests in lua/neovim). It's likely that some of them won't succeed after these changes. Don't worry too much about them for now.

I will have some time to review and adapt the tests next weekend. I did have a chance to take a quick look when you submitted the PR and looked great. Thank you so much for your help with this!

karb94 avatar Jul 23 '22 10:07 karb94

Hi again, pardon the long wait, was moving to a new place.

I reformatted the code and added stylua.toml to the root of the repo, though that has led to an even bigger diff. Perhaps it might make sense to reformat master first and then merge this PR? I also fixed one test which I think was broken (it tested for wo.scrolloff without opts.use_local_scrolloff)

It might also make sense to eventually implement tests for separate windows, but I think this pr is bloated enough as it is.

alfaix avatar Jul 29 '22 09:07 alfaix

Is the code supposed to work at this stage or it's still in progress? I get a attempt to call field 'nvim_cmd' (a nil value) error whenever I try to scroll. I'm just using the defaults (require('neoscroll').setup {}). I haven't had time to investigate further.

karb94 avatar Jul 31 '22 22:07 karb94

Definitely supposed to work. I'm guessing it's a Neovim version thing - I use 0.8 and only tested on 0.8, sorry.

Will test on 0.5 and update the PR

alfaix avatar Aug 01 '22 05:08 alfaix

Added a polyfill for nvim_cmd to support earlier neovims. Some tests still fail on v0.5.1 , as far as I can tell, this is due to neovim/neovim#13964, which is not fixed in v0.5.1. All of the failing tests are in the respect_scrolloff_spec file.

I could add a couple of crutches in the tests to work around it, but I'm not sure if that's the right thing to do, as https://github.com/karb94/neoscroll.nvim/commit/54c5c419f6ee2b35557b3a6a7d631724234ba97a removes all mention of use_local_scrolloff. Although my PR adds that back...

Basically the question is, should setups with broken neovim/neovim#13964 still be supported? If yes, then use_local_scrolloff probably needs to be reintroduced. If not, then it probably makes sense to bump the minimum neovim version required in README.md

alfaix avatar Aug 01 '22 08:08 alfaix