I personally hope whatever the group decides to do with the plugins doesn't involve requiring a rewrite of all of them, because we'll surely lose plugins and supporters that way.

Concerning some of your perceived shortcomings:

On 03/18/2015 10:42 AM, Thomas Martitz wrote:

Currently geany exports a pointer to a struct, that contains more structs, which contain function points to the API functions. Fortunately this is nicely hidden to developers via macros. But due to gtkbuilder all functions and nothing prevents plugins from accessing these. And the macros are awkward and strange anyway. There is currently the linkage-cleanup PR in the works which improves this by actually exporting the API functions, and _only_ the API functions to plugins.

Maybe I'm completely wrong on this from an architecture perspective, but part of what I like about writing plugins for geany is accessibility. If we only get access to a subset of functions, then it seems less flexible what our plugins can actually do. Yes, this allows us to write bad plugins that can do some sloppy things, but I say "so what". They are plugins. Someone would have to go out of their way to install most plugins outside of geany-plugins, and there is some vetting for that list of plugins. I say by keeping the restrictions minimal on what plugins can access, we can get more powerful plugins and not block off potential plugins by our over-abstraction.

Take chrome/chromium browser, for instance. They basically have restricted all plugins to be at most a button on the toolbar or effecting web pages. There seems to be no possible way to write a plugin to get vertical tabbing I so appreciate in firefox (and geany for that matter) because chrome seems to have this stuck-up mac attitude that it's the way they intended, no customization allowed, "mission accomplished https://www.youtube.com/watch?v=MgSQA1jqFpM". Maybe I'm wrong, maybe that's not chrome's motive, but I certainly don't like the lack of flexibility of their plugin architecture. (If anyone knows a way on linux to get vertical tabs in chrome, that would be awesome ;-)

I suppose you could argue that having access to almost everything requires more frequent updating of plugins, but personally I haven't had more than one or two line changes with any update to geany. Plus then we have to worry more about plugin support and it's own set of bugs.

That's just my opinion. Thoughts?

Thanks,

Steve

On 18.3.2015 г. 18:42, Thomas Martitz wrote:

• Global symbols. Plugins binaries have to export a number of global

symbols (geany_{functions,data,plugin}, plugin_{init,...,cleanup}). This kind of sucks, because they pollute the global namespace (in theory). Luckily on unix or win32 systems this is not a problem because they can restrict the symbol visibility of shared libraries. It's still bad practice. Ideally plugins should have zero global symbols, everything being static or hidden to the plugin binary.

Scope contains 20 source files and 22 headers. Using static is not an option, and marking everything global as hidden will be cumbersome, ugly, and easy to miss (inexperienced plugin developers are sure to miss symbols).

The only practical option seems to be compiling with hidden visibility, and geany_load_module() should be pre-defined as exported.

[...] void (*init) (GeanyPlugin *plugin, gpointer pdata);

Please make this gboolean. A plugin may have the correct API and ABI, but be unable to startup / initialize for some reason. For example, Scope requires scope.glade in the plugin data directory).

-- E-gards: Jimmy

On 15-03-18 01:23 PM, Dimitar Zhekov wrote:

On 18.3.2015 г. 18:42, Thomas Martitz wrote:

...

• Global symbols. Plugins binaries have to export a number of global

symbols (geany_{functions,data,plugin}, plugin_{init,...,cleanup}). This kind of sucks, because they pollute the global namespace (in theory). Luckily on unix or win32 systems this is not a problem because they can restrict the symbol visibility of shared libraries. It's still bad practice. Ideally plugins should have zero global symbols, everything being static or hidden to the plugin binary.

Scope contains 20 source files and 22 headers. Using static is not an option, and marking everything global as hidden will be cumbersome, ugly, and easy to miss (inexperienced plugin developers are sure to miss symbols).

Why does it need so many globals? Shouldn't the only globals really be the stuff Geany requires? I'm wondering because one day it would be cool to able to do stuff like having multiple "instances" in-process and to allow a plugin per in-process "instance" or some stuff like this. With the additional userdata pointer, in theory one could make a big huge structure containing all their global (instance) state and have that passed around, and then there's less issue with symbols and multiple instances and such.

(I get that it's _not_ like that for Scope, but in theory it could've been and for new plugins it could be recommended).

...

[...] void (*init) (GeanyPlugin *plugin, gpointer pdata);

Please make this gboolean. A plugin may have the correct API and ABI, but be unable to startup / initialize for some reason. For example, Scope requires scope.glade in the plugin data directory).

+1, I've always wanted a way to signal Geany "don't bother, it's just going to crash you if you keep going". The only way I can see to handle critical failures without a status return from there is to keep a global variable and guard each function so it can't execute its normal code, which is a bit of a pain and weird for users if the plugin loads but doesn't do anything.

Another useful thing might be to provide a way to return a reason message as well, then when Geany gets the failure return it can open an error dialog and show the user a message that the plugin couldn't be loaded, and show the reason string and maybe some bug-reporting info or something.

Cheers, Matthew Brush

On 15-03-18 09:42 AM, Thomas Martitz wrote:

Hello,

tl;dr -> scroll down

I am working on a new plugin architecture that deals with some of the shortcomings of the current state. My primary motivation is to be able to use libpeas to load plugins, both C and non-C (Python!), as you might have learned from other threads I started. However the situation can be improved regardless of that goal.

