393212 - Autosave leaves ghost messages in drafts on cancelling compose

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Bruno 'Aqualon' Escherl]

This will be a port of Thunderbird bug 307046 (including patches for bug 369893 and bug 379859).

Comment 1

[Bruno 'Aqualon' Escherl]

Attachment: port of TB fixes

This patch adds the functionality from bug 307046 with the regression fixes from bug 369893 and bug 379859.

Comment 2

[[e-mail]]

Comment on attachment 277790 [details] [diff] [review]
port of TB fixes

>+  gEditingDraft = gMsgCompose.compFields.draftId;
This isn't true when editing a template, right?

>+  gAutoSaveKickedIn = false;
>+  gEditingDraft = true;
>+
>   GenericSendMessage(nsIMsgCompDeliverMode.SaveAsDraft);
Shouldn't these happen after the save, in case that fails?

>         case 0: //Save
>           gCloseWindowAfterSave = true;
>           SaveAsDraft();
>           return false;
Do we need to do this if we've autosaved the current version?

>+    var draftUri = gMsgCompose.compFields.draftId;
Is this the latest autosaved version? [If so then gAutoSaveKickedIn is equivalent to gMsgCompose.compFields.draftId != gEditingDraft]

>+    try {
>+      const MSG_FOLDER_FLAG_DRAFTS = 0x0400;
>+      if (folder.flags & MSG_FOLDER_FLAG_DRAFTS)
Why this check? If it's important, shouldn't it be outside the try/catch block? I know you won't get an exception if the folder is an imap non-drafts folder but it looks odd for your exception handler to be outside this if.

>+      imapFolder.storeImapFlags(8, true, keyArray, 1, null);
What does the 8 mean?

Comment 3

[Bruno 'Aqualon' Escherl]

Attachment: WIP patch

First thanks to Karsten for a lot of help!

I made a few changes, cause gMsgCompose.compFiels.draftId isn't != "" only for drafts but also for all edited and already saved mails (templates, reply to, edit as new,...). gEditingDraft is now only true if we really are editing a draft.

The main remaining issue is, that I don't really understand at the moment why we have to distinguish between IMAP and Local Messages and just can't call folder.deleteMessages(...) in RemoveDraft(). Will have to investigate this further.

Comment 4

[Bruno 'Aqualon' Escherl]

Attachment: WIP v2

Next WIP patch, changes include only calling SaveAsDraft() if there really was a change in the mail and simplifying the code in RemoveDraft().

The main remaining issue is, that folder.flags is 0 in the check if we're editing a draft, when the Drafts folder is on an IMAP account. So at the moment gEditingDraft is 0, hence an edited draft is removed when autosave kicked in and the user chooses Don't save (what in some cases probably means, don't save the changes I made to the draft).

Possible options to solve this are:

1. Restore the check for folder.flags & MSG_FOLDER_FLAG_DRAFTS in RemoveDraft() like in the previous WIP patch. Doing this would never delete drafts from an IMAP account, but that's probably better than loosing autosaved drafts in some cases.

2. Look if folder.name is "Drafts". Probably not the best thing to do.

3. Find out why folder.flags isn't set for the Drafts folder (also applies to the Sent folder but not to Templates and Inbox) and fix that problem.

Approach 3 is probably the best one, but so far I have no idea why that's happening. Any suggestions are welcome.

Comment 5

[Bruno 'Aqualon' Escherl]

Attachment: Patch v2

I found a solution to be really sure we're editing a draft. We have to build the folderURI from the draftId and check the folder.flags if we opened a message from the Drafts folder.

The nested try-catch in RemoveDraft() is necessary to get all errors. The inner try-catch catches errors on IMAP-Servers with no support for GetMessageHeader. The outer try-catch is for possible errors during sRDF.GetResource();

Comment 6

[[e-mail]]

Comment on attachment 280355 [details] [diff] [review]
Patch v2

>         case 0: //Save
>           gCloseWindowAfterSave = true;
>-          SaveAsDraft();
>+          // only save draft if there's a change
>+          if (!gContentChanged && !gMsgCompose.bodyModified)
>+            SaveAsDraft();
>           return false;
This isn't quite right. If I start composing a new message, then let it autosave, then close the window, then the window should close. Try this:
// we can close immediately if we already autosaved the draft
if (!gContentChanged && !gMsgCompose.bodyModified)
  break;
gCloseWindowAfterSave = true;
SaveAsDraft();
return false;

Comment 7

[Bruno 'Aqualon' Escherl]

Attachment: Patch v3 with changes from comment #6

This patch uses the code Neil suggested in #c6, carrying over sr+.

Comment 8

[Karsten Düsterloh]

Comment on attachment 280446 [details] [diff] [review]
Patch v3 with changes from comment #6

Landed on trunk.

Comment 9

[Karsten Düsterloh]

And I don't think that autosave should overwrite edited drafts if the user might want to cancel the edit, so I filed bug 395821.