GNOME Bugzilla – Bug 673916
local-metadata: Support all GIO supported schemes
Last modified: 2012-04-13 14:41:59 UTC
.
Created attachment 211845 [details] [review] local-metadata: Support all GIO supported schemes And not just file:///
Created attachment 211846 [details] [review] local-metadata: Parse filenames for videos too So we can fetch some extra metadata like Series name, or the likes.
Created attachment 211851 [details] [review] local-metadata: Don't say we support UPnP URLs Because we don't want to use UPnP URLs for guessing metadata.
Created attachment 211852 [details] [review] local-metadata: Support using the Title For metadata guessing, in case the URL is unusable/unsupported.
Created attachment 211853 [details] [review] local-metadata: Rework may_resolve To be understandable by normal humans, and require the URL only if the title isn't usable, or require the title if the URL isn't usable.
Review of attachment 211853 [details] [review]: ::: src/metadata/local-metadata/grl-local-metadata.c @@ +911,3 @@ + return TRUE; + return FALSE; + } Conditionals are wrong. Actually, it should check for URL only if the requested key is thumbnail. It should be: if (GRL_IS_MEDIA_IMAGE (media)) { if (key_id == GRL_METADATA_KEY_THUMBNAIL) { if (!grl_data_has_key (GRL_DATA (media), GRL_METADATA_KEY_URL)) goto missing_url; if (!has_compatible_media_url (media)) return FALSE; return TRUE; } else { return FALSE; }
Review of attachment 211852 [details] [review]: Looks good for me
Review of attachment 211851 [details] [review]: Good for me
Review of attachment 211846 [details] [review]: ::: src/metadata/local-metadata/grl-local-metadata.c @@ +942,3 @@ error = g_error_new (GRL_CORE_ERROR, GRL_CORE_ERROR_RESOLVE_FAILED, "local-metadata cannot resolve any of the given keys"); + if (GRL_IS_MEDIA_IMAGE (rs->media) && can_access == FALSE) I think this should also be checked for GrlMediaVideo when thumbnail is requested. That is resolve_image() can be invoked only if can_access == TRUE
Review of attachment 211845 [details] [review]: Thumbs up
Review of attachment 211853 [details] [review]: Correct. I've made similar changes, but without reworking the depth.
Created attachment 211987 [details] [review] local-metadata: Rework may_resolve To be understandable by normal humans, and require the URL only if the title isn't usable, or require the title if the URL isn't usable.
Review of attachment 211846 [details] [review]: ::: src/metadata/local-metadata/grl-local-metadata.c @@ +942,3 @@ error = g_error_new (GRL_CORE_ERROR, GRL_CORE_ERROR_RESOLVE_FAILED, "local-metadata cannot resolve any of the given keys"); + if (GRL_IS_MEDIA_IMAGE (rs->media) && can_access == FALSE) flags can be used to request multiple types of metadata, right? When do you fail for video? For image, it's easy, the only type of metadata that can be requested is the thumbnail. If you don't have the URL, you can't access the thumbnail. For video, one can request the thumbnail, the episode number, the series name, etc. Do we fail if thumbnail is requested without the URL, even if the other metadata could be gathered?
Review of attachment 211987 [details] [review]: Seems good now.
Review of attachment 211846 [details] [review]: ::: src/metadata/local-metadata/grl-local-metadata.c @@ +942,3 @@ error = g_error_new (GRL_CORE_ERROR, GRL_CORE_ERROR_RESOLVE_FAILED, "local-metadata cannot resolve any of the given keys"); + if (GRL_IS_MEDIA_IMAGE (rs->media) && can_access == FALSE) Yes, sorry, you are right. I thought I were reviewing the may_resolve() function. So patch is good indeed.
All pushed to 0.1.x and master with the changes mentioned for the may_resolve rework. Attachment 211845 [details] pushed as fd0b9ab - local-metadata: Support all GIO supported schemes Attachment 211846 [details] pushed as 5ddfcad - local-metadata: Parse filenames for videos too Attachment 211851 [details] pushed as 0ed4a7d - local-metadata: Don't say we support UPnP URLs Attachment 211852 [details] pushed as 739f4ad - local-metadata: Support using the Title Attachment 211987 [details] pushed as 4ab76a9 - local-metadata: Rework may_resolve