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

Fix issues with sorting of nodes #856

Merged
merged 5 commits into from
Jul 31, 2018
Merged

Fix issues with sorting of nodes #856

merged 5 commits into from
Jul 31, 2018

Conversation

PhilRunninger
Copy link
Member

@PhilRunninger PhilRunninger commented Jul 2, 2018

This PR fixes two issues:

  1. should compare with type instead of its value #842 (See autoload/nerdtree.vim)

The sort algorithm was incorrectly comparing the value in the sort key to the type of the integer 0. Instead it should have been checking the type of the sort key's value to see it is numeric.

  1. NERDTreeSortOrder doesn't seem to be respected after the first element in the list. #277 (See lib/nerdtree/path.vim, lib/nerdtree/tree_dir_node.vim, and plugin/NERD_tree.vim)

This issue reported that NERDTree was respecting only the first item in the sort order. It was probably more accurate to say that changing the NERDTreeSortOrder in the current session, then refreshing NERDTree didn't produce correctly sorted nodes. When doing this, NERDTree was using the cached value for the nodes' sortKey, which could be different than it needed to be for the new SortOrder. To fix this, I changed the code to always calculate the sortKey, instead of relying on a cached value. This adds some processing time, which may or may not be deemed acceptable.

Phil Runninger (mac) added 2 commits July 1, 2018 19:55
The cached _sortkey wasn't being recalculated after changing the
NERDTreeSortOrder, resulting in incorrect sort orders.
Phil Runninger (mac) added 2 commits July 2, 2018 08:55
The call to AddDefaultGroupToSortOrder in NERD_tree.vim is redundant
because it's also done every time sortChildren is called. And since the
check is done only once, there's no need for a function either.
sortChildren now just contains the needed if statement.
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.

Everything looks okay to me, functionality wise. However, the NERDTree is definitely slower with this change. I opened a large directory and benchmarked a root refresh for 100 iterations. With the new sorting code, it took 70.61s, compared to 55.09s without the cached sort key. Would you mind investigating to make this a tad faster? I can share my benchmarking tools if you need them.

Also add a new global variable to track when the g:NERDTreeSortOrder
changes. If it has been changed, or when the cached _sortKey value is
uninitialized, then calculate the sort key. This improves processing
speed over the previous commit, and allows on-the-fly changes to the
sort order, (without required vim to be restarted.)
@PhilRunninger
Copy link
Member Author

@lifecrisis, Take a look at this now. I believe this will be sufficiently fast enough. The sort key will be recalculated only if it's uninitialized or when the g:NERDTreeSortOrder changes. Using your numbers above, your 100 root refreshes should take only 1 * 70.61 / 100 + 99 * 55.09 / 100 or 55.25 seconds.

@lifecrisis
Copy link
Contributor

This works! I'm no longer seeing the big slow-down I noticed before.

I did only a perfunctory review of the changes to functionality, so I'm mainly just trusting you on that front. I don't make use of this feature myself. Thus, I haven't tested it much "in the field."

Unless you have other qualms about this change, feel free to merge!

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.

2 participants