Only warn when an extension starts with a dot, because it's unlikely to be what the author wanted, but leave the dot there. This both forces the author to fix the extension, and allows future use of the dot if wanted.
Also, include the proxy plugin name in the warning message.
Follow-up to #1212. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1233
-- Commit Summary --
* Do not strip dots from proxy plugin extensions
-- File Changes --
M src/plugins.c (20)
-- Patch Links --
https://github.com/geany/geany/pull/1233.patch https://github.com/geany/geany/pull/1233.diff
What are the expected future uses of the dot?
Another approach would be to just remove the warning and allow either dotted or un-dotted extensions to be used, since it's extremely unlikely anyone will intentionally be wanting to handle plugin files in Geany's plugin dir like `theplugin..so`.
In any case, I'm fine with this PR. I actually paste-binned an almost identical patch to @kugel- the other day on IRC when we were discussing this.
What are the expected future uses of the dot?
None from me, but see below.
Another approach would be to just remove the warning and allow either dotted or un-dotted extensions to be used, since it's extremely unlikely anyone will intentionally be wanting to handle plugin files in Geany's plugin dir like `theplugin..so`.
Yes, that would be a valid fix from my POV. I don't really like the current state because it emits a warning only for something that is very easy to fix; so it's easy to forget, while easy to fix. This way, it's likely authors might just not see/ignore the warning because it has absolutely no side effect, and then the dot prefix effectively becomes a part of the API. So, IMO it should be either allowed or disallowed, not transparently "fixed" yet not officially allowing it.
BTW, even with stripping the leading dot unconditionally, someone could very well support `..so` extensions by simply pass `..so` as the extension: one dot is stripped, the other stays, and voila. Well, actually it would require changing how the extension matching is implemented to merely do `str_has_suffix()` otherwise the dot will not be considered, but still.
So, IMO it should be either allowed or disallowed, not transparently "fixed" yet not officially allowing it.
My opinion too, and I prefer disallowing.
So, IMO it should be either allowed or disallowed, not transparently "fixed" yet not officially allowing it.
My opinion too, and I prefer disallowing.
I agree warning and magic fixing is kind of weird. Either always allowing the dot or just a warning and not loading the plugin is fine with me. I'm still curious what future use of the dot was being referenced though, I don't think I understood that possibility, and might have a different opinion if I did.
kugel- commented on this pull request.
@@ -2067,8 +2055,14 @@ gboolean geany_plugin_register_proxy(GeanyPlugin *plugin, const gchar **extensio
foreach_strv(ext, extensions) { + if (**ext == '.') + { + g_warning(_("Proxy plugin '%s' extension '%s' starts with a dot. " + "Please fix your proxy plugin."), p->info.name, *ext); + }
Returning false is a bit involved, since you'd want to roll back adding earlier extensions from the same call.
I'm still curious what future use of the dot was being referenced though, I don't think I understood that possibility, and might have a different opinion if I did.
As said this was a purely theoretical remark, meaning that if we strip it implicitly it becomes part of the API so has to be kept "forever". I have no plan nor ideas why giving the leading not a special meaning would be a good idea.
meaning that if we strip it implicitly it becomes part of the API so has to be kept "forever"
Then we could make it more flexible (as you eluded to) and just do simple suffix-matching. This would support less-common file names/extensions like `myplugin.tar.gz` or `myplugin.GeanyPy.py` or whatever. It wouldn't be a huge change and can probably be done in a backwards-compatible manner.
The alternative more flexible approach mentioned in the previous comment would look something like this (untested, except for compiling): https://github.com/codebrainz/geany/commit/267a1651b9e405674bf762e59a4a1039a...
@codebrainz yeah. That one would break API, though :)
@b4n not really, but it would be worth an API bump to allow proxy plugins to detect the change if they want.
well, as you pointed yourself in the comment, if the plugin doesn't update the suffixes to include the dot, it'll match additional things.
Yeah, it will match `fatso` as well as `fat.so`, but it doesn't change the C API, and only does a minor tweak to the semantics. Also it will only affect an estimated 2 actual proxy plugins known to exist :) But an API version bump is no problem (and accounted for in the comments, but forgotten in the `plugindata.h` file).
Also proxy plugins matching blindly against extensions, without checking the file contents, are basically broken already, since all the plugins are only allowed in a single directory and the likelihood of extension clashes is great.
Actually what's the point of pre-matching by file extension/suffix at all? Why can't the proxy plugin's `probe()` functions do this alone? We could add a `utils_has_filename_suffix()` if all the want to do is suffix-matching.
Guess it saves every proxy probing every file in the single plugins dir.
Guess it saves every proxy probing every file in the single plugins dir.
I wonder if that would be a problem in practice though? Most likely it will be < 50 files and likely no more than a few hundred at the upper limit.
fwiw, peasy's probe() expects to get a filename with a dot in it. It would probably work for "foo.pypy" but if there is no dot at all it would crash.
:bike: :house: :)
It would probably work for "foo.pypy" but if there is no dot at all it would crash.
Sounds like it would be easy to fix.
We should do something for 1.29 I guess, not to have the current ambiguity released.
We should do something for 1.29 I guess, not to have the current ambiguity released.
Also I believe these `PluginProxy` structures are leaked, probably should fix before next release:
https://github.com/geany/geany/blob/1.28.0/src/plugins.c#L2016
It's fixed in #1236
Merged #1233.
github-comments@lists.geany.org