Skip to content

Make Go to Symbol commands show symbol type/signature in list #3475

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

Merged
merged 1 commit into from
May 8, 2023

Conversation

ntrel
Copy link
Member

@ntrel ntrel commented Apr 27, 2023

(Shown when there is more than one matching symbol).
For variables, show type.
For callables, show signature.
Show non-first item using italic file:line:.

When there is more than one matching symbol.
For variables, show type.
For callables, show (at least) parameter list.
Show non-first item using italic `file:line:`.
@ntrel ntrel merged commit 5466599 into geany:master May 8, 2023
@ntrel ntrel deleted the goto-tag-sig branch May 8, 2023 11:44
@ntrel ntrel added this to the 1.39/2.0 milestone May 8, 2023
@b4n
Copy link
Member

b4n commented May 9, 2023

I'm late to complain, but using this for a couple hours I find it a bit annoying with long C++ signatures. I got many cases where the popup now spans most of my screen width, making the thing a bit hard to use quickly. Also, although useful, the signature is so big and prominent that the filename is almost buried and harder to find; and many times I have the same signature because it's just all implementations of an interface, so the signature itself is not that interesting in that case.

I didn't test the tooltip version, but I'd guess I'd like it better, what was the reasons for you to abandon it? Maybe we could find a middle ground :)

big-popup1
big-popup2

@ntrel
Copy link
Member Author

ntrel commented May 9, 2023

Oh sorry. Perhaps I could add a preference:

  • Just file and line with tooltip
  • Just scope and signature with tooltip for file/line
  • Both

Or perhaps limit the length of each item?

BTW I'd like to do something similar for calltips - show a list of overloads rather than having to click the down arrow 10 times to find the one I want. Unless clicking the arrow shows the list and selecting an item collapses it.

@elextr
Copy link
Member

elextr commented May 9, 2023

oooh, @b4n using C++, there is hope yet 😄

I didn't get a chance to try it yet, but it sounded like a good idea, just file:line isn't much help selecting between function overloads.

But I can also see what @b4n is complaining about.

Personally I can't see file:line adds anything if scope and signature are shown so I would just do the second, with the tooltip for edge case situations like the exact same function being implemented more than once (with #ifs selecting).

@ntrel
Copy link
Member Author

ntrel commented May 10, 2023

what was the reasons for you to abandon it?

Because looking for a signature is far more common for me than looking for file and line.

@ntrel
Copy link
Member Author

ntrel commented May 10, 2023

I got many cases where the popup now spans most of my screen width, making the thing a bit hard to use quickly.

I made #3495 to limit the width, does that help?

Also, although useful, the signature is so big and prominent that the filename is almost buried and harder to find

With that pull, the width shouldn't exceed the editor widget, so you can look for the filename on the left of the popup.

@elextr
Copy link
Member

elextr commented May 10, 2023

Whats the point of showing file:line? The goto symbol will go to it, and if the user knows file:line they won't be using goto symbol, what is important is to be able to select the correct version of the symbol to goto.

@b4n
Copy link
Member

b4n commented May 10, 2023

I made #3495 to limit the width, does that help?

I'll give it a try, thanks

Whats the point of showing file:line? The goto symbol will go to it, and if the user knows file:line they won't be using goto symbol, what is important is to be able to select the correct version of the symbol to goto.

