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 746974 - Tracker plugin's change signal is useless
Tracker plugin's change signal is useless
Status: RESOLVED FIXED
Product: grilo
Classification: Other
Component: plugins
unspecified
Other Linux
: Normal normal
: ---
Assigned To: grilo-maint
grilo-maint
Depends on:
Blocks: 729004
 
 
Reported: 2015-03-29 13:28 UTC by Bastien Nocera
Modified: 2016-08-24 18:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tracker: fix wrong usage of g_assert() (936 bytes, patch)
2015-09-23 21:14 UTC, Xavier Claessens
committed Details | Review
tracker: fix wrong usage of GHashTable (1.70 KB, patch)
2015-09-23 21:14 UTC, Xavier Claessens
none Details | Review
tracker: fix wrong usage of GHashTable (2.00 KB, patch)
2015-09-23 21:16 UTC, Xavier Claessens
none Details | Review
tracker: fix inefficient usage of GHashTable (2.00 KB, patch)
2015-09-24 14:30 UTC, Xavier Claessens
committed Details | Review
tracker: rewrite notification code (28.35 KB, patch)
2015-09-25 16:05 UTC, Xavier Claessens
none Details | Review
tracker: rewrite notification code (29.79 KB, patch)
2015-09-29 21:05 UTC, Xavier Claessens
none Details | Review
tracker: rewrite notification code (30.14 KB, patch)
2015-10-19 21:02 UTC, Xavier Claessens
none Details | Review
tracker: rewrite notification code (30.13 KB, patch)
2015-10-19 21:04 UTC, Xavier Claessens
none Details | Review
tracker: rewrite notification code (32.83 KB, patch)
2015-10-21 18:47 UTC, Xavier Claessens
committed Details | Review

Description Bastien Nocera 2015-03-29 13:28:11 UTC
The tracker plugin, when something changes, sends a GrlMedia that contains just an "ID", and nothing else.

The GrlMedia should be pre-populated instead.
Comment 1 Bastien Nocera 2015-03-29 13:29:13 UTC
In Totem, I receive 4 "content-added" signals, with the same ID.
Comment 2 Xavier Claessens 2015-09-23 21:14:06 UTC
Created attachment 311983 [details] [review]
tracker: fix wrong usage of g_assert()
Comment 3 Xavier Claessens 2015-09-23 21:14:10 UTC
Created attachment 311984 [details] [review]
tracker: fix wrong usage of GHashTable
Comment 4 Bastien Nocera 2015-09-23 21:15:51 UTC
Review of attachment 311983 [details] [review]:

Thanks, for master and 0.2.x
Comment 5 Xavier Claessens 2015-09-23 21:16:47 UTC
Created attachment 311985 [details] [review]
tracker: fix wrong usage of GHashTable
Comment 6 Bastien Nocera 2015-09-23 21:17:33 UTC
Review of attachment 311984 [details] [review]:

What's "wrong" about it? Because this isn't immediately obvious. Should be mentioned in the commit message.

