GNOME Bugzilla – Bug 608863
Separate the Empathy Accounts dialog into its own program
Last modified: 2010-02-15 16:14:01 UTC
Created attachment 152906 [details] [review] Initial patch to separate the accounts dialog into its own program It would be nice to have the accounts dialog be its own program, since these Telepathy accounts are more general than Empathy itself. And it would make sense for this to be installed as any other control panel. This would more cleanly separate some of that code out of the main program and better-integrate Telepathy accounts into the Gnome desktop as a general service. The attached patch does a few things: 1. Separates the dialog out into its own program 2. Adds a --socket argument to provide the ID of a GtkSocket; this way it can be embedded into a single-window control center, as developed by Thomas Wood [1] 3. Makes the accounts program a UniqueApp, so launching it a second time merely brings the existing window to the front. Current issues: a. regression: no longer highlights an account with a known error b. (minor) regression: if Empathy starts hidden and would show the accounts assistant/wizard, it won't automatically open upon making Empathy visible c. (minor) regression: the accounts program doesn't strictly stay on top of the Empathy main window (since it's effectively impossible to set that between processes). d. there's some mysterious g_object_unref () called upon a non-GObject in a certain use case. Brief debugging didn't turn up anything obvious - it seems to be happening in empathy-accounts, but it doesn't happen if you launch empathy-accounts before empathy (so it's impossible to just run through GDB). I've tested this against Thomas' single-window control center, and it seems to work fine. [1] http://blogs.gnome.org/thos/2010/01/15/gnomemoblin-control-center/
Danielle, could you please review this? (See the notes above) If you test it directly, you'll want to apply the patch on #608813 to avoid a crasher.
Review of attachment 152906 [details] [review]: Need to also fix the unreffing issue. It would be nice to be able to specify --account=<account id> (e.g. --account=salut/local_xmpp/account0) as a parameter. ::: data/empathy-accounts.desktop.in.in @@ +6,3 @@ +Exec=empathy-accounts +Icon=empathy +StartupNotify=false Really? ::: src/empathy-accounts-dialog.c @@ +2202,3 @@ + + command = g_strdup_printf ("empathy-accounts %s %s %s", + socket_option, try_import ? "--import" : "", hidden ? "--hidden" : ""); Rather than allocating strings, why not just create a char*[N_ITEMS] = { NULL, } and then have a counter i that you increment as you add items to the char* ? Something like: char *argv[5] = { NULL, }; int i = 0; if (try_import) argv[i++] = "--import" @@ +2206,3 @@ + g_free (socket_option); + + g_spawn_command_line_async (command, &error); Use gdk_spawn_on_screen()
(In reply to comment #0) Hi Travis. I didn't look at the code yet as Danielle already did a review. > a. regression: no longer highlights an account with a known error I'm not sure to really understand this regression. Does that mean that if you open the accounts dialog from the connection error infobar, the faulty account is not selected by default? If yes, that should be fixed before merging. Adding a --account arg as Danielle suggested would solve this. > d. there's some mysterious g_object_unref () called upon a non-GObject in a > certain use case. Brief debugging didn't turn up anything obvious - it seems to > be happening in empathy-accounts, but it doesn't happen if you launch > empathy-accounts before empathy (so it's impossible to just run through GDB). You can use "gdb --pid=$PID" to attach it to a running process.
Created attachment 153380 [details] [review] Separates the accounts dialog into its own program and provide a GIOModule-based version for the new integrated control center This fixes all the issues mentioned in my original comment and Danielle's comment #2 except for the strict window parenting. This removes the --socket option (since the module-based embedding makes it obsolete). It also adds the --select-account=<account ID> option for highlighting a specific account.
Danielle, could you please review the new patch? It's a bit different (for the new control center design) and fixes the mentioned bugs (see the description). Sorry that it's so long in a single patch - if you'd like, I can split it up (figured it was more important to get it out before I finish for the day).
Review of attachment 153380 [details] [review]: Just a few things that I've noticed. Although I don't understand the difference between a Page and a Panel. ::: src/cc-empathy-accounts-page.c @@ +45,3 @@ + * destroyed in our finalize(), since it causes side-effects if destroyed + * earlier */ + GtkWidget *accounts_window; Out of interest, what side effects? @@ +50,3 @@ +enum { + PROP_0, +}; Redundant? @@ +203,3 @@ + "display-name", _("Messaging and VoIP Accounts"), + "id", "general", + NULL); Can't put these in init() ? ::: src/cc-empathy-accounts-panel.c @@ +42,3 @@ +enum { + PROP_0, +}; Also redundant? @@ +103,3 @@ + "display-name", _("Messaging and VoIP Accounts"), + "id", "empathy-accounts.desktop", + NULL); Also can't this go in init() ? @@ +133,3 @@ + + object_class->get_property = cc_empathy_accounts_panel_get_property; + object_class->set_property = cc_empathy_accounts_panel_set_property; No properties, why define these? ::: src/empathy-accounts-common.c @@ +153,3 @@ + +out: + g_object_unref (cm_mgr); Does this unref belong here, or below @ line 224. I'm pretty sure that calling prepare_async acquires a ref for you that is released when you exit this callback (correct me if I'm wrong, but I've used that pattern elsewhere in Empathy). ::: src/empathy-accounts-dialog.c @@ +2216,3 @@ + account_path = tp_proxy_get_object_path (TP_PROXY (selected_account)); + if (!tp_account_parse_object_path (account_path, NULL, NULL, + &account_id, &error)) I think this is wrong. mc-tool refers to accounts with names like "gabble/jabber/danielle_2emadeley_40collabora_2eco_2euk0", but here you are simply passing "danielle_2emadeley_40collabora_2eco_2euk0". The spec implies that this is unique, but is that really true? It might just be easier to pass &account_path[strlen(TP_ACCOUNT_OBJECT_PATH_BASE)]. At the very least this makes it consistent with mc-tool. You can also then easily recreate the account path by prepending TP_ACCOUNT_OBJECT_PATH_BASE. ::: src/empathy-accounts.c @@ +77,3 @@ + tp_account_manager_prepare_async (account_mgr, NULL, + (GAsyncReadyCallback) empathy_accounts_manager_ready_for_show_assistant, + GUINT_TO_POINTER (hidden)); This is confusing me a lot. This callback is because you've prepared the account manager. Why are you preparing it again? @@ +102,3 @@ + GList *l; + + for (l = accounts; l; l = l->next) Yeah, you could skip this whole loop, and just try to look up the Account directly. path = g_strdup_printf ("%s%s", TP_ACCOUNT_OBJECT_PATH_BASE, account_path); account = tp_account_new (bus, path, &error); tp_account_prepare_async (account, NULL, account_ready, argv[2]); @@ +253,3 @@ + { + tp_account_manager_prepare_async (account_manager, NULL, + account_manager_ready_cb, NULL); Callback name could be clearer. @@ +269,3 @@ + g_object_unref (unique_app); + + return 0; You've used EXIT_SUCCESS elsewhere.
Created attachment 153461 [details] [review] Fixes based on Danielle's review in comment #6
(In reply to comment #6) The new attachment should address all these issues. Please let me know if there's anything else that needs fixing before merging. Individual points below. > Review of attachment 153380 [details] [review]: > > Just a few things that I've noticed. Although I don't understand the difference > between a Page and a Panel. A panel is basically a GtkNotebook, and a page is a page/tab. For the control center modules with multiple pages, each shows up as a tab in the panel (which is loaded when you click on an item in the control center). > ::: src/cc-empathy-accounts-page.c > @@ +45,3 @@ > + * destroyed in our finalize(), since it causes side-effects if destroyed > + * earlier */ > + GtkWidget *accounts_window; > > Out of interest, what side effects? It seems to invalidate all its original children, even if they've already been reparented by the time you destroy it. I've clarified that comment in the new patch. > @@ +50,3 @@ > +enum { > + PROP_0, > +}; > > Redundant? Cut. > @@ +203,3 @@ > + "display-name", _("Messaging and VoIP Accounts"), > + "id", "general", > + NULL); > > Can't put these in init() ? Actually, I've moved them into the call to g_object_new() and cut the constructor. > ::: src/cc-empathy-accounts-panel.c > @@ +42,3 @@ > +enum { > + PROP_0, > +}; > > Also redundant? Yes, additionally also redundant. Cut. > @@ +103,3 @@ > + "display-name", _("Messaging and VoIP Accounts"), > + "id", "empathy-accounts.desktop", > + NULL); > > Also can't this go in init() ? They're construct-only properties, so that doesn't work out (since the control center depends upon the "id" being set - it even crashes on g_str_hash(NULL) if I move the g_object_set() to init()). > @@ +133,3 @@ > + > + object_class->get_property = cc_empathy_accounts_panel_get_property; > + object_class->set_property = cc_empathy_accounts_panel_set_property; > > No properties, why define these? Forgot to wipe those out from the original (which also has these vestigial functions). Fixed in both the panel and page. > ::: src/empathy-accounts-common.c > @@ +153,3 @@ > + > +out: > + g_object_unref (cm_mgr); > > Does this unref belong here, or below @ line 224. I'm pretty sure that calling > prepare_async acquires a ref for you that is released when you exit this > callback (correct me if I'm wrong, but I've used that pattern elsewhere in > Empathy). Looking at the code for empathy_connection_managers_prepare_async(), it doesn't seem to ref cm_mgr, so I think this is correct. I'm fairly sure I didn't change this pattern from the original code. > ::: src/empathy-accounts-dialog.c > @@ +2216,3 @@ > + account_path = tp_proxy_get_object_path (TP_PROXY (selected_account)); > + if (!tp_account_parse_object_path (account_path, NULL, NULL, > + &account_id, &error)) > > I think this is wrong. mc-tool refers to accounts with names like > "gabble/jabber/danielle_2emadeley_40collabora_2eco_2euk0", but here you are > simply passing "danielle_2emadeley_40collabora_2eco_2euk0". The spec implies > that this is unique, but is that really true? It might just be easier to pass > &account_path[strlen(TP_ACCOUNT_OBJECT_PATH_BASE)]. > > At the very least this makes it consistent with mc-tool. You can also then > easily recreate the account path by prepending TP_ACCOUNT_OBJECT_PATH_BASE. Fixed. > ::: src/empathy-accounts.c > @@ +77,3 @@ > + tp_account_manager_prepare_async (account_mgr, NULL, > + (GAsyncReadyCallback) empathy_accounts_manager_ready_for_show_assistant, > + GUINT_TO_POINTER (hidden)); > > This is confusing me a lot. This callback is because you've prepared the > account manager. Why are you preparing it again? And this is why we do reviews :) It got refactored and pushed around several times, so I just didn't notice the absurdity there. Fixed. > @@ +102,3 @@ > + GList *l; > + > + for (l = accounts; l; l = l->next) > > Yeah, you could skip this whole loop, and just try to look up the Account > directly. > > path = g_strdup_printf ("%s%s", TP_ACCOUNT_OBJECT_PATH_BASE, account_path); > account = tp_account_new (bus, path, &error); > tp_account_prepare_async (account, NULL, account_ready, argv[2]); It's a short list, doesn't require adding another callback, etc., so I think it's just cleaner to handle it as-is. > @@ +253,3 @@ > + { > + tp_account_manager_prepare_async (account_manager, NULL, > + account_manager_ready_cb, NULL); > > Callback name could be clearer. Fixed -> now account_manager_ready_for_assistant_cb(). > @@ +269,3 @@ > + g_object_unref (unique_app); > + > + return 0; > > You've used EXIT_SUCCESS elsewhere. Fixed.
(In reply to comment #8) > > Out of interest, what side effects? > > It seems to invalidate all its original children, even if they've already been > reparented by the time you destroy it. I've clarified that comment in the new > patch. That seems very wrong. > > Also can't this go in init() ? > > They're construct-only properties, so that doesn't work out (since the control > center depends upon the "id" being set - it even crashes on g_str_hash(NULL) if > I move the g_object_set() to init()). Aaah. Ok. > > ::: src/empathy-accounts-common.c > > @@ +153,3 @@ > > + > > +out: > > + g_object_unref (cm_mgr); > > > > Does this unref belong here, or below @ line 224. I'm pretty sure that calling > > prepare_async acquires a ref for you that is released when you exit this > > callback (correct me if I'm wrong, but I've used that pattern elsewhere in > > Empathy). > > Looking at the code for empathy_connection_managers_prepare_async(), it doesn't > seem to ref cm_mgr, so I think this is correct. I'm fairly sure I didn't change > this pattern from the original code. Hmm, I should check that I didn't stuff this up when I fixed that uninitialised access the other day. > > @@ +102,3 @@ > > + GList *l; > > + > > + for (l = accounts; l; l = l->next) > > > > Yeah, you could skip this whole loop, and just try to look up the Account > > directly. > > > > path = g_strdup_printf ("%s%s", TP_ACCOUNT_OBJECT_PATH_BASE, account_path); > > account = tp_account_new (bus, path, &error); > > tp_account_prepare_async (account, NULL, account_ready, argv[2]); > > It's a short list, doesn't require adding another callback, etc., so I think > it's just cleaner to handle it as-is. An additional callback to prepare the account? The account still needs to be prepared somewhere. Accounts coming out of the AM are not prepared. If it doesn't happen in empathy_accounts_show_accounts_ui() then that's another bug. Totally failed to pick this up last time.
(In reply to comment #9) > > > @@ +102,3 @@ > > > + GList *l; > > > + > > > + for (l = accounts; l; l = l->next) > > > > > > Yeah, you could skip this whole loop, and just try to look up the Account > > > directly. > > > > > > path = g_strdup_printf ("%s%s", TP_ACCOUNT_OBJECT_PATH_BASE, account_path); > > > account = tp_account_new (bus, path, &error); > > > tp_account_prepare_async (account, NULL, account_ready, argv[2]); > > > > It's a short list, doesn't require adding another callback, etc., so I think > > it's just cleaner to handle it as-is. > > An additional callback to prepare the account? The account still needs to be > prepared somewhere. Accounts coming out of the AM are not prepared. If it > doesn't happen in empathy_accounts_show_accounts_ui() then that's another bug. > Totally failed to pick this up last time. Thanks for pointing that out. I think we're safe on that front, though. I don't see any place where we rely on any of the properties which require a prior tp_account_prepare_async(). The account just gets fed into empathy_accounts_dialog_show(), and I haven't changed anything below that. The EmpathyAccountsDialog sets up an EmpathyAccountSettings upon its construction, and that ends up preparing all the accounts. Is there another way that it would matter that I'm not preparing the account here?
Created attachment 153492 [details] [review] Ensure the selected account is prepared before we try to select it in the accounts dialog
(In reply to comment #10) > Thanks for pointing that out. I think we're safe on that front, though. I don't > see any place where we rely on any of the properties which require a prior > tp_account_prepare_async(). The account just gets fed into > empathy_accounts_dialog_show(), and I haven't changed anything below that. The > EmpathyAccountsDialog sets up an EmpathyAccountSettings upon its construction, > and that ends up preparing all the accounts. > > Is there another way that it would matter that I'm not preparing the account > here? Scratch this -- I've created a patch that should ensure an account's preparedness by the time we open the accounts dialog. Please review.
Created attachment 153563 [details] [review] Make the control center embedding support conditional
OK, this last patch should make this safe for mainlining. Guillaume, could you please review these 4 patches and let me know if they're ready to push to master? Note that the last 3 are fixes for the first one, so it may be slightly easier to squash them all to remove some noise.
(In reply to comment #14) > OK, this last patch should make this safe for mainlining. > > Guillaume, could you please review these 4 patches and let me know if they're > ready to push to master? > > Note that the last 3 are fixes for the first one, so it may be slightly easier > to squash them all to remove some noise. As decided on IRC, Danielle will review the last patch and I'll push all of them to master as soon as I get approval on this last one.
Review of attachment 153563 [details] [review]: Looks pretty good. I'm happy with this. Perhaps squash some of these last commits together and merge it? ::: src/Makefile.am @@ +132,3 @@ +#check_c_sources += $(libempathy_accounts_panel_la_SOURCES) +#endif + Drop this commented out bit?
(In reply to comment #16) > Review of attachment 153563 [details] [review]: > > Looks pretty good. I'm happy with this. Perhaps squash some of these last > commits together and merge it? > > ::: src/Makefile.am > @@ +132,3 @@ > +#check_c_sources += $(libempathy_accounts_panel_la_SOURCES) > +#endif > + > > Drop this commented out bit? Fixed that, rebased, tested, and pushed to master. Thanks for all the reviews.
*** Bug 599174 has been marked as a duplicate of this bug. ***