@b4n requested changes on this pull request.
I'm not against a way to better discriminate between similar-looking paths, but
I think it needs more work not to be as specific to your use case where the
parent's directory name is enough.
Also, please split changes into individual commits (e.g. the change to window
size seem unrelated), and make sure the rationale behind each change is clear
-- for example, the one behind the window size isn't clear to me.
> - gchar *label = g_markup_printf_escaped ("<big>%s</big>", item_label);
+ gchar *basename = g_path_get_basename (item_label);
+ gchar *dirname = g_path_get_dirname (item_label);
+ gchar *dirname_basename = g_path_get_basename(dirname);
+ gchar *label;
+
+ if (g_strcmp0(".", dirname_basename) == 0) {
+ label = g_markup_printf_escaped ("<big>%s</big>", basename);
+ } else {
+ label = g_markup_printf_escaped ("<big>%s/%s</big>",
dirname_basename, basename);
+ }
This seems overly fragile to me, as menu item labels can contain pretty much
anything, including `/`s.
Take for example *Edit → Insert Date → yyyy/mm/dd*: with this change, it
appears as *mm/dd*.
> + gchar *dirname = g_path_get_dirname (DOC_FILENAME (documents[i]));
+ gchar *dirname_basename = g_path_get_basename (dirname);
+ gchar *label = g_markup_printf_escaped ("<big>%s/%s</big>\n"
"<small><i>%s</i></small>",
+ dirname_basename,
Here it's not as problematic as those *are* paths. However, the dirname is not
necessarily helping much as it might very well still be the same (e.g. if there
are `foo/src/main.c` and `bar/src/main.c`, this change won't help).
If this is really an issue you encounter, I suggest investigating a more clever
solution, possibly verifying there are effectively duplicates, and then compute
the difference between the paths in order to show parts that are actually
relevant. IIRC there's something like that in Geany's *got to* popup you might
get inspiration from.
Once that's dealt with I wonder what the best visual layout would be… maybe
`<big>$basename</big>
<small>($path_diff)</small>\n<small><i>$path</i></small>`? Not tested though,
but I think preserving focus on the filename, or actually what's really
relevant in it, is important. So maybe even highlight the different parts?
> + if (window_width >= 500) {
+ window_width = window_width / 2;
+ }
Is 500 really too big? Otherwise if the goal is to make it *bigger*, then the
threshold should be whatever makes it larger than 500 (math suggests it'd be
1000).
Also note that this is *not* dynamic during a Geany session, it's only the
default size when opening the panel for the first time, if you change the Geany
window size later it won't affect this. This is not necessarily an issue
though.
> @@ -804,4 +828,4 @@ void
plugin_help (void)
{
utils_open_browser (DOCDIR "/" PLUGIN "/README");
-}
+}
Please bring the trailing newline back :slightly_smiling_face:
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1394#pullrequestreview-2594255868
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1394/review/2594255...@github.com>
