531757 - Extend nsIMessenger.idl to allow detachment to folders

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[Kent James (:rkent)]

The standard detach calls in nsIMessenger.idl are meant to be used in conjunction with the UI to detach messages, and therefore bring up a folder dialog box to select the folder. For a detach filter action, we would really like to specify the folder as is done in saveAttachmentToFolder. But the equivalent call does not exist for detach that is accessible to js, though the C++ version seems to.

There may be some easy way to do this that I am not seeing. The goal is to be able to write a function call in javascript that would detach all messages from a message, saving the files (with duplicate prevention) to a specified folder.

Comment 1

[Kent James (:rkent)]

Attachment: add DetachAttachmentsToFiles method

Comment 2

[Kent James (:rkent)]

This patch adds a new method which is useful in extensions and tests.

The only effect on existing behavior that I am aware of is that it extends the use of the withoutWarning option in attachment processing to eliminate progress windows, and to create unique files instead of prompting if an existing file exists. That option was added by an extension writer, and is not used in the core code, so it is difficult to be sure that this will not impact any existing uses. I could have (and will if desired) complicated the code some more to add another set of options that would preserve any existing behavior, but it seems to me that the intent of withoutWarning was to get rid of all unnecessary window interactions, which this change does.

The main point of this for me is that it will allow a custom filter action "Detach Attachments" which will then let users, controlled by filters, decide to move attachments from messages to files. I also need bug 555051 for that to work reliably, though this bug is not dependent on that one.

From the project perspective, you get a minimal unit test of the detach attachments feature, which is one of those multi-step async operations that is easy to break (and also prone unfortunately to intermittent orange tests, not that I have seen any for this test.)

Comment 3

[David :Bienvenu]

Comment on attachment 435949 [details] [diff] [review]
add DetachAttachmentsToFiles method

looks reasonable, though I've yet to run it. a few nits -
 +     * @param aListener         Listener to inform of start end stop of detach

"start and stop"

+  nsresult rv;
+
+  nsSaveAllAttachmentsState *saveState;
+  nsCOMPtr<nsIFile> clone;
+  rv = aDestFolder->Clone(getter_AddRefs(clone));

move decl of rv to where it's first used.

+//   Get the full message content.
+//
+//   aMsgHdr: nsIMsgDBHdr object whose text body will be read
+//   returns: string with full message contents

should use /**
            *
            */
comment style here.

I wonder if detachAttachmentsToFiles might be better named detachAttachmentsWOPrompts, to make the distinction between it and the other detach methods clearer.

Comment 4

[Kent James (:rkent)]

The rename and other nits are fine. I assume you don't need another patch before you finish the review, but I will be happy to provide one if you like.

Comment 5

[David :Bienvenu]

Comment on attachment 435949 [details] [diff] [review]
add DetachAttachmentsToFiles method

no, I don't need to see an other patch, thx.

Comment 6

[Kent James (:rkent)]

Attachment: fixed nits

Comment 7

[Kent James (:rkent)]

Comment on attachment 436595 [details] [diff] [review]
fixed nits

Checked in http://hg.mozilla.org/comm-central/rev/84862c492dfd

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 436595 [details] [diff] [review]
fixed nits

>+Components.utils.import("resource://app/modules/gloda/mimemsg.js", mimeMsg);

Please, s/app// per bug 555715. Thanks.

Comment 9

[Kent James (:rkent)]

Comment on attachment 436595 [details] [diff] [review]
fixed nits

Backed out http://hg.mozilla.org/comm-central/rev/4b9007c8d27b
 due to intermittent orange in its unit test on Linux.

Comment 10

[Kent James (:rkent)]

Attachment: add small delay before tests

I setup a Linux build environment, and was able to reproduce the failed test in that environment. When the test fails, I could look at the raw mail file, and see that the detachment had actually succeeded, but perhaps the file operations in Linux are not entirely done in a sync manner.

I tested delays of 0 and 20 ms. 0 fails, 20 succeeds reliably. I upped that to 200 milliseconds as a safety margin.

I think that we really need to look at the base code for some of these functions, and try to understand how we can better detect the end of file operations in Linux. Although this particular case was only a testing artifact, it is possible that we are ignoring this delayed file save in some multistep operations in the code base, resulting in operations that fail in the real application. But I would rather not delay this particular bug over that, particularly so close to a code freeze.

Asking for a simple r? since I am interpreting this as a test-only change from a previously reviewed patch.

Comment 11

[Kent James (:rkent)]

Attachment: use resource:///

Sorry, I forgot the requested change to resource:///

Comment 12

[Kent James (:rkent)]

Comment on attachment 436991 [details] [diff] [review]
use resource:///

Checked in http://hg.mozilla.org/comm-central/rev/2b2fa08efe3d

Comment 13

[Kent James (:rkent)]

Attachment: test bustage fix

There ws a minor typo in the line that I added. Checked in as http://hg.mozilla.org/comm-central/rev/a7c5b4e8222d