GNOME Bugzilla – Bug 748550
Unregister custom keys on grl_registry_unload_plugin
Last modified: 2015-09-01 13:42:03 UTC
We shouldn't keep custom keys on registry if the plugin that created them (or can handled them) were unload. Not very trivial as current registry api doesn't map the custom-key to the source or map to whom can resolve them. Possible solutions should be automatically unload/unref custom-key on unload_plugin or provide api on registry that should be called by plugins on deinit
I don't think it's a problem to keep the keys alive, they shouldn't call into any functions or data inside the plugin itself, so will be kept usable. It also avoids problems when 2 different plugins register the same keys. I would just make sure that there's no warnings if an already registered key exists.
Created attachment 302478 [details] [review] grl-registry: don't warn on already registered key On grl_registry_register_metadata_key returns the key if they were already registered.
(In reply to Bastien Nocera from comment #1) > I don't think it's a problem to keep the keys alive, they shouldn't call > into any functions or data inside the plugin itself, so will be kept usable. > It also avoids problems when 2 different plugins register the same keys. > > I would just make sure that there's no warnings if an already registered key > exists. Okay so we are not able to register the same key but if this key is already register is it okay to return it as I did in my PATCH? This is a behavior change on registry API and may benefit a few plugins like, for instance, both grl-tmdb and grl-thevdb implements imdb-id under thetvdb-imdb-id and tmdb-imdb-id.. we could combine this as 'imdb-id' (because is not specific to any of this sources). What do you think about this?
Review of attachment 302478 [details] [review]: ::: src/grl-registry.c @@ +834,3 @@ } else if ( GRL_METADATA_KEY_INVALID != key_id_handler_get_key (handler, name)) { /* _key or name is already in use! */ + GRL_DEBUG ("Cannot register %d:%s because name is already registered with key %d", I would keep it a GRL_WARNING, but only throw an error if any of its details changed. For example, different range, different type, etc. Might be best to split code into a separate function though.
(In reply to Victor Toso from comment #3) > (In reply to Bastien Nocera from comment #1) > > I don't think it's a problem to keep the keys alive, they shouldn't call > > into any functions or data inside the plugin itself, so will be kept usable. > > It also avoids problems when 2 different plugins register the same keys. > > > > I would just make sure that there's no warnings if an already registered key > > exists. > > Okay so we are not able to register the same key but if this key is already > register is it okay to return it as I did in my PATCH? > > This is a behavior change on registry API and may benefit a few plugins > like, for instance, both grl-tmdb and grl-thevdb implements imdb-id under > thetvdb-imdb-id and tmdb-imdb-id.. we could combine this as 'imdb-id' > (because is not specific to any of this sources). > > What do you think about this? That would be fine by me, but you'd need to be sure that key types matched, hence the comment in comment 4.
Created attachment 302737 [details] [review] grl-registry: allow register same metadata key In case the plugin wants to reregister a metadata-key we don't throw an error. Common situation for this is when application unload() and load() again a plugin with custom metadata keys.
Review of attachment 302737 [details] [review]: ::: src/grl-registry.c @@ +912,3 @@ + } + + return (*err_msg == NULL); Hmm, this isn't super useful. Descriptions being different isn't a problem to running the plugins, the name is obviously not different (otherwise we wouldn't compare them). The nick isn't used at all. When I meant comparing the GParamSpec, I meant comparing: - the types - for numerical values, comparing the ranges (min/max values) - for enums, that they match the same enum type etc.
Created attachment 310030 [details] [review] grl-registry: allow register same metadata key In case the plugin wants to reregister a metadata-key we don't throw an error. Common situation for this is when application unload() and load() again a plugin with custom metadata keys.
Review of attachment 310030 [details] [review]: ::: src/grl-registry.c @@ +911,3 @@ +/* @curr: The current spec we have + * @new: The spec to match + * @err_msg: a string with what differs between both specs or NULL if equal Need to update the comment (remove @err_msg line)
Review of attachment 310030 [details] [review]: ::: src/grl-registry.c @@ +886,3 @@ + * GParamSpec this rounding issues should be fine and equal to both values */ +static gboolean +numeric_limits_are_equal (gdouble max1, gdouble min1, gdouble default1, You'll get into trouble if you cast large values to gdouble, certainly. I would do a version for uint64, int64 and gdouble (would be fine to implement as a macro though). @@ +925,3 @@ + + if (ctype == G_TYPE_PARAM_INT) { + GParamSpecInt *c = G_PARAM_SPEC_INT (cur); Pretty sure all this could be implemented as a macro, so all the checks are one-liners, and you won't need to do "else if" because going into the branch will return.
Created attachment 310412 [details] [review] grl-registry: allow register same metadata key In case the plugin wants to reregister a metadata-key we don't throw an error. Common situation for this is when application unload() and load() again a plugin with custom metadata keys.
Created attachment 310415 [details] [review] grl-registry: allow register same metadata key In case the plugin wants to reregister a metadata-key we don't throw an error. Common situation for this is when application unload() and load() again a plugin with custom metadata keys.
Review of attachment 310412 [details] [review]: Looks good.
Review of attachment 310415 [details] [review]: Looks good.
Attachment 310415 [details] pushed as 9971e34 - grl-registry: allow register same metadata key