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.

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

1836 view debounce delay #1871

Merged
merged 10 commits into from
Jan 1, 2023
Merged

1836 view debounce delay #1871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4648ed4
fix(#1836):add view.debounce_delay to avoid some unnecessary explorer…
D0ot Dec 24, 2022
2b450d6
Merge remote-tracking branch 'D0ot/master' into 1836-view-debounce-delay
chomosuke Dec 31, 2022
6de1163
fixed BufReadPost & BufUnload nil pointer
chomosuke Dec 31, 2022
88c7c25
update_focused_file.debouce_delay to view.debounce_delay
chomosuke Jan 1, 2023
f5a0378
changed docs to be more accurate
chomosuke Jan 1, 2023
521c624
added debounce on modified update
chomosuke Jan 1, 2023
f452c38
Using same event for filter buffer
chomosuke Jan 1, 2023
aec57a9
removed unused View.debounce_delay
chomosuke Jan 1, 2023
7c88f30
Merge remote-tracking branch 'origin/master' into 1836-view-debounce-…
chomosuke Jan 1, 2023
9bd9e42
changed docs to be more accurate
alex-courtis Jan 1, 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
12 changes: 6 additions & 6 deletions doc/nvim-tree-lua.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ Subsequent calls to setup will replace the previous configuration.
adaptive_size = false,
centralize_selection = false,
cursorline = true,
debounce_delay = 15,
width = 30,
hide_root_folder = false,
side = "left",
Expand Down Expand Up @@ -286,7 +287,6 @@ Subsequent calls to setup will replace the previous configuration.
},
update_focused_file = {
enable = false,
debounce_delay = 15,
update_root = false,
ignore_list = {},
},
Expand Down Expand Up @@ -520,11 +520,6 @@ until it finds the file.
Enable this feature.
Type: `boolean`, Default: `false`

*nvim-tree.update_focused_file.debounce_delay*
Idle milliseconds between BufEnter and update.
The last BufEnter will be focused, others are discarded.
Type: `number`, Default: `15` (ms)

*nvim-tree.update_focused_file.update_root* (previously `update_focused_file.update_cwd`)
Update the root directory of the tree if the file is not under current
root directory. It prefers vim's cwd and `root_dirs`.
Expand Down Expand Up @@ -716,6 +711,11 @@ Window / buffer setup.
Enable |cursorline| in the tree window.
Type: `boolean`, Default: `true`

*nvim-tree.view.debounce_delay*
Idle milliseconds before some reload / refresh operations.
Increase if you experience performance issues around screen refresh.
Type: `number`, Default: `15` (ms)

