Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(winfixbuf): avoid windows that have winfixbuf when opening buffers #1390

Merged
merged 4 commits into from Mar 13, 2024

Conversation

pysan3
Copy link
Collaborator

@pysan3 pysan3 commented Mar 11, 2024

Ref: neovim/neovim#12517

First challenge to workaround new winfixbuf option added to nvim-nightly.

Please read the long discussion about this new feature here: vim/vim#13903.

@pysan3 pysan3 marked this pull request as draft March 11, 2024 09:13
@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 11, 2024

Wait a second.

@pysan3 pysan3 marked this pull request as ready for review March 11, 2024 11:15
@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 11, 2024

It was quite complicated...

If you do

  • :e some-file
  • :set winfixbuf
  • :Neotree left

we have two windows and two buffers.

  • winid: 1000, bufname: some-file, vim.wo.winfixbuf == true
  • winid: 1001, bufname: neo-tree filesystem [1], vim.wo.winfixbuf == false

Now try to open a new-file from neo-tree with <CR>, neo-tree tries to open the file at window 1000, but this fails with error E1513: Cannot edit buffer. 'winfixbuf' is enabled.

So we need to open a new split just like when neo-tree is the single window.
This PR opens a new split to open new-file, so the windows are like this now.

  • winid: 1000, bufname: some-file, vim.wo.winfixbuf == true
  • winid: 1001, bufname: neo-tree filesystem [1], vim.wo.winfixbuf == false
  • winid: 1002, bufname: new-file, vim.wo.winfixbuf == false

Here comes the difficult part. Run vim.api.nvim_set_current_win(1000) and then go back to neo-tree.
When you try to open yet another file another-file, get_appropriate_window tries to open at the most recent focused window before entering neo-tree (i.e. 1000). But again, this is winfixbuf so neo-tree falls back to creating a new split as I explained above.
However what we actually want here is to find window 1002 and open another-file there.
So in get_appropriate_window, it should find the most recent focused window but not winfixbuf instead.

  • winid: 1000, bufname: some-file, vim.wo.winfixbuf == true
  • winid: 1001, bufname: neo-tree filesystem [1], vim.wo.winfixbuf == false
  • winid: 1002, bufname: another-file, vim.wo.winfixbuf == false

The story doesn't end here. Run vim.api.nvim_set_current_win(1000) and go back to neo-tree once again.
Now, select some-file and <CR>.
If get_appropriate_window finds the most recent focused window but not winfixbuf, some-file will be opened in window 1002.
But since window 1000 is already some-file, we want to open some-file in 1000 in this case.

  • winid: 1000, bufname: some-file, vim.wo.winfixbuf == true <- do nothing!
  • winid: 1001, bufname: neo-tree filesystem [1], vim.wo.winfixbuf == false
  • winid: 1002, bufname: another-file, vim.wo.winfixbuf == false

This is the reason why I had to wait for result, err in open_file and then decide where to open instead.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 11, 2024

At first glance, you might think why not set winfixbuf for all neo-tree windows?
Unfortunately this is not acceptable since neo-tree window may be overwritten by another neo-tree source (which is in a different buffer).

If we set winfixbuf for neo-tree windows, things such as <, > from source selector won't work.

Then now you might wonder, why not open a new dedicated window for each source and close the previous one?
Uh-uh, we will have the flickering issue once again #713 (comment).

@cseickel
Copy link
Contributor

cseickel commented Mar 11, 2024

winid: 1000, bufname: some-file, vim.wo.winfixbuf == true <- do nothing!

I don't consider that a valid situation that we should have code to cover. It's not reasonable for a file that someone would open from neo-tree has winfixbuf set, because that is not what the option is meant for.

Also, the tests are failing so there's still more work to do here.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 12, 2024

winid: 1000, bufname: some-file, vim.wo.winfixbuf == true <- do nothing!

I don't consider that a valid situation that we should have code to cover. It's not reasonable for a file that someone would open from neo-tree has winfixbuf set, because that is not what the option is meant for.

I doubt that. I will want to occasionally reference a single file while editing multiple files, and manually triggering winfixbuf will be very helpful.
Eg pinning defaults.lua to see the config options while doing other stuffs.

Also, the tests are failing so there's still more work to do here.

This is more of an existing issue.

You can replicate this by

  • :e <file-A>
  • :Neotree float !Don't close this
  • :vsplit <file-B>

The expected behavior is 1) neo-tree closes, 2) a new window with file-B opens next to file-A.
Whereas with the latest main, 1) neo-tree closes, 2) file-B opens on top of file-A, single window.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 12, 2024

In order to avoid this issue, we need to basically wait one more event loop.

Therefore, vim.cmd(cmd) to close/toggle neo-tree in the tests finish too early and :Neotree close does not finish completely in the current event loop.

Running a short vim.wait after each vim.cmd will make all tests succeed.

image

I'll push these changes as well.

@cseickel
Copy link
Contributor

I doubt that. I will want to occasionally reference a single file while editing multiple files, and manually triggering winfixbuf will be very helpful.
Eg pinning defaults.lua to see the config options while doing other stuffs.

That makes no sense, you are abusing the new feature which is meant for tool windows. If you don't want to open a new file in the window with defaults.lua, just do whatever you do now to make that happen.

I'm going to rant for a second here. This is part of the reason why I'm on the edge of leaving the project. People have so many bad ideas predicated on unnaturally forcing nvim to be vs code when there are much simpler ways of doing things. If you are going to use splits and want to control which windows something opens in, then go to/create the window you want to use first, then use :Neotree current to pick the file. Most of the time a sidebar is just not the right way to do it and it over complicates everything if you insist of using a sidebar for every use case.

