Re: Patch reviews, change API and resolve API

[Bastien Nocera <hadess hadess net>]

On Tue, 2014-01-21 at 10:08 +0100, Juan A. Suarez Romero wrote:
> On Tue, 2014-01-21 at 02:05 +0100, Bastien Nocera wrote:
>
> > The only blocking patch for Totem is:
> > https://bugzilla.gnome.org/show_bug.cgi?id=722358
>
> I'll give more priority to this.

Great, thanks.

> > 2) I'd also like some guidance on the last patch to:
> > https://bugzilla.gnome.org/show_bug.cgi?id=722629
> >
> > I don't quite understand the API of:
> > grl_source_notify_change()
> > and the "content-changed" signal.
> >
> > For example, I'd like to signal that a particular media (say, a DVD
> > image) in the optical media source got removed. It seems that I can't
> > just pass the GrlMedia representing the DVD image to the function. I'd
> > need to pass %NULL to represent the root of the source, and then browse
> > again?
>
> I think it is allowed to pass a GrlMedia, if you want to notify the
> exact media that has changed.
>
> Actually, a plugin can use a GrlMediaBox (or %NULL for root container)
> if it is easier for it to just say something changed inside the
> container, without explicitly saying the element added/removed, or
> specify the exact element that has changed. So far, I think all plugins
> that handles notification follows the first approach.
>
> I'm not sure if allowing both approaches is something good or not. Using
> a Box makes things easier to handle: application only needs to re-launch
> the browse/search, and that's all. But it has the drawback that it has
> more cost than just remove/add the exact element from a list.

Right, passing a fully-resolved GrlMedia is great for front-ends. Having
to re-do a browse for a single changed item isn't too great, so the less
of that, the better.

That also means that I didn't waste my time when I added proper change
support to optical-media ;)

Although, that won't work with added though, as either you'll pass the
GrlMediaBox that contained the added item (meaning, "you need to browse
this again") or pass the new GrlMedia which is fine if you know that you
don't have any children items in the source but is useless otherwise.

Should I file a bug about this? I'd fix this by adding a new
grl_source_notify_change()-like call, deprecate the old one, and still
send out the compat "content-changed" signal.

> > Should grl_source_notify_change() warn if the GrlMedia is neither %NULL
> > nor a GrlMediaBox?
> Right now, I don't think so. 

> From the above, that makes sense.

> > 3) Finally, I wanted to be able to add thumbnails to items retrieved
> > from the Tracker plugin. It looks like I need to use the local-metadata
> > plugin to do that, but, in the future, I want to use the tmdb plugin to
> > look up more data.
> >
> > Is it possible to do lookups only using local-metadata but not tmdb for
> > some calls, but use all available sources for others?
>
> I think if you want to do lookups only using one source, you would need
> to do it by hand, calling resolve().

grl_source_resolve() needs to be called by hand in all cases, doesn't
it? Maybe an explanation of what GrlSource needs to be passed to
grl_source_notify_change() would be useful.

Cheers