GNOME Bugzilla – Bug 562468
Contact list view options are scattered over "Chat" menu and "General" Preferences
Last modified: 2009-07-13 17:01:57 UTC
Empathy 2.24.1, Ubuntu 8.10 Empathy has four options controlling the appearance of contacts in the contact list: 1. "Chat" > "Show Offline Contacts" 2. "Edit" > "Preferences" > "General" > "Appearance" > "Show compact contact list" 3. "Edit" > "Preferences" > "General" > "Appearance" > "Show avatars" 4. "Edit" > "Preferences" > "General" > "Contact List" > "Sort by name"/"Sort by state". Including offline contacts in the contact list is not obviously a "Chat" function. "General" is not a helpful categorization. And "Sort by name"/"Sort by state" seems like it would be the sort of option you'd want to use quickly and temporarily, to find a contact in your contact list, but it is relegated to the Preferences window. I suggest that this be fixed by moving the options into a "View" menu in the Contact List window. Something like: View ---- Show Offline Contacts ------------------------- * by Name by Status ------------------------- * Normal Size Compact
Oops, I missed the "Show avatars" option. Since that's mutually exclusive with the "Show compact contact list" option, it could be shown as: ---------------------------- * Normal Size Normal Size Without Icons Compact An even simpler and more flexible way of presenting it would be: ---------------------------- / Contact Icons / Statuses
I like the idea.
We could maybe put the upcoming "Show Contacts on a map" menu item in View too?
So, if we use the more flexible way that Matthew showed : / Contacts Icons / Statuses (I assume those are check boxes) Then, to get the "Normal Size" option that 2 boxes have to be checked, for "Compact" the 2 unchecked, and for "Normal Without Icons" only 'Statuses' checked. Then, to make sure it's mutually exclusive, when you check 'Contact Icons' then 'Statuses' is automatically checked because we can't have the first without the last. Same thing in reverse : if you uncheck 'Statuses' first then 'Contacts Icons' has to be unchecked automatically. Is that right ?
IMHO, checking other menus items for the user is not a good idea. When you are in the preferences dialog, you can see the other items being checked or grayed out. But when you click on menu item, the menu disappear and you don't see the other items anymore. I think we can go with for now: (until we implement Compact with icons) View ---- Show Offline Contacts ------------------------- * by Name by Status ------------------------- * Normal Size Normal Size Without Icons Compact What do others think?
I think that make sense.
Ok, now if we remove those options from the preferences, do we remove them from the schema also? It seems to me like sorting could be by Name by default all the time without trouble. But, for the contact list size, I'm not so sure if we should put a default value or keep the schema...
Hum, I don't see why we should stop having these as permanent settings. If I set sort by status (my personal favorite), I'd want my contacts to be sorted by status the next time I start Empathy. Even though Matthew says: And "Sort by name"/"Sort by state" seems like it would be the sort of option you'd want to use quickly and temporarily, to find a contact in your contact list, but it is relegated to the Preferences window. I never resort my contacts to find one, but maybe I'll do more once it is more accessible. Yet, I personally want that change to be permanent.
So what you say is remove those options from the "Preferences" window, but keep the permanent config and connect them to the new options in the "View" menu. That way, the window will reopen with this new configuration. Any other thoughts ?
Created attachment 136885 [details] [review] Proposed fix on branch fix-562468 http://gitorious.org/~gmillaire/empathy/gmillaires-clone/commits/fix-562468 src/empathy-main-window.c | 154 +++++++++++++++++++++++++++++--------------- src/empathy-main-window.ui | 57 ++++++++++++++++ src/empathy-preferences.c | 65 ++----------------- src/empathy-preferences.ui | 94 --------------------------- 4 files changed, 165 insertions(+), 205 deletions(-)
Hum, I think you shouldn't remove main_window_notify_compact_contact_list_cb and main_window_notify_show_avatars_cb. We still want the contact list to change if the gconf setting is changed. That means that the radio menus need to be updated to match the changes too.
Created attachment 137025 [details] [review] New patch Added removed notifications to confs Show avatar, Compact List and Sort criterium. Also made sure to check Sort criterium string before doing any action. http://gitorious.org/~gmillaire/empathy/gmillaires-clone/commits/fix-562468
Created attachment 137027 [details] [review] Now with the good patch, sorry! Adds notification for confs in fix-562468 http://gitorious.org/~gmillaire/empathy/gmillaires-clone/commits/fix-562468 fix-536383.patch | 466 ++++++++++++++++++++++++++++++++++++++++++++ fix-562468.patch | 298 ++++++++++++++++++++++++++++ fix-564259.patch | 13 ++ fix-579484.patch | 111 +++++++++++ fix-581871.patch | 458 +++++++++++++++++++++++++++++++++++++++++++ src/empathy-main-window.c | 180 ++++++++++++----- src/empathy-main-window.ui | 57 ++++++ src/empathy-preferences.c | 65 +------ src/empathy-preferences.ui | 94 --------- 9 files changed, 1539 insertions(+), 203 deletions(-)
Created attachment 137028 [details] [review] After some trouble with git, here is the patch that makes sense ! fix-562468 http://gitorious.org/~gmillaire/empathy/gmillaires-clone/commits/fix-562468 src/empathy-main-window.c | 180 +++++++++++++++++++++++++++++++------------ src/empathy-main-window.ui | 57 ++++++++++++++ src/empathy-preferences.c | 65 ++-------------- src/empathy-preferences.ui | 94 ----------------------- 4 files changed, 193 insertions(+), 203 deletions(-)
Note to the reviewer: I am getting ** (/home/pierlux/Collabora/empathy/src/.libs/lt-empathy:20893): CRITICAL **: empathy_contact_get_presence: assertion `EMPATHY_IS_CONTACT (contact)' failed when running this branch, but I am also getting it without the branch. So it isn't related.
I'm not convinced by "Normal Size _Without Icons". It should be avatars and not Icons. I'm sure we could find a better phrasing. main_window_view_sort_contacts_cb Don't mix variable declarations and code. const gchar *valueStr = NULL; We use "value_str" style for variables. Remove #if 0 from empathy-preferences
(In reply to comment #16) > Remove #if 0 from empathy-preferences Heh I told him to do that. There is already a lot of code under #if 0 in that file. It is being kept for future use, just in case we add a preference using that widget. In this patch, I think it removes the only radio buttons in the preferences, so instead of deleting the (reusable) code, I suggested him to comment it.
Thanks for the comments Guillaume. As Pierre-Luc said, I asked him if I should remove what is #if 0. Since it would be necessary to have it if we added a radio button in the preferences window, we opted to leave it there. I don't know if there was a reason Matthew used "icons" instead of "avatars", but I agree "avatars" sounds better. So, we could stay with the same phrasing and replace "icons" by "avatars". Or, we could do something like : * Show Avatars * Normal Size * Compact Size Where "Normal size" is without avatars. It's shorter and quite clear too since it's mutually exclusive. What do you say ?
(In reply to comment #18) > Or, we could do something like : > * Show Avatars > * Normal Size > * Compact Size I'd rather go with more consistent options: * Normal size with avatars * Normal size * Compact size
Created attachment 137600 [details] [review] Proposed fix after review by Guillaume. In branch fix-562468 http://gitorious.org/~gmillaire/empathy/gmillaires-clone/commits/fix-562468 src/empathy-main-window.c | 178 +++++++++++++++++++++++++++++++------------ src/empathy-main-window.ui | 57 ++++++++++++++ src/empathy-preferences.c | 65 ++--------------- src/empathy-preferences.ui | 94 ----------------------- 4 files changed, 191 insertions(+), 203 deletions(-)
After doing some research, by fixing this bug it should close this bug also : #562461
*** Bug 562461 has been marked as a duplicate of this bug. ***
Seems to be a very good start. But I have some comments still: - main_window_view_sort_contacts_cb and main_window_notify_sort_contact_cb should be grouped together, it is confusing to have list_size functions between them. Same for main_window_view_contacts_list_size_cb and main_window_notify_contact_list_size_cb - In main_window_notify_sort_contact_cb you are using your #defined values, but not for main_window_view_sort_contacts_cb. I think both should just use the EmpathyContactListStoreSort enum, no? - When the gconf value change, you just change the value for GtkAction but don't update the contact list itself. I guess this is because chaning the value on the action will emit changed signal anyway, which will update the contact list. Probably you should make that clear by adding some comments in the code. - main_window_notify_sort_contact_cb: Probably you should take the value as a EmpathyContactListStoreSort. Then make switch() on it, instead of the if/else we have now. That makes easier to add more values later (like sort by log size). That's all I think. Thanks!
(In reply to comment #23) > - main_window_notify_sort_contact_cb: Probably you should take the value as a > EmpathyContactListStoreSort. Then make switch() on it, instead of the if/else > we have now. That makes easier to add more values later (like sort by log > size). Seeing as each case in the switch would call the same function with only the enum being different, wouldn't it be better to just call the function with the enum_value->value in parameter ? And if we add more values later it won't be different, unlesswe want to call another function...
Right, we can just make sure that the value from the .ui match the value from the enum. Then you can just pass enum_value->value directly with no if-else/switch. And like that you can remove the #define
Created attachment 138200 [details] [review] Regroup similar functions, uses enum instead of define, remove unnecessary if/else. Fix in branc fix-562468 http://gitorious.org/~gmillaire/empathy/gmillaires-clone/commits/fix-562468 src/empathy-main-window.c | 222 +++++++++++++++++++++++++++++--------------- src/empathy-main-window.ui | 57 +++++++++++ src/empathy-preferences.c | 64 +------------ src/empathy-preferences.ui | 94 ------------------- 4 files changed, 209 insertions(+), 228 deletions(-)
Very little details: - main_window_view_sort_contacts_cb: You don't really need value_str var, you can directly give enum_value->value_nick to empathy_conf_set_string, no? - main_window_notify_contact_list_size_cb: You should add the same comment than in main_window_notify_sort_contact_cb Otherwise, it seems all fine :D
Created attachment 138208 [details] [review] Minor fixes to the code. Fix in branch http://gitorious.org/~gmillaire/empathy/gmillaires-clone/commits/fix-562468 src/empathy-main-window.c | 222 +++++++++++++++++++++++++++++--------------- src/empathy-main-window.ui | 57 +++++++++++ src/empathy-preferences.c | 64 +------------ src/empathy-preferences.ui | 94 ------------------- 4 files changed, 209 insertions(+), 228 deletions(-)
You seemed to have fixed all Xavier's comments so I merged the branch. Thanks a lot for your great work. 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.