Merge lp:~larsu/indicator-messages/watch-desktop-files into lp:indicator-messages/12.10

[Lars Karlitski]

[jenkins (martin-mrazik+qa)]

PASSED: Continuous integration, rev:310
http://jenkins.qa.ubuntu.com/job/indicator-messages-ci/13/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-messages-ci/./label=quantal/13/console

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

Overall this is a good patch as usual. :)

Needs Fixing:

  * in app-section.c, the file-scope variable destroy_count should be static

  * the new function app_section_get_name() is not used anywhere, was this included by accident?

Comment:

  * in remove_application(), the new call to g_hash_table_remove() added when (section == NULL) should be removed.

  * remove_section() is wrapped in a g_hash_table_lookup() safety check when called from remove_application(), but not when called from the destroy callback. Both this and the previous bullet point could be addressed by moving the g_hash_table_lookup() + g_warning() from remove_application() to remove_section().

review: Needs Fixing

[Lars Karlitski (larsu)]

> Overall this is a good patch as usual. :)
>
> Needs Fixing:
>
> * in app-section.c, the file-scope variable destroy_count should be static

Fixed in r311. (you meant destroy_signal, right?)

> * the new function app_section_get_name() is not used anywhere, was this
> included by accident?

That's a very old function. Removed it anyway in r312.

> Comment:
>
> * in remove_application(), the new call to g_hash_table_remove() added when
> (section == NULL) should be removed.

Right, fixed in r313.

> * remove_section() is wrapped in a g_hash_table_lookup() safety check when
> called from remove_application(), but not when called from the destroy
> callback. Both this and the previous bullet point could be addressed by moving
> the g_hash_table_lookup() + g_warning() from remove_application() to
> remove_section().

I didn't do this because remove_section is called from the "destroy" signal from the AppSection. There's no need to look up the AppSection again in the hash table, because we already have it. That said, are you okay with keeping this as it is for now? I plan on refactoring thatq bit starting next cycle anyway.

Thanks for reviewing!

[jenkins (martin-mrazik+qa)]

PASSED: Continuous integration, rev:313
http://jenkins.qa.ubuntu.com/job/indicator-messages-ci/15/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-messages-ci/./label=quantal/15/console

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

Yes, I'm fine with that part as-is, especially if it's getting refactored away :)

review: Approve

[jenkins (martin-mrazik+qa)]

FAILED: Autolanding.
No commit message was specified.
http://jenkins.qa.ubuntu.com/job/indicator-messages-autolanding/7/

review: Needs Fixing (continuous-integration)

=== modified file 'src/app-section.c'
--- src/app-section.c	2012-08-27 15:34:30 +0000
+++ src/app-section.c	2012-09-05 19:31:19 +0000
@@ -37,7 +37,7 @@
 struct _AppSectionPrivate
 {
 	GDesktopAppInfo * appinfo;
-	guint unreadcount;
+	GFileMonitor *desktop_file_monitor;
 
 	IndicatorDesktopShortcuts * ids;
 
@@ -64,6 +64,7 @@
 };
 
 static GParamSpec *properties[NUM_PROPERTIES];
+static guint destroy_signal;
 
 /* Prototypes */
 static void app_section_class_init   (AppSectionClass *klass);
@@ -98,6 +99,11 @@
 						 const gchar *action_name,
 						 gpointer user_data);
 static gboolean	action_draws_attention		(GVariant *state);
+static void	desktop_file_changed_cb		(GFileMonitor      *monitor,
+						 GFile             *file,
+						 GFile             *other_file,
+						 GFileMonitorEvent  event,
+						 gpointer           user_data);
 
 /* GObject Boilerplate */
 G_DEFINE_TYPE (AppSection, app_section, G_TYPE_OBJECT);
@@ -138,6 +144,14 @@
 								  G_PARAM_READABLE);
 
 	g_object_class_install_properties (object_class, NUM_PROPERTIES, properties);
+
+	destroy_signal = g_signal_new ("destroy",
+				       APP_SECTION_TYPE,
+				       G_SIGNAL_RUN_FIRST,
+				       0,
+				       NULL, NULL,
+				       g_cclosure_marshal_VOID__VOID,
+				       G_TYPE_NONE, 0);
 }
 
 static void
@@ -151,7 +165,6 @@
 	priv = self->priv;
 
 	priv->appinfo = NULL;
-	priv->unreadcount = 0;
 
 	priv->menu = g_menu_new ();
 	priv->static_shortcuts = g_simple_action_group_new ();
@@ -209,12 +222,18 @@
 		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
 	}
 }
