1427617 - Opening and closing a draft with attachments without any changes claims "Message has not been sent or saved in your drafts folder (Drafts)."

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Thomas D. (:thomas8)]

STR

1) open existing draft with attachments
2) close "Write" window without changing anything

Actual

Modal alert: Message has not been sent or saved in your drafts folder (Drafts).
Nagging me for nothing, and lying:
- of course it hasn't been sent, because it's still a "Write" window, and I never clicked "Send", dummy!
- This exact message is already saved in my drafts folder (so just closing it doesn't lose anything and shouldn't nag)

Expected

Don't nag for nothing, if there are no changes, just let me close my draft without asking questions.

Technical background:

I think this was never intended. It's a side-effect of the fact that when we load the message, we use AddAttachments() function, which works but always sets gContentChanged = true whenever attachments are added, hence this bug.
To fix this, this function must not set gContentChanged = true when we're opening the draft.

https://dxr.mozilla.org/comm-central/rev/058aa6495c2db7c13eac1e23c74d2354bf9d6355/mail/components/compose/content/MsgComposeCommands.js#2933
> AddAttachments(gMsgCompose.compFields.attachments);

https://dxr.mozilla.org/comm-central/rev/058aa6495c2db7c13eac1e23c74d2354bf9d6355/mail/components/compose/content/MsgComposeCommands.js#4509-4510
>  if (addedAttachments.length > 0) {
>    gContentChanged = true;

Jörg, Aceman, can we just add a parameter like "fireGContentChanged = true" to the function, then call with fireGContentChanged = false for the opening draft case? Any other more elegant ideas?

Comment 1

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere) from comment #0)
> Jörg, Aceman, can we just add a parameter like "fireGContentChanged = true"
> to the function, then call with fireGContentChanged = false for the opening
> draft case? Any other more elegant ideas?
If I were to answer that, I'd have to investigate it myself ;-(

BTW, Aceman doesn't follow bugs as closely as I do, so NI him.

Comment 2

[:aceman]

Looks like you included the fix for this in bug 1461170 so you can dupe it if you agree.

Comment 3

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #2)
> Looks like you included the fix for this in bug 1461170 so you can dupe it
> if you agree.

Yes, this is a bug in its own right which I fixed, and the patch happened to end up in bug 1461170 for the convenience of reviewers and fast, consolidated landing.

Comment 4

[Thomas D. (:thomas8)]

Strangely when I'm testing this, I'm under the impression that it seems to fail for every other first time, and the existing draft which I tested on seems to be deleted when I click "Discard (changes)", which is wrong (only changes must be discarded, not the entire existing draft). But I am failing to reproduce...

Comment 5

[:aceman]

But next time please do not merge unrelated fixes.
The fix for this bug was relatively small (an argument to the AddAttachments() function) so could be kept here as a separate patch and it would be easy to review.

Comment 6

[Jorg K (CEST = GMT+2)]

Note bug 1330535.

Comment 8

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #5)
> But next time please do not merge unrelated fixes.
> The fix for this bug was relatively small (an argument to the
> AddAttachments() function) so could be kept here as a separate patch and it
> would be easy to review.

Well, the fixes were actually closely related and I was trying to save everyone's time by merging them (creating another small patch eats my time, and landing another small patch also consumes some of Jörg's precious time...).
I actually changed the handling of the same new argument which fixes this bug in bug 1461170, so it becomes a matter of reviewer's taste and working style if he prefers to see the full picture in one patch or incremental changes in several patches.

That said, I obviously agree to the general idea of keeping separate things separate, as incremental changes are typically easier to understand / review, maintain and back out should the need arise.