Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
Awesome, thanks! -- 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#issuecomment-444275376
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
Yep, thanks :) -- 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#issuecomment-444274695
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
Merged #1445 into master. -- 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#event-2005586108
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
@b4n are we good to go? -- 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#issuecomment-444258483
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
@kugel- See https://github.com/kugel-/geany/pull/6 -- 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#issuecomment-443498042
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
I rebased and imply removed the API bump that was causing the conflict. I hope that's what you wanted? -- 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#issuecomment-443462293
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
@kugel- pushed 1 commit. c599a1e fix inverted logic in assertion -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/geany/geany/pull/1445/files/dbafede20cd3622867b2ec4649b16ee5947f6083..c599a1e7eab887ff2a792fd15eddb8a51d4570ed
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
@kugel- just in case you didn't see my review here like on the other PR. -- 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#issuecomment-443449989
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
b4n commented on this pull request.
> + * @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)
+ */
+GEANY_API_SYMBOL
+gchar **utils_strv_shorten_file_list(gchar **file_names, gssize num)
+{
+ gsize i;
+ gchar *prefix, *substring, *lcs;
+ gchar *start, *end;
+ gchar **names;
+ gsize prefix_len, lcs_len;
+
+ /* We don't dereference file_names if num == 0. */
+ g_return_val_if_fail(num != 0 && file_names == NULL, NULL);
This should be `num == 0 || file_names != NULL`, as it's basically an
assertion, and should list conditions expected to pass.
--
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-178057950
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
``` (geany:25173): Geany-CRITICAL **: utils_strv_shorten_file_list: assertion 'num != 0 && file_names == NULL' failed Erreur de segmentation ``` oops! Just tried to go to a location -- 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#issuecomment-441384267
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
b4n requested changes on this pull request.
Still annoying :)
Otherwise looks good.
You also should resolve the conflict, but apart from that I think we're good do
go.
> @@ -2031,6 +2031,208 @@ 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 -1 if it's
terminated by @c NULL.
+ *
+ * @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, gssize num)
+{
+ gchar *prefix;
unused now
--
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-178057077
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
@kugel- pushed 1 commit. 0789cf2 Changes for review comments -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/geany/geany/pull/1445/files/16540421553139eb248e241118874c527737210f..0789cf2c9cafdaa381e47eb2e025c012ae0d596a
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
kugel- commented on this pull request.
> +
+ /* 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);
> So this might not be so important in the end.
Ok, so I don't plan changes for now in this regard. Speak up if you chnage your
mind.
--
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#discussion_r234442583
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
kugel- commented on this pull request.
> + 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.
strv is very gir friendly, and since the function would decompose a GPtrArray
to handle its raw strv it might as well get the strv directly.
--
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#discussion_r234442526
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
codebrainz commented on this pull request.
> + 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.
Probably a stupid question (I didn't look at the calling code), but couldn't it
use a ready-made collection type like GArray or GPtrArray instead of passing
separate arguments? If the parameter isn't modified and the caller does have
separate pointer and length, they could just stuff them in a GArray on the
stack and pass by const pointer. Or the function could mutate the array in
place for better performance. Would also allow to deduplicate the input array
without handing back an array that has to be counted by walking through it.
Seems cleaner and more efficient than using a plain C array and separate
length, but maybe I'm brainwashed by C++ by now, so feel free to ignore :)
--
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#discussion_r234405043
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
b4n commented on this pull request.
> + 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.
IMO it should not be an error if you allow a non-`NULL`-terminated array,
because as you said yourself, allocating a 0-length array will give you a
`NULL` pointer -- which is fine as you shouldn't be accessing anything past its
0th element.
--
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#discussion_r234402282
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
kugel- commented on this pull request.
> + 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.
I can change it, no problem. FWIW: I'd argue that passing NULL should be an
error (might add an g_return_val_if_fail() since it's an API function).
--
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#discussion_r234341732
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
b4n commented on this pull request. > @@ -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. I'm not sure there is one for strvs, but there are quite some for strings, the first that comes to mind right now is [`g_utf8_strlen()`](https://developer.gnome.org/glib/stable/glib-Unicode-Manipulation.html#g-utf8-strlen) -- 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#discussion_r234337897
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
b4n commented on this pull request.
> + 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.
The thing is that even in that case as said it's a problem for this function,
as it'll pass on a NULL strv to `g_strv_len()`.
But although `malloc()` returning NULL on 0 length is perfectly normal, it's
not my point; the thing is that 0 is a perfectly valid, and not that unlikely
value for a meaningful length (see my other examples).
--
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#discussion_r234337331
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
kugel- commented on this pull request.
> +
+ /* 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);
Meh, what's the point G_DIR_SEPARATOR if you need to handle both. Also, what
about legitimate paths that contain the other slash (e.g. a file can contain
the \ character on linux, not sure if the same is true for / on Windows).
Are you sure we don't have problems with this elsewhere inside Geany? I.e. is
this something we support or not?
--
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#discussion_r234302628
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
kugel- commented on this pull request. > @@ -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. I can change if you prefer. But I couldn't find an example in the glib documentation. -- 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#discussion_r234300945
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
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
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
> Fixing this isn't trivial That's why I suggested in an earlier comment that the fact `utils_strv_find_lcs()` looks generic is cute, but in practice this function does *not* do what `utils_strv_shorten_file_list()` needs. > and without proper unit tests there is a risk to break previously working > cases. Yeah, we should probably add tests for this. Fortunately, it's probably one of the few things that should be easy enough to add a test for, because `utils.c` don't usually have dependencies on internal Geany state, which means it can likely be tested by a minimal test program linking libgeany. > I suggest to accept the behavior for now (and merge) and I'll follow up with > a fix + unit tests. But I would hate if this doesn't make it into the release > just because of select (edge) cases because the status quo is just plain > unusable for me. In fact I've been using this patch at work for ages where I > have real-world cases and I never came across issues like this. Fair enough; I must admit I specifically crafted the test to fail, looking at the weakness of the implementation :) This said, note that I have another implementation (see a message above) that doesn't have the issue. It has arguably worse performance, and can probably use some improvement in other area, but I didn't find a bug in its results yet. -- 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#issuecomment-439360404
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
@kugel- there still are problems shortening some paths, like e.g. */src/a/app-1.2.3/src/lib/module/source.c* and */src/b/app-2.2.3/src/module/source.c*: path | current result | my expectation --||-- `/src/a/app-1.2.3/src/lib/module/source.c` | `a/app-1.2.3/src/lib/module/source.c` | `a/app-1.2.3/src/lib/.../source.c` `/src/b/app-2.2.3/src/module/source.c` | `b/app-2.2.3/src/module/source.c` | `b/app-2.2.3/src/.../source.c` -- 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#issuecomment-439209070
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
> (ellipsizing is clearer when the 3 dots are encosed by words. I've never seen > a ellipsizing that cuts of the entire front). I agree with @kugel- here, and would have expected what @b4n showed as "current result" anyway. -- 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#issuecomment-378428162
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
@b4n friendly ping :-) -- 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#issuecomment-378340497
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
> Looking at the table below, is this expected, or is it a bug? it's debatable. From the plain algorithm, your expectation is right and the result is buggy. But one can argue that the actual result may be less confusing to the user (ellipsizing is clearer when the 3 dots are encosed by words. I've never seen a ellipsizing that cuts of the entire front). > Also, your code doesn't seem to support Windows path separators What do you mean? I used G_DIR_SEPARATOR everywhere. I didn't test on Windows but it should work. -- 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#issuecomment-376006137
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
Also, your code doesn't seem to support Windows path separators. I'm not 100% sure we need it in Geany here, but it would probably be good to have support for it if it's supposed to be a generic utility. -- 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#issuecomment-375792236
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
Looking at the table below, is this expected, or is it a bug? path | current result | what I expected | | `/aaa/bbb/cc/c/d` | `cc/.../d` | `.../d` `/aaa/bbb/xxx/yyy/cc/c/d` | `xxx/yyy/cc/.../d` | `xxx/yyy/.../d` -- 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#issuecomment-375791884
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
I hope the code is clearer now and the bug you mentioned should be fixed -- 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#issuecomment-373435785
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
@kugel- pushed 1 commit. 1654042 Refactoring and review comments -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/geany/geany/pull/1445/files/47f43928054ba848d2e9b2c00daa1a7211bbf44f..16540421553139eb248e241118874c527737210f
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
b4n commented on this pull request.
small stuff I noticed first time I went over, if it can be useful.
Not a complete review.
> +{
+ gchar *prefix, **ptr;
+
+ 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++)
+ {
+ for (gint j = 1; j < num; j++)
+ {
+ gchar *s = strv[j];
`const` would make sense, the string should not be modified
> +
+ if (num == 0)
+ num = g_strv_length(strv);
+
+ /* sub is the working area where substrings from first are copied to */
+ sub = g_malloc(len+1);
+ lcs = g_strdup("");
+ foreach_str(_sub, first)
+ {
+ gsize chars_left = len - (_sub - first);
+ /* No point in continuing if the remainder is too short */
+ if (max > chars_left)
+ break;
+ for (n_chars = 1; n_chars <= chars_left; n_chars++)
+ {
+ /* strlcpy() ftw! */
[`g_strlcpy()`](https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strlcpy)
is there for you if you like.
> + if (prefix[i] == '\0')
+ break;
+ }
+ return prefix;
+}
+
+/* * Returns the longest common substring 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.
+ *
+ * @return The common prefix that is part of all strings.
+ */
+gchar *utils_strv_find_lcs(gchar **strv, size_t num)
this should be static
> +/** 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)
do we need it in the API right away? I mean, why not, but if there's no real
use (and although it seems nice, it is fairly specific still) I'd rather wait a
little.
> +GEANY_API_SYMBOL
+gchar **utils_strv_shorten_file_list(gchar **file_names, size_t num)
+{
+ gint i, j;
+ gchar *prefix, *substring, *name, *sep, **s;
+ TMTag *tmtag;
+ gchar **names;
+ gsize len;
+
+ /* 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_new(gchar *, num + 1);
+ names[num] = 0;
should be `NULL`
--
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-61653071
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
Well, yeah I don't like inefficient code when it can easily be improved in that aspect. OTOH, I prefer easy to read/maintain code. And I'll have to check again, maybe I was tired, but I found your code oddly hard to follow, and tricky to fix the issues I found. At the time I found that working on path components directly made everything simpler at many level. And for the matter of otherwise useful reusable functions, yes, but for example I think I remember one bug is that the longest common substring is *not* necessarily what you want here, like in */a/b/c/d/longilename* vs. */z/b/c/y/longfilename*: *longfilename* is obviously the longest substring, but you actually want */b/c/* -- so IIRC your code will just not shorten this because it'll find *longfilename* as the substring, but will rightly ignore it because it the basename. So to fix this the function should be updated to find the longest substring *surrounded by separators*, becoming a lot less useful for other callers. However yes, if you can fix the issues I'll try and give it a new shot and propose some changes instead of a totally different approach. -- 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#issuecomment-371999526
Re: [Github-comments] [geany/geany] Improve goto-symbols popup (#1445)
@b4n to be honest, I prefer my version. I find it easier to grasp because it's split into 3 strv-related utility functions that have well defined and separated functionality (the functions should be useful on their own). I don't so how your code is really any shorter (and neither is worryingly long, each is well below 200 LOCs with 20-30 of them being comments). And it's more efficient. I'm surprised, as you usually seem care about efficiency? Efficiency is normally not a big deal for me, but I care if the more efficient version is already written (or if the change is trivial and/or obvious but that doesn't apply here). So I would rather fix the bugs you mention and proceed with my version. What do you think? -- 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#issuecomment-370073581
