Issafalcon / lsp-overloads.nvim

Extends the native nvim-lsp handlers to allow easier navigation through method overloads.
MIT License
88 stars 4 forks source link

added toggling #23

Closed Polynominal closed 1 year ago

Polynominal commented 1 year ago

resolve #18

I didn't add a binding as I am too lazy to think of a good binding. Using this right now and it appears to work quite well so I am submitting it here, very much in favor of consensual lsp otherwise its really bad for things like c# Func / Actions as params eg () => { ... it keeps showing up here... }

I am new to the nvim plugin stuff so I may have messed up the docs

Issafalcon commented 1 year ago

Excellent! Thanks for this @Polynominal. I'll have a good look over this asap.

Issafalcon commented 1 year ago

Just had a run through with this code, and checked it over. I have a couple of points:

  1. I like the 'enabled' option. Maybe calling it something like 'enable_auto_trigger' to reflect the fact that what it's really doing is preventing the triggers_chars from being the thing that causes the signature popup to appear
  2. I don't think this completely solves #18 because whilst this allows toggling the signature popup ON, when 'enabled' is false, it doesn't allow the signature_window to then be removed again using a toggle switch. Also, when 'enabled' is true, it still doesn't allow toggling the popup when in insert mode, via some key-binding. I think this was the main issue in #18 .

Also, I happened to be doing a bit of work to re-factor the code base to make it more OOP based, and tidy some things I wasn't happy with. If you were happy to resolve the conflicts and rework the PR a bit with the suggestions above in mind, that would be great! You might find it easier using the new OOP style.

Polynominal commented 1 year ago

I didn't add any key binds as I use which-key where I define all my keybinds, I don't have a personal use case for it so I'd be glad if someone else contributed.

In this PR there are 2 ways in which the display appears

  1. When the 'enabled' bool is true and you start typing within the signature
  2. You manually request it with the vim command, even if 'enable' is set to false

I approve of the name change. I should clarify that what this pr means by 'enable' is if the window will appear when you are typing, it does not represent the state of the actual display, I made that decision so the plugin is more versatile.

Additionally, it allows the display to be shown on manual request only by the user, I make the assumption that if a user requests the display, any existing 'enabled' settings are to be ignored as the user has clearly displayed their intent. Therefore enabling the 'display only when I tell you to' behavior.

I do save the window handle as part of lsp-overloads, such that if you toggle it, any existing window will be removed

  if (not M.enabled and M.open_fwin) then 
    vim.api.nvim_win_close(m.open_fwin)
    M.open_fwin = nil
  end

I think there is some confusion regarding the window not disappearing, it will only disappear if the 'enable' is set to false and its already open, in some cases a toggle might not work as expected such as when enabled is set to false, the display is manually triggered and user toggles again thereby enable goes to true instead of false.

What are you thoughts on something like this?

M.visible = function()
    return M.open_fwin ~= nil
end
M.hide = function()
  if (M.visible()) then
     vim.api.nvim_win_close(M.open_fwin)
  end
end

Note that hiding has no impact on the 'enabled' state, such that the signature window will appear again as you start typing.

Thanks for the feedback,

I'd be happy to make the changes you suggested, but I would appreciate it if you could introduce the keybinds, if you cannot do that then I can look into it. It's technically feasible I am just lazy as I don't want to read source code for which key to figure it out considering I will have no benefit from it and will have to strip that functionality out for my own use case.

Polynominal commented 1 year ago

I submitted changes regarding 1) mainly splitting Automatic display toggles and the actual Display toggle. Such that we have

:LspOverloadsSignature
:LspOverloadsSignatureAutoToggle
:LspOverloadsSignatureDisplayToggle

The actual part about closing the window was bugged as it wouldn't pick up the fwin handle, now its fixed so it should work as intended.

Issafalcon commented 1 year ago

Hi @Polynominal - Sorry it's taken so long for me to get back to this! It's been a busy couple months for me!

I've taken some of your ideas and tailored them to fit the new OO model of the signature. It works pretty well now I think.

I simplified the commands a bit so there are only two now. One to manually trigger the popup, and one to toggle the auto-display setting. I've also fixed an issue with keybind conflicts, so any conflicting keybindings are automatically reset back to the users originals when the signature popup is closed. Should help with some other annoyances, and allows the open and close keybindings to be the same, acting as a toggle.

The README hopefully explains this all clearly.

Thanks again for the contribution!