List of current shortcomings:

• (A separate change but nevertheless: ) Currently geany exports a

pointer to a struct, that contains more structs, which contain function points to the API functions. Fortunately this is nicely hidden to developers via macros. But due to gtkbuilder all functions and nothing prevents plugins from accessing these. And the macros are awkward and strange anyway. There is currently the linkage-cleanup PR in the works which improves this by actually exporting the API functions, and _only_ the API functions to plugins.

• Global symbols. Plugins binaries have to export a number of global

symbols (geany_{functions,data,plugin}, plugin_{init,...,cleanup}). This kind of sucks, because they pollute the global namespace (in theory). Luckily on unix or win32 systems this is not a problem because they can restrict the symbol visibility of shared libraries. It's still bad practice. Ideally plugins should have zero global symbols, everything being static or hidden to the plugin binary.

• The plugin entry points / callbacks are inconsistent w.r.t to the

parameters they receive, and none receive some kind of a plugin handle referencing to the plugin itself (there is only the geany_plugin global).

• The plugin entry points / callbacks do not allow for the plugin

associate private/user data with the plugin handle, except hand-maintain hash tables. This is not a problem for the most part because it can be stored in some plugin-wide static variable, however it does become problematic when you attempt to have one plugin act as a proxy for other plugins (see geanypy or my pluxy effort)

• The plugin does the ABI/API verification. We currently trust the

plugins to use PLUGIN_VERSION_CHECK() or otherwise implement plugin_version_check() correctly. Plugins decide whether they are api/abi compatible with geany. Pure crazyness!

• Plugins cannot register plugins recursively. It would be awesome if

a plugin could register a plugin on behalf of others, in such a manner that they appear in the PM dialog and can be handled by the user like normal plugins (*proper* proxy plugins are not possible).

To improve the situation I propose the following mechaism and new plugin hooks:

tl;dr <- Key functions

gboolean geany_load_module(GeanyPlugin *, GModule *)

What is the GModule* for? Is it a .dll that Geany opened on behalf of the plugin based on selection in Plugin Manager?

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.

Now to the plugin hooks: typedef struct _PluginHooks { PluginCallback *callbacks; void (*set_info) (GeanyPlugin *plugin, gpointer pdata); void (*init) (GeanyPlugin *plugin, gpointer pdata); GtkWidget* (*configure) (GeanyPlugin *plugin, GtkDialog *dialog, gpointer pdata); void (*help) (GeanyPlugin *plugin, gpointer pdata); void (*cleanup) (GeanyPlugin *plugin, gpointer pdata); } PluginHooks;

What if instead of PluginHooks it was called `Plugin` (GeanyPlugin is taken, so for this discussion I'll use `Plugin` :) and instead of just the callback function pointers it contained the (possibly sub-)plugin's info, like this:

``` typedef struct { const char *name; const char *version; const char *author; const char *description; unsigned api_min; unsigned abi_ver; void *plugin_data; // pdata/plugin context

bool (*init) (Plugin*); GtkWidget (*configure) (Plugin,GtkDialog*); gchar* (*help) (Plugin*); // if not NULL ret, show URL in browser bool (*deinit) (Plugin*); // could signal unloading problem } Plugin; ```

Then the "register" function could be like:

``` bool plugin_register (GeanyPlugin *module, // geany's plugin context Plugin *plugin); // real/sub-plugin ctx ```

Inside a normal plugin it could do:

``` static Plugin my_plugin = { .name = "Foo", .version = "0.1", .author = "Me", .description = "Foo plugin", .api_min = 200, .abi_ver = GEANY_ABI_VERSION, .init = my_plugin_init, .configure = my_plugin_configure, .help = my_plugin_help, .deinit = my_plugin_cleanup, };

G_MODULE_EXPORT bool plugin_load_module (GeanyPlugin *module_plugin) { return plugin_register (module_plugin, &my_plugin); } ```

Or for a proxying-type plugin (ex. GeanyPy/GeanyLua):

``` G_MODULE_EXPORT bool plugin_load_module (GeanyPlugin *module_plugin) { Plugin *plug = g_new0 (Plugin, 1); *plug = my_plugin; plug->name = "Joe's plugin"; plug->version = etc... plug->plugin_data = PyObject_New(...); // or lua_new() or whatever return plugin_register (module_plugin, plug); } ```

Just some ideas based on yours and mine previous work around this. There's many ways to skin this cat :)

Cheers, Matthew Brush

On 15-03-18 03:05 PM, Thomas Martitz wrote:

Am 18.03.2015 um 22:15 schrieb Matthew Brush:

...

...

...

[...] void (*init) (GeanyPlugin *plugin, gpointer pdata);

Please make this gboolean. A plugin may have the correct API and ABI, but be unable to startup / initialize for some reason. For example, Scope requires scope.glade in the plugin data directory).

+1, I've always wanted a way to signal Geany "don't bother, it's just going to crash you if you keep going". The only way I can see to handle critical failures without a status return from there is to keep a global variable and guard each function so it can't execute its normal code, which is a bit of a pain and weird for users if the plugin loads but doesn't do anything.

Another useful thing might be to provide a way to return a reason message as well, then when Geany gets the failure return it can open an error dialog and show the user a message that the plugin couldn't be loaded, and show the reason string and maybe some bug-reporting info or something.

