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 756203 - Implement GrlRelatedKeys
Implement GrlRelatedKeys
Status: RESOLVED OBSOLETE
Product: grilo
Classification: Other
Component: lua
unspecified
Other Linux
: Normal normal
: ---
Assigned To: grilo-maint
grilo-maint
Depends on:
Blocks:
 
 
Reported: 2015-10-07 20:45 UTC by Victor Toso
Modified: 2018-09-24 09:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
lua-factory: factor out function to add key (9.42 KB, patch)
2016-08-22 16:16 UTC, Gaurav Narula
reviewed Details | Review
lua-factory: add support for GrlRelatedKeys (7.27 KB, patch)
2016-08-22 16:17 UTC, Gaurav Narula
needs-work Details | Review

Description Victor Toso 2015-10-07 20:45:06 UTC
From Grilo reference [0] we should be able to use GrlRelatedKeys which basically binds two keys/values.

[0] https://developer.gnome.org/grilo/unstable/GrlRelatedKeys.html#GrlRelatedKeys.description

--

Maybe we could do it by using the array index of the media to refer to those related keys, for instace:

<snip>

media = {}
media.title = "this is a picture"
media[0] = { uri="http://...", mime-type="image/jpeg" }
grl.callback(media, 0)

</snip>

The developer could use #media to track the indexes.
Comment 1 Gaurav Narula 2016-08-22 16:16:56 UTC
Created attachment 333925 [details] [review]
lua-factory: factor out function to add key
Comment 2 Gaurav Narula 2016-08-22 16:17:26 UTC
Created attachment 333926 [details] [review]
lua-factory: add support for GrlRelatedKeys
Comment 3 Victor Toso 2016-08-22 16:54:53 UTC
Review of attachment 333925 [details] [review]:

Just small comments, this one is ready to go with the suggested changes.
The following patch (with the adjustments) will need the unit test to check it and avoid regressions.

::: src/lua-factory/grl-lua-library.c
@@ +323,3 @@
+grl_util_add_key (lua_State *L,
+                  GObject *object,
+                  gboolean is_media)

is_media belongs to the second patch

@@ +333,3 @@
+  /* Handled before */
+  if (g_strcmp0 (key_name, "type") == 0) {
+    goto next_key;

remove the goto/label, include the free and return here;

@@ +342,3 @@
+
+  key_id = grl_registry_lookup_metadata_key (registry, key_name);
+  if (key_id != GRL_METADATA_KEY_INVALID) {

Take the opportunity to change this as well, please. Use instead:
if (key_id == GRL_METADATA_KEY_INVALID) {
  GRL_WARNING (..)
  g_free (key_name)
  return;
}

type = ..

so we can lower a little bit the indentation

@@ +355,3 @@
+            grl_data_set_int (GRL_DATA (object), key_id, value);
+          else
+            grl_data_set_int64 (GRL_DATA (object), key_id, value);

Please add the {} in this if/else

@@ +388,3 @@
+                     lua_typename (L, lua_type(L, -1)), key_name);
+      }
+      break;

Add a space here between break; case G_TYPE_BOOLEAN:
Comment 4 Victor Toso 2016-08-22 17:03:20 UTC
Review of attachment 333926 [details] [review]:

This one needs the unit test.

::: src/lua-factory/grl-lua-library.c
@@ +363,3 @@
+              grl_data_set_int64 (GRL_DATA (object), key_id, value);
+            else
+              GRL_WARNING ("GrlRelatedKeys does not support INT64");

it does: https://git.gnome.org/browse/grilo/tree/src/data/grl-related-keys.c#n619
Maybe we are missing that in the docs?

@@ +372,3 @@
+          grl_util_add_table_to_media (L, GRL_MEDIA (object), key_id, key_name, type);
+        else
+              GRL_WARNING ("GrlRelatedKeys does not support tables");

Extra indentation here... not sure about this one (if we don't support tables/arrays on GrlRelatedKeys)

@@ +463,3 @@
+                                        key_id,
+                                        binary,
+                                        size);

all if/else above with {}
Comment 5 GNOME Infrastructure Team 2018-09-24 09:37:27 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/grilo/issues/71.