Hi,
in the hope I answered your pluxy-related questions in the other post I'll try to concentrate on the loader for this reply.
Am 29.03.2015 um 19:04 schrieb Colomban Wendling:
Hi,
I'm answering both to the initial mail and this one here, so expect mixed citations. This will avoid raising the same points twice :)
First, stuff extracted from the first mail. Disclaimer: Sorry if some remarks look slightly off-formulated, but while I altered them in the light of the last changes, I didn't have the courage of rewrite all sentences from the ground up when they were still (mostly) relevant.
gboolean geany_plugin_register(GeanyPlugin *, gint api, gint abi, PluginHooks *(see below), gpointer)
The plugin defines a single global function, geany_load_module(GeanyPlugin *, GModule *). This is the only function that geany learns about using g_module_symbol(). And the only thing this function ought to do is to call geany_plugin_register(). This does 4 things
- Provide the plugin handle to the plugin very early
- Perform abi and abi checks, so this is finally done inside geany.
Added bonus is that geany knows the requested api and can possibly apply backcompat workarounds on a per plugin basis (instead of globally), warn about very old plugins or even deny loading them. 3) Register the remaining hooks and callbacks (see below) 4) Associate a userdata pointer to the plugin, geany will pass this pointer back to future calls into the plugin (good for proxies)
In the future geany_plugin_register should be able to be used to register plugins recursivly, by passing the appropriate GeanyPlugin pointer, i.e. plugin A should be able to call geany_plugin_register(plugin_B, ...) to realize pluxies.
How does the proxy plugin create plugin_B? plugin_A is given by Geany, but how is plugin_B created?
See the other thread. plugin_b is also created by Geany, in terms of locating the file, allocating GeanyPluginPrivate and integrations with the PM dialog. Only the job of loading/unloading is offloaded to the proxy (because it knows how to do it), and nothing else.
geany_load_module(...) { static struct RealPlugin self = { N_("foo"), N_("john doe"), "42", ... my_plugin_init, my_plugin_uninit, my_plugin_configure, ... }; geany_plugin_register(... &self ...); }e.g. give Geany a structure representing the plugin, and that's all. Note that with this apporach, you can even add "class-style" custom data to a plugin simply by having a struct larger than RealPlugin:
struct MyRealPlugin { RealPlugin parent; /* offset 0 has the Geany struct, so it's binary compatible */ /* plugin-specific fields here */ int the_game; };
Now this is a big methodology change, from Geany-allocated plugin handles to plugin-allocated plugin handles. This change has so many implications and touches so many things that I really don't want to do it. And given we want to maintain compatiblity to old plugins we have to support both at the same time.
My new loader is much less invasive, *by design*. It only changes the way the already known hooks are registered and called (but still being transparent to the plugin), in order to provide a solution to the problems I outlined in the initial mail. And it provides binary compatibility to older plugins at very little extra complexity.
I am mostly happy with how we load plugins, with my new loader I am completely happy, so I don't feel the big change you propose is justified. I don't do all of this for the sake of change, I want to provide an effective solution, and the fundament for proxy plugins. I don't want to change all the way we interact with plugins, which will also require plugin developers to re-learn.
This is by far not the primary reason, but I also try to keep the changes less invasive to actually improve the chance of it being reviewed and merged in a reasonable time frame.
Maybe proxy plugins need a data argument, but maybe not: they should be able to pack whatever they need in the struct they register for the "sub"-plugins. Not extending the structure from plugins might make easier extending the structure on our side, without needing padding though.
But anyway whether it's an extra data* argument (or member) or an extended struct doesn't change much my point, which would be that all the plugin description would be given to Geany as one single thing.
With your new proposal that gets rid of set_info() the "how does a proxied plugin give its info" (mostly) moot, but there still is the question why having separate argument holding pointless stuff (GeanyPlugin) while all could be packed in a new and better structure.
GeanyPlugin is Geany's *handle* to the plugin and required to call lots of our API functions. With no global pointer being set in the plugin we have to provide the pointer to the plugin code through function arguments.
You could only pass it to init and expect plugins to store their own pointer, but I find that bad practice, because it needs to be done in all plugins, because it otherwise won't be useful (because it can't call many API functions). If we can make things more convenient for plugin developers I'm for it.
E.g. my point could maybe be summarized with "why split things up while they actually represent the same thing?".
And BTW, remember that we need the (translated) plugin infos in all cases, so it should be no difference giving it straight ahead -- even proxied plugins.
I don't understand this part. What's the implications of translated plugin info? I think it only needs GeanyData to be available?
[…] Additionally the existing PluginCallbacks pointer registered here for all plugins that want statically defined signal handlers.
I'm really not sure if we should bother keeping this. Is there any benefit over plugin_signal_connect()?
But what's the problem with PluginCallback? It's a convenient and useful feature to allow signal connections to be statically defined. The cost to support is really tiny, and it's not like we deprecated this API aspect so I don't see removing it is justified. Removing things from the API still requires some considerations.
[…] Please also note that each hook receives the GeanyPlugin pointer and the userdata pointer, these are the same passed to geany_plugin_register.
Why pass the GeanyPlugin? I mean, what useful info is in this? there is the PluginInfo, which seem hardly useful, and the GeanyPluginPrivate, which is totally opaque, so totally useless. OK you add the GeanyData too which is useful (but could be part of the RealPlugin thing just as well).
Again, it's needed for some API functions and the global pointer is gone. To my understanding it's the primary handle that Geany has to the plugin and the plugin sees as well.
Again, here I'd have expected something like my_plugin_init(self, ...), as 1) it follows the common "pass the self/this as first parameter" scheme, and it's actually the only thing that seem to matter. Extra args are of course useful when they are needed, like GtkDialog* for configure.
Le 28/03/2015 23:56, Thomas Martitz a écrit :
[…] geany_load_module(), which is called by Geany via pointer obtained from g_module_symbol(). In this function the plugin should perform the following steps:
- Test against the passed geany_api_version if it can function under the
current Geany.
- Test for any additional runtime dependencies
- Call geany_plugin_register() appropate and test the return value
If any of the above steps fail FALSE should be returned, else TRUE.
Why return gboolean? is this just meant to allow geany to say the current "incompatible plugin, pleas rebuild"? If so, shouldn't this rather be a result of a failing geany_plugin_register()?
I don't get why geany_load_module() should fail, IIUC it should just not call geany_plugin_register() if it can't work, shouldn't it?
It could be done this way too, indeed. Actually I don't use the return value currently, because I check the loaded_ok flag set by geany_plugin_register() [after successfull API/ABI/PluginInfo checks]. I mainly added the return value as a future provision, but I can remove it if you like.
This proposal does not implement pluxies by itself, however I have done the work to implement pluxies on top of the new loader. I will post another mail about this when we finalized the new loader.
That might be where I don't get it. As I see it, the only missing bit from my slightly altered proposal is a geany_plugin_unregister() so a proxy plugin can remove its sub-plugins when it gets deactivated. Maybe I miss something, but that seem to be the only bit that would be required for a proxy plugin a normal plugin wouldn't need.
I hope the other reply shed some light. But let me say here that geany_plugin_unregister() is not necessary under the current or new loader, neither for standard nor proxied plugins. Deregistering is simply not a thing in our scheme. Once registered, the plugin will be considered for the PM dialog (after some checks). That doesn't mean it's enabled or activated, it just means the PM dialog potentially shows it. Deregistering doesn't need to happen, it would only mean that the PM dialog wouldn't show it anymore. Which is the same thing as if there is no pluxy available for it, i.e. the corresponding pluxy is disabled.
In my current proxy plugin work it's not possible to disable pluxies if there are active sub-plugins depending on it, but this is purely a policy decision. We can easily change to automatically disable sub-plugins as well (perhaps with a dialog warning the user). The point is, even here there is no deregister. Geany will deregister on it's own, and only notify the plugin via its cleanup() function.
This is not to be confused with unloading, which happens when the PM dialog is closed for inactive plugins or when Geany quits for active plugins. Here Geany also derigsters on its own, but it does need the help of the pluxy to actually unload the plugin(s) (via the unload hook).
Best regards.