stevearc / dressing.nvim

Neovim plugin to improve the default vim.ui interfaces
MIT License
1.82k stars 32 forks source link

Enhancement: expose ID of window created by `vim.ui.input` #58

Closed smjonas closed 2 years ago

smjonas commented 2 years ago

Hi, I'm currently trying to add proper support for dressing.nvim in my plugin inc-rename.nvim (the current implementation has issues, see https://github.com/smjonas/inc-rename.nvim/issues/17). Basically, I need to control the window created by dressing.nvim from the command line and change the input text while the user is typing. For this to work, I need to get the ID of the window created by vim.ui.input.

At the moment, I am doing the following to find the ID:

-- Open the input window and find the buffer and window IDs
vim.ui.input({}, function() end)
vim.schedule(function()
  for _, win_id in ipairs(vim.api.nvim_list_wins()) do
    local bufnr = vim.api.nvim_win_get_buf(win_id)
    if vim.bo[bufnr].filetype == "DressingInput" then
      -- found the window ID
    end
  end
end)

This almost works but using vim.schedule is problematic as that doesn't work well with Nvim's command preview callback. Omitting vim.schedule won't work because you use schedule_wrap in input.lua here (so I need to schedule my code to run after yours):

https://github.com/stevearc/dressing.nvim/blob/96b09a0e3c7c457140303c796bd84f13cfd9dbc0/lua/dressing/input.lua#L276

Therefore, I am proposing the following enhancements:

Please let me know if you have any another ideas / suggestions! I can send a PR if you want :)

stevearc commented 2 years ago

The original reason why vim.ui.input was wrapped was to work around an issue during neovim startup (see #15). I've updated the wrapper to only use schedule_wrap during startup, and to execute synchronously otherwise. I believe that inc-rename will only ever be called after startup, so this should make all vim.ui.input calls from inc-rename synchronous. Does that work for you?

I'm a little more hesitant to return the window ID from the call because this starts to change the default built-in API. There is some precedent for this, as I decided to add an API extension in e607dd99aeb5ce21e9a3d8f4c028650db12bd3af, but I want to be extra cautious about these sorts of things and preferably do it in a way that is guaranteed to never conflict with the built-in API.

smjonas commented 2 years ago

Thanks a lot, this should resolve my main issue. I'm fine with leaving it like this if you don't want to change the API since there exists a workaround to get the ID. Still, I'm curious what exactly the API contract is here / why is returning something instead of nil problematic?

stevearc commented 2 years ago

Two reasons:

  1. It's possible that neovim will refactor vim.ui.input to return some value in the future, at which point it will clash. Not incredibly likely, but not beyond the realm of possibility.
  2. It introduces a non-standard API that users or plugin authors could start to depend on. They might start to expect that if vim.ui.input returns nil, it doesn't open a floating window. That in turn might cause bugs or unexpected behavior if someone uses a vim.ui.input implementation that isn't dressing.nvim.

These are small potential issues and I could be convinced otherwise, but in general I think that maintaining exact API compatibility is valuable and should not be broken lightly.

stevearc commented 2 years ago

Note that I had to revert this change to vim.ui.select because it turns out we need the schedule_wrap for other reasons when opening the select windows. vim.ui.input is unchanged, so hopefully this will not affect you.

smjonas commented 2 years ago

Thanks for letting me know! No, as it stands now this won't affect me :)