Skip to content

fix: infinite recursion in empty folder with hide_root_node = true #729

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Feb 5, 2023
Merged

fix: infinite recursion in empty folder with hide_root_node = true #729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4bc0609
fix: infinite recursion in empty folder with `hide_root_node = true`
figsoda Feb 4, 2023
9a82dff
refactor: unrecurse `get_folder_node` and avoid infinite loop
figsoda Feb 4, 2023
2182e72
refactor: let get_folder_node loop infinitely and abort when no paren…
figsoda Feb 4, 2023
f5ed1fd
chore: remove outdated comment
figsoda Feb 4, 2023
903d9b1
refactor: make get_folder_node return when node is nil
figsoda Feb 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 30 additions & 23 deletions lua/neo-tree/sources/common/commands.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,34 +11,41 @@ local log = require("neo-tree.log")
local help = require("neo-tree.sources.common.help")
local Preview = require("neo-tree.sources.common.preview")

---Gets the node parent folder recursively
---@param tree table to look for nodes
---@param node table to look for folder parent
---@return table table
local function get_folder_node(state, node)
---Gets the node parent folder
---@param state table to look for nodes
---@return table? node
local function get_folder_node(state)
local tree = state.tree
if not node then
node = tree:get_node()
end
local node = tree:get_node()
local last_id = node:get_id()

while node do
local insert_as_local = state.config.insert_as
local insert_as_global = require("neo-tree").config.window.insert_as
local use_parent
if insert_as_local then
use_parent = insert_as_local == "sibling"
else
use_parent = insert_as_global == "sibling"
end

local insert_as_local = state.config.insert_as
local insert_as_global = require("neo-tree").config.window.insert_as
local use_parent
if insert_as_local then
use_parent = insert_as_local == "sibling"
else
use_parent = insert_as_global == "sibling"
end
local is_open_dir = node.type == "directory" and (node:is_expanded() or node.empty_expanded)
if use_parent and not is_open_dir then
return tree:get_node(node:get_parent_id())
end

local is_open_dir = node.type == "directory" and (node:is_expanded() or node.empty_expanded)
if use_parent and not is_open_dir then
return tree:get_node(node:get_parent_id())
end
if node.type == "directory" then
return node
end

if node.type == "directory" then
return node
local parent_id = node:get_parent_id()
if parent_id or parent_id == last_id then
return node
else
last_id = parent_id
node = tree:get_node(parent_id)
end
end
return get_folder_node(state, tree:get_node(node:get_parent_id()))
end

---The using_root_directory is used to decide what part of the filename to show
Expand Down
27 changes: 10 additions & 17 deletions lua/neo-tree/ui/renderer.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ create_nodes = function(source_items, state, level)
extra = item.extra,
is_nested = item.is_nested,
skip_node = item.skip_node,
is_empty_with_hidden_root = item.is_empty_with_hidden_root,
-- TODO: The below properties are not universal and should not be here.
-- Maybe they should be moved to the "extra" field?
is_link = item.is_link,
Expand Down Expand Up @@ -315,7 +316,13 @@ end

local prepare_node = function(item, state)
if item.skip_node then
return nil
if item.is_empty_with_hidden_root then
local line = NuiLine()
line:append("(empty folder)", highlights.MESSAGE)
Copy link
Contributor Author

@figsoda figsoda Feb 4, 2023

Choose a reason for hiding this comment

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

Your solution is completely bypassing the Nui Nodes that the rest of Neo-tree and any third party plugins use

Would setting the type of the node to message here help resolve the issue? I couldn't figure out how to do that, the text just ended up with normal highlighting when I do item.type = "message" and return "(empty folder)"

Copy link
Collaborator

@miversen33 miversen33 Feb 4, 2023

Choose a reason for hiding this comment

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

Ya setting the type to message should work. When you say

return "(empty folder)"

What do you mean?
I'm unsure we want to be overwriting the type that is passed in.

A side note, if we completely remove the changes that are being proposed to the renderer, your solution still works. I don't know that we need to be touching renderer at all (aside from the nil bug I mentioned earlier, though I can submit a PR for that if we want as its outside the scope of this issue).

Copy link
Contributor Author

@figsoda figsoda Feb 4, 2023

Choose a reason for hiding this comment

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

What do you mean?

with this patch, (empty folder) doesn't highlight as a message, probably because item.type = "message" doesn't do what I think it does. If there is some way to change the type to "message inside prepare_node, would it address your concerns about bypassing the Nui Nodes?

--- a/lua/neo-tree/ui/renderer.lua
+++ b/lua/neo-tree/ui/renderer.lua
@@ -317,9 +317,8 @@ end
 local prepare_node = function(item, state)
   if item.skip_node then
     if item.is_empty_with_hidden_root then
-      local line = NuiLine()
-      line:append("(empty folder)", highlights.MESSAGE)
-      return line
+      item.type = "message"
+      return "(empty folder)"
     else
       return nil
     end

if we completely remove the changes that are being proposed to the renderer, your solution still works

On my end, the hang is gone, but the created file ends up in foo_empty_message instead of foo. I am not experiencing the nil bug, if you can't reproduce this I can create a minimal reproducible example.

Copy link
Collaborator

@miversen33 miversen33 Feb 4, 2023

Choose a reason for hiding this comment

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

It seems I got lost while looking at the PR, I was getting the changes in prepare_node and show_nodes mixed in my head lol. In this case, item.type isn't going to be used as I referenced above. Disregard my current complaint about prepare_node changes :)

return line
else
return nil
end
end
-- pre_render is used to calculate the longest node width
-- without actually rendering the node.
Expand Down Expand Up @@ -1074,7 +1081,6 @@ M.show_nodes = function(sourceItems, state, parentId, callback)
--local id = string.format("show_nodes %s:%s [%s]", state.name, state.force_float, state.tabnr)
--utils.debounce(id, function()
events.fire_event(events.BEFORE_RENDER, state)
local is_empty_with_hidden_root = false
state.longest_width_exact = 0
local parent
local level = 0
Expand All @@ -1093,10 +1099,8 @@ M.show_nodes = function(sourceItems, state, parentId, callback)
if config.hide_root_node then
if not parentId then
sourceItems[1].skip_node = true
if sourceItems[1].children and #sourceItems[1].children > 0 then
is_empty_with_hidden_root = false
else
is_empty_with_hidden_root = true
if not (sourceItems[1].children and #sourceItems[1].children > 0) then
sourceItems[1].is_empty_with_hidden_root = true
end
end
if not config.retain_hidden_root_indent then
Expand Down Expand Up @@ -1163,17 +1167,6 @@ M.show_nodes = function(sourceItems, state, parentId, callback)
if sourceItems then
-- normal path
local nodes = create_nodes(sourceItems, state, level)
if is_empty_with_hidden_root then
local nodeData = {
id = state.path .. "_empty_message",
name = "(empty folder)",
type = "message",
level = 0,
is_last_child = true,
}
local node = NuiTree.Node(nodeData, {})
table.insert(nodes, node)
end
draw(nodes, state, parentId)
else
-- this was a force grouping of a lazy loaded folder
Expand Down