After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 791230 - Should allow choosing sending address always when account has multiple addresses configured
Should allow choosing sending address always when account has multiple addres...
Status: RESOLVED FIXED
Product: geary
Classification: Other
Component: composer
master
Other Linux
: Normal normal
: 0.14.0
Assigned To: Geary Maintainers
Geary Maintainers
: 738902 (view as bug list)
Depends on:
Blocks: 778764
 
 
Reported: 2017-12-04 23:58 UTC by Michael Catanzaro
Modified: 2018-06-26 15:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Always allow changing from address if account has multiple addresses (1.35 KB, patch)
2017-12-06 13:48 UTC, Michael Catanzaro
none Details | Review
Never enter inline compact mode when account has multiple emails (1.92 KB, patch)
2017-12-06 13:48 UTC, Michael Catanzaro
none Details | Review
tlsclientconnection: Update use-ssl3 documentation (3.58 KB, patch)
2018-01-07 22:15 UTC, Michael Catanzaro
none Details | Review
tlsclientconnection: Deprecate ssl3 property and functions (3.29 KB, patch)
2018-01-07 22:15 UTC, Michael Catanzaro
none Details | Review
Always allow changing from address if account has multiple addresses (1.35 KB, patch)
2018-01-08 00:58 UTC, Michael Catanzaro
committed Details | Review
Never enter inline compact mode when account has multiple emails (1.92 KB, patch)
2018-01-08 00:58 UTC, Michael Catanzaro
committed Details | Review
Improve heuristic to decide whether to show From selection (8.54 KB, patch)
2018-01-08 00:58 UTC, Michael Catanzaro
needs-work Details | Review

Description Michael Catanzaro 2017-12-04 23:58:55 UTC
I'm trying to send a mail to the Fedora devel list, but getting rejected. Geary is sending the mail with my personal Posteo address, but I need it to be sent using my @gnome.org alias as that's the address I'm subscribed with. I have that configured in Geary as an "Additional email address" on my Posteo account.

There's no easy way for Geary to guess this. If we want to be really smart, it could look at the last (i.e. the first/original) Delivered-To header in the file and use that as a heuristic to choose the right sending address. But really, we should always display From and let the user choose when an account has multiple email addresses enabled. Otherwise, users will always be mad when Geary fails to be omniscient.
Comment 1 Michael Catanzaro 2017-12-06 13:48:20 UTC
Created attachment 365106 [details] [review]
Always allow changing from address if account has multiple addresses
Comment 2 Michael Catanzaro 2017-12-06 13:48:23 UTC
Created attachment 365107 [details] [review]
Never enter inline compact mode when account has multiple emails

If the current account has multiple emails, we must never enter inline
compact mode, otherwise the user has no chance to pick the right email.
Geary currently tries to guess which email to send the mail from, but
it can't always be right.
Comment 3 Michael Gratton 2017-12-07 13:36:24 UTC
Hey, thanks for these patches. I think I tend to agree with the rationale, will have a look at them on the weekend.
Comment 4 Michael Gratton 2017-12-07 13:38:04 UTC
*** Bug 738902 has been marked as a duplicate of this bug. ***
Comment 5 Michael Gratton 2017-12-14 00:13:09 UTC
This also sounds a bit like Bug 778764. Does detaching the composer let you change the email in this case?

I'm wondering if a slightly smarter version of this is to look at the being-replied-to recipients, and if at least one matches those configured for the account then use that as the from address and display a compact composer, otherwise do as these patches do and show a full-blown one?

We also probably want to allow the user to select a from address if multiple are configured in the inline-but-not-compact case.
Comment 6 Michael Catanzaro 2017-12-14 12:26:51 UTC
(In reply to Michael Gratton from comment #5)
> This also sounds a bit like Bug 778764. Does detaching the composer let you
> change the email in this case?

Um, yes, I never noticed that... of course, it would be nice to not need to remember to detach the composer before sending my mails. ;)

> I'm wondering if a slightly smarter version of this is to look at the
> being-replied-to recipients, and if at least one matches those configured
> for the account then use that as the from address and display a compact
> composer, otherwise do as these patches do and show a full-blown one?

Nope, that doesn't work for mailing lists. I think Geary already does that to decide which sending address to select... if that worked, I wouldn't be filing this bug.

> We also probably want to allow the user to select a from address if multiple
> are configured in the inline-but-not-compact case.

