GNOME Bugzilla – Bug 759295
Get rid of XML plugins descriptors
Last modified: 2015-12-15 16:33:09 UTC
0.2.x introduced a XML describing the plugins: author, name, description, and so on. It was introduced also with the idea of adding more functionality in the XML in a declarative way, without needing to code in C. But at the end it didn't work, and now it is most a pain to require both the .so module plus the .xml. So let's remove the XML descriptor and stick with the module file only, like in GStreamer.
Created attachment 317102 [details] [review] core: add GRL_PLUGIN_DEFINE() Defines a new plugin that can be loaded later. It adds all the fields that usually go in the XML file.
Created attachment 317103 [details] [review] core: Add GrlPluginDescriptor in GrlPluginPrivate Integrate the plugin descriptor inside private class.
Created attachment 317104 [details] [review] core: remove optional info in plugins Set only a restricted set of information in plugins.
Created attachment 317105 [details] [review] core: split plugin loading from plugin activation Explicitly separate the loading plugin process from activation process. Thus we can load several plugins from different places, and activate all of them together. Loading the plugin will run the plugin's registering function, while activating will run the plugin's activation function.
Created attachment 317106 [details] [review] all: Get rid of XML plugin definitions Use the new GRL_PLUGIN_DEFINE() to create the plugins.
Created attachment 317107 [details] [review] all: explicitly activate plugins Use the new API to explicitly activate the plugins when required.
Let's explain all patches pushed. The former 4 patches apply to grilo core, while the other two apply to grilo plugins. The changes are inspired by GStreamer. The first 3 patches basically introduces GRL_PLUGIN_DEFINE() macro (replaces GRL_PLUGIN_REGISTER). This macro allows to define a plugin, allowing to specify all the information that was previously defined in the XML: name, description, author, version, etc. It also declares the function used to register the keys, and the major and minor version required from grilo core. This will allow in future to check for compatibility in plugins. The 4th patch arised while working on the other two. So far, when you load a plugin, automatically becomes active, registering all sources available. But sometimes we could require to first load all plugins, and only after all plugins are loaded activate them. Reason is because in the loading process we want to register custom keys, and then at the end start the sources. So the sources will have all the required keys. This was working fine when loading all plugins: the process itself loaded first all plugins, and afterwards start all of them. But this doesn't work if we want to load plugins manually. For instance, if we want to load two different plugins, it will load and activate the first, and load and activate the second. What this patch do is precisely break this two process: grl_registry_load_plugin[_foo]() will only load the plugins (running the key register funcion), but not activate them. Developer can activate them (activate_all_plugins() or activate_plugin_by_id()) after all required plugins are loaded. For simplicity, grl_registry_load_all_plugins() has a boolean that can be used to activate automaticatlly all plugins after they are loaded. The patches for grilo plugins basically use GRL_PLUGIN_DEFINE() macro, and also explicitly activate the plugins when required.
Review of attachment 317102 [details] [review]: Looks fine otherwise. ::: bindings/vala/grilo-0.3.vapi @@ +811,3 @@ public delegate void OperationCancelCb (void* data); + [CCode (cheader_filename = "grilo.h", has_target = false)] + public delegate void PluginDeinitFunc (Grl.Plugin plugin); Doesn't look related to the rest of the patch.
Review of attachment 317103 [details] [review]: ::: bindings/vala/grilo-0.3.vapi @@ -330,3 @@ public void set_id (string id); public void set_info (string key, string value); - public void set_load_func (void* load_function); Isn't that changes from the previous patch? ::: src/grl-plugin.c @@ +517,3 @@ { g_return_val_if_fail (GRL_IS_PLUGIN (plugin), NULL); + White space change.
Review of attachment 317104 [details] [review]: Looks fine as well.
Review of attachment 317105 [details] [review]: Sure.
Review of attachment 317106 [details] [review]: That all looks fine to me.
Review of attachment 317107 [details] [review]: Looks good.
When I committed the patches from bug 747026, I unfortunately didn't add a test case for the builtin plugins. I think that it would still be fine to commit all this and get a release out, and I will update my local Totem branch which uses the feature, and create a test case for it, and fix whatever problem I see.
(In reply to Bastien Nocera from comment #8) > Review of attachment 317102 [details] [review] [review]: > > Looks fine otherwise. > > ::: bindings/vala/grilo-0.3.vapi > @@ +811,3 @@ > public delegate void OperationCancelCb (void* data); > + [CCode (cheader_filename = "grilo.h", has_target = false)] > + public delegate void PluginDeinitFunc (Grl.Plugin plugin); > > Doesn't look related to the rest of the patch. GrlPluginDeinitFunc is defined in this patch (grl-registry.h:125).
(In reply to Bastien Nocera from comment #9) > Review of attachment 317103 [details] [review] [review]: > > ::: bindings/vala/grilo-0.3.vapi > @@ -330,3 @@ > public void set_id (string id); > public void set_info (string key, string value); > - public void set_load_func (void* load_function); > > Isn't that changes from the previous patch? > Correct. > ::: src/grl-plugin.c > @@ +517,3 @@ > { > g_return_val_if_fail (GRL_IS_PLUGIN (plugin), NULL); > + > > White space change.
(In reply to Juan A. Suarez Romero from comment #16) > (In reply to Bastien Nocera from comment #9) > > Review of attachment 317103 [details] [review] [review] [review]: > > > > ::: bindings/vala/grilo-0.3.vapi > > @@ -330,3 @@ > > public void set_id (string id); > > public void set_info (string key, string value); > > - public void set_load_func (void* load_function); > > > > Isn't that changes from the previous patch? > > > > Correct. > Speaking too fast.... Changes in the vapi file were generated automatically due changes in C code. In this case, those changes are added automatically from this patch.
(In reply to Bastien Nocera from comment #9) > ::: src/grl-plugin.c > @@ +517,3 @@ > { > g_return_val_if_fail (GRL_IS_PLUGIN (plugin), NULL); > + > > White space change. Added on purpose to keep consistency with the other functions.
Attachment 317102 [details] pushed as 222beef - core: add GRL_PLUGIN_DEFINE() Attachment 317103 [details] pushed as d1c2975 - core: Add GrlPluginDescriptor in GrlPluginPrivate Attachment 317104 [details] pushed as e0d59b3 - core: remove optional info in plugins Attachment 317105 [details] pushed as 9c47636 - core: split plugin loading from plugin activation
Attachment 317106 [details] pushed as b4ec244 - all: Get rid of XML plugin definitions Attachment 317107 [details] pushed as 697caae - all: explicitly activate plugins