743913 - nsIMsgCompose needs saveAsFile() to save composing message

Status: NEW

(MailNews Core :: Composition, defect)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Before implemting the new method, nsMsgCompose::SendMsg  and nsMsgCompose::_SendMsg should be cleaned up, because the new method also needs the codes SendMsg and _SendMsg.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 1/3] Split nsMSgCompose::SendMsg into shorter methods.

nsMsgCompose::SendMsg is split into ConvertBodyText, AttachVCard, OpenProgressDialog and DisplayErrorMessage.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 2/3] Split nsMsgCompose::_SendMsg into shorter methods

GetConvertedBody and SetCompFields are split out.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 3/3] Implement nsIMsgCompose.saveAsFile

Comment 5

[David :Bienvenu]

With these patches applied, compose xpcshell test test_attachment.js crashes in RememberQueuedDisposition with a null m_identity.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 3/3] Implement nsIMsgCompose.saveAsFile

(In reply to David :Bienvenu from comment #5)
> With these patches applied, compose xpcshell test test_attachment.js crashes
> in RememberQueuedDisposition with a null m_identity.

The last patch did accidentally remove "m_identity = aIdentity" just before invoking _SendMsg(). Anyway it my fault. I should have run all compose xpcshell tests.

The last patch causes test_nsMsgCompose2.js failure because the test calls onStopSending with null for returnFile. This patch also fixes this failure.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I confirmed all xpcshell tests passed on Linux.

And pushed to tryserver now.
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=35d329483b4e

Comment 8

[David :Bienvenu]

Comment on attachment 613496 [details] [diff] [review]
[PATCH 1/3] Split nsMSgCompose::SendMsg into shorter methods.

thanks for the new patch...

I think rv might be used unitialized in  nsMsgCompose::ConvertBodyText() if m_editor is false. Minusing for that issue...
 

nsMsgCompose::OpenProgressDialog can probably directly return the mProgress->OpenProgressDialog call result.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 1/3] Split nsMSgCompose::SendMsg into shorter methods.

Addresses comment #8.

Comment 10

[David :Bienvenu]

Comment on attachment 618126 [details] [diff] [review]
[PATCH 1/3] Split nsMSgCompose::SendMsg into shorter methods.

thx for the new patch

Comment 11

[David :Bienvenu]

Comment on attachment 613497 [details] [diff] [review]
[PATCH 2/3] Split nsMsgCompose::_SendMsg into shorter methods

Instead of Adopt, we could use getter_Copies in +nsresult nsMsgCompose::GetConvertedBody(nsACString &aBody) in the call to nsMsgI18NSaveAsCharset

you might need to pass in nsCString & instead of nsACString, but since it's not a public method, that's OK.

r=me, with that change.

Comment 12

[David :Bienvenu]

Comment on attachment 615209 [details] [diff] [review]
[PATCH 3/3] Implement nsIMsgCompose.saveAsFile


thx for the patch!

+## @name NS_MSG_UNABLE_TO_SAVE_FILE
+12601=Unable to save your message as file.

either "as a file" or "to a file"

I think you should add the same strings to suite's properties file.

need doxygen-style comments for this method, and wrap this long line:

+  void saveAsFile(in string aFileName, in nsIMsgIdentity aIdentity, in string accountKey, in nsIMsgWindow aMsgWindow, in nsIMsgProgress aProgress);
+

+  nsresult rv = SetCompFields(aIdentity);
+
this error is ignored; Should it have NS_ENSURE_SUCCESS(rv, rv); ?

I think most of these:
+  if (NS_FAILED(rv))
+    return rv;

can be NS_ENSURE_SUCCESS(rv, rv); since these are unexpected errors.

I'd also like a comment above this method, saying that we save to a temp file, and then when it's done, rename the temp file over the desired target file. Also, what happens if the target file already exists? Do we warn the user, overwrite the file, or fail?

