Re: [PATCH 5/5] core: Add a default root media in notify_change()

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

On Tue, 2011-02-01 at 12:41 +0000, Iago Toral wrote:
> On Tue,  1 Feb 2011 09:07:46 +0100, "Juan A. Suarez Romero" 
>  <jasuarez igalia com> wrote:
> > If plugin use a NULL-media in notify_change(), replace it by the 
> > GrlBox that
> > represents the root.
> >
> > Signed-off-by: Juan A. Suarez Romero <jasuarez igalia com>
> > ---
> >  src/grl-media-source.c |   17 +++++++++++++++--
> >  1 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/grl-media-source.c b/src/grl-media-source.c
> > index 4402c8c..c1559d5 100644
> > --- a/src/grl-media-source.c
> > +++ b/src/grl-media-source.c
> > @@ -2583,7 +2583,7 @@ void grl_media_source_notify_change_stop
> > (GrlMediaSource *source)
> >  /**
> >   * grl_media_source_notify_change:
> >   * @source: a media source
> > - * @media: (allow-none): the media which has changed
> > + * @media: (allow-none): the media which has changed, or @NULL to
> > use the root box.
> >   * @change_type: the type of change
> >   * @location_unknown: if change has happpened in @media or any 
> > descendant
> >   *
> > @@ -2596,13 +2596,26 @@ void grl_media_source_notify_change
> > (GrlMediaSource *source,
> >                                       GrlMediaSourceChangeType 
> > change_type,
> >                                       gboolean location_unknown)
> >  {
> > +  GrlMedia *root;
> > +
> >    g_return_if_fail (GRL_IS_MEDIA_SOURCE (source));
> >    g_return_if_fail (!media || GRL_IS_MEDIA (media));
> >
> > +  if (!media) {
> > +    root = grl_media_box_new();
> > +    grl_media_set_source (root,
> > +                          grl_metadata_source_get_id
> > (GRL_METADATA_SOURCE (source)));
> > +  } else {
> > +    grl_media_set_source (media,
> > +                          grl_metadata_source_get_id
> > (GRL_METADATA_SOURCE (source)));
> > +  }
> 
>  I suggest this code instead to handle the situation:
> 
>  --- 8< ---
> 
>  if (!media) {
>   media = grl_media_box_new ();
>  }
>  grl_media_set_source (media,
>                        grl_metadata_source_get_id (GRL_METADATA_SOURCE 
>  (source)));
> 
>  --- 8< ---
> >    g_signal_emit (source,
> >                   registry_signals[SIG_CONTENT_CHANGED],
> >                   0,
> > -                 media,
> > +                 media? media: root,
>                      ^^^^^^^^^^^^^^^^^^
>  No need for this with my suggestion
> 
> >                   change_type,
> >                   location_unknown);
> > +  if (root) {
> > +    g_object_unref (root);
> > +  }
> 
>  And who would free media if it was non null? I guess it is simpler and 
>  more consistent with the rest of the fw if all media objects provided by 
>  grilo plugins are owned by the client. Would make the code here easier 
>  as well.

That is why I had that code: the one creating the media was the media
owner and the one in charge of freeing it.

Thus if, media==NULL, grl_media_source_notify_change() would create a
media and free it; else, the plugin invoking notify_change() would free
it.

Now, why media is not transferred to client? The point here is that
actually client can connect several times to "content-changed" signal,
invoking different callbacks. So, which callback will be the owner and
therefore be in charge of freeing it?


	J.A.