neovim / neovim-ruby

Ruby support for Neovim
MIT License
340 stars 17 forks source link

Update the pwd on each request #22

Closed alexgenco closed 7 years ago

alexgenco commented 8 years ago

Attempts to resolve https://github.com/alexgenco/neovim-ruby/issues/21.

justinmk commented 8 years ago

If a DirChanged event were implemented could we avoid doing this on every request?

alexgenco commented 8 years ago

@justinmk yeah I think that would be an improvement, although we'd probably have to use something like this as a fallback for older versions.

alexgenco commented 7 years ago

@justinmk come to think of it, if DirChanged is in the nvim roadmap, I might just hold off on fixing this until then. Thoughts?

justinmk commented 7 years ago

Sure. Remind me if I forget...

alexgenco commented 7 years ago

Will readdress this when DirChanged is implemented.

justinmk commented 7 years ago

@alexgenco DirChanged will be implemented on nvim master soon: https://github.com/neovim/neovim/pull/5928

alexgenco commented 7 years ago

@justinmk I've been thinking about this one and it doesn't seem as straightforward as I was hoping. The ruby provider host can't hook into the DirChanged event without exposing a remote manifest to nvim, which would require the user to UpdateRemotePlugins or something in order to opt into the behavior (which I think is unacceptable, it should just work out of the box).

Here is a possible solution to add support to both legacy providers and remote plugins:

  1. Establish a DirChanged autocmd when loading ruby/python legacy providers. It could maybe happen right here, after the host channel is initialized: https://github.com/neovim/neovim/blob/c86caf7e41c77f85861fc92e1ef1462508d74b24/runtime/autoload/provider/ruby.vim#L57. The autocmd would simply rpcrequest(s:host, 'ruby_chdir', v:event) or something.
  2. Add a default DirChanged autocmd to all remote plugin manifests that updates the current working directory. Users won't have to define this themselves, it will happen automatically.

Thoughts? Has the python client come across this issue yet?

alexgenco commented 7 years ago

And of course there's always the naive approach that fetches getcwd() on every RPC request.

justinmk commented 7 years ago

The python client has the same issue and hasn't addressed it yet.

Establish a DirChanged autocmd when loading ruby/python legacy providers.

That was my original line of thought, except I assumed that the host could set this up itself (without specific support added to nvim runtime files) when it is first initialized. Am I missing something?

nvim_command('au DirChanged * call rpcrequest(...)')
alexgenco commented 7 years ago

@justinmk that might work, although now I'm running into some other issues, or maybe I'm overthinking it.

For example, if you :new inside a tabpage, then :lcd in the new split, :pwd returns different values in each split (which is expected). Then, if you :new again inside one of the two splits, you inherit the directory of the window you split from. Again, this seems like the correct behavior to me.

However I don't think there's a way for the Ruby host to determine the directory in this case, unless it can determine which window the current window split from. I would expect the logic to first look at whether the current window has a specified directory, then fallback to the current tabpage, then fallback to the global directory. But that logic doesn't work in this scenario because the last opened window would fall back to the tabpage default, rather than its parent window's.

Definitely feels like I'm missing something basic; your help would be appreciated :)

justinmk commented 7 years ago

@alexgenco You're right, the DirChanged event doesn't expose the internal/implicit directory changes, which happen more often than an explicit :lcd.

$ strace nvim -u NONE +'split|lcd ..|wincmd w' +qa 2>&1|grep chdir
chdir("/home/vagrant/neovim")           = 0
chdir("/home/vagrant")                  = 0
chdir("/home/vagrant/neovim")           = 0
chdir("..")                             = 0
chdir("/home/vagrant/neovim")           = 0

One could add a handler for e.g. BufWinEnter and/or WinEnter and then check the VimL getcwd() function. However based on the documentation that we've defined for :h DirChanged I think it might make sense for DirChanged to fire whenever the user focuses a window which causes the result of getcwd() to change.

justinmk commented 7 years ago

see https://github.com/neovim/neovim/issues/6054

justinmk commented 7 years ago

Should be fixed on nvim master.

alexgenco commented 7 years ago

Just redid this branch to account for the new nvim behavior. @justinmk a couple thoughts/questions:

  1. This implementation has no logic around the scope of the directory change. As far as I can tell, updating the host process's pwd on every DirChanged event should always put us in the right place. Does that sound right?
  2. Are the legacy provider and rplugin host run as separate co processes? Updating the global pwd like this could have unintended interactions if not.
justinmk commented 7 years ago

As far as I can tell, updating the host process's pwd on every DirChanged event should always put us in the right place. Does that sound right?

Yes. The "scope" is just there in case plugins need it.

Are the legacy provider and rplugin host run as separate co processes? Updating the global pwd like this could have unintended interactions if not.

Not sure, actually. We could change it if needed (proof-of-concept PR would be a big help).

alexgenco commented 7 years ago

Did some investigation, it looks like they are separate processes with independent working dirs, so I think we're good.

I should also note this change doesn't add directory tracking for remote plugins, only the legacy provider. I feel like the rplugin API is flexible enough that users can add the autocmd themselves if they need it.

I'll merge and cut a release sometime this weekend unless there's more feedback.