My first patch is supposed to accomplish that; does it not work for you?
Comment 7 Michael Catanzaro 2017-12-14 12:36:35 UTC
(In reply to Michael Gratton from comment #5)
> I'm wondering if a slightly smarter version of this is to look at the
> being-replied-to recipients, and if at least one matches those configured
> for the account then use that as the from address and display a compact
> composer, otherwise do as these patches do and show a full-blown one?

Ah yes, sorry, I misunderstood. Yes, that would work!
Comment 8 Federico Bruni 2017-12-17 09:55:14 UTC
Related: bug #747893 (set a default From address for an account having multiple email addresses).
Comment 9 Michael Catanzaro 2018-01-07 22:15:00 UTC
Created attachment 366469 [details] [review]
tlsclientconnection: Update use-ssl3 documentation

The property documentation correctly indicates how this code works
nowadays, but the function documentation is obsolete and misleading.
Update it.

https://bugzilla.gnome.org/show_bug.cgi?id=745637
Comment 10 Michael Catanzaro 2018-01-07 22:15:03 UTC
Created attachment 366470 [details] [review]
tlsclientconnection: Deprecate ssl3 property and functions

I originally planned to introduce a new property and functions to
replace these, with the same behavior but less-confusing names. But that
might not be the best approach in the long run. Instead, let's just
deprecate them without replacement.

TLS 1.2 intolerance is no longer a thing in the wild, and no known
GTlsBackend supports TLS 1.3 yet. But you might need to use this
property in the future, even though it's deprecated, if your
GTlsBackend has added support for TLS 1.3 and you need to talk to a
server that is TLS 1.3 intolerant.

Independently of all that, these APIs simply no longer do what their
names suggest, so deprecation is sensible regardless.

https://bugzilla.gnome.org/show_bug.cgi?id=745637
Comment 11 Michael Catanzaro 2018-01-07 22:15:50 UTC
Sorry, wrong bug.

This was my second attempt. I just attached these patches to two wrong bugs, twice in a row. Grrr.
Comment 12 Michael Catanzaro 2018-01-08 00:58:46 UTC
Created attachment 366473 [details] [review]
Always allow changing from address if account has multiple addresses
Comment 13 Michael Catanzaro 2018-01-08 00:58:50 UTC
Created attachment 366474 [details] [review]
Never enter inline compact mode when account has multiple emails

If the current account has multiple emails, we must never enter inline
compact mode, otherwise the user has no chance to pick the right email.
Geary currently tries to guess which email to send the mail from, but
it can't always be right.
Comment 14 Michael Catanzaro 2018-01-08 00:58:55 UTC
Created attachment 366475 [details] [review]
Improve heuristic to decide whether to show From selection

The previous two commits ensured that we do not hide the From selection
when an account has multiple addresses, and that we do not enter inline
compact mode in this case either (which would also result in the From
selection not appearing). But it's actually OK to hide From if Geary is
going to be able to guess the right address to use for the reply, which
is generally the case in the absence of mailing lists or automatic
forwarding. If exactly one of this account's addresses appears in the
email headers of the mail being replied to, then that is probably the
account the user will want to use to reply.

To implement this heuristic, we obviously have to check the To, CC, and
BCC fields of the mail being replied to, to see if the mail was directly
addressed to one of this account's emails. But we also have to check
From, because the user might have sent the last mail in the
conversation.

I'm not sure where the code that actually picks the address to use
lives, but wherever it is, it seems to do a good job whenever this
heuristic says to hide the From selection.
Comment 15 Michael Catanzaro 2018-01-08 01:00:10 UTC
(In reply to Michael Gratton from comment #5)
> I'm wondering if a slightly smarter version of this is to look at the
> being-replied-to recipients, and if at least one matches those configured
> for the account then use that as the from address and display a compact
> composer, otherwise do as these patches do and show a full-blown one?

OK, I was able to implement this. Does it look OK?

> We also probably want to allow the user to select a from address if multiple
> are configured in the inline-but-not-compact case.

That's already handled by my original patches.
Comment 16 Michael Gratton 2018-01-26 04:51:59 UTC
Review of attachment 366474 [details] [review]:

Looks good, thanks. Pushed to master as commit 77d8516.
Comment 17 Michael Gratton 2018-01-26 04:52:41 UTC
Review of attachment 366473 [details] [review]:

Looks good, thanks. Pushed to master as commit 77d8516.
Comment 18 Michael Gratton 2018-01-26 04:53:40 UTC
(In reply to Michael Gratton from comment #16)
> Review of attachment 366474 [details] [review] [review]:
> 
> Looks good, thanks. Pushed to master as commit 77d8516.

Actually, the second patch was commit dec4416.
Comment 19 Michael Gratton 2018-01-26 05:03:12 UTC
Review of attachment 366475 [details] [review]:

This in general looks good, but there's some some stuff I'd like to see tidied up before committing this:

::: src/client/composer/composer-widget.vala
@@ +367,3 @@
     private bool is_closing = false;
 
+    // The mail that is being replied to

Nit: This should probably read "replied to or forwarded"

@@ +386,2 @@
     public ComposerWidget(Geary.Account account,
+                          Geary.Email? referred,

This param duplicates the the referred param for the ComposerWidget::load() method, further down in the class. Since these should always refer to the same object, it would be good to remove the dup param from load() and just refer to the instance property in that method.

@@ +542,3 @@
     }
 
+    public ComposerWidget.from_mailto(Geary.Account account, Geary.Email? referred,

This ctor only gets used for external and internal mailto URI links, so unless there some way we could work out what the referrnt is in those cases, the referred param should be removed here and just set to null when calling the default ctor.
Comment 20 Michael Catanzaro 2018-01-29 01:52:31 UTC
(In reply to Michael Gratton from comment #19)
> This param duplicates the the referred param for the ComposerWidget::load()
> method, further down in the class. Since these should always refer to the
> same object, it would be good to remove the dup param from load() and just
> refer to the instance property in that method.

I don't know; that implies removing the call to email_stores.get() at the bottom of GearyController.create_compose_widget_async(), since the retrieved Geary.Email would no longer be needed to be passed to ComposerWidget.load(). I don't know if it's important or not. Is it needed to "update" the email somehow? It's easy to change, I'm just not sure whether it's right or not.
Comment 21 Michael Gratton 2018-06-26 04:47:35 UTC
Bump tickets to 0.14 that aren't going to make 0.13.
Comment 22 Michael Catanzaro 2018-06-26 15:12:02 UTC
This is already solved.

The last patch never made it in, but I don't have time for it anymore unfortunately, and it's really a separate issue.