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]

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. This should now work on the latest main.

---

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.

[echasnovski]

This is visibly better now, thanks! Added the next round of review.

[echasnovski]

• 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.

This should now work on the latest main. As well as calling vim.ui.select() or vim.ui.input() when inside 'mini.files' explorer.

[TheLeoP]

I just tested the latest changes on main and they work perfectly. Thanks!

[echasnovski]

To spare both of us the back and forth about mine vague "make less nesting" suggestion, I've pushed what I think is a reasonable improvement (4b300bc). It did increase line count by 12 lines, but this is probably due to comments, new robustness checks, and performance improvements (mostly by avoiding doing things when they are not needed). This should work okay now.

Please double check the new code. If you notice an incorrect behavior, please point that out. After that - squash all commits before it.

---

Now the less fun part - tests. Yes, I'd like this to be done by an explicit new mocked LSP server (there are 'tests/mock-lsp' examples for reference). It essentially should test:

• All LSP actions are done in full when they are expected to be done. With appropriate parameters at appropriate time.
• No LSP actions are done if they are expected to be not done (like not fit any kind of filter, etc.).
• Works with several active LSP servers.