Closed Bug 393212 Opened 17 years ago Closed 17 years ago

Autosave leaves ghost messages in drafts on cancelling compose

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: bugzilla)

References

Details

Attachments

(1 file, 4 obsolete files)

This will be a port of Thunderbird bug 307046 (including patches for bug 369893 and bug 379859).
Attached patch port of TB fixes (obsolete) (deleted) — Splinter Review
This patch adds the functionality from bug 307046 with the regression fixes from bug 369893 and bug 379859.
Attachment #277790 - Flags: superreview?(neil)
Attachment #277790 - Flags: review?(mnyromyr)
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?
Attached patch WIP patch (obsolete) (deleted) — Splinter Review
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.
Attachment #277790 - Attachment is obsolete: true
Attachment #277790 - Flags: superreview?(neil)
Attachment #277790 - Flags: review?(mnyromyr)
OS: Windows XP → All
Hardware: PC → All
Attached patch WIP v2 (obsolete) (deleted) — Splinter Review
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.
Attachment #278234 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) (deleted) — Splinter Review
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();
Attachment #279316 - Attachment is obsolete: true
Attachment #280355 - Flags: superreview?(neil)
Attachment #280355 - Flags: review?(mnyromyr)
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;
Attachment #280355 - Flags: superreview?(neil) → superreview+
This patch uses the code Neil suggested in #c6, carrying over sr+.
Attachment #280355 - Attachment is obsolete: true
Attachment #280446 - Flags: superreview+
Attachment #280446 - Flags: review?(mnyromyr)
Attachment #280355 - Flags: review?(mnyromyr)
Comment on attachment 280446 [details] [diff] [review] Patch v3 with changes from comment #6 Landed on trunk.
Attachment #280446 - Flags: review?(mnyromyr) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 307046
No longer blocks: TB2SM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: