After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 626644 - It's difficult to use plugins that implement multiple interfaces
It's difficult to use plugins that implement multiple interfaces
Status: RESOLVED FIXED
Product: libpeas
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: libpeas-maint
libpeas-maint
Depends on:
Blocks:
 
 
Reported: 2010-08-11 16:56 UTC by Chris Lord
Modified: 2011-11-19 18:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Allow multiple interfaces for all extensions (67.86 KB, patch)
2011-09-27 11:20 UTC, Garrett Regier
reviewed Details | Review
Allow multiple interfaces for all extensions v2 (62.90 KB, patch)
2011-10-27 23:41 UTC, Garrett Regier
none Details | Review

Description Chris Lord 2010-08-11 16:56:56 UTC
When creating an extension for a loaded plugin, libpeas only allows to specify a single interface type. This is quite limiting for the use-case where your plugin implements multiple interfaces.

For example, let's say you have an interface 'Information' that retrieves information about an object (let's say this information is dynamic too, so you can't specify it in the plugin info)

Let's say you have a second interface 'InformationList', which allows iteration over a number of these 'Information' objects.

You could optionally implement these interfaces to represent a hierarchical data structure (like a scene-graph, or an xml tree).

These are two interface that you'd want to use together - you'd want one object that you definitely want to be able to retrieve information on, and if it implements the InformationList interface, you'd want to be able to use that on the same object too.

This doesn't seem possible with the current API, which mandates that a plugin object instance only implement a single interface. You could remedy this by having an interface that just had a 'GetObject' method, but this seems a bit counter-intuitive (libpeas would be quite overkill for such a simple extension method).
Comment 1 Steve Frécinaux 2011-01-25 10:48:19 UTC
For the record,
.


<nud> Cwiiis, for you, are those multiple-interface-plugins single instance?
<Cwiiis> nud: Yes, we have various provider interfaces, so a plugin can implement multiple providers - but you only have one instance of the plugin object
<nud> ok
<nud> so basically, choosing between single-instance or multiple-instance for a plugin would be enough
<nud> if it is single instance and implements multiple ifaces it will work just fine
<nud> it's actually quite easy to achieve that in C right now
<nud> Cwiiis, what language were you using? python or javascript?
<Cwiiis> nud: C :)
<Cwiiis> It would've been really nice to have JavaScript plugin support though
<nud> Cwiiis, oh, but then you choked on a non-issue :-)
<nud> you'd just have to use peas_object_module_register_extension_factory() with a your_extension_get_default()-like callback
<nud> we could have a register_extension_singleton or something which would do the same

I think it's possible to do so by only using python as well (do that using an override ?). I guess we'd need some more development to do to support this way of doing thing...
Comment 2 Steve Frécinaux 2011-01-25 10:49:17 UTC
> I guess we'd need some more development to do to support this way
> of doing thing...

I meant for javascript.
Comment 3 Michal Hruby 2011-02-15 17:29:41 UTC
I'd like to see this too, I think this is trying to achieve a sort of optional inheritance, and therefore the API for this could be something like:

peas_engine_create_extension_subclassable (PeasEngine, PeasPluginInfo, GType extension_base_type, GType* optional_interface_types, guint n_optional_interface_types, gchar* first_prop, ...)

(Yea, _subclassable is not a good name, pick whatever you like)

To elaborate more, my interface looks like this:
interface Match
{
  MatchType type;
  string title;
  string description;
}

interface UriMatch : Match
{
  string uri;
}

interface ApplicationMatch : Match
{
  AppInfo app_info;
}

// note that this interface is more generic, it doesn't have Match as a prerequisite
interface Searchable
{
  List get_similar_matches ();
}

Now an object might just implement the base Match class and it's displayable in the UI, but it can also (optionally) implement any or all of the other interfaces and then it's easy for the code to perform actions on the UI item. And of course you always want just one instance of this object, not one per interface.
Comment 4 Michal Hruby 2011-02-15 17:35:49 UTC
Note that the same could be used for a single instance Activatable + Configurable. You'd just ask for Activatable extension that optionally implements Configurable. Of course this would need a way to check that the c/python/seed object implements an interface before the extension object is instantiated, but that should be doable, right?
Comment 5 Garrett Regier 2011-09-24 19:20:39 UTC
I think the thing to do for this is to just implement all of the interfaces and then if the application wants to use an optional interface they can do a NAMESPACE_IS_BLAH(extension) to check that the optional interface is implemented by that extension.

