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

NERDTree: Refreshing the root node. This could take a while... #684

Closed
gpakosz opened this issue Apr 20, 2017 · 22 comments · Fixed by #1026
Closed

NERDTree: Refreshing the root node. This could take a while... #684

gpakosz opened this issue Apr 20, 2017 · 22 comments · Fixed by #1026

Comments

@gpakosz
Copy link

gpakosz commented Apr 20, 2017

Environment:

  • Operating System: Mac OS 10.11.6
  • Vim version :version: VIM - Vi IMproved 8.0 (2016 Sep 12, compiled Apr 11 2017 10:04:07)
  • NERDTree version git rev-parse --short HEAD: 45f4d61
  • NERDTree settings applied in your vimrc, if any:
" -- NERDTree settings ---------------------------------------------------------

if has("autocmd")
	" open a NERDTree when vim starts up with no files specified
  autocmd StdinReadPre * let s:std_in=1
  autocmd VimEnter * if argc() == 0 && !exists("s:std_in") | NERDTree | endif

  " close vim if the only window left open is a NERDTree
  autocmd BufEnter * if (winnr("$") == 1 && exists("b:NERDTreeType") && b:NERDTreeType == "primary") | q | endif

  autocmd WinEnter * if exists('b:NERDTree') | execute 'normal R' | endif
endif

" toggle NERDTree with <C-N>
nnoremap <silent> <C-N> :NERDTreeToggle<CR>
" find current buffer in NERDTree with <C-F>
nnoremap <silent> <C-F> :NERDTreeFind<CR>

let NERDTreeBookmarksFile=$HOME."/.vim/.NERDTreeBookmarks"
let NERDTreeQuitOnOpen=1
let NERDTreeShowBookmarks=1
let NERDTreeShowHidden=1

set noequalalways " prevents windows size equalization after NERDTree closed

Process:

  1. $ vim path/to/file
  2. hit Ctrl+f
  3. navigate to another file with Up/Down
  4. hit Return

Current Result:

NERDTree displays:

NERDTree: Refreshing the root node. This could take a while...

and DONE is somewhat blinking before it's really done.

Expected Result:

Previously, NERDTree would just open the file without trying to refresh its tree.


Optional

Regression has been introduced by commit 35b04fa3dfdef5cbfbcd6ada59fce3a7bc31eeaa:

