fix: change dir with explorer nodes unchanged #2850
Conversation
There was a problem hiding this comment.
Resource leaks aside, this does pass the initial testing.
As this is a change in default behaviour, this should be an experiment as per #2819
Not going to lie: I'm not comfortable with new_from_parent - there's a lot of magic in there.
Alternative proposal:
- create a temporary graph with open node state
- keep existing explore/node creation unchanged
- apply the graph to the new explorer
| @@ -85,7 +85,7 @@ M.force_dirchange = add_profiling_to(function(foldername, should_open_view) | |||
| if should_change_dir() then | |||
| cd(M.options.global, foldername) | |||
| end | |||
| core.init(foldername) | |||
| core.change_root(foldername) | |||
There was a problem hiding this comment.
Unfortunately this is a resource leak for the nodes and its watchers - core.init will destroy each node and their watchers, via Explorer:destroy
You can monitor this by setting log.types.watcher and looking at ${XDG_STATE_HOME}/nvim/nvim-tree.log
grep -E "(Watcher:new|Watcher:destroy)" should be balanced.
Try tail -n 0 -F ${XDG_STATE_HOME}/nvim/nvim-tree.log | grep -E "(Watcher:new|Watcher:destroy)" to see it live.
There was a problem hiding this comment.
There should be no leaks now, please review.
linepi
force-pushed
the
change_dir_with_nodes_unchanged
branch
2 times, most recently
from
59811a4
to
91ce350
Compare
linepi
force-pushed
the
change_dir_with_nodes_unchanged
branch
from
91ce350
to
0385835
Compare
I have now created an option for this action, and I believe this will not affect the default behavior, and thus will not impact existing users. |
We don't need an option; you've identified and fixed a bug and the new behaviour is correct. We just have to be careful as this is core functionality. |
There was a problem hiding this comment.
This is good. It's a "graft" that puts a branch from the old tree in the new, which is the most efficient method.
We need to cut the branch from the old tree so that it is not destroyed with the old tree.
Suggestion: find and cut off the branch first, then destroy the old tree.
| local newTreeExplorer = explorer.Explorer.new(path) | ||
| if newTreeExplorer == nil then | ||
| return | ||
| end |
There was a problem hiding this comment.
4 lines repeated; can be moved above if
| newTreeExplorer.nodes = child_node.nodes; | ||
| end | ||
| TreeExplorer:destroy() | ||
| TreeExplorer = newTreeExplorer |
There was a problem hiding this comment.
2 lines repeated; can be moved below end
| end | ||
| for _, node in ipairs(newTreeExplorer.nodes) do | ||
| if node.absolute_path == TreeExplorer.absolute_path then | ||
| node.nodes = TreeExplorer.nodes |
There was a problem hiding this comment.
These nodes are being destroyed when TreeExplorer:destroy() is executed.
Test case:
create.sh
#!/bin/sh
mkdir -p a/1/A
mkdir -p a/2/A
mkdir -p a/2/B
mkdir -p b/1/A
mkdir -p b/2/B
touch foo
touch a/1/foo
touch a/1/A/bar
touch a/2/A/baz
touch a/2/B/foo
touch b/1/A/bar
touch b/2/B/bazOpen from directory a and expand all E.
From another terminal or cmd mode touch a/1/A/zero and see it appear.
- to change directory up.
rm a/1/A/zero does not update the tree until manual refresh R.
This is because the watcher has been destroyed.
There was a problem hiding this comment.
Oh, I overlooked that fact. Let me make some changes. Thank you for providing the test case.
Situation
Every time I change the path in the explorer, the expanded folders in a certain directory will be reset. It's annoyning.
Here are some demonstration GIF images.
api:
api.tree.change_root_to_parentFrom path
~/tmp/tto~/tmpapi:
api.tree.change_root_to_nodeEnter folder
~/tmp/tObject
Change dir with explorer nodes unchanged.