(Note that I don't understand what the original code is doing either)
Comment 7 Bastien Nocera 2015-09-23 21:18:07 UTC
Review of attachment 311985 [details] [review]:

As for the previous version.
Comment 8 Xavier Claessens 2015-09-24 14:03:16 UTC
I basically do that:

keys = g_hash_table_get_keys(table);
while (keys != NULL)
  {
    value = g_hash_table_lookup(table, keys->data);

    ...

    keys = g_list_delete_link (keys, keys);
  }

Which is a really inefficient way of iterating a hash table.
Comment 9 Bastien Nocera 2015-09-24 14:07:33 UTC
(In reply to Xavier Claessens from comment #8)
> I basically do that:

"It" does that?

> keys = g_hash_table_get_keys(table);
> while (keys != NULL)
>   {
>     value = g_hash_table_lookup(table, keys->data);
> 
>     ...
> 
>     keys = g_list_delete_link (keys, keys);
>   }
> 
> Which is a really inefficient way of iterating a hash table.

Right, so s/wrong/inefficient/ and I'm fine with the patch.
Comment 10 Xavier Claessens 2015-09-24 14:30:42 UTC
Created attachment 312063 [details] [review]
tracker: fix inefficient usage of GHashTable
Comment 11 Xavier Claessens 2015-09-24 14:31:03 UTC
Attachment 311983 [details] pushed as 6d5fb32 - tracker: fix wrong usage of g_assert()
Attachment 312063 [details] pushed as 5c81c2e - tracker: fix inefficient usage of GHashTable
Comment 12 Xavier Claessens 2015-09-24 14:32:33 UTC
Reopen, those were only small cleanup I found while investigating the real issue.
Comment 13 Xavier Claessens 2015-09-25 16:05:16 UTC
Created attachment 312156 [details] [review]
tracker: rewrite notification code

The GrlMedia object passed to "content-changed" signal was useless
because it didn't even contain an URL. To have at least that bare
minimum information we have to query tracker for all subjects when
we receive GraphUpdated.
Comment 14 Xavier Claessens 2015-09-25 16:06:28 UTC
That's a rewrite from scratch of the notification code, better review the whole file after applying the patch.
Comment 15 Xavier Claessens 2015-09-29 15:16:35 UTC
Comment on attachment 312156 [details] [review]
tracker: rewrite notification code

Removal doesn't work because we can't query the nie:url of a media being removed from tracker. I'm not sure how to handle that case. Looks like the only way is to keep a hash table of subject->url.
Comment 16 Xavier Claessens 2015-09-29 21:05:07 UTC
Created attachment 312375 [details] [review]
tracker: rewrite notification code

The GrlMedia object passed to "content-changed" signal was useless
because it didn't even contain an URL. To have at least that bare
minimum information we have to query tracker for all subjects when
we receive GraphUpdated.
Comment 17 Bastien Nocera 2015-10-05 10:25:07 UTC
Review of attachment 312375 [details] [review]:

Against which version of grilo did you generate the patch? It doesn't apply cleanly against master.
Comment 18 Bastien Nocera 2015-10-05 10:28:14 UTC
Review of attachment 312375 [details] [review]:

Never mind, problem on my side.
Comment 19 Bastien Nocera 2015-10-05 10:36:33 UTC
Review of attachment 312375 [details] [review]:

Looks good overall, will CC: people with more tracker experience.

::: src/tracker/grl-tracker-source-notif.c
@@ +106,1 @@
+  if (!source || !GRL_IS_TRACKER_SOURCE (source) ||

How can "!GRL_IS_TRACKER_SOURCE (source)" happen?

@@ +319,3 @@
+  singleton->conn = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL);
+  singleton->updates = g_hash_table_new (NULL, NULL);
+  singleton->cache = g_hash_table_new_full (NULL, NULL,

That's some pretty awful indentation right there.

@@ +322,3 @@
+      NULL,
+      (GDestroyNotify) media_info_free);
+  singleton->graph_updated_id = g_dbus_connection_signal_subscribe (

Ditto. I prefer:

foooooooooooooooooooo =
         g_dbus_connection... (singleton->conn,
...

to the lone parenthesis.
Comment 20 Bastien Nocera 2015-10-05 10:38:32 UTC
Philip, Carlos, can you do a review of the above patch? If you don't have a recent checkout, I applied it and put the final file at:
http://paste.fedoraproject.org/274817/40414511
Comment 21 Carlos Garnacho 2015-10-05 11:34:27 UTC
Review of attachment 312375 [details] [review]:

Looks mostly good to me, just one nit

::: src/tracker/grl-tracker-source-notif.c
@@ +270,3 @@
+    } else if (g_hash_table_lookup (self->updates, key) != ADDED) {
+      g_hash_table_insert (self->updates, key, CHANGED);
+    }

It is true that rdf:type is mostly static during the lifetime of a resource, so may be used to track insertions/deletions. However, those are not inserted at once (eg. tracker-extract might add further ones afterwards, or delete inaccurate ones for file formats where content sniffing is ambiguous). 

I would suggest adding querying tracker:id(nfo:FileDataObject) too, and doing

  if (predicate == self->rdf_type_id && object == self->nfo_filedataobject_id) 

both here and in the deletion handling, updates to other rdf:types should be dealt with as "changes".
Comment 22 Philip Withnall 2015-10-05 14:33:25 UTC
Review of attachment 312375 [details] [review]:

::: src/tracker/grl-tracker-source-notif.c
@@ +42,3 @@
+  /* subject id -> GrlSourceChangeType */
+  GHashTable *updates;
+  guint updates_count;

This could do with a comment saying it’s a count of the number of updates in progress at the moment, in order to freeze notification emissions until all updates are complete.

@@ +146,3 @@
   }
 
+  g_hash_table_remove_all (self->updates);}

`}` should be on a new line.

@@ -220,4 +181,4 @@
-  if (!grl_tracker_per_device_source)
-    source = grl_tracker_source_find ("");
-
-  if (!source && datasource)
+  tracker_sparql_cursor_next_async (cursor,
+                                    NULL,
+                                    update_cursor_next_cb,
+                                    self);

Take a ref to `self` here in case it disappears while the async call is in progress.

@@ -279,3 +208,4 @@
-  tracker_sparql_cursor_next_async (evt->cursor, NULL,
-                                    (GAsyncReadyCallback) tracker_evt_update_orphan_item_cb,
-                                    (gpointer) evt);
+  tracker_sparql_cursor_next_async (cursor,
+                                    NULL,
+                                    update_cursor_next_cb,
+                                    self);

`cursor` is leaked here. Also take a ref on `self` while the async call is in progress.

@@ +238,3 @@
+             class_name,
+             (unsigned long) g_variant_iter_n_children (iter2),
+             (unsigned long) g_variant_iter_n_children (iter1));

Use `G_GSSIZE_FORMAT` instead of casting.

