Created attachment 153448 [details] [review]
Add a popup menu to quickly enable/disable accounts

This patch is not yet done finished. Seems the info banner does not update correctly, and the default presence is not set when I enable the account.

I like the idea. But what about having a 2 entry menu:
X Enable
Y Disable

with X == the icon of the most available presence (the presence that we'll set on the account when activating it) and Y == the offline icon.

We'll unsensitive one entry according the current state of the account.

Created attachment 176615 [details] [review]
popup feature

I am a little doubtful about whether the reference to the context menu should be sunken or not. Rest works and is fully tested.

Review of attachment 176615 [details] [review]:

Looks functionally fine. Stylistic comments.

::: src/empathy-accounts-dialog.c
@@ +1528,3 @@
+    {
+      gtk_tree_path_free (path);
+      return FALSE;

I think a 'goto finally;' with a 'finally:' at the end is better.

@@ +1529,3 @@
+      gtk_tree_path_free (path);
+      return FALSE;
+    }

Empty line after block.

@@ +1537,3 @@
+  /* Get images for menu items */
+  presence = tp_account_manager_get_most_available_presence
+      (priv->account_manager, NULL, NULL);

Bad wrapping. '(' belongs on line above.

@@ +1539,3 @@
+      (priv->account_manager, NULL, NULL);
+  icon_name = empathy_icon_name_for_presence (presence);
+  image_enable = gtk_image_new_from_icon_name (icon_name, GTK_ICON_SIZE_MENU);

Can you fold 'presence' and 'icon_name' into this line?

@@ +1541,3 @@
+  image_enable = gtk_image_new_from_icon_name (icon_name, GTK_ICON_SIZE_MENU);
+  image_disable = gtk_image_new_from_icon_name
+      (empathy_icon_name_for_presence ( TP_CONNECTION_PRESENCE_TYPE_OFFLINE),

Bad wrapping. '(' belongs on line above. Extra space.

@@ +1557,3 @@
+      g_signal_connect_data (item_disable, "activate",
+          G_CALLBACK (accounts_dialog_treeview_enabled_cb),
+          g_object_ref (account), (GClosureNotify) g_object_unref, 0);

I wonder if you need to hold a reference to the account. If the account is removed from the treeview and finalized, we probably can't Enable/Disable it anyway.

Perhaps using tp_g_signal_connect_object(), which will disconnect the signal if the account finalizes is better.

@@ +1566,3 @@
+          g_object_ref (account), (GClosureNotify) g_object_unref, 0);
+      gtk_widget_set_sensitive (item_disable, FALSE);
+    }

Empty line here please.

Created attachment 176882 [details] [review]
Style Fixed

The style fixes.
If they look good, i'll do a fixup on the diffs.

Review of attachment 176882 [details] [review]:

::: src/empathy-accounts-dialog.c
@@ +1517,2 @@
   if (event->button != 3)
+    goto out;

'finally' is preferred over 'out'. So that we're following the try/finally/except language common to other languages.

@@ +1525,3 @@
   if (!gtk_tree_model_get_iter (model, &iter, path))
     {
       gtk_tree_path_free (path);

The point was to move this code that releases the resources into 'finally'.

@@ +1540,3 @@
+      GTK_ICON_SIZE_MENU);
+  image_disable = gtk_image_new_from_icon_name (
+      empathy_icon_name_for_presence ( TP_CONNECTION_PRESENCE_TYPE_OFFLINE),

Extra space.

@@ +1541,3 @@
+  image_disable = gtk_image_new_from_icon_name (
+      empathy_icon_name_for_presence ( TP_CONNECTION_PRESENCE_TYPE_OFFLINE),
+        GTK_ICON_SIZE_MENU);

These two uses of GTK_ICON.. don't line up, that looks strange to me.

@@ +1576,2 @@
   g_object_unref (account);
   gtk_tree_path_free (path);

Put finally above this section where you start freeing resources. Use tp_clear_object() and tp_clear_pointer().

> 'finally' is preferred over 'out'. So that we're following the
> try/finally/except language common to other languages.

Really?! When did this happen?

% git grep "goto finally;" | wc -l
3
% git grep "goto out;" | wc -l
73

Created attachment 176887 [details] [review]
Style Fixed

Created attachment 176889 [details] [review]
Style Fixed

I squashed the patch and style fixes into one and pushed to http://gitorious.org/glassrose-gnome/empathy/commits/enable-disable-accounts-popup-609568

Merged.This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.