[Geany-Devel] New plugin loader mechanisms

[Colomban Wendling]

Hi,

Le 30/03/2015 08:52, Thomas Martitz a écrit :
>> […]
>>
>> ```
>> 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.

OK, this explanation makes me feel better with your proposal :)

What concerned me mostly was that you were proposing a new API, but I
felt it being riddled with "relics from the past" -- e.g. instead of
designing the new thing to be great on its own, you made it to be the
same as the old one, just getting rid of the most obvious shortcomings.

And I didn't feel very comfortable with this, because I felt like we'd
then probably want to rewrite it again in the (near) future, which would
mean breakage or introducing a third API.  Basically I feel that if a
new thing was introduced, it should be done right, and the complexity of
keeping compatibility should be a minor concern.

However, with some of the reasoning above (and below), I feel more
confident it is not only choices made for facility, so that's a bit of a
relief :)

And I must admit that indeed low change impact is unfortunately a
factor… Scotty, we need more (man) power! :)

>> […]
>>
>> 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.

Good point.  Though in my new thing idea the plugin-provided self would
have been the new handle, but it's true that it'd require API changes.

>> 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?

This last sentence is partly a leftover from when you still had a
set_info() plugin vfunc, so you can mostly forget it.  The only real
point I tried to make was that we need the plugin info no matter what,
so we could (should) give it when registering the plugin, not after.
But you addressed that now, so this point is moot :)

>>> […] 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.

I was suggesting this consideration :)
I never saw much use for it, but if people prefer this way of setting
handlers, well, why not.  Just remember this can *only* connect to
Geany's own signals, none else.

>> […]
>> 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.

I don't mind it really, I just couldn't understand why it was useful.

BTW, if geany_plugin_register() can only be ever called once from
geany_load_module(), do we really need it (for normal plugins)?  I mean,
we could do this:

```
typedef struct GeanyPlugin
{
	struct GeanyPluginPrivate *priv;	/* private */

	PluginInfo	*info;	/**< Fields set in plugin_set_info(). */
	GeanyData	*geany_data;

	/* new style plugins fill these */
	gpointer	plugin_data;	/**< pointer to arbitrary plugin data.
					 **  should be set on init() and freed on cleanup() */

	/* or keep using ->info, either way */
	const gchar	*name;			/**< The name of the plugin. */
	const gchar	*description;	/**< The description of the plugin. */
	const gchar	*version;		/**< The version of the plugin. */
	const gchar	*author;		/**< The author of the plugin. */
	guint		api_required;
	guint		abi_version;
	/* ... */

	/* vfuncs */
	gboolean	(*init)			(GeanyPlugin *plugin);
	void		(*cleanup)		(GeanyPlugin *plugin);
	GtkWidget*	(*configure)	(GeanyPlugin *plugin, GtkDialog *dialog);
	void		(*help)			(GeanyPlugin *plugin);
}
GeanyPlugin;
```

and simply have the geany_load_module() fill in the passed GeanyPlugin,
and return TRUE if all is well, FALSE otherwise.  Only downside I see is
that a plugin could forgot setting ->api_* members, but that could
easily be caught by the loader and considered failure.  Unless the
plugin needs to know the return value of geany_plugin_register()?

And as/if this is required for proxy plugins, this can be part of the
proxy plugin specific API along with the other specific stuff.

My point here is that I feel like geany_plugin_register() is a bit
artificial for normal plugins, because they have to call it exactly
once, and could report the same information some other, possibly more
natural, ways.


Anyways, what about putting plugin info inside GeanyPlugin instead of
introducing PluginHooks or something?  A plugin would just do:

```
geany_load_module(GeanyPlugin *plugin, ...) {
	plugin->/*info->*/name = _("Dummy");
	plugin->/*info->*/author = _("Myself");
	plugin->/*info->*/version = "42";
	...
	plugin->init = my_plugin_init;
	plugin->cleanup = my_plugin_cleanup;
	...
}
```

Regardless of whether we do need geany_plugin_register(), isn't this
better API than having separate structure for the various bits?

So, I'm formally suggesting extending GeanyPlugin structure itself more
or less as shown above :)


BTW, I see a memory management problem with your `pdata` argument:

> gboolean geany_plugin_register(GeanyPlugin *plugin, gint api_version, gint min_api_version,
>                                gint abi_version, GeanyPluginHooks *hooks, gpointer pdata); 

If `pdata` is provided in geany_plugin_register(), how does it get
released?  If it has to be a pointer to static data it's a bit limiting,
and forces use of (static) globals, which is one thing that this new API
tries to avoid, doesn't it?  We could also ask for a GDestroyNotify, but
that's an additional arg we might not need.

And also, providing the pdata in geany_load_module() might mean memory
allocation, allocation that might never be used if the plugin isn't
activated.

OTOH, if `pdata` is a member of GeanyPlugin, it can be allocated in
init() and freed in cleanup() without having to tell Geany how it has to
free it.

>>> 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. […]

OK.  (So I guess in my dreamland my API was enough even without
unregister() :) )


Regards,
Colomban