Draft messages not deleted when Send Later used

VERIFIED FIXED

Status

MailNews Core
Composition
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: Jake Howlett, Assigned: Scott MacGregor)

Tracking

({fixed1.8.1, verified1.8.0.5})

Trunk
fixed1.8.1, verified1.8.0.5
Bug Flags:
blocking1.8.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Thunderbird 1.5 Beta 2 (20051006)

I enabled the auto save option which created draft messages and saved them every two minutes. When I sent the message using the Send Later option the draft message would remain in the folder and I would have to delete it myself.

Reproducible: Always

Steps to Reproduce:
1. Enable Auto Save option and set to a low value like one minute
2. Create a new message and wait for it to be saved to drafts
3. Send message using Send Later option
4. Message will remain in drafts folder

Actual Results:  
Unwanted draft messages

Expected Results:  
No draft messages

Comment 1

12 years ago
Confirmed.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051026 Thunderbird/1.5 ID:2005102604
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All

Comment 2

12 years ago
I'm seeing this on Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a1) Gecko/20051214 MultiZilla/1.8.1.0u SeaMonkey/1.5a as well. Could someone with authority please change this from Thunderbird to Core?

Lewis

Comment 3

12 years ago
-> Core
Component: General → MailNews: Composition
Product: Thunderbird → Core
Version: unspecified → 1.8 Branch

Comment 4

12 years ago
After reviewing source code on this issue, without breaking and modifying all the source, one possible solution would be to modify nsMsgComposeSendListener::OnStopCopy function in nsMsgCompose.cpp as follow :

      // We should only close the window if we are done. Things like templates
      // and drafts aren't done so their windows should stay open
      if ( (mDeliverMode != nsIMsgSend::nsMsgSaveAsDraft) &&
           (mDeliverMode != nsIMsgSend::nsMsgSaveAsTemplate) )
        compose->CloseWindow(PR_TRUE);
      else
      {
        compose->NotifyStateListeners(eSaveInFolderDone,aStatus);
        if (mDeliverMode == nsIMsgSend::nsMsgSaveAsDraft || mDeliverMode == nsIMsgSend::nsMsgSaveAsTemplate)
        {
          // Remove the current draft msg when saving to draft is done.
          compose->SetDeleteDraft(PR_TRUE);
          RemoveCurrentDraftMessage(compose, PR_TRUE);
        }
      }

      // Bug 314009 : https://bugzilla.mozilla.org/show_bug.cgi?id=314009
      if (mDeliverMode == nsIMsgSend::nsMsgQueueForLater)
        {
          compose->SetDeleteDraft(PR_TRUE);
          RemoveCurrentDraftMessage(compose, PR_TRUE);
        }

I've checked this on thunderbird-rc1 source tree. Must check this on the actual trunk to confirm, but AFAIK it should work. Once I setup a trunk source tree synchronised with current HEAD, I will be able to submit a patch.

Let me know your feeling about this possible solution ?

Thanks in advance,

Chris

Comment 5

