GNOME Bugzilla – Bug 734636
Tracker plugin doesn't get thumbnails
Last modified: 2014-08-15 21:12:37 UTC
Created attachment 283150 [details] [review] Add thumbnail url to GrlMedia objects gotten with g_file_query_info. Grl-tracker plugin doesn't add thumbnail information to GrlMedia objects given in browse or search operations, since tracker doesn't store that (but tracker-needle displays thumbnails by using g_file_query_info). The attached patch adds thumbnail information and works in my testing, but it could definitely use a good review.
Review of attachment 283150 [details] [review]: 2 problems: 1) This is making a blocking call, urgh 2) You should use the local-metadata plugin to request additional file-based metadata. That's how grilo's metadata system works.
1) Yes I realize I should have used the async call for getting the file info. 2) Looking at the local-metadata plugin I see it doesn't do search or browse operations at all, but only resolve? So in order to get a list of media from grilo applications should do the following: Search and/or Browse to get a list of GrlMedia Resolve also, to make sure we have all known information about the given media. Is that right?
(In reply to comment #2) > 1) Yes I realize I should have used the async call for getting the file info. > 2) Looking at the local-metadata plugin I see it doesn't do search or browse > operations at all, but only resolve? So in order to get a list of media from > grilo applications should do the following: > > Search and/or Browse to get a list of GrlMedia > Resolve also, to make sure we have all known information about the given media. > > Is that right? That's right. This is what Totem does to get thumbnails.
Ok, I figured out how to get resolving to work. However, if tracker indexed an audio file it puts the mediaart into ~/.cache/media-art but grl-local-metadata is only looking for the mediaart in a local file (.mediaartlocal) not in ~/.cache/media-art. I've created a patch that makes it check both, that seems to work fine. Should I attach that here or post it to a new bug ?
(In reply to comment #4) > Ok, I figured out how to get resolving to work. However, if tracker indexed an > audio file it puts the mediaart into ~/.cache/media-art but grl-local-metadata > is only looking for the mediaart in a local file (.mediaartlocal) not in > ~/.cache/media-art. I've created a patch that makes it check both, that seems > to work fine. Should I attach that here or post it to a new bug ? local-metadata uses libmediaart, as tracker should. Which version of grilo-plugins are you testing this against?
I'm using grilo-plugins 0.2.12 and libmediaart 0.4.0. I created the grl-local-metadata patch against master though. libmediaart's api lets you ask for a cache_path and a local_path, and grilo-plugins grl-local-metadata was previously only asking for the local_path, so would only get the .mediaartlocal files, nothing in ~/.cache/media-art would appear. I'll attach the patch here it's pretty small.
Created attachment 283405 [details] [review] Ask libmediaart for thumbnails from both cache and local paths Here's the small patch that makes grl-local-metadata resolve thumbnails from both ~/.cache/media-art and .mediaartlocal locations.
Review of attachment 283405 [details] [review]: ::: src/local-metadata/grl-local-metadata.c @@ +647,3 @@ if (thumbnail_uri) { grl_media_set_thumbnail (rs->media, thumbnail_uri); g_free (thumbnail_uri); This leaks when both cache_uri and thumbnail_uri are set.
Review of attachment 283405 [details] [review]: Also, awful API. From reading the API docs, I really thought that path and local_uri were two versions of the same data.
Created attachment 283406 [details] [review] Updated to fix leak Yes, the libmediaart api is a bit strange in that regard, I agree. Anyway, this patch fixes the leak.
Review of attachment 283406 [details] [review]: ::: src/local-metadata/grl-local-metadata.c @@ +653,3 @@ } + if (thumbnail_uri) + g_free (thumbnail_uri); g_free() can take a NULL. But you'll need to initialise it to NULL.
I filed bug 734837 against libmediaart as well.
Created attachment 283535 [details] [review] initialize strings to NULL Updated to initialize the strings to NULL and always call g_free on them. removes two if statements.
Review of attachment 283535 [details] [review]: Looks good, other than that. ::: src/local-metadata/grl-local-metadata.c @@ +650,3 @@ + grl_media_set_thumbnail (rs->media, cache_uri); + else + g_debug ("Found no thumbnail for artist %s and album %s", artist, album); GRL_DEBUG()
Created attachment 283538 [details] [review] use GRL_DEBUG Good catch, feel free to push I don't have git push access.