[Github-comments] [geany/geany] Improve goto-symbols popup (#1445)

[Colomban Wendling]

b4n requested changes on this pull request.



> @@ -1927,15 +1927,23 @@ static void show_goto_popup(GeanyDocument *doc, GPtrArray *tags, gboolean have_b
 	GdkEventButton *button_event = NULL;
 	TMTag *tmtag;
 	guint i;
-
+	gchar **short_names, **file_names, **p;

`p` is not used anywhere.

> @@ -2031,6 +2031,209 @@ gchar **utils_strv_join(gchar **first, gchar **second)
 	return strv;
 }
 
+/* * Returns the common prefix in a list of strings.
+ *
+ * The size of the list may be given explicitely, but defaults to @c g_strv_length(strv).
+ *
+ * @param strv The list of strings to process.
+ * @param num The number of strings contained in @a strv. Can be 0 if it's terminated by @c NULL.

GLib APIs generally use `-1` to mean this, and although it has the problem of unsigned here (thus would "bug" on a `G_SIZE_MAX`-long input not `NULL`-terminated) it has the nice property of allowing 0 as a normal value which is often handy because then the `strv` wouldn't be used at all.
It's currently private so I don't mind much though.

> + * @return The common prefix that is part of all strings (maybe empty), or NULL if an empty list
+ *         was passed in.
+ */
+static gchar *utils_strv_find_common_prefix(gchar **strv, gsize num)
+{
+	gchar *prefix;
+
+	if (!NZV(strv))
+		return NULL;
+
+	if (num == 0)
+		num = g_strv_length(strv);
+
+	prefix = g_strdup(strv[0]);
+
+	for (gint i = 0; prefix[i]; i++)

`guint` or `gsize` here.

> +	for (gint i = 0; prefix[i]; i++)
+	{
+		for (gsize j = 1; j < num; j++)
+		{
+			gchar *s = strv[j];
+			if (s[i] != prefix[i])
+			{
+				/* terminate prefix on first mismatch and return */
+				prefix[i] = '\0';
+				break;
+			}
+		}
+		if (prefix[i] == '\0')
+			break;
+	}
+	return prefix;

What about avoiding the initial copy and simply do:
```c
	for (gsize i = 0; strv[0][i]; i++)
	{
		for (gsize j = 1; j < num; j++)
		{
			if (strv[j][i] != strv[0][i])
			{
				/* return prefix on first mismatch */
				return g_strndup(strv[0], i);
			}
		}
	}
	return g_strdup(strv[0]);
```
?

> +/** Transform file names in a list to be shorter.
+ *
+ * This function takes a list of file names (porbably with absolute paths), and
+ * transforms the paths such that they are short but still unique. This is intended
+ * for dialogs which present the file list to the user, where the base name may result
+ * in duplicates (showing the full path might be inappropriate).
+ *
+ * The algorthm strips the common prefix (e-g. the user's home directory) and
+ * replaces the longest common substring with an ellipsis ("...").
+ *
+ * @param file_names @array{length=num} The list of strings to process.
+ * @param num The number of strings contained in @a file_names. Can be 0 if it's terminated by @c NULL.
+ * @return @transfer{full} A newly-allocated array of transformed paths strings, terminated by
+            @c NULL. Use @c g_strfreev() to free it.
+ *
+ * @since 1.31 (API 232)

OK then

> +	return lcs;
+}
+
+
+/** Transform file names in a list to be shorter.
+ *
+ * This function takes a list of file names (probably with absolute paths), and
+ * transforms the paths such that they are short but still unique. This is intended
+ * for dialogs which present the file list to the user, where the base name may result
+ * in duplicates (showing the full path might be inappropriate).
+ *
+ * The algorthm strips the common prefix (e-g. the user's home directory) and
+ * replaces the longest common substring with an ellipsis ("...").
+ *
+ * @param file_names @array{length=num} The list of strings to process.
+ * @param num The number of strings contained in @a file_names. Can be 0 if it's terminated by @c NULL.

Same comment for the value meaning "compute it".  It's especially problematic here as you annotated `file_names` with `array_length=num`; so this would suggest a caller could do:
```c
gsize array_len = 0;
gchar **array = g_malloc(sizeof *array * array_len);
gchar **short = utils_strv_shorten_file_list(array, array_len);
```
This doesn't seem far-fetched for auto-generated code like Vala, and AFAIK there's nothing telling the GIR consumer it's incorrect.  The reason it's a big problem is that [`g_strv_length()` does not allow a `NULL` parameter](https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strv-length).
But I could imagine other reasons why it wouldn't be convenient, like for processing only a part of an already-existing strv: the caller would have to carefully special-case processing only 0 elements not to get unexpected behavior.  Same, it can be handy that 0 leads to no access of the array pointer at all, because it's a fairly innocent case of not initializing any members, as the caller can expect there will be no access -- and then again, have to special-case 0.

IMO, it'd be a lot easier to use `-1` as the special value like many other functions around.  I'd be fine for it to sill be a `gsize` and then asking for a `(gsize) -1` value for the default.

> +
+	/* The return value shall have exactly the same size as the input. If the input is a
+	 * GStrv (last element is NULL), the output will follow suit. */
+	if (!num)
+		num = g_strv_length(file_names);
+	/* Always include a terminating NULL, enables easy freeing with g_strfreev() */
+	names = g_new0(gchar *, num + 1);
+
+	prefix = utils_strv_find_common_prefix(file_names, num);
+	/* First: determine the common prefix, that will be stripped.
+	 * Don't strip single-letter prefixes, such as '/' */
+	prefix_len = 0;
+	if (NZV(prefix) && prefix[1])
+	{
+		/* Only strip directory components, include trailing '/' */
+		start = strrchr(prefix, G_DIR_SEPARATOR);

As mentioned (not clearly, I agree) in a comment, IIUC this doesn't provide full Windows support, because IIUC Windows allows both `/` and `\\` as path separators.
I guess we'll most likely get `\\`-separated paths, but I'm not sure.  Also being an API function suggests it's not specific for a particular subset of paths.

The same comment applies thorough the function.

>  	menu = gtk_menu_new();
 
+	/* If popup would show multiple files presend a smart file list that allows

@elextr seems to indeed have spotted a small typo :)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1445#pullrequestreview-175739285
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20181116/4134b6bf/attachment-0001.html>