<p><b>@b4n</b> requested changes on this pull request.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1445#discussion_r234156034">src/symbols.c</a>:</p>
<pre style='color:#555'>> @@ -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;
</pre>
<p><code>p</code> is not used anywhere.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1445#discussion_r234158887">src/utils.c</a>:</p>
<pre style='color:#555'>> @@ -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.
</pre>
<p>GLib APIs generally use <code>-1</code> to mean this, and although it has the problem of unsigned here (thus would "bug" on a <code>G_SIZE_MAX</code>-long input not <code>NULL</code>-terminated) it has the nice property of allowing 0 as a normal value which is often handy because then the <code>strv</code> wouldn't be used at all.<br>
It's currently private so I don't mind much though.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1445#discussion_r234159459">src/utils.c</a>:</p>
<pre style='color:#555'>> + * @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++)
</pre>
<p><code>guint</code> or <code>gsize</code> here.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1445#discussion_r234159635">src/utils.c</a>:</p>
<pre style='color:#555'>> + 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;
</pre>
<p>What about avoiding the initial copy and simply do:</p>
<div class="highlight highlight-source-c"><pre> <span class="pl-k">for</span> (gsize i = <span class="pl-c1">0</span>; strv[<span class="pl-c1">0</span>][i]; i++)
{
<span class="pl-k">for</span> (gsize j = <span class="pl-c1">1</span>; j < num; j++)
{
<span class="pl-k">if</span> (strv[j][i] != strv[<span class="pl-c1">0</span>][i])
{
<span class="pl-c"><span class="pl-c">/*</span> return prefix on first mismatch <span class="pl-c">*/</span></span>
<span class="pl-k">return</span> <span class="pl-c1">g_strndup</span>(strv[<span class="pl-c1">0</span>], i);
}
}
}
<span class="pl-k">return</span> g_strdup(strv[<span class="pl-c1">0</span>]);</pre></div>
<p>?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1445#discussion_r234160348">src/utils.c</a>:</p>
<pre style='color:#555'>> +/** 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)
</pre>
<p>OK then</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1445#discussion_r234164440">src/utils.c</a>:</p>
<pre style='color:#555'>> + 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.
</pre>
<p>Same comment for the value meaning "compute it". It's especially problematic here as you annotated <code>file_names</code> with <code>array_length=num</code>; so this would suggest a caller could do:</p>
<div class="highlight highlight-source-c"><pre>gsize array_len = <span class="pl-c1">0</span>;
gchar **array = g_malloc(<span class="pl-k">sizeof</span> *array * array_len);
gchar **<span class="pl-k">short</span> = utils_strv_shorten_file_list(array, array_len);</pre></div>
<p>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 <a href="https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strv-length" rel="nofollow"><code>g_strv_length()</code> does not allow a <code>NULL</code> parameter</a>.<br>
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.</p>
<p>IMO, it'd be a lot easier to use <code>-1</code> as the special value like many other functions around. I'd be fine for it to sill be a <code>gsize</code> and then asking for a <code>(gsize) -1</code> value for the default.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1445#discussion_r234165293">src/utils.c</a>:</p>
<pre style='color:#555'>> +
+ /* 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);
</pre>
<p>As mentioned (not clearly, I agree) in a comment, IIUC this doesn't provide full Windows support, because IIUC Windows allows both <code>/</code> and <code>\\</code> as path separators.<br>
I guess we'll most likely get <code>\\</code>-separated paths, but I'm not sure. Also being an API function suggests it's not specific for a particular subset of paths.</p>
<p>The same comment applies thorough the function.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1445#discussion_r234168851">src/symbols.c</a>:</p>
<pre style='color:#555'>> menu = gtk_menu_new();
+ /* If popup would show multiple files presend a smart file list that allows
</pre>
<p><a class="user-mention" data-hovercard-type="user" data-hovercard-url="/hovercards?user_id=811085" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/elextr">@elextr</a> seems to indeed have spotted a small typo :)</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/geany/geany/pull/1445#pullrequestreview-175739285">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABDrJ1BhjdRyd6Eq0Gv7tafbPJSwlL1Fks5uvpzogaJpZM4MnYrc">mute the thread</a>.<img src="https://github.com/notifications/beacon/ABDrJ_lt0bx1nDECC1_Rk4Y5GeaI_AzEks5uvpzogaJpZM4MnYrc.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/geany/geany","title":"geany/geany","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/geany/geany"}},"updates":{"snippets":[{"icon":"PERSON","message":"@b4n requested changes on #1445"}],"action":{"name":"View Pull Request","url":"https://github.com/geany/geany/pull/1445#pullrequestreview-175739285"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/geany/geany/pull/1445#pullrequestreview-175739285",
"url": "https://github.com/geany/geany/pull/1445#pullrequestreview-175739285",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
},
{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"hideOriginalBody": "false",
"originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB",
"title": "@b4n requested changes on 1445",
"sections": [
{
"text": "",
"activityTitle": "**Colomban Wendling**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@b4n",
"facts": [
]
}
],
"potentialAction": [
{
"targets": [
{
"os": "default",
"uri": "https://github.com/geany/geany/pull/1445#pullrequestreview-175739285"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 211651292\n}"
}
],
"themeColor": "26292E"
}
]</script>