Re: [BRANCHES] Giving some sanity to dates

[Guillaume Emont <guijemont igalia com>]

On 27/09/2011 14:40, Guillaume Emont wrote:
> Hi Juan, and thanks for your comments.
> 
> See my answers below, and I will soon update the branch accordingly.
> 
> Also, I'd like to ask: was this the fruit of a full review, or only some
> preliminary comments with more to come later?
> 
> Guij
> 
> On 23/09/2011 18:59, Juan A. Suarez Romero wrote:
>> Couple of comments:
>>
>> In grl-media.c, grl_media_get_publication_date():
>>
>> **
>>  * grl_media_get_publication_date:
>>  * @media: the media object
>>  *
>>  * Returns: (transfer full): the media's date (TBD)
>>  *
>>  * Since: 0.1.4
>>  */
>>
>>
>> It should tell
>>
>> * Returns: (transfer full): the publication date of @media
>>
> Good catch. Forgot to update that.
>>
>>
>> In grl-media.c, grl_media_get_publication_date():
>>
>> GDateTime *
>> grl_media_get_publication_date (GrlMedia *media)
>> {
>>   GDateTime *date;
>>   date = grl_data_get_boxed (GRL_DATA (media),
>>                              GRL_METADATA_KEY_PUBLICATION_DATE);
>>   if (date)
>>     return g_date_time_ref (date);
>>   return NULL;
>> }
>>
>>
>> Is there any reason to return date as reffed? The same happen in
>> grl_media_get_modification_date().
> 
> Hmm, it's been a while ago. I probably made it like that out of fear
> that _set_modification_date() gets called while we are using the value
> returned by the getter without taking a reference.
> But arguably, there's the same issue with all the getters of GrlMedia,
> and if we document that the returned value remains owned by the
> GrlMedia, people should expect to have to handle that properly. I'm OK
> to change that (seems to work with bindings too) for consistency.

Also, there was the fact that the convention here was to return a const
for things that are owned by the media, but returning a const is not
practical for a GDateTime because the whole g_date_time_*() API takes
non-consts as argument. But I guess it's OK to break this unwritten
convention as long as things are well documented.

>>
>> My opinion is that we shouldn't return it reffed, and let users to do a
>> ref() if they want to keep it.
>>
>> The rationale is that all grl_media_get_foo() functions are always
>> returning constant information that is owned by GrlMedia. And in all
>> cases, specially when you handle strings, you know that you need to make
>> a copy of data if you want to keep it.
>>
>> So in order to keep it coherent, and not introduce doubts in usesr when
>> dealing with the returned data ("must I free the returned value?", "do i
>> need to ref it or not?", ...) , my proposal is to return the GDateTime
>> without reffing it, and writing (of course) in the doc that the value is
>> owned by GrlMedia and user should ref() it if they want to keep it.
>>
>> 	J.A.
>>
>>
>> _______________________________________________
>> grilo-list mailing list
>> grilo-list gnome org
>> http://mail.gnome.org/mailman/listinfo/grilo-list
>>
> 
> _______________________________________________
> grilo-list mailing list
> grilo-list gnome org
> http://mail.gnome.org/mailman/listinfo/grilo-list
>