Well, I myself very often use the goto feature as a quick way to go where I want, even if I know the file (yet, usually not the line). And most of the time I do know the file I want, actually more often than the signature (as again, very often it's interface implementations rather than overloads, so the signature is the same -- yet, the namespace/class is usually not)

@elextr
Copy link
Member

elextr commented May 10, 2023

actually more often than the signature (as again, very often it's interface implementations rather than overloads, so the signature is the same

Well, goto declaration vs goto definition is supposed to separate the interface from the implementation, but I havn't actually evaluated how well that happens.

@b4n
Copy link
Member

b4n commented May 10, 2023

@elextr it mostly does, but that's not my point. My point is that if you have an "interface" implemented by several classes:

class Iface {
public:
  virtual bool maybeDoSomething(int n) = 0;
};

class Foo : public Iface {
public:
  virtual bool maybeDoSomething(int n) override { return n == 42; }
};

class Bar : public Iface {
public:
  virtual bool maybeDoSomething(int n) override { return (n % 42) == 0; }
};

Having the signature is fairly irrelevant: it's gonna always be the same. What's interesting is the symbol name and scope, and as that'll usually be spread in different files, the source file isn't useless.

Having the signature is useful when having overloads, not really for overrides.

@b4n
Copy link
Member

b4n commented May 10, 2023

BTW, this is giving weird results when going to anything that doesn't have a tooltip (e.g. anything that doesn't look like a callable), e.g. a type. Especially weird when called on a class name, as it'll look something like this, mixing names and nothing:

  • foo.hxx:42:
  • foo.hxx:44: ns::Foo::Foo ()
  • foo.hxx:44: ns::Foo::Foo (const Foo &foo)

There should at least always be the fully qualified name on the right if we want to display something.

@elextr
Copy link
Member

elextr commented May 10, 2023

Ahh, what is shown as the signature of the two implementations in in your example, I don't have the PR applied?

I would have thought they would be bool Foo::maybedosomething(int n) and bool Bar::maybedosomething(int n) not the same?

@b4n
Copy link
Member

b4n commented May 10, 2023

I would have thought they would be bool Foo::maybedosomething(int n) and bool Bar::maybedosomething(int n) not the same?

It wouldn't, that's what I wrote (emphasis mine):

Having the signature is fairly irrelevant: it's gonna always be the same. What's interesting is the symbol name and scope, and as that'll usually be spread in different files, the source file isn't useless.

@b4n
Copy link
Member

b4n commented May 10, 2023

Anyway, I would have to try and use it without the file and line and see if I can adapt to it, so I can have a better idea of whether the file really is useful or just a habit I have.

@elextr
Copy link
Member

elextr commented May 10, 2023

Maybe we have a different understanding of "signature", in my mind its the whole type, return (remember an override can be any covariant type, its not necessarily exactly the same as the Iface:: declaration), qualifying scopes, name, and parameters, but IIUC you are using "signature" above just for the parameters?

Anyway, I would have to try and use it without the file and line and see if I can adapt to it, so I can have a better idea of whether the file really is useful or just a habit I have.

I mean, I'm not against having file:line, and @ntrel suggested having it as a tooltip IIRC, I'm just looking at ways to shorten the selection box since your image in the OP is indeed hard to use.

@b4n
Copy link
Member

b4n commented May 10, 2023

Maybe we have a different understanding of "signature", in my mind its the whole type, return (remember an override can be any covariant type, its not necessarily exactly the same as the Iface:: declaration), qualifying scopes, name, and parameters, but IIUC you are using "signature" above just for the parameters?

I meant return type and parameter list. But it doesn't matter much if we understand each other now :)

Anyway, I've been playing with this a bit to see how it feels, and I'm trying this now, not hating it yet (not saying that'll last 🙂):
goto-popup-v2-1
goto-popup-v2-2

One improvement would be using the editor font itself so it looks more natural, and that shouldn't be too hard (either manually specifying the font in the markup or with a tad of CSS)

this is the current diff:

diff --git a/src/symbols.c b/src/symbols.c
index 58451166e..1de3968d9 100644
--- a/src/symbols.c
+++ b/src/symbols.c
@@ -486,7 +486,7 @@ static void hide_empty_rows(GtkTreeStore *store)
 }
 
 
-static const gchar *get_symbol_name(GeanyDocument *doc, const TMTag *tag, gboolean found_parent)
+static const gchar *get_symbol_name(GeanyDocument *doc, const TMTag *tag, gboolean found_parent, gboolean include_line)
 {
 	gchar *utf8_name;
 	const gchar *scope = tag->scope;
@@ -530,7 +530,8 @@ static const gchar *get_symbol_name(GeanyDocument *doc, const TMTag *tag, gboole
 	if (! doc_is_utf8)
 		g_free(utf8_name);
 
-	g_string_append_printf(buffer, " [%lu]", tag->line);
+	if (include_line)
+		g_string_append_printf(buffer, " [%lu]", tag->line);
 
 	return buffer->str;
 }
@@ -949,7 +950,7 @@ static void update_tree_tags(GeanyDocument *doc, GList **tags)
 
 					/* only update fields that (can) have changed (name that holds line
 					 * number, tooltip, and the tag itself) */
-					name = get_symbol_name(doc, found, parent_name != NULL);
+					name = get_symbol_name(doc, found, parent_name != NULL, TRUE);
 					tooltip = get_symbol_tooltip(doc, found);
 					gtk_tree_store_set(store, &iter,
 							SYMBOLS_COLUMN_NAME, name,
@@ -1006,7 +1007,7 @@ static void update_tree_tags(GeanyDocument *doc, GList **tags)
 			expand = ! gtk_tree_model_iter_has_child(model, parent);
 
 			/* insert the new element */
-			name = get_symbol_name(doc, tag, parent_name != NULL);
+			name = get_symbol_name(doc, tag, parent_name != NULL, TRUE);
 			tooltip = get_symbol_tooltip(doc, tag);
 			gtk_tree_store_insert_with_values(store, &iter, parent, 0,
 					SYMBOLS_COLUMN_NAME, name,
@@ -1509,20 +1510,28 @@ static void show_goto_popup(GeanyDocument *doc, GPtrArray *tags, gboolean have_b
 		GtkWidget *image;
 		gchar *fname = short_names[i];
 		gchar *text;
+		gchar *tooltip;
 		gchar *sym = get_symbol_tooltip(doc, tmtag);
 
 		if (!sym)
-			sym = g_strdup("");
-		if (! first && have_best)
+			sym = g_strdup(get_symbol_name(doc, tmtag, FALSE, FALSE));
+		if (!sym)
 			/* For translators: it's the filename and line number of a symbol in the goto-symbol popup menu */
-			text = g_markup_printf_escaped(_("<b>%s:%lu:</b> %s"), fname, tmtag->line, sym);
+			sym = g_strdup_printf(_("%s: %lu"), fname, tmtag->line);
+		if (! first && have_best)
+			/* For translators: it's the symbol in the goto-symbol popup menu */
+			text = g_markup_printf_escaped(_("<b><tt>%s</tt></b>"), sym);
 		else
 			/* For translators: it's the filename and line number of a symbol in the goto-symbol popup menu */
-			text = g_markup_printf_escaped(_("<i>%s:%lu:</i> %s"), fname, tmtag->line, sym);
+			text = g_markup_printf_escaped(_("<tt>%s</tt>"), sym);
+		/* For translators: it's the tooltip for a symbol in the goto-symbol popup menu */
+		tooltip = g_markup_printf_escaped(_("<b>%s:%lu:</b> <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, NULL);
+		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);
 		g_signal_connect_data(item, "activate", G_CALLBACK(on_goto_popup_item_activate),
 		                      tm_tag_ref(tmtag), (GClosureNotify) tm_tag_unref, 0);
@@ -1531,6 +1540,7 @@ static void show_goto_popup(GeanyDocument *doc, GPtrArray *tags, gboolean have_b
 		if (! first)
 			first = item;
 
+		g_free(tooltip);
 		g_free(text);
 		g_free(fname);
 	}

@techee
Copy link
Member

techee commented Aug 12, 2023

I just noticed this problem and I personally also find it a little too verbose and harder to use than the previous simple filename: line_number.

@ntrel ntrel mentioned this pull request Aug 13, 2023
@ntrel
Copy link
Member Author

ntrel commented Aug 13, 2023

Sorry for the long delay. I submitted Colomban's code as a pull request.

@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

I meant to post the above to #3542 so I reposted it there too.

techee pushed a commit to techee/geany that referenced this pull request Oct 4, 2023
When there is more than one matching symbol.
For variables, show type.
For callables, show (at least) parameter list.
Show non-first item using italic `file:line:`.
@techee techee mentioned this pull request Oct 8, 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

Successfully merging this pull request may close these issues.

4 participants