Merge lp:~desrt/indicator-appmenu/hud-rewrite-wip into lp:indicator-appmenu/0.4

[Allison Lortie]

Switch to identifying items by unique identifier rather than their index in a possibly-changing list of results.

This should improve the reliability of item activation.

[Charles Kerr (charlesk)]

I haven't been following hud enough to review the 'why' of this patch, but the 'how' looks fine except for item_key, which can be leaked iff its type isn't uint64

[lp:~desrt/indicator-appmenu/hud-rewrite-wip]

205. By Allison Lortie on 2012-03-08

hud service: don't leak item key in case of type error

Caught by Charles Kerr.

=== modified file 'src/hud-service.c'
--- src/hud-service.c	2012-03-05 04:46:53 +0000
+++ src/hud-service.c	2012-03-08 17:50:26 +0000
@@ -36,13 +36,9 @@
 describe_query (HudQuery *query)
 {
   GVariantBuilder builder;
-  GVariant *query_key;
-  guint64 generation;
   gint n, i;
 
-  generation = hud_query_get_generation (query);
   n = hud_query_get_n_results (query);
-  query_key = hud_query_get_query_key (query);
 
   g_variant_builder_init (&builder, G_VARIANT_TYPE ("(sa(sssssv)v)"));
 
@@ -63,12 +59,12 @@
                              hud_item_get_app_icon (item),
                              hud_item_get_item_icon (item),
                              "" /* complete text */ , "" /* accel */,
-                             g_variant_new ("(vtu)", query_key, generation, i));
+                             g_variant_new_uint64 (hud_item_get_id (item)));
     }
   g_variant_builder_close (&builder);
 
   /* Query key */
-  g_variant_builder_add (&builder, "v", query_key);
+  g_variant_builder_add (&builder, "v", hud_query_get_query_key (query));
 
   return g_variant_builder_end (&builder);
 }
@@ -84,25 +80,6 @@
                                  describe_query (query), NULL);
 }
 
-static gboolean
-unpack_key (GVariant  *parameters,
-            GVariant **query_key,
-            guint64   *generation,
-            guint     *i)
-{
-  gboolean success;
-  GVariant *key;
-
-  g_variant_get_child (parameters, 0, "v", &key);
-
-  if ((success = g_variant_is_of_type (key, G_VARIANT_TYPE ("(vtu)"))))
-    g_variant_get (key, "(vtu)", query_key, generation, i);
-
-  g_variant_unref (key);
-
-  return success;
-}
-
 static GVariant *
 unpack_platform_data (GVariant *parameters)
 {
@@ -145,48 +122,33 @@
 
   else if (g_str_equal (method_name, "ExecuteQuery"))
     {
-      GVariant *query_key;
-      guint64 generation;
-      HudQuery *query;
-      guint i;
-
-      /* FIXME: ExecuteQuery DBus API should be changed to make this less awkward. */
-      if (!unpack_key (parameters, &query_key, &generation, &i))
-        {
-          g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS,
-                                                 "query key has invalid format");
-          return;
-        }
-
-      query = hud_query_lookup (query_key);
-      g_variant_unref (query_key);
-
-      if (query == NULL)
-        {
-          g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS,
-                                                 "query specified by query key does not exist");
-          return;
-        }
-
-      if (generation != hud_query_get_generation (query))
-        {
-          g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS,
-                                                 "query specified by query key has changed (%lu v %lu)",
-                                                 generation, hud_query_get_generation (query));
-          return;
-        }
-
-      if (i < hud_query_get_n_results (query))
-        {
-          GVariant *platform_data;
-          HudResult *result;
-
-          result = hud_query_get_result_by_index (query, i);
-
-          platform_data = unpack_platform_data (parameters);
-          hud_item_activate (hud_result_get_item (result), platform_data);
-          g_variant_unref (platform_data);
-        }
+      GVariant *platform_data;
+      GVariant *item_key;
+      HudItem *item;
+
+      g_variant_get_child (parameters, 0, "v", &item_key);
+
+      if (!g_variant_is_of_type (item_key, G_VARIANT_TYPE_UINT64))
+        {
+          g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS,
+                                                 "item key has invalid format");
+          g_variant_unref (item_key);
+          return;
+        }
+
+      item = hud_item_lookup (g_variant_get_uint64 (item_key));
+      g_variant_unref (item_key);
+
+      if (item == NULL)
+        {
+          g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS,
+                                                 "item specified by item key does not exist");
+          return;
+        }
+
+      platform_data = unpack_platform_data (parameters);
+      hud_item_activate (item, platform_data);
+      g_variant_unref (platform_data);
 
       g_dbus_method_invocation_return_value (invocation, NULL);
     }

=== modified file 'src/huditem.c'
--- src/huditem.c	2012-03-07 04:11:28 +0000
+++ src/huditem.c	2012-03-08 17:50:26 +0000
@@ -43,6 +43,9 @@
  * This is the class vtable for #HudItem.
  **/
 
+static GHashTable *hud_item_table;
+static guint64 hud_item_next_id;
+
 struct _HudItemPrivate
 {
   GObject parent_instance;
@@ -52,6 +55,7 @@
   HudStringList *tokens;
   gboolean enabled;
   guint usage;
+  guint64 id;
 };
 
 G_DEFINE_TYPE (HudItem, hud_item, G_TYPE_OBJECT)
