1691253 - Add From: field to ComposeDetails, was: Enhance {get|set}ComposeDetails() to provide access to more fields in nsIMsgCompFields

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[Addons]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.150 Safari/537.36 Edg/88.0.705.63

Steps to reproduce:

{get|set}ComposeDetails() only provides limited access to nsIMsgCompFields, see:
https://thunderbird-webextensions.readthedocs.io/en/78/compose.html#composedetails
Looking for the nsIMsgCompFields.from via the id that's provided is currently cumbersome, besides, the From can be edited and then doesn't reflect the details stored in the identity any more.

Also desirable is access to nsIMsgCompFields.priority and maybe others.

Comment 1

[Addons]

See here for a cheap experiment:
https://github.com/pgpgeek/thunderbird-addon-eml-editor/pull/5/commits/b90ca8a6d3c1f56af5d8c86cf6486c74a0f8620f#diff-ff5b2da53ab3d00dc61859c57f8b62e56b00c488407450629b74ebef129c66ef

Comment 2

[Mark Banner (:standard8)]

I'm not sure priority is worth supporting - is it really used these days? The Thunderbird UI to set it is hidden away, there's a column that is not displayed by default, and as far as I can tell, Gmail doesn't support it at all.

I think it should be a question for the Thunderbird team of it is still worth supporting, if it is, then it could be added here, otherwise we should probably just encourage its removal.

Comment 3

[dreadnaut]

Adding to the case for this to be implemented: the From address can be different from the identity default address if the user has enabled the account option "Reply from this identity when delivery headers match". At the moment there is no way for an add-on to tell that the address does not match the identity.

For more details, see: https://github.com/dreadnaut/bordercolors-d/issues/21

Comment 4

[Frank Steiner]

There's also a quite interesting use-case that I'd like to write an extension for: I don't know how that feature is called but the mail server at our university accepts all "[e-mail]" as valid email address if "[e-mail]" is one. Everyone at our chair is really enjoying that by e.g. using a seperate emails like blabla-amazon@, blabla-ebay@ etc. for every online shop. This makes it very easy to filter certain emails to /dev/null if some shop sells your email to spammers.

I've about 300 of these -whatever@ emails, and whenever I get a mail to one of these addresses, I'd like to use it as From-email when replying. But none of these is in an account in my TB, only [e-mail] is. An addon could easily check for To=blabla-whatever@
if blabla@ is the email of one of my identities and then set blabla-whatever as From: when I reply.

So being able to set the From address even if it doesn't match any identity would be great.

Comment 5

[John Bieling]

I concentrate on the from field. For the priority I would first check if we keep that at all.

Do you have any code for me to re-use to set the from field without going through the UI?

Comment 6

[Addons]

Sorry, I don't understand the question "going through the UI". I was helping the author of
https://addons.thunderbird.net/en-GB/thunderbird/addon/eml-editor/

In the last version we ripped out the setComposeDetails() call and replaced it with an experiment, but in 1.7 the call is still there and you can add a from field:
https://github.com/pgpgeek/thunderbird-addon-eml-editor/pull/6/commits/84bdd063a894504da95764d9d6a5cfa38104205a#diff-f95210083e34617d272c53da44c42bf560e1c184dc20fcd3d2ff8a3e6c8d150dL205

Comment 7

[John Bieling]

Sorry, I don't understand the question "going through the UI".

Using Document.getElementById (or Document.querySelector) to find the from field in the UI and setting a custom from value by changing the content of that UI field.

but in 1.7 the call is still there and you can add a from field

Does not work for me. Saving an email with arbitrary from and importing it via that add-on falls back to my identity. But that is fine, I will find a way.

Comment 8

[Addons]

Hmm, there seem to be more misunderstandings here. I thought you were looking for an add-on as a test frame to exercise the API once implemented. V1.7 of that add-on uses both {get|set}ComposeDetails() and would be suitable for testing. On an unrelated issue, I noticed that even the experiment with its OpenComposeWindow() call doesn't manage to restore an edited From field. It works fine to restore the identity.

Comment 9

[dreadnaut]

