After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 673916 - local-metadata: Support all GIO supported schemes
local-metadata: Support all GIO supported schemes
Status: RESOLVED FIXED
Product: grilo
Classification: Other
Component: plugins
git master
Other All
: Normal normal
: ---
Assigned To: grilo-maint
grilo-maint
Depends on:
Blocks:
 
 
Reported: 2012-04-11 15:47 UTC by Bastien Nocera
Modified: 2012-04-13 14:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
local-metadata: Support all GIO supported schemes (1.50 KB, patch)
2012-04-11 15:47 UTC, Bastien Nocera
committed Details | Review
local-metadata: Parse filenames for videos too (2.17 KB, patch)
2012-04-11 15:48 UTC, Bastien Nocera
committed Details | Review
local-metadata: Don't say we support UPnP URLs (1.19 KB, patch)
2012-04-11 16:50 UTC, Bastien Nocera
committed Details | Review
local-metadata: Support using the Title (1.66 KB, patch)
2012-04-11 16:50 UTC, Bastien Nocera
committed Details | Review
local-metadata: Rework may_resolve (3.09 KB, patch)
2012-04-11 16:50 UTC, Bastien Nocera
needs-work Details | Review
local-metadata: Rework may_resolve (3.09 KB, patch)
2012-04-13 12:46 UTC, Bastien Nocera
committed Details | Review

Description Bastien Nocera 2012-04-11 15:47:49 UTC
.
Comment 1 Bastien Nocera 2012-04-11 15:47:51 UTC
Created attachment 211845 [details] [review]
local-metadata: Support all GIO supported schemes

And not just file:///
Comment 2 Bastien Nocera 2012-04-11 15:48:21 UTC
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.
Comment 3 Bastien Nocera 2012-04-11 16:50:30 UTC
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.
Comment 4 Bastien Nocera 2012-04-11 16:50:40 UTC
Created attachment 211852 [details] [review]
local-metadata: Support using the Title

For metadata guessing, in case the URL is unusable/unsupported.
Comment 5 Bastien Nocera 2012-04-11 16:50:47 UTC
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.
Comment 6 Juan A. Suarez Romero 2012-04-13 12:36:32 UTC
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;
    }
Comment 7 Juan A. Suarez Romero 2012-04-13 12:36:50 UTC
Review of attachment 211852 [details] [review]:

Looks good for me
Comment 8 Juan A. Suarez Romero 2012-04-13 12:37:03 UTC
Review of attachment 211851 [details] [review]:

Good for me
Comment 9 Juan A. Suarez Romero 2012-04-13 12:37:12 UTC
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
Comment 10 Juan A. Suarez Romero 2012-04-13 12:37:27 UTC
Review of attachment 211845 [details] [review]:

Thumbs up
Comment 11 Bastien Nocera 2012-04-13 12:45:41 UTC
Review of attachment 211853 [details] [review]:

Correct. I've made similar changes, but without reworking the depth.
Comment 12 Bastien Nocera 2012-04-13 12:46:26 UTC
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.
Comment 13 Bastien Nocera 2012-04-13 12:57:51 UTC
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?
Comment 14 Juan A. Suarez Romero 2012-04-13 14:28:43 UTC
Review of attachment 211987 [details] [review]:

Seems good now.
Comment 15 Juan A. Suarez Romero 2012-04-13 14:32:46 UTC
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.
Comment 16 Bastien Nocera 2012-04-13 14:41:44 UTC
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