Merge lp:~ted/hud/dee-sync into lp:hud/13.10

[Ted Gould]

Waiting for Dee Models to be synchronized before reporting they're ready

Makes it so that the HUD client waits until the DeeModels report that they are synchronized before saying that the models are ready. This should make it so that QtDee gets appropriate data when initialized.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:263
http://jenkins.qa.ubuntu.com/job/hud-ci/27/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/hud-raring-amd64-ci/31
    SUCCESS: http://jenkins.qa.ubuntu.com/job/hud-raring-i386-ci/16

Click here to trigger a rebuild:
http://s-jenkins:8080/job/hud-ci/27/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

As discussed on IRC, this should be done on the server side (where the shared model is created before its name passed over dbus), once it's there this isn't needed.

Moreover without that change this doesn't fix anything, cause the synchronized notification will be emitted if the client becomes leader of the swarm.

review: Disapprove

[Ted Gould (ted)]

While I understand that it doesn't fix this bug, my question would be whether this is an overall improvement. It seems like informing the client that we have models before they're sync'd isn't really useful for the client.

[Michal Hruby (mhr3)]

> While I understand that it doesn't fix this bug, my question would be whether
> this is an overall improvement. It seems like informing the client that we
> have models before they're sync'd isn't really useful for the client.

Only partially, if you only wait for the synchronization, this client can still become leader of the swarm (if the server crashes in between) which means it'll be considered synchronized and an empty model with uninitialized schema will be still passed to the client. Therefore it's not really protecting the client from uninitialized models.

Unmerged revisions

263. By Ted Gould on 2013-05-31

Attaching bug

262. By Ted Gould on 2013-05-31

Temp workaround for Dee bug

261. By Ted Gould on 2013-05-31

Connect to voice signals before playing with possible emitions and callbacks

260. By Ted Gould on 2013-05-31

Don't emit the connection changed signal until after we know both models are synchronized

=== modified file 'libhud-client/query.c'
--- libhud-client/query.c	2013-05-29 13:56:50 +0000
+++ libhud-client/query.c	2013-05-31 15:06:37 +0000
@@ -35,8 +35,15 @@
 	HudClientConnection * connection;
 	guint connection_changed_sig;
 	gchar * query;
+
 	DeeModel * results;
 	DeeModel * appstack;
+
+	gulong results_sync_signal;
+	gulong appstack_sync_signal;
+
+	gboolean results_initial_sync;
+	gboolean appstack_initial_sync;
 };
 
 #define HUD_CLIENT_QUERY_GET_PRIVATE(o) \
@@ -261,8 +268,22 @@
 static void
 connection_status (HudClientConnection * connection, gboolean connected, HudClientQuery * cquery)
 {
+	/* Clear results */
+	if (cquery->priv->results_sync_signal != 0) {
+		g_signal_handler_disconnect(cquery->priv->results, cquery->priv->results_sync_signal);
+		cquery->priv->results_sync_signal = 0;
+	}
 	g_clear_object(&cquery->priv->results);
+	cquery->priv->results_initial_sync = FALSE;
+
+	/* Clear appstack */
+	if (cquery->priv->appstack_sync_signal != 0) {
+		g_signal_handler_disconnect(cquery->priv->appstack, cquery->priv->appstack_sync_signal);
+		cquery->priv->appstack_sync_signal = 0;
+	}
 	g_clear_object(&cquery->priv->appstack);
+	cquery->priv->appstack_initial_sync = FALSE;
+
 	g_clear_object(&cquery->priv->proxy);
 
 	g_signal_emit(G_OBJECT(cquery), hud_client_query_signal_models_changed, 0);
@@ -275,6 +296,46 @@
 	return;
 }
 
