11387 - [FEATURE] Need to Track Message ID for Replacement on Saving Drafts (multiple copies are saved in drafts folder)

Status: REOPENED

(SeaMonkey :: MailNews: Message Display, defect, P2)

Description

[rhp (gone)]

We need a way for the FE to keep track of an existing message that it may be
editing when dealing with a loaded Draft message. When you load a draft, you
have to keep a hold on this message ID since this will be necessary to pass
back in to the back end so we replace the message being edited and not add a
new message.

Also, when you send a message that was a draft, you need to delete that message
from the Drafts folder.

Comment 1

[lchiang]

adding to the main [FEATURE] bug.

Comment 2

[Phil Peterson]

Bulk move mail/news M10 bugs to M11

Comment 3

[sol]

Drafts are not a beta 1 feature, so moving to M14.

Comment 4

[Jean-Francois Ducarroz]

Rich, why not just using the compose field messageID.  As draft are constructed from a compFields and as the front

End never touch it therefore you should be already able to use it to track a draft message! Else I can just add a new

field: draftID.

Comment 5

[rhp (gone)]

I think that the message id in the comp fields is the long cryptic string but
the message ID we need to track is the one that is the ID for the mail store.
Does this make sense? I think that we will need to add another field.

- rhp

Comment 6

[Jean-Francois Ducarroz]

OK, I will a new one. BTW, do you know how we did it in 4.x?

Comment 7

[rhp (gone)]

MWContext knows all :-)

- rhp

Comment 8

[Jean-Francois Ducarroz]

Ok, let re-introduce MWContext :->

Comment 9

[pmock]

Changing QA assigned to [e-mail]

Comment 10

[Jean-Francois Ducarroz]

Ok, message compose now (no yet checked in) keep a copy of the original draft message ID into the compose field 
"draftId". When you open a draft message, mime set the draftId. However, we need to do the same when we save a 
message as draft.

We need also to implement all the from end side of the save command. Rich, how to you want to proceed with this 
feature.

Comment 11

[rhp (gone)]

We should chat about this...I'm not sure I understand what you want me to do, 
but I remember when I filed this bug that I was pretty sure that all the back 
end work that was necessary was complete.

- rhp

Comment 12

[jefft]

Taking per selmer's request...

Comment 13

[Jean-Francois Ducarroz]

I have already check in my part of the fix. The message ID (URI) is now stored in the compose fields.

Comment 14

[lchiang]

move to M16.  Not M15 stoppers

Comment 15

[jefft]

There are array of behaviors we need to address. Such as, subsequent save, save 
as etc. Templates need to be handled as well.

Comment 16

[jefft]

Message key is really what we need to track not the message id. For imap 
messages, this is the uid. For pop3 and local messages this is the offset of the 
message in berkerley folder format.

Comment 17

[Scott MacGregor]

Me again...last message I swear =). The usual story about this is listed as a
feature bug which isn't nominated for beta2. That means we either punt it or
nominate for beta2 and take to the PDT team for discussion.

Comment 18

[jefft]

Future release, M20 .....

Comment 19

[lchiang]

Peter - can you check to make sure current functionality is acceptable?  Then, 
I'll move this out to Future milestone for any more work that needs to be done 
to make this better.

Comment 20

[jefft]

Compare to 4.x. We don't delete drafts after we sent. We don't replacing the 
draft if we modify it and then save it again. 

Comment 21

[pmock]

Thanks Jeff for checking for me. I get the same behavior using yesterday Win32 
2000-060721-m17 build.  I will check linux and mac just to check those behavior.

Lisa, the current functionality is acceptable.  We should just make a note in 
the most common mail issues.

Comment 22

[pmock]

fyi
Mac 2000-060811-17 and linux 2000-060811-m17 exhibits the same behavior as 
win32.

Comment 23

[lchiang]

thanks.  Move to Future target milestone and add relnote keyword

Comment 24

[jglick]

FYI, Windows 4.x behavior. Drafts are removed from Drafts folder once they are 
sent. We don't replacing the draft if we modify it and then save it again. 

Comment 25

[jefft]

> We don't replacing the draft if we modify it and then save it again. 

