feat(mini.files): make file system actions LSP aware

[TheLeoP]

• I have read CONTRIBUTING.md
• I have read CODE_OF_CONDUCT.md

Addresses #2215

This is a proof of concept to test the idea of including this kind of LSP support inside of mini.files. It may not be merged at all or it may help to add new autocmds to allow users/other plugins to implement this themselves.

A few things to take into account

• the control flow of H.fs_do.delete had to be changed to allow checking success a single time before triggering didDeleteFile notifications (actually, it seems like I broke something)
• a big part of the code simply filters the files before sending the requests/notifications to a language server as specified by the LSP spec (and, honestly, that logic should probably be extracted into a single function)
• for now, the timeout for client:request_sync is harcoded
• the code should make some considerations to support older Neovim versions (vim.glob.to_lpeg has a bug prior to 0.11 that requires manually sorting items inside brackets as a workaround, client:request_sync/client:notify are not methods prior to 0.11, etc)
• I though about checking the user permissions on a given directory to further test if the file operations will succeed, but the file permissions semantics for these kind of operations seem to be different in Linux and Windows (and, as far as I can tell, libuv does not offer an abstraction on top of it. Maybe fs_stat related function would work on Windows with the same semantics, I haven't tested it yet)

[TheLeoP]

To test the PR using lua_ls you can have a project with the following structure

test_project/
|- main.lua
|- something.lua
|- .git

.git is an empty file, I simply use it so `lua_ls` has a `root_marker`

with the following content

-- main.lua
local something = require("something").something

something()

-- something.lua
local M = {}

M.something = function()
	print("something")
end

return M

you can then nvim main.lua, : lua MiniFiles.open()m rename something.lua to something_else.lua and accept the changes with =. Neovim should prompt you for confirmation before updating the import statement on main.lua to local something = require("something_else").something

[TheLeoP]

The tests errors seem to go away if I comment out

mini.nvim/lua/mini/files.lua

Line 2898 in 134f9dd

H.lsp_will_fs_do('delete', lsp_params)

and

mini.nvim/lua/mini/files.lua

Line 2911 in 134f9dd

if success then H.lsp_did_fs_do('delete', lsp_params) end

But I'm not sure why.

In particular, the problematic lines seem to be

mini.nvim/lua/mini/files.lua

Line 2754 in 134f9dd

local clients = vim.lsp.get_clients({ method = full_method })

and

mini.nvim/lua/mini/files.lua

Line 2802 in 134f9dd

local clients = vim.lsp.get_clients({ method = full_method })

changing them to

local clients = {}

seems to also make the test pass.

Are these tests specially time sensitive? I can't think of another reason of why they would be failing

[echasnovski]

Thanks for the PR!

There are at least two good things: the LSP complexity seems to be understood and managed (the worst part for me :) ) and there is a room for making it more concise.

[echasnovski]

It seems that willCreateFiles is for files only, right? This will also send a request for a directory.

[TheLeoP]

It seems like language servers should exclude file/directories if they are not interested in those.

Edit: while looking this up, I realized that my current implementation of filters assumes that all filters should match a file for it to be considered, but it seems like any filter must match instead. This makes the implementation a bit more complex, let me know what you think

[echasnovski]

I have so many questions (mostly due to LSP complexity, not only due to the PR itself) ...

1. It seems that it is possible to make requests/notifications in bulk for every three operation kinds. I'd say it is worth taking advantage of that: it is less talking back-and-forth between the client and the server, less computation based on the client capabilities, etc.
2. There is a lot of code duplication between two functions. Either moving that to a separate function is better. Or even probably just using a single function that has something like if method:sub(1, 3) == 'did' then ... else ... end.
3. I think H.lsp_{will,did}_fs_do_method is a bit too much. Maybe supplying a method directly when calling a function is more explicit.

