Skip to content

feat: async rename and create file #1870

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions doc/nvim-tree-lua.txt
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,12 @@ Subsequent calls to setup will replace the previous configuration.
watcher = false,
},
},
experimental = {
async = {
create_file = false,
rename_file = false,
}
}
} -- END_DEFAULT_OPTS
<

Expand Down Expand Up @@ -1206,6 +1212,20 @@ Configuration for diagnostic logging.
|nvim-tree.filesystem_watchers| processing, verbose.
Type: `boolean`, Default: `false`

*nvim-tree.experimental*
Copy link
Member

Choose a reason for hiding this comment

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

TODO @alex-courtis expand on these and think about how to present them in the readme/wiki etc.

Configuration for experimental features.

*nvim-tree.experimental.async*
Control experimental async behavior.

*nvim-tree.experimental.async.create_file*
Toggle async behavior of create file operation.
Type: `boolean`, Default: `false`

*nvim-tree.experimental.async.rename_file*
Toggle async behavior of rename file operation.
Type: `boolean`, Default: `false`

==============================================================================
5. API *nvim-tree-api*

Expand Down
6 changes: 6 additions & 0 deletions lua/nvim-tree.lua
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,12 @@ local DEFAULT_OPTS = { -- BEGIN_DEFAULT_OPTS
watcher = false,
},
},
experimental = {
async = {
create_file = false,
rename_file = false,
},
},
} -- END_DEFAULT_OPTS

local function merge_options(conf)
Expand Down
140 changes: 90 additions & 50 deletions lua/nvim-tree/actions/fs/create-file.lua
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,28 @@ local core = require "nvim-tree.core"
local notify = require "nvim-tree.notify"

local find_file = require("nvim-tree.actions.finders.find-file").fn
local async = require "nvim-tree.async"

local M = {}

---@async
local function create_and_notify(file)
local ok, fd = pcall(vim.loop.fs_open, file, "w", 420)
if not ok then
local fd, err
if M.enable_async then
err, fd = async.call(vim.loop.fs_open, file, "w", 420)
else
fd, err = vim.loop.fs_open(file, "w", 420)
end
if err then
notify.error("Couldn't create file " .. file)
return
end
vim.loop.fs_close(fd)
events._dispatch_file_created(file)
end

local function create_file(file)
if utils.file_exists(file) then
local prompt_select = "Overwrite " .. file .. " ?"
local prompt_input = prompt_select .. " y/n: "
lib.prompt(prompt_input, prompt_select, { "y", "n" }, { "Yes", "No" }, function(item_short)
utils.clear_prompt()
if item_short == "y" then
create_and_notify(file)
end
end)
if M.enable_async then
async.call(vim.loop.fs_close, fd)
else
create_and_notify(file)
vim.loop.fs_close(fd)
end
events._dispatch_file_created(file)
end

local function get_num_nodes(iter)
Expand All @@ -41,6 +37,73 @@ local function get_num_nodes(iter)
return i
end

local function create_file(new_file_path)
-- create a folder for each path element if the folder does not exist
-- if the answer ends with a /, create a file for the last path element
local is_last_path_file = not new_file_path:match(utils.path_separator .. "$")
local path_to_create = ""
local idx = 0

local num_nodes = get_num_nodes(utils.path_split(utils.path_remove_trailing(new_file_path)))
local is_error = false
for path in utils.path_split(new_file_path) do
idx = idx + 1
local p = utils.path_remove_trailing(path)
if #path_to_create == 0 and utils.is_windows then
path_to_create = utils.path_join { p, path_to_create }
else
path_to_create = utils.path_join { path_to_create, p }
end
if is_last_path_file and idx == num_nodes then
if M.enable_async then
async.schedule()
end
if utils.file_exists(new_file_path) then
local prompt_select = "Overwrite " .. new_file_path .. " ?"
local prompt_input = prompt_select .. " y/n: "
if M.enable_async then
local item_short = async.call(lib.prompt, prompt_input, prompt_select, { "y", "n" }, { "Yes", "No" })
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to make the prompt async? Only create_and_notify etc. needs to be async.

Copy link
Contributor Author

@yioneko yioneko Jan 22, 2023