Yes, we do. "Ctrl-s" and "Save" do replacing the draft. "Save As" do *not* 
replacing the draft. We create a new draft. The semantic is the same with most 
of the windows program. For example, word document. Templates behave the same.

Comment 26

[jglick]

Yes, you're right. Sorry.

Comment 27

[David :Bienvenu]

if you've used the product, you'll notice a proliferation of draft messages.
It's very unnattractive. We need to fix this.

Comment 28

[lchiang]

ok, david.  I'll move back to m18 to coincide w/your nsbeta3 nomination.

Comment 29

[lchiang]

- per mail triage

Comment 30

[laurel]

*** Bug 56510 has been marked as a duplicate of this bug. ***

Comment 31

[scottputterman]

removing mail2 keyword.

Comment 32

[laurel]

*** Bug 59235 has been marked as a duplicate of this bug. ***

Comment 33

[jefft]

mscott, the same mechanism you used in bug 22158 should be able to handle
deleteing Drafts/Templates after they have bend sent/saved. Thanks, mscott.

Comment 34

[jglick]

Add mail3 keyword so bug considered for 6.5

Comment 35

[Jean-Francois Ducarroz]

*** Bug 52331 has been marked as a duplicate of this bug. ***

Comment 36

[scottputterman]

marking nsbeta1+

Comment 37

[scottputterman]

moving to mozilla0.8 milestone.

Comment 38

[laurel]

*** Bug 64653 has been marked as a duplicate of this bug. ***

Comment 39

[laurel]

*** Bug 64653 has been marked as a duplicate of this bug. ***

Comment 40

[scottputterman]

moving to mozilla0.9

Comment 41

[scottputterman]

marking nsbeta1- and moving to future milestone.

Comment 42

[David :Bienvenu]

*** Bug 69806 has been marked as a duplicate of this bug. ***

Comment 43

[David :Bienvenu]

*** Bug 67130 has been marked as a duplicate of this bug. ***

Comment 44

[jglick]

Do we really want to minus this bug?  That is 6 dups so far.  Obviously this is 
a problem to people.

Comment 45

[Fabian Guisset]

*** Bug 69942 has been marked as a duplicate of this bug. ***

Comment 46

[Philipp Wertz]

> Obviously this is a problem to people.

It is! If you really use the product it's very annoying!

-- phil

Comment 47

[jglick]

OK, thats 7 dups so far. Removing the "-" from the nsbeta1 keyword so this gets 
re-considered.  Do I need to change the target milestone to "---" as well to get 
this re-considered?

Comment 48

[esther]

reassigning this to me as QA

Comment 49

[scottputterman]

removing the nsbeta1+ for the next round of triaging.

Comment 50

[scottputterman]

marking nsbeta1+ and moving to mozilla0.9

Comment 51

[laurel]

*** Bug 71838 has been marked as a duplicate of this bug. ***

Comment 52

[scottputterman]

reassigning to cavin

Comment 53

[scottputterman]

moving to mozilla0.9.1

Comment 54

[Cavin Song]

Attachment: Fix draft message save and send behaviors

Comment 55

[Cavin Song]

Attachment: Incorporated review comments

Comment 56

[Jean-Francois Ducarroz]

I don't think so we need to keep around the following line:

+
			//newDraftIdURL += "?fetchCompleteMessage=true";


except this little comment, R=ducarroz

Comment 57

[laurel]

*** Bug 77246 has been marked as a duplicate of this bug. ***

Comment 58

[(not reading, please use [e-mail] instead)]

this will be great to have!

I've got some issues with this patch.

1) fix the typo in your idl file:  savedFoldeURI should be savedFolderURI (that
will affect your C++, too.)

2)  [nocript]

how'd that get by the IDL compiler?  can you

3)  minor nit, fix the spelling of "Retreive", it should be "Retrieve".

4) in MailNewsTypes2.idl, we define the nsMsgKey type,  you should be able to
change 

[noscript] attribute  unsigned long messageKey;

to

[noscript] attribute nsMsgKey messageKey;

5)

+/* attribute MSG_ComposeType type; */
+NS_IMETHODIMP nsMsgCompose::SetType(MSG_ComposeType aType)
+{
+  NS_ENSURE_ARG_POINTER(aType);

aType is not a pointer.  is 0 not legal?  If so, use NS_ENSURE_ARG()
my guess is that line isn't needed.

6)  style nit:

+  else
+  // Remove the current draft msg when saving to draft is done. Also,
+  // if it was a NEW comp type, it's now DRAFT comp type. Otherwise
+  // if the msg is then sent we won't be able to remove the saved msg.
+  if (mDeliverMode == nsIMsgSend::nsMsgSaveAsDraft)
+  {
+
compose->SetType(nsIMsgCompType::Draft);
+
RemoveCurrentDraftMessage(compose, PR_TRUE);
+  }

can you do:

// comment
else if
{
}

or 

else if
{
  // comment
}

7) rv = compFields->GetDraftId(&curDraftIdURL);

instead of using a char *, use an nsXPIDLCString, so you won't have to worry
about the free.  that should allow to reorganize the code to return on failure,
and avoid all the nested if statements.

8) rv = msgFolder->DeleteMessages(messageArray, nsnull, PR_TRUE, PR_FALSE, nsnull);

is DeleteMessages() synchronous?  (I'm not sure).  if not, is that a problem
when drafts is on your imap server.
  
9) all that string parsing / appending code has to go.  [the stuff before the
call to SetDraftId()]

you've got the newUid and you've got the saved folder URI.  use the RDF service
GetResource() to turn the saved folder uri into a resource, and then query
interface into an nsIMsgFolder.

[we do this all over the code, so you should be able to find out how to do it. 
but this would be a could time to read the docs on RDF.  I can also give you a
crash course]

nsIMsgFolder has a readonly attribute, baseMessageURI.  see 
nsMsgDBView::GenerateURIForMsgKey() for some to generate a message uri with the
key.  instead of copy and paste, I'd push the common code into nsIMsgFolder (add
string generateMessageUri(in nsMsgKey key) to the nsIMsgFolder interface) and
fix your code and nsMsgDBView::GenerateURIForMsgKey() to use it.

once all that is fixed, please attach a new patch and explain how you tested it:

10) testing

a) did you test with a draft folder on local and on imap?
b) on send, do we still remove the most resent draft of the message from the
draft folder

Comment 59

[(not reading, please use [e-mail] instead)]

> 2)  [nocript]
>
> how'd that get by the IDL compiler?  can you

...can you log a bug?

Comment 60

[Jean-Francois Ducarroz]

the idl compiler just ignore it, without any warning or error. This was i
mistake I did and I asked cavin to fix it as he was modifing the IDL file.

Comment 61

[Cavin Song]

Attachment: Addressed comments from sspitzer

Comment 62

[(not reading, please use [e-mail] instead)]

looking good,  a few comments:

+nsMsgComposeSendListener::RemoveCurrentDraftMessage

a style nit:  you don't have to continually nest your ifs.
that quickly gets out of hand.  you can do avoid the hard to follow nested ifs
by checking and returning (and asserting) if things aren't right.

instead of using kRDFServiceCID, can you use the prog-id instead.

+
compFields->SetDraftId((const char *)newDraftIdURL.get());

that cast is not necessary.  

+  NS_ENSURE_ARG_POINTER(aURI);
+  nsXPIDLCString baseURI;
+  GetBaseMessageURI(getter_Copies(baseURI));

I'd do:

nsresult rv = GetBaseMessageURI(getter_Copies(baseURI));
NS_ENSURE_SUCCESS(rv,rv);

+  nsCAutoString uri;
+  uri.Assign(baseURI);
+
+  // append a "#" followed by the message key.
+  uri.Append('#');
+  uri.AppendInt(msgKey);
+
+  *aURI = uri.ToNewCString();
you should check if that *aURI is null. if so, return NS_ERROR_OUT_OF_MEMORY;
+  return NS_OK;

Comment 63

[Cavin Song]

Attachment: Fixed coding style issues

Comment 64

[(not reading, please use [e-mail] instead)]

ok, there is a problem.

try to compose a message, hit save, make a change, hit save, repeat.