@@ -61,6 +65,7 @@
 {
   HudItem *item = HUD_ITEM (object);
 
+  g_hash_table_remove (hud_item_table, &item->priv->id);
   hud_string_list_unref (item->priv->tokens);
   g_free (item->priv->desktop_file);
 
@@ -79,6 +84,8 @@
 {
   GObjectClass *gobject_class = G_OBJECT_CLASS (class);
 
+  hud_item_table = g_hash_table_new (g_int64_hash, g_int64_equal);
+
   gobject_class->finalize = hud_item_finalize;
 
   g_type_class_add_private (class, sizeof (HudItemPrivate));
@@ -110,6 +117,9 @@
   item->priv->tokens = hud_string_list_ref (tokens);
   item->priv->desktop_file = g_strdup (desktop_file);
   item->priv->enabled = enabled;
+  item->priv->id = hud_item_next_id++;
+
+  g_hash_table_insert (hud_item_table, &item->priv->id, item);
 
   //item->usage = usage_tracker_get_usage (usage_tracker_get_instance (), desktop_file, identifier);
 
@@ -241,3 +251,37 @@
 {
   return item->priv->enabled;
 }
+
+/**
+ * hud_item_get_id:
+ * @item: a #HudItem
+ *
+ * Gets the unique identifier of this #HudItem.
+ *
+ * The identifier can be used to refetch the item using
+ * hud_item_lookup() for as long as the item has not been destroyed.
+ *
+ * Returns: the ID of the item
+ **/
+guint64
+hud_item_get_id (HudItem *item)
+{
+  return item->priv->id;
+}
+
+/**
+ * hud_item_lookup:
+ * @id: an item identifier
+ *
+ * Looks up a #HudItem by its ID.
+ *
+ * The ID for a #HudItem can be queried with hud_item_get_id().
+ *
+ * Returns: (transfer none): the #HudItem with the given @id, or %NULL
+ *   if none exists
+ **/
+HudItem *
+hud_item_lookup (guint64 id)
+{
+  return g_hash_table_lookup (hud_item_table, &id);
+}

=== modified file 'src/huditem.h'
--- src/huditem.h	2012-03-04 16:08:53 +0000
+++ src/huditem.h	2012-03-08 17:50:26 +0000
@@ -70,5 +70,7 @@
 const gchar *           hud_item_get_item_icon                          (HudItem       *item);
 guint                   hud_item_get_usage                              (HudItem       *item);
 gboolean                hud_item_get_enabled                            (HudItem       *item);
+guint64                 hud_item_get_id                                 (HudItem       *item);
+HudItem *               hud_item_lookup                                 (guint64        id);
 
 #endif /* __HUD_ITEM_H__ */

=== modified file 'src/hudquery.c'
--- src/hudquery.c	2012-03-01 04:44:19 +0000
+++ src/hudquery.c	2012-03-08 17:50:26 +0000
@@ -35,10 +35,6 @@
  * The query maintains a list of results from the search which are
  * sorted by relevance and accessible by index.  Contrast this with the
  * stateless nature of #HudSource.
- *
- * The query has a "generation" counter that is bumped each time the
- * results list changes.  This can be used to guard against stale
- * index-based references to specific search results.
  **/
 
 /**
@@ -56,7 +52,6 @@
   gint num_results;
   guint refresh_id;
 
-  guint64 generation;
   GPtrArray *results;
 };
 
@@ -105,7 +100,6 @@
   guint max_usage = 0;
 
   g_ptr_array_set_size (query->results, 0);
-  query->generation++;
 
   if (query->search_string[0] != '\0')
     hud_source_search (query->source, query->results, query->search_string);
@@ -173,9 +167,6 @@
    * @query: a #HudQuery
    *
    * Indicates that the results of @query have changed.
-   *
-   * The generation counter is increased each time this signal fires.
-   * See hud_query_get_generation().
    **/
   hud_query_changed_signal = g_signal_new ("changed", HUD_TYPE_QUERY, G_SIGNAL_RUN_LAST, 0, NULL,
                                            NULL, g_cclosure_marshal_VOID__VOID, G_TYPE_NONE, 0);
@@ -282,24 +273,6 @@
 }
 
 /**
- * hud_query_get_generation:
- * @query: a #HudQuery
- *
- * Gets the current value of the generation counter for query.
- *
- * The generation counter is incremented each time the #HudQuery emits a
- * the ::changed signal.  Its purpose is to guard against stale
- * index-based references to results of the query.
- *
- * Returns: the generation number
- **/
-guint64
-hud_query_get_generation (HudQuery *query)
-{
-  return query->generation;
-}
-
-/**
  * hud_query_get_n_results:
  * @query: a #HudQuery
  *

=== modified file 'src/hudquery.h'
--- src/hudquery.h	2012-02-28 18:29:03 +0000
+++ src/hudquery.h	2012-03-08 17:50:26 +0000
@@ -36,8 +36,6 @@
                                                                          const gchar *search_string,
                                                                          gint         num_results);
 
-guint64                 hud_query_get_generation                        (HudQuery    *query);
-
 GVariant *              hud_query_get_query_key                         (HudQuery    *query);
 
 HudResult *             hud_query_get_result_by_index                   (HudQuery    *query,