*nvim-tree.view.hide_root_folder*
Hide the path of the current working directory on top of the tree.
Type: `boolean`, Default: `false`
Expand Down
18 changes: 12 additions & 6 deletions lua/nvim-tree.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,9 @@ local function setup_autocommands(opts)
(filters.config.filter_no_buffer or renderer.config.highlight_opened_files ~= "none")
and vim.bo[data.buf].buftype == ""
then
reloaders.reload_explorer()
utils.debounce("Buf:filter_buffer", opts.view.debounce_delay, function()
reloaders.reload_explorer()
end)
end
end,
})
Expand All @@ -381,7 +383,9 @@ local function setup_autocommands(opts)
(filters.config.filter_no_buffer or renderer.config.highlight_opened_files ~= "none")
and vim.bo[data.buf].buftype == ""
then
reloaders.reload_explorer(nil, data.buf)
utils.debounce("Buf:filter_buffer", opts.view.debounce_delay, function()
Copy link
Member

Choose a reason for hiding this comment

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

Was the bug caused by having separate categories?

Copy link
Collaborator Author

@chomosuke chomosuke Jan 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's simply because view.debounce_delay was used instead of opts.view.debounce_delay, which resulted in nil exception somewhere else.

Copy link
Member

Choose a reason for hiding this comment

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

Ah! Good catch

reloaders.reload_explorer(nil, data.buf)
end)
end
end,
})
Expand Down Expand Up @@ -416,7 +420,7 @@ local function setup_autocommands(opts)
if opts.update_focused_file.enable then
create_nvim_tree_autocmd("BufEnter", {
callback = function()
utils.debounce("BufEnter:find_file", opts.update_focused_file.debounce_delay, function()
utils.debounce("BufEnter:find_file", opts.view.debounce_delay, function()
find_file(false)
end)
end,
Expand Down Expand Up @@ -481,8 +485,10 @@ local function setup_autocommands(opts)
if opts.modified.enable then
create_nvim_tree_autocmd({ "BufModifiedSet", "BufWritePost" }, {
callback = function()
modified.reload()
reloaders.reload_explorer()
utils.debounce("Buf:modified", opts.view.debounce_delay, function()
Copy link
Member

Choose a reason for hiding this comment

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

This is looking good.

I'd eventually like to have all reload_explorer calls debounced (from within) however that will require some refactoring first. This gradual approach will work well.

Idea: could we use one category for this and "Buf:filter_buffer"? We would need to move modified.reload into reload_explorer, which should be OK as it is not expensive. The category could just be "Buf".

"BufEnter:find_file" will need to remain separate for now due to its extra complexity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm, that sounds sensible, though I feel like if we do this too much then reload_explorer will become slower and slower as we add more feature to it.

I actually have another refactoring idea after I finished modified, it goes like this:

We split nvim-tree into three distinct part: core, external modules & line builder.

Git, diagnostics, modified & opened file becomes external modules. They interact with core to attach attributes to paths (i.e. directories & files). Core can propagate attributes to parents / children at the external module's request.

Core calls line builder with all the path's attributes + other info like name, depth. Line builder returns everything the core needs to display including padding, signcolumn, highlights etc.

This would allow us to do a few things:

  • Only calling the line builder if any attribute for that specific node has changed.
  • Let user add their own external module / override the line builder.

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

We split nvim-tree into three distinct part: core, external modules & line builder.

I like this approach.

Files and git status are somewhat detached in that they don't trigger a full refresh, only a draw for the affected directory.

Everything else does a full refresh, which is unnecessary and expensive. Examples: filter, most nvim events, fs operations.

They interact with core to attach attributes to paths (i.e. directories & files). Core can propagate attributes to parents / children at the external module's request.

That would work very nicely: core can decide upon the least expensive action and propagate that.

It would be fantastic if you could start on this one: we could "proof of concept" this by having some events call reload.refresh_nodes_for_path or just reload.refresh_node.

This can be done iteratively ande we can split up the functionality as we go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just would like to add that we should not be afraid of breaking changes if benefits out weight inconveniences of one off config changes for users.

Project longevity and maintainability is more important.

Copy link
Member

Choose a reason for hiding this comment

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

if benefits out weight inconveniences of one off config changes for users.

Absolutely. We have successfully made a few such small changes with no reported issues.

Copy link
Member

Choose a reason for hiding this comment

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

  api.tree.toggle_no_buffer_filter()
  api.tree.expand_all()

doesn't work as you'd expect, as the filter is debounced and completes after expansion.

Having the toggle draw synchronously with no refresh would resolve that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be fantastic if you could start on this one: we could "proof of concept" this by having some events call reload.refresh_nodes_for_path or just reload.refresh_node.

This can be done iteratively ande we can split up the functionality as we go.

Sounds great, I'll think about opening a tracking issue and start with highilght_opened_files in a few days.

Do we have any plan for versioning nvim-tree? I think it'd be nice if we put those refactoring onto release candidate for nvim-tree 2.0.

Copy link
Member

Choose a reason for hiding this comment

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

Big 👍

Versioning is a work in progress. We don't have any immediate plans to break anything large. Also... this change would be non-breaking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tracked in #1881

modified.reload()
reloaders.reload_explorer()
end)
end,
})
end
Expand Down Expand Up @@ -510,6 +516,7 @@ local DEFAULT_OPTS = { -- BEGIN_DEFAULT_OPTS
adaptive_size = false,
centralize_selection = false,
cursorline = true,
debounce_delay = 15,
width = 30,
hide_root_folder = false,
side = "left",
Expand Down Expand Up @@ -604,7 +611,6 @@ local DEFAULT_OPTS = { -- BEGIN_DEFAULT_OPTS
},
update_focused_file = {
enable = false,
debounce_delay = 15,
update_root = false,
ignore_list = {},
},
Expand Down
3 changes: 3 additions & 0 deletions lua/nvim-tree/legacy.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ local function refactored(opts)

-- 2022/11/22
utils.move_missing_val(opts, "renderer", "root_folder_modifier", opts, "renderer", "root_folder_label", true)

-- 2023/01/01
utils.move_missing_val(opts, "update_focused_file", "debounce_delay", opts, "view", "debounce_delay", true)
end

local function removed(opts)
Expand Down