After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 609568 - Add a popup menu to quickly enable/disable accounts
Add a popup menu to quickly enable/disable accounts
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: General
unspecified
Other All
: Normal normal
: ---
Assigned To: empathy-maint
empathy-maint
Depends on:
Blocks:
 
 
Reported: 2010-02-10 18:09 UTC by Xavier Claessens
Modified: 2010-12-24 08:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add a popup menu to quickly enable/disable accounts (3.39 KB, patch)
2010-02-10 18:09 UTC, Xavier Claessens
needs-work Details | Review
popup feature (4.54 KB, patch)
2010-12-17 20:32 UTC, Chandni Verma
reviewed Details | Review
Style Fixed (3.18 KB, patch)
2010-12-22 06:51 UTC, Chandni Verma
reviewed Details | Review
Style Fixed (3.30 KB, patch)
2010-12-22 09:05 UTC, Chandni Verma
none Details | Review
Style Fixed (3.31 KB, patch)
2010-12-22 09:21 UTC, Chandni Verma
none Details | Review

Description Xavier Claessens 2010-02-10 18:09:08 UTC
We should have a quick way to enable/disable an account. I suggest to right-click in the accounts treeview to get a popup menu.
Comment 1 Xavier Claessens 2010-02-10 18:09:11 UTC
Created attachment 153448 [details] [review]
Add a popup menu to quickly enable/disable accounts
Comment 2 Xavier Claessens 2010-02-10 18:10:12 UTC
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.
Comment 3 Guillaume Desmottes 2010-02-11 10:49:43 UTC
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.
Comment 4 Chandni Verma 2010-12-17 20:32:15 UTC
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.
Comment 5 Danielle Madeley 2010-12-20 06:53:13 UTC
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.
Comment 6 Chandni Verma 2010-12-22 06:51:33 UTC
Created attachment 176882 [details] [review]
Style Fixed

The style fixes.
If they look good, i'll do a fixup on the diffs.
Comment 7 Danielle Madeley 2010-12-22 06:56:12 UTC
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().
Comment 8 Jonny Lamb 2010-12-22 07:58:42 UTC
> '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
Comment 9 Chandni Verma 2010-12-22 09:05:32 UTC
Created attachment 176887 [details] [review]
Style Fixed
Comment 10 Chandni Verma 2010-12-22 09:21:07 UTC
Created attachment 176889 [details] [review]
Style Fixed
Comment 11 Chandni Verma 2010-12-23 11:42:51 UTC
I squashed the patch and style fixes into one and pushed to http://gitorious.org/glassrose-gnome/empathy/commits/enable-disable-accounts-popup-609568
Comment 12 Chandni Verma 2010-12-24 08:21:46 UTC
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.