BUGFIX: Allow ":NERDTreeFind" to reveal new files #785

lifecrisis · 2017-12-21T15:10:38Z

Here, I'm addressing issue #779.

When a new file is written after the NERDTree for the current tab has been initialized, attempting to use :NERDTreeFind on that new file will fail with an error message. The reason for this is subtle, but it has to do with the following functions:

NEDRTreeDirNode.reveal() (this is what really drives the :NERDTreeFind command)
NERDTreeDirNode.open()
NERDTreeDirNode._initChildren()

First, note that reveal() must open() all nodes along the path, starting with root, to "get to" the desired node. Also note: open() calls _initChildren() only if the node's children have not yet been initialized. This is for efficiency... but doesn't help much here because nodes will likely need to be opened anyway. It only helps if one of the dirs along the path has already been opened and has, say 200+ children.

That last point is the key. When the root is open()ed, it's children are not checked again. However, if we create a new file several dirs deep (dirs that have not yet been opened), the :NERDTreeFind call will succeed because the dirs along the path are opened after the path was created.

In effect, the find command is searching the tree, not the file system... so some mechanism is needed to "freshly open" the nodes along the search path as we go. We can be sure the path exists, we just need to make sure it's guaranteed to be in the tree when we get there!

The last commit on this branch fixes the issue in the most efficient way possible... but some may still find it undesirable... Read the comment in that commit when it shows up to see why it's most efficient...

Oh, and feed back will be appreciated!

PhilRunninger

I'm still getting an error with this workflow:

:NERDTreeToggle
<C-W><C-W>
:w foobar
:NERDTreeFind

PhilRunninger · 2017-12-21T15:29:39Z

autoload/nerdtree/ui_glue.vim

+
+    let l:node = b:NERDTree.root.reveal(l:pathObj)
+
+    if empty(l:node)


Is this if block left over from testing? It seems out of place.

PhilRunninger · 2017-12-21T15:30:11Z

autoload/nerdtree/ui_glue.vim

+    let l:node = b:NERDTree.root.reveal(l:pathObj)
+
+    if empty(l:node)
+        echomsg 'l:node is totally empty...'


If this is to be kept, use the nerdtree function instead of echomsg.

The ":NERDTreeFind" command calls the "reveal()" method on the NERDTree root node. The "reveal()" method would, in turn, call the node's "open()" method. Since the "open()" method would only initialize the child nodes of the root (i.e., read them from disk) when the list of child nodes was empty, new paths would not be included in the list. This commit will result in the refreshing of the child node list whenever "reveal()" is called on a directory node (unless it is the first time the node is being opened... the most efficient option). The result is that ":NERDTreeFind" will discover newly created paths that exist on disk but are not cached in the NERDTree. A stray debugging message is also removed. Fixes issue preservim#779.

lifecrisis · 2017-12-21T15:37:24Z

@PhilRunninger... I hadn't pushed the fix yet! Try it now!

Hadn't pushed fix yet...

lifecrisis · 2017-12-21T15:43:42Z

@juanibiapina ... if you want to weigh in, let me know what you think. I think this is as efficient as its gonna get.

lifecrisis · 2017-12-21T15:48:06Z

Just a note... documentation still needs to be updated, and it looks like the function behaves improperly when the buffer doesn't have an associated path (try :NERDTreeFind in the quickfix window for example...).

PhilRunninger · 2017-12-21T16:20:35Z

It looks like :NERDTreeFind, run from the Quickfix window, takes you to cwd(). What do you suggest it should be doing?

PhilRunninger · 2017-12-21T16:20:51Z

Sorry. Wrong button.

lifecrisis · 2017-12-21T17:53:46Z

Well, previously, :NERDTreeFind would tell you that no file existed for the current buffer (if it was a new buffer, say). I think that should be the default behavior.

If you specify a directory as an arg... then it should reveal the directory as expected.

That makes the most sense to me, anyway.

If a file does not exist for the current buffer, this function should fail with a clear warning message. Here, I improved the messages that this function prints so that it fails gracefully when no path can be determined in the calling context.

The docs for ":NERDTreeFind" are updated. Some additional formatting changes are made to other sections.

lifecrisis · 2017-12-22T14:26:09Z

@PhilRunninger,

This pull request is O.K., but I'm not happy with the efficiency of my solution. I don't really know a better way to do it... Here, I refresh the nodes along the path, which is better than refreshing the whole tree... however, it could be more efficient if I added just the new file to the tree (a single node).

I'm ambivalent, because the file we're finding could be in an unopened, but huge directory... which will be slow no matter what you do. Thus, worrying about efficiency will be a waste of our time in probably half to three quarters of all situations.

I've tried this on a directory of 350 nodes... it takes about 1.06s. To me this is acceptable... if a developer is structuring their projects well, directories will be much smaller than this anyway.

Large directories are just always going to be a problem... basically no matter what you do...

What do you think?

PhilRunninger · 2017-12-22T14:31:55Z

I agree. Large directories and vimscript itself both work against a perfect solution. I think we're OK to go with this.

lifecrisis · 2017-12-22T14:33:27Z

Sounds good. I'll review and merge in the next half-hour or so.

