617011 - composeStartup() Fixups.

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, defect)

Description

[Philip Chee]

I'm going to port the following patches in one go because they all touch composeStartup() and it's easier to do it in one go:

[Bug 384160] Cannot send attachments with cli and file with accents.
  [Bug 524428] "thunderbird -compose attachment=file://..." fails to send if the attachment file url is encoded.
[Bug 530779] With main window/3-pane closed, sent email cannot be copied to "sent"-folder and drafts cannot be saved - "There was an error copying the message to the Sent folder. Retry?"
[Bug 295956] Editing message in drafts folder (with IMAP and multiple profiles) results in error.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Port stuff.

> [Bug 530779] With main window/3-pane closed, sent email cannot be copied to
> "sent"-folder and drafts cannot be saved - "There was an error copying the
> message to the Sent folder. Retry?"
I didn't port the Addressbook part of this patch here. Should I do it here or in some other bug/patch?

> [Bug 295956] Editing message in drafts folder (with IMAP and multiple profiles)
> results in error.
I only ported the first half of this patch. I couldn't identify the equivalent code for the rest of the patch.

Comment 2

[Ian Neal]

(In reply to comment #1)
> Created attachment 495511 [details] [diff] [review]
> Patch v1.0 Port stuff.
> 
> > [Bug 530779] With main window/3-pane closed, sent email cannot be copied to
> > "sent"-folder and drafts cannot be saved - "There was an error copying the
> > message to the Sent folder. Retry?"
> I didn't port the Addressbook part of this patch here. Should I do it here or
> in some other bug/patch?
It needs doing whilst we think about it, whichever is easier/more manageable for you.
> 
> > [Bug 295956] Editing message in drafts folder (with IMAP and multiple profiles)
> > results in error.
> I only ported the first half of this patch. I couldn't identify the equivalent
> code for the rest of the patch.
That code was added to TB in Bug 214687, so depends on whether we want that feature - Mnyromyr?

Comment 3

[Ian Neal]

Comment on attachment 495511 [details] [diff] [review]
Patch v1.0 Port stuff.

r=me
Second part of that patch is not needed for SM as you pointed out, so just the AB part to be done of the other bug.

Comment 4

[Philip Chee]

Attachment: Patch v1.1 Include Address Book bits. r=IanN

>> I didn't port the Addressbook part of this patch here. Should I do it here or
>> in some other bug/patch?
> It needs doing whilst we think about it, whichever is easier/more manageable
> for you.

> Ian Neal      2010-12-14 06:47:17 PST
> 
> r=me
> Second part of that patch is not needed for SM as you pointed out, so just the
> AB part to be done of the other bug.

OK. New patch will include Address book bits.

Comment 5

[[e-mail]]

Comment on attachment 497782 [details] [diff] [review]
Patch v1.1 Include Address Book bits. r=IanN

>+var msgWindow = Components.classes["@mozilla.org/messenger/msgwindow;1"]
>+                          .createInstance(Components.interfaces.nsIMsgWindow);
So what does this actually achieve?

>+          if (/^file:\/\//i.test(attachmentStr))
>+          {
>+            attachment.url = encodeURI(attachmentStr);
>+          }
>+          else
>+          {
>+            localFile.initWithPath(attachmentStr);
>+            attachment.url = fileHandler.getURLSpecFromFile(localFile);;
[Nit: ;;]
I would actually do this the other way around: assume that attachmentStr is a path, and if the init fails, fall back to treating it as a URL.

Comment 6

[Philip Chee]

Attachment: Patch v1.2 Fix review issues. r=IanN

>>+          if (/^file:\/\//i.test(attachmentStr))
>>+          {
>>+            attachment.url = encodeURI(attachmentStr);
>>+          }
>>+          else
>>+          {
>>+            localFile.initWithPath(attachmentStr);
>>+            attachment.url = fileHandler.getURLSpecFromFile(localFile);;
> [Nit: ;;]
> I would actually do this the other way around: assume that attachmentStr is a
> path, and if the init fails, fall back to treating it as a URL.
Fixed.

Tested with:
-compose "attachment='file:///C:/T1/aéa.txt'"
-compose attachment=file:///C:/T1/t%C3%A9%C3%A0%C3%A8%C3%B9.txt
-compose "attachment='c:\T1\aéa.txt'"

>>+var msgWindow = Components.classes["@mozilla.org/messenger/msgwindow;1"]
>>+                          .createInstance(Components.interfaces.nsIMsgWindow);
> So what does this actually achieve?

Looking more closely at Bug 530779 Comment 36:
> oh, I should say, this bug won't occur on the mac, or w/ SeaMonkey.

So this is not needed (retested without this part of the patch), but then why do we have this in our mail3pane window?
<http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWindow.js?mark=167-167,194-194#167>

Comment 7

[[e-mail]]

(In reply to comment #6)
> why do we have this in our mail3pane window?
> <http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailWindow.js?mark=167-167,194-194#167>
Because we need e.g. to add a folder listener to it.

According to bug 530779, Thunderbird also uses it to detect shutdown, but I've no idea why it needs to do it like that.

Comment 8

[[e-mail]]

Comment on attachment 498012 [details] [diff] [review]
Patch v1.2 Fix review issues. r=IanN

>+            attachment.url = fileHandler.getURLSpecFromFile(localFile);;
Nit: ;;

Comment 9

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/c6c5290dc2ae