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

[Iago Toral <itoral igalia com>]

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.

Iago