So all in all, my first instinct is that adding all LSP related stuff (done in bulk) in the H.fs_actions_apply() (so that it is "willXxx requests", "action", "didXxx notifications") should be less complex and more concise. Yes, it will probably require an duplicating check about if the actions is predicted to be successful, but can be worked around later. This way it can also reuse computed parameters.

[TheLeoP]

I pushed a couple of commits that should address all of these comments. The only difference is that instead of reusing the parameters for the LSP operations, I'm checking which operations succeeded for the didXxx notifications

[echasnovski]

Are these tests specially time sensitive? I can't think of another reason of why they would be failing

I don't think so. Probably, some logic has been broken. Didn't read too much into the code.

• the code should make some considerations to support older Neovim versions (vim.glob.to_lpeg has a bug prior to 0.11 that requires manually sorting items inside brackets as a workaround, client:request_sync/client:notify are not methods prior to 0.11, etc)

Restricting this functionality to only Neovim<0.11 is fine.

• I though about checking the user permissions on a given directory to further test if the file operations will succeed, but the file permissions semantics for these kind of operations seem to be different in Linux and Windows (and, as far as I can tell, libuv does not offer an abstraction on top of it. Maybe fs_stat related function would work on Windows with the same semantics, I haven't tested it yet)

I am okay with these checks for willXxx requests be on the "best effort" basis. It is the fault of the LSP specifications to have designed such a questionable (i.e. I don't understand why it is like this) approach :)

---

I'll also take a look into how to better handle the "close on lost focus" in 'mini.files'. It will probably have to be "don't close if inside non-normal buffer in a floating window" type of exception.

[echasnovski]

I'll also take a look into how to better handle the "close on lost focus" in 'mini.files'. It will probably have to be "don't close if inside non-normal buffer in a floating window" type of exception.

Okay, I've looked into this. Going the "don't close explorer if in floating window" goes against some of the use cases that originally prompted the tracking of lost focus. Like "I want to nvim . and open file picker" and in general opening a file picker when inside an explorer.

I think the best compromise here would be to temporarily adjust vim.ui.select() to set a temporary flag to ignore tracking lost cursor.

Something like this (as of the current state of this PR) seems to work:

diff --git a/lua/mini/files.lua b/lua/mini/files.lua
index 304e895f..fce1bbb7 100644
--- a/lua/mini/files.lua
+++ b/lua/mini/files.lua
@@ -1540,7 +1540,7 @@ end
 H.explorer_track_lost_focus = function()
   local track = vim.schedule_wrap(function()
     local ft = vim.bo.filetype
-    if ft == 'minifiles' or ft == 'minifiles-help' then return end
+    if H.skip_track_lost_focus or ft == 'minifiles' or ft == 'minifiles-help' then return end
     local cur_win_id = vim.api.nvim_get_current_win()
     MiniFiles.close()
     pcall(vim.api.nvim_set_current_win, cur_win_id)
@@ -2753,6 +2753,15 @@ H.lsp_will_fs_do = function(action, params)
   local full_method = 'workspace/' .. method .. 'Files'
   local clients = vim.lsp.get_clients({ method = full_method })
 
+  local ui_select_orig = vim.ui.select
+  vim.ui.select = function(items, opts, on_choice)
+    H.skip_track_lost_focus = MiniFiles.get_explorer_state() ~= nil
+    ui_select_orig(items, opts, function(...)
+      H.skip_track_lost_focus, vim.ui.select = nil, ui_select_orig
+      on_choice(...)
+    end)
+  end
+
   -- TODO(TheLeoP): configurable timeout
   local timeout = 1000
   for _, client in ipairs(clients) do

One thing to extra consider is whether there will be any side effects if vim.ui.select() is not called as a result of the LSP request.

---

Thinking about it more, maybe it is a good idea to have this kind of mock in general for the whole duration of the explorer. Will also work nicely with something like 'mini.snippets' (which can use vim.ui.select to show matched snippets). Hmm... Need to think a bit more.

[echasnovski]

Thinking about it more, maybe it is a good idea to have this kind of mock in general for the whole duration of the explorer. Will also work nicely with something like 'mini.snippets' (which can use vim.ui.select to show matched snippets). Hmm... Need to think a bit more.

Yeah, ignoring tracking lost focus while vim.ui.select() (or vim.ui.input()) is active is probably a good idea anyway. Here is my current WIP solution.

I tried it with this PR and:

• With using 'mini.pick' for vim.ui.select renaming a single file works (with lua_ls), renaming two - does not. Because it tries to spawn two 'mini.pick' windows in parallel, which is not possible and there are cryptic errors. I think using vim.ui.select() for this kind of stuff in core is a mistake (especially with coroutine kind of magic). But at least this is a one more argument for bulk processing LSP requests.
• With 'snacks.nvim' for vim.ui.select renaming even a single just doesn't work. No LSP related adjustments are made.

[TheLeoP]

Thanks for your feedback! I'll be a bit busy today and tomorrow, so I'll address your comments and I'll keep working on this PR on Sunday

[echasnovski]

• With using 'mini.pick' for vim.ui.select renaming a single file works (with lua_ls), renaming two - does not. Because it tries to spawn two 'mini.pick' windows in parallel, which is not possible and there are cryptic errors. I think using vim.ui.select() for this kind of stuff in core is a mistake (especially with coroutine kind of magic). But at least this is a one more argument for bulk processing LSP requests.

Soo... I spent some time investigating in the hopes of opening an issue in neovim/neovim to drop the vim.ui.select() + coroutine solution. Key takeaways:

• The reason for this approach was feat(lsp): run handler in coroutine to support async response neovim/neovim#21026.
• Many consecutive calls that make window/showMessageRequest (which is for LuaLS is done after workspace/didRenameFiles) work with built-in vim.ui.select() and some simple async one. But it doesn't work with all currently popular vim.ui.select() implementations: 'mini.pick', 'snacks.picker', 'fzf-lua'. My guess is that all of them use coroutines themselves to have optimized behavior.
• I don't think Neovim core is going to change anything since it was a result of a wider change of running with coroutines. Hence I am not keen on spending countless hours arguing that for core.
• The best way forward looks to me like:
  • Make only a single LSP request/notify per method+sync pair. I.e. aggregate create, rename, delete actions and advertise them to the LSP servers once per synchronization. This will minimize issues with all vim.ui.select implementations.
  • Figure out a way for 'mini.pick' to have two and more pickers "in the queue". Maybe only for vim.ui.select(). Right now it throws an error, which is not nice.

---

Repro steps for what I did:

• Create even more minimal Lua project:

  Test Lua project

  'main.lua':

  local uuu = require('uuu')
  local vvv = require('vvv')

  'uuu.lua':

  return 'uuu'

  'vvv.lua':

  return 'vvv'

• Create an 'init.lua' with a separate NVIM_APPNAME:

  ~/.config/nvim-repro/init.lua

  -- vim.ui.select() implementation =============================================
  
  -- -- Custom implementation
  -- local n = 0
  -- vim.ui.select = function(items, opts, on_choice)
  --   local win_id
  --   local width = math.ceil(0.25 * vim.o.columns)
  --   local height = math.min(vim.o.lines, #items)
  --
  --   local buf_id = vim.api.nvim_create_buf(false, true)
  --   local lines = vim.tbl_map(opts.format_item or tostring, items)
  --   vim.api.nvim_buf_set_lines(buf_id, 0, -1, false, lines)
  --
  --   n = n + 1
  --   local win_opts = { relative = 'editor', row = n, col = n, height = height, width = width, border = 'single' }
  --   win_id = vim.api.nvim_open_win(buf_id, true, win_opts)
  --   vim.api.nvim_win_set_cursor(win_id, { 1, 0 })
  --
  --   -- Choose and close
  --   local close = function(idx)
  --     vim.api.nvim_win_close(win_id, true)
  --     vim.api.nvim_buf_delete(buf_id, { force = true })
  --     on_choice(items[idx], idx)
  --   end
  --
  --   local choose = function() close(vim.api.nvim_win_get_cursor(win_id)[1]) end
  --   vim.keymap.set('n', '<CR>', choose, { buffer = buf_id })
  --
  --   local quit = function() close() end
  --   vim.keymap.set('n', '<Esc>', quit, { buffer = buf_id })
  -- end
  
  -- From 'mini.pick'
  vim.pack.add({ 'https://github.com/nvim-mini/mini.nvim' })
  require('mini.pick').setup()
  
  -- -- From 'fzf-lua'
  -- vim.pack.add({ 'https://github.com/ibhagwan/fzf-lua' })
  -- require('fzf-lua').setup()
  -- require('fzf-lua').register_ui_select()
  
  -- -- From 'snacks.nvim'
  -- vim.pack.add({ 'https://github.com/folke/snacks.nvim' })
  -- require('snacks').setup({ picker = { enable = true } })
  
  -- Helpers for `workspace/didRenameFiles` notifications =======================
  _G.start_luals = function()
    local config = { name = 'lua-ls', cmd = { 'lua-language-server' }, root_dir = vim.fn.getcwd() }
    _G.client_id = vim.lsp.start(config)
  end
  
  local make_didrename_params = function(from_name, to_name)
    local cwd = vim.fs.abspath(vim.fn.getcwd())
    local old = 'file://' .. vim.fs.joinpath(cwd, from_name)
    local new = 'file://' .. vim.fs.joinpath(cwd, to_name)
    return { files = { { oldUri = old, newUri = new } } }
  end
  
  _G.make_one_notify = function()
    local client = vim.lsp.get_client_by_id(_G.client_id)
    client:notify('workspace/didRenameFiles', make_didrename_params('uuu.lua', 'uuu2.lua'))
  end
  
  _G.make_two_notify = function()
    local client = vim.lsp.get_client_by_id(_G.client_id)
    client:notify('workspace/didRenameFiles', make_didrename_params('uuu.lua', 'uuu2.lua'))
    client:notify('workspace/didRenameFiles', make_didrename_params('vvv.lua', 'vvv2.lua'))
  end
  
  _G.make_two_notify_simul = function()
    local cwd = vim.fs.abspath(vim.fn.getcwd())
    local old_1 = 'file://' .. vim.fs.joinpath(cwd, 'uuu.lua')
    local new_1 = 'file://' .. vim.fs.joinpath(cwd, 'uuu2.lua')
    local old_2 = 'file://' .. vim.fs.joinpath(cwd, 'vvv.lua')
    local new_2 = 'file://' .. vim.fs.joinpath(cwd, 'vvv2.lua')
    local params = { files = { { oldUri = old_1, newUri = new_1 }, { oldUri = old_2, newUri = new_2 } } }
  
    local client = vim.lsp.get_client_by_id(_G.client_id)
    client:notify('workspace/didRenameFiles', params)
  end

• NVIM_APPNAME=nvim-repro nvim -- main.lua uuu.lua vvv.lua from the root of the test Lua project. Assumes lua-language-server is installed systemwide.

• :lua start_luals() and wait for it to load (diagnostic hints will be shown in sign column and statusline).

• :lua make_one_notify() and accept. It works. Revert its changes.

• :lua make_two_notify_simul() and accept. It works. Revert its changes.

• :lua make_two_notify(). It works only for built-in and custom minimal vim.ui.select(), but not for 'mini.pick' (throws error), 'fzf-lua' (only shows one selection window and acts only on one notification), 'snacks.picker' (just does nothing).

[TheLeoP]

The bulk LSP request/notifications should be working now. I left them on separated commits to make reviewing a bit easier.

Let me know if you have any more feedback regarding the implementation. After that is done, I'll focus on creating some tests. What would be your preference for testing this? My guess is that it would be to mock a language server.

I can take a look at mini.pick later to see if I can do something to allow it to queue vim.ui.select calls.