GNOME Bugzilla – Bug 611894
Add option to make chatroom "always urgent"
Last modified: 2011-08-29 10:12:37 UTC
Created attachment 155298 [details] [review] Add option to make chatroom "always urgent" Often a user wants to be immediately notified of posts to a chatroom even when the post does not address them directly by name. This patch adds a room option to make all posts to the room "urgent", meaning that the system-specific urgency action should be taken -- notification, window urgency hint, etc. Two specific use cases for when one may want this: i) Low-traffic rooms (so that one does not have to go check the room all the time) ii) Error-logging room (room to which errors from some other system(s) are logged) The "always urgent" option is off by default when joining a room.
Review of attachment 155298 [details] [review]: Hi Jonas, Thanks a lot for your contribution. You introduced some coding style regressions. You should ensure that "make check" still pass with your patch. I inlined some comments: ::: libempathy/empathy-chatroom.c @@ +127,3 @@ + g_param_spec_boolean ("always_urgent", + "Always Urgent", + "TRUE if all posts to the chatroom should be considered urgent", This line is too long. @@ +703,3 @@ + priv = GET_PRIV (chatroom); + + if (priv->always_urgent == always_urgent) { No need for { } ::: src/empathy-chat-window.c @@ +856,3 @@ + + if (chatroom == NULL) { + const gchar *name; This code has been copied from the favorite cb. It would be good to have a ensure_chatroom function doing that and returning a ref to the chatroom. @@ +1363,1 @@ * alias. */ This comment should be updated. ::: src/empathy-chat-window.ui @@ +35,3 @@ + <object class="GtkToggleAction" id="menu_conv_always_urgent"> + <property name="name">menu_conv_always_urgent</property> + <property name="label" translatable="yes">Always _Urgent</property> This option isn't very clear. Any idea of a better phrasing?
I've gone through your comments and made the necessary changes. I split up the patches a bit, too, for easier review. i) I added a function "chatroom_manager_ensure_chatroom" as per your suggestion. ii) I added some code to the chatroom manager so that it saves a rooms always_urgent state iii) Settled on the wording "Notify Always" for the menu... not sure what would be better and still short enough to fit in the menu. Patches (4) attached. PS: Any good way to use tools like git-email and have the patches end up here or somewhere where they're visible to Empathy developers???
Created attachment 155322 [details] [review] Add function ensure_chatroom
Created attachment 155323 [details] [review] Use ensure_chatroom in favorites cb
Created attachment 155324 [details] [review] Add option to make chatroom "always urgent"
Created attachment 155325 [details] [review] Retain always_urgent state
Created attachment 155327 [details] [review] Use ensure_chatroom in favorites cb
Created attachment 155328 [details] [review] Add option to make chatroom "always urgent" Previous patch got a bit garbled when I tried to split in Git... better now.
You can pull these patches from my git repo if it makes it any easier for you: git://git.southpole.se/~jonas/git/empathy.git
Convention for _ensure function is to always return a new reference to the object. Also, you mixed conding styles in this function. You should use tabs as this file still use the old coding style. chatroom_manager_parse_chatroom: use tp_strdiff instead of strcmp (I know old code doesn't use but let's get the new code right. :) - (const xmlChar *) empathy_chatroom_get_name (chatroom)); + (const xmlChar *) empathy_chatroom_get_room (chatroom)); I guess this was a bug, right? Ideally this should have been recoreded in a separated patch but I'm nipticking. Except that it looks pretty good to me. Nice job at splitting work in different patches. :) Matthew, Nick: any suggestion for a better name for this option?
> Convention for _ensure function is to always return a new reference to > the object. Not sure I under this comment... do you mean like this? EmpathyChatroom * empathy_chatroom_manager_ensure_chatroom (EmpathyChatroomManager *manager, TpAccount *account, const gchar *room, const gchar *name) { EmpathyChatroom *chatroom; chatroom = empathy_chatroom_manager_find (manager, account, room); if (chatroom) { return g_object_ref(chatroom); } else { chatroom = empathy_chatroom_new_full (account, room, name, FALSE); empathy_chatroom_manager_add (manager, chatroom); return chatroom; } }
Created attachment 155544 [details] [review] Make chatroom manager retain room's always_urgent state i) use tp_strdiff instead of strcmp ii) pull out unrelated bug fix that snuck into this patch
Created attachment 155545 [details] [review] Bug fix Does this need a new bug report or can it go via this bug? It's a pretty trivial fix...
(In reply to comment #11) > > Convention for _ensure function is to always return a new reference to > > the object. > > Not sure I under this comment... do you mean like this? Yep. And the caller has to release the ref once it's done with it.
(In reply to comment #13) > Created an attachment (id=155545) [details] [review] > Bug fix > > Does this need a new bug report or can it go via this bug? It's a pretty > trivial fix... Can you please push this branch is its own branch so I can already merge this fix?
(In reply to comment #15) > (In reply to comment #13) > > Created an attachment (id=155545) [details] [review] [details] [review] > > Bug fix > > > > Does this need a new bug report or can it go via this bug? It's a pretty > > trivial fix... > > Can you please push this branch is its own branch so I can already merge this > fix? Pushed to branch bug-611894 at git://git.southpole.se/~jonas/git/empathy.git
(In reply to comment #14) > (In reply to comment #11) > > > Convention for _ensure function is to always return a new reference to > > > the object. > > > > Not sure I under this comment... do you mean like this? > > Yep. And the caller has to release the ref once it's done with it. Pushed to branch always_urgent at git://git.southpole.se/~jonas/git/empathy.git
Please enter a more descriptive commit msg. "Bug fix" isn't really useful. :)
Your urgent branch looks good to me. I'll merge it once 2.30 is branched as we are now in UI freeze. I'd still like to find a better phrasing for the menu item though. Thanks a lot for your great work.
(In reply to comment #18) > Please enter a more descriptive commit msg. "Bug fix" isn't really useful. :) Fixed... it's a rather trivial (and obvious) fix, though.
(In reply to comment #19) > Your urgent branch looks good to me. I'll merge it once 2.30 is branched as we > are now in UI freeze. > I'd still like to find a better phrasing for the menu item though. > > Thanks a lot for your great work. Great! Thanks.
(In reply to comment #20) > (In reply to comment #18) > > Please enter a more descriptive commit msg. "Bug fix" isn't really useful. :) > > Fixed... it's a rather trivial (and obvious) fix, though. Merged to master; thanks! I keep this bug open until the always_urgent branch is merged.
In answer to the question of how to label the option: Any time a checkbox label begins with "Always", it's a good sign that a pair of radio buttons should be used instead. If I understand the purpose of this option correctly, I suggest showing it something like: In chat rooms, notify me of: ( ) Every message (*) Messages mentioning me
> If I understand the purpose of this option correctly, I suggest showing it > something like: > > In chat rooms, notify me of: ( ) Every message (*) Messages mentioning me I like this wording, but the current user interface doesn't really allow for radio buttons. The chatroom preferences are limited to a drop-down menu and it will be tough to squeeze in radio buttons as it currently stands. It's already tough getting in a text description that describes this option at all due to the lack of space in the menu. I imagine the chatroom options could use a redesign in any case, and then these radio buttons should be incorporated. By the bye, will this branch be merged to master???
Ok, what about naming this checkbox "Important" and add a tooltip explaning what it did? Just change that and rebase on top of master and I'll merge your branch right away. :) Sorry for the delay.
I renamed the label and merged the branch. Thanks a lot! 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.