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

Add split explorer to NERDTree #1389

Merged
merged 7 commits into from
Dec 23, 2023
Merged

Conversation

msibal6
Copy link
Contributor

@msibal6 msibal6 commented Dec 12, 2023

Description of Changes

This adds a new user defined command NERDTreeExplorer to a split explorer for a directory.

Motivation

I toggle NERDTree often after opening my file, so I tend to already use it more as an explorer than a drawer. I saw #1237 for adding split explorer had a pending action item, so I decided to implement it since it was a simple add when implemented as a command.

I also want to give a quick shout out to @rzvxa for taking the initiative to handling the majority of these most recent PRs since Phil's retirement. It's cool to see a tool live on even when it is very robust and there is not a bunch of changes happening. Thanks to you and everybody for continuing to support this tool and preventing bit rot into it! 😁

@rzvxa
Copy link
Member

rzvxa commented Dec 12, 2023

Hi,

I appreciate your kind words and more than that I appreciate this PR, it was something I thought of doing in the past few weeks, And I'm so happy to see that somebody else stepped up to implement it.
The moment I saw the title in my email I thought of #1237 which is a useful feature since interacting with file trees in a split fashion helps with the amount of eye movement you need to make to keep track of stuff happening.
As you mentioned it is pretty straightforward to implement it as a command instead of a configuration since the underlying functionality is there and we just need to expose it as a command to the users.

Your PR by itself is completely fine in my opinion but I've found a few quirks in the CreateWindowTree function call.
For example, I get weird behavior from it while running in a modified buffer:
WindowsTerminal_YFKmvvj3lY

This function is being used for replacing the current directory buffer which can not be modified so this wasn't an issue until now.
I'm not sure if the author of #1237 has done anything to prevent this issue, But we need to either fix the problem on this PR or block merging this one until we fix the issues in CreateWindowTree via another merge request(s).

On the other hand, we are moving away from the previous versioning and going towards automated changelogs via Conventional Commits and multi-bulletin patches, So there is no need for editing the CHANGELOG and I would be grateful if you revert it. The final merge commit message is going to be used to write the changelogs.

Let me know if you like to work on tracking this and other possible issues with this new use of CreateWindowTree.

PS: In your fashion, I also would like to give a shoutout to @alerque who despite his busy schedule has always spent some time reviewing changes to this project.

(edit: I just want to add to my gif, It could be fixable via checking the current buffer, If it is a file we should create a new buffer instead of replacing the current one)

@msibal6
Copy link
Contributor Author

msibal6 commented Dec 12, 2023

Thank you for the quick response! I see now that #1237 used enew each time before calling CreateWindowTree, so your proposed check is correct. I would like to work on tracking this new usage and any issues for 'CreateWindowTree` if you and @alerque are okay with that. I will work on fixing this behavior within this same merge request. Thank you! 😄

@rzvxa
Copy link
Member

rzvxa commented Dec 12, 2023

In my opinion, since the changes are pretty relevant to the feature itself, It is appropriate to include them in this PR.
Just one thing, Be mindful of other parts of the code that call this function in case you want to change the underlying function itself.

@alerque
Copy link
Member

alerque commented Dec 12, 2023

I don't have much time to review this but a cursory glance looks like things are going in the right direction from what I know. Don't hesitate to run with things @rzvxa. I was especially keeping an eye on things early on to get you rolling and your PRs so there were two pairs of eyes on things, but in this case helping another contributor between you you are two pairs of eyes on this already. I'll still pitch in where I can, but don't wait for me.

@msibal6
Copy link
Contributor Author

msibal6 commented Dec 13, 2023

Hi @alerque, like @rzvxa already mentioned, thank you for your sign off and continued support on this plugin! It is cool to be a small part of the collaboration that can happen on these open source projects.

@rzvxa, I created a new function to avoid having to mess with the underlying function and fixing any changes that might be needed for other dependent functions. The split on a modified buffer addresses the situation you mentioned and we are still able to leverage the 'CreateWindowTree' function. Please let me know if you have any concerns. Thank you!

lib/nerdtree/creator.vim Outdated Show resolved Hide resolved
@rzvxa rzvxa merged commit fefea5d into preservim:master Dec 23, 2023
1 check passed
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