Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

That aggressive popup (again) #26

Closed
elextr opened this issue Nov 14, 2023 · 18 comments
Closed

That aggressive popup (again) #26

elextr opened this issue Nov 14, 2023 · 18 comments

Comments

@elextr
Copy link

elextr commented Nov 14, 2023

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
Copy link
Owner

techee commented Nov 15, 2023

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:

#indicator index; SCI_INDICSETFORE; SCI_INDICSETALPHA; SCI_INDICSETOUTLINEALPHA; SCI_INDICSETSTYLE
#indicator index is an integer from 8 to 31 and should be different for every style
diagnostics_error_style=13;#ff3030;70;255;1
diagnostics_warning_style=14;#ee00ee;70;255;1
diagnostics_info_style=15;#909090;70;255;14
diagnostics_hint_style=16;#909090;70;255;14

@elextr
Copy link
Author

elextr commented Nov 16, 2023

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
Copy link
Author

elextr commented Nov 16, 2023

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

Will keep an eye out for it.

@elextr
Copy link
Author

elextr commented Nov 16, 2023

I implemented symbol highlighting in the latest version.

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

@techee
Copy link
Owner

techee commented Nov 16, 2023

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
Copy link
Owner

techee commented Nov 16, 2023

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
Copy link
Author

elextr commented Nov 16, 2023

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
Copy link
Owner

techee commented Nov 16, 2023

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
Copy link
Author

elextr commented Nov 17, 2023

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
Copy link
Author

elextr commented Nov 17, 2023

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
Copy link
Owner

techee commented Nov 17, 2023

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
Copy link
Owner

techee commented Nov 17, 2023

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
Copy link
Author

elextr commented Nov 18, 2023

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

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 😉, 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
Copy link
Author

elextr commented Nov 18, 2023

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

@techee
Copy link
Owner

techee commented Nov 19, 2023

hover_popup_max_lines should be your friend now.

@elextr
Copy link
Author

elextr commented Nov 20, 2023

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
Copy link
Owner

techee commented Nov 20, 2023

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

@techee
Copy link
Owner

techee commented Nov 23, 2023

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.

@techee techee closed this as completed Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants