314009 - Draft messages not deleted when Send Later used

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Jake Howlett]

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

[Magnus Melin [:mkmelin]]

Confirmed.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051026 Thunderbird/1.5 ID:2005102604

Comment 2

[Lewis Rosenthal]

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

[Magnus Melin [:mkmelin]]

-> Core

Comment 4

[Chris]

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

[Lewis Rosenthal]

(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

[Chris]

(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

[Chris]

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

Comment 8

[Lewis Rosenthal]

(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

Comment 9

[Andy Willis (abwillis)]

Attachment: 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

[Chris]

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

[Chris]

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

Thanks in advance,

Chris

Comment 12

[OstGote!]

(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. 

Comment 13

[Chris]

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

[Ray Booysen]

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

[Chris]

Attachment: 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

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

This is a proper diff of Chris' patch (since he had problems with cvs..). The fix works fine when I tested.

Comment 17

[David :Bienvenu]

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?

Comment 18

[Andy Willis (abwillis)]

I apologize, I apparently clicked the wrong file when I was adding the diff and only just now noticed it.

Comment 19

[Chris]

(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

[Magnus Melin [:mkmelin]]

Attachment: 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.

Comment 21

[David :Bienvenu]

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

thx for the patch - sorry I was mistaken before.

Comment 22

[Chris]

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

[Martin Sch]

See also bug 307046

Comment 24

[Ravi]

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 .

Comment 25

[David :Bienvenu]

fixed on the trunk, thanks, folks.

Comment 26

[Scott MacGregor]

David B, what do you think about taking something like this on the 1.8.0.x branch at some point? 

Comment 27

[David :Bienvenu]

I think this would be fine for the .0x branch, along with the autosave not disabling the ui fix, I think.

Comment 28

[Daniel Veditz [:dveditz]]

Please ask for approval on the patch if you still want this.

Comment 29

[Scott MacGregor]

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

per Dan's suggestion :)

Comment 30

[Daniel Veditz [:dveditz]]

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

approved for 1.8.0 branch, a=dveditz for drivers

Comment 31

[Adam Guthrie]

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.

Comment 32

[Wim]

(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

Comment 34

[Marcia Knous [:marcia]]

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.

Comment 36

[Frank von Delft]

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

[Magnus Melin [:mkmelin]]

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