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 keeps 'No Name' buffer after deleting current node 'md' #755 (closes #755) #756

Merged
merged 4 commits into from
Nov 2, 2017

Conversation

mboughaba
Copy link
Contributor

Go to the new buffer in buffer list instead of creating a new buffer.
If only one buffer is open, now we go back to nerdtree buffer.

…ervim#755

Go to the new buffer in buffer list instead of creating a new buffer.
If only one buffer is open, now we go back to nerdtree buffer.
@lifecrisis
Copy link
Contributor

I'm leaning toward accepting this change. It avoids creating a new buffer, which I like.

@PhilRunninger, do you approve?

@PhilRunninger
Copy link
Member

This changes the current behavior when only one non-NERDTree buffer is open. It causes NERDTree to be the only window after md. This leads to window sizing issues that have caused a lot of confusion for other users. Maybe it would be better to leave the old behavior if deleting the last listed buffer. But I do like this change in all other situations.

@mboughaba
Copy link
Contributor Author

This changes the current behavior when only one non-NERDTree buffer is open. It causes NERDTree to be the only window after md. This leads to window sizing issues that have caused a lot of confusion for other users. Maybe it would be better to leave the old behavior if deleting the last listed buffer. But I do like this change in all other situations.

Thanks guys! I agree.
I tried at least to put that in small words in my commit

If only one buffer is open, now we go back to nerdtree buffer.

I'll work on that

…ervim#755 (closes preservim#755) preservim#756

Go to the next buffer in buffer list if at least one extra buffer is listed. Otherwise open a new empty buffer.
@@ -55,7 +55,14 @@ function! s:promptToDelBuffer(bufnum, msg)
" Is not it better to close single tabs with this file only ?
let s:originalTabNumber = tabpagenr()
let s:originalWindowNumber = winnr()
exec "tabdo windo if winbufnr(0) == " . a:bufnum . " | exec ':enew! ' | endif"
let s:listedBufferCount = len(getbufinfo({'buflisted':1}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that getbufinfo() is not supported in Vim 7.4.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't approve the change until this is fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the find. I wasn't aware that getbufinfo is not supported in 7.4. Will work on that.

Copy link
Contributor Author

@mboughaba mboughaba Nov 1, 2017

Choose a reason for hiding this comment

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

Below works on vim 7.2 in my linux machine

:echo len(filter(range(1, bufnr('$')), 'buflisted(v:val)'))

Although the syntax above works as well on 8.0, wouldn't it be better to keep the newer way for the latest version?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with it either way - old syntax or both.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mboughaba, I'll eventually push for a fork that will break with Vim 7 and only support Vim 8. No point in doing it right now though, adoption is not wide enough to to warrant the move.

Believe me, if we break compatibility with 7 at this time, complaints will flow!

…ervim#755 (closes preservim#755) preservim#756

Check vim version before getting listed buffer count.
Old behavior is kept for v 7.0 and 7.1.
starting from 7.2 new behavior will be applicable. Switch to open buffer if any instead of opening a new one.
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.

Why are you using a script-local variable (i.e., s:listedBufferCount) here? Shouldn't you use a function local variable (prefix with l:)?

Please don't clutter the script namespace!

NERDTree keeps 'No Name' buffer after deleting current node 'md' preservim#755 (closes preservim#755) preservim#756
@lifecrisis lifecrisis merged commit a8c6245 into preservim:master Nov 2, 2017
alexanderscott added a commit to alexanderscott/nerdtree that referenced this pull request Nov 21, 2017
* 'master' of https://github.com/scrooloose/nerdtree: (181 commits)
  Make the "o" mapping consistent with "x"
  Fix a problem with the "x" handler (preservim#768)
  Clean up the handler for the "x" mapping (preservim#767)
  Revert change to tab opening method (preservim#766)
  Add back support for "b:NERDTreeRoot" (preservim#765)
  Update the CHANGELOG
  Have new tabs open as the last tab (with '$')
  Add a note/warning to "TreeDirNode.activate()"
  Document "t" and "T" mappings in the quick help
  Silence messages when opening a file with "T"
  Fix handlers for "t" and "T" on bookmarks
  Explicitly call open() in "ui_glue.vim" callbacks
  Remove an unnecessary assignment
  Clean up the Creator.createTabTree() function
  Clean up the commentary in "creator.vim"
  Extract a common line to the top of a function
  Remove an unnecessary "else" clause
  Clean up the NERDTreeOpener constructor
  Merge pull request preservim#754 from branch comma-separated-file-size
  Merge pull request preservim#756 from mboughaba/master
  ...
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.

3 participants