Re: [PATCH 3/6] core: made plugin registry handle itself key id assignments

[Guillaume Emont <guijemont igalia com>]

On 12/05/2011 12:25, Iago Toral Quiroga wrote:
> El mié, 11-05-2011 a las 20:31 +0200, Guillaume Emont escribió:
>> Instead of relying on GQuarks, a similar mechanism is implemented in the plugin
>> registry. This gives more flexibility and allows  to #define the values of core
>> keys, which automatically give their symbols in GObject-introspection bindings.
> 
> Not sure I follow this properly, can you elaborate on the benefits we
> obtain with this solution for the introspection based bindings?
> 
In a nutshell, that is the simplest solution I found to have symbols of
the type Grl.METADATA_KEY_FOO available in bindings: for that, we need
to have the GRL_METADATA_KEY_FOO values #define'd to constants, else the
GI scanner won't know how to handle them, and they won't appear in bindings.
If we were to use GQuarks, then the GRL_METADATA_KEY_FOO identifiers
need to either be global variables, or stuff like:
#define GRL_METADATA_KEY_FOO g_quark_from_string("foobar)
In both cases, they won't be visible in GI bindings.

>> ---
>>  src/grl-plugin-registry.c |  229 +++++++++++++++++++++++++++++++--------------
>>  src/grl-plugin-registry.h |    8 ++-
>>  2 files changed, 166 insertions(+), 71 deletions(-)
>>
>> diff --git a/src/grl-plugin-registry.c b/src/grl-plugin-registry.c
>> index eca8b86..cdebebf 100644
>> --- a/src/grl-plugin-registry.c
>> +++ b/src/grl-plugin-registry.c
>> @@ -61,6 +61,13 @@ GRL_LOG_DOMAIN(plugin_registry_log_domain);
>>                                 GRL_TYPE_PLUGIN_REGISTRY,        \
>>                                 GrlPluginRegistryPrivate))
>>  
>> +/* GQuark-like implementation, where we manually assign the first IDs. */
>> +struct IDHandler {
>> +  GHashTable *string_to_id;
>> +  GArray *id_to_string;
>> +  gint last_id;
>> +};
> 
> I'd rename IDHandler to KeyIDHandler, id alone is too generic to get its
> true purpose without looking at the rest of the code (plugins also have
> IDs for example).
> 
> I would rename the internal APIs to use that naming convention as well.
> (...)
Anything that makes the code clearer makes me happy :). Will do that.
> 
>> +/**
>> + * id_handler_add:
>> + * @handler: the handler
>> + * @key: a specific key for system keys, or GRL_METADATA_KEY_INVALID for it to
>> + * be assigned
>                   ^^ I would add "automatically" at the end.
>> + * @name: the name of the key.
>> + *
>> + * Add a new key<->name correspondence.
>> + *
>> + * Returns: the new key number, or GRL_METADATA_KEY_INVALID if the key could
>> + * not be created (typically if @key or @name is already registered).
>> + */
>> +static GrlKeyID
>> +id_handler_add (struct IDHandler *handler, GrlKeyID key, const gchar *name)
>> +{
>> +  GrlKeyID _key = key;
>> +
>> +  if (_key == GRL_METADATA_KEY_INVALID) {
>> +    /* existing keys go from 1 to (id_to_string->len - 1), so the next
>> +     * available key is id_to_string->len, which will be incremented by
>> +     * g_array_insert_val() */
>> +    _key = handler->id_to_string->len;
>> +  }
>> +
>> +  if (NULL != id_handler_get_name (handler, _key)) {
>> +    GRL_WARNING ("Cannot register %d:%s because key is already defined as %s",
>> +                 _key, name, id_handler_get_name (handler, _key));
>> +    return GRL_METADATA_KEY_INVALID;
>> +  } else if ( GRL_METADATA_KEY_INVALID != id_handler_get_key (handler, name)) {
>                 ^ blank space
> 
>> +    /* _key or name is already in use! */
>> +    GRL_WARNING ("Cannot register %d:%s because name is already registered with key %d",
>> +                 _key, name, id_handler_get_key (handler, name));
>> +    return GRL_METADATA_KEY_INVALID;
>> +  } else {
>> +    /* name_copy is shared between handler->id_to_string and
>> +     * handler->string_to_id */
>> +    gchar *name_copy = g_strdup (name);
>> +
>> +    if (_key >= handler->id_to_string->len)
>> +      g_array_set_size (handler->id_to_string, _key + 1);
>> +
>> +    /* yes, g_array_index() is a macro that give you an lvalue */
>                                                    ^^ missing 's'
> 
>> +    g_array_index (handler->id_to_string, const gchar *, _key) = name_copy;
>> +    g_hash_table_insert (handler->string_to_id,
>> +                         name_copy, GRLKEYID_TO_POINTER (_key));
>> +  }
>> +
>> +  return _key;
>> +
>> +}
> 
> 
> _______________________________________________
> grilo-list mailing list
> grilo-list gnome org
> http://mail.gnome.org/mailman/listinfo/grilo-list