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

Improve Go to Tag menu #3542

Closed
wants to merge 2 commits into from
Closed

Improve Go to Tag menu #3542

wants to merge 2 commits into from

Conversation

ntrel
Copy link
Member

@ntrel ntrel commented Aug 13, 2023

Limit width and ellipsize like #3495.
Fallback to file & line if no signature.
Show tooltip with file & line.

For screenshots and code by @b4n (unchanged), see: #3475 (comment)

Fallback to file & line if no signature.
Show tooltip with file & line.
@techee
Copy link
Member

techee commented Aug 13, 2023

I don't know but wouldn't it be better to do it the other way round - i.e. display the file name and line number directly in the list as before and show the signature as a tooltip? What I get right now doesn't seem very useful and if you for instance want to navigate to gpointer data, one would have to go through many items in the list below and wait for the tooltip to appear to locate the right one.

Screenshot 2023-08-13 at 21 01 42

@techee
Copy link
Member

techee commented Aug 13, 2023

Also, regarding the ellipsize, I think it would be useful to split it into two parts:

  1. path plus line number - max length 40 with PANGO_ELLIPSIZE_START
  2. signature - max length 40 with PANGO_ELLIPSIZE_END

and then join them for the resulting string. This way you'll always have the file name preserved and only the leading path truncated (which isn't so interesting I think).

@ntrel
Copy link
Member Author

ntrel commented Aug 14, 2023

@techee The list of variables would be much more useful if it showed the scope, e.g.:
image

I'll add that.

@techee
Copy link
Member

techee commented Aug 21, 2023

I'm still not very happy with the result:

Screenshot 2023-08-21 at 22 19 22

I understand that seeing the signature may be useful in some cases (like multiple constructors) but it doesn't answer the question where will the jump go which is stupid when multiple files are involved (and going through the list and waiting for the tooltips to appear is tedious).

I'd suggest making this configurable so people can chose what fits their needs (I'd lean towards defaulting to the previous behavior). The tooltip could then show the other piece of information - for filename:line_number it would show the signature and for signature it would show filename:line_number (I don't think that it's necessary in the tooltip to show both, it's just an unnecessary duplication of information).

What do you think?

@elextr
Copy link
Member

elextr commented Aug 22, 2023

What one IDE does that seems to work for autocomplete (its goto is precise AFAICT so no list needed for that) is to display the list of unique names only and then for the selected one a fixed width multiline subwindow shows one of the the overloads with full file, line, and signature and an up/down arrow to cycle them.

I expect it does not simply use a second level menu showing all the overloads is that for the example I tried get has 24 overloads and most of the ones I cycled to were multiple lines, so the submenu would be HUUUUUGE.

Maybe a calltip can be used for the second level to simplify implementation?

@elextr
Copy link
Member

elextr commented Aug 22, 2023

@eht16 the windows CI appears to be broke again.

@kugel-
Copy link
Member

kugel- commented Aug 22, 2023

The file is absolutely necessary information for me. I typically edit multiple checkouts of the same code (managing different features/bugs/tasks simultaneously, so I can work on one thing while waiting on review for the others things). Often this involves the same files. So I quickly need to know the location where to jump or I will edit in the wrong checkout (happens often enough already, please don't make it even harder).

@ntrel
Copy link
Member Author

ntrel commented Aug 22, 2023

I can make it configurable.

@techee how are you getting those text declarations? It seems like there should be some scope info probably.

What about a hybrid where if there's no parameter list and no scope info then show file and line.

@ntrel
Copy link
Member Author

ntrel commented Aug 22, 2023

BTW this pull only adds info to the menu so I'd like to merge it. Though maybe you want the configuration option before going ahead with refinements to the merged pull that shows signatures.

@techee
Copy link
Member

techee commented Aug 22, 2023

What about a hybrid where if there's no parameter list and no scope info then show file and line.

Nobody will understand that. For users it will just confusing why they sometimes see filenames and sometimes the signature. And as for me (and apparently @kugel- and @elextr too), always seeing file names is just necessary.

@techee how are you getting those text declarations? It seems like there should be some scope info probably.

Frankly, I did't properly updated your branch on my machine, they are present now, sorry. But the goto popup has to work reasonably well for all languages, such as Python where you don't have the type information or some other parsers that don't support scope parsing (or there's no concept of scope in the languages themselves).

BTW this pull only adds info to the menu so I'd like to merge it.

No, it doesn't - this PR removes the file name from the popup which simply isn't good and would have to be fixed by any subsequent PR. Also, modifying tm_parser_format_variable() means the scope will also be shown in the tooltips of the symbols in the sidebar which IMO adds unnecessary clutter as this info is visible from the symbol tree itself.

Rather than this PR, even though @b4n maybe thinks otherwise, it would be better to apply #3495 (just ellipsizing in a smarter way) and avoid the configuration altogether.

I can make it configurable.

Would you wait for a few days? I'd like to play with this a bit and see what works and what not.

@techee techee mentioned this pull request Aug 23, 2023
@b4n
Copy link
Member

b4n commented Oct 10, 2023

Closing now #3547 has been merged. Feel free to open further adjustments again 😉

@b4n b4n closed this Oct 10, 2023
@techee
Copy link
Member

techee commented Oct 10, 2023

Closing now #3547 has been merged. Feel free to open further adjustments again 😉

As a last minute patch just before the release :-).

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

Successfully merging this pull request may close these issues.

None yet

5 participants