+/* Called initially to check for sync and then called again when
+   the models update their synchronized property.  Hopefully we'll
+   get to a state where we've got nice data. */
+static void
+model_initial_sync_check (G_GNUC_UNUSED GObject * model, G_GNUC_UNUSED GParamSpec * spec, gpointer user_data)
+{
+	HudClientQuery * cquery = HUD_CLIENT_QUERY(user_data);
+
+	/* Since we only want to track the initial value, we only want
+	   to update if it's not sync'd yet */
+	if (!cquery->priv->results_initial_sync) {
+		cquery->priv->results_initial_sync = dee_shared_model_is_synchronized(DEE_SHARED_MODEL(cquery->priv->results));
+	}
+
+	if (!cquery->priv->appstack_initial_sync) {
+		cquery->priv->appstack_initial_sync = dee_shared_model_is_synchronized(DEE_SHARED_MODEL(cquery->priv->appstack));
+	}
+
+	/* If we've gotten sync'd on one of them we can stop watching the
+	   signal so hopeful we won't waste time with updates */
+	if (cquery->priv->results_initial_sync && cquery->priv->results_sync_signal != 0) {
+		g_signal_handler_disconnect(cquery->priv->results, cquery->priv->results_sync_signal);
+		cquery->priv->results_sync_signal = 0;
+	}
+
+	if (cquery->priv->appstack_initial_sync && cquery->priv->appstack_sync_signal != 0) {
+		g_signal_handler_disconnect(cquery->priv->appstack, cquery->priv->appstack_sync_signal);
+		cquery->priv->appstack_sync_signal = 0;
+	}
+
+	/* If we don't have one of these, we're done for now, wait some more. */
+	if (!cquery->priv->appstack_initial_sync || !cquery->priv->results_initial_sync) {
+		return;
+	}
+
+	g_signal_emit(G_OBJECT(cquery), hud_client_query_signal_models_changed, 0);
+	return;
+}
+
+/* Called when we have a new query constructed */
 static void
 new_query_cb (HudClientConnection * connection, const gchar * path, const gchar * results, const gchar * appstack, gpointer user_data)
 {
@@ -302,10 +363,6 @@
 		return;
 	}
 
-	/* Set up our models */
-	cquery->priv->results = dee_shared_model_new(results);
-	cquery->priv->appstack = dee_shared_model_new(appstack);
-
 	/* Watch for voice signals */
 	g_signal_connect_object (cquery->priv->proxy, "voice-query-loading",
 		G_CALLBACK (hud_client_query_voice_query_loading), G_OBJECT(cquery), 0);
@@ -314,7 +371,19 @@
 	g_signal_connect_object (cquery->priv->proxy, "voice-query-heard-something",
 	    G_CALLBACK (hud_client_query_voice_query_heard_something), G_OBJECT(cquery), 0);
 
-	g_signal_emit(G_OBJECT(cquery), hud_client_query_signal_models_changed, 0);
+	/* Set up our models */
+	cquery->priv->results = dee_shared_model_new(results);
+	cquery->priv->appstack = dee_shared_model_new(appstack);
+
+	model_initial_sync_check(NULL, NULL, cquery);
+
+	if (!cquery->priv->results_initial_sync) {
+		cquery->priv->results_sync_signal = g_signal_connect(G_OBJECT(cquery->priv->results), "notify::synchronized", G_CALLBACK(model_initial_sync_check), cquery);
+	}
+
+	if (!cquery->priv->appstack_initial_sync) {
+		cquery->priv->appstack_sync_signal = g_signal_connect(G_OBJECT(cquery->priv->appstack), "notify::synchronized", G_CALLBACK(model_initial_sync_check), cquery);
+	}
 
 	g_object_unref(cquery);
 
@@ -336,8 +405,20 @@
 		_hud_query_com_canonical_hud_query_call_close_query_sync(self->priv->proxy, NULL, NULL);
 	}
 
+	/* Clear results */
+	if (self->priv->results_sync_signal != 0) {
+		g_signal_handler_disconnect(self->priv->results, self->priv->results_sync_signal);
+		self->priv->results_sync_signal = 0;
+	}
 	g_clear_object(&self->priv->results);
+
+	/* Clear appstack */
+	if (self->priv->appstack_sync_signal != 0) {
+		g_signal_handler_disconnect(self->priv->appstack, self->priv->appstack_sync_signal);
+		self->priv->appstack_sync_signal = 0;
+	}
 	g_clear_object(&self->priv->appstack);
+
 	g_clear_object(&self->priv->proxy);
 	g_clear_object(&self->priv->connection);
 