Yes we should avoid opening in winfixbuf windows rather than cause an error. No we should not add extra code to ensure it focuses a window that is obviously already open if someone tries to open a file that is already visible and that file happens to have been set with winfixbuf. That's just ridiculous.

This is more of an existing issue.

You can replicate this by

:e
:Neotree float !Don't close this
:vsplit
The expected behavior is 1) neo-tree closes, 2) a new window with file-B opens next to file-A.
Whereas with the latest main, 1) neo-tree closes, 2) file-B opens on top of file-A, single window.

I don't quite get that, !Don't close this a name of a file? How is it relevant? Is :vsplit <file-B> meant to convey using a mapping in neo-tree that should open another file in a split?

In order to avoid this issue, we need to basically wait one more event loop.

Therefore, vim.cmd(cmd) to close/toggle neo-tree in the tests finish too early and :Neotree close does not finish completely in the current event loop.

Running a short vim.wait after each vim.cmd will make all tests succeed.

image

I'll push these changes as well.

That's good. I know the tests are flaky and if a simple wait is all that's needed then that's awesome.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 12, 2024

Most of the time a sidebar is just not the right way to do it and it over complicates everything if you insist of using a sidebar for every use case.

I absolutely agree with this lol We should make position = "float" the default in the next version tbh.

Yes we should avoid opening in winfixbuf windows rather than cause an error. No we should not add extra code to ensure it focuses a window that is obviously already open if someone tries to open a file that is already visible and that file happens to have been set with winfixbuf. That's just ridiculous.

My story was basically explaining why it took so long to get the final version from the first iteration.
The PR I propose here actually kills all birds at once so there's no toggle between supporting this feature or not.

I haven't explained above but since user can also call s (open_split) it is not an easy task anyways.
When neo-tree is floating and user does one of open, open_split, open_vsplit (and other open commands), neo-tree must first nvim_set_current_win to a ground window (non-floating) and then perform vim.cmd.split, vim.cmd.edit, ... relative to that window.
When this ground window is winfixbuf, vim.cmd.split works just fine but vim.cmd.edit won't. And there's no easy way of detecting which command is going to be performed due to how the code is structured. All these commands end up in utils.open_files.

Actually open a file that is already visible in winfixbuf succeeds first, and then the normal operation will happen after initial try fails. So that feature is actually a by-product that just happens. It'll be more difficult to prevent that.

I don't quite get that, !Don't close this a name of a file? How is it relevant? Is :vsplit <file-B> meant to convey using a mapping in neo-tree that should open another file in a split?

  • :e <file-A>
  • :Neotree float
    • Don't close this
  • :vsplit <file-B>

!Don't close this was just a comment, meaning to literally press : and start writing vsplit while floating neo-tree is in focus.

record.mp4

Lazy won't close automatically so there's a difference, but you can see that the new buffer should be created as a new split in the background.

@cseickel
Copy link
Contributor

Regarding the floating neo-tree -> vsplit command issue, that seems like more of a neovim glitch to me, I'm not sure how we are affecting that behavior. We do automatically close the window when it loses focus so that must be related. Either way it's another silly thing to do so I wouldn't personally give it a second thought one way or another.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 12, 2024

Regarding the floating neo-tree -> vsplit command issue, that seems like more of a neovim glitch to me, I'm not sure how we are affecting that behavior. We do automatically close the window when it loses focus so that must be related. Either way it's another silly thing to do so I wouldn't personally give it a second thought one way or another.

Please test this branch. The issue should be fixed with this PR.

@cseickel cseickel merged commit d8b4687 into nvim-neo-tree:main Mar 13, 2024
2 checks passed
@cseickel
Copy link
Contributor

Somehow this commit has caused a bug with :Neotree current toggle.

  1. If you have more than one split,
  2. and you use :Neotree current toggle
  3. then open a file
  4. then :Neotree current toggle
  5. you'll get an error (see below)
  6. then your current window will be closed

Error from step 5:

Error executing vim.schedule lua callback: ...is/.local/share/nvim/lazy/nui.nvim/lua/nui/tree/init.lua:188: missing bufnr                                                     
stack traceback:                                                                                                                                                              
        [C]: in function 'error'                                                                                                                                              
        ...is/.local/share/nvim/lazy/nui.nvim/lua/nui/tree/init.lua:188: in function 'init'                                                                                   
        .../.local/share/nvim/lazy/nui.nvim/lua/nui/object/init.lua:132: in function 'NuiTree'                                                                                
        ...e/chris/repos/neo-tree.nvim/lua/neo-tree/ui/renderer.lua:778: in function 'create_tree'                                                                            
        ...e/chris/repos/neo-tree.nvim/lua/neo-tree/ui/renderer.lua:1190: in function 'draw'                                                                                  
        ...e/chris/repos/neo-tree.nvim/lua/neo-tree/ui/renderer.lua:1326: in function 'show_nodes'                                                                            
        ...ree.nvim/lua/neo-tree/sources/filesystem/lib/fs_scan.lua:96: in function 'render_context'                                                                          
        ...ree.nvim/lua/neo-tree/sources/filesystem/lib/fs_scan.lua:165: in function <...ree.nvim/lua/neo-tree/sources/filesystem/lib/fs_scan.lua:164> 

@pysan3 I'm not sure I am going to be able to debug so I was thinking about reverting it for now. Do you want to look into it before I do?

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 20, 2024

I don't see this error with nvim-nightly in the first place so there must be an issue (or I used a nightly only feature again) with nvim-stable.

Plz wait for one hour and I'll report you back.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 20, 2024

@cseickel Found the bug.

@pysan3
Copy link
Collaborator Author

pysan3 commented Mar 20, 2024

Could you also check if it's working before merging the PR? @cseickel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants