Status

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

(Blocks 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

9 years ago
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.
Assignee

Comment 1

9 years ago
Posted patch Patch v1.0 Port stuff. (obsolete) — Splinter Review
> [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.
Attachment #495511 - Flags: review?(iann_bugzilla)

Comment 2

9 years ago
(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

9 years ago
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.
Attachment #495511 - Flags: review?(iann_bugzilla) → review+
Assignee

Comment 4

9 years ago
>> 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.
Attachment #495511 - Attachment is obsolete: true
Attachment #497782 - Flags: superreview?(neil)
Attachment #497782 - Flags: review+
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.
Assignee

Updated

9 years ago
Blocks: 455870
Assignee

Comment 6

9 years ago
>>+          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>
Attachment #497782 - Attachment is obsolete: true
Attachment #498012 - Flags: superreview?(neil)
Attachment #498012 - Flags: review+
Attachment #497782 - Flags: superreview?(neil)
(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 on attachment 498012 [details] [diff] [review]
Patch v1.2 Fix review issues. r=IanN

>+            attachment.url = fileHandler.getURLSpecFromFile(localFile);;
Nit: ;;
Attachment #498012 - Flags: superreview?(neil) → superreview+
Assignee

Comment 9

9 years ago
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/c6c5290dc2ae
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

8 years ago
Blocks: 663193
You need to log in before you can comment on or make changes to this bug.