This currently works in the multi-iface branch on my github.
Comment 6 Garrett Regier 2011-09-27 11:20:40 UTC
Created attachment 197555 [details] [review]
Allow multiple interfaces for all extensions

Instead of only implementing the required interface implement
every interface that type implements. This way applications can check for optional interfaces with NAMESPACE_IS_BLAH(exten) and use the extra interfaces.
Comment 7 Steve Frécinaux 2011-10-05 20:04:00 UTC
Review of attachment 197555 [details] [review]:

Patch looks good, the idea is there.

::: libpeas/peas-extension-subclasses.c
@@ +82,2 @@
       else
+        arguments[i].v_pointer = *((gpointer **) args[i + 1]);

I don't understand this change of offsets. Where does it come from?

::: libpeas/peas-extension.c
@@ +96,3 @@
+
+  if (!PEAS_IS_EXTENSION_WRAPPER (exten))
+    g_free (interfaces);

For clarity, I'd prefer to have a gboolean must_free_interfaces variable set at the same time you set interfaces.

::: libpeas/peas-introspection.c
@@ +312,3 @@
   GIRepository *repo;
   GIBaseInfo *iface_info;
+  GIFunctionInfo *method_info;

I don't like when you rename stuff in a big patch just for the sake of renaming stuff ;-)

@@ +343,3 @@
+/* Only for interfaces! */
+GType
+peas_gi_get_type_from_name (const gchar *type_name)

Where is this method used? Only in the seed loader?

@@ +460,3 @@
+  g_return_val_if_fail (G_TYPE_CHECK_INSTANCE_TYPE (instance, iface_type),
+                        FALSE);
+  g_return_val_if_fail (method_name != NULL, FALSE);

Adding this should have been a separate patch.

::: loaders/c/peas-plugin-loader-c.c
@@ +147,3 @@
   /* As we do not instantiate a PeasExtensionWrapper, we have to remember
    * somehow which interface we are instantiating, to make it possible to use
+   * the deprecated peas_extension_get_extension_type() method.

This should be a separate patch. Feel free to commit this immediately.

::: loaders/gjs/peas-extension-gjs.c
@@ +230,2 @@
   /* Return value is an out arg */
+  g_callable_info_load_return_type (method_info, &arg_cache[0].type_info);

Yet another free renaming. Those really make a patch hard to read. Could you please avoid such renamings in big patches?

::: tests/libpeas/plugins/extension-c/extension-c-plugin.c
@@ +3,3 @@
  * This file is part of libpeas
  *
+ * Copyright (C) 2011 Garrett Regier

Is the change on this last line really needed ? ;-)

::: tests/libpeas/plugins/extension-js/extension-js.js
@@ +29,3 @@
   "IntrospectionCallable": callable_extension,
+  "IntrospectionProperties": properties_extension,
+  "IntrospectionHasPrerequisite": callable_extension

I can't help but think extra optional interfaces should be defined in the extension itself as a _implements: member or something, as we should probably not allow creating an extension with a supporting type instead of the main, prerequisite-less, one.
Comment 8 Garrett Regier 2011-10-27 23:41:39 UTC
Created attachment 200149 [details] [review]
Allow multiple interfaces for all extensions v2

(In reply to comment #7)
> Review of attachment 197555 [details] [review]:
> 
> Patch looks good, the idea is there.
> 
> ::: libpeas/peas-extension-subclasses.c
> @@ +82,2 @@
>        else
> +        arguments[i].v_pointer = *((gpointer **) args[i + 1]);
> 
> I don't understand this change of offsets. Where does it come from?

Because we are using an invoker info and not a callback info the param offsets have changed.

> ::: libpeas/peas-extension.c
> @@ +96,3 @@
> +
> +  if (!PEAS_IS_EXTENSION_WRAPPER (exten))
> +    g_free (interfaces);
> 
> For clarity, I'd prefer to have a gboolean must_free_interfaces variable set at
> the same time you set interfaces.

Done

> ::: libpeas/peas-introspection.c
> @@ +312,3 @@
>    GIRepository *repo;
>    GIBaseInfo *iface_info;
> +  GIFunctionInfo *method_info;
> 
> I don't like when you rename stuff in a big patch just for the sake of renaming
> stuff ;-)