=== modified file 'tests/test-hud-client-query.c'
--- tests/test-hud-client-query.c	2013-05-14 14:50:59 +0000
+++ tests/test-hud-client-query.c	2013-05-31 15:06:37 +0000
@@ -69,6 +69,11 @@
 	
 	g_free(search);
 
+    /* FIXME: This is required because Dee doesn't cancel and
+	   clean up it's handlers on dispose:
+	   https://code.launchpad.net/~ted/dee/errors-fix/+merge/166811
+	*/
+    hud_test_utils_process_mainloop (100);
 	g_object_unref(query);
 
 	g_object_unref(client_connection);
@@ -126,6 +131,11 @@
 	g_object_unref(testcon);
 
 	/* Clean up */
+    /* FIXME: This is required because Dee doesn't cancel and
+	   clean up it's handlers on dispose:
+	   https://code.launchpad.net/~ted/dee/errors-fix/+merge/166811
+	*/
+    hud_test_utils_process_mainloop (100);
 	g_object_unref(query);
 	g_object_unref(client_connection);
 
@@ -167,6 +177,11 @@
   dbus_mock_assert_method_call_results (connection, DBUS_NAME, QUERY_PATH,
       "UpdateQuery", "\\(\\[\\(\\d+, \\[<'test2'>\\]\\)\\],\\)");
 
+  /* FIXME: This is required because Dee doesn't cancel and
+     clean up it's handlers on dispose:
+     https://code.launchpad.net/~ted/dee/errors-fix/+merge/166811
+  */
+  hud_test_utils_process_mainloop (100);
   g_object_unref(query);
 
   hud_test_utils_stop_hud_service (service, connection, results_model,
@@ -263,6 +278,11 @@
   dbus_mock_assert_method_call_results (connection, DBUS_NAME, QUERY_PATH,
       "UpdateApp", "\\(\\[\\(\\d+, \\[<'application-id'>\\]\\)\\],\\)");
 
+  /* FIXME: This is required because Dee doesn't cancel and
+     clean up it's handlers on dispose:
+     https://code.launchpad.net/~ted/dee/errors-fix/+merge/166811
+  */
+  hud_test_utils_process_mainloop (100);
   g_object_unref(query);
 
   hud_test_utils_stop_hud_service (service, connection, results_model,
@@ -304,6 +324,11 @@
   dbus_mock_assert_method_call_results (connection, DBUS_NAME, QUERY_PATH,
       "ExecuteCommand", "\\(\\[\\(\\d+, \\[<uint64 4321>, <uint32 1234>\\]\\)\\],\\)");
 
+  /* FIXME: This is required because Dee doesn't cancel and
+     clean up it's handlers on dispose:
+     https://code.launchpad.net/~ted/dee/errors-fix/+merge/166811
+  */
+  hud_test_utils_process_mainloop (100);
   g_object_unref(query);
 
   hud_test_utils_stop_hud_service (service, connection, results_model,
@@ -346,6 +371,12 @@
       "ExecuteParameterized", "\\(\\[\\(\\d+, \\[<uint64 4321>, <uint32 1234>\\]\\)\\],\\)");
 
   g_object_unref(param);
+
+  /* FIXME: This is required because Dee doesn't cancel and
+     clean up it's handlers on dispose:
+     https://code.launchpad.net/~ted/dee/errors-fix/+merge/166811
+  */
+  hud_test_utils_process_mainloop (100);
   g_object_unref(query);
 
   hud_test_utils_stop_hud_service (service, connection, results_model,
@@ -416,6 +447,11 @@
       "\\(\\[\\(\\d+, \\[<'undo'>, <uint32 53312>\\]\\)\\],\\)");
   dbus_mock_clear_method_calls (connection, DBUS_NAME, QUERY_PATH);
 
+  /* FIXME: This is required because Dee doesn't cancel and
+     clean up it's handlers on dispose:
+     https://code.launchpad.net/~ted/dee/errors-fix/+merge/166811
+  */
+  hud_test_utils_process_mainloop (100);
   g_object_unref(query);
 
   hud_test_utils_stop_hud_service (service, connection, results_model,