+
 static void
 app_section_dispose (GObject *object)
 {
 	AppSection * self = APP_SECTION(object);
 	AppSectionPrivate * priv = self->priv;
 
+	if (priv->desktop_file_monitor) {
+		g_signal_handlers_disconnect_by_func (priv->desktop_file_monitor, desktop_file_changed_cb, self);
+		g_clear_object (&priv->desktop_file_monitor);
+	}
+
 	g_clear_object (&priv->menu);
 	g_clear_object (&priv->static_shortcuts);
 
@@ -289,6 +308,9 @@
 							       G_KEY_FILE_DESKTOP_GROUP,
 							       "X-MessagingMenu-UsesChatSection",
 							       &error);
+
+	g_object_notify_by_pspec (G_OBJECT (self), properties[PROP_USES_CHAT_STATUS]);
+
 	if (error) {
 		if (error->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND) {
 			g_warning ("could not read X-MessagingMenu-UsesChatSection: %s",
@@ -298,34 +320,48 @@
 		goto out;
 	}
 
-	if (self->priv->uses_chat_status)
-		g_object_notify_by_pspec (G_OBJECT (self), properties[PROP_USES_CHAT_STATUS]);
-
 out:
 	g_key_file_free (keyfile);
 	g_free (contents);
 }
 
 static void
-app_section_set_app_info (AppSection *self,
-			  GDesktopAppInfo *appinfo)
+g_menu_clear (GMenu *menu)
+{
+	gint n_items = g_menu_model_get_n_items (G_MENU_MODEL (menu));
+
+	while (n_items--)
+		g_menu_remove (menu, 0);
+}
+
+static void
+g_simple_action_group_clear (GSimpleActionGroup *group)
+{
+	gchar **actions;
+	gchar **it;
+
+	actions = g_action_group_list_actions (G_ACTION_GROUP (group));
+	for (it = actions; *it; it++)
+		g_simple_action_group_remove (group, *it);
+
+	g_strfreev (actions);
+}
+
+static void
+app_section_update_menu (AppSection *self)
 {
 	AppSectionPrivate *priv = self->priv;
 	GSimpleAction *launch;
 	GFile *keyfile;
 	GMenuItem *item;
 	gchar *iconstr;
-
-	g_return_if_fail (priv->appinfo == NULL);
-
-	if (appinfo == NULL) {
-		g_warning ("appinfo must not be NULL");
-		return;
-	}
-
-	priv->appinfo = g_object_ref (appinfo);
-
-	launch = g_simple_action_new_stateful ("launch", NULL, g_variant_new_boolean (FALSE));
+	gboolean is_running;
+
+	g_menu_clear (priv->menu);
+	g_simple_action_group_clear (priv->static_shortcuts);
+
+	is_running = priv->name_watch_id > 0;
+	launch = g_simple_action_new_stateful ("launch", NULL, g_variant_new_boolean (is_running));
 	g_signal_connect (launch, "activate", G_CALLBACK (activate_cb), self);
 	g_signal_connect (launch, "change-state", G_CALLBACK (launch_action_change_state), self);
 	g_simple_action_group_insert (priv->static_shortcuts, G_ACTION (launch));
@@ -366,12 +402,63 @@
 
 	keyfile = g_file_new_for_path (g_desktop_app_info_get_filename (priv->appinfo));
 	g_file_load_contents_async (keyfile, NULL, keyfile_loaded, self);
+
+	g_object_notify_by_pspec (G_OBJECT (self), properties[PROP_ACTIONS]);
+
 	g_object_unref (keyfile);
+	g_object_unref (launch);
+}
+
+static void
+desktop_file_changed_cb (GFileMonitor      *monitor,
+			 GFile             *file,
+			 GFile             *other_file,
+			 GFileMonitorEvent  event,
+			 gpointer           user_data)
+{
+	AppSection *self = user_data;
+
+	if (event == G_FILE_MONITOR_EVENT_CHANGED) {
+		app_section_update_menu (self);
+	}
+	else if (event == G_FILE_MONITOR_EVENT_DELETED ||
+		 event == G_FILE_MONITOR_EVENT_UNMOUNTED) {
+		g_signal_emit (self, destroy_signal, 0);
+	}
+}
+
+static void
+app_section_set_app_info (AppSection *self,
+			  GDesktopAppInfo *appinfo)
+{
+	AppSectionPrivate *priv = self->priv;
+	GFile *desktop_file;
+	GError *error = NULL;
+
+	g_return_if_fail (priv->appinfo == NULL);
+	g_return_if_fail (priv->desktop_file_monitor == NULL);
+
+	if (appinfo == NULL) {
+		g_warning ("appinfo must not be NULL");
+		return;
+	}
+
+	priv->appinfo = g_object_ref (appinfo);
+
+	desktop_file = g_file_new_for_path (g_desktop_app_info_get_filename (appinfo));
+	priv->desktop_file_monitor = g_file_monitor (desktop_file, G_FILE_MONITOR_SEND_MOVED, NULL, &error);
+	if (priv->desktop_file_monitor == NULL) {
+		g_warning ("unable to watch desktop file: %s", error->message);
+		g_error_free (error);
+	}
+	g_signal_connect (priv->desktop_file_monitor, "changed",
+			  G_CALLBACK (desktop_file_changed_cb), self);
 
 	g_object_notify_by_pspec (G_OBJECT (self), properties[PROP_APPINFO]);
-	g_object_notify_by_pspec (G_OBJECT (self), properties[PROP_ACTIONS]);
-
-	g_object_unref (launch);
+
+	app_section_update_menu (self);
+
+	g_object_unref (desktop_file);
 }
 
 AppSection *
@@ -407,25 +494,6 @@
 	g_simple_action_set_state (action, value);
 }
 
