GNOME Bugzilla – Bug 604742
Add more properties to PeasPluginInfo
Last modified: 2019-03-23 20:34:36 UTC
Note the few FIXMEs in the code.
Created attachment 149843 [details] [review] Add more properties to PeasPluginInfo Add a _get/_set_visible for applications to hide some plugins. Add _get_keys to get a list of arbitrary keys from the plugin's information keyfile. Don't check for IAge inside the plugin engine, allow applications to do it instead.
Created attachment 149861 [details] [review] Avoid warning when disposing of a PeasPluginInfo
Review of attachment 149843 [details] [review]: I don't really get the planned usage for the "visible" flag, as it is not derived from the plugin info. As I understand it, you are planning to read an "extra" key in totem for autoloaded plugins, and set the visibility afterwards. But I'm not sure I really like this approach, it looks a bit ad-hoc. What would you think about implementing autoloading at the libpeas level? You could then add support for a "visible" and an "autoload" key, and autoload=true would imply visible=false? The engine could then activate all the plugins after syncing the info files. Extra-keys support is also a nice addition, but I'd prefer to see it as a separate patch if you don't mind ::: libpeas/peas-plugin-info-priv.h @@ +49,3 @@ + int iage; + gboolean visible; + GHashTable *keys; Style: struct item names are usually not aligned within the libpeas source code ::: libpeas/peas-plugin-info.c @@ +95,3 @@ +static void +_value_free (gpointer data) _ prefixes are usually for extern functions that should not be exported by the library. @@ +107,3 @@ + GKeyFile *plugin_file, + const gchar *section_header, + const gchar **keys) Style: the arguments name should be aligned. Same remark about the '_' prefix, you can just drop it. @@ +111,3 @@ + guint i; + + for (i = 0; keys[i] != NULL; i++) { Style: libpeas uses gtk+/gnu style for {} @@ +121,3 @@ + g_str_equal (keys[i], "Loader") || + g_str_equal (keys[i], "Name") || //FIXME i18n + g_str_equal (keys[i], "Description") || //FIXME i18n Doesn't GKeyFile handle localized keys transparently ? i.e. you will only get a Name key even if you are actually using Name[fr] won't you?
One more comment, about IAge this time: as every application are going to check for IAge, maybe it should be a property of the engine so the test can be done automatically or something?
(In reply to comment #3) > Review of attachment 149843 [details] [review]: > > I don't really get the planned usage for the "visible" flag, as it is not > derived from the plugin info. No, it's get and set by the application itself, thus it could choose to have all but one plugin visible, or only make certain plugins builtin, or god knows what. > As I understand it, you are planning to read an > "extra" key in totem for autoloaded plugins, and set the visibility afterwards. Yep. > But I'm not sure I really like this approach, it looks a bit ad-hoc. The extra keys is certainly a way to make sure that extending the code is achievable without needing a new version of libpeas. > What would you think about implementing autoloading at the libpeas level? You > could then add support for a "visible" and an "autoload" key, and autoload=true > would imply visible=false? The engine could then activate all the plugins after > syncing the info files. Given that the current code has no idea about GConf, or any other storage backends to save the list of activated plugins, I'm not sure that's a good idea. > Extra-keys support is also a nice addition, but I'd prefer to see it as a > separate patch if you don't mind Mkay. > @@ +121,3 @@ > + g_str_equal (keys[i], "Loader") || > + g_str_equal (keys[i], "Name") || //FIXME i18n > + g_str_equal (keys[i], "Description") || //FIXME i18n > > Doesn't GKeyFile handle localized keys transparently ? i.e. you will only get a > Name key even if you are actually using Name[fr] won't you? Nope, certainly not transparent. FWIW, maybe it would be easier to load _all_ the keys in _peas_plugin_info_new() and remove them as they're read. That would avoid all the pointless string comparisons and check from a hash table instead. (In reply to comment #4) > One more comment, about IAge this time: as every application are going to check > for IAge, maybe it should be a property of the engine so the test can be done > automatically or something? I would expect a lot of code not to care, especially new code that didn't cut'n'paste from gedit/rhythmbox/totem/etc. :)
Created attachment 149903 [details] [review] Add more properties to PeasPluginInfo Add a _get/_set_visible for applications to hide some plugins. Add _get_keys to get a list of arbitrary keys from the plugin's information keyfile. Don't check for IAge inside the plugin engine, allow applications to do it instead.
Created attachment 149904 [details] [review] Add more properties to PeasPluginInfo Add a _get/_set_visible for applications to hide some plugins. Add _get_keys to get a list of arbitrary keys from the plugin's information keyfile. Don't check for IAge inside the plugin engine, allow applications to do it instead.
So 2 things: - I didn't add a check for iage in the engine. Half-way through implementing it I thought that most applications either wouldn't use it, or would like to support multiple versions of the iage. I can imagine applications adding support for new features and supporting both the old one and the new one for a few releases for example. - I didn't make any changes to get/set visible, as I still think it's a good idea to have it handled by applications and not by the engine.
Review of attachment 149904 [details] [review]: (In reply to comment #5) > Given that the current code has no idea about GConf, or any other storage > backends to save the list of activated plugins, I'm not sure that's a good > idea. The plan is to integrate with GSettings at some point in there, or make it easy to use GSettings with libpeas. It explains why we use a gchar ** in peas_engine_get/set_active_plugins rather than a GList like it was the case previously. GConf was explicitely not included because it's an external dependency (to glib/gtk+) that's going to be deprecated anyway. I'm still unsure about having PeasPluginInfo store information that does not come from the key file, and still think autoload belongs somehow to libpeas (actually I planned to implement this at some point to make libpeas more suitable for rb and totem). But it can still go in this way (it's still possible to change it afterwards anyway) if you feel like the patch's current way of doing things is closer from what you need. > > Doesn't GKeyFile handle localized keys transparently ? i.e. you will only get a > > Name key even if you are actually using Name[fr] won't you? > > Nope, certainly not transparent. FWIW, maybe it would be easier to load _all_ > the keys in _peas_plugin_info_new() and remove them as they're read. That would > avoid all the pointless string comparisons and check from a hash table instead. Yes I tested it and you get both Name and Name[fr] in the key list, so to ignore those keys you could use g_str_has_prefix("Name[") for instance. But I also feel like all the strcmp()/g_str_has_prefix() look bad/inefficient but is it possible to get rid of them? ::: libpeas/peas-plugin-info.c @@ +105,3 @@ + +static void +parse_extra_keys (PeasPluginInfo *info, The {} in this function are not in the right style: @@ +113,3 @@ + + for (i = 0; keys[i] != NULL; i++) { + GValue *value = NULL; > for (i = 0; keys[i] != NULL; i++) > { > GValue *value = NULL etc.
(In reply to comment #9) > Review of attachment 149904 [details] [review]: > > (In reply to comment #5) > > Given that the current code has no idea about GConf, or any other storage > > backends to save the list of activated plugins, I'm not sure that's a good > > idea. > > The plan is to integrate with GSettings at some point in there, or make it easy > to use GSettings with libpeas. It explains why we use a gchar ** in > peas_engine_get/set_active_plugins rather than a GList like it was the case > previously. GConf was explicitely not included because it's an external > dependency (to glib/gtk+) that's going to be deprecated anyway. OK. > I'm still unsure about having PeasPluginInfo store information that does not > come from the key file, and still think autoload belongs somehow to libpeas > (actually I planned to implement this at some point to make libpeas more > suitable for rb and totem). But it can still go in this way (it's still > possible to change it afterwards anyway) if you feel like the patch's current > way of doing things is closer from what you need. We can always rework the code when the dconf support lands. > > > Doesn't GKeyFile handle localized keys transparently ? i.e. you will only get a > > > Name key even if you are actually using Name[fr] won't you? > > > > Nope, certainly not transparent. FWIW, maybe it would be easier to load _all_ > > the keys in _peas_plugin_info_new() and remove them as they're read. That would > > avoid all the pointless string comparisons and check from a hash table instead. > > Yes I tested it and you get both Name and Name[fr] in the key list, so to > ignore those keys you could use g_str_has_prefix("Name[") for instance. But I > also feel like all the strcmp()/g_str_has_prefix() look bad/inefficient but is > it possible to get rid of them? We could load all the keys into a hashtable to make lookup less expensive, but we'd still need to do manual string comparisons for Name and Description. > ::: libpeas/peas-plugin-info.c > @@ +105,3 @@ > + > +static void > +parse_extra_keys (PeasPluginInfo *info, > > The {} in this function are not in the right style: OK.
Commited a modified version of patch 149904, to fit coding style.