From 81a42acb97deac6b83db464999d77036c5bf42da Mon Sep 17 00:00:00 2001 From: Jason Franklin Date: Sat, 10 Jun 2017 09:20:52 -0400 Subject: [PATCH 1/5] Refactor the bookmark query function The function in "bookmark.vim" that allows the caller to query the list of Bookmarks by name had stale commentary. In addition, the internals of the function needed to be reworked to improve readability. Making this function very clean is important because it is heavily used elsewhere. As a side note, it might be beneficial to later rename this function to something like "GetBookmarkByName" to further improve readability. That change is not critical and can be safely delayed. --- lib/nerdtree/bookmark.vim | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/nerdtree/bookmark.vim b/lib/nerdtree/bookmark.vim index ad8c3d1..9024077 100644 --- a/lib/nerdtree/bookmark.vim +++ b/lib/nerdtree/bookmark.vim @@ -44,15 +44,20 @@ function! s:Bookmark.BookmarkExistsFor(name) endfunction " FUNCTION: Bookmark.BookmarkFor(name) {{{1 -" Class method to get the bookmark that has the given name. {} is return if no -" bookmark is found +" Class method that returns the Bookmark object having the specified name. +" Throws "NERDTree.BookmarkNotFoundError" if no Bookmark is found. function! s:Bookmark.BookmarkFor(name) - for i in s:Bookmark.Bookmarks() - if i.name ==# a:name - return i + let l:result = {} + for l:bookmark in s:Bookmark.Bookmarks() + if l:bookmark.name ==# a:name + let l:result = l:bookmark + break endif endfor - throw "NERDTree.BookmarkNotFoundError: no bookmark found for name: \"". a:name .'"' + if empty(l:result) + throw 'NERDTree.BookmarkNotFoundError: "' . a:name . '" not found' + endif + return l:result endfunction " FUNCTION: Bookmark.BookmarkNames() {{{1 From b0f60552eabede5d698cb95fba2b17dbceb6e734 Mon Sep 17 00:00:00 2001 From: Jason Franklin Date: Sat, 10 Jun 2017 09:57:18 -0400 Subject: [PATCH 2/5] Rewrite and expand the header in bookmark.vim The header in "bookmark.vim" was pretty weak. It provided no description of the class it contains and no direction for the reader. In particular it did not note the dual purpose of the "Bookmark" class. The fact that the "Bookmark" class serves two purposes must be noted because many readers will expect class definitions to obey the single responsibility principle! If there is a chance for a major refactor of this class in the future, a priority would be splitting the class in two so that a "BookmarkList" class can assume the responsibility for providing a container for all "Bookmark" objects. --- lib/nerdtree/bookmark.vim | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/nerdtree/bookmark.vim b/lib/nerdtree/bookmark.vim index 9024077..d56407f 100644 --- a/lib/nerdtree/bookmark.vim +++ b/lib/nerdtree/bookmark.vim @@ -1,5 +1,13 @@ -"CLASS: Bookmark -"============================================================ +" ============================================================================ +" CLASS: Bookmark +" +" The Bookmark class serves two purposes: +" (1) It is the top-level prototype for new, concrete Bookmark objects. +" (2) It provides an interface for client code to query and manipulate the +" global list of Bookmark objects within the current Vim session. +" ============================================================================ + + let s:Bookmark = {} let g:NERDTreeBookmark = s:Bookmark From c0b90811b08b336c4378cff17e0b6af578ae2b18 Mon Sep 17 00:00:00 2001 From: Jason Franklin Date: Sat, 10 Jun 2017 11:02:28 -0400 Subject: [PATCH 3/5] Add an argument sigil in a Bookmark class method A missing argument sigil is effectively a syntax error in VimL. The function in which the error occurred was called in the execution of at least three buffer-local NERDTree commands: 1. :Bookmark (specifically, when trying to overwrite a Bookmark) 2. :OpenBookmark 3. :RevealBookmark Only one specific type of error message associated with these commands is fixed here (see issue #677). The problems with the above commands are not fully addressed by this commit, and their behavior can be improved immensely by further refactoring. However, no one has been able to even use these commands at all before now because the fix given here was not in place. More work will need to be done to improve the behavior of these commands so that they truly function as any reasonable user would expect. Fixes #677. --- lib/nerdtree/bookmark.vim | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/nerdtree/bookmark.vim b/lib/nerdtree/bookmark.vim index d56407f..bf95621 100644 --- a/lib/nerdtree/bookmark.vim +++ b/lib/nerdtree/bookmark.vim @@ -172,11 +172,13 @@ function! s:Bookmark.getNode(nerdtree, searchFromAbsoluteRoot) endfunction " FUNCTION: Bookmark.GetNodeForName(name, searchFromAbsoluteRoot, nerdtree) {{{1 -" Class method that finds the bookmark with the given name and returns the -" treenode for it. +" Class method that returns the tree node object for the Bookmark with the +" given name. Throws "NERDTree.BookmarkNotFoundError" if a Bookmark with the +" name does not exist. Throws "NERDTree.BookmarkedNodeNotFoundError" if a +" tree node for the named Bookmark could not be found. function! s:Bookmark.GetNodeForName(name, searchFromAbsoluteRoot, nerdtree) - let bookmark = s:Bookmark.BookmarkFor(a:name) - return bookmark.getNode(nerdtree, a:searchFromAbsoluteRoot) + let l:bookmark = s:Bookmark.BookmarkFor(a:name) + return l:bookmark.getNode(a:nerdtree, a:searchFromAbsoluteRoot) endfunction " FUNCTION: Bookmark.GetSelected() {{{1 From a03a639390011a51358ec3460a3be58d6cd86e8a Mon Sep 17 00:00:00 2001 From: Jason Franklin Date: Sat, 10 Jun 2017 13:29:27 -0400 Subject: [PATCH 4/5] Refactor and re-document Bookmark.getNode() A few minor changes were made to the "Bookmark.getNode()" function for the purposes of improving readability and documentation clarity. This process also led me to the conclusion that the "findNode()" function should be refactored to throw an error if a node cannot be found. This would lead to greater uniformity in the reporting of failures to find a node. It is generally better style to have an error thrown as close to the source as possible. A substantial change like this should wait for now. --- lib/nerdtree/bookmark.vim | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/nerdtree/bookmark.vim b/lib/nerdtree/bookmark.vim index bf95621..268664b 100644 --- a/lib/nerdtree/bookmark.vim +++ b/lib/nerdtree/bookmark.vim @@ -157,18 +157,23 @@ function! s:Bookmark.delete() endfunction " FUNCTION: Bookmark.getNode(nerdtree, searchFromAbsoluteRoot) {{{1 -" Gets the treenode for this bookmark +" Returns the tree node object associated with this Bookmark. +" Throws "NERDTree.BookmarkedNodeNotFoundError" if the node is not found. " " Args: -" searchFromAbsoluteRoot: specifies whether we should search from the current -" tree root, or the highest cached node +" searchFromAbsoluteRoot: boolean flag, search from the highest cached node +" if true and from the current tree root if false function! s:Bookmark.getNode(nerdtree, searchFromAbsoluteRoot) - let searchRoot = a:searchFromAbsoluteRoot ? a:nerdtree.root.AbsoluteTreeRoot() : a:nerdtree.root - let targetNode = searchRoot.findNode(self.path) - if empty(targetNode) - throw "NERDTree.BookmarkedNodeNotFoundError: no node was found for bookmark: " . self.name + if a:searchFromAbsoluteRoot + let l:searchRoot = a:nerdtree.root.AbsoluteTreeRoot() + else + let l:searchRoot = a:nerdtree.root endif - return targetNode + let l:targetNode = l:searchRoot.findNode(self.path) + if empty(l:targetNode) + throw 'NERDTree.BookmarkedNodeNotFoundError: node for bookmark "' . self.name . '" not found' + endif + return l:targetNode endfunction " FUNCTION: Bookmark.GetNodeForName(name, searchFromAbsoluteRoot, nerdtree) {{{1 From 3063dfb76631ecadde1b947d21b1422a3c8a53c4 Mon Sep 17 00:00:00 2001 From: Jason Franklin Date: Sat, 10 Jun 2017 15:59:56 -0400 Subject: [PATCH 5/5] Refactor the :OpenBookmark command I altered the behavior of the ":OpenBookmark" command to match that of the "NERDTree-o" mapping. This is acceptable for the following reasons: 1. It was broken, so no one was using it. 2. The name matches its behavior. If a bookmark is to be opened in an explorer window, we should have a command with a matching name for that behavior (":ExploreBookmark", for example). This can be added later if there is enough demand for the feature. Otherwise, this is a perfectly valid change. --- autoload/nerdtree/ui_glue.vim | 20 +++++++++----------- doc/NERD_tree.txt | 11 ++++++----- lib/nerdtree/creator.vim | 2 +- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/autoload/nerdtree/ui_glue.vim b/autoload/nerdtree/ui_glue.vim index 2aa3bec..2478fd3 100644 --- a/autoload/nerdtree/ui_glue.vim +++ b/autoload/nerdtree/ui_glue.vim @@ -441,21 +441,19 @@ function! s:jumpToSibling(currentNode, forward) endfunction " FUNCTION: nerdtree#ui_glue#openBookmark(name) {{{1 -" put the cursor on the given bookmark and, if its a file, open it +" Open the Bookmark that has the specified name. This function provides the +" implementation for the ":OpenBookmark" command. function! nerdtree#ui_glue#openBookmark(name) try - let targetNode = g:NERDTreeBookmark.GetNodeForName(a:name, 0, b:NERDTree) - call targetNode.putCursorHere(0, 1) - redraw! - catch /^NERDTree.BookmarkedNodeNotFoundError/ - call nerdtree#echo("note - target node is not cached") - let bookmark = g:NERDTreeBookmark.BookmarkFor(a:name) - let targetNode = g:NERDTreeFileNode.New(bookmark.path, b:NERDTree) + let l:bookmark = g:NERDTreeBookmark.BookmarkFor(a:name) + catch /^NERDTree.BookmarkNotFoundError/ + call nerdtree#echoError('bookmark "' . a:name . '" not found') + return endtry - if targetNode.path.isDirectory - call targetNode.openExplorer() + if l:bookmark.path.isDirectory + call l:bookmark.open(b:NERDTree) else - call targetNode.open({'where': 'p'}) + call l:bookmark.open(b:NERDTree, {'where': 'p'}) endif endfunction diff --git a/doc/NERD_tree.txt b/doc/NERD_tree.txt index d50cfb2..e976615 100644 --- a/doc/NERD_tree.txt +++ b/doc/NERD_tree.txt @@ -158,7 +158,7 @@ click bookmarks or use the |NERDTree-o| mapping to activate them. See also, ------------------------------------------------------------------------------ 2.2.2. Bookmark commands *NERDTreeBookmarkCommands* -Note that the following commands are only available in the NERD tree buffer. +Note: The following commands are only available within the NERDTree buffer. :Bookmark [] Bookmark the current node as . If there is already a @@ -178,10 +178,11 @@ Note that the following commands are only available in the NERD tree buffer. (i.e. directory nodes above it will be opened) and the cursor will be placed on it. -:OpenBookmark - must point to a file. The file is opened as though |NERDTree-o| - was applied. If the node is cached under the current root then it will be - revealed and the cursor will be placed on it. +:OpenBookmark + The Bookmark named is opened as if |NERDTree-o| was applied to + its entry in the Bookmark table. If the Bookmark points to a directory, + it is made the new root of the current NERDTree. If the Bookmark points + to a file, that file is opened for editing in another window. :ClearBookmarks [] Remove all the given bookmarks. If no bookmarks are given then remove all diff --git a/lib/nerdtree/creator.vim b/lib/nerdtree/creator.vim index 952811c..92e1abe 100644 --- a/lib/nerdtree/creator.vim +++ b/lib/nerdtree/creator.vim @@ -14,7 +14,7 @@ function! s:Creator._bindMappings() command! -buffer -nargs=? Bookmark :call nerdtree#ui_glue#bookmarkNode('') command! -buffer -complete=customlist,nerdtree#completeBookmarks -nargs=1 RevealBookmark :call nerdtree#ui_glue#revealBookmark('') - command! -buffer -complete=customlist,nerdtree#completeBookmarks -nargs=1 OpenBookmark :call nerdtree#ui_glue#openBookmark('') + command! -buffer -complete=customlist,nerdtree#completeBookmarks -nargs=1 OpenBookmark call nerdtree#ui_glue#openBookmark('') command! -buffer -complete=customlist,nerdtree#completeBookmarks -nargs=* ClearBookmarks call nerdtree#ui_glue#clearBookmarks('') command! -buffer -complete=customlist,nerdtree#completeBookmarks -nargs=+ BookmarkToRoot call g:NERDTreeBookmark.ToRoot('', b:NERDTree) command! -buffer -nargs=0 ClearAllBookmarks call g:NERDTreeBookmark.ClearAll() call b:NERDTree.render()