GNOME Bugzilla – Bug 636693
There should be a way of retrieving a GrlMedia with a given URI
Last modified: 2010-12-30 11:03:49 UTC
It is a common case that you want to browse a source, given a start-point - take file-systems and uPnP as examples. I don't often want to browse the root folder of my hard-disk. The file-system plugin allows you to configure the starting directory, but this isn't really enough - you may want to represent multiple directories that don't share a common ancestor, or aren't otherwise closely related. At the moment, you can do this in the file-system back-end by creating a media-box and setting the id as the path you want, like so: GrlMedia *box = grl_media_box_new (); grl_media_set_id (box, "/some/path/i/want"); And using that as the container when you call browse. Given that this is undocumented behaviour relying on the internals of the file-system plugin specifically though, this probably isn't something you should be doing. SUMMARY: It would be good to have a way of retrieving a media-box for a specified path from a source. This may require an extra vfunc in the plugins object, perhaps.
I think this bug and bug #635394 should be fixed together. I did a proposal at http://mail.gnome.org/archives/grilo-list/2010-December/msg00008.html but a different one is welcomed.
Sounds like a good proposal - I'd suggest adding some kind of convenience function for the simpler types, to avoid having to create a GValue, but otherwise looks good.
I think that having a convenience function for strings should be enough, at least for now. Another alternative to avoid creating a GValue is using a gpointer. As all keys have a expected type, from the key we can interpret the gpointer as a string, or a pointer to int, and so far. Anyway, my personal preference is still using the GValue and having a convenience function for the case of string.
Bug #635394 has been fixed, which means we have API defined in core to do this. To fix this bug we only need an implementation of this API in the filesystem plugin. FYI, we decided to have specific API for the URI => GrlMedia case and use the query() API for other metadata => GrlMedia transformations, see my last comments in bug #635394 for more details.
(In reply to comment #4) > FYI, we decided to have specific API for the URI => GrlMedia case and use the > query() API for other metadata => GrlMedia transformations, see my last > comments in bug #635394 for more details. See also bug #636394, comment #18.
(In reply to comment #5) > (In reply to comment #4) > > > FYI, we decided to have specific API for the URI => GrlMedia case and use the > > query() API for other metadata => GrlMedia transformations, see my last > > comments in bug #635394 for more details. > > > See also bug #636394, comment #18. I meant bug #635394, comment #18
Created attachment 176789 [details] [review] filesystem: implemented media_from_uri() and test_media_from_uri() Here is a first version of media_from_uri() and test_media_from_uri() in filesystem.
Review of attachment 176789 [details] [review]: The patch looks ok overall, but has a few details that need fixing. ::: src/filesystem/grl-filesystem.c @@ +658,3 @@ + path = g_filename_from_uri (uri, NULL, &error); + if (error != NULL) + return FALSE; You are leaking 'error' here @@ +690,3 @@ + path = g_filename_from_uri (mfus->uri, NULL, &error); + if (error != NULL) { + mfus->callback (source, NULL, mfus->user_data, error); I think Grilo API clients should only receive erros from Grilo (with error codes defined in grilo that make sense to them). I think here we should always raise errors with GRL_CORE_ERROR_MEDIA_FROM_URI_FAILED code. @@ +699,3 @@ + media = create_content (NULL, path, FALSE, FALSE); + mfus->callback (source, media, mfus->user_data, NULL); + g_object_unref (media); You should not unref 'media' here.
Created attachment 176817 [details] [review] filesystem: implemented media_from_uri() and test_media_from_uri() Thanks for the review! Here's an updated version. I also ensured errors that are passed to callbacks are not freed (should be the job of the callback, right?)
(In reply to comment #9) > Thanks for the review! > Here's an updated version. I also ensured errors that are passed to callbacks > are not freed (should be the job of the callback, right?) Actually, at the moment we do have the policy of freeing the errors right after invoking the callback, so that needs to be put back in the patch. We could discuss whether that policy makes sense or not, but for now we must be consistent with the rest of the code.
Created attachment 176837 [details] [review] filesystem: implemented media_from_uri() and test_media_from_uri() Fine. New version, freeing GErrors. Not sure it's consistent to transmit ownership of the GrlMedia but not the GError. Arguably, they are different things though. I wonder if other libraries do something similar, and what convention they use.
Created attachment 177250 [details] [review] Patch. final version The bug was finally fixed with this patch: commit bd01f190d32a90f5114c106e02755ac785b89bc3 Author: Guillaume Emont <gemont@igalia.com> Date: Sat Dec 18 16:43:52 2010 +0100 filesystem: implemented media_from_uri() and test_media_from_uri() Signed-off-by: Iago Toral Quiroga <itoral@igalia.com>