Choose a reason for hiding this comment

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

Yes, this is mandatory in async version. In fact, lib.prompt itself is already async because of usage of callback, the only difference is that the async context of lib.prompt is not controlled. If we use callback instead of wrapping with async.call, create_and_notify may not be called in the same async context as where async.exec begins and the immediate following async.call in create_and_notify will throw error.

You can test it with overriding vim.ui.input with https://github.com/stevearc/dressing.nvim or even a simple delayer:

local original = vim.ui.input
vim.ui.input = function(opts, on_confirm)
  original(opts, function(input)
     vim.defer_fn(function() on_confirm(input) end, 0)
  end)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In principle, all callback-style functions should be wrapped to make the async context consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I still don't understand. Yes, the vim.ui.select calls themselves are asynchronous, however the callbacks appear to be executed on the main thread / context.

function M.prompt(prompt_input, prompt_select, items_short, items_long, callback)
  ...
  if M.select_prompts then
    vim.ui.select(items_short, { prompt = prompt_select, format_item = format_item }, function(item_short)
      log.line("dev", "prompt select in_fast_event=%s is_thread=%s", vim.in_fast_event(), vim.is_thread())
      callback(item_short)
    end)
  else
    vim.ui.input({ prompt = prompt_input }, function(item_short)
      log.line("dev", "prompt input in_fast_event=%s is_thread=%s", vim.in_fast_event(), vim.is_thread())
      callback(item_short)
    end)
  end
end
[2023-01-23 10:29:44] [dev] prompt input in_fast_event=false is_thread=false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because it might be called in main thread (not in async thread), we need to use async.call to transfer it to async thread, otherwise create_and_notify will throw error because async.call uses coroutine.yield under the hood.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

async.exec is not required to be called from main thread. It's possible to create new coroutine in another coroutine. The coroutine (or thread, here) is just stackful statemachine and could be created anywhere.

Would you mind I make a simple guidance for understanding of current basic async implementation? I've seen that there are plenty of misunderstandings between us and possibly with future contributors.

Copy link
Member

Choose a reason for hiding this comment

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

Yes please. A detailed explaination would be gratefully appreciated.

The outcome of this PR:

  • file system operations for create / rename are asynchronous
  • code changes are minimal and clear
  • asynchronous operations are localised i.e. not spread over multiple callbacks / functions

Unless there is a very good reason, inputs should not be asynchronous. This adds unnecessary complexity.

Copy link
Contributor Author

@yioneko yioneko Jan 23, 2023

Choose a reason for hiding this comment

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

Sorry for that I'm gonna to be off-topic, I think the referred async input codepath would not be ever reached, as we already checked the existence of file before async.exec:

