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 774748 - tracker: Support favorite read/write
tracker: Support favorite read/write
Status: RESOLVED FIXED
Product: grilo
Classification: Other
Component: plugins
unspecified
Other All
: Normal normal
: ---
Assigned To: grilo-maint
grilo-maint
Depends on:
Blocks: 774754
 
 
Reported: 2016-11-20 14:11 UTC by Marinus Schraal
Modified: 2016-12-02 21:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tracker: Support favorite read/write (5.38 KB, patch)
2016-11-20 14:11 UTC, Marinus Schraal
none Details | Review
tracker: Support favorite read/write (5.87 KB, patch)
2016-11-20 15:52 UTC, Marinus Schraal
none Details | Review
tracker: small rework on _tracker_get_insert_string() (2.47 KB, patch)
2016-11-21 17:58 UTC, Victor Toso
committed Details | Review
tracker: Support favorite read/write (5.36 KB, patch)
2016-11-22 10:43 UTC, Marinus Schraal
committed Details | Review
tracker: Support last-played metadata storage (2.25 KB, patch)
2016-11-22 22:32 UTC, Marinus Schraal
none Details | Review
tracker: Support last-played metadata storage (2.22 KB, patch)
2016-12-02 13:09 UTC, Marinus Schraal
committed Details | Review

Description Marinus Schraal 2016-11-20 14:11:05 UTC
This is a proposed solution for reading/writing the 
GRL_METADATA_KEY_FAVOURITE in the tracker plugin.

The issue is that trackers boolean representation does not fit with 
tracker's creating/deleting of a tag 
(https://developer.gnome.org/ontology/stable/nao-Tag.html).

More of trackers properties do not line up with grilo. So if at some 
point we might want to handle more of those keys, it might require 
coming up with a more generic solution.
Comment 1 Marinus Schraal 2016-11-20 14:11:09 UTC
Created attachment 340354 [details] [review]
tracker: Support favorite read/write

Support metadata KEY_FAVOURITE reading & writing in the tracker
backend. The tracker ontology for this uses hasTag in combination with
the nao:predefined-tag-favorite property, which doesn't line up directly
with the boolean format Grilo uses. So either set the key with the given
property or delete the tag completely.
Comment 2 Marinus Schraal 2016-11-20 15:52:03 UTC
Created attachment 340358 [details] [review]
tracker: Support favorite read/write

Support metadata KEY_FAVOURITE reading & writing in the tracker
backend. The tracker ontology for this uses hasTag in combination with
the nao:predefined-tag-favorite property, which doesn't line up directly
with the boolean format Grilo uses. So either set the key with the given
property or delete the tag completely.
Comment 3 Victor Toso 2016-11-21 17:44:10 UTC
Review of attachment 340358 [details] [review]:

Other than that, looks okay too me. I'll post a patch here reorganing grl_tracker_tracker_get_insert_string(). Feel free to rebase in top of it or just rework your patch and I'll rebase in top of it before pushing

::: src/tracker/grl-tracker-utils.c
@@ +96,3 @@
+          gint                 column,
+          GrlMedia            *media,
+          GrlKeyID             key)

indentation

@@ +98,3 @@
+          GrlKeyID             key)
+{
+  const gchar *str = tracker_sparql_cursor_get_string (cursor, column, NULL);

as str could be NULL, you will need to check that before g_str_has_suffix().. what about:

{
  const gchar *str = tracker_sparql_cursor_get_string (cursor, column, NULL);
  gboolean is_favourite = FALSE;

  if (str != NULL && g_str_has_suffix (str, "predefined-tag-favorite"))
    is_favourite = TRUE;

  grl_media_set_favourite (media, is_favourite);
}

@@ +508,3 @@
+      g_string_append_printf (gstr, "%s nao:predefined-tag-favorite",
+                              assoc->sparql_key_attr);
+      }

ident

@@ +532,3 @@
         if (grl_data_has_key (GRL_DATA (media),
                               GRLPOINTER_TO_KEYID (key->data))) {
+          /* The favourite key is really setting or deleteing a tag

typo: deleting

@@ +535,3 @@
+           * in tracker, so in the case of setting it to false skip
+           * the insert string creation step for this key completely.
+           */

Based on the comment, we don't need to check the metadadata-key value of favourite in the if, right?
Comment 4 Victor Toso 2016-11-21 17:58:29 UTC
Created attachment 340466 [details] [review]
tracker: small rework on _tracker_get_insert_string()

Clear a bit the logic so it gets easier to include changes.
This patch does:
- Change GList iteration from while() to for()
- Move variables to its local scope;
- Use *continue* inside the for;
- Remove if else check on gboolean first
Comment 5 Marinus Schraal 2016-11-22 10:43:45 UTC
Created attachment 340507 [details] [review]
tracker: Support favorite read/write

Support GRL_METADATA_KEY_FAVOURITE reading & writing in the tracker
backend. The tracker ontology for this uses hasTag in combination with
the nao:predefined-tag-favorite property, which doesn't line up directly
with the boolean format Grilo uses. So either set the key with the given
property or delete the tag completely.
Comment 6 Marinus Schraal 2016-11-22 10:56:44 UTC
(In reply to Victor Toso from comment #3)
> Review of attachment 340358 [details] [review] [review]:

> Based on the comment, we don't need to check the metadadata-key value of
> favourite in the if, right?

It does need to be checked. We want to skip creating an insert string when the key value is set to false: we want the tag to be removed completely.

Updating values is implemented in tracker by deleting the value completely and then inserting it anew (this how all changes work). Not providing an insert string thus deletes it.

Note that this TRACKER_DELETE_REQUEST only applies to the case in which we manipulate only 1 metadata key (like i want to do in music) and just this one because it skips the insert string creation. In the case of multiple keys it will always use TRACKER_SAVE_REQUEST, but with the favourite string skipped and other properties treated as regular this amounts to the same outcome: the favourite tag to be removed and the others to be updated.
Comment 7 Marinus Schraal 2016-11-22 22:32:24 UTC
Created attachment 340561 [details] [review]
tracker: Support last-played metadata storage

Allows storing the last played date-time in tracker. Use the
nfo:fileLastAccessed & nie:contentAccessed property for this as they
are both valid.
Comment 8 Marinus Schraal 2016-12-02 13:09:29 UTC
Created attachment 341238 [details] [review]
tracker: Support last-played metadata storage

Allows storing the last played date-time in tracker. Use the
nfo:fileLastAccessed & nie:contentAccessed property for this as they
are both valid.
Comment 9 Victor Toso 2016-12-02 21:23:19 UTC
Review of attachment 340507 [details] [review]:

Ack!

I'm fixing two small things and pushing.

::: src/tracker/grl-tracker-utils.c
@@ +96,3 @@
+          gint                      column,
+          GrlMedia                 *media,
+          GrlKeyID                  key)

All the other functions indent the parameters vertically, just after the open bracket.

@@ +541,3 @@
+      if (assoc->grl_key == GRL_METADATA_KEY_FAVOURITE &&
+          !grl_data_get_boolean (GRL_DATA(media), assoc->grl_key))
+        continue;

changing grl_data_get_boolean() to grl_media_get_favourite()
Comment 10 Victor Toso 2016-12-02 21:24:29 UTC
Attachment 340466 [details] pushed as 9183260 - tracker: small rework on _tracker_get_insert_string()
Attachment 340507 [details] pushed as bb3a85d - tracker: Support favorite read/write
Attachment 341238 [details] pushed as b07bf05 - tracker: Support last-played metadata storage