As a result of discussions in #1233 comments.
Overview: - Allows proxy plugins to register to pre-match "glob-style" patterns for subplugins. - Allows passing `NULL` to register to probe all potential subplugin files (ie. pattern `*`). You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1236
-- Commit Summary --
* Change proxy plugins to match by filename suffix * Change from suffix to Glob-style pattern matching * Allow proxies to pass `NULL` for the `patterns` parameter
-- File Changes --
M src/plugindata.h (4) M src/plugins.c (101)
-- Patch Links --
https://github.com/geany/geany/pull/1236.patch https://github.com/geany/geany/pull/1236.diff
The first two commits should be squished together.
@codebrainz pushed 1 commit.
526b9a4 Update proxy plugins documentation
codebrainz commented on this pull request.
@@ -1344,6 +1347,10 @@ void plugins_init(void)
g_signal_connect(geany_object, "save-settings", G_CALLBACK(update_active_plugins_pref), NULL); stash_group_add_string_vector(group, &active_plugins_pref, "active_plugins", NULL);
+ builtin_proxy_glob = g_strconcat("*.", G_MODULE_SUFFIX, NULL); + builtin_so_proxy.glob = g_pattern_spec_new(builtin_proxy_glob);
This would probably be better like `g_pattern_spec_new("*." G_MODULE_SUFFIX);` rather than building a dynamic string and then freeing it right away.
Seems overkill. What's the use case? Also why do you want to match all plugins?
Seems overkill.
In what way? 3 of the 4 files changed are documentation, and the one file with code changes (`plugins.c`) has the same lines added as removed if you don't count doc-comment lines. Further, some of those lines are to cleanup an existing leak of the `PluginProxy`s as well as 3 more lines that can be eliminated as noted in the inline comment. So in the end, it's actually less code to give enhanced features.
What's the use case?
Read the comments in the linked #1233. Matching files with other extensions like `*.tar.bz2` or `*.geany++.plugin`.
Also why do you want to match all plugins?
Read the comments in the linked #1233. A better question is why do you want to match file extensions at all? They aren't unique enough to decide whether a file is for a given proxy and you have to check them further in the `probe()` function anyway.
Read the comments in the linked #1233. A better question is why do you want to match file extensions at all? They aren't unique enough to decide whether a file is for a given proxy and you have to check them further in the probe() function anyway.
I couldn't find an answer, other than you're thinking it's unnecessary.
The idea was to pre-filter inside so that not every proxy has to inspect every filename, since I expect that any given proxy would at least look at the file extension.
It's true that peasy also looks again at the specific extension, but it can assume the file name has one at all (data files of other plugins could have no extension) and it's one of the registered ones which already helps.
FWIW, I don't oppose this change here, it just seems overkill at this point but I can see that it potentially becomes handy, especially if we get more proxies that handle the same file type (e.g. peasy and geanypy handle *.py, peasy, geany++ and geany itself handle *.so).
I couldn't find an answer, other than you're thinking it's unnecessary.
I summarized the points from #1233 again in the part you quoted there. But to repeat from that PR:
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.
Further, proxy plugins that register more than one extension will most likely have to discriminate between extensions in their probe() function anyway.
But if you think it's useful to pre-filter by extension, then why limit it to caseless, single-dot extensions?
it just seems overkill at this point
Again, please describe what you mean by overkill (In what sense? "too flexible"? "too much code"? "too much CPU/memory overhead"? etc), it's not clear from the context what you mean. As I mentioned previously, it's actually less code, is no more complex, fixes the caselessness problem, supports multi-dot extensions, and allows various other simple patterns.
b4n commented on this pull request.
Breaks API (as the semantics of proxy registration changed).
Though, the new API makes sense to me, at least in theory.
* but not a problem in practice yet */
foreach_list(node, active_proxies.head) { PluginProxy *proxy = node->data; - if (utils_str_casecmp(ext, proxy->extension) == 0) + if (g_pattern_match(proxy->glob, strlen(file), file, NULL))
maybe cache the result of `strlen(file)` outside the loop?
@@ -1344,6 +1347,10 @@ void plugins_init(void)
g_signal_connect(geany_object, "save-settings", G_CALLBACK(update_active_plugins_pref), NULL); stash_group_add_string_vector(group, &active_plugins_pref, "active_plugins", NULL);
+ builtin_proxy_glob = g_strconcat("*.", G_MODULE_SUFFIX, NULL); + builtin_so_proxy.glob = g_pattern_spec_new(builtin_proxy_glob);
yeah
codebrainz commented on this pull request.
* but not a problem in practice yet */
foreach_list(node, active_proxies.head) { PluginProxy *proxy = node->data; - if (utils_str_casecmp(ext, proxy->extension) == 0) + if (g_pattern_match(proxy->glob, strlen(file), file, NULL))
Sounds like a reasonable optimization.
Breaks API (as the semantics of proxy registration changed).
I did bump the API version, should I also bump the ABI version to break all plugins (rather than just fixing the one proxy plugin known to exist which uses this)?
should I also bump the ABI version to break all plugins (rather than just fixing the one proxy plugin known to exist which uses this)?
Technically, yes. In practice it might be annoying for little gain, but I'm not sure it's a good thing to let breaks pass and "guess it'll be alright". So… I'd like to say it's not useful, but I'm not sure I do believe it.
So… I'd like to say it's not useful, but I'm not sure I do believe it.
One factor is that it's only for "proxy plugins", no usual plugins would be using this. I sort of think it would be useful to have a separate API/ABI version for proxy plugin API for such a case.
I would like peasy to continue to work on 1.28.
I would like peasy to continue to work on 1.28.
You can do that.
How if `geany_plugin_register_proxy()` has changed behavior? We haven't merged `geany_api_version()` yet.
Btw, geanypy is another proxy plugin and IIRC your geany++ too so that's at least 3.
How if geany_plugin_register_proxy() has changed behavior?
By those proxy plugins checking the API version and changing the extensions. It's either that or we bump the ABI version and break all plugins and still have to update the proxy plugins.
The code would look like:
```c ... #if GEANY_API_VERSION >= 230 const gchar *patterns[] = { "*.so", "*.dll", "*.plugin", NULL }; #else const gchar *patterns[] = { "so", "dll", "plugin", NULL }; #endif geany_plugin_register_proxy(plugin, patterns); ... ```
I wasn't clear, I mean without recompiling. Your suggestion is a compile time check.
Why didn't we merge `geany_api_version()` that was suggested a while ago? Then this would be less of a problem.
I wasn't clear, I mean without recompiling. Your suggestion is a compile time check.
You said "I would like peasy to continue to work on 1.28", which it will, without change. The compile-time check would only be needed to work correctly on 1.28 and 1.29 if this change is merged.
Why didn't we merge geany_api_version() that was suggested a while ago?
I don't know what that is, but assuming it's the run-time API version, you would still need to change your proxy plugin to use it.
It was suggested by you or maybe b4n when I wasn't allowed to pass Geany's runtime api version to geany_load_module for #469
It was suggested by you or maybe b4n
Ah right, I made [a PR for it](https://github.com/kugel-/geany/pull/3), but you closed without merging my commits.
Still I don't really see what it buys you, the example proxy change would then look like this instead of above:
```c ... const gchar *patterns[4] = { NULL }; if (geany_api_version() >= 230) { patterns[0] = "*.so"; patterns[1] = "*.dll"; patterns[2] = "*.plugin"; } else { patterns[0] = "so"; patterns[1] = "dll"; patterns[2] = "plugin"; } geany_plugin_register_proxy(plugin, patterns); ... ```
Not really an improvement over just using `GEANY_API_VERSION` with the preprocessor, unless I misunderstand?
Am 24.10.2016 um 21:27 schrieb Matthew Brush:
It was suggested by you or maybe b4nAh right, I made a PR for it https://github.com/kugel-/geany/pull/3, but you closed without merging my commits.
You suggested to not merge it until an actual use case arises.
Still I don't really see what it buys you, the example proxy change would then look like this instead of above:
... const gchar *patterns[4] = { NULL }; if (geany_api_version() >= 230) { patterns[0] = "*.so"; patterns[1] = "*.dll"; patterns[2] = "*.plugin"; } else { patterns[0] = "so"; patterns[1] = "dll"; patterns[2] = "plugin"; } geany_plugin_register_proxy(plugin, patterns); ...
Not really an improvement over just using |GEANY_API_VERSION| with the preprocessor, unless I misunderstand?
The improvement is you can upgrade Geany without recompiling Peasy. But if yea, if the ABI version is changed it needs to be recompiled anyway.
I simply would like to avoid having to recompile Peasy between 1.28 and 1.29, but don't mind if it's too much work.
Best regards
Hum, if the new proposed API is case-sensitive and the previous wasn't, it could be a problem if it's not clear enough in the docs. Also, how does it affect Windows? (where filesystem names are case insensitive I believe)
Hum, if the new proposed API is case-sensitive and the previous wasn't, it could be a problem if it's not clear enough in the docs.
I don't think so since it should do what is expected for filenames (see below).
Also, how does it affect Windows? (where filesystem names are case insensitive I believe)
It basically fixes non-Windows and "breaks" Windows. In one of the intermediate commits on this branch where I added support for the dot but before using pattern matching, I had accounting for this by adding a conditional code to do proper UTF-8 windows case insensitive comparison, but it doesn't apply this this.
One way to support it would be to casefold all of the extensions and casefold the filename and then compare them in a `#if` for Windows.
Conflicts and still discussion in progress, moved to 1.31
@codebrainz are you still interested in this? Otherwise we can close this, right?
I would be useful to have it.
I think we still need to resolve the case sensitivity question. Case sensitivity can be a problem on Windows since you can't easily rename to lower case (must rename to a temporary name first)
github-comments@lists.geany.org