(In reply to Frank Steiner from comment #4)

I've about 300 of these -whatever@ emails, and whenever I get a mail to one of these addresses, I'd like to use it as From-email when replying. But none of these is in an account in my TB, only [e-mail] is. An addon could easily check for To=blabla-whatever@
if blabla@ is the email of one of my identities and then set blabla-whatever as From: when I reply.

That sounds like the option "Reply from this identity when delivery headers match" in the account settings. What you need might already be possible in vanilla Thunderbird.

Comment 10

[Frank Steiner]

(In reply to dreadnaut from comment #9)

(In reply to Frank Steiner from comment #4)

I've about 300 of these -whatever@ emails, and whenever I get a mail to one of these addresses, I'd like to use it as From-email when replying. But none of these is in an account in my TB, only [e-mail] is. An addon could easily check for To=blabla-whatever@
if blabla@ is the email of one of my identities and then set blabla-whatever as From: when I reply.

That sounds like the option "Reply from this identity when delivery headers match" in the account settings. What you need might already be possible in vanilla Thunderbird.

Unfortunately not, because all the blabla-whatever@ mails are nowhere in TBs config, only blabla@ is defined as identity. But as all blabla-whatever@ are valid addresses I'd like to reply with blabla-whatever@ in from: whenever the mail reached me with blabla-whatever in to: (and blabla@ is one of my identities). Currently I copy the To: address and set it manually in the From: field.

Comment 11

[dreadnaut]

Mmmh, it sounds exactly like the option I mentioned: instead of creating many identities, it allows you to create a "catch-all" matching a pattern, such as blabla-*@domain.com. Give it a try, you find it at the bottom of the account Settings.

This is off-topic though, so I'll let it be now :)

Comment 12

[Frank Steiner]

(In reply to dreadnaut from comment #11)

instead of creating many identities, it allows you to create a "catch-all" matching a pattern, such as blabla-*@domain.com.

You're right, but I don't want to use a predefined From: address from an identity. I want to use the To: address as From: address when replying if the To: matched a pattern. Even if this email address had not been defined anywhere in TB (only the "base" [e-mail]). Currently I can reply with a given identity when the pattern matches, but not with a dynamically created, arbitrary From: address (expect setting it manually). That's what the extension discussed here would allow.

Comment 13

[Magnus Melin [:mkmelin]]

That is what it that option does, it keeps the To address when replying.

Comment 14

[Frank Steiner]

Hmm, it doesn't work here...
I have checked the option in my main account which has primary identity [e-mail]. I've also defined an additional identity [e-mail] for that account.
When I receive an email to [e-mail] and reply, the From: is set to [e-mail]. But when I receive a mail to [e-mail] (with [e-mail] nowhere defined in any identity in TB) the From: is set to [e-mail] and not to [e-mail]. So it only keeps the To address if it can be found in some identity.
This is with TB 78. Maybe it works different with latest dev version?

Comment 15

[Magnus Melin [:mkmelin]]

And the field is set to *@domain.com?

Comment 16

[Frank Steiner]

Ah, no! It was set to blabla-*@domain.com and that doesn't work. It works only with *@domain.com, not even with [e-mail] for mails received with "To: [e-mail]".

The "@domain.com" setting is dangerous, however, because I will reply with my colleagues From-address if the mail was sent to his address in To: and me only via a list ([e-mail]) in CC. So I would need to match sth. like [e-mail] to make sure only certain mails are kept.

Comment 17

[Addons]

Is this discussion relevant to this bug? The bug is about enhancing a MailExtension API.

Comment 18

[John Bieling]

Attachment: bug1691253_add_from_to_ComposeDetails.patch

We have two scenarios where from could be set:

• by calling setComposeDetails
• directly in our beginNew, beginReply and beginForward functions

We need to mimic the behavior of this code piece:
https://hg.mozilla.org/comm-central/file/tip/mail/components/compose/content/MsgComposeCommands.js#l3621

I do not think it is reasonable to actually try to trigger that code, as it only works for one of the two code paths used by our begin* functions (some are calling OpenComposeWindowWithParams [1], while others call OpenComposeWindow without parameters and afterwards setComposeDetails [2]). And for setComposeDetails we need our own version anyhow.

So I think the most simple approach is to update setComposeDetails and manually invoke the from-setter where needed.

However, for some reason this does not work for beginForward(id, "forwardInline", details). Even though I run the from-setter after the window has loaded (using the compose-editor-ready event). Something is resetting the from field to the identity and I was not able to find the cause for that today. Any idea?

Is this going into the right direction?

[1] https://hg.mozilla.org/comm-central/file/tip/mail/components/extensions/parent/ext-compose.js#l247
[2] https://hg.mozilla.org/comm-central/file/tip/mail/components/extensions/parent/ext-compose.js#l158

Comment 19

[Addons]

Comment on attachment 9204864 [details] [diff] [review]
bug1691253_add_from_to_ComposeDetails.patch

Review of attachment 9204864 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/extensions/parent/ext-compose.js
@@ +330,5 @@
> +    // Silently take the first parsed from entry.
> +    let from = (await parseComposeRecipientToArray(details.from)).shift();
> +    if (from) {
> +      // Manually update from if given, using the same approach used in
> +      // https://hg.mozilla.org/comm-central/file/tip/mail/components/compose/content/MsgComposeCommands.js#l3621

Sorry for the drive-by comment. Please use permalinks since the source code moves fast. Any reason not to use searchfox links? Or is this for illustration only to be removed in the final version?

Comment 20

[John Bieling]

I was told to use hg as that is the source, but I did forget to generate a permalink grr.

As the code is so complicated I tend to add comments so I will be able to understand where and why I did something.

Comment 21

[Addons]

Well, in BMO no one uses HG links, they all use Searchfox permalinks (and used to use DXR permalinks). Usually we reference other code by function name, like: "copied from foo() in bar.cpp". But that's quite infrequent. Typically people will just look for the code snippet in Searchfox.

Comment 22

[Addons]

Comment on attachment 9204864 [details] [diff] [review]
bug1691253_add_from_to_ComposeDetails.patch

Review of attachment 9204864 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/extensions/parent/ext-compose.js
@@ +331,5 @@
> +    let from = (await parseComposeRecipientToArray(details.from)).shift();
> +    if (from) {
> +      // Manually update from if given, using the same approach used in
> +      // https://hg.mozilla.org/comm-central/file/tip/mail/components/compose/content/MsgComposeCommands.js#l3621
> +      composeWindow.MakeFromFieldEditable(true);

Sorry, a bit more substance this time: I don't think it's necessary to make this editable if the from matches an identity.

Comment 23

[Geoff Lankow (:darktrojan) [OOO, back 21 July]]

Comment on attachment 9204864 [details] [diff] [review]
bug1691253_add_from_to_ComposeDetails.patch

Seems reasonable but a little overcomplicated for my liking. I think I would add another argument to parseComposeRecipientList that made it return the first usable address it found, then pass [details.from] to it. That makeFromDisplayAddress could return multiple addresses is annoying in this case.

I think Jörg is right, if the from address matches an identity we should use that identity and not make the field editable unless the identity is also specified. (But what if the address matches but not the name? Hmm, not sure.) And you should make sure the identity is set before the from address or the from address could be wiped out.

Comment 24

[John Bieling]

Attachment: bug1691253_add_from_to_ComposeDetails_v2.patch

Patch had an error.

Comment 25

[John Bieling]

Attachment: bug1691253_add_from_to_ComposeDetails_v3.patch

Setting the from field now always works.

I added the optional parameter for parseComposeRecipientList.

Something I find odd is, that a non-array "list" parameter value for parseComposeRecipientList(list) is being passed thru without checks and that it cannot be an object with a contact ID (limited in the scheme and of course not evaluated). Both could be fixed by returning parseComposeRecipientList([list]) instead of just list.

The from field is not an identity selector. It is intended to override the visible sender address for a given identity. So only if the from email matches the current identity email, the field is not made editable.

To be honest, I would like to make that a simple from != identity.value check, because even changing the name requires to make the field editable for the ordinary user.

Comment 26

[Addons]

Sorry, I cannot detect a question here. Is it a request for feedback? What I find "surprising" is that you wrap a "give me the first result" bool parameter into an object. Are you planning for more options? As for the question when to make the field editable: I'd make it editable when it differs from the identity, either by e-mail address or by display name since the user would have had to make it editable to change either of those. Otherwise it looks fine.

Comment 27

[John Bieling]

Thanks for your feedback. I went for the options object, to improve code readability, as you do not have to look up the meaning of the second parameter. But if that is uncommon here, I revert to a simple boolean.

Comment 28

[John Bieling]

Attachment: bug1691253_add_from_to_ComposeDetails_v4.patch

Ready for review.

Comment 29

[Geoff Lankow (:darktrojan) [OOO, back 21 July]]

Comment on attachment 9206306 [details] [diff] [review]
bug1691253_add_from_to_ComposeDetails_v4.patch

Review of attachment 9206306 [details] [diff] [review]:
-----------------------------------------------------------------

A few more cases to handle.

::: mail/components/extensions/parent/ext-compose.js
@@ +66,3 @@
>    }
> +
> +  return parseComposeRecipientList([list], returnOnlyFirst);

Just convert the single item to a list at the top:

if (!Array.isArray(list)) {
  list = [list];
}

@@ +327,5 @@
>    return details;
>  }
>  
> +async function setFromField(composeWindow, details, extension) {
> +  if (details?.from) {

I think you should throw if from is an empty string. It's a bad value and extensions shouldn't be sending it.

Also consider returning early instead of making if blocks containing the rest of the function. Particularly in cases like this first one where you're checking whether the function should do anything at all.

@@ +329,5 @@
>  
> +async function setFromField(composeWindow, details, extension) {
> +  if (details?.from) {
> +    // Take the first parsed from entry by setting second parameter returnOnlyFirst = true.
> +    let from = await parseComposeRecipientList(details.from, true);

Should we be checking this is a usable address? It could be any string which I guess is checked at send time, but we could do it early to let the extension know. You might also be passed a mailing list, which isn't going to end well.

Comment 30

[John Bieling]

Attachment: bug1691253_add_from_to_ComposeDetails_v5.patch

Bad patch, forgot to update the description.

Comment 31

[John Bieling]

Attachment: bug1691253_add_from_to_ComposeDetails_v6.patch

There are so many ways to do this wrong. Hope this is ok.

Comment 32

[John Bieling]

Attachment: bug1691253_add_from_to_ComposeDetails_v7.patch

Updated patch to use the same isValidAddress() function as other parts of c-c.

Comment 33

[Geoff Lankow (:darktrojan) [OOO, back 21 July]]

Comment on attachment 9206676 [details] [diff] [review]
bug1691253_add_from_to_ComposeDetails_v7.patch

Review of attachment 9206676 [details] [diff] [review]:
-----------------------------------------------------------------

Better. The error messages could do with some improvement, I've given some suggestions. You should give the value and where it was input, where practical (it's not very practical in some of these cases), so that developers' debugging time is minimised.

::: mail/components/extensions/parent/ext-compose.js
@@ +42,5 @@
> +      );
> +
> +      for (let ao of addressObjects) {
> +        if (requireSingleValidEmail && !isValidAddress(ao.email)) {
> +          throw new ExtensionError("Invalid address.");

`Invalid address: ${ao.email}`

@@ +60,5 @@
> +      if (
> +        requireSingleValidEmail &&
> +        !isValidAddress(contactNode.item.primaryEmail)
> +      ) {
> +        throw new ExtensionError("Invalid contact.");

Something like `Contact does not have a valid email address: ${recipient.id}`?

@@ +357,5 @@
> +
> +  let from = await parseComposeRecipientList(details.from, true);
> +  if (!from) {
> +    // The from address cannot be cleared.
> +    throw new ExtensionError("Exactly one address required.");

Exactly one address required in details.from?
From address must not be set to an empty string?

::: mail/components/extensions/test/browser/browser_ext_compose_details.js
@@ +78,5 @@
>          }),
> +        empty: await browser.contacts.create(addressBook, {
> +          DisplayName: "Sherlock Holmes",
> +          PrimaryEmail: "",
> +        }),

Call this contact something else to avoid confusion. "Jim Moriarty" will do nicely.

Comment 34

[John Bieling]

Attachment: bug1691253_add_from_to_ComposeDetails_v8.patch

Took most of your suggestions. The only thing I did differently: I did not make parseComposeRecipientList() throw an error with a specific reference to details.from.

To get that information into the error message and keeping parseComposeRecipientList() generic, I made setFromField() re-throw all errors it got from parseComposeRecipientList() with a ComposeDetails.from: prefix.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b498cb94fd30
Add from field to ComposeDetails. r=darktrojan

Comment 36

[dreadnaut]

John, Geoff, Addons — thank you for your work!

I'll try to update my add-on as soon as possible. Do you know in which beta version this change will land?

Comment 37

[John Bieling]

Beta 88. I try to uplift to ESR.

Comment 38

[Addons]

Note that only a subset of what was asked for was implemented: The enhancement request was to be able to control all details of the compose window, well, somewhat badly phrased as "access to more fields in nsIMsgCompFields". There are more fields like priority, returnReceipt, DSN, deliveryFormat. etc.

Comment 39

[John Bieling]

Comment on attachment 9206938 [details] [diff] [review]
bug1691253_add_from_to_ComposeDetails_v8.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
I am evaluating

Comment 40

[dreadnaut]

status-thunderbird_esr78: affected → wontfix

Just to make sure I read this correctly: the fix will be in TB 87.x / 88, but it won't be backported to 78.x?

Comment 41

[John Bieling]

Yes, because my resources are limited and I want to get as much done for TB91 as possible.

Comment 42

[dreadnaut]

Understood, I'll version my add-on accordingly. Thank you for your work!