if utils.file_exists(new_file_path) then
notify.warn "Cannot create: file already exists"
return
end
if M.enable_async then
async.exec(create_file, new_file_path, function(err)
.
Do you think it's safe to remove this? I just cp from the original implementation. There is no other place with async call to user input.

Copy link
Member

Choose a reason for hiding this comment

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

I see... that's a good catch!

It does look like this file_exits will fail and exit early. The local function create_file(new_file_path) call to file_exists will always pass and the "Overwrite " .. new_file_path .. " ?" prompt cannot occur.

This is existing behaviour: we can safely remove the file_exists check in create_file.

Please raise a separate PR to remove this dead code path. We will merge that first, keeping our changes small and atomic and easily revertable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that I referred the wrong variable, new_file_path -> path_to_create, sorry about that. However, the truth that this is a dead code remains; and I think we should early return instead of prompting for overwrite for already existing file.

utils.clear_prompt()
if item_short == "y" then
create_and_notify(new_file_path)
end
else
lib.prompt(prompt_input, prompt_select, { "y", "n" }, { "Yes", "No" }, function(item_short)
utils.clear_prompt()
if item_short == "y" then
create_and_notify(new_file_path)
end
end)
end
else
create_and_notify(new_file_path)
end
elseif not utils.file_exists(path_to_create) then
local err
if M.enable_async then
err = async.call(vim.loop.fs_mkdir, path_to_create, 493)
else
local _
_, err = vim.loop.fs_mkdir(path_to_create, 493)
end
if err then
notify.error("Could not create folder " .. path_to_create .. ": " .. err)
is_error = true
break
end
events._dispatch_folder_created(new_file_path)
end
end
if not is_error then
notify.info(new_file_path .. " was properly created")
end
if M.enable_async then
async.schedule()
end
-- synchronously refreshes as we can't wait for the watchers
find_file(utils.path_remove_trailing(new_file_path))
end

local function get_containing_folder(node)
if node.nodes ~= nil then
return utils.path_add_trailing(node.absolute_path)
Expand All @@ -49,7 +112,8 @@ local function get_containing_folder(node)
return node.absolute_path:sub(0, -node_name_size - 1)
end

function M.fn(node)
--TODO: once async feature is finalized, use `async.wrap` instead of cb param
function M.fn(node, cb)
node = node and lib.get_last_group_node(node)
if not node or node.name == ".." then
node = {
Expand All @@ -74,45 +138,21 @@ function M.fn(node)
return
end

-- create a folder for each path element if the folder does not exist
-- if the answer ends with a /, create a file for the last path element
local is_last_path_file = not new_file_path:match(utils.path_separator .. "$")
local path_to_create = ""
local idx = 0

local num_nodes = get_num_nodes(utils.path_split(utils.path_remove_trailing(new_file_path)))
local is_error = false
for path in utils.path_split(new_file_path) do
idx = idx + 1
local p = utils.path_remove_trailing(path)
if #path_to_create == 0 and vim.fn.has "win32" == 1 then
path_to_create = utils.path_join { p, path_to_create }
else
path_to_create = utils.path_join { path_to_create, p }
end
if is_last_path_file and idx == num_nodes then
create_file(path_to_create)
elseif not utils.file_exists(path_to_create) then
local success = vim.loop.fs_mkdir(path_to_create, 493)
if not success then
notify.error("Could not create folder " .. path_to_create)
is_error = true
break
if M.enable_async then
async.exec(create_file, new_file_path, function(err)
if cb then
cb(err)
end
events._dispatch_folder_created(new_file_path)
end
end
if not is_error then
notify.info(new_file_path .. " was properly created")
end)
else
create_file(new_file_path)
end

-- synchronously refreshes as we can't wait for the watchers
find_file(utils.path_remove_trailing(new_file_path))
end)
end

function M.setup(opts)
M.enable_reload = not opts.filesystem_watchers.enable
M.enable_async = opts.experimental.async.create_file
end

return M
41 changes: 32 additions & 9 deletions lua/nvim-tree/actions/fs/rename-file.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ local lib = require "nvim-tree.lib"
local utils = require "nvim-tree.utils"
local events = require "nvim-tree.events"
local notify = require "nvim-tree.notify"
local async = require "nvim-tree.async"

local M = {}

Expand All @@ -15,26 +16,34 @@ local function err_fmt(from, to, reason)
return string.format("Cannot rename %s -> %s: %s", from, to, reason)
end

function M.rename(node, to)
---@async
function M.rename(node, to, use_async)
if utils.file_exists(to) then
notify.warn(err_fmt(node.absolute_path, to, "file already exists"))
return
return notify.warn(err_fmt(node.absolute_path, to, "file already exists"))
end

events._dispatch_will_rename_node(node.absolute_path, to)
local success, err = vim.loop.fs_rename(node.absolute_path, to)
local success, err
if use_async then
err, success = async.call(vim.loop.fs_rename, node.absolute_path, to)
else
success, err = vim.loop.fs_rename(node.absolute_path, to)
end
if not success then
return notify.warn(err_fmt(node.absolute_path, to, err))
end
notify.info(node.absolute_path .. " ➜ " .. to)
if use_async then
async.schedule()
end
utils.rename_loaded_buffers(node.absolute_path, to)
events._dispatch_node_renamed(node.absolute_path, to)
end

function M.fn(default_modifier)
default_modifier = default_modifier or ":t"

return function(node, modifier)
return function(node, modifier, cb)
if type(node) ~= "table" then
node = lib.get_node_at_cursor()
end
Expand Down Expand Up @@ -76,17 +85,31 @@ function M.fn(default_modifier)
if not new_file_path then
return
end

M.rename(node, prepend .. new_file_path .. append)
if M.enable_reload then
require("nvim-tree.actions.reloaders.reloaders").reload_explorer()
if M.enable_async then
async.exec(M.rename, node, prepend .. new_file_path .. append, true, function(err)
if err then
notify.error(err)
end
if M.enable_reload then
require("nvim-tree.actions.reloaders.reloaders").reload_explorer()
end
if cb then
cb(err)
end
end)
else
M.rename(node, prepend .. new_file_path .. append, false)
if M.enable_reload then
require("nvim-tree.actions.reloaders.reloaders").reload_explorer()
end
end
end)
end
end

