fix!: add proper support for dressing.nvim

smjonas commented 2 years ago

Finally, after a lot of struggles I have managed to add actual support for dressing.nvim! :tada: This PR depends on #16.

@RaafatTurki I am pretty confident that the issues you reported here should be resolved with this update :)

@Akianonymus Could you both please test out this feature (again :sweat_smile:) on the latest Neovim nightly version? This PR introduces a breaking change: you no longer need to call require("inc_rename").rename() but instead simply type out the :IncRename command (or use a keymap which does that for you).

For this to work, use the following in setup:

require("inc_rename").setup {
  input_buffer_type = "dressing",
}

The text you type in the command line will be mirrored in dressing's input buffer.

Closes #17.

smjonas commented 2 years ago

The only remaining issue I have found is that the title of the dressing window is not immediately visible or is misplaced: Using vim.cmd("redraw") doesn't seem to fix that.

RaafatTurki commented 2 years ago

Very nice work! A few notes:

cmp wildmenu gets cut off the moment I type :Inc
after every rename the dressing buffer gets filled with the line the cursor was on and this message shows and would only go away after pressing enter again
the LSP server (gopls in this case) doesn't seem to see the updated files upon renaming across multiple files gopls diagnostic after renaming sub -> subb across files
about the dressing title issue you've mentioned, it seems to be at the top left corner first, then disappears, then goes to its correct position. those stages switch upon any update (adding or deleting a character)
I miss the cursor being inside the dressing buffer

smjonas commented 2 years ago

Thanks for testing!

cmp wildmenu gets cut off the moment I type :Inc

I was able to reproduce that, although I have no idea how one could fix this honestly... It's possibly that it's the same kind of problem as with the input window title (some kind of redrawing problem, ~maybe even a Neovim bug since resizing the window makes it go away). I wish it was as easy as vim.cmd("redraw") but it's not~. Edit: apparently, redrawing is not allowed in the preview callback: https://github.com/neovim/neovim/issues/20094

after every rename the dressing buffer gets filled with the line the cursor was on

I can't directly reproduce this (there is no Press ENTER or type command to continue prompt for me) but if I press <C-f> while renaming to leave command line mode, the whole buffer contents get copied into the input buffer which is definitely wrong and should be fixable ^^

the LSP server (gopls in this case) doesn't seem to see the updated files upon renaming across multiple files

That seems like it's not a new issue. I think this is more of a limitation of Nvim's LSP implementation itself (you need to save all affected buffers after renaming across multiple files). Correct me if I'm wrong though, i.e. if this doesn't occur with vim.lsp.buf.rename.

about the dressing title issue you've mentioned, it seems to be at the top left corner first, then disappears, then goes to its correct position. those stages switch upon any update (adding or deleting a character)

~Yup, I've opened this issue, maybe the author has a hunch what the problem might be :) (https://github.com/stevearc/dressing.nvim/issues/61)~ Seems to be the same issue as the Neovim bug mentioned above.

I miss the cursor being inside the dressing buffer

Good point, I have added a basic fake cursor (https://github.com/gelguy/wilder.nvim/issues/156#issuecomment-1237291539).

smjonas commented 2 years ago

@RaafatTurki I think for the nvim-cmp and dressing title issues we'll have to wait for an upstream fix :) Could you check again for your second issue? Because I can't reproduce the issue (I tested with sumneko_lua and gopls).

RaafatTurki commented 2 years ago

I see The second issue is fixed with this commit

That seems like it's not a new issue. I think this is more of a limitation of Nvim's LSP implementation itself ...

You're right, my bad.

smjonas commented 2 years ago

You know what - let's just merge this, what could go wrong :smile:?

RaafatTurki commented 2 years ago

Did you just YOLO a merge hahahah, well since this is merged now maybe you can help me figure out how to use the new thing I want to have it triggered by a function call that can be invoked by anything (not just a keybinds), for comparison the readme says

vim.keymap.set("n", "<leader>rn", function()
  return ":IncRename " .. vim.fn.expand("<cword>")
end, { expr = true })

but I want something like

function LspRename()
  -- some logic goes here
end

vim.keymap.set("n", "<leader>rn", function() LspRename() end)

that way I can use LspRename in all sorts of things

smjonas commented 2 years ago

Did you just YOLO a merge hahahah

Yes haha I just wanted to get this out of my way after the countless issues that popped up :rofl: Didn't think this would be so tedious to implement. Pretty happy with how it turned out though.

that way I can use LspRename in all sorts of things

Well, since the plugin highly depends on Nvim's command preview feature you need to be in command mode for this to work. You could do something like this:

function LspRename()
    local new_name = "example"
    vim.api.nvim_feedkeys(":IncRename " .. new_name, "n", false)
end

vim.keymap.set("n", "<leader>rn", function() LspRename() end)

Is that good enough for your needs?

RaafatTurki commented 2 years ago

I understand, yes I can work with that, thanks a lot for the snippet!

I should also point out that this merge does get in the way of achieving auto completed LSP renames since the dressing buffer no longer has a cursor.

Good point, I have added a basic fake cursor

while this would look nice, I don't think it can trigger something like a cmp completion, I couldn't test since the fake cursor doesn't show up on my end in the 1st place.

I do understand that the new approach simplifies a lot of unneeded complexity, this isn't a feature request, it's best to wait and see what nvim brings more onto the table.

I think an another approach would be to create a cmp source cmp-inc-rename which auto completes :IncRename itself with cmp sources like rg, buffer, spell .. etc, I'll look further into this

This isn't a highly needed feature but it would still be nice to know what you think about it

smjonas commented 2 years ago

I should also point out that this merge does get in the way of achieving https://github.com/stevearc/dressing.nvim/issues/55 since the dressing buffer no longer has a cursor.

Oh that's a good point that I didn't consider before.

I don't think it can trigger something like a cmp completion

Yes, that's true - it's basically a space character with a highlight haha. That sounds almost like multiple cursors to me so I don't think that can be currently implemented.

Your idea with adding a new cmp source sounds promising. Let me know if you've decided to implement this as a plugin! (We might even add it to inc-rename itself). As you said, if Neovim decides to implement something like https://github.com/neovim/neovim/issues/19856, that would open up a whole new world of possibilities and pretty much solve all the drawbacks of the current way of doing things!

RaafatTurki commented 2 years ago

it's basically a space character with a highlight

Ah, that explains why it didn't show up on my end, I had the Cursor hl group set to nothing

Your idea with adding a new cmp source sounds promising

Yeah I've figured this is the cleanest thing we could have

Let me know if you've decided to implement this as a plugin

I will start looking into this within this week, development will most likely be slow at 1st since I've never made a cmp source b4 but we'll cross that bridge when we get to it

if Neovim decides to implement something like ...

Indeed, we could have "real" dressing support then

smjonas / inc-rename.nvim

fix!: add proper support for dressing.nvim #18