GNOME Bugzilla – Bug 764283
Misc usability improvements to the imap-smtp provider
Last modified: 2021-07-05 10:58:22 UTC
A bunch of small changes I made while testing the GTask work.
Created attachment 324882 [details] [review] imap-smtp: Workaround bgo#710888 gtk_widget_hide followed gtk_widget_show is sometimes not working with GtkInfoBar, especially with gtk+ 3.20. This can be reproduced when adding an IMAP/SMTP account and triggering the 'self-signed certificate' infobar, or the 'invalid password' one for IMAP and then SMTP. This commit works around this by disabling the GtkRevealer used by GtkInfoBar.
Created attachment 324883 [details] [review] smtp: Reuse imap username by default We already automatically reuse the IMAP password on the SMTP page. It makes sense to also keep the IMAP username as they are likely to go in tandem.
Created attachment 324884 [details] [review] imap-smtp: Split guess_imap_smtp Guessing SMTP parameters right before we need to show them allows us to reuse the user-modification to the IMAP data in our guess. Currently, IMAP username and password are reused as is. IMAP server is reused as is, unless it starts with 'imap.'. In this case, it's replaced with 'smtp.'
Created attachment 324885 [details] [review] imap-smtp: Hide cluebar after user input When a cluebar is shown, it's still being shown after the user has clicked on the 'Ignore'/'Next' button until there is successful validation of the password. It's better to hide it while the new verification is ongoing, and show a new infobar if needed.
Created attachment 324896 [details] [review] imap-smtp: Fix add_account() leak ==9457== 11 bytes in 1 blocks are definitely lost in loss record 972 of 23,129 ==9457== at 0x4C28BF6: malloc (vg_replace_malloc.c:299) ==9457== by 0x1456711C: g_malloc (gmem.c:94) ==9457== by 0x145673FE: g_malloc_n (gmem.c:331) ==9457== by 0x14581DCE: g_strdup (gstrfuncs.c:363) ==9457== by 0x5BE4A17: goa_utils_parse_email_address (goautils.c:557) ==9457== by 0x5BD62A5: add_account (goaimapsmtpprovider.c:1084) ==9457== by 0x5BBD6A1: goa_provider_add_account (goaprovider.c:382) ==9457== by 0x49C7D1: add_account_dialog_add_account (cc-online-accounts-add-account-dialog.c:83) ==9457== by 0x49C864: list_box_row_activated_cb (cc-online-accounts-add-account-dialog.c:102) ==9457== by 0x142CC6D4: g_cclosure_marshal_VOID__OBJECTv (gmarshal.c:2102) ==9457== by 0x142C7C43: _g_closure_invoke_va (gclosure.c:867) ==9457== by 0x142E2524: g_signal_emit_valist (gsignal.c:3294) ==9457== by 0x142E369C: g_signal_emit (gsignal.c:3441) ==9457== by 0x1264EAF2: gtk_list_box_activate (gtklistbox.c:1754) ==9457== by 0x1264EB4B: gtk_list_box_select_and_activate_full (gtklistbox.c:1768) ==9457== by 0x1264F115: gtk_list_box_multipress_gesture_released (gtklistbox.c:1964) ==9457== by 0x1D52AD2F: ffi_call_unix64 (unix64.S:76) ==9457== by 0x1D52A79A: ffi_call (ffi64.c:525)
Review of attachment 324896 [details] [review]: Thanks for catching this. There is a similar leak in refresh_account. ::: src/goabackend/goaimapsmtpprovider.c @@ +1083,2 @@ g_cancellable_reset (data.cancellable); + g_free (domain); Can you please move it just above the "goto smtp_again;" statement? That is also where we unref smtp_auth.
Review of attachment 324883 [details] [review]: ::: src/goabackend/goaimapsmtpprovider.c @@ +1033,3 @@ + /* Re-use the username and password from the IMAP page */ + gtk_entry_set_text (GTK_ENTRY (data.smtp_username), imap_username); Nice. Maybe we should remove the corresponding line from guess_imap_smtp? Possibly with a comment? What do you think? @@ +1372,3 @@ + /* Re-use the username and password from the IMAP page */ + gtk_entry_set_text (GTK_ENTRY (data.smtp_username), imap_username); This doesn't seem right because we don't expect the usernames to change in refresh_account. We only expect that the password has changed. That is why we use gtk_editable_set_editable to mark some of the entries as read-only.
Review of attachment 324883 [details] [review]: ::: src/goabackend/goaimapsmtpprovider.c @@ +1033,3 @@ + /* Re-use the username and password from the IMAP page */ + gtk_entry_set_text (GTK_ENTRY (data.smtp_username), imap_username); This is removed by the next commit 'imap-smtp: Split guess_imap_smtp' I guess this commit could be squashed into it actually @@ +1372,3 @@ + /* Re-use the username and password from the IMAP page */ + gtk_entry_set_text (GTK_ENTRY (data.smtp_username), imap_username); Ah, I haven't been able to find a way to see the refresh_account UI actually :) I'll drop this hunk then.
Created attachment 325064 [details] [review] imap-smtp: Fix add_account() leak While at it, fix the same leak in refresh_account() ==9457== 11 bytes in 1 blocks are definitely lost in loss record 972 of 23,129 ==9457== at 0x4C28BF6: malloc (vg_replace_malloc.c:299) ==9457== by 0x1456711C: g_malloc (gmem.c:94) ==9457== by 0x145673FE: g_malloc_n (gmem.c:331) ==9457== by 0x14581DCE: g_strdup (gstrfuncs.c:363) ==9457== by 0x5BE4A17: goa_utils_parse_email_address (goautils.c:557) ==9457== by 0x5BD62A5: add_account (goaimapsmtpprovider.c:1084) ==9457== by 0x5BBD6A1: goa_provider_add_account (goaprovider.c:382) ==9457== by 0x49C7D1: add_account_dialog_add_account (cc-online-accounts-add-account-dialog.c:83) ==9457== by 0x49C864: list_box_row_activated_cb (cc-online-accounts-add-account-dialog.c:102) ==9457== by 0x142CC6D4: g_cclosure_marshal_VOID__OBJECTv (gmarshal.c:2102) ==9457== by 0x142C7C43: _g_closure_invoke_va (gclosure.c:867) ==9457== by 0x142E2524: g_signal_emit_valist (gsignal.c:3294) ==9457== by 0x142E369C: g_signal_emit (gsignal.c:3441) ==9457== by 0x1264EAF2: gtk_list_box_activate (gtklistbox.c:1754) ==9457== by 0x1264EB4B: gtk_list_box_select_and_activate_full (gtklistbox.c:1768) ==9457== by 0x1264F115: gtk_list_box_multipress_gesture_released (gtklistbox.c:1964) ==9457== by 0x1D52AD2F: ffi_call_unix64 (unix64.S:76) ==9457== by 0x1D52A79A: ffi_call (ffi64.c:525)
Created attachment 325066 [details] [review] smtp: Reuse imap username by default We already automatically reuse the IMAP password on the SMTP page. It makes sense to also keep the IMAP username as they are likely to go in tandem.
Review of attachment 325066 [details] [review]: Thanks Christophe. Looks good to me. Pushed to master and gnome-3-20.
Review of attachment 325064 [details] [review]: ::: src/goabackend/goaimapsmtpprovider.c @@ +1140,2 @@ g_clear_object (&smtp_auth); + g_free (domain); On second thoughts, g_clear_pointer will be slightly better because it will completely revert it back to the state it was in before smtp_again.
Created attachment 327152 [details] [review] imap-smtp: Fix add_account() leak Fixed and pushed.
Review of attachment 324885 [details] [review]: Pushed to both master and gnome-3-20.
Review of attachment 324884 [details] [review]: I like this, but let me ramble a bit for the sake of leaving some paper trail. :) Personally, I like this because my email server uses mail.lostca.se for both IMAP and SMTP. So it is better if it just works after I have edited imap.lostca.se to be mail.lostca.se. I also agree that most email setups out there are either imap/smtp.<domain> or <something>.<domain> and it makes sense to optimize for the majority use-case. However, I wonder if it makes it more complicated for those who have some weird setup like imap2.<something>.<domain>. I guess, something more sophisticated like RFC 6186 [1] or http://api.gnome.org/evolution/autoconfig/1.1/<domain> (that Evolution uses) should adequately address those. [1] http://tools.ietf.org/html/rfc6186 ::: src/goabackend/goaimapsmtpprovider.c @@ +837,3 @@ + if (g_str_has_prefix (smtp_server, "imap.")) { + strncpy (smtp_server, "smtp.", strlen ("smtp.")); + } Nitpick: we don't need the braces and the indentation should be 2 spaces.
Review of attachment 324882 [details] [review]: Thanks for catching this, Christophe. ::: src/goabackend/goaimapsmtpprovider.c @@ +669,3 @@ + gtk_revealer_set_transition_type (GTK_REVEALER (revealer), + GTK_REVEALER_TRANSITION_TYPE_NONE); + gtk_revealer_set_transition_duration (GTK_REVEALER (revealer), 0); We should really fix this in gtk+. There were a few regressions in GtkRevealer during the 3.20 cycle (eg., bug 761760 and bug 762996) and this is yet another.
(In reply to Debarshi Ray from comment #16) > Review of attachment 324882 [details] [review] [review]: > > Thanks for catching this, Christophe. > > ::: src/goabackend/goaimapsmtpprovider.c > @@ +669,3 @@ > + gtk_revealer_set_transition_type (GTK_REVEALER (revealer), > + GTK_REVEALER_TRANSITION_TYPE_NONE); > + gtk_revealer_set_transition_duration (GTK_REVEALER (revealer), 0); > > We should really fix this in gtk+. There were a few regressions in > GtkRevealer during the 3.20 cycle (eg., bug 761760 and bug 762996) and this > is yet another. Do you have a simple reproducer? If not, I can write one.
(In reply to Debarshi Ray from comment #17) > (In reply to Debarshi Ray from comment #16) > > Review of attachment 324882 [details] [review] [review] [review]: > > > > Thanks for catching this, Christophe. > > > > ::: src/goabackend/goaimapsmtpprovider.c > > @@ +669,3 @@ > > + gtk_revealer_set_transition_type (GTK_REVEALER (revealer), > > + GTK_REVEALER_TRANSITION_TYPE_NONE); > > + gtk_revealer_set_transition_duration (GTK_REVEALER (revealer), 0); > > > > We should really fix this in gtk+. There were a few regressions in > > GtkRevealer during the 3.20 cycle (eg., bug 761760 and bug 762996) and this > > is yet another. > > Do you have a simple reproducer? If not, I can write one. It's in https://bugzilla.gnome.org/show_bug.cgi?id=710888#c8 I think Company/baedert/cosimoc were discussing something related on #gtk+ today.
Review of attachment 324882 [details] [review]: ::: src/goabackend/goaimapsmtpprovider.c @@ +669,3 @@ + gtk_revealer_set_transition_type (GTK_REVEALER (revealer), + GTK_REVEALER_TRANSITION_TYPE_NONE); + gtk_revealer_set_transition_duration (GTK_REVEALER (revealer), 0); Ok, time to change my opinion. Given that (a) this particular bug (ie. bug 710888) has been around since 2013, and (b) the conversation between Company and cosimoc in #gtk+ yesterday [*], I think we are better off working around it. However, as I mentioned on bug 710888, I think it would be better to put the GtkInfoBar in a GtkRevealer, instead of poking at the one that is inside it. [*] I wish I had a reference.
(In reply to Debarshi Ray from comment #19) > Review of attachment 324882 [details] [review] [review]: > > ::: src/goabackend/goaimapsmtpprovider.c > @@ +669,3 @@ > + gtk_revealer_set_transition_type (GTK_REVEALER (revealer), > + GTK_REVEALER_TRANSITION_TYPE_NONE); > + gtk_revealer_set_transition_duration (GTK_REVEALER (revealer), 0); > > Ok, time to change my opinion. > > Given that (a) this particular bug (ie. bug 710888) has been around since > 2013, and (b) the conversation between Company and cosimoc in #gtk+ > yesterday [*], I think we are better off working around it. This has since been fixed in gtk+ master: https://bugzilla.gnome.org/show_bug.cgi?id=710888#c15
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-online-accounts/-/issues/ Thank you for your understanding and your help.