GNOME Bugzilla – Bug 596575
Automatic salut account creation doesn't work
Last modified: 2010-01-25 14:13:57 UTC
If the user didn't edit informations in the about-me dialog, the self contact does not exist in eds and salut account creation fails. I think it is the case for most users... I propose to take first/lastname from system's real name if it fails to get them from eds. If system's real name (returned by g_get_real_name) is in the form "Foo Bar" we assume firstname to be "Foo" and lastname "Bar". That should work for almost all cases. For example at ubuntu's installation the real name is asked to the user to be his fullname. Of course it's not 100% unbreakable, but that should be enough for a default. The user can still edit his salut account afterward. Patch: http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=commitdiff;h=da560f21218a396be09962cd0e628127dbaf9623
*** Bug 596557 has been marked as a duplicate of this bug. ***
*** Bug 596628 has been marked as a duplicate of this bug. ***
Your commit doesn't contain the bug number. I'm not convinced by trying to infer the last name from the first name. Furthermore with current code you leak the second half of the first_name string (the part after the ' '). Maybe we should just let the result of g_get_real_name () in first_name and set last_name to '' or something. We can't guess which parts of the real_name are the first/last name so maybe we shouldn't pretend to know it.
<sjoerd> cassidy: simple, parsing names into first and last names is made of fail, don't do that <sjoerd> cassidy: the gnome-about-me/ebook stuff should solve that issue, not us <sjoerd> If you can't do it automagically don't do it automagically <sjoerd> I'd rather we pop up a UI asking the user then get it wrong <sjoerd> cassidy: don't be so scared to ask the user for a bit more information <sjoerd> cassidy: also note that in case of live cds the system username could be nonsense Solution could indeed be to ask to user about his first and last name. There are 2 cases here: - the account is created in the context of the wizard; then the wizard should ask for them - the account isn't created in the wizard; then we should probably display a dialog. We could share the same widget of course.
I notice that "People nearby" fails to connect if I don't provide a nickname, though that field seems optional in the UI. This is the same bug, right?
Murray: no, Salut is suposed to be able to connect if you fill the first and last name fields. Could you please open another bug about that and attach Salut logs please?
Created attachment 152015 [details] [review] http://git.collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/salut-account-596575 .../empathy-account-widget-local-xmpp.ui | 2 +- libempathy-gtk/empathy-account-widget.c | 5 + src/empathy-account-assistant.c | 461 ++++++++++++++++---- src/empathy-account-assistant.h | 5 +- src/empathy-auto-salut-account-helper.c | 113 +++-- src/empathy-auto-salut-account-helper.h | 3 + src/empathy.c | 129 ++++-- tests/interactive/test-empathy-account-assistant.c | 25 +- 8 files changed, 571 insertions(+), 172 deletions(-)
Review of attachment 152015 [details] [review]: General comments: - one thing I don't like: as we're able to predict if the salut account creation is going to be impossible for the lack of the CM, why do we show that as an option at all in that case? I think it'd be better to hide the last checkbox entirely from the wizard instead of having an error page in which we explain what to install. - there seems to be a bug in the code; when you run the assistant, the "Back" button is not displayed in the enter/create page until you change the option in the "create again" checkbox. Also, the Back button is never displayed in the import page (I think it should). Also, I inlined some comments below: ::: src/empathy-account-assistant.c @@ +114,3 @@ static GtkWidget * + const gchar *secondary_message) +build_error_page (const gchar *primary_message, It's probably better to s/build_error_page/build_error_vbox/ as we already have a similarly-named account_assistant_build_error_page() </nitpick> @@ +174,3 @@ + (_("The error message was: <span style=\"italic\">%s</span>"), + error->message); +} (I know it's my bad as I wrote that code in the first place, but anyway :P) To help translators, it's probably better to leave the markup out of the string marked for translation; something like markup = g_markup_printf_escaped (<span...>%s</span>, message); secondary = g_strdup_printf (_("The error message was: %s", markup)) would probably work. Also, secondary_message is declared here as const char* but it's not const. @@ +1057,3 @@ + "details below are correct. " + "You can easily change these details later or disable this feature " + "by using the 'Accounts' dialog.")); I'd add here a hint on how to reach that dialog (adding "(Edit->Accounts)", maybe in italic, at the end of the paragraph would do). @@ +1075,3 @@ + g_signal_connect (w, "toggled", + G_CALLBACK (create_salut_check_box_toggled_cb), self); + gtk_widget_show (w); I don't like this checkbox too much; we already have a "back" button in case the user doesn't want to enable the feature. Also, there seems to be a bug in the code; if you enable the checkbox and click on "Apply", a blank page will be displayed. Anyway, I think it'd be better to remove the checkbox entirely. @@ +1108,3 @@ + "network as you because telepathy-salut is not installed.\n" + "If you want to enable this feature, you should install " + "telepathy-salut and activate it in the Accounts dialog")); If we're going to display the error page, I would change the wording a bit...how about "You won't be able to chat with people connected to your local network, as telepathy-salut is not installed. If you want to enable this feature, please install the telepathy-salut package and create a People Nearby account from the Accounts dialog (Edit->Accounts)"? @@ +1171,3 @@ + + g_assert (priv->connection_mgrs != NULL); + g_assert (empathy_connection_managers_is_ready (priv->connection_mgrs)); Is this really needed? Couldn't we delay the initialization of the object until the cms are ready and remove the assert? ::: src/empathy-auto-salut-account-helper.c @@ +125,3 @@ + DEBUG ("Failed to get self econtact: %s", + error ? error->message : "No error given"); + g_error_free (error); As we check for error != NULL in the DEBUG line, we should either check it also here or never check it.
(In reply to comment #8) > - one thing I don't like: as we're able to predict if the salut account > creation is going to be impossible for the lack of the CM, why do we show that > as an option at all in that case? I think it'd be better to hide the last > checkbox entirely from the wizard instead of having an error page in which we > explain what to install. I did it that way because: a) I think it's nice to keep the assistant looking the same in all cases so all users will have the same "first time user experience" which will always fit the documentation. b) If we skip this screen, user will never know that he's missing a component and won't be able to understand why he can't see his friends connected on the network. > - there seems to be a bug in the code; when you run the assistant, the "Back" > button is not displayed in the enter/create page until you change the option in > the "create again" checkbox. Also, the Back button is never displayed in the > import page (I think it should). Fixed.
Review of attachment 152015 [details] [review]: ::: src/empathy-account-assistant.c @@ +114,3 @@ static GtkWidget * +build_error_page (const gchar *primary_message, + const gchar *secondary_message) Agreed. Fixed. @@ +174,3 @@ + secondary_message = g_markup_printf_escaped + (_("The error message was: <span style=\"italic\">%s</span>"), + error->message); Good point; done. @@ +1057,3 @@ + "details below are correct. " + "You can easily change these details later or disable this feature " + "by using the 'Accounts' dialog.")); done. @@ +1075,3 @@ + g_signal_connect (w, "toggled", + G_CALLBACK (create_salut_check_box_toggled_cb), self); + gtk_widget_show (w); The "intro" page doesn't have an option to say "I don't want to do anything". Furthermore, this page is also displayed when you're done with importing or creating your accounts, so we do need a way to say "no thanks". I fixed the blank page bug. @@ +1108,3 @@ + "network as you because telepathy-salut is not installed.\n" + "If you want to enable this feature, you should install " + "telepathy-salut and activate it in the Accounts dialog")); done. @@ +1171,3 @@ + + g_assert (priv->connection_mgrs != NULL); + g_assert (empathy_connection_managers_is_ready (priv->connection_mgrs)); Not really. :\ We could add the pages later but then they won't be displayed as the caller will already have call show_all on the assistant. We could display them ourself but that will be a bit weird and not really in the idea of GTK+ widgets. ::: src/empathy-auto-salut-account-helper.c @@ +125,3 @@ + DEBUG ("Failed to get self econtact: %s", + error ? error->message : "No error given"); + g_error_free (error); I checked in the code and there is no reason why error would be NULL if the function returned FALSE. Fixed.
(In reply to comment #9) > I did it that way because: > a) I think it's nice to keep the assistant looking the same in all cases so all > users will have the same "first time user experience" which will always fit the > documentation. > b) If we skip this screen, user will never know that he's missing a component > and won't be able to understand why he can't see his friends connected on the > network. Ok, I agree with the rationale in a). I re-checked your updated branch and you seem to have added some markup in translatable strings in commits c28544c5e0e0fa0af7e781482ec85963480b2545 and 3fdd529baac701a921d656673e892d1d5d12b715; it would be nice if you split that markup from the strings marked for translation. With that fixed, feel free to merge :)
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.