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