+NS_IMETHODIMP nsMsgCompose::SaveAsFile(const char *aPath,

Comment 13

[David :Bienvenu]

hmm, you know, I think when I run with these patches, my compose window closes down after an auto-save of a draft message. Have you tried that? I should probably provisionally revoke my r+'s until that's figured out...

Comment 14

[David :Bienvenu]

Hiro, are you able to reproduce the issue with auto save of drafts closing the compose window? I have my autosave interval set to 1 minute, so this is easy to reproduce for me.

Comment 15

[David :Bienvenu]

Comment on attachment 618126 [details] [diff] [review]
[PATCH 1/3] Split nsMSgCompose::SendMsg into shorter methods.

clearing review request until the auto-save issue is diagnosed.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Now I can reproduce the closing compose window issue. I will investigate it.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

I found the cause of the closing issue.

The cause lies in [PATCH 3/3]. The mDeliverMode in nsMsgComposeSendListener is wrong.

   // right now, AutoSaveAsDraft is identical to SaveAsDraft as
   // far as the msg send code is concerned. This way, we don't have
   // to add an nsMsgDeliverMode for autosaveasdraft, and add cases for
   // it in the msg send code.
   MSG_DeliverMode deliverMode =
     (mDeliverMode == nsIMsgCompDeliverMode::AutoSaveAsDraft) ?
       nsIMsgCompDeliverMode::SaveAsDraft : mDeliverMode;

-  nsRefPtr<nsIMsgCompose> msgCompose(this);
-  composeSendListener->SetMsgCompose(msgCompose);
-  composeSendListener->SetDeliverMode(deliverMode);

In the original source code, nsIMsgCompose gets the converted deliver mode value, but
+
+nsresult nsMsgCompose::GetMsgSendListener(nsIMsgSendListener **aListener)
+{
+  nsresult rv;
+  nsCOMPtr<nsIMsgComposeSendListener> composeSendListener =
+    do_CreateInstance(NS_MSGCOMPOSESENDLISTENER_CONTRACTID, &rv);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  nsRefPtr<nsIMsgCompose> msgCompose(this);
+  composeSendListener->SetMsgCompose(msgCompose);
+  composeSendListener->SetDeliverMode(mDeliverMode);

This mDeliverMode is not converted here.

I will post two patches to make review easier and a conjunction patch.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Use nsMsgDeliverMode instead nsIMsgCompDeliverMode in nsMsgComposeSendListener to be consistent with nsMsgCompose.

In nsMsgComposeSendListener class, mDeliverMode is compared with nsIMsgSend::nsMsgSave or others, but it should be nsIMsgCompDeliverMode there.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Check also nsIMsgCompDeliverMode::AutoSaveAsDraft in nsMsgComposeSendListener::OnStopCopy

This patch solves the closing window issue.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 3/3] Implement nsIMsgCompose.saveAsFile

This is the conjunction patch.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 2/3] Split nsMsgCompose::_SendMsg into shorter methods

Use getter_Copies.

This change breaks [PATCH 3/3]. I will re-post [PATCH 3/3] again.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: [PATCH 3/3] Implement nsIMsgCompose.saveAsFile

Updated conjunction patch.


(In reply to David :Bienvenu from comment #12)
> Comment on attachment 615209 [details] [diff] [review]
> [PATCH 3/3] Implement nsIMsgCompose.saveAsFile
> 
> +## @name NS_MSG_UNABLE_TO_SAVE_FILE
> +12601=Unable to save your message as file.
> 
> either "as a file" or "to a file"

Done.

> I think you should add the same strings to suite's properties file.

Done.

> need doxygen-style comments for this method, and wrap this long line:
> 
> +  void saveAsFile(in string aFileName, in nsIMsgIdentity aIdentity, in
> string accountKey, in nsIMsgWindow aMsgWindow, in nsIMsgProgress aProgress);
> +

Done.

> +  nsresult rv = SetCompFields(aIdentity);
> +
> this error is ignored; Should it have NS_ENSURE_SUCCESS(rv, rv); ?

Done.

> I think most of these:
> +  if (NS_FAILED(rv))
> +    return rv;
> 
> can be NS_ENSURE_SUCCESS(rv, rv); since these are unexpected errors.

Done.

> I'd also like a comment above this method, saying that we save to a temp
> file, and then when it's done, rename the temp file over the desired target
> file. 

Done. Copied your sentence as it is.

> Also, what happens if the target file already exists? Do we warn the
> user, overwrite the file, or fail?

Current implementation is that "overwrite". While I was implementing this, I thought the file name check should be done in caller of this method. Because file dialog might be opened there, and can be also checked the file name there.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 618126 [details] [diff] [review]
[PATCH 1/3] Split nsMSgCompose::SendMsg into shorter methods.

This patch is not related to the closing window issue.

Comment 24

[David :Bienvenu]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> > Also, what happens if the target file already exists? Do we warn the
> > user, overwrite the file, or fail?
> 
> Current implementation is that "overwrite". While I was implementing this, I
> thought the file name check should be done in caller of this method. Because
> file dialog might be opened there, and can be also checked the file name
> there.

makes sense - a comment to that effect in the idl would be helpful, I think.

Comment 25

[David :Bienvenu]

Comment on attachment 622276 [details] [diff] [review]
[PATCH 3/3] Implement nsIMsgCompose.saveAsFile

r=me, but I'd really like a comment in nsIMsgCompose.idl about what will happen if the file already exists.

Comment 26

[Mark Banner (:standard8)]

Hiro, did you forget about these patches?

Comment 27

[Wayne Mery (:wsmwk)]

(In reply to Mark Banner (:standard8) from comment #26)
> Hiro, did you forget about these patches?

Comment 28

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.