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

NERDTreeCWD: reset CWD if changed by NERDTreeFocus #878

Merged
merged 6 commits into from Sep 10, 2018

Conversation

PhilRunninger
Copy link
Member

Fixes #855.

When the user has 'autochdir' turned on, opening a new NERDTree will
cause the current working directory to change. To prevent this
happening, remember the CWD and reset it if NERDTreeFocus caused it to
change.

When the user has `'autochdir'` turned on, opening a new NERDTree will
cause the current working directory to change. To prevent this
happening, remember the CWD and reset it if NERDTreeFocus caused it to
change.
Copy link
Contributor

@lifecrisis lifecrisis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good start. However, I don't see any care taken to work around the possibility of a window-local current working directory. You would need to use the :lcd command in this case.

It's an edge case. But I think it deserves some investigation.

call NERDTreeFocus()
if l:cwd != getcwd()
exec 'cd '.l:cwd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should wrap l:cwd with fnameescape() here.

@lifecrisis
Copy link
Contributor

lifecrisis commented Aug 25, 2018

If you read the documentation at :help :NERDTreeCWD, it leads one to believe that our function should be more like this:

function! NERDTreeCWD()
    let l:cwdPath = g:NERDTreePath.New(getcwd())
    call NERDTreeFocus()

    if b:NERDTree.root.path.equals(l:cwdPath)
        return
    endif

    let l:newRoot = g:NERDTreeFileNode.New(l:cwdPath, b:NERDTree)
    call b:NERDTree.changeRoot(l:newRoot)
endfunction

This matches the documentation perfectly, reproduced below:

:NERDTreeCWD                                                  :NERDTreeCWD
    Change tree root to current directory. If no NERD tree exists for this
    tab, a new tree will be opened.

I assert that changing the working directory in any way is not in the scope of this command!

Also, the documentation may be improved a tad...

:NERDTreeCWD                                                  :NERDTreeCWD
    Change the NERDTree root to the current working directory.  If no
    NERDTree exists for this tab, a new one is opened.

EDIT: You can extend this idea to change the working directory of the NERDTree window (if it needs a local one) or to change the global working directory. The important idea here is that the details are saved before we leave the initial context.

EDIT: It should also be pointed out that changing the working directory is pointless when 'autochdir' is set, anyway. Even if we set the directory, leaving and coming back to the NERDTree will just reset it.

@lifecrisis
Copy link
Contributor

lifecrisis commented Aug 26, 2018

I thought about it more last night... I think my above comment and suggested function definition is right on the money. Even in the documentation for 'autochdir', we see:

Note: When this option is on some plugins may not work.

I just don't think the setting is working the way the user thinks it should.

@PhilRunninger
Copy link
Member Author

@lifecrisis , ready for a 2nd review.

@lifecrisis
Copy link
Contributor

Hey @PhilRunninger. Updates look great!

Just added a couple of commits on top of yours. If you're okay with everything, then so am I. Let me know if this is good or if you think of anything else.

@PhilRunninger
Copy link
Member Author

Looks good. I hadn't considered the CD mapping. It's good to have them use the same code.

@PhilRunninger PhilRunninger merged commit 15d06b6 into master Sep 10, 2018
@PhilRunninger PhilRunninger deleted the autochdir_interference branch September 10, 2018 13:27
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