function M.setup(opts)
M.enable_reload = not opts.filesystem_watchers.enable
M.enable_async = opts.experimental.async.rename_file
end

return M
74 changes: 74 additions & 0 deletions lua/nvim-tree/async.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
---Idea taken from: https://github.com/ms-jpq/lua-async-await
Copy link
Member

Choose a reason for hiding this comment

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

There's a lot of unused codepaths in here. Let's trim it down. If we do need functionality we can refer to the source.

It looks like:
in_async
wrap
all
interrupt/abort

local co = coroutine

local M = {}

---@type table<thread, boolean>
local async_threads = {}

---Execuate an asynchronous function
---@param func function
---@param ... any The arguments passed to `func`, plus a callback receives (err, ...result)
function M.exec(func, ...)
local nargs = select("#", ...)
local args = { ... }
---@type function
local cb = function() end
if nargs > 0 then
cb = args[nargs]
args[nargs] = nil
end

local thread = co.create(func)
async_threads[thread] = true

local function step(...)
local res = { co.resume(thread, ...) }
local ok = table.remove(res, 1)
local err_or_next = res[1]
if co.status(thread) ~= "dead" then
local _, err = xpcall(err_or_next, debug.traceback, step)
if err then
cb(err)
end
else
async_threads[thread] = nil
if ok then
cb(nil, unpack(res))
else
cb(debug.traceback(thread, err_or_next))
end
end
end

step(unpack(args))
end

---Test whether we are in async context
---@return boolean
function M.in_async()
local thread = co.running()
return async_threads[thread] ~= nil
end

---Asynchronously call a function, which has callback as the last parameter (like luv apis)
---@param func function
---@param ... any
---@return any
function M.call(func, ...)
local args = { ... }
return co.yield(function(cb)
table.insert(args, cb)
func(unpack(args))
end)
end

---Asynchronous `vim.schedule`
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to do this? Please add comments here and at call sites as to why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As documented in :h lua-loop-callbacks, we cannot call some vim.api.* and all vim.fn.* in luv callbacks. So every time before these calls, we should manually call vim.schedule (async.schedule here to avoid callback hells). It's my fault to assume that we already get this; I'll add necessary comments about it.

Copy link
Member

Choose a reason for hiding this comment

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

I see. So this schedule is essentially a thread join.

I do not understand the timing; surely it should be called immediately after the .call or during the callback. It's called once at the start of create_file and once at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly we could do that for every callback, but usually this is only needed when the callback contains vim apis (or we are uncertain about this, such as user provided callback). In some cases we may not want to use vim.schedule to delay the execution of callback like a sequence of consecutive luv calls and always delaying may have potential negative impact on performance.

Copy link
Member

Choose a reason for hiding this comment

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

I see. In that case, please move the calls to schedule immediately after the calls that require this join. It should be clear exactly when and where it is required.

---See `:h lua-loop-callbacks` and `:h api-fast`. Usually this should be used before `vim.api.*` and `vim.fn.*` calls.
function M.schedule()
return co.yield(function(cb)
vim.schedule(cb)
end)
end

return M
2 changes: 1 addition & 1 deletion lua/nvim-tree/marks/bulk-move.lua
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function M.bulk_move()
for _, node in pairs(marks) do
local head = vim.fn.fnamemodify(node.absolute_path, ":t")
local to = utils.path_join { location, head }
FsRename.rename(node, to)
FsRename.rename(node, to, false)
end

if M.enable_reload then
Expand Down