Closed Bug 1287268 Opened 4 years ago Closed 3 years ago

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

Categories

(MailNews Core :: MIME, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 52.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Attached patch Proposed solution (v1a). (obsolete) — Splinter Review
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?
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8771673 - Flags: review?(acelists)
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.
Attachment #8771673 - Flags: review?(acelists) → feedback+
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.
Attachment #8771673 - Flags: review?(mkmelin+mozilla)
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.
OS: Unspecified → All
Hardware: Unspecified → All
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
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.
(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.
Right, sorry. But since you need it to be writable, just make the attribute writable.
Attached patch We don't want this, do we? (obsolete) — Splinter Review
Readonly without extra method would look like this:

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

Not pretty.
I don't think so. Since code will have to set it really should be a writable attribute.
Depends on: 1169184
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.
Attachment #8771673 - Attachment is obsolete: true
Attachment #8772141 - Attachment is obsolete: true
Attachment #8771673 - Flags: review?(mkmelin+mozilla)
Attachment #8772331 - Flags: feedback?(mkmelin+mozilla)
Attachment #8772331 - Flags: feedback?(mkmelin+mozilla) → feedback+
Magnus, can you please clarify: You gave f+ but you still want a different solution based on identities, right?
Summary: Create facility to tell whether a composition is based on an own draft or template, restore edited From header in this case → 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
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.
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.
Attachment #8772394 - Flags: review?(mkmelin+mozilla)
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.
Attachment #8772394 - Flags: feedback+
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?
(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
(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.
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.
(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.
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 ;-)
(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.
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.
(In reply to :aceman from comment #24)
> We should check that on an IMAP account.

FCC: imap://info%40hostalpancheta.es@mail.hostalpancheta.es/INBOX/Sent
X-Identity-Key: id2
X-Account-Key: account29
From: Hostal Pancheta <info@hostalpancheta.es>
Message-ID: <ed07eb68-9956-b607-8272-b723685a31a5@hostalpancheta.es>
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

;-)
I stand corrected :)
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.
Attachment #8772331 - Flags: review?(mkmelin+mozilla)
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.)
Attachment #8772394 - Flags: review?(mkmelin+mozilla) → review+
Attachment #8772331 - Attachment is obsolete: true
Attachment #8772331 - Flags: review?(mkmelin+mozilla)
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?
Attachment #8772394 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
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 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
(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?
Flags: needinfo?(mkmelin+mozilla)
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
Flags: needinfo?(mkmelin+mozilla)
creatorIdentityKey works for me
Flags: needinfo?(mkmelin+mozilla)
OK, creatorIdentityKey it is.
Attachment #8794624 - Attachment is obsolete: true
Attachment #8795009 - Flags: review+
https://hg.mozilla.org/comm-central/rev/1b6364120f38
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
You need to log in before you can comment on or make changes to this bug.