Re: Removing empty keys from GrlData

["Juan A." Suárez Romero <jasuarez igalia com>]

On Tue, 2011-03-29 at 14:06 +0000, Iago Toral wrote:
> On Tue, 29 Mar 2011 14:07:39 +0200, "Juan A." Suárez Romero 
> > { "http://world.com/1.oth";, NULL, NULL }
> > { "http://world.com/2.mp3";, "audio/mp3", NULL }
> > { "http://world.com/3.unk";, NULL, NULL }
> > { "http://world.com/4.ogg";, "audio/ogg", NULL }
> > { "http://world.com/5.wav";, "audio/wav", NULL }
> >
> >
> > So we have added those 2 related keys in our GrlData. Now the doubts
> >
> > 1.a) What grl_data_has_key (data, GRL_METADATA_KEY_URL) should 
> > return?
> > 1.b) What grl_data_has_key (data, GRL_METADATA_KEY_MIME) should 
> > return?
> > 1.c) What grl_data_has_key (data, GRL_METADATA_KEY_BITRATE) should
> > return?
> >
> > In current implementation, all functions return TRUE. The reason is 
> > that
> > this function returns TRUE if the asked key or any related key has a
> > value.
> 
>  I'd say that if we are forcing that all the keys must have a value, 
>  then has_key cannot return TRUE for BITRATE, because it does not 
>  actually have a value.
> 

While I see coherent to return FALSE in the case of bitrate, I'm not so
sure about the MIME either.

My doubt comes because in GrlData there are functions to handle
single-valued keys and functions to handle multi-valued keys.

The grl_data_has_key() was there since the very beginning, and depending
on if it must handle single or multi-valued elements the behaviour
changes.

If we want grl_data_has_key() only deals with single-valued keys
(therefore meaning it would only check the first value), then it should
return FALSE for MIME key.

If it must deal with multi-valued keys, then obviously it should return
TRUE.

Now I'm wondering if we should keep the function to handle only
single-valued keys. For the case of multi-valued keys, user can either
ask for all GrlRelatedKeys and then checking it related_keys to see if
it has the key, or adding a new function like
grl_data_related_keys_has_key(). I feel a bit inclined to do this.



> >
> > 2) What grl_data_get_keys (data) should return?
> >
> > In current implementation, it returns the three keys:
> > grl_data_get_keys() returns all keys in GrlData as well as the 
> > related
> > keys.
> >
> > I think this has something to do with the work to do here: seems 
> > logical
> > that bitrate shouldn't be returned, as it does not have any value.
> >
> > But what about mime? If get_keys() only checks the first value, then
> > clearly it should only return url. But if it checks all values, then
> > mime should also be returned.
> >
> > Thus, we should decide if this function should only check the first
> > value or not.
> 
>  I think we should return the keys for which there is at least one known 
>  value.
>  In the example I think it should return url and mime, but not bitrate.
> 
>  This must be consistent with the behaviour of has_key(). If has_key(k) 
>  returns TRUE get_keys() must return 'k' too.
> 

I brought this question because, as it happened with grl_data_has_key(),
grl_data_get_keys() were there to handle single-valued keys.

Now I wonder if we should change the semantic (and therefore possibly
breaking some client), or introducing a new function to get all keys
with some value (something like grl_data_get_all_keys()).

> > 3.a) What grl_data_key_is_known (data, GRL_METADATA_KEY_URL) should
> > return?
> > 3.b) What grl_data_key_is_known (data, GRL_METADATA_KEY_MIME) should
> > return?
> > 3.a) What grl_data_key_is_known (data, GRL_METADATA_KEY_BITRATE) 
> > should
> > return?
> >
> > In current implementation, this function only checks the first value,
> > and thus would return TRUE, FALSE, FALSE.
> 
>  I think this "keys is known" made sense when we allowed keys that did 
>  not have a value. I think it does not make sense now, or am I missing 
>  something?
> 

Yeah, you're right. Both functions do the same. We can get rid of
grl_data_key_is_known().

> >
> > 4.a) What grl_data_length (data, GRL_METADATA_KEY_URL) should return?
> > 4.b) What grl_data_length (data, GRL_METADATA_KEY_MIME) should 
> > return?
> > 4.c) What grl_data_length (data, GRL_METADATA_KEY_BITRATE) should
> > return?
> >
> > In current implementation, all of them returns 5: checks how many
> > elements the key and related keys have.
> >
> > And this has something to do also with this proposal: shouldn't this
> > function return 5, 3 and 0 respectively?
> 
>  We should ask ourselves why would someone use length() to begin with. I 
>  can see two reasons:
> 
>  1) Because they want to traverse the list of values: in that case all 
>  the functions should return 5.
>  2) Because they want to know how many media resources are available: in 
>  that case all the functions should return 5.
> 
>  I don't see a clear case where the user would be interested in knowing 
>  the number of all non-null values, but if there is we should probably 
>  add specific API for that and keep the current one as is (or maybe add a 
>  gboolean parameter to the current API to control whether we are 
>  interested in NULL values or not).
> 

Mostly I agree. The main reason used to introduce this function was to
implement an iterator that would get all GrlRelatedKeys.

So I think returning 5 is fine.

> >
> > 5.a) What grl_data_get_all_single_related_keys (data,
> > GRL_METADATA_KEY_URL) should return?
> > 5.b) What grl_data_get_all_single_related_keys (data,
> > GRL_METADATA_KEY_MIME) should return?
> > 5.c) What grl_data_get_all_single_related_keys (data,
> > GRL_METADATA_KEY_BITRATE) should return?
> >
> >
> > In current implementation (which btw doesn't honour to what
> > documentation says), all of them returns the list with the 5 related
> > keys with their values.
> >
> > Now, I wonder if (5.a) shouldn't return a list with the 5 url values,
> > (5.b) a list with the 3 mimetypes values, and (5.c) an empty list.
> 
>  Well, wasn't this intended to just get all values for a key without 
>  caring about related keys at all? In that case I guess it should do just 
>  that. That is, it should do  what you suggest and not what is doing at 
>  the moment.
> 

Yes. In fact that is what the documentation states :) But not sure why I
implemented in other way (probably an absentmindedness).

>  BTW, I don't like the name we chose for this at all, its behaviour is 
>  not obvious from its name:
>  - Why the heck did we name this with the words "related_keys" in there 
>  if this is supposed to ignore related keys all together?
>  - Why is this named _get_all_single_related_keys if this is supposed to 
>  return values and not keys?
> 
>  I suggest we rename this to something more descriptive like: 
>  grl_data_get_single_values_for_key() or something along this line.


To be honest, I don't remember why we chose those names. And nobody
complained :)

In any case, I'll rename the functions.

BTW, in all this task, instead of directly removing/renaming the
functions, I'm marking them as "deprecated"; and my plan is to get rid
of them in next or after next release.

	J.A.