Thinking about it, if the plugin can't run because it's missing resource files required for its operation, then I think it should be treaded like incompatible plugins. This has the benefit that they will be attempted to be loaded on the next startup if the user had previously selected it.

For resource files like say a GtkBuilder UI file, I'd agree, but there may be some other cases, for example if a plugin dynamically loaded some particular library (or variant of a library) based on user configuration, it'd be useful to report to the user that the library is wrong, or no longer available, or whatever.

Incompatible plugins simply return false in geany_load_module() [I've implemented a safety guard geany_load_modules() even though the abi/api version check failed]. The plugin can also print the reason to stdout/msgwin/error dialog from within that function, although IMO a dialog is too intrusive.

I generally hate dialogs, but I think a plugin failing to load is a fine use for one since it doesn't happen with high frequency, and just dumping g_critical() or equivalent on the console is useless/confusing if the user hasn't launched from a terminal (and doesn't realize about Help->Debug Messages).

I'm thinking if the plugin loaded successfully, then it should be operational too. Meaning that init() should not fail, but simply activate the plugin. As outlined above, my proposal already covers the case "compatible but not operational due to missing runtime dependencies" you described.

For cases like GeanyPy which loads arbitrary Python scripts (which are even fully executed on import), and in a language where Exceptions are common (especially during development), it would probably be useful to signal that the plugin script couldn't be loaded and maybe even be able to provide a formatted traceback of the Python exception or such.

Cheers, Matthew Brush

Am 18.03.2015 um 22:23 schrieb Lex Trotman:

Hi Thomas,

In general this looks like an improvement to the current method of setting up plugins, except it continues to require the plugin .so file to be dlloaded just to populate the plugin manager. That will run the dll initialization code, and may therefore break Geany. Have you thought about having a separate plain data file with the data for the plugin manager so the dll doesn't need loading until its activated by the PM?

A few specific things in-line below.

Yes I have thought a lot about this, and I concluded that libpeas is the right way to accomplish this. Therefore my longterm goal is to use libpeas for geany plugins. According to my current roadmap I'll implement this as a plugin (i.e. a plugin that uses libpeas to proxy other plugins), which I already have prototyped. In this scenario the other plugins use the peas-style .ini metafile.

To be able to do this I need a few, relatively minor changes, to Geany to support this infastructure. This will also benefit geanypy, which can then too act as a proxy and finally show scipts in the main PM dialog and support keybindings properly. All of these infrastructure changes remain API and ABI compatibility to all existing plugins.

For Geany core I don't plan to bring a metafile soon. I tried to get libpeas into the core, but such major changes are not feasible at the moment.

Ok, the number of symbols went from 5-6 to 1 but we still have the same name exported from each dll, so the original problem of symbols still exists. I don't think there is a solution really.

This is a solution, and I think it's the only one. The difference is that with this symbol it is only referenced once through g_module_symbol() by geany, by using the GModule handle. This way namespace collisions *cannot* occur. Afterwards the function is never referenced anymore, neither by the core nor plugin, so in the case that a second plugin's geany_load_module() collides with the one of the first plugin the first one is never referenced again anyway. There is no problem here.

Whereas now plugins reference their geany_plugin globals (without g_module_symbol()) which, unless measures are taken, collide accross plugins (so plugin 1 could accidentally access plugin 2's geany_plugin).

There is no other way of loading C shared libraries and registering them as plugins.

...

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.

This precludes the plugin from adapting itself to Geany since the plugin is not supplied with the API and ABI numbers. It would be good if a plugin could do simple things like:

 if (ABI > 124) f1() else f2();

Surely you mean if (API > 124) f1() else f2() right?

But indeed, the API version could be passed to geany_load_module() to allow for the above. Passing the abi version is pretty useless as the plugin will only load with one specific abi anyway so it can't adapt.

Also it would be good to consider the semantics of API and ABI since there are some conditions where we cannot express a change in the interface except for the hammer of an ABI break.

We should be able with the new loader mechanism, since we learn about the plugin's minimum API version. Now we can not only deny based on the ABI version but also based on the minimum API (we could possibly deny loading plugins which are written against too old API version).

Best regards

Am 19.03.2015 um 01:19 schrieb Matthew Brush:

On 15-03-18 03:55 PM, Thomas Martitz wrote:

...

Am 18.03.2015 um 23:21 schrieb Matthew Brush:

...

On 15-03-18 03:05 PM, Thomas Martitz wrote:

...

Am 18.03.2015 um 22:15 schrieb Matthew Brush:

...

...

> [...] > void (*init) (GeanyPlugin *plugin, gpointer pdata);

Please make this gboolean. A plugin may have the correct API and ABI, but be unable to startup / initialize for some reason. For example, Scope requires scope.glade in the plugin data directory).

Thinking about it, if the plugin can't run because it's missing resource files required for its operation, then I think it should be treaded like incompatible plugins. This has the benefit that they will be attempted to be loaded on the next startup if the user had previously selected it.

For resource files like say a GtkBuilder UI file, I'd agree, but there may be some other cases, for example if a plugin dynamically loaded some particular library (or variant of a library) based on user configuration, it'd be useful to report to the user that the library is wrong, or no longer available, or whatever.

Based on user configuration implies that it's a decision that is made after the plugin's init(). If it allows the user to configure it without this dependency then the plugin is considered operational, and init() should not fail. Remember that a init() == FALSE would imply that the plugin cannot be activated, and therefore not configured (i.e. you cannot configure back so that it doesn't need the missing dependency).

Such custom requirements & errors are better placed in the plugin code itself.

...

...

I'm thinking if the plugin loaded successfully, then it should be operational too. Meaning that init() should not fail, but simply activate the plugin. As outlined above, my proposal already covers the case "compatible but not operational due to missing runtime dependencies" you described.

For cases like GeanyPy which loads arbitrary Python scripts (which are even fully executed on import), and in a language where Exceptions are common (especially during development), it would probably be useful to signal that the plugin script couldn't be loaded and maybe even be able to provide a formatted traceback of the Python exception or such.

In my roadmap geanypy does not load scripts in its init(), but through a separate API (so that the scripts integrate into the main plugin manager). Anyway, geanypy init() isn't the right place because geanypy can load multiple scripts, and which scripts can change afterwards after init has run. And finally, all those scripts do not change the fact that geanypy itself is operational, and this is what we're talking about.

I think I misunderstood the purpose of your `init()` function. I thought it was a hook to allow the plugin manager/geany to be able to initialize multiple plugins from the same .dll/module (ex. sub-plugins, etc). If that's not the case, isn't the `init()` pointer in the struct basically redundant as plugins could do their initialization in the roughly equivalent `geany_load_module()` that is also called once per .dll?

I haven't changed the semantics of init function (it is the same plugin_init() that already exists), and there is no support for pluxies or sub-plugins yet. My new loader doesn't do that, it only changes how the plugin is registered and the parameters plugins receive in the hooks.

The difference between the geany_load_module() and init() is that the former is always called, even if the plugin is not enabled by the user. And it can fail (e.g. if the ABI check fails) which prints warnings and hides it from the PM GUI.

geany_load_module() doesn't have an equivalent, therefore plugins with runtime hard-dependencies could previously only fail in init somehow (as a hack). Now geany_load_module() can do that. I think init() is generally not the right place for that. When it is called then the plugin was enabled by the user, i.e. it is expected to function somehow from here on. Proper plugins will allocate resources and hook into the GUI in this function that are not needed when the plugin is disabled. At last, I do not want to change the semantics of the existing hooks if possible because that makes merging my patch harder.

For that matter, why not just leave the hooks all as loose functions (as opposed to set into a structure), and just fix the prototypes to pass around GeanyPlugin* and/or user_data, or whatever improvements? AFAIK there's no issue with symbols/collisions if Geany just uses RTLD_LOCAL when dlopen-ing the plugin module.

Well it was determined that we want new functions instead of adding or changing the parameters of the existing hooks. And I think it is desirable to minimize the amount of globals in the plugin while at it. Besides, the new geany_load_module() provides some true benefits, such as ABI/API verification in the core instead of in the plugin.

Best regards

Am 19.03.2015 um 23:34 schrieb Lex Trotman:

On 19 March 2015 at 09:16, Thomas Martitz kugel@rockbox.org wrote: There is no other way of loading C shared libraries and registering them as plugins.

Doesn't Geany already load dlls local and extract the symbols it wants via dlsym()?

dlsym() takes an argument of the loaded dll handle, so it can't access the wrong library.

Yes, there is no problem in this if accesses go through dlsym with the appropriate library handle. Global namespace collisions can occur of symbols exported by multiple shared libraries are accessed the normal way, either by geany or plugins. This happens in the current loader, as plugins access their geany_plugin pointer (but does not happen under my new loader). As we use RTLD_LOCAL so we have no problems here either, however it is still nice to have to minimize the amount of globals exported by the plugin.

API/ABI just a typo :) Note that allowing the plugin to make this decision means it has to have a way of overriding the default >= test you have moved into geany.

I can't follow this.

...

But indeed, the API version could be passed to geany_load_module() to allow for the above. Passing the abi version is pretty useless as the plugin will only load with one specific abi anyway so it can't adapt.

...

Also it would be good to consider the semantics of API and ABI since there are some conditions where we cannot express a change in the interface except for the hammer of an ABI break.

We should be able with the new loader mechanism, since we learn about the plugin's minimum API version. Now we can not only deny based on the ABI version but also based on the minimum API (we could possibly deny loading plugins which are written against too old API version).

The decision of "too old" needs to be made by the plugin, Geany doesn't know what the plugin uses, that a plugin declares "I can run with any API newer than 123" doesn't mean it actually uses 123. It is just being friendly and saying "I can run with older Geany's as well".

The plugin may know it better, that's why I follow your suggestion and pass Geany's API version to geany_load_module.

I was saying that we could deny loading plugins that report 123 as their minimum if we made an incompatible API break in 130 like removing a function all together. While it's true that 123 doesn't mean it uses that very function, Geany could be conservative and deny loading. (I know this was a bad example because we bump the ABI for removed functions, and plugins that use the removed function would not recompile successfully, but you get the idea).

Best regards

On 18.3.2015 г. 22:51, Thomas Martitz wrote:

Am 18.03.2015 um 21:23 schrieb Dimitar Zhekov:

...

On 18.3.2015 г. 18:42, Thomas Martitz wrote:

Do you generally like or support my proposal?

Yes, because the final goal is to be able to support different languages with gtk+ bindings, like/with libpeas that you mentioned.

(Of course, a loader for each new language would still be required (peas has loaders C/C++, Python and JavaScript IIRC), but it would be much easier if gtk+ bindings are used.)

-- E-gards: Jimmy

Am 20.03.2015 um 19:45 schrieb Dimitar Zhekov:

...

Thinking about it, if the plugin can't run because it's missing resource files required for its operation, then I think it should be treaded like incompatible plugins.

There seem like two different things to me.

The first thing a loader needs to do is query each plugin for compatibility. Incompatible plugins should not be listed at all, and it'll be good if the loader emits status messages for them in the Status tab. As an alternative, they may be listed disabled, with a message why the module can't be used.

Now, do we really want the plugins to run arbitrary resource checking code, and display their own error messages, only because they are queried (or registered, as you put it), each time the list is build?

They [incompatible plugins and plugins that are unable to function due to missing runtime deps] are different things, but the resulting action should be the same (hide it from the PM dialog, possibly give the user a hint). Hiding from the PM dialog should IMO be done before it even showed up, to avoid creating expectations that cannot be met and negative user surprise.

Yes plugins should be able to run arbitrary checking code because that's necessary for proxy plugins. Whether Geany can provide APIs to report results to a the user in a consistent manner is on another piece of paper, one that's IMO slightly out of scope for my proposal.

...

I'm thinking if the plugin loaded successfully, then it should be operational too.

I can check if scope.glade exists, but is it valid XML, compatible with the current version of gtk_builder? The only way to be 100% sure is to actually load it. And them immediately unload it, because Scope is being queried only, not activated. And then load it again on init...

My opinion is that if the XML isn't compatible to the current GtkBuilder version then scope shouldn't even show up in the PM dialog, because there is no way it can be functional. This is not fundamentally different to a plugin being incompatible to Geany's ABI version. So yes, that implies validating the XML in geany_plugin_register() (although loading seems excessive to me, because the XML should be considered distributed with the plugin and not to be edited by the user; at some point you have to make some assumption to keep problems managable).

If this seems so inefficient you could try to rescue the state from geany_plugin_register() to init. However keep in mind that init can be called without prior call to geany_load_module(), if the user toggles plugins in the PM dialog. And that init might not be called at all, so plugins shouldn't be wasteful with resources in geany_load_module() if might never be needed.

But nobody can guarantee that it exists and is valid on init. What if the Plugin manager is open, scope.glade is removed or altered, and then "[ ] Scope Debugger" is checked? It's low probability, of course, but how am I supposed to react - just crash? If init remains void, then it would be no better than the current void plugin_init(), and I'll simply check anything in load - why bother, if I *still* need to manually disable scope on initialization failure?

What do you do currently?

Besides, I didn't mean to make init() any better than the current plugin_init() w.r.t. to its semantics. The only difference is more/other parameters.

Geany does not handle failing init() currently, and I don't want to change that now because I think that's out of scope for my proposal. I merely want to transform the current hooks into a new mechanism without introducing large discrepancy between the two mechaisms (because the current one will be still supported). I also tend to think that a failing init is misdesigned.

What do others think about this point?

Best regards.

On 15-03-25 04:22 PM, Thomas Martitz wrote:

Am 18.03.2015 um 22:55 schrieb Matthew Brush:

...

On 15-03-18 09:42 AM, Thomas Martitz wrote:

...

tl;dr <- Key functions

gboolean geany_load_module(GeanyPlugin *, GModule *)

What is the GModule* for? Is it a .dll that Geany opened on behalf of the plugin based on selection in Plugin Manager?

That's the function exported by native (C/C++) plugins that want to register as plugins with Geany. With my proposal that's the only function of a plugin that Geany gets through g_module_open(). Since this only applies to native shared libraries, the corresponding GModule is passed.

This function is similar to libpeas' peas_register_types().

Is it where "resident" plugins should register their GTypes similar to Peas?

The main reason I asked about it is because it seems to have the same call time, argument and similar return semantics as the existing GModule hook `g_module_check_init()`. I get that it passes the plugin struct too, and has a more useful name, mostly I was just curious about when to use which one.

What is the expected use of the GModule parameter? I'm curious if it's useful at all, or just exposing the loader's externals, and maybe whether if it's only for special use-case, that it might be better to just use `g_module_open(NULL)` in those place(s).

Cheers, Matthew Brush

On 26.3.2015 г. 01:16, Thomas Martitz wrote:

...

Now, do we really want the plugins to run arbitrary resource checking code, and display their own error messages, only because they are queried (or registered, as you put it), each time the list is build?

Yes plugins should be able to run arbitrary checking code because that's necessary for proxy plugins. Whether Geany can provide APIs to report results to a the user in a consistent manner is on another piece of paper, one that's IMO slightly out of scope for my proposal.

From your initial proposal:

"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()."

As a non-Englisher, I read this as "the only thing must / should / is supposed / is preferable", and am not sure which one. Some of the meanings contradict "should be able to run arbitrary checking code".

...

But nobody can guarantee that it exists and is valid on init. [...] how am I supposed to react - just crash? If init remains void, then it would be no better than the current void plugin_init(), and I'll simply check anything in load - why bother, if I *still* need to manually disable scope on initialization failure?

What do you do currently?

Emit a message in Status() + g_warning() and stop further init, plus check in cleanup(). A popup or status bar message would be acceptable too, since it's only if the plugin is actually activated, unlike any possible check in geany_load_module().

Besides, I didn't mean to make init() any better than the current plugin_init() w.r.t. to its semantics. The only difference is more/other parameters.

Indeed. That's why I asked "please make this gboolean".

Geany does not handle failing init() currently, and I don't want to change that now because I think that's out of scope for my proposal.

OK then, that's it.

I also tend to think that a failing init is misdesigned. [...] However keep in mind that init can be called without prior call to geany_load_module(), if the user toggles plugins in the PM dialog.

Under the current loading mechanism, a plugin is re-loaded with g_module_open() on each toggle. If you intend to change that, you'll break any global variable initializations in all plugins, most importantly the assumption that a global C variable is automatically set to zero.

If you are going to open the module each time, but call load only the first time, the "loadable" state may be wrong if something has changed between the open-s.

(And if you assume that init may be called without load, how can a falling init be misdesigned? We can mask it as void, but failure is still a failure.)

And that init might not be called at all, so plugins shouldn't be wasteful with resources in geany_load_module() if might never be needed.

(yes, that's why I said "load the glade file and then unload it")

As a conclusion, a gboolean load is better than nothing. The resource errors in Scope were due to the fact that the resource directories defined by Geany were wrong. AFAICT, this is fixed in the latest gits, but a g_file_test() in init will still be helpful.

--

Hello,

based on the discussion I refined my proposal (see below). Additionally I have implemented all of this, including updating demoplugin.c, see [1]. And last but not least, I have a fully working implementation of proxy plugins aka pluxies (in fact, I simply updated my year-old branch to use the new hooks) plus a real-world pluxy for libpeas-based plugins, see [2], [3].

I also formally suggest this work for merging. I havent done a pull request yet, though, because it depends on the linkage-cleanup work, which should be merged first. I do target the 1.25 release still, and I'd absolutely love to if we can make this happen.

The changes since my initial proposal summed up: - geany_plugin_register() now additionally takes the API version the plugin is compiled against (in addition to the minimum API it needs and the ABI version). This gives Geany full insight of what the plugin can be expected to work with. - set_info() is removed from the hooks. version_check() is obviously obsolete (the checks are performed within geany_plugin_register()). The plugin is expected to full plugin->info before calling geany_plugin_register() which makes set_info() superfluous - The conceptual role of geany_load_module() is extended: It should also check for plugin-specific runtime dependencies, and return FALSE if such checks fail (as a result the plugin will be hidden from the PM dialog). Otherwise call geany_plugin_register() appropriately. - geany_load_module() passes Geany's API version, sos that plugins can perform their own specific checks based on the API of the currently running Geany instance (this can very well be != the plugins GEANY_API_VERSION) - The GeanyData pointer is now part of GeanyPlugin structure for plugins to access (not passed to init anymore). Actually it was this way in my first version too, but I didn't explicitly mention it. - PluginHooks is renamed to GeanyPluginHooks

Now to the (updated) proposal:

Please see my initial mail [4] about the motivation and advantages of this work.

gboolean geany_load_module(GeanyPlugin *plugin, GModule *module, gint geany_api_version); gboolean geany_plugin_register(GeanyPlugin *plugin, gint api_version, gint min_api_version, gint abi_version, GeanyPluginHooks *hooks, gpointer pdata);

typedef struct _GeanyPluginHooks { /** Array of plugin-provided signal handlers (@see PluginCallback) */ PluginCallback *callbacks; /** Called when the plugin is enabled by the user */ void (*init) (GeanyPlugin *plugin, gpointer pdata); /** plugins configure dialog, optional (can be @c NULL) */ GtkWidget* (*configure) (GeanyPlugin *plugin, GtkDialog *dialog, gpointer pdata); /** Called when the plugin should show some help, optional (can be @c NULL) */ void (*help) (GeanyPlugin *plugin, gpointer pdata); /** Called when the plugin is disabled or when Geany exits */ void (*cleanup) (GeanyPlugin *plugin, gpointer pdata); } GeanyPluginHooks;

Plugins from now on define a single global function, 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.

The plugin shall pass a statically allocated instance of GeanyPluginHooks as part of the geany_plugin_register() call. The function pointers should point to function that are semantically the same as the current plugin_{init,configure,help,cleanup}. Except these function get other parameters and can make use of them.

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.

[1] https://github.com/kugel-/geany/tree/new_hooks [2] https://github.com/kugel-/geany/tree/pluxy [3] https://github.com/kugel-/peasy [4] http://lists.geany.org/pipermail/devel/2015-March/009299.html

Please reply with your options or other ideas.

Best regards

Le 26/03/2015 00:16, Thomas Martitz a écrit :

Am 20.03.2015 um 19:45 schrieb Dimitar Zhekov:

...

...

Thinking about it, if the plugin can't run because it's missing resource files required for its operation, then I think it should be treaded like incompatible plugins.

There seem like two different things to me.

The first thing a loader needs to do is query each plugin for compatibility. Incompatible plugins should not be listed at all, and it'll be good if the loader emits status messages for them in the Status tab. As an alternative, they may be listed disabled, with a message why the module can't be used.

Now, do we really want the plugins to run arbitrary resource checking code, and display their own error messages, only because they are queried (or registered, as you put it), each time the list is build?

They [incompatible plugins and plugins that are unable to function due to missing runtime deps] are different things, but the resulting action should be the same (hide it from the PM dialog, possibly give the user a hint). Hiding from the PM dialog should IMO be done before it even showed up, to avoid creating expectations that cannot be met and negative user surprise.

Yes plugins should be able to run arbitrary checking code because that's necessary for proxy plugins. Whether Geany can provide APIs to report results to a the user in a consistent manner is on another piece of paper, one that's IMO slightly out of scope for my proposal.

...

...

I'm thinking if the plugin loaded successfully, then it should be operational too.

I can check if scope.glade exists, but is it valid XML, compatible with the current version of gtk_builder? The only way to be 100% sure is to actually load it. And them immediately unload it, because Scope is being queried only, not activated. And then load it again on init...

My opinion is that if the XML isn't compatible to the current GtkBuilder version then scope shouldn't even show up in the PM dialog, because there is no way it can be functional. This is not fundamentally different to a plugin being incompatible to Geany's ABI version. So yes, that implies validating the XML in geany_plugin_register() (although loading seems excessive to me, because the XML should be considered distributed with the plugin and not to be edited by the user; at some point you have to make some assumption to keep problems managable).

If this seems so inefficient you could try to rescue the state from geany_plugin_register() to init. However keep in mind that init can be called without prior call to geany_load_module(), if the user toggles plugins in the PM dialog. And that init might not be called at all, so plugins shouldn't be wasteful with resources in geany_load_module() if might never be needed.

...

But nobody can guarantee that it exists and is valid on init. What if the Plugin manager is open, scope.glade is removed or altered, and then "[ ] Scope Debugger" is checked? It's low probability, of course, but how am I supposed to react - just crash? If init remains void, then it would be no better than the current void plugin_init(), and I'll simply check anything in load - why bother, if I *still* need to manually disable scope on initialization failure?

What do you do currently?

Besides, I didn't mean to make init() any better than the current plugin_init() w.r.t. to its semantics. The only difference is more/other parameters. Geany does not handle failing init() currently, and I don't want to change that now because I think that's out of scope for my proposal.

Well, while I can understand not extending the scope of the changes, as it introduces *new* API, it's a good time to improve things without breaking any compatibility :)

[…] I also tend to think that a failing init is misdesigned.

What do others think about this point?

Well, while I agree a plugin shouldn't fail to initialize (when the user clicks on it), I tend to agree with Dimitar: there are some things that can fail, and that you can only know whether or not they do by actually trying. Checking many things when querying the module seem wrong to me, because if we start to make e.g. extensive I/O there, it gets slow for no reason. And it also seem wrong to run anything more than strictly needed when the plugin hasn't even been activated.

E.g., I don't want to bother about performance with 250 plugins installed beside how long Geany takes to scan the SOs.

And in the end, I agree that if init() can still fail to do its job (which will be the case, unless all failable initialization is done in load_module() only), then it's better to have a way to notify the user something failed, rather than pretending it worked but having a non-functional plugin.

All this said, a counter argument is that many plugins will load external UI for configure(), and there we have no way to fail either.

So anyway, I think I'd be in favor of a failable init(), maybe like

gboolean (*init) (..., GError **error);

if we want to be able to report messages and the like. Or just let each plugin do the same msgwing_add() or whatever, but a common integrated thing might be worth it.

Regards, Colomban

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?

Actually, when I started reading the mail I thought you'd have something like this (which is basically like Matthew's proposal):

``` struct RealPlugin /* good name would be better */ { /* we may need this indeed, set by register_plugin() */ GeanyData *geany_data; /* plugin metadata */ const gchar *name; const gchar *author; const gchar *version; /* ... */ /* plugin vfuncs */ gboolean (*init) (RealPlugin *self); void (*uninit) (RealPlugin *self); GtkWidget (*configure) (RealPlugin *self, GtkDialog *dlg); /* ... */ }; ```

and that a plugin's geany_load_module() would do something like

``` 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; }; ```

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.

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.

[…] 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()?

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

With this new mechanism, proxy plugins are more than doable. geanypy should benifit as well.

Well, with your proposal, I don't get how a proxy plugins will actually register the plugins it proxies.

I would have imagined a proxy plugin would simply do something like this:

``` struct ProxyPlugin { RealPlugin parent; GList *plugins; }

struct SubPlugin { RealPlugin parent; ProxyPlugin *proxy; /* the proxy parent, if needed */ /* ... whatever state for each proxied plugin */ InterpreterData *data; }

static gboolean sub_init(SubPlugin *sub) { /* whatever to actually start the proxied plugi and alike */ interpreter_init_or_something(sub->data) interpreter_call(sub->data, "plugin_init"); }

static void sub_uninit(SubPlugin *sub) { /* stuff to shut the sub-plugin down */ interpeter_uninint(sub->data); }

proxy_plugin_init(ProxyPlugin *self) { self->plugins = NULL; foreach (proxied_plugins as pp) { struct SubPlugin *sub = g_malloc0(sizeof *sub); sub->init = sub_init; sub->uninit = sub_uninit; ...

geany_plugin_register(sub); self->plugins = g_list_append(self->plugins, sub); } }

proxy_plugin_uninit(ProxyPlugin *self) { foreach (self->plugins as sub) { geany_plugin_unregister(sub); free_sub_plugin(sub); } g_list_free(self->plugins); }

geany_load_module(...) { static struct ProxyPlugin self = { ... proxy_plugin_init, proxy_plugin_uninit, ... .list = NULL };

geany_plugin_register(&self); } ```

or something around these lines. But with the current status, I can't seem to understand how the proxy plugin could forge things like GeanyPlugin and the like?

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?

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.

Regards, Colomban

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.

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

Re-sending as I don't see my email in https://lists.geany.org/cgi-bin/mailman/listinfo/devel

Thanks. I didn't realize this. If you have a chance, maybe you could spot check my plugins:

https://github.com/sblatnick/geany-plugins

quick-* plugins external-tools tab-utils hide-menu

I worry most of them have at non-api calls. Is there an easy way to tell what is API and what is not? I primarily had been just grepping the code for what I needed.

Also, since I'm on the topic of how to write plugins currently, can anyone suggest a good tutorial for adding plugins to the geany-plugins build system? I think that may be the biggest thing keeping me from adding my plugins to geany-plugins.

I apologize if I missed replying to anyone since there have been a lot of emails going around about plugins and I may have missed other references to mine.

Thanks,

Steve

On 03/29/2015 09:51 AM, Colomban Wendling wrote:

Hi,

Le 18/03/2015 18:11, Steven Blatnick a écrit :

...

On 03/18/2015 10:42 AM, Thomas Martitz wrote:

...

Currently geany exports a pointer to a struct, that contains more structs, which contain function points to the API functions. Fortunately this is nicely hidden to developers via macros. But due to gtkbuilder all functions and nothing prevents plugins from accessing these. And the macros are awkward and strange anyway. There is currently the linkage-cleanup PR in the works which improves this by actually exporting the API functions, and _only_ the API functions to plugins.

Maybe I'm completely wrong on this from an architecture perspective, but part of what I like about writing plugins for geany is accessibility. If we only get access to a subset of functions, then it seems less flexible what our plugins can actually do. Yes, this allows us to write bad plugins that can do some sloppy things, but I say "so what". They are plugins. […]

In addition to what Thomas said (which is very true), realize two things:

1. plugins that use functions not part of the API won't work e.g. on

Windows (for technical reasons, all functions are currently actually usable under *NIX, but on Windows only explicitly exported ones are). So if you care about your plugin working on Windows you'll stick to the official API anyway (the one we commit to and maintain).

2. before Geany 1.22, you couldn't use non-API anyway. If you were

happy with the API before, you'll still be after this change.

Regards, Colomban _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel

Am 30.03.2015 um 23:07 schrieb Thomas Martitz:

Am 30.03.2015 um 14:57 schrieb Colomban Wendling:

...

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.

I see what you mean. Indeed, this could be a problem.

I wanted the pdata parameter such that it is friendly to language bindings. This doesn't work if the user_data is hidden down in some other structs passed as parameter. For example, I wanted to make it possible to use vala member functions classes directly as plugin hooks. And this works, however there is indeed a leak. I have a prototype, see below.

Considering this use case I would rather take the GDestroyNotify than hiding down pdata in another param. Passing it to geany_plugin_register() also allows for using a member function for the init() hook already.

What do you think?

Replying to self after having thought about it a bit more.

Since I do want to maintain the ability to set the data in geany_load_module(), or rather just before the plugin's init() (to enable the use case cited below), but also at the same time still allow it to be also set in the plugin's init() (some plugins don't want expensive allocs before init()) AND definitely pass the pointer as a separate parameter, the only think I came up with is a separate API function to set the data, including the destroy notify. So I have now:

geany_plugin_register(GeanyPlugin *, gint api, gint min_api, gint abi, GeanyPluginHooks *); /* pdata param is removed */ geany_plugin_set_data(GeanyPlugin *, gpointer pdata, GDestroyNotify free_func);

geany_plugin_set_data() can be called before the plugin's init, or within, or any later. It'll set the data that is passed to all the hooks (NULL is passed if geany_plugin_set_data() was never called). The GDestroyNotify is called before the plugin is unloaded. Most importantly it is also called if geany_plugin_set_data() was called before init() and the plugin did not became activated at all.

I have successfully prototyped that the below Vala example can work without leaks using the separate API function:

- p.register(Geany.API_VERSION, 224, Geany.ABI_VERSION, - ref hooks, new Tester() /* LEAK */); + p.register(Geany.API_VERSION, 224, Geany.ABI_VERSION, ref hooks); + p.set_data(new Tester() /* no leak, g_object_unref is automatically passed as destroy notify */);

Is that sound?

Best regards

Appendix: The prototype is like this:

using Geany;

class Tester { public void init(Plugin p) { }

public void help(Plugin p) { /* shows a dialog */ }

public void cleanup(Plugin p) { }

}

private PluginHooks hooks; public bool geany_load_module(Plugin p, GLib.Module mod, int geany_api_ver) { hooks.init = Tester.init; hooks.help = Tester.help; hooks.cleanup = Tester.cleanup; p.register(Geany.API_VERSION, 224, Geany.ABI_VERSION, ref hooks, new Tester() /* LEAK */);

mod.make_resident();

/* ... */
return true;

}

---

Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel