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

Update the pwd on each request #22

Merged
merged 3 commits into from
Mar 18, 2017
Merged

Update the pwd on each request #22

merged 3 commits into from
Mar 18, 2017

Conversation

alexgenco
Copy link
Collaborator

Attempts to resolve #21.

@justinmk
Copy link
Member

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

@alexgenco
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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
Copy link
Member

justinmk commented Oct 8, 2016

Sure. Remind me if I forget...

@alexgenco
Copy link
Collaborator Author

Will readdress this when DirChanged is implemented.

@justinmk
Copy link
Member

@alexgenco DirChanged will be implemented on nvim master soon: neovim/neovim#5928

@alexgenco
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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

@justinmk
Copy link
Member

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
Copy link
Collaborator Author

@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
Copy link
Member

justinmk commented Feb 3, 2017

@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
Copy link
Member

justinmk commented Feb 3, 2017

see neovim/neovim#6054

@justinmk
Copy link
Member

Should be fixed on nvim master.

@alexgenco
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator Author

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.

@alexgenco alexgenco force-pushed the 21-pwd branch 3 times, most recently from 12bb473 to 1e9c4fd Compare March 18, 2017 19:17
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.

Ruby support does not keep up with directory changes
2 participants