for local mailboxes (when the drafts folder is on a pop or local folders) it 
works fine and I only have 1 version of the message in my drafts folder.

when my drafts folder is on my IMAP server, the first time I hit "save" it 
works fine.  the second time works fine, we replace the message.

the 3rd, 4th and fifth time, we aren't removing the old message.

cavin, do you see that?

my imap server is IMAP4rev1 v12.250, which is a UW server, right?

I haven't tried our internal imap server.

Comment 65

[(not reading, please use [e-mail] instead)]

one more data point:

if my draft folder is open in the thread pane when I do all this, the 2nd save
works but the rest fail to delete the old copy.  so if I hit save 3 times, I get
2 copies.

if I'm viewing another folder in the thread pane, none of the deletes work.
so if I hit save 3 times, I get 3 copies.

Comment 66

[Cavin Song]

Seth, the UW IMAP server you're running does not return uid right away in the 
response for the 'APPEND' cmd. So in this case we (pretty smart) issue a SEARCH 
cmd on the msg id (like [e-mail]) to get the uid for the 
msg we just saved. My guess is that the code which processes the SERACH response 
does set the m_messageKey in the nsMsgComposeAndSend object (like the code 
handling APPEND response does). The new code heavily replies on the uid returned 
from the server when the APPEND cmd is completed. Since uid is never set for the 
(old/existing) msg the DELETE cmd is never issued, and that's why msgs were 
piling up in the draft folder.  We need to look into the code of handling search 
response to find solution to the problem.

Glad you brought it up, I think there's another problem even with non-UW servers 
(like ours here) - timing issue. So if you hit the Save button very very fast 
(for the sake of testing) some older draft msgs may not get deleted. This is 
mainly because the new uid is not returned to our code fast enough before the 
next Save button is clicked due to some uncontrollable factors like network 
performance, server processing speed, etc. This one is a bit harder to solve.

Comment 67

[Cavin Song]

Attachment: Fixed UW server problem

Comment 68

[Cavin Song]

Here is the explanation of the fix:

What happens is that the backend imap code does process the SEARCH response from 
the server (ie, handles the new uid) and set m_messageKey in the 
nsMsgComposeAndSend object. It stores the uid in a list array and uses only the 
very first value in the array to reset m_messageKey. However, the array is never 
reset so m_messageKey is always set to the first value in the array even though 
the new uids are correctly stored in the array (ie, the 2nd, 3rd etc). So the 
fix is to reset the array right before the SEARCH (uid by msg id) cmd is issued. 
This way, the first value in the array is guaranteed to be the new uid returned 
from the server. The change is in nsImapProtocol.cpp.

Comment 69

[(not reading, please use [e-mail] instead)]

cavin's fix to nsImapProtocol.cpp fixes the UW problem for me.

once we get that part reviewed by an IMAP person, I can sr this bug and land 
cavin's fix.

Comment 70

[David :Bienvenu]

r=bienvenu for the imap part.

Comment 71

[(not reading, please use [e-mail] instead)]

sr=sspitzer for the whole bug.

I'll land the patch today.  (It's in my tree and I've been testing it.)

again, excellent work cavin!

Comment 72

[(not reading, please use [e-mail] instead)]

fixed.

cavin, can you log the bug on the "rapid save of imap draft" problem?

Comment 73

[Cavin Song]

The timing issue with save imap draft is logged as #78391.

Comment 74

[esther]

Using build 2001-05-02 on win, mac and linux this is fixed.  I tested this 
with the Drafts folder on IMAP and POP & our default Local. Thanks Cavin we've 
been waiting a long time for this.  Verified

Comment 75

[Boris 'pi' Piwinger]

*** Bug 141209 has been marked as a duplicate of this bug. ***

Comment 76

[Boris 'pi' Piwinger]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6a) Gecko/20030924

Many dupes to this bug say that multiple copies of a message are saved in the
drafts folder. This is still happening? Reopen or file a new bug?

pi

Comment 77

[Jonathan Larmour]

Yes, please reopen this bug. This isn't completely fixed.

I believe what was fixed was that if you open a draft message and hit save
again, it will replace the old one.