12 years ago
(In reply to comment #4)

<snip>

> Let me know your feeling about this possible solution ?
> 
> Thanks in advance,
> 
> Chris
> 

From what little I know of the code, this looks like a viable solution, Chris; well done. It would be nice to get another opinion on it, and frankly, I'm surprised that we don't have more subscribers to this bug in general (I just manually zapped about 150 old drafts...most annoying).

Lewis

Comment 6

12 years ago
(In reply to comment #5)
> From what little I know of the code, this looks like a viable solution, Chris;
> well done. It would be nice to get another opinion on it, and frankly, I'm
> surprised that we don't have more subscribers to this bug in general (I just
> manually zapped about 150 old drafts...most annoying).

Thanks for your feedback Lewis.
IMHO, this bug is very annoying when used with auto-save-as-draft feature. I'm surprised too about so few subscribers on this bug. Solving it could improve end-user experience and general feeling of the product.

Concerning the patch, I have trouble to get access to cvs-mirror.mozilla.org. And so, can't do 'cvs diff' to get a good patch (always give me 'cvs [diff aborted]: error writing to server: Connection timed out').

Is it possible to submit a patch with 'diff -u8p' with trunk version renamed with suffix .orig ?

Comment 7

12 years ago
Sorry, I forgot to mention that I tested successfully proposed solution on the trunk of 2006-01-18.

Comment 8

12 years ago
(In reply to comment #6)
> Thanks for your feedback Lewis.

My pleasure.

> IMHO, this bug is very annoying when used with auto-save-as-draft feature. I'm
> surprised too about so few subscribers on this bug. Solving it could improve
> end-user experience and general feeling of the product.
> 
I agree. I use the auto-save-as-draft feature, and this exactly my concern over this. Sometimes, I have numerous copies of one message, and after I finally send the message, I'm left with a ton of remnants in my Drafts folder.

> Concerning the patch, I have trouble to get access to cvs-mirror.mozilla.org.
> And so, can't do 'cvs diff' to get a good patch (always give me 'cvs [diff
> aborted]: error writing to server: Connection timed out').
> 
> Is it possible to submit a patch with 'diff -u8p' with trunk version renamed
> with suffix .orig ?
> 
I wish I could answer you on this, Chris, but unfortunately, I've not had the occasion to submit a patch, nor do I have any authority over such things. I'll see if I can grab someone's attention who may be able to provide some guidance, though. Please bear with me while I see what I can do.

Lewis
Created attachment 208988 [details] [diff] [review]
Chris's patch diffed from Trunk

Lewis contacted me about the issue here, I haven't experienced the issue myself but don't use the autosave feature.  I rarely use the "save as draft" at all but when I have this has not affected me, that may be why there are relatively few people following the bug - maybe not many have started using the autosave yet.
I don't have any idea why Chris is not able to cvs diff, I use "cvs diff -u10 >diff" myself.  Chris if you are able to pull then I would expect you to be able to diff so I don't know what is happening there.  
I have used the diff as you suggested before I learned of the cvs diff but I know that cvs diff is preferred (being told that is how I learned of it :) ).

Comment 10

12 years ago
Thanks Lewis & Andy for your help.

I am still not able to pull any file from cvs. I used trunk tarball for my test. It may be a temporary problem or an ISP one. I will check this.

Thanks anyway for the patch.

Comment 11

12 years ago
If the solution and the patch are acceptable, could someone with authority change this bug resolution to 'fixed' ?

Thanks in advance,

Chris

Comment 12

12 years ago
(In reply to comment #11)
> If the solution and the patch are acceptable, could someone with authority
> change this bug resolution to 'fixed' ?

It is fixed then the patch is checked in (on the trunk). I see no indication for that. 

Version: 1.8 Branch → Trunk

Comment 13

12 years ago
Sorry for the mistake.
AFAIK, the patch is not checked in. I have no rights to check it in (and have problems for check out ...).
Must I request rights to check it in or someone could do it and then set resolution field to 'fixed' ?

Comment 14

12 years ago
Chris, you need to do a proper diff for this patch.  The patch is 166KB big and there is no way to determine what has changed.  Because of this it will be impossible for someone to review it.

Comment 15

12 years ago
Created attachment 209289 [details] [diff] [review]
Patch bug 314009 proposal

I really understand but actually I have big trouble with cvs-mirror.mozilla.org.  I cannot run correctly the required 'cvs diff -u8p' to create the patch.

However, I attached to the bug a patch proposal obtained with 'diff -u8p' with the actual 'nsMsgCompose.cpp' file, (rev 1.479) from the trunk, renamed as 'nsMsgCompose.cpp.orig'.

I cannot do more actually and I hope it will be ok to be reviewed.

Thanks anyway Ray for your comment.

Comment 16

12 years ago
Created attachment 209468 [details] [diff] [review]
proposed fix

This is a proper diff of Chris' patch (since he had problems with cvs..). The fix works fine when I tested.
Attachment #208988 - Attachment is obsolete: true
Attachment #209289 - Attachment is obsolete: true
Attachment #209468 - Flags: review?(bienvenu)

Comment 17

12 years ago
Comment on attachment 209468 [details] [diff] [review]
proposed fix

thx for working on this.

why not add || mDeliverMode == nsIMsgSend::nsMsgQueueForLater to the previous if? Wouldn't that be the same, and cleaner?
Attachment #209468 - Flags: review?(bienvenu) → review-
I apologize, I apparently clicked the wrong file when I was adding the diff and only just now noticed it.

Comment 19

12 years ago
(In reply to comment #17)
> why not add || mDeliverMode == nsIMsgSend::nsMsgQueueForLater to the previous
> if? Wouldn't that be the same, and cleaner? 

Thanks for your comment David.
Doing so will not solve this bug. Previous 'if' is included in an 'if/else' statement with condition on mDeliverMode.
Let's have a look at http://lxr.mozilla.org/mailnews/source/mailnews/compose/src/nsMsgCompose.cpp#3167
IMHO, previous 'if' is executed only when deliver mode is set to save as draft or as template.

I choose to put it after in order to minimize impact of the fix and to prevent rewriting above statements.

Let me know your feeling.

Thanks Magnus for the proper fix, and thanks anyway Andy (this can happen to everyone).

Comment 20

12 years ago
Created attachment 209718 [details] [diff] [review]
proposed fix, v2

Addressing review comments.

I reworked the previous if/else clause to make this fit in better. 
Thanx Chris for the original patch.
Attachment #209468 - Attachment is obsolete: true
Attachment #209718 - Flags: review?(bienvenu)

Comment 21

12 years ago
Comment on attachment 209718 [details] [diff] [review]
proposed fix, v2

thx for the patch - sorry I was mistaken before.
Attachment #209718 - Flags: superreview?(mscott)
Attachment #209718 - Flags: review?(bienvenu)
Attachment #209718 - Flags: review+

Comment 22

12 years ago
Thanks Magnus for the proper patch according to review comments and thanks David for reviewing it (even if it was not clear enough in its first proposal).

Comment 23

12 years ago
See also bug 307046

Comment 24

12 years ago
I am not suprised at so few subscribers on this bug - the autosave functionality is very annoying in its current implementation due to a host of other issues. I have it switched off permanently and I suspect so do most other users. See bugs bug 307042 , bug 307044 , bug 307046, bug 307028 .
(Assignee)

Updated

11 years ago
Attachment #209718 - Flags: superreview?(mscott) → superreview+

Updated

11 years ago
Whiteboard: [checkin needed]

Comment 25

11 years ago
fixed on the trunk, thanks, folks.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Keywords: fixed1.8.1
Whiteboard: [checkin needed]
(Assignee)

Comment 26

11 years ago
David B, what do you think about taking something like this on the 1.8.0.x branch at some point? 
Flags: blocking1.8.0.5?

Comment 27

11 years ago
I think this would be fine for the .0x branch, along with the autosave not disabling the ui fix, I think.
Please ask for approval on the patch if you still want this.
Flags: blocking1.8.0.5? → blocking1.8.0.5+
(Assignee)

Comment 29

11 years ago
Comment on attachment 209718 [details] [diff] [review]
proposed fix, v2

per Dan's suggestion :)
Attachment #209718 - Flags: approval1.8.0.5?
Comment on attachment 209718 [details] [diff] [review]
proposed fix, v2

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #209718 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.5

Comment 31

11 years ago
Verified FIXED using Thunderbird version 1.5.0.5 (20060621). 

When I choose to Send Later the message is removed from the Drafts folder and on restart I am prompted to send unsent messages.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.5 → verified1.8.0.5

Comment 32

11 years ago
(In reply to comment #36)
> Wim/Axel:  Can one of you please test the latest 1.5.0.5 rc3 fy-NL builds and
> verify that this bug is fixed?  The builds should be ready by Monday.

Due to my holiday a very late answer:
I have tested version 1.5.0.6 and all seems to be ok, so this one is fixed.

Wim

Updated

11 years ago
Duplicate of this bug: 104818
In trying to verify this on the Tbird 2 cand build with my IMAP acccount, I still see the Unwanted draft messages in my folder after the following the steps in the initial report. I mentioned this to mscott, who indicated there is another bug open to cover the IMAP issue. Will have to hunt around for it.

Updated

10 years ago
Duplicate of this bug: 327993

Updated

9 years ago
Blocks: 427936

Updated

9 years ago
No longer blocks: 427936

Comment 36

9 years ago
Is this bug meant to be fixed in v2.0?  I have IMAP too, and now have 1000s of drafts that never go away.  (That's Tbird 2.0.0.14).

Comment 37

9 years ago
Yes, the fix even went into tb1.5.0.5. If you have a similar problem, feel free to open a new bug about it. And attach an imap protocol log to the new bug - see http://wiki.mozilla.org/MailNews:Logging
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.