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)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
mnyromyr
:
review+
bugzilla
:
superreview+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•17 years ago
|
||
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 2•17 years ago
|
||
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?
Assignee | ||
Comment 3•17 years ago
|
||
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)
Updated•17 years ago
|
OS: Windows XP → All
Hardware: PC → All
Assignee | ||
Comment 4•17 years ago
|
||
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
Assignee | ||
Comment 5•17 years ago
|
||
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 6•17 years ago
|
||
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+
Assignee | ||
Comment 7•17 years ago
|
||
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 8•17 years ago
|
||
Comment on attachment 280446 [details] [diff] [review]
Patch v3 with changes from comment #6
Landed on trunk.
Attachment #280446 -
Flags: review?(mnyromyr) → review+
Comment 9•17 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•