[Geany-Devel] New plugin loader mechanisms

[Thomas Martitz]

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
>> 1) Provide the plugin handle to the plugin very early
>> 2) 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.