In Totem, I receive 4 "content-added" signals, with the same ID.

Created attachment 311983 [details] [review]
tracker: fix wrong usage of g_assert()

Created attachment 311984 [details] [review]
tracker: fix wrong usage of GHashTable

Review of attachment 311983 [details] [review]:

Thanks, for master and 0.2.x

Created attachment 311985 [details] [review]
tracker: fix wrong usage of GHashTable

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)

Review of attachment 311985 [details] [review]:

As for the previous version.

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.

(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.

Created attachment 312063 [details] [review]
tracker: fix inefficient usage of GHashTable

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

Reopen, those were only small cleanup I found while investigating the real issue.

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.

That's a rewrite from scratch of the notification code, better review the whole file after applying the patch.

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.

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.

Review of attachment 312375 [details] [review]:

Against which version of grilo did you generate the patch? It doesn't apply cleanly against master.

Review of attachment 312375 [details] [review]:

Never mind, problem on my side.

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.

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

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".

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.

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.

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.

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.

"It's more worse than before." --> read "It's not worse than before."

(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.

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.

Review of attachment 313825 [details] [review]:

Looks good to me, granted that it works as expected.

FWIW, the tracker bits look good to me.

Review of attachment 313825 [details] [review]:

Marking as acn, following Carlos' review.

Please commit to the 0.2 branch as well.

Attachment 313825 [details] pushed as 41e4a21 - tracker: rewrite notification code

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

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.