What still doesn't work is that if you have a newly composed message and save it
for the first time, it keeps that compose window open. If you save again from
that compose window, then it adds a new draft to the draft folder. It does not
replace the already saved message.

Comment 78

[Boris 'pi' Piwinger]

Well, then, reopening.

pi

Comment 79

[Amit D Chaudhary]

Hi,

Just a comment, with Mozilla 1.5 on linux, I have not seen the problem including
the following by Jonathan.
>What still doesn't work is that if you have a newly composed message and save it
for the first time, it keeps that compose window open. If you save again from
that compose window, then it adds a new draft to the draft folder. It does not
replace the already saved message.

Four different saving of drafts resulted in only one copy.
1. New message composed and save. Then save again, using both CTrl+S and menu,
no new copy, no new copy in drafts folder
2. New message composed and save. Then modify the message in the window that
stays open and save again, using both CTrl+S and menu, no new copy in drafts folder.
3. Open an existing draft message, then save. no new message.
4. Open an existing draft message, modify subject in one case and body in
another and then save. no new message, the current message is modified.

I would resolved it as fixed.

Thanks a lot for the fix as I have a tendency of pressing Ctrl+S when I am
composing large emails.

Regards
Amit

Comment 80

[Jonathan Larmour]

Nope, I have verified this as definitely still affecting Mozilla 1.5 on Windows
(I doubt the host OS is the factor though - moz 1.4 on linux also fails which is
also from after this patch). It fails both (1) and (2) of what Amit tried.

However I now see it works with drafts in "Local Folders". So it appears to be
something specific to the IMAP backend, which is what I'm using.

Comment 81

[Tom Liotta]

Due to Netscape Webmail going the non-proprietary IMAP route (after migration to AOL mail), issues such as this have become more important. Some very significant issues relate to Drafts, but no good progress is likely until underlying issues are clean. I use either Moz 1.7.11 or Netscape 7.2 and both have real problems with Drafts.

Comment 82

[Jim Hill]

I get multiple copies in Drafts when connected to a
SmarterMail Enterprise Edition 3.3.2439 IMAP server,
from TB 1.5.0.7  on WinXP sp2.

But now I will use a local drafts folder
to avoid the problem until it's fixed.

jh

Comment 83

[Jesse]

I'd like to drop a line to say I've been experiencing this same bug with the nightly builds for several months now.  I'm currently running:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5pre) Gecko/20070707 Thunderbird/2.0.0.5pre - Build ID: 2007070705

My host says they're running Courier 3.0.3 IMAP server.  I will also use the local drafts folder until this is resolved.

Comment 84

[Petr Tuma]

A small note to add - I am observing this behavior on Linux with Thunderbird 2.0.0.4 but only in offline mode (when I am taking a long time to write a message, the autosave feature creates multiple drafts that never get deleted). Never encountered it when online though (Dovecot IMAP folders on the server side).

Comment 85

[Wayne Mery (:wsmwk)]

can you test a trunk build?
 ftp://ftp.mozilla.org/pub/thunderbird/nightly/latest-trunk/

both this and bg 78391 WFM using thunderbird trunk build 

Comment 86

[Wayne Mery (:wsmwk)]

WFM  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120303 SeaMonkey/2.0a1pre

Comment 87

[Boris 'pi' Piwinger]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.13) Gecko/20100914 SeaMonkey/2.0.8

After several years I am back to SeaMonkey as my mail client. I tend to save longer mail messages I write many times (I am totally paranoid about crashes). It is not 100% clear to me when several copies are saved, it does not happen all the time, but more often than not. Even worse, if I choose to save a copy (actually, I would like to move) in another folder, then this folder is spoiled.

I guess this relates to this old bug. Not sure if bug 482836 is related somehow.

pi

Comment 88

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5901d0a0f7dc
[components] For https://github.com/mozilla-mobile/android-components/issues/11387 - Remove deprecated constructor from PromptFeature.kt
https://hg.mozilla.org/integration/autoland/rev/6125686440d3
[components] For https://github.com/mozilla-mobile/android-components/issues/11387 - Update changelog with an entry on the PromptFeature constructor removal (https://github.com/mozilla-mobile/android-components/pull/11442)