GNOME Bugzilla – Bug 626009
[PATCH] Allow selection of persona to chat to
Last modified: 2010-08-16 16:21:34 UTC
This branch adds submenus to the "Chat", "Audio Call" and "Video Call" entries in the context menu of a contact with multiple Personas. The submenus list all the Personas in the Individual, allowing selection of the one you'd like to chat with or call. http://git.collabora.co.uk/?p=user/pwith/empathy;a=shortlog;h=refs/heads/linking-ui-rebase5 It doesn't modify any of the other menu entries (such as "Send File"), since I'm not sure we want to clutter the menu. Ideas are welcome. As such, the first commit probably isn't quite ready to apply. The second commit in the branch changes the contact information dialogue to show information for all the Personas in an Individual. It remains the same for Individuals which only have one Persona, and just adds extra instances of EmpathyContactWidget for extra Personas. This is ugly, but I think it'll do until a proper EmpathyIndividualWidget is written (which I'm partially working on as part of the linking dialogue work). Again, thoughts are welcome as to how this widget should look.
This branch depends on folks 0.1.13 which doesn't exist yet. Also, I'm not convinced this is the best approach. Having submenus for some entries but not for others seem wrong to be, we should stay coherent. On the other hand, as you said, we shouldn't clutter the menu and I agreed with that. What about keeping the chat, VoIP, FT entries unchanged and add an extra entry at end of the menu growing a submenu allowing to choose a specific persona. Each persona would have its own contact-menu. So something like: * Chat * Call [...] * Personas -> alice@gmail.com -> * Chat * Call -> alice@hotmail.com -> * Chat * Call We should probably find a better name than "Personas" but you get the idea.
(In reply to comment #1) > This branch depends on folks 0.1.13 which doesn't exist yet. I'll do a release before merging this, once we've decided what the best approach is. > Also, I'm not convinced this is the best approach. Having submenus for some > entries but not for others seem wrong to be, we should stay coherent. On the > other hand, as you said, we shouldn't clutter the menu and I agreed with that. > > What about keeping the chat, VoIP, FT entries unchanged and add an extra entry > at end of the menu growing a submenu allowing to choose a specific persona. > Each persona would have its own contact-menu. There are two problems with this approach: * In order to keep the Chat, VoIP, FT, etc. entries unchanged, we'd need some notion of a "default" persona for each individual. From the discussion Sjoerd, Rob and I had last night, this is a bad idea, since the best persona for one action (e.g.) chatting might be different from the best persona for another (e.g. VoIP). We were thinking about having some per-action ranking of personas, but that's quite complex, and there certainly isn't enough time to implement it this cycle. * When I want to chat to someone, I'm typically thinking "I want to chat", rather than "I want to interact with the foo@jabber.org persona". I think it's better to have the actions (chat, VoIP, etc.) higher in the menu than the personas for this reason. I could be swayed, though. > So something like: > > * Chat > * Call > [...] > * Personas -> alice@gmail.com -> * Chat > * Call > -> alice@hotmail.com -> * Chat > * Call > > We should probably find a better name than "Personas" but you get the idea.
Work that I'm doing on EmpathyIndividualWidget has ended up encompassing the work I did previously on the Information dialogue, so this bug can just be about the menus. I'll open a new bug tomorrow about the Information dialogue and EmpathyIndividualWidget.
Guillaume, I've continued on Philip's work, so this handles all the context menu items, including the Personas menu you suggested: http://git.collabora.co.uk/?p=user/treitter/empathy.git;a=shortlog;h=refs/heads/linking-ui-rebase8 Please review.
Created attachment 167781 [details] [review] attaching diff for easier review
Review of attachment 167781 [details] [review]: Maybe we shouldn't display the "Personas" menu when there is only one persona? I'm sure we can find a better than "Personas" (translators won't be able to translate it anyway). "Identities" maybe? Or maybe should we just remove this level and directly listen the different personas in the main menu? If we keep this menu, it shouldn't have the "chat" icon. The 'avatar-default' could be a better choice. I think it would be good to display the protocol icons as well. Something like: [presence icon] alice@badger.com ([account icon] account name) ::: libempathy-gtk/empathy-individual-dialogs.c @@ +39,3 @@ #include "empathy-ui-utils.h" +static GList *information_dialogs = NULL; Please describe the list: the type of objects stored, borrowed or owned. @@ +70,3 @@ + + l = g_list_find_custom (information_dialogs, individual, + (GCompareFunc) individual_dialogs_find); add a \n @@ +71,3 @@ + l = g_list_find_custom (information_dialogs, individual, + (GCompareFunc) individual_dialogs_find); + if (l) if (l != NULL) @@ +78,3 @@ + + /* Create dialog */ + dialog = gtk_dialog_new (); We should have a EmpathyInvidualInformationDialog object inheriting from GtkDialog and living in its own file. @@ +120,3 @@ + } + + g_object_set_data (G_OBJECT (dialog), "individual", individual); Shouldn't be needed if we had a proper object. ::: libempathy-gtk/empathy-individual-menu.c @@ +260,3 @@ + EmpathyContact *contact, + GCallback activate_callback, + gboolean (sensitivity_predicate) (EmpathyContact *)) Define a typedef for this function pointer, it's easier to read/understand. @@ +266,1 @@ + if (sensitivity_predicate) != NULL @@ +278,2 @@ +static GtkWidget * +menu_item_set_first_contact (GtkWidget *item, A small comment explaining what this function does would be good. @@ +280,3 @@ + FolksIndividual *individual, + GCallback activate_callback, + gboolean (sensitivity_predicate) (EmpathyContact *)) typedef @@ +647,3 @@ + gtk_widget_show (action); + } + Why not display the "Show previous conversations" as well? @@ +961,1 @@ + name_room_map = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, Please describe this hash table.
(In reply to comment #6) Everything I haven't specifically replied to has been fixed as you suggested in the new branch: http://git.collabora.co.uk/?p=user/treitter/empathy.git;a=shortlog;h=refs/heads/linking-ui-rebase9 I've already squashed the trivial fixes (sorry), so I'll leave linking-ui-rebase8 around in case you want to compare the commits in the two branches. > I'm sure we can find a better than "Personas" (translators won't be able to > translate it anyway). "Identities" maybe? > Or maybe should we just remove this level and directly listen the different > personas in the main menu? Good points. I've changed the new branch to bypass the "Personas" menu, as you suggested. > If we keep this menu, it shouldn't have the "chat" icon. The 'avatar-default' > could be a better choice. Also fixed, though a moot point now. > I think it would be good to display the protocol icons as well. Something like: > [presence icon] alice@badger.com ([account icon] account name) I've added the presence icon. Is there any way to inline an account icon in the menu item text like that? And do you mean the protocol icon? > @@ +78,3 @@ > + > + /* Create dialog */ > + dialog = gtk_dialog_new (); > > We should have a EmpathyInvidualInformationDialog object inheriting from > GtkDialog and living in its own file. Done. I'll squash it on top of the original commit before merging. > @@ +961,1 @@ > + name_room_map = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, > > Please describe this hash table.
Created attachment 167940 [details] [review] new diff
Review of attachment 167940 [details] [review]: Humm removing the sub-menu made the contact menu pretty large. See http://people.collabora.co.uk/~cassidy/contact-menu.jpg I'm not sure what's best, having the extra menu or not. Thoughts? > I've added the presence icon. Is there any way to inline an account icon in the > menu item text like that? And do you mean the protocol icon? Did you forgot to push this patch? I don't see it. I meant to use the icon returned by tp_account_get_icon_name(). ::: libempathy-gtk/empathy-individual-menu.c @@ +103,3 @@ + contact = empathy_contact_dup_from_tp_contact (tp_contact); + + label = g_strdup ( This label is unused and leaked.
(In reply to comment #9) > Review of attachment 167940 [details] [review]: > > Humm removing the sub-menu made the contact menu pretty large. See > http://people.collabora.co.uk/~cassidy/contact-menu.jpg > I'm not sure what's best, having the extra menu or not. Thoughts? > > > I've added the presence icon. Is there any way to inline an account icon in the > > menu item text like that? And do you mean the protocol icon? > > Did you forgot to push this patch? I don't see it. > I meant to use the icon returned by tp_account_get_icon_name(). > > ::: libempathy-gtk/empathy-individual-menu.c > @@ +103,3 @@ > + contact = empathy_contact_dup_from_tp_contact (tp_contact); > + > + label = g_strdup ( > > This label is unused and leaked. I fixed the label leak and removed "from " from the persona menu entries (ie, they're now like "foo@example.org (bar@example.org) >". I also squashed the commits I said I would above. I'll add the account icon for the "from" account in a later commit. This is all committed to mainline now.