techee / geany-lsp

LSP plugin for the Geany editor
GNU General Public License v2.0
19 stars 1 forks source link

That aggressive popup (again) #26

Closed elextr closed 1 year ago

elextr commented 1 year ago

Put the cursor on a symbol, delay for a moment, right click, the symbol popup overwrites the menu making it impossible to select some of the items in the menu. And once its there its persistent, have to go to another symbol to get rid of it, also of course closing the menu.

Its very timing dependent, only happened once or twice. Maybe something like the right click happening after the popup has been idle queued, but not rendered yet (wild ass guess).

techee commented 1 year ago

Its very timing dependent, only happened once or twice. Maybe something like the right click happening after the popup has been idle queued, but not rendered yet (wild ass guess).

Something similar I think - I check whether Scintilla has focus and if not, don't perform the hover request. But once the request was sent, I didn't check if Scintilla had focus when the asynchronous response arrived and just displayed it. So if you manage to trigger the context menu between sending the request and receiving the response, you get this problem.

I hope it's fixed now (but it's so hard to trigger that I'm not sure).

And this thing is keybindingable now so you can disable the annoying hover popup, sacrifice Geany documentation, and assign it to F1. This is the right way to use this feature ;-).

By the way, I implemented symbol highlighting in the latest version. But I also made an incompatible change in the config file where I prepend Scintilla indicator index in front of these lines so you'll have to add them to your user config file, e.g. by copying them from the global config file:

https://github.com/techee/geany-lsp/blob/bf1c5f840d1fbe170f401efe0f05e6d766ac124b/plugins/lsp/data/lsp.conf#L26-L31

elextr commented 1 year ago

add them to your user config file,

Can't, the Tools->LSP Client-> now shows the right click menu, not the config files ...

Oh wait they are there just buried.

Why add all those window specific functions to that menu? I guess it doesn't hurt, but I doubt anybody will use it.

elextr commented 1 year ago

but it's so hard to trigger that I'm not sure

Will keep an eye out for it.

elextr commented 1 year ago

I implemented symbol highlighting in the latest version.

erm, what do you mean, can't see any difference

techee commented 1 year ago

erm, what do you mean, can't see any difference

This thing:

Screenshot 2023-11-16 at 10 14 03

But if you use some theme with gray background, you may not see it with the default settings. Check the global config file for the new config item where you can change the color.

techee commented 1 year ago

Why add all those window specific functions to that menu? I guess it doesn't hurt, but I doubt anybody will use it.

I'll probably move them somewhere else. This is mainly for discoverability of the particular features, people will assign keybindings for the more frequently used ones.

elextr commented 1 year ago

But if you use some theme with gray background, you may not see it with the default settings.

ok, doesn't work on "invert syntax colours" (the lazy mans dark theme :-)

ok, copied setting and changed #000000 to #ffffff

techee commented 1 year ago

I've just added renaming based on the highlighting info (using Scintilla's support of multiple cursors). This works for renaming inside single file only, I'll add a full-blown project-wide rename later.

elextr commented 1 year ago

I've just added renaming based on the highlighting info (using Scintilla's support of multiple cursors). This works for renaming inside single file only,

Ok, seems to work with single test :-)

I'll add a full-blown project-wide rename later.

A comment for that which even applies to the single file version. Since it can edit things the user can't see maybe best to do as Vscode does, show a "Colomban inset":tm: (or just a boring popup/dialog) with a list of the files to be changed and the lines to be changed in them and highlight the change and have a checkbox in front of each file and line so the user can verify they want that file/line changed and then click apply or discard. Vscode calls this a "Refactor Preview".

The use-case for this is to change only some instances to reference a different name instead.

elextr commented 1 year ago

Back to the OP topic, we gotta think of something easy(ish) to do with that popup, I just put the cursor on vector<xxxxx> and got this:

Screenshot from 2023-11-17 10-07-47

PS thats a 27" QHD display

Edit: And if I put the cursor on seqs in the line above the popup I get one that contains offset and size in bytes, C++ is not C, its a high level language (sorta) nobody cares about the bytes offset and not really much about the size either. Thats just wasted popup space. That said, I'm not sure what filters to apply, the useful info seems to be all over the place, its in line 1 of the popup above, but for the seqs popup its in the second group, the first line says just "field seqs" so thats not any use.

techee commented 1 year ago