The small change here reverts an attempted bugfix from pull request children of the root whenever ":NERDTreeFind" was invoked. This was disruptive (as reported in preservim#793), so a new method must be found to solve the problem of ":NERDTreeFind" not opening newly created files. Fixes preservim#793.

The small change here reverts an attempted bugfix from pull request of the root whenever ":NERDTreeFind" was invoked. This was disruptive (as reported in preservim#793), so a new method must be found to solve the problem of ":NERDTreeFind" not opening newly created files. Fixes preservim#793.

The small change here reverts an attempted bugfix from pull request number 785. That change resulted in the unintended side-effect of closing other children of the root the root whenever ":NERDTreeFind" was invoked. This was disruptive (as reported in preservim#793), so a new method must be found to solve the problem of ":NERDTreeFind" not opening newly created files. Fixes preservim#793.

The small change here reverts an attempted bugfix from preservim#785. That change resulted in the unintended side-effect of closing other children of the root whenever ":NERDTreeFind" was invoked. This was disruptive (as reported in preservim#793), so a new method must be found to solve the problem of ":NERDTreeFind" not opening newly created files. Fixes preservim#793.

Revert the bugfix from pull request #785 and reopen issue #779. We keep the good style changes from PR #785 intact.

lifecrisis · 2018-01-05T13:36:50Z

NOTE: This PR was reverted (but all style changes were kept) in PR #794. The reasons for the change are given there.

Jason Franklin added 3 commits December 21, 2017 08:28

Remove code duplication in "findAndRevealPath"

496d9a1

Improve variable naming in "findAndRevealPath"

5301dd2

Add a debugging message

90d08dc

lifecrisis requested a review from PhilRunninger December 21, 2017 15:10

PhilRunninger previously requested changes Dec 21, 2017

View reviewed changes

PhilRunninger approved these changes Dec 21, 2017

View reviewed changes

PhilRunninger closed this Dec 21, 2017

PhilRunninger reopened this Dec 21, 2017

Jason Franklin added 2 commits December 22, 2017 08:45

Have "finAndRevealPath()" fail on no file

01b011d

If a file does not exist for the current buffer, this function should fail with a clear warning message. Here, I improved the messages that this function prints so that it fails gracefully when no path can be determined in the calling context.

Update the documentation

f6dad47

The docs for ":NERDTreeFind" are updated. Some additional formatting changes are made to other sections.

lifecrisis force-pushed the find-new-file branch from cd2ac54 to f6dad47 Compare December 22, 2017 14:16

lifecrisis merged commit 57788ab into preservim:master Dec 22, 2017

lifecrisis deleted the find-new-file branch December 22, 2017 14:44

lifecrisis mentioned this pull request Dec 22, 2017

NERDTreeFind on a new file in a new directory fails with E718: Funcref required #667

Closed

lifecrisis mentioned this pull request Jan 2, 2018

NERDTreeFind closes all currently expanded nodes in the tree view. #793

Closed

lifecrisis added a commit that referenced this pull request Jan 5, 2018

Merge pull request #794 from lifecrisis/reopen779

5fc48b0

Revert the bugfix from pull request #785 and reopen issue #779. We keep the good style changes from PR #785 intact.

lifecrisis mentioned this pull request Jan 5, 2018

A newly written file cannot be found with NERDTreeFind #779

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX: Allow ":NERDTreeFind" to reveal new files #785

BUGFIX: Allow ":NERDTreeFind" to reveal new files #785

lifecrisis commented Dec 21, 2017

PhilRunninger left a comment

PhilRunninger Dec 21, 2017

PhilRunninger Dec 21, 2017

lifecrisis commented Dec 21, 2017

lifecrisis commented Dec 21, 2017

lifecrisis commented Dec 21, 2017

PhilRunninger commented Dec 21, 2017

PhilRunninger commented Dec 21, 2017

lifecrisis commented Dec 21, 2017

lifecrisis commented Dec 22, 2017 •

edited

Loading

PhilRunninger commented Dec 22, 2017

lifecrisis commented Dec 22, 2017

lifecrisis commented Jan 5, 2018


		let l:node = b:NERDTree.root.reveal(l:pathObj)

		if empty(l:node)

BUGFIX: Allow ":NERDTreeFind" to reveal new files #785

BUGFIX: Allow ":NERDTreeFind" to reveal new files #785

Conversation

lifecrisis commented Dec 21, 2017

PhilRunninger left a comment

Choose a reason for hiding this comment

PhilRunninger Dec 21, 2017

Choose a reason for hiding this comment

PhilRunninger Dec 21, 2017

Choose a reason for hiding this comment

lifecrisis commented Dec 21, 2017

lifecrisis commented Dec 21, 2017

lifecrisis commented Dec 21, 2017

PhilRunninger commented Dec 21, 2017

PhilRunninger commented Dec 21, 2017

lifecrisis commented Dec 21, 2017

lifecrisis commented Dec 22, 2017 • edited Loading

PhilRunninger commented Dec 22, 2017

lifecrisis commented Dec 22, 2017

lifecrisis commented Jan 5, 2018

lifecrisis commented Dec 22, 2017 •

edited

Loading