-guint
-app_section_get_count (AppSection * self)
-{
-	AppSectionPrivate * priv = self->priv;
-
-	return priv->unreadcount;
-}
-
-const gchar *
-app_section_get_name (AppSection * self)
-{
-	AppSectionPrivate * priv = self->priv;
-
-	if (priv->appinfo) {
-		return g_app_info_get_name(G_APP_INFO(priv->appinfo));
-	}
-	return NULL;
-}
-
 const gchar *
 app_section_get_desktop (AppSection * self)
 {

=== modified file 'src/app-section.h'
--- src/app-section.h	2012-08-19 20:41:23 +0000
+++ src/app-section.h	2012-09-05 19:31:19 +0000
@@ -51,8 +51,6 @@
 
 GType app_section_get_type (void);
 AppSection * app_section_new (GDesktopAppInfo *appinfo);
-guint app_section_get_count (AppSection * appitem);
-const gchar * app_section_get_name (AppSection * appitem);
 const gchar * app_section_get_desktop (AppSection * appitem);
 GActionGroup * app_section_get_actions (AppSection *self);
 GMenuModel * app_section_get_menu (AppSection *appitem);

=== modified file 'src/messages-service.c'
--- src/messages-service.c	2012-08-27 13:38:04 +0000
+++ src/messages-service.c	2012-09-05 19:31:19 +0000
@@ -45,7 +45,6 @@
 static GMenuModel *chat_section;
 static GSettings *settings;
 
-
 static gchar *
 g_app_info_get_simple_id (GAppInfo *appinfo)
 {
@@ -138,6 +137,23 @@
 	g_object_unref (first_section);
 }
 
+static void
+remove_section (AppSection  *section,
+		const gchar *id)
+{
+	int pos = g_menu_find_section (menu, app_section_get_menu (section));
+	if (pos >= 0)
+		g_menu_remove (menu, pos);
+	g_action_muxer_remove (action_muxer, id);
+
+	g_signal_handlers_disconnect_by_func (section, actions_changed, NULL);
+	g_signal_handlers_disconnect_by_func (section, draws_attention_changed, NULL);
+	g_signal_handlers_disconnect_by_func (section, uses_chat_status_changed, NULL);
+	g_signal_handlers_disconnect_by_func (section, remove_section, NULL);
+
+	g_hash_table_remove (applications, id);
+}
+
 static AppSection *
 add_application (const gchar *desktop_id)
 {
@@ -167,6 +183,11 @@
 				  G_CALLBACK (draws_attention_changed), NULL);
 		g_signal_connect (section, "notify::uses-chat-status",
 				  G_CALLBACK (uses_chat_status_changed), NULL);
+		g_signal_connect_data (section, "destroy",
+				       G_CALLBACK (remove_section),
+				       g_strdup (id),
+				       (GClosureNotify) g_free,
+				       0);
 
 		/* TODO insert it at the right position (alphabetically by application name) */
 		menuitem = g_menu_item_new_section (NULL, app_section_get_menu (section));
@@ -197,20 +218,12 @@
 
 	section = g_hash_table_lookup (applications, id);
 	if (section) {
-		int pos = g_menu_find_section (menu, app_section_get_menu (section));
-		if (pos >= 0)
-			g_menu_remove (menu, pos);
-		g_action_muxer_remove (action_muxer, id);
-
-		g_signal_handlers_disconnect_by_func (section, actions_changed, NULL);
-		g_signal_handlers_disconnect_by_func (section, draws_attention_changed, NULL);
-		g_signal_handlers_disconnect_by_func (section, uses_chat_status_changed, NULL);
+		remove_section (section, id);
 	}
 	else {
 		g_warning ("could not remove '%s', it's not registered", desktop_id);
 	}
 	
-	g_hash_table_remove (applications, id);
 	g_free (id);
 	g_object_unref (appinfo);
 }