A comment for that which even applies to the single file version.

I don't think it's necessary for single-file rename because it's easily undoable. In vscode it corresponds to "change all occurrences" and behaves the same way

Since it can edit things the user can't see maybe best to do as Vscode does, show a "Colomban inset"™️ (or just a boring popup/dialog) with a list of the files to be changed and the lines to be changed in them and highlight the change and have a checkbox in front of each file and line so the user can verify they want that file/line changed and then click apply or discard. Vscode calls this a "Refactor Preview".

So I just implemented the rename but only used a (hopefully scary enough) dialog warning users about how terrible things might happen and that they should have all changes committed in VCS. In my experience, you can't really review the whole change in the simple GUI you propose anyway and it's really best to do it on top of a committed state and see the differences against the last commit.

In any case, at least showing the affected files would be good because sometimes one expects the rename to have just a local scope and it turns out to be much bigger afterwards. Regarding the LSP implementation, I'm now doing just a "shallow" implementation of the individual LSP features to have the basic communication with LSP servers done and don't want to get stuck on one feature for too long - I'll possibly improve various things later (and global renaming isn't something I use too frequently so it will rather have lower priority for me).

techee commented 1 year ago

we gotta think of something easy(ish) to do with that popup

Keep the feature disabled, simple :-).

OK, but for other popups like signature help or diagnostics I stole utils_wrap_string() from Geany and just forgot to use it for hover too - shouldn't be so wide at least now.

The other things I can think of - have a config file option setting the max line number of the hover popup and display ... when trimmed. Or have some configurable regex pattern identifying the end of the useful part of the message - but as you say, it's hard to say if it's possible.

I was thinking in the future to implement our own popup - the one provided by Scintilla is extremely limited, it can only show plain text. For instance for function calltip it would be nice to show the currently typed parameter in bold. The popup could be scrollable for hints which would solve the problem I think.

elextr commented 1 year ago

In vscode it corresponds to "change all occurrences" and behaves the same way

AFAICT it when using "Rename Symbol" it shows the refactor preview if any of the occurrences are off screen, it only does it automatically if all changes are visible. And if the symbol exists outside the current scope all those are shown in the refactor preview as well, but with the check boxes not checked.

So I just implemented the rename but only used a (hopefully scary enough) dialog warning users about how terrible things might happen and that they should have all changes committed in VCS.

sh-sh-sh-udddddddd-er, @elextr is scared. :wink:

But really thats not a good answer, encouraging committing of half done changes to see diffs due to refactor is a bad idea:tm:.

I was thinking in the future to implement our own popup

For vscode the "Refactor Preview" is shown in the equivalent of the Geany message pane, not as a popup. That seems to have changed very recently AFAIK, with the addition of the check boxes and accept/dismiss buttons. That would be easier for Geany too.

In any case, at least showing the affected files would be good

Thats a start,then just add line numbers and line contents and highlight changes and checkboxes ... ahem, yes well, lets at least start with files.

Keep the feature disabled, simple :-).

Ok, so long as it works when its disabled ;-P

I was thinking in the future to implement our own popup

Yeah, if it accepted Pango markup for eg. Neil can't do that since its not portable to non-GTK, but its ok for us. Or maybe Markdown.

The other things I can think of - have a config file option setting the max line number of the hover popup and display ... when trimmed. Or have some configurable regex pattern identifying the end of the useful part of the message - but as you say, it's hard to say if it's possible.

Of course the answer is another option, this is Geany after all :wink:, maybe only show the first line/part, it seems to be intended as a heading, and have a keybinding to show the whole war and peace. But don't know how that will work with other not clangd LSPs, guess its a per language setting.

elextr commented 1 year ago

hehe, I tried Python, open scripts/create_php_tags.py and hover on line 54 urlopen ... and I thought clangd was bad :-)

techee commented 1 year ago

hover_popup_max_lines should be your friend now.

elextr commented 1 year ago

Better :-), can we have a "show to first blank line only" option to see how well it works? Both pylsp and clangd seem to put a summarised version first followed by a blank line.

techee commented 1 year ago

OK, I added hover_popup_max_paragraphs where you can specify how many blank line separated paragraphs are shown.

techee commented 1 year ago

I'm closing this one as I believe/hope the OP is fixed and that there are some workaround settings now to make this feature more useful. Feel free to reopen if there's something else left.

The right long-term fix here is #28.