GNOME Bugzilla – Bug 574879
remember which account/server I joined a chat on last
Last modified: 2012-01-26 04:33:04 UTC
While joining chat rooms for new mandated communications mechanism, I got bothered by needing to re-select Account and reenter Server in Room->Join New every time. Particularly because I'd forget to select Account (since I wasn't thinking about the fact that I also had a gmail account at the top of the list that I never use, and failure to join a room didn't result in any actual message suggesting that I'd just screwed up).
Right, this could be added in the new Join New dialog.
Created attachment 185493 [details] [review] patch solving the issue I provide a way to save the last active account from the user by using a function to store the value, then when the account chooser is created I connect to ready signal and the account chooser is updated.
Review of attachment 185493 [details] [review]: ::: src/empathy-new-chatroom-dialog.c @@ +523,3 @@ + EmpathyNewChatroomDialog *dialog) +{ + gpointer iter; Avoid passing things through gpointer. Use the type it really is. @@ +525,3 @@ + gpointer iter; + + iter = g_object_get_data(dialog,"last_active"); Missing spaces. @@ +527,3 @@ + iter = g_object_get_data(dialog,"last_active"); + + gtk_combo_box_set_active_iter (combobox,iter); This is wrong, "last_active" is a TpAccount* not a GtkTreeIter*. You want to use empathy_account_chooser_set_account(). @@ +696,3 @@ account = empathy_account_chooser_get_account (account_chooser); + + g_object_set_data(dialog,"last_active",account); Missing spaces. You don't want to store this on the dialog, because the dialog will be destroyed when you click join, and so will this information. You will need to store the information on the dialog's parent (gtk_window_get_transient_for() should return the parent, but you need to check for == NULL). You will also want to give this a more descriptive name like "new-chatroom-last-account". You also need to hold a reference to the account object with g_object_set_data_full, g_object_ref and g_object_unref.
*** Bug 646087 has been marked as a duplicate of this bug. ***
Also it would be cool if you could provide a nice patch generated using git format-patch.
Created attachment 185764 [details] [review] Corrected patch
Review of attachment 185764 [details] [review]: Mostly style. Although one mistake that would stop this compiling... ::: src/empathy-new-chatroom-dialog.c @@ +521,3 @@ static void +new_chatroom_dialog_account_ready_cb (EmpathyAccountChooser *account_chooser, + EmpathyNewChatroomDialog *dialog) Bad indenting. In old Empathy style, parameters should line up. @@ +523,3 @@ + EmpathyNewChatroomDialog *dialog) +{ + TpAccount *account; //pa ova da ne go definiram tuka??? Don't include comments to yourself in a final patch. Code comments should be written in English /* in C style */. @@ +526,3 @@ + GtkWindow *dialog_parent; + + dialog_parent = gtk_window_get_transient_for(GTK_WINDOW (dialog->window)); Missing space. old_empathy_style ("is", "like this"); @@ +528,3 @@ + dialog_parent = gtk_window_get_transient_for(GTK_WINDOW (dialog->window)); + if (dialog_parent != NULL) { + TpAccount *account; //pa ova da ne go definiram tuka??? Missing spaces. @@ +530,3 @@ + account = g_object_get_data(dialog_parent,"new-chatroom-last-account"); + if (account != NULL) { + GtkWindow *dialog_parent; Also missing a space here. @@ +532,3 @@ + empathy_account_chooser_set_account(account_chooser, account); + } + Weird indenting on these braces. Should line up with the start of the block they open. @@ +702,3 @@ account = empathy_account_chooser_get_account (account_chooser); + + dialog_parent = gtk_window_get_transient_for(GTK_WINDOW (dialog->window)); Missing space. @@ +705,3 @@ + + if (dialog_parent != NULL) { + g_object_set_data_full(GOBJECT (dialog_parent), "new-chatroom-last-account", g_object_ref(account),(GDestroyNotify) g_object_unref); Missing space. Wrap lines to 80 characters. Also a missing underscore in G_OBJECT(). Please test patches before you submit them.
Also use "make check" to detect common coding style errors.
Jovanka: any progress on this? I'd like to get this bug closed for 3.2.
Adding the gnome-love keyword as finishing this patch should be pretty easy.
I think that's not that easy, because between calls to Join... the last irc account selected could have gone to a disconnected state.
Created attachment 193356 [details] [review] New chatroom dialog remembers last account selected in the account chooser This is a slightly modified version of the last patch published here. It works for me, but I'm a newbi and I'm not sure the patch has been generated correctly.
What about making this persisent? We could store the account path in a gsettings key so this will be restored even if empathy has been restarted.
Probably it's a better solution. One could have two IRC accounts, one with well known channels defined as favorites; and other with a set of favorites channels as well as some amount of channels that he joins sporadicly.
I don't know if I am correct, could you please review this approach to the solution? I plan to store in a GSettings key located at org.gnome.empathy (don't have a name for it yet) the string representing the display-name property of the selected TpAccount. Then empathy could retrieve this value in new_chatroom_dialog_new(), and set the account_chooser to display the account previously saved, based on its name, which could have changed since last store operation.
Rather than storing the display name, store the account object path, e.g. /org/freedesktop/Telepathy/Account/jabber/gabble/danni1. This is the unique, immutable path to refer to an account and can also be used to re-acquire the TpAccount proxy.
That's what I've written by now inside new_chatroom_dialog_show(), retrieve the dbus object path, create the tpaccount and select it: settings = g_settings_new (EMPATHY_GSETTINGS_PATH); account_object_path_value = g_settings_get_value (settings, EMPATHY_GSETTINGS_LAST_ACCOUNT_KEY); if (account_object_path_value != NULL) { account_object_path = g_variant_get_string (account_object_path_value, NULL); account_chooser_account = tp_account_new (tp_dbus_daemon_dup (NULL), account_object_path, NULL); empathy_account_chooser_set_account (EMPATHY_ACCOUNT_CHOOSER (dialog->account_chooser), account_chooser_account); g_object_unref (account_chooser_account); } I don't know if doing the whole job inside this function is ok. If it's ok I'll start doing the 'store part'. Thank you, and sorry, I know this is an easy job and I'm a new comer.
Sorry, don't review my last message. That approach is wrong, I'm working on a better solution.
Created attachment 196198 [details] [review] Progress on this bug The patch I send contains the result of my work. But the bug is still not solved. I get a segmentation fault sometimes, or almost everytime. I think storing the object path for the account works fine. Retrieving the object path from GSettings also works fine. The segmentation fault occurs when calling set_account on the account chooser. I've not been able to find why yet. In the patch you can see plenty of debug statements, sorry, this is work in progress. To set the account I first wait untill it's ready, by calling prepare_async, but I get the same result. In fact, set_account seems to work fine, but after setting the account, a segmentation fault is raised. I would appreciate your help if you could point me to a better solution, or give some advice. Also, there's a change in set_account (data->set = FALSE) that doesn't belong to this bug, and has been properly reported. Thank you very much.
The best way to debug such crash is to start empathy in gdb, reproduce the crash and get a stack trace ('bt' in gdb).
Created attachment 196652 [details] [review] New patch After some talks on IRC I've came up with this solution. It doesn't work don't you before apply the patch provided in https://bugzilla.gnome.org/show_bug.cgi?id=658909 But I still have a problem, I can't make gsettings read the updated object path from its record until empathy is restarted. But I can see in dconf-editor the new value.
Review of attachment 196652 [details] [review]: Cool thanks for the patch! I didn't test it yet but here is a first review. You should set a more descriptive commit message and add a link to this bug. ::: src/empathy-new-chatroom-dialog.c @@ +34,3 @@ +#include <gio/gio.h> +#define EMPATHY_GSETTINGS_SCHEMA "org.gnome.Empathy" +#define EMPATHY_GSETTINGS_LAST_ACCOUNT_KEY "last-account" You should define this in libempathy/empathy-gsettings.h I think I'd name it "org.gnome.Empathy.conversation.room-last-account" to be clearer. You should also add the key to data/org.gnome.Empathy.gschema.xml.in so it has a structure in d-conf. @@ +140,3 @@ +static void new_chatroom_dialog_set_last_account (GSettings *settings, + EmpathyAccountChooser *account_chooser); +static void new_chatroom_store_last_account (GSettings *settings, Do you really need those prototypes? Can't you define those functions before they are used? @@ +211,3 @@ TRUE, TRUE, 0); + /* Settings manager */ This comment isn't really useful, the code speaks for itself. :) @@ +268,3 @@ } + g_object_unref (dialog->settings); Please use tp_clear_object @@ +786,3 @@ + +static void +{ name is a bit miss leading. select_last_account() maybe? @@ +801,3 @@ + DEBUG ("Setting account path '%s'", account_path); + +{ You should unref it once you're done, _dup() gives you a ref. @@ +808,3 @@ + path = tp_proxy_get_object_path (account); + + GList *accounts, *l; use tp_strdiff @@ +814,3 @@ + account); + } + You leak accounts if you early return. @@ +822,3 @@ + +static void + DEBUG ("Setting account path '%s'", account_path); No need to pass settings, you can get them from priv. @@ +830,3 @@ + + account = empathy_account_chooser_get_account (account_chooser); + accounts = tp_account_manager_get_valid_accounts (manager); split this on 2 lines. @@ +835,3 @@ + DEBUG ("Storing account path '%s'", account_path); + + TpAccount *account = l->data; I don't think you have to dup the string.
Created attachment 196887 [details] [review] Modified patch This is a reworked version of the previous patch, based on the comments pointed out in the review. Sorry if there is something wrong.
Review of attachment 196887 [details] [review]: This looks pretty good! I'll do some testing before merging (I can't merge now as we are almost in freeze but will once we have branched for 3.4). Thanks a lot! ::: data/org.gnome.Empathy.gschema.xml.in @@ +233,3 @@ </key> + <key name="room-last-account" type="o"> + <default>"/org/freedesktop/Telepathy/Account/noaccountselected"</default> Can't we have an empathy default?
Sorry, I don't know what do you mean by empathy default, maybe not to mention Telepathy but Empathy? And as I said in a previous post, I have encountered problems with GSettings: it seems to write ok, but when it reads, the previous value is retrieved, but dconf-editor shows the correct value. I don't know if it's my fault.
Oops sorry, I meant "empty", I guess I'm typing "empathy" too much. :)
Since it is an object path I think it could not be empty. In fact, setting it to "" gives /opt/gnome/share/glib-2.0/schemas/org.gnome.Empathy.gschema.xml: Error on line 236 char 1: 0-2:not a valid object path. This entire file has been ignored. Removing the quotes gives /opt/gnome/share/glib-2.0/schemas/org.gnome.Empathy.gschema.xml: 0:expected value. This entire file has been ignored.
Review of attachment 196887 [details] [review]: ::: src/empathy-new-chatroom-dialog.c @@ +251,3 @@ + g_settings_set_value (gsettings, + EMPATHY_PREFS_CHAT_ROOM_LAST_ACCOUNT, + if (account == NULL) Why not just use g_settings_set() here? @@ +477,3 @@ + account_path_value = g_settings_get_value (gsettings, + EMPATHY_PREFS_CHAT_ROOM_LAST_ACCOUNT); + GList *accounts, *l; And use g_settings_get() here? Or get_string()
Try setting it to '/' that's how we indicate _no object_ in Telepathy. As for the reading back the old value. That sounds like a bug in your code, but I can't immediately see where.
Created attachment 197039 [details] [review] Improved patch Modified patch according to the comments made by Danielle. It seems to work ok. Regarding the GSettings issue, I have checked that I get the same results when setting the chat theme, so it is a problem with GSettings in my computer. It should work fine in a working installation of GSettings. Thank you for your help and suggestions.
Review of attachment 197039 [details] [review]: Looks pretty good! I tested it and it works perfectly so I think we're ready to merge your patch as soon as we branch for 3.2 (master is in hard code freeze atm). Just a minor improvement and a possible improvement. ::: src/empathy-new-chatroom-dialog.c @@ +471,3 @@ + TpConnectionStatus status; + + TpAccountManager *manager; I'd add here: if (!g_str_has_prefix (account_path, TP_ACCOUNT_OBJECT_PATH_BASE)) continue; So we'll ignore invalid account paths; including '/' which is now the default. @@ +475,3 @@ + + manager = tp_account_manager_dup (); + TpAccount *account; Did you try using tp_account_manager_ensure_account() here? As the account manager has been prepared, it should return the same TpAccount object than the one in the accounts list and will save you iterating over the list.
Created attachment 197143 [details] [review] Switch to ensure_account() Yep, in fact, after a talk on IRC with some developers they pointed me out to both solutions, getting valid accounts and ensuring account. I do not remember why I chose the first one. I think there is no problem: if ensure_account returns an account that does not exist, it cannot be set when calling account_chooser_set_account().
Created attachment 197144 [details] [review] get_valid_accounts() solution I have updated the patch to fix some indentation errors.
Review of attachment 197143 [details] [review]: ::: src/empathy-new-chatroom-dialog.c @@ +479,3 @@ + account_path, + NULL, + GError *err = NULL; err is not used; you can set it to NULL.
Created attachment 197148 [details] [review] Fixed Done, GError *err = NULL; removed.
Review of attachment 197148 [details] [review]: Looks good! I'll merge it once we have branched for 3.2
Congratulations, your patch is the first one merged for 3.4 :)
*** Bug 589007 has been marked as a duplicate of this bug. ***