Fixed

> @@ +343,3 @@
> +/* Only for interfaces! */
> +GType
> +peas_gi_get_type_from_name (const gchar *type_name)
> 
> Where is this method used? Only in the seed loader?

It is used by GJS and Seed

> @@ +460,3 @@
> +  g_return_val_if_fail (G_TYPE_CHECK_INSTANCE_TYPE (instance, iface_type),
> +                        FALSE);
> +  g_return_val_if_fail (method_name != NULL, FALSE);
> 
> Adding this should have been a separate patch.

Too lazy

> ::: loaders/c/peas-plugin-loader-c.c
> @@ +147,3 @@
>    /* As we do not instantiate a PeasExtensionWrapper, we have to remember
>     * somehow which interface we are instantiating, to make it possible to use
> +   * the deprecated peas_extension_get_extension_type() method.
> 
> This should be a separate patch. Feel free to commit this immediately.

Only valid once this patch gets in, I like to keep comments up to date.

> ::: loaders/gjs/peas-extension-gjs.c
> @@ +230,2 @@
>    /* Return value is an out arg */
> +  g_callable_info_load_return_type (method_info, &arg_cache[0].type_info);
> 
> Yet another free renaming. Those really make a patch hard to read. Could you
> please avoid such renamings in big patches?

Fixed

> ::: tests/libpeas/plugins/extension-c/extension-c-plugin.c
> @@ +3,3 @@
>   * This file is part of libpeas
>   *
> + * Copyright (C) 2011 Garrett Regier
> 
> Is the change on this last line really needed ? ;-)

This is actually a different file and the patch just looks really weird.

> ::: tests/libpeas/plugins/extension-js/extension-js.js
> @@ +29,3 @@
>    "IntrospectionCallable": callable_extension,
> +  "IntrospectionProperties": properties_extension,
> +  "IntrospectionHasPrerequisite": callable_extension
> 
> I can't help but think extra optional interfaces should be defined in the
> extension itself as a _implements: member or something, as we should probably
> not allow creating an extension with a supporting type instead of the main,
> prerequisite-less, one.

That would cause some complications. It would work if you only wanted to implement optional interfaces always, but if the extension might be created as a AInterface or BInterface and it must implement both {A,B}Interface then it would cause some problems.
Comment 9 Steve Frécinaux 2011-11-19 18:25:42 UTC
(In reply to comment #8)
> > @@ +460,3 @@
> > +  g_return_val_if_fail (G_TYPE_CHECK_INSTANCE_TYPE (instance, iface_type),
> > +                        FALSE);
> > +  g_return_val_if_fail (method_name != NULL, FALSE);
> > 
> > Adding this should have been a separate patch.
> 
> Too lazy

Best reason ever.

> > ::: loaders/c/peas-plugin-loader-c.c
> > @@ +147,3 @@
> >    /* As we do not instantiate a PeasExtensionWrapper, we have to remember
> >     * somehow which interface we are instantiating, to make it possible to use
> > +   * the deprecated peas_extension_get_extension_type() method.
> > 
> > This should be a separate patch. Feel free to commit this immediately.
> 
> Only valid once this patch gets in, I like to keep comments up to date.

Ok.

> > ::: tests/libpeas/plugins/extension-js/extension-js.js
> > @@ +29,3 @@
> >    "IntrospectionCallable": callable_extension,
> > +  "IntrospectionProperties": properties_extension,
> > +  "IntrospectionHasPrerequisite": callable_extension
> > 
> > I can't help but think extra optional interfaces should be defined in the
> > extension itself as a _implements: member or something, as we should probably
> > not allow creating an extension with a supporting type instead of the main,
> > prerequisite-less, one.
> 
> That would cause some complications. It would work if you only wanted to
> implement optional interfaces always, but if the extension might be created as
> a AInterface or BInterface and it must implement both {A,B}Interface then it
> would cause some problems.

Yeah, I was more thinking about replacing the extensions dict with a list at the same time. Can probably be done later anyway as we would still need to support dicts.
Comment 10 Garrett Regier 2011-11-19 18:46:30 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.