Improve tag-goto popup #3547
Improve tag-goto popup #3547
Conversation
1. Show signature for all tag types using get_symbol_name() when get_symbol_tooltip() returns NULL. Modify get_symbol_name() to drop line number when needed. (More or less taken over the complete implementation from Colomban) 2. Add scope information to the signature both when using get_symbol_name() and get_symbol_tooltip(). (Based on Nick's idea) 3. Truncate the length of the popup to at most 80 characters (More or less taken over the complete implementation from Nick) 4. Improve formatting of entries in the popup so individual pieces of information are easier to distinguish: - file name and line number are always in italics - the following signature is in small monospaced font 5. Add tooltip to every entry (based on Nick's implementation). - split the tooltip into two lines - the first line shows the file and the line number, the second line the signature - both of the lines are formatted in the same way as described in (4) (Thanks to Nick Treleaven and Colomban Wendling for the original implementation.)
|
First thoughts
I might have a chance to actually try it this weekend and will comment further. Footnotes
|
This is done using HTML tags like
Yeah, basically the thought was that we want to preserve Geany's behavior (showing path and line number) and truncate the extra part - the signature (of course unless the path is over 80 characters, I'm just not sure how common that is). Users can then get the full signature using the tooltip.
To be sure if it weren't clear - it was just a joke. |
Ahh, ok, I guess it gets more complex to use a full font spec that can be set from a dialog in prefs.
sane? GTK? no chance!!
To be clear the former was a question, did the answer mean fixed width 80 chars? Or is it fixed width user settable, default 80?
But as @kugel- pointed out, they are slow.
Smiley was noted, probably the response was too serious. |
It's fixed 80 now. But if it started truncating paths, we could also get the
Yes, but his (and mine as well) problem was that you'd have to wait for the calltip for the path which should be always displayed now. Or in other words, I tried to make the popup behave in a backwards-compatible way - you should always see what you saw in the previous Geany releases and it's the extra part could possibly be truncated. |
Ok, neat. |
|
I've just added the 2.0 milestone to this issue as I think it should be fixed some way. I don't insist it should be done exactly the way this patch proposes - some other variant is of course possible too. Or we could revert back to the original behavior. |
|
@ntrel Any opinion on this PR and how it behaves? Does it look acceptable or would you prefer something else? |
|
@techee I haven't looked at the code, but the screenshot and description look fine. Much better than just showing file and line like the last release and before. |
OK, good to hear, thanks. |
|
First thoughts:
This is not to say there's nothing to like here, but I think it needs improving. diffdiff --git a/src/symbols.c b/src/symbols.c
index 1ee559209..9a25922ec 100644
--- a/src/symbols.c
+++ b/src/symbols.c
@@ -1490,6 +1490,7 @@ static void show_goto_popup(GeanyDocument *doc, GPtrArray *tags, gboolean have_b
{
GtkWidget *first = NULL;
GtkWidget *menu;
+ GtkSizeGroup *group = gtk_size_group_new(GTK_SIZE_GROUP_HORIZONTAL);
GdkEvent *event;
GdkEventButton *button_event = NULL;
TMTag *tmtag;
@@ -1509,6 +1510,7 @@ static void show_goto_popup(GeanyDocument *doc, GPtrArray *tags, gboolean have_b
{
GtkWidget *item;
GtkWidget *label;
+ GtkWidget *box;
GtkWidget *image;
gchar *fname = short_names[i];
gchar *text;
@@ -1522,20 +1524,25 @@ static void show_goto_popup(GeanyDocument *doc, GPtrArray *tags, gboolean have_b
if (! first && have_best)
/* For translators: it's the filename, line number, and signature of a symbol in the goto-symbol popup menu */
- text = g_markup_printf_escaped(_("<i><b>%s:%lu</b></i><small><tt> %s</tt></small>"), fname, tmtag->line, sym);
+ text = g_markup_printf_escaped(_("<b>%s:%lu</b>"), fname, tmtag->line);
else
/* For translators: it's the filename, line number, and signature of a symbol in the goto-symbol popup menu */
- text = g_markup_printf_escaped(_("<i>%s:%lu</i><small><tt> %s</tt></small>"), fname, tmtag->line, sym);
+ text = g_markup_printf_escaped(_("%s:%lu"), fname, tmtag->line);
/* For translators: it's the filename, line number, and signature of a symbol in the goto-symbol popup menu */
- tooltip = g_markup_printf_escaped(_("<i>%s:%lu</i>\n<small><tt>%s</tt></small>"), fname, tmtag->line, sym);
+ tooltip = g_markup_printf_escaped(_("%s:%lu\n<small><tt>%s</tt></small>"), fname, tmtag->line, sym);
- g_free(sym);
image = gtk_image_new_from_pixbuf(symbols_icons[get_tag_class(tmtag)].pixbuf);
- label = g_object_new(GTK_TYPE_LABEL, "label", text, "use-markup", TRUE, "xalign", 0.0,
- "tooltip-markup", tooltip, "max-width-chars", 80,
- "ellipsize", PANGO_ELLIPSIZE_END, NULL);
- item = g_object_new(GTK_TYPE_IMAGE_MENU_ITEM, "image", image, "child", label, "always-show-image", TRUE, NULL);
+ box = g_object_new(GTK_TYPE_BOX, "orientation", GTK_ORIENTATION_HORIZONTAL, "spacing", 12, NULL);
+ label = g_object_new(GTK_TYPE_LABEL, "label", text, "use-markup", TRUE, "xalign", 0.0, NULL);
+ gtk_size_group_add_widget(group, label);
+ gtk_box_pack_start(GTK_BOX(box), label, FALSE, FALSE, 0);
+ g_free(text);
+ text = g_markup_printf_escaped("<small><tt>%s</tt></small>", sym);
+ gtk_box_pack_start(GTK_BOX(box), g_object_new(GTK_TYPE_LABEL, "label", text, "use-markup", TRUE, "xalign", 0.0,
+ "max-width-chars", 80, "ellipsize", PANGO_ELLIPSIZE_END, NULL), FALSE, FALSE, 0);
+ item = g_object_new(GTK_TYPE_IMAGE_MENU_ITEM, "image", image, "child", box, "always-show-image", TRUE,
+ "tooltip-markup", tooltip, NULL);
g_signal_connect_data(item, "activate", G_CALLBACK(on_goto_popup_item_activate),
tm_tag_ref(tmtag), (GClosureNotify) tm_tag_unref, 0);
gtk_menu_shell_append(GTK_MENU_SHELL(menu), item);
@@ -1543,6 +1550,7 @@ static void show_goto_popup(GeanyDocument *doc, GPtrArray *tags, gboolean have_b
if (! first)
first = item;
+ g_free(sym);
g_free(text);
g_free(fname);
}
@@ -1563,6 +1571,8 @@ static void show_goto_popup(GeanyDocument *doc, GPtrArray *tags, gboolean have_b
button_event ? (GDestroyNotify) gdk_event_free : NULL);
ui_menu_popup(GTK_MENU(menu), goto_popup_position_func, doc->editor->sci,
button_event ? button_event->button : 0, gtk_get_current_event_time ());
+
+ g_object_unref(group);
}
|
|
PS: this has string changes, not sure how bad that is for release… but I think this PR might be worth sorting out anyway. |
I didn't really want them, I just tried to unify all the rows. And yes, I agree that the non-italics version looks better.
My version was just a poor man's attempt to improve the everyting-in-a-single-text-blob version, yours is definitely better.
The disadvantage of your version is that the popup could get much bigger in some extreme cases but I think it's acceptable. (Just a shameless promotion - with my LSP plugin the clangd server seems to always return the "right" symbol based on its true visibility from the given position so no popup like that seems to be necessary.) So yeah, I definitely like how it looks like. |
Yes, I am aware of that and I actually talked with @frlan about it today in person at Prague Linux Days. And he wasn't too serious about the string freeze in this case. I'm actually not sure if these strings should be translatable - is it useful for anything? I thought was meant for RTL languages but for these you might want to swap filename and symbol name which you can't do. So shouldn't these be just normal strings? If we want to keep the translatable strings, we should also update the "For translators" comments to reflect the changes. |
Yeah, if we want we can also limit the symbol side under 80 if it's a concern -- although again, if there isn't enough info it can stop being useful.
There is a LSP plugin for Geany? Nice, I should give that a try :)
One thing it was sued for was to use proper punctuation in some languages. For example in French we like to put a NBSP1 before e.g. a colon, so it used to be I'm not very knowledgeable about RTL, but you probably don't mind, as it's reversed on screen as well, so you probably want the string in the same order… or it'd be a very common issue. Yet, you should be able to get away with the GNU Footnotes
|
See https://github.com/techee/geany-lsp and #3571 |
You took the words right out my mouth, thats what happens with Vscode using LSP (assuming the build is right), no popup. That precision also means Vscode can afford a multi-line calltip on hover to show the declaration since its only showing one result (a feature I'm sure will be available with the LSP :-). So I'm thinking this shouldn't be rushed into 2.0, it certainly is still needed until LSP is more widely available, but its not a bug that needs fixing NOW!!
I'm not sure what strings @b4n was talking about, but if its the code strings shown in the popup then the advice we got from a multi-linguistic translation service back when I worked for a big (French) company was that code and similar printouts should match the code, it is not human language, it cannot be translated. |
The problem is that that now it looks this way: and this is a serious regression in terms of usability of this feature. I'm totally fine if it gets back where it was in the previous Geany release, i.e. reverting but this should be addressed somehow. |
What I mean is, yes its very ugly1, and that makes it hard for humans to use, but it doesn't crash or provide wrong information. We seem to have concentrated on C++ (probably my fault) but its likely to be useful for simple situations (ie not C++ :-) I would expect that for languages without overloading like C there will only be a few in the popup when multiple versions of the same file are open, and then the file/line and signature is likely to be useful as @kugel- said. So how bad is it for non-C++ languages, say C and Python, thats one non-overloading and one overloading? That should decide if we need to revert #3475, if it works ok for most languages my vote would be leave it and improve it from there. At this point we don't seem to have an agreed PR, and jamming something that cannot be well tested into 2.0 is a risky way to do it, that just leads to 2.0.1 (ask @eht16). Footnotes
|
This makes the signatures separated visually and easier to read overall. Patch provided by Colomban Wendling, thanks!
|
Regardless of whether this PR gets used for the next release, I just tested the Colomban's code and added his patch to this PR (I just updated the "For translators" strings). It works and looks great IMO and my personal vote would be to use this patch for the next release. |
|
I'll test it a bit but I quite like it at the moment. And it basically adds a new column from what I think I remember was the previous release (if I can still remember), so it's almost less different, yet adding the extra info @ntrel wanted, and that I grew to like as well. I agree that it could use more testing but I have been using a not-that-different version for a bit, and I think it's a fairly important thing to sort out for the release, so I'd vote for integrating it if nobody has complains about it. As for translatable strings, I believe that what's in the latest version here doesn't matter in that regard, the only thing left is |
|
Works (with 10 mins testing) and aligning the signatures makes file:line much easier to distinguish from the signature. And code looks ok by quick inspection except the translation comments. |
|
Should I somehow squash the commits or is it OK to merge it as it is? |
|
@techee I don't think it matters much here (but maybe the "all all"? 😁), but it could also be one single commit (basically your first one already combines various things together) |
|
(Merging as it is to avoid complicating things further.) |
OK, so here's my take on #3542. I realized that I actually liked most of the stuff, it was just a matter of different presentation and some implementation details.
I think from the discussion in #3542 it was clear that most people wanted to see the path and line number and to to be able to distinguish it from the signature easily - the core thing was to use a different font for the file name (italics) and for the signature (small monospaced font). For the tooltip, I just separated these two by a newline.
In more details, this is what I did:
Show signature for all tag types using get_symbol_name() when get_symbol_tooltip() returns NULL. Modify get_symbol_name() to drop line number when needed. (More or less taken over the complete implementation from Colomban)
Add scope information to the signature both when using get_symbol_name() and get_symbol_tooltip(). (Based on Nick's idea)
Truncate the length of the popup to at most 80 characters (More or less taken over the complete implementation from Nick)
Improve formatting of entries in the popup so individual pieces of information are easier to distinguish:
(Thanks to Nick Treleaven and Colomban Wendling for the original implementation.)
@b4n @ntrel @elextr @kugel- What do you think? (Pinging about everyone, this will be one of those features where we'll all have a different opinion and hate each other with passion ;-)
The result looks something like this: