1287268 - Create facility to tell whether a composition is based on an own draft or template, restore edited From header and spellcheck language in this case

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Jorg K (CEST = GMT+2)]

This is a continuation from bug 1254666. It would be nice to maintain the edited From header if an "own" draft or template is edited, either as "edit draft" or "edit as new".

We should make the fact that "X-Mozilla-Draft-Info" is present available.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a).

OK, here is a little tweak to detect our own draft or template.

I think it's useful for maintaining the edited From header and it will be useful to maintain the Content-Language (bug 1169184).

What do you think?

Comment 2

[:aceman]

Comment on attachment 8771673 [details] [diff] [review]
Proposed solution (v1a).

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

This looks fine to me (assuming you fix the 1 comment), but I'd forward this to Magnus, maybe he knows if this info isn't already available somewhere.

::: mailnews/compose/public/nsIMsgCompFields.idl
@@ +53,5 @@
>    /// Delivery format for the mail being composed
>    /// (auto = 4, text = 1, html = 2, text and html = 3).
>    attribute long deliveryFormat;
> +  readonly attribute boolean isOwnDraftOrTemplate;
> +  void setOwnDraftOrTemplate();

Why is the attribute readonly and then you have the setOwnDraftOrTemplate function?

If you need to set it from external code (outside nsMsgCompFields.*), you make the attribute readwrite and then the idl parser generates also the SetIsOwnDraftOrTemplate function declaration. That will also allow us to set the value to false in case we need that in the future.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8771673 [details] [diff] [review]
Proposed solution (v1a).