commit 35b04fa3dfdef5cbfbcd6ada59fce3a7bc31eeaa
Author: Greg Hurrell <[email protected]>
Date:   Thu May 5 09:32:19 2016 -0700

    Suppress autocmds less aggressively

    This is the counterpart to a PR I just submitted to undotree
    (https://github.com/mbbill/undotree/pull/61).

    I noticed that my statusline doesn't update properly when using NERDTree to move
    between revisions of a file with `go` or `gi`
    (https://github.com/wincent/wincent/issues/16). I established that this was
    because it was using `'eventignore'` to suppress all autocmds, which in turn
    prevents the statusline from updating.

    Commenting out the `set eventignore=all` line makes the failure to update go
    away, at the cost of firing more autocmds.

    I considered adding an option for opting out of this behavior (eg. `let
    g:NERDTreeEventignore=0` or something), or rearchitecting my statusline to use
    an approach like vim-airline does based on CursorMoved autocmds (see
    https://github.com/vim-airline/vim-airline/issues/82; see also
    https://github.com/vim-airline/vim-airline/blob/30f078daf569e7d5e4f7829e39316387af349b41/plugin/airline.vim#L36-L50
    for current implementation), but then realized that a simpler fix is to have
    NERDTree just disable only the autocmds that it uses instead of disabling all of
    them.

    This is probably not enough to unbreak every bit of code in the world that
    depends on those autocmds, but it does at least unbreak my use case, because it
    allows my `WinLeave` autocmd to run and update the statusline.
@gpakosz
Copy link
Author

gpakosz commented May 16, 2017

Any idea please?

@PhilRunninger
Copy link
Member

@gpakosz, your WinEnter autocmd is causing this behavior because of the PR you identified. If you remove that line, you will see the responsiveness improve, but of course NERDTree won't Refresh every time you enter the NERDTree window.

@wincent, what do you think about adding WinEnter into the list of ignored events? Would that cause any interference that your PR intended to eliminate?

@scrooloose , any thoughts?

@wincent
Copy link
Contributor

wincent commented May 16, 2017

@PhilRunninger. I don't think adding WinEnter is a good idea. As explained in 35b04fa, it's not really NERDTree's business to suppress autocommands that it itself didn't set up. An argument can be made for either being a "good citizen" and only suppressing its own autocommand events (ie. the approach taken in that commit), or for taking an opinionated stance and arguing that NERDTree should suppress all events (although, I personally think that's overly aggressive). It is much harder to make an argument along the lines of "my local set-up wants WinEnter events suppressed, and as long as it doesn't mess with some other person's set-up, then that should be ok"; it's totally arbitrary.

Rather than add WinEnter, it would be better to make the list configurable via an option, so that people can bend it into whatever crazy shape they want.

@PhilRunninger
Copy link
Member

I agree you, @wincent. Thanks for giving your opinion.

@gpakosz
Copy link
Author

gpakosz commented May 16, 2017

Thanks for the insight. The issue I'm facing is partly due to my Vim ignorance.

Is there a way to replace

autocmd WinEnter * if exists('b:NERDTree') | execute 'normal R' | endif

to something that would trigger a refresh when moving from any window to the NERDTree window?

As far as I understand, what I'm facing is NERDTree refreshing itself when moving away from the NERDTree window.

@wincent
Copy link
Contributor

wincent commented May 16, 2017

You can try to figure out what autocommands are firing with something like:

:set verbose=15
:set verbosefile=vlog

Then do stuff, then look in the vlog log file.

You shouldn't get a WinEnter on leaving NERDTree, but maybe the log sheds some light on why.

There might be an alternate implementation you can do that relies on the following:

  1. On WinLeave, set a global variable g:whatever if the window is not a NERDTree window.
  2. On WinEnter, check the global variable and if you are entering a NERDTree window:
    • Unset the global.
    • Refresh the view.

Or something like that.

@PhilRunninger
Copy link
Member

In a simple investigation, I used

autocmd WinEnter * echomsg localtime() . ':  ' . bufname(winbufnr(0))

to track which windows are being entered. It turns out NERDTree is switching windows a couple of times when you select a file to open, as seen below. I had vimrc open, toggled NERDTree, then used it to open README.md, and this is what it echoed.

"vimrc" 47L, 1743C
1494970121:  vimrc
1494970121:  NERD_tree_1
1494970121:  vimrc
1494970121:  NERD_tree_1
1494970121:  vimrc
1494970121:  NERD_tree_1
1494970121:  vimrc
"README.md" 141L, 7463C

I don't know what's going on in the code, or why, but with your autocmd, it is triggering a Refresh of the root node three times each time you open a file to edit.

@gpakosz
Copy link
Author

gpakosz commented May 16, 2017

I take the blame for putting an autocmd in the first place.

Yet that ping-pong even before opening the new file is dubious 🤔

@gpakosz
Copy link
Author

gpakosz commented May 20, 2017

I can confirm that when disabling all my autocmds then adding

autocmd WinEnter * echomsg localtime() . ':  ' . bufname(winbufnr(0))

I'm observing the same behavior as you:

Messages maintainer: Bram Moolenaar <[email protected]>
1495297000:
1495297000:  NERD_tree_1
1495297000:
1495297000:  NERD_tree_1
1495297000:
1495297000:  NERD_tree_1
1495297000:
"urls.txt" 4L, 96C

NERDTree switches windows 3 times. I'm still unsure how to debug this being really unfamiliar with vimscript

gpakosz added a commit to gpakosz/nerdtree that referenced this issue May 20, 2017
NERDTree relies on executing 'wincmd p' back and forth as part of its
normal operation.

This implementation detail is not obvious to NERDTree users that may
set up autocommands.

As such, it makes sense to ignore WinEnter and WinLeave autocmnds in
nerdtree#exec(cmd).

This PR fixes preservim#684.
@gpakosz
Copy link
Author

gpakosz commented May 20, 2017

Alright, a few echomsg messages later I circled it down.

When activating a file node, we end up in s:Opener._gotoTargetWin() which itself calls Opener._previousWindow().

Opener._previousWindow() calls Opener._isWindowUsable(winnumber) two times and ultimately, Opener._isWindowUsable(winnumber) goes back to the previous window to figure out whether it's a special window:

    let oldwinnr = winnr()
    call nerdtree#exec(a:winnumber . "wincmd p")
    let specialWindow = getbufvar("%", '&buftype') != '' || getwinvar('%', '&previewwindow')
    let modified = &modified
    call nerdtree#exec(oldwinnr . "wincmd p")

This explains the first two ping-pongs. I can mitigate them by rewriting Opener._previousWindow() and introduce a usable variable:

function! s:Opener._previousWindow()
    let usable = self._isWindowUsable(winnr("#"))
    if !usable && self._firstUsableWindow() ==# -1
        call self._newSplit()
    else
        try
            if !usable
                call nerdtree#exec(self._firstUsableWindow() . "wincmd w")
            else
                call nerdtree#exec('wincmd p')
            endif
        catch /^Vim\%((\a\+)\)\=:E37/
            call g:NERDTree.CursorToTreeWin()
            throw "NERDTree.FileAlreadyOpenAndModifiedError: ". self._path.str() ." is already open and modified."
        catch /^Vim\%((\a\+)\)\=:/
            echo v:exception
        endtry
    endif
endfunction

The last ping pong is caused by call self._checkToCloseTree(0) at the end of Opener._gotoTargetWin(). This ends up calling call g:NERDTree.CloseIfQuitOnOpen() which itself calls NERDTree.Close()

function! s:NERDTree.Close()
    if !s:NERDTree.IsOpen()
        return
    endif

    if winnr("$") != 1
        if winnr() == s:NERDTree.GetWinNum()
            call nerdtree#exec("wincmd p")
            let bufnr = bufnr("")
            call nerdtree#exec("wincmd p")
        else
            let bufnr = bufnr("")
        endif

        call nerdtree#exec(s:NERDTree.GetWinNum() . " wincmd w")
        close
        call nerdtree#exec(bufwinnr(bufnr) . " wincmd w")
    else
        close
    endif
endfunction

So yeah, NERDTree depends a lot on being able to ping-pong with wincmd p. In that respect, I slightly disagree with @wincent's comment: it's not obvious to users that NERDTree's implementation relies on ping-ponging between windows. I suggest both WinEnter and WinLeave should be added back to eventignore.

@wincent
Copy link
Contributor

wincent commented May 21, 2017

The problem with ignoring those autocommands, @gpakosz, is that it would break the set-up of anybody who relies on them, with no way of avoiding the breakage short of forking NERDTree or never upgrading it.

On the other hand, not ignoring them leads to two possible outcomes, both of which are better than outright breakage: either the user does nothing and sees a slowdown (but only if their autocommands are expensive), or they implement a workaround using some global variables like I suggested above. The point is, they have options.

Another question is whether NERDTree should be doing this pingpong thing in the first place. It does sound a bit kludgish, so perhaps the answer is "ideally not".

One final observation: if we can't reach an agreement on which autocommands should be ignored, that itself may be evidence of the fact that it should be configurable (an unpleasantly low level configuration, for sure) or perhaps you are solving the wrong problem in the first place: if you want to run something on entering NERDTree, and if NERDTree is going to break Vim's built-in mechanism for responding to such an event (ie. WinEnter) then perhaps it needs to fire custom autocommand of its own that you could hook into. Perhaps it already does? (I haven't searched in the docs.)

@PhilRunninger
Copy link
Member

I can't seem to find it now, but I remember seeing another issue very similar to this, where the OP solved the unresponsiveness by using BufEnter instead of WinEnter. Give that a try, and see if things speed up for you.

@gpakosz
Copy link
Author

gpakosz commented May 22, 2017

@wincent I will try to debug more and educate myself more because at that point I still fail to understand how as a user one would rely on NERDTree pingponging between windows as an implementation detail 😐

It seems to me that all but the last WinEnter and WinLeave events should be filtered. In that respect maybe an additional a:eventignore parameter should be given to nerdtree#exec(). What do you think?

@PhilRunninger
Copy link
Member

@gpakosz Have you tried using BufEnter instead of WinEnter, as I last suggested? I believe that is more in line with what you want the refresh to do, and it doesn't get trapped in the window ping-ponging that NERDTree is doing.

@gpakosz
Copy link
Author

gpakosz commented Jun 13, 2017

Toggling NERDTree doesn't fire a BufEnter event. How can that be the solution to this issue?

@PhilRunninger
Copy link
Member

BufEnter will fire when you switch from another window into a NERDTree window that's already open. I thought that was the situation you were trying to handle with WinEnter. To refresh NERDTree when toggling it, why not change your <C-n> mapping like so?

nnoremap <silent> <C-N> :NERDTreeToggle<CR>:execute 'normal R'<CR>

or

nnoremap <silent><expr> <C-N> (winnr()==g:NERDTree.GetWinNum() ? ":NERDTreeClose\<CR>" : ":NERDTreeFocus\<CR>:execute 'normal R'\<CR>")

@lifecrisis
Copy link
Contributor

lifecrisis commented Jun 13, 2017

@PhilRunninger, this is exactly what I was going to suggest... that he stack the command to refresh on top of the :NERDTreeToggle command.

You might make one adjustment:

function! s:NERDTreeToggleWithRefresh()
  if exists('b:NERDTree')
    NERDTreeClose
  else
    NERDTreeFocus
    exe 'silent normal R'
  endif
endfunction
nmap <silent> <C-N> :call <SID>NERDTreeToggleWithRefresh()<CR>

This doesn't exactly toggle the NERDTree. You'd need some kind of method on the NERDTree object to determine if a tree was open or not (this is doable if it's not done already...).

This is basically the same as above, but it's a little cleaner because it's a script-local function for the vimrc file, and the refreshing of the NERDTree is silent (i.e. no message is printed).

EDIT: Just realized the :NERDTreeFocus command is not documented. Raising an issue! See issue #706.

@gpakosz
Copy link
Author

gpakosz commented Jun 20, 2017

@PhilRunninger With WinEnter it was handling both situations

This doesn't exactly toggle the NERDTree. You'd need some kind of method on the NERDTree object to determine if a tree was open or not (this is doable if it's not done already...).

@lifecrisis I don't understand the "This doesn't exactly toggle the NERDTree" part and why it matters. As for the "You'd need some kind of method on the NERDTree object to determine if a tree was open or not" I guess you're after g:NERDTree.IsOpen() ?

So far, I went for

function! s:NERDTreeToggleAndRefresh()
  if g:NERDTree.IsOpen()
    NERDTreeClose
  else
    NERDTreeFocus
    exe 'silent normal R'
  endif
endfunction

function! s:NERDTreeFindAndRefresh()
  if !g:NERDTree.IsOpen()
    NERDTreeFind
    exe 'silent normal R'
  endif
endfunction

" toggle NERDTree with <C-N>
nnoremap <silent> <C-N> :call <SID>NERDTreeToggleAndRefresh()<CR>
" find current buffer in NERDTree with <C-F>
nnoremap <silent> <C-F> :call <SID>NERDTreeFindAndRefresh()<CR>

autocmd BufEnter * if g:NERDTree.IsOpen() | execute 'silent normal R' | endif

What do you recommend between winnr()==g:NERDTree.GetWinNum(), exists('b:NERDTree') and g:NERDTree.IsOpen() ?

@gpakosz
Copy link
Author

gpakosz commented Jun 20, 2017

Note that I still believe NERDTree should filter some WinEnter and WinLeave while doing its ping-ponging in order to fire 1 WinEnter instead of 3. It's non obvious to users these events will fire so many times when focusing/unfocusing NERDTree.

@wincent
Copy link
Contributor

wincent commented Jun 20, 2017

Better would be to not ping-pong in the first place, but I don't know how easy/tractable making such a change would be.

@lifecrisis
Copy link
Contributor

Looks like progress on this has stalled. Closing.

If anything can be done that will satisfy everyone in the future, we'll require a separate issue with a clearly defined problem and solution.

@PhilRunninger
Copy link
Member

Coming momentarily, a PR to address the ping-ponging mentioned above.

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 a pull request may close this issue.

4 participants