@@ -364,6 +285,6 @@
-static void
-tracker_evt_update_items_cb (gpointer              key,
-                             gpointer              value,
... 3 more ...
+  if (query_needed) {
+    g_string_prepend (query,
+      "SELECT rdf:type(?urn) tracker:id(?urn) nie:dataSource(?urn) nie:url(?urn) "
... 3 more ...

Could do with a comment saying this is to drop the trailing comma.

@@ -376,6 +293,6 @@
-  g_assert (source != NULL);
-
-  if (!grl_tracker_source_can_notify (source)) {
... 3 more ...
+    GRL_DEBUG ("Query: %s", query->str);
+    tracker_sparql_connection_query_async (grl_tracker_connection,
+                                           query->str,
... 3 more ...

Need to take a ref on `self` in case it’s destroyed before the query completes.

@@ -492,1 +343,1 @@
     return;

Leaves the `singleton` in an inconsistent state.

@@ -492,2 +343,2 @@
     return;
   }

Can we return a GError here and let the caller issue the GRL_WARNING()? At least we’re one step closer to sensible error handling.

@@ +347,3 @@
+    GRL_WARNING ("Error: %s", error->message);
+    g_clear_error (&error);
+    return;

Leaks `cursor` and leaves the `singleton` in an inconsistent state.
Comment 23 Xavier Claessens 2015-10-19 21:02:44 UTC
Created attachment 313699 [details] [review]
tracker: rewrite notification code

The GrlMedia object passed to "content-changed" signal was useless
because it didn't even contain an URL. To have at least that bare
minimum information we have to query tracker for all subjects when
we receive GraphUpdated.
Comment 24 Xavier Claessens 2015-10-19 21:04:06 UTC
Created attachment 313700 [details] [review]
tracker: rewrite notification code

The GrlMedia object passed to "content-changed" signal was useless
because it didn't even contain an URL. To have at least that bare
minimum information we have to query tracker for all subjects when
we receive GraphUpdated.
Comment 25 Xavier Claessens 2015-10-19 21:07:50 UTC
About reffing self, it's not an object, it's just a struct that is forever leaked, I was too lazy to write the proper GObject boilerplate. It's more worse than before.

About leaving it in inconsistent state, if it fails once there is no reason it will succeed later, so better just always early return from grl_tracker_source_dbus_start_watch(). If we ever add grl_tracker_source_dbus_stop_watch() then it will have to take care freeing its content. It was already working like that before, so I'm not making stuff worse at least.
Comment 26 Xavier Claessens 2015-10-19 21:08:32 UTC
"It's more worse than before." --> read "It's not worse than before."
Comment 27 Bastien Nocera 2015-10-21 15:26:07 UTC
(In reply to Xavier Claessens from comment #25)
> About reffing self, it's not an object, it's just a struct that is forever
> leaked, I was too lazy to write the proper GObject boilerplate. It's more
> worse than before.

This patch (to be finished) would allow doing that:
http://paste.fedoraproject.org/281892/43318814

Could you integrate it in your patch?

> About leaving it in inconsistent state, if it fails once there is no reason
> it will succeed later, so better just always early return from
> grl_tracker_source_dbus_start_watch(). If we ever add
> grl_tracker_source_dbus_stop_watch() then it will have to take care freeing
> its content. It was already working like that before, so I'm not making
> stuff worse at least.

grl_tracker_source_dbus_stop_watch() is in that unfinished patch above.
Comment 28 Xavier Claessens 2015-10-21 18:47:50 UTC
Created attachment 313825 [details] [review]
tracker: rewrite notification code

The GrlMedia object passed to "content-changed" signal was useless
because it didn't even contain an URL. To have at least that bare
minimum information we have to query tracker for all subjects when
we receive GraphUpdated.
Comment 29 Bastien Nocera 2015-11-03 11:06:10 UTC
Review of attachment 313825 [details] [review]:

Looks good to me, granted that it works as expected.
Comment 30 Carlos Garnacho 2015-11-03 12:57:39 UTC
FWIW, the tracker bits look good to me.
Comment 31 Bastien Nocera 2015-11-03 13:02:28 UTC
Review of attachment 313825 [details] [review]:

Marking as acn, following Carlos' review.

Please commit to the 0.2 branch as well.
Comment 32 Xavier Claessens 2015-11-03 18:13:19 UTC
Attachment 313825 [details] pushed as 41e4a21 - tracker: rewrite notification code
Comment 33 Victor Toso 2016-08-24 16:54:57 UTC
Hi,

(In reply to Bastien Nocera from comment #19)
> Review of attachment 312375 [details] [review] [review]:
> 
> Looks good overall, will CC: people with more tracker experience.
> 
> ::: src/tracker/grl-tracker-source-notif.c
> @@ +106,1 @@
> +  if (!source || !GRL_IS_TRACKER_SOURCE (source) ||
> 
> How can "!GRL_IS_TRACKER_SOURCE (source)" happen?

I did not find the answer to this question and seems very important we do as we have the source but GRL_IS_TRACKER_SOURCE segfaults, crashing grl-tracker apps.

See: Bug 767684
Comment 34 Xavier Claessens 2016-08-24 18:44:46 UTC
The GRL_IS_TRACKER_SOURCE (source) was already there before my patch. I see no reason to keep it, grl_tracker_source_find() surely should only return GrlTrackerSource objects.