(In reply to :aceman from comment #2)
> If you need to set it from external code (outside nsMsgCompFields.*), you
> make the attribute readwrite and then the idl parser generates also the
> SetIsOwnDraftOrTemplate function declaration. That will also allow us to set
> the value to false in case we need that in the future.
No. If the attribute is writeable, you can set it from JS code. That is not desired. What I've done is correct. Take a look at:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/public/nsIMsgCompFields.idl#64
We have
readonly attribute nsISimpleEnumerator attachments;
and some functions that manipulate the attachments and indirectly set/change the value of the attachments attribute.
void addAttachment(in nsIMsgAttachment attachment);
void removeAttachment(in nsIMsgAttachment attachment);
void removeAttachments();

The only point to consider here is whether to have
+  void setOwnDraftOrTemplate();
or
+  void setOwnDraftOrTemplate(in boolean aValue);
so you can unset it.

Comment 4

[:aceman]

Why do we not want to set it JS ? Yes, today you do not need it, but in the future? At least you could name the function so that it is the same name as it will get if we once make the attribute writable.

Comment 5

[Jorg K (CEST = GMT+2)]

Since the system decides whether it's an "own" draft or template. By definition that is readonly, like hasRecipients:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/public/nsIMsgCompFields.idl#64

Comment 6

[Magnus Melin [:mkmelin]]

I agree with aceman, it doesn't make sense to have the method too, but that doesn't seem needed anyway. A readonly attribute should be fine, that can be made readable if someone needs it later.

It could be useful to instead of having it as a boolean, make it something like "ownerIdentity" to actually get which identity it is. (And null if not own). For instance for the editable From, you can get the wrong identity for sending even if the email address would be correct, leading to wrong settings, saved copy in wrong place and other problems.

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #6)
> I agree with aceman, it doesn't make sense to have the method too, but that
> doesn't seem needed anyway.
How do I set the value then? nsIMsgCompFields.h is generated from nsIMsgCompFields.idl and the call to set the value is in mimedrft.cpp where I have nsIMsgCompFields * compFields.
You don't want me to cast that to nsMsgCompFields without the "I" the dirty way?. What am I missing?

> It could be useful to instead of having it as a boolean, make it something
> like "ownerIdentity" to actually get which identity it is. (And null if not
> own). For instance for the editable From, you can get the wrong identity for
> sending even if the email address would be correct, leading to wrong
> settings, saved copy in wrong place and other problems.
I'll look into that.

Comment 8

[Magnus Melin [:mkmelin]]

Right, sorry. But since you need it to be writable, just make the attribute writable.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: We don't want this, do we?

Readonly without extra method would look like this:

     if (draftInfo && fields && !forward_inline)
     {
+      nsMsgCompFields* _fields = static_cast<nsMsgCompFields*>(fields.get());
+      _fields->SetOwnDraftOrTemplate();
+

Not pretty.

Comment 10

[Magnus Melin [:mkmelin]]

I don't think so. Since code will have to set it really should be a writable attribute.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: Possible solution (v2) based in draft info.

OK, this is the previous solution based on draft info with the attribute isOwnDraftOrTemplate made writeable and the method names changed accordingly.

Magnus said in comment #6 that he prefers something based on identity.

Well, the draft info is processed here:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1253

and just below, the entity info is also read from the draft:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1302

So I'll see what I can do.

Comment 12

[Magnus Melin [:mkmelin]]

xref bug 394216

Comment 13

[Jorg K (CEST = GMT+2)]

Magnus, can you please clarify: You gave f+ but you still want a different solution based on identities, right?

Comment 14

[Jorg K (CEST = GMT+2)]

Having looked at bug 394216 I can see that it would be more useful to make the X-Identity-Key available as a compose field. That can be used here and in other contexts, right?

I'll submit a patch in the next 24-48 hours.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Possible solution (v3) based on X-Identity-Key.

Here is the whole thing again, but this time based on the identity.

This should also be very useful for bug 394216 which should require one line to fix:
if (params.composeFields.composeIdentity)
  params.identity = params.composeFields.composeIdentity;
at MsgComposeCommands.js, line 2362 +- 5.

Comment 16

[:aceman]

Comment on attachment 8772394 [details] [diff] [review]
Possible solution (v3) based on X-Identity-Key.

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

This looks interesting. Just some drive-by comments.

::: mailnews/compose/public/nsIMsgCompFields.idl
@@ +53,5 @@
>    /// Delivery format for the mail being composed
>    /// (auto = 4, text = 1, html = 2, text and html = 3).
>    attribute long deliveryFormat;
>    attribute string contentLanguage;
> +  attribute string composeIdentity;

Please add a comment that this attribute contains the key (with this name someone could expect it to be an identity object) and that it is expected to only contain a value if the message is in state of composition (draft or template). Then the other changes will make sense to casual readers (like, how does .composeIdentity relate to content language...)

::: mailnews/mime/src/mimedrft.cpp
@@ +1315,5 @@
>          {
>              nsCOMPtr< nsIMsgIdentity > overrulingIdentity;
>              rv = accountManager->GetIdentity( nsDependentCString(identityKey), getter_AddRefs( overrulingIdentity ) );
>  
> +            if ( NS_SUCCEEDED(rv) && overrulingIdentity ) {

Please fix the messy spaces around brackets while here.

Comment 17

[:aceman]

On the other hand, I do not see how this would help bug 394216 . You only read/save the identity key. And that bug is exactly about the fact they are reading in the same draft (with one key) on multiple machines/TB install that have accounts/identities set up non-identically. So the same key means different identities in those TB installs.

Also, why did you actually have to add "X-Identity-Key" to the headers array? We already write the header out somewhere. Is that using some raw writing without going through the CompFields object?

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #16)
Thanks for the comments.

> Please add a comment that this attribute contains the key (with this name
> someone could expect it to be an identity object) and that it is expected to
> only contain a value if the message is in state of composition (draft or
> template). Then the other changes will make sense to casual readers (like,
> how does .composeIdentity relate to content language...)
I'll add a comment. This could also be named composeIdentityKey. Magnus, do you have a preference?

(In reply to :aceman from comment #17)
> On the other hand, I do not see how this would help bug 394216.
Sorry, maybe it doesn't, I only looked at the other bug briefly. However, it appeared useful to have the identity whose draft/template we're processing readily available to JS.

> Also, why did you actually have to add "X-Identity-Key" to the headers
> array? We already write the header out somewhere.
That how strings are stored in the nsIMsgCompFields object via Get/SetAsciiHeader. Don't confuse the CompFields (headers) with the mail headers.
https://dxr.mozilla.org/comm-central/source/mailnews/mime/public/nsMailHeaders.h#17
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompFields.h#45
Or am I missing something? I'm only slowly getting acquainted with this code ;-)

> Is that using some raw
> writing without going through the CompFields object?
Yes, read here:
https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1302
written here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#4500

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #17)
> On the other hand, I do not see how this would help bug 394216.
Hmm, looking at bug 394216 comment #3 Magnus said (quote):
  Check the X-Identity-Key: header of the drafts. That's used to see which identity to use.
I can't see where, can you explain/enlighten me. I'd say that this would be good enough.

Comment 20

[Magnus Melin [:mkmelin]]

Well it says something like 
X-Identity-Key: id2

... but you'd have to check if id2 address is correct. If not, there's a mismatch and we will (now) select the wrong identity.

Comment 21

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #19)
> (In reply to :aceman from comment #17)
>   Check the X-Identity-Key: header of the drafts. That's used to see which
> identity to use.
> I can't see where, can you explain/enlighten me. I'd say that this would be
> good enough.

Yes, but the bug is that we read this key, find the identity and it is incorrect. Because the key was put there by other TB installion where the same key was a different identity.

Comment 22

[Jorg K (CEST = GMT+2)]

Sorry guys, I finally had the time to read through bug 394216. Indeed, the new composeIdentity(Key) attribute won't help much in sorting out that bug. In fact, in bug 394216 they even call for the removal of the X-Identity-Key header.

Anyway, the purpose of this bug here is to create a facility to detect own drafts and templates to
1) restore the edited From address
2) restore the stored language.

That's basically two hunks:
-  if (gComposeType == nsIMsgCompType.Draft && params.composeFields.from)
+  if ("own draft/template detected" && params.composeFields.from)
and
-  if (gComposeType == nsIMsgCompType.Draft && gMsgCompose.compFields.contentLanguage) {
+  if ("own draft/template detected" && gMsgCompose.compFields.contentLanguage) {

How we detect that, I don't care much about. I made two suggestions, via the X-Mozilla-Draft-Info header or via the X-Identity-Key header.

In fact, now that my Content-Language has landed without the facility, I'm basically happy. But it would be good to extend the restoring for more known benign cases.

I'll let the guys with more experience, Magnus and Aceman, lead the way here ;-)

Comment 23

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #22)
> How we detect that, I don't care much about. I made two suggestions, via the
> X-Mozilla-Draft-Info header or via the X-Identity-Key header.

X-Identity-Key is POP only, and only really of use for global inbox.

Comment 24

[:aceman]

Surely? We have identities in IMAP too. In the other bug people claim the problematic drafts are in IMAP shared folders and think the identity key is stored in the draft and is the culprit.
We should check that on an IMAP account.

Comment 25

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #24)
> We should check that on an IMAP account.

FCC: imap://[e-mail]/INBOX/Sent
X-Identity-Key: id2
X-Account-Key: account29
From: Hostal Pancheta <[e-mail]>
Message-ID: <[e-mail]>
Date: Wed, 20 Jul 2016 15:26:11 +0200
X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=1; DSN=1; uuencode=0;
 attachmentreminder=0; deliveryformat=4
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101
 Thunderbird/50.0a1
MIME-Version: 1.0
Content-Type: text/html; charset=windows-1252
Content-Language: en-GB
Content-Transfer-Encoding: 7bit

;-)

Comment 26

[Magnus Melin [:mkmelin]]

I stand corrected :)

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8772331 [details] [diff] [review]
Possible solution (v2) based in draft info.

This bug has waited a long time for review. Thinking about it, basing this on the "draft info" header is the simpler solution and I therefore prefer it.

Note that this should land on TB 52 the latest since afterwards we can't do IDL changes any more.

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 8772394 [details] [diff] [review]
Possible solution (v3) based on X-Identity-Key.

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

I like this better, but could you change the name to say, identityKey instead? Like aceman said otherwise it sounds like an object. (And the compose part is clear as it's in compFields already.)

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: Possible solution (v3b) based on X-Identity-Key.

I've changed it to identityKey and added the comment Aceman asked for.

The question is: Do you really prefer this solution, since personally, I don't like it at all. Here's why.

There is an existing X-Identity-Key header (HEADER_X_MOZILLA_IDENTITY_KEY) and if you click here:
https://dxr.mozilla.org/comm-central/search?q=HEADER_X_MOZILLA_IDENTITY_KEY&redirect=false
you can see how this is written into the draft. It's not written from any compose field like many other headers but from elsewhere.

I find it very confusing to create a compose field now called identityKey which actually does not contain the key of the identity in most cases. It is only populated when a draft or template are edited (as new, in the case of the template).

The 500% more honest solution is to provide the fact that I'm editing a draft or template based on the presence of the "draft info" header (or perhaps another header, but still call the compose field isOwnDraftOrTemplate).

Once again, a compose field identityKey is highly misleading. Can you please reconsider?

Comment 30

[Jorg K (CEST = GMT+2)]

If you create a template using identity 1 and then use this template as identity 2, identityKey will be id1 which has nothing to do with the identity that is used while composing.

The honest name would be:
indentityKeyOfIdentityWhichCreatedThisDraftOrTemplate.

But then again, we don't care who created it, we only care that the header is present to tell us that we're editing our own draft or template. That's why this solution is really no good.

Comment 31

[Magnus Melin [:mkmelin]]

Comment on attachment 8794624 [details] [diff] [review]
Possible solution (v3b) based on X-Identity-Key.

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

::: mailnews/compose/public/nsIMsgCompFields.idl
@@ +53,5 @@
>    /// Delivery format for the mail being composed
>    /// (auto = 4, text = 1, html = 2, text and html = 3).
>    attribute long deliveryFormat;
>    attribute string contentLanguage;
> +  /// this is populated with the key of the identity which created the draft or template.

nit: This

Comment 32

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #30)
> But then again, we don't care who created it, we only care that the header
> is present to tell us that we're editing our own draft or template.

But we do care. Not in this bug, but for instance bug 394216. How do you propose to solve that without knowing what the identity was?

Comment 33

[Jorg K (CEST = GMT+2)]

I got all confused with bug 394216 and didn't look at it any further :-(

OK, you want a facility to dig out the identity key that created the draft from the draft or template. Fair enough. Let's give the attribute a good name and then I'll land it.

As detailed in comment #29 and comment #30 I still think that identityKey is confusing and indentityKeyOfIdentityWhichCreatedThisDraftOrTemplate was meant as a joke. So do we have a better solution?

previousIdentityKey
draftIdentityKey
draftOrTemplateIdentityKey
originalIdentityKey
composeIdentityKey
creatorIdentityKey

Comment 34

[Magnus Melin [:mkmelin]]

creatorIdentityKey works for me

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: Possible solution (v3c) based on X-Identity-Key.

OK, creatorIdentityKey it is.

Comment 36

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/1b6364120f38