GNOME Bugzilla – Bug 569449
new presence chooser widget
Last modified: 2011-06-20 08:21:50 UTC
I've done some work on a new presence selector for Empathy. It looks like this: http://people.collabora.co.uk/~davyd/images/empathy-presence-real.png A screencast of an earlier version: http://people.collabora.co.uk/~davyd/images/empathy-presence-davyd.ogv Branch: http://git.collabora.co.uk/?p=user/davyd/empathy-davyd.git;a=shortlog;h=refs/heads/presence-chooser
I like how this selector avoids the need for a separate "Custom message" dialog with its own text field. The most awkward thing about it, I think, is that about 90% of the control's target area is for setting a custom message, while only about 10% of the target area is for choosing one of the standard or previously-entered messages. I think actual use is much more like the opposite: 90% of the time using a pre-existing status, 10% setting a custom status. This would make people slower in changing their status. Another awkwardness is that the menu ends up with not one, not two, but three "Custom message…" items. (Why does the second one exist?) Have you considered the possibility of having a normal option menu as before, with an "Other…" item that replaces the menu with a text field and a Cancel button? That would similarly remove the need to go into a "Custom message" dialog, while retaining the large target area for choosing one of the pre-existing statuses.
This design was heavily influenced by the GMail UI (which - FWIW - only has Available/Busy I assume it assumes it can take care of Away). The point of having 3 entries is one for each state, so you can enter a custom message in that state. I wondered about making the state offer a separate dropdown to the message, and decided that was just nutty. You could select 'Busy' and then type in a custom message, but it would cause your friends to see two transitions 'Busy' -> 'Reading bugzilla'. If you use the [Busy] Custom Message... option it only sends one message 'Reading bugzilla'. FWIW. the Edit Custom Messages dialog is back (just not in that screencast), but I plan on reworking it to simply be a way of managing your preset messages.
I agree with mtp here. I don't like having all those "Custom message" items. Some questions: 1) How do you add/remove presets? From the screencast I see no item to get the current custom message dialog. If you add it back we'll have yet another "custom message" item that has a different action... 2) How does the status icon's menu works with that new code? Like before? So the UI is not consistant anymore? 3) Do you set the message on timeout and focus out? I think users could just type the message then forget to hit ENTER or click the icon (very unusuall to click icons in entries to apply). My suggestion: - Remove all "Custom message" items. - When selecting "Busy" presence, the text in the entry is set to "Busy", it is preselected and focus is on that entry. So just typing one letter will remove the preset test and change it to a custom message. - If the user do not type anything for ~10sec the presence is set without message. Same on focus out and ENTER. - If the user type a letter, the timeout is reset to ~10sec and after that timeout (or focus out, or ENTER), the custom message is set. - The right icon is a spin. If you click it the message is remembered as preset and appear in the dropdown menu. We need a way to remove presets. Obviously the current dialog is ugly. We could make automatic cleanup, like keeping only 10last custom messages, or something like that. Problem I see with that is if I set a message with a spelling error, I change to a new custom message and the error is kept. Gossip as a "remove all custom messages" button that removes everything and you have to type again messages you wanted to keep. I think that's wrong. Possible solution is: - Select the preset message you wan to remove. - Click the spin button to uncheck it and remove completely the preset, or change the message in the entry to change the preset. --> Problem that way you are likely to set the presence for the time you edit/remove it. Other solution: A manage preset dialog that display the full list of remembered messages and with "cancel/add/remove" buttons. That dialog could be a bit more complex because it won't be used a lot. Ideas?
(In reply to comment #3) > 1) How do you add/remove presets? From the screencast I see no item to get the > current custom message dialog. If you add it back we'll have yet another > "custom message" item that has a different action... The screencast is out of date: the dialog has been added back: http://people.collabora.co.uk/~davyd/images/empathy-presence-real-2.png I would like to rework the dialog itself to be more straight forward. Simply make it deal with presets, not with changing the message. Nice big Add/Remove buttons. > 2) How does the status icon's menu works with that new code? Like before? So > the UI is not consistant anymore? At the moment, the status menu is unchanged. I propose it would work similarly to how it used to work. > 3) Do you set the message on timeout and focus out? I think users could just > type the message then forget to hit ENTER or click the icon (very unusuall to > click icons in entries to apply). Never on timeout. Focus out is an open question (there is a FIXME in the code). I'm tempted to say "yes". The icon prelights when you hover over it, and has a tooltip, so the user should discover its purpose. Or just press Enter, because it is the Enter icon. > - If the user do not type anything for ~10sec the presence is set without > message. Same on focus out and ENTER. I don't think timeouts are a good way forward. Even if you animate a timer showing the user there is a timeout, I don't think it's good UI. > We need a way to remove presets. Obviously the current dialog is ugly. We could > make automatic cleanup, like keeping only 10last custom messages, or something > like that. Problem I see with that is if I set a message with a spelling error, > I change to a new custom message and the error is kept. Gossip as a "remove all > custom messages" button that removes everything and you have to type again > messages you wanted to keep. I think that's wrong. I'd like to rework the dialog to a simple list of presets and Add/Remove buttons. I'll do this in the next couple of days.
*** Bug 567584 has been marked as a duplicate of this bug. ***
(In reply to comment #4) > I don't think timeouts are a good way forward. Even if you animate a timer > showing the user there is a timeout, I don't think it's good UI. I think the icon can be greyed until the presence is effectively set. I can't be worst than having 4 "Custom message" times in the menu for an action users usually don't need...
Created attachment 128562 [details] [review] Implementd in http://git.collabora.co.uk/?p=user/davyd/empathy-davyd.git;a=shortlog;h=refs/heads/presence-chooser configure.ac | 2 +- data/Makefile.am | 2 +- libempathy-gtk/empathy-contact-menu.c | 12 +- libempathy-gtk/empathy-presence-chooser.c | 858 ++++++++++--------- libempathy-gtk/empathy-presence-chooser.h | 6 +- libempathy-gtk/empathy-smiley-manager.c | 2 +- libempathy/empathy-account-manager.c | 4 +- libempathy/empathy-call-factory.c | 2 +- libempathy/empathy-chatroom-manager.c | 2 +- libempathy/empathy-contact-factory.c | 2 +- libempathy/empathy-contact-manager.c | 2 +- libempathy/empathy-dispatcher.c | 2 +- libempathy/empathy-idle.c | 17 +- libempathy/empathy-log-manager.c | 2 +- libempathy/empathy-tp-call.c | 78 ++ libempathy/empathy-tp-call.h | 9 + libempathy/empathy-types.h | 2 +- po/nb.po | 635 +++++++------- po/pt_BR.po | 352 ++++---- po/sv.po | 324 ++++---- po/vi.po | 1339 ++++++++++++----------------- python/pyempathy/pyempathy.defs | 54 ++ src/empathy-call-window.c | 5 +- src/empathy-call-window.glade | 14 +- src/empathy-event-manager.c | 2 +- src/empathy-ft-manager.c | 2 +- src/empathy-status-icon.c | 11 +- src/empathy.c | 1 + src/ephy-spinner.c | 2 +- tests/Makefile.am | 4 +- tests/test-empathy-presence-chooser.c | 49 ++ 31 files changed, 1867 insertions(+), 1931 deletions(-)
Created attachment 128572 [details] [review] correct patch (hopefully) configure.ac | 2 +- libempathy-gtk/empathy-presence-chooser.c | 858 +++++++++++++++-------------- libempathy-gtk/empathy-presence-chooser.h | 6 +- src/empathy-status-icon.c | 2 +- tests/Makefile.am | 4 +- tests/test-empathy-presence-chooser.c | 49 ++ 6 files changed, 511 insertions(+), 410 deletions(-)
Attachment 128562 [details] is just broken.
After a long discussion, I have another proposal: If you have currently a custom message "Foo", and a saved preset "Bar" for presence busy, here is your menu: Available Foo Busy Foo Bar Away Foo Hidden Offline --- Manage presets... So the current message is placed instead of the "custom message..." of davyd's branch. It's useful to keep the current message and just change the presence. In all cases, clicking will return to main window, the text clicked ("Available" or "Foo", or "Busy") is selected and focus is on it. The presence is not actually set directly, if waits for either: ENTER typed, focus-out, or 10s timeout. Once one of them occure, the text is not selected anymore. If later the user change the text, it is also applied after either ENTER, focus-out or timeout. I aggree focus-out and timeout should not be the primary way for UI, but it is necessary otherwise the user will forget to apply for sure. Also to quickly add/remove presets, I suggest to use the firefox's star icon on the right of the entry. Clicking it will set/remove the current message from presets and of course the star becomes filled/empty just like firefox's bookmarks. The manage presets dialog will be a more "complex" dialog to add/remove/edit presets.
Created attachment 130660 [details] [review] [PATCH] Fix focus-out autocommit using an idle handler configure.ac | 2 +- libempathy-gtk/empathy-presence-chooser.c | 994 +++++++++++++++++------------ libempathy-gtk/empathy-presence-chooser.h | 6 +- src/empathy-status-icon.c | 2 +- tests/Makefile.am | 4 +- tests/test-empathy-presence-chooser.c | 49 ++ 6 files changed, 660 insertions(+), 397 deletions(-)
Updated patch. Based on feedback from a number of people, also fixes a couple of bugs. Changes include: - focus handling improvements - commiting the message if the user focuses out (this includes changing window) - allowing the user to easily change from Available to Away, etc. keeping their custom status message - Fixing bugs introduced by latest GTK+ (2.15.5) The 3 menu items still say 'Custom message', otherwise they don't really afford their usage. Having 3 of them isn't really an issue IMO, at most it takes up 50% of the target space (certainly not 90%). What I'd like to trial, once this branch is merged is firefox style bookmarking for presence settings, as well as reworking the presets management dialog.
I just tried it out and bug 576434 (ellipsize long status messages in the status combobox) is also present here.
If you start typing into the status picker, and then an account finishes connecting, your message-in-progress is clobbered.
Here are my comments for the latest code in presence-chooser branch: * Coding style: Use that for brackets: if (foo ) { } * Coding style: Do not call functions when declaring a variable. It is fine to set an initial value if it is NULL, 0 or GET_PRIV, but not complex function call. * Coding style: Always declare vars on top of a block, never in the middle of your code. Always add an empty line after the var declaration and the first instruction. * Coding style: After a if statment always add brackets, even for one instruction. if (foo) { return; } * For static function in EmpathyFoo you should use foo_ prefix. * libempathy-gtk/empathy-presence-chooser.c: +// FIXME - what's the correct debug flag? --> I think OTHER is better. * + int previous_type; --> you should make a typedef for the enum and is it. * create_model(): - gtk_list_store_append + gtk_list_store_set == gtk_list_store_insert_with_values - missing _() for "<i>Custom Message...</i>" and "Edit Custom Messages..." - I don't like having functions without args like that. I think it should take self in args and set the model on it instead of returning it. * presence_chooser_popup_shown_cb: You create the model each time. Why don't you create the model in the _init function of presence chooser? * presence_chooser_entry_key_press_event_cb: When esc is pressed, you call presence_chooser_set_status_editing but it's already done in presence_chooser_reset_status * to get toplevel window, you can do gtk_widget_get_ancestor widget, GTK_TYPE_WINDOW); if the return is !=NULL you are sure to have the toplevel window. * presence_chooser_changed_cb: You call presence_chooser_dialog_show (GTK_WINDOW (window)); but window could be NULL => warning. * empathy_presence_chooser_init: - Why do you use g_signal_connect_object? entry and chooser will die at the same time anyway... Also the gtkdoc of that func always scared me, I don't really understand if that function should be used... - gtk_cell_layout_clear (GTK_CELL_LAYOUT (chooser)); --> seems useless, how could the layout not be cleared already? We are in the chooser's init func! - "tooltip-text", _("Set your presence and current status"), --> "Change your presence and message" ? * presence_chooser_flash_stop: You can remove commented code. Those flash stuff is obsolete code from Gossip. We should probably remove it. * tests/test-empathy-presence-chooser.c: window and chooser should be declared on top of the block.
Ok updated. > * Coding style: Use that for brackets: > if (foo ) { > } Fixed. > * Coding style: Do not call functions when declaring a variable. It is fine > to set an initial value if it is NULL, 0 or GET_PRIV, but not complex > function > call. Fixed. > * Coding style: Always declare vars on top of a block, never in the middle of > your code. Fixed (but C89 is 20 years old, when are we going to be free of it?). > * Coding style: After a if statment always add brackets, even for one > instruction. > if (foo) { > return; > } Fixed. > * For static function in EmpathyFoo you should use foo_ prefix. Fixed. > * libempathy-gtk/empathy-presence-chooser.c: > +// FIXME - what's the correct debug flag? > --> I think OTHER is better. Fixed. > * + int previous_type; > --> you should make a typedef for the enum and is it. Fixed. > * create_model(): > - gtk_list_store_append + gtk_list_store_set == > gtk_list_store_insert_with_values Fixed. > - missing _() for "<i>Custom Message...</i>" and "Edit Custom Messages..." Fixed. > - I don't like having functions without args like that. I think it should > take self in args and set the model on it instead of returning it. Why? > * presence_chooser_popup_shown_cb: > You create the model each time. Why don't you create the model in the > _init function of presence chooser? Well, you need to recreate the entire contents of the model every time. TBH, letting the allocator do your work for you is probably faster than clearing the model. > * presence_chooser_entry_key_press_event_cb: > When esc is pressed, you call presence_chooser_set_status_editing but it's > already done in presence_chooser_reset_status Fixed. > * to get toplevel window, you can do > gtk_widget_get_ancestor widget, GTK_TYPE_WINDOW); > if the return is !=NULL you are sure to have the toplevel window. I used the recommended check from the docs. > * presence_chooser_changed_cb: > You call presence_chooser_dialog_show (GTK_WINDOW (window)); but window > could > be NULL => warning. presence_chooser_dialog_show() checks the parent argument, so I don't think this can happen. > * empathy_presence_chooser_init: > - Why do you use g_signal_connect_object? > entry and chooser will die at the same time anyway... Also the gtkdoc of > that > func always scared me, I don't really understand if that function should > be > used... You're right, they should do at the same time, unless someone does something very strange. I used it because it's technically the right function to use. The leaking 4-bytes thing is when the weakly-referenced object is destroyed, it's cleaned up when the first argument is destroyed, so is rarely an issue IME. > - gtk_cell_layout_clear (GTK_CELL_LAYOUT (chooser)); --> seems useless, how > could the layout not be cleared already? We are in the chooser's init > func! Habit because of an old GTK+ bug, sometimes your layout had stuff in it, even when you expected it not to (this can still be true of some CellLayout widgets). > - "tooltip-text", _("Set your presence and current status"), > --> "Change your presence and message" ? Not really an improvement. > * presence_chooser_flash_stop: You can remove commented code. Those flash > stuff is obsolete code from Gossip. We should probably remove it. Yeah, I figured this was gossip code. I don't know if we want to drop it or not. TBH, I always liked that flash away/available thing. > * tests/test-empathy-presence-chooser.c: > window and chooser should be declared on top of the block. Fixed.
Will. Your bug should be fixed too.
Created attachment 132055 [details] [review] updated patch libempathy-gtk/empathy-presence-chooser.c | 984 +++++++++++++++++------------ libempathy-gtk/empathy-presence-chooser.h | 6 +- src/empathy-status-icon.c | 2 +- tests/Makefile.am | 4 +- tests/test-empathy-presence-chooser.c | 52 ++ 5 files changed, 651 insertions(+), 397 deletions(-)
(In reply to comment #16) > Fixed (but C89 is 20 years old, when are we going to be free of it?). Hear hear. > > * presence_chooser_changed_cb: > > You call presence_chooser_dialog_show (GTK_WINDOW (window)); but window > > could > > be NULL => warning. > > presence_chooser_dialog_show() checks the parent argument, so I don't think > this can happen. What happens if you pass NULL to GTK_WINDOW () ?
It's fine. ~~ #include <gtk/gtk.h> int main (int argc, char **argv) { g_type_init (); g_print ("%p\n", GTK_WINDOW (NULL)); } ~~ [davyd@adelie src]$ gcc -o test `pkg-config --cflags --libs gtk+-2.0` test.c [davyd@adelie src]$ ./test (nil)
Right, I though that generate a warning, but it seems fine. Thanks for your fixes, I'll check your code asap.
(In reply to comment #16) > > > * Coding style: Always declare vars on top of a block, never in the middle of > > your code. > > Fixed (but C89 is 20 years old, when are we going to be free of it?). The problem is not to build with C89, it's just a coding style convention.
The new code is not committed. I keep this bug open because all UI issues are not resolved... Or mtp and me are the only one to find stupid all those "custom message" entries?
s/mtp/mpt/ sorry :)
s/The new code is not committed./The new code is now committed./ Monday morning, need more cofee :/
To make things cleared here are points that still need work: 1) Having all those "Custom message" entries don't sounds good to me. Especially since it's not really about setting a custom message but entring the editing mode (You can still set a custom message even without those entries). 2) We agreed I think to add the firefox-like bookmarking for custom messages. This is not yet implemented. 3) The Custom message dialog have to be completely reworked. I think a treeview with all saved custom messages, with add/remove/edit buttons is better. Maybe we could open 3 new bugs for those issues, but IMO it's better to keep all the history in the bug.
Created attachment 133008 [details] [review] [PATCH] Icons from Firefox data/icons/Makefile.am | 8 ++ data/icons/hicolor_apps_16x16_empathy-starred.png | Bin 0 -> 1468 bytes .../icons/hicolor_apps_16x16_empathy-unstarred.png | Bin 0 -> 1424 bytes data/icons/hicolor_apps_22x22_empathy-starred.png | Bin 0 -> 2310 bytes .../icons/hicolor_apps_22x22_empathy-unstarred.png | Bin 0 -> 2390 bytes data/icons/hicolor_apps_24x24_empathy-starred.png | Bin 0 -> 2634 bytes .../icons/hicolor_apps_24x24_empathy-unstarred.png | Bin 0 -> 2693 bytes data/icons/hicolor_apps_32x32_empathy-starred.png | Bin 0 -> 3989 bytes .../icons/hicolor_apps_32x32_empathy-unstarred.png | Bin 0 -> 4086 bytes libempathy-gtk/empathy-presence-chooser.c | 131 ++++++++++++++++++-- src/empathy-main-window.c | 6 +- 11 files changed, 134 insertions(+), 11 deletions(-)
Demo screencast: http://people.collabora.co.uk/~davyd/images/empathy-presence-chooser-firefox-style-favs.ogv Git branch: http://git.collabora.co.uk/?p=user/davyd/empathy-davyd.git;a=shortlog;h=refs/heads/message-favs
The icons here came from Firefox (perhaps we want to get some new ones drawn up?). WRT licensing, it should be fine, I think. """ Overall, the firefox project is licensed under the terms of the Mozilla Public License version 1.1 or, at your option, under the terms of the GNU eneral Public License version 2 or subsequent, or the terms of the GNU Lesser General Public License version 2.1 or subsequent. """
Created attachment 133089 [details] reworked messages dialog This is my rework of the custom messages dialog. http://git.collabora.co.uk/?p=user/davyd/empathy-davyd.git;a=shortlog;h=refs/heads/presets-dialog
Created attachment 133091 [details] [review] presets dialog patch libempathy-gtk/Makefile.am | 4 +- libempathy-gtk/empathy-presence-chooser.c | 237 +-------------- libempathy-gtk/empathy-presence-chooser.ui | 140 --------- libempathy-gtk/empathy-status-preset-dialog.c | 385 ++++++++++++++++++++++++ libempathy-gtk/empathy-status-preset-dialog.h | 63 ++++ libempathy-gtk/empathy-status-preset-dialog.ui | 116 +++++++ libempathy-gtk/empathy-ui-utils.c | 2 +- po/POTFILES.in | 3 +- tests/.gitignore | 1 + tests/Makefile.am | 4 +- tests/test-empathy-presence-chooser.c | 4 + tests/test-empathy-status-preset-dialog.c | 49 +++ 12 files changed, 636 insertions(+), 372 deletions(-)
- new files (empathy-status-preset-dialog.[ch] and tests/test-empathy-status-preset-dialog.c) should follow tp-coding style: http://telepathy.freedesktop.org/wiki/Style - empathy_status_preset_dialog_new: usually _new function is a simple wrapper calling g_object_new (). All the logic being done in the constructor. But I don't have any experience with widgets subclassing so maybe it's not relevant here. - empathy_status_preset_dialog_init: havint this code in the _init seems weird to me (the above remark about my inexperience in this area still apply) - DEBUG ("ADD PRESET (%i, %s)\n", state, status); WHY ARE YOU SHOUTING? :) - some comments explaining block_add_combo_changed would be good
Given that I've just spent most of my day tweaking this dialog, I'm pretty sure it was merged.