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

Force sort to recalculate the cached sortKey. #898

Merged
merged 2 commits into from
Oct 18, 2018

Conversation

PhilRunninger
Copy link
Member

Fixes #880.

The problem in issue #880 was caused by the sort using the old sortKey.
For example, given nodes A, B, and C, if B were renamed to D, the sort
was still using B as its sortKey, thus not moving it.

It's a bit of a hack, but if we set g:NERDTreeOldSortOrder to an empty
list, the cached sortKey will be recalculated. I did the same thing for
both the Copy and Add functions as well.

The problem in issue #880 was caused by the sort using the old sortKey.
For example, given nodes A, B, and C, if B were renamed to D, the sort
was still using B as its sortKey, thus not moving it.

It's a bit of a hack, but if we set g:NERDTreeOldSortOrder to an empty
list, the cached sortKey will be recalculated. I did the same thing for
both the Copy and Add functions as well.
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.

Hey, @PhilRunninger!

I just confirmed that this fixes the reported problem. Two ideas:

  1. Can we possibly clear the old list closer to where it's used so that this only happens in one place (following the DRY principle)?
  2. If not, this may be enough of a hack to deserve a comment explaining why you're clearing the list. It's in the commit history (and your commit message is perfect), but I'd still like some way to express in the code what's going on.

Other than that, I'm just fine with this change. This also taught me that the NERDTree can sort naturally, which I did not know. Good stuff!

@PhilRunninger
Copy link
Member Author

The most logical place to DRY this code up is to put let g:NERDTreeOldSortOrder = [] in the b:NERDTree.root.refresh() function before sorting, but we would run into the same performance issue you noticed in #856. Question is: How often will refresh be called in the real world?

@lifecrisis
Copy link
Contributor

Question is: How often will refresh be called in the real world?

I wonder this myself. I use it all the time... but it's mainly when I'm testing NERDTree code!

In light of your observation, I say merge it. The fs_menu.vim code needs some refactoring later, anyway. A comment may help clarify things, but I'm otherwise okay with everything you've got here.

@PhilRunninger PhilRunninger merged commit f98078d into master Oct 18, 2018
@PhilRunninger PhilRunninger deleted the sort_after_rename_880 branch October 18, 2018 20:13
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