It is very hard to debug if the proxy plugin happens to give extensions starting with a dot. If they do, detect this case, warn in the debug messages, and fix the extension. It seems unlikely that files with two dot extensions will be desired, so it should case no harm.
Note that it is documented to provide extensions without the dots, but it's easy to miss that one place (and in the proxy howto). IMO, we should at least print a warning for this case if not also fix the extension like in this PR. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1212
-- Commit Summary --
* Gracefully handle proxies registering invalid extensions
-- File Changes --
M src/plugins.c (14)
-- Patch Links --
https://github.com/geany/geany/pull/1212.patch https://github.com/geany/geany/pull/1212.diff
@@ -1963,6 +1963,18 @@ static void pm_show_dialog(GtkMenuItem *menuitem, gpointer user_data) }
+static const gchar *fix_extension(const gchar *ext) +{
- if (*ext == '.')
- {
g_warning(_("Proxy plugin extension '%s' starts with a dot, ""stripping. Please fix your proxy plugin."), ext);
would be nice to include the plugin name somewhere
Sounds reasonable
I'd prefer to error out (perhaps we need the dot in the future?) but I don't feel strong.
@@ -1963,6 +1963,18 @@ static void pm_show_dialog(GtkMenuItem *menuitem, gpointer user_data) }
+static const gchar *fix_extension(const gchar *ext) +{
- if (*ext == '.')
- {
g_warning(_("Proxy plugin extension '%s' starts with a dot, ""stripping. Please fix your proxy plugin."), ext);
indeed
I'd prefer to error out (perhaps we need the dot in the future?) but I don't feel strong.
I personally don't feel strong about the specifics either, just that it should give some help.
Mostly I made this PR out frustration after spending an hour or so scratching my head trying to understand why my proxy plugin hooks weren't being called, sprinkling `printf`s all over my code and deep inside the bowels of Geany to solve it (yeah, I know I should've used my debugger). I simply just missed the mention in the API reference that said it doesn't include the dots. To my mind the dot is part of the extension, so I didn't even suspect that could be the problem until I really started pinning down the program flow.
(and in the Proxy Howto).
Certainly if its a requirement its should be in the documentation, not the howto. If you don't want to fix that here can you make an issue pointing it out.
I meant "and" like "in addition to the reference docs". It's mentioned in the `@param` doc for the `extensions` argument.
Ok, I read "that one place" to be in the HOWTO :)
@codebrainz Can you make the change if you don't feel strong either? The documentation is clear so we don't have to be forgiving.
Merged #1212.
@codebrainz Can you make the change if you don't feel strong either? The documentation is clear so we don't have to be forgiving.
If you feel strongly, go ahead and add a commit to change it, I will support it.
Meta point: I kinda have to agree with @kugel- that his remark should have been addressed on way or another before merging, especially as it looks like a fairly easy change. Sure one can further improve things later, but it kind of misses the point of reviewing then; and requires a fairly higher involvement so is likely to lead to not happen even for a very simple fix, leading to the potentially worth remark to just be thrown away.
I kinda have to agree with @kugel- that his remark should have been addressed on way or another before merging, especially as it looks like a fairly easy change.
He said he didn't feel strongly, and obviously I implemented it this way so I at least feel more strongly. But point taken.
So do you want me to change it so it does a hard crash of Geany when a string-based plugin typo is detected?
He said he didn't feel strongly, and obviously I implemented it this way so I at least feel more strongly. But point taken.
I'm not really saying either way is better per se, but you said you didn't care much, and he asked you to change it (https://github.com/geany/geany/pull/1212#issuecomment-245685550). As the discussion reads from here, it *looks* like you ignored it. Same with showing the plugin name in the warning (https://github.com/geany/geany/pull/1212#r77622584) -- probably a mere forgetfulness, though.
I'm not trying to blame or try and stop people from going ahead with merges, just trying to smooth things out and get everyone happy :)
So do you want me to change it so it does a hard crash of Geany when a string-based plugin typo is detected?
Hum, I didn't think so no, rather just warn and leave the plugin not working as expected, like in #1233, which see.
As the discussion reads from here, it looks like you ignored it.
I kind of did ignore it, because as I understood what he meant was to use `g_error()` or alike and crash Geany, which sounded kind of harsh/insane. This was possibly a misunderstanding on my part though. Since it was such a trivial patch and the number of people who will encounter this code path so small, I figured I'd just make an "executive decision" so to speak.
Same with showing the plugin name in the warning (#1212 (comment)) -- probably a mere forgetfulness, though.
That was indeed forgetfulness (or rather misreading), I missed your comment when reviewing the comments before merging.
I kind of did ignore it, because as I understood what he meant was to use `g_error()` or alike and crash Geany, which sounded kind of harsh/insane. This was possibly a misunderstanding on my part though.
Fair point. Maybe a more clear comment on why you thought it was a bad idea, or why @kugel- thought it was could have made things clearer. But well, it's harder to see how other will understand us, that's for sure.
Since it was such a trivial patch and the number of people who will encounter this code path so small, I figured I'd just make an "executive decision" so to speak.
Which does make sense, otherwise if nobody "feels strongly" nothing ever happens.
github-comments@lists.geany.org