Last Comment Bug 555972 - Save multiple messages as individual files in directory
: Save multiple messages as individual files in directory
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: General (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.1a2
Assigned To: Philip Chee
:
:
Mentors:
Depends on: 58140 537448
Blocks: TB2SM
  Show dependency treegraph
 
Reported: 2010-03-30 07:14 PDT by Philip Chee
Modified: 2010-05-27 02:04 PDT (History)
48 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 Initial port of front-end code. (5.39 KB, patch)
2010-03-30 07:38 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.1 Allow saveAs from the standalone message window for .eml files. (8.80 KB, patch)
2010-04-01 12:54 PDT, Philip Chee
iann_bugzilla: review-
Details | Diff | Splinter Review
Patch v1.2 use Object.defineProperty() (8.90 KB, patch)
2010-04-29 11:19 PDT, Philip Chee
iann_bugzilla: review+
mnyromyr: feedback+
Details | Diff | Splinter Review
Patch v1.3 Fix nits. r=IanN (8.91 KB, patch)
2010-05-05 10:08 PDT, Philip Chee
philip.chee: review+
neil: superreview-
Details | Diff | Splinter Review
Patch v1.4 Addressed Issues. r=IanN (5.82 KB, patch)
2010-05-09 10:34 PDT, Philip Chee
philip.chee: review+
Details | Diff | Splinter Review
Patch 1.4a Final nits r=IanN sr=Neil (5.84 KB, patch)
2010-05-10 02:01 PDT, Philip Chee
philip.chee: review+
philip.chee: superreview+
Details | Diff | Splinter Review

Description Philip Chee 2010-03-30 07:14:36 PDT
> (In reply to Bug 58140 comment #123)
>> Reproduce with SeaMonkey/20100130-trunk/WinXP.
>> Reopen or File for New bug?
> Yes file a new bug about hooking up the front-end code for seamonkey.

+++ This bug was initially created as a clone of Bug #58140 +++

In the MailNews file menu there is a "Save As" submenu containing two items: 
File and Template.  Both options work as expected for a single message, but when 
multiple messages are selected they remain enabled but simply do nothing.

The simplest solution to this would be to just disable those two menu items when 
more then one message was selected.  The better solution would be to get them to 
work with multiple messages as you would normally expect.  IMHO, saving a whole 
group of messages to a single folder with one command would be one of the most 
useful applications of the Save as File menu item.
Comment 1 Philip Chee 2010-03-30 07:38:00 PDT
Created attachment 435896 [details] [diff] [review]
Patch v1.0 Initial port of front-end code.

Basically a straight port of the front end portion of TB Bug 58140 but by sharing some code with Bug 537448 I've reduced the size of the patch.

> +function SaveAsFile(uris)
>  {
> +  var fileNames = [];
> +  for (let i = 0; i < uris.length; i++) {
> +    let subject = messenger.messageServiceFromURI(uris[i])
> +                           .messageURIToMsgHdr(uris[i])
> +                           .mime2DecodedSubject;
> +    let uniqueFileName = suggestUniqueFileName(subject.substr(0, 120), ".eml",
> +                                               fileNames);
> +    fileNames[i] = uniqueFileName;
>    }
> +  if (uris.length == 1)
> +    messenger.saveAs(uris[0], true, null, fileNames[0]);
> +  else
> +    messenger.saveMessages(uris.length, fileNames, uris);

Also by reusing suggestUniqueFileName() from the threadpane DnD the filenames generated are consistent whether you save the messages via DnD or via the menu items. In Thunderbird the filenames generated depend on the code path taken.

>  function MsgSaveAsFile()
>  {
> -    if (GetNumSelectedMessages() == 1) {
> -        SaveAsFile(GetFirstSelectedMessage());
> -    }
> +    var num = GetNumSelectedMessages();
> +    if (num == 1)
> +        SaveAsFile([GetFirstSelectedMessage()]);
> +    else if (num > 1)
> +        SaveAsFile(gFolderDisplay.selectedMessageUris);

Unfortunately our standalone message window doesn't have a fake gFolderDisplay so I'm taking a longer path.
Comment 2 Jens Hatlak (:InvisibleSmiley) 2010-03-30 08:20:43 PDT
(In reply to comment #1)
> Unfortunately our standalone message window doesn't have a fake gFolderDisplay

Yes it has.

<http://hg.mozilla.org/comm-central/rev/2750fdd9a5f6>
<http://hg.mozilla.org/releases/comm-1.9.1/rev/2fa7744a093f>
Comment 3 Philip Chee 2010-03-30 10:29:29 PDT
>> Unfortunately our standalone message window doesn't have a fake gFolderDisplay
> Yes it has.

What I meant was that we don't have a subclassed standalone folderDisplay widget so I'm unsure if gFolderDisplay.selectedMessageUris always returns the same URI as GetLoadedMessage().

I guess in OnLoadMessageWindow() I could overrride selectedMessageUris with:

gFolderDisplay.__defineGetter__("selectedMessageUris", function()
  {
    return gCurrentMessageUri? [gCurrentMessageUri] : null;
  }
);
Comment 4 Jens Hatlak (:InvisibleSmiley) 2010-03-30 10:39:13 PDT
(In reply to comment #3)
> >> Unfortunately our standalone message window doesn't have a fake gFolderDisplay
> > Yes it has.
> 
> What I meant was that we don't have a subclassed standalone folderDisplay
> widget so I'm unsure if gFolderDisplay.selectedMessageUris always returns the
> same URI as GetLoadedMessage().

I think it does (Mnyromyr or Neil may correct me if I'm wrong). selectedMessageUris calls gDBView.getURIsForSelection which calls gDBView.GetSelectedIndices which special-cases the standalone message window:

http://mxr.mozilla.org/comm-central/source/suite/mailnews/folderDisplay.js#104
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#2285
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#1198

The same is true for gFolderDisplay.selectedMessage (see 549802 comment 4).

MailNews backend FTW! :-)
Comment 5 Philip Chee 2010-03-30 11:37:18 PDT
In the 3pane window do: File->Open File...
Pick a random .eml file on your hard disk.

GetFirstSelectedMessage(); => file:///c:/PROJECTS/XUL/test.eml?type=application/x-message-display

gFolderDisplay.selectedMessageUris; => null

Of course then SaveAsFile() falls because it doesn't know how to deal with file:/// uris:

Error: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgMessageService.messageURIToMsgHdr]
Source file: chrome://messenger/content/mailCommands.js
Line: 414
Comment 6 Jens Hatlak (:InvisibleSmiley) 2010-03-30 13:19:24 PDT
(In reply to comment #5)
> In the 3pane window do: File->Open File...
> Pick a random .eml file on your hard disk.
> 
> GetFirstSelectedMessage(); =>
> file:///c:/PROJECTS/XUL/test.eml?type=application/x-message-display
> 
> gFolderDisplay.selectedMessageUris; => null

Hmm, strangely that's true for external files (.eml) but not for internal messages (e.g. in Local Folders) opened in a standalone message window. :-/

TB's gFolderDisplay.selectedMessageUris looks similar enough to ours so the difference is probably in the backend or its initialization, especially since the same applies to gFolderDisplay.selectedMessage.

I think it's worth further investigating this instead of doing a workaround because we have to fix it anyway at some point in time. Maybe TB fixed it as part of its JS-driven folder pane upgrade. If Mnyromyr doesn't know, maybe one of the TB devs (bienvenu, asuth, ...) knows.
Comment 7 Philip Chee 2010-03-30 20:27:25 PDT
> TB's gFolderDisplay.selectedMessageUris looks similar enough to ours.

The TB gFolderDisplay instance in the standalone message window has a different folderWidget backend:

StandaloneFolderDisplayWidget::get selectedMessageUris() {
    if (this.messageDisplay.displayedUri)
      return [this.messageDisplay.displayedUri];
    return this._superSelectedMessageUrisGetter.call(this);
  },

So in the TB standalone message window:

gFolderDisplay.selectedMessageUris; =>
file:///C:/T1/bug53233.eml?type=application/x-message-display

Of course their (File->Save As) SaveAsFile(gFolderDisplay.selectedMessageUris); also falls over at the same point.
Comment 8 Philip Chee 2010-04-01 12:54:20 PDT
Created attachment 436545 [details] [diff] [review]
Patch v1.1 Allow saveAs from the standalone message window for .eml files.

Changes since the last patch:
1. Implement a fake minimal gFolderDisplay to emulate the fake standalone folder widget in TB.
2. This allows me to not special case the message window in mailWindowOverlay.js->MsgSaveAsFile().
3. SaveAsFile() can now save external .eml files loaded into the message window.
Comment 9 Ian Neal 2010-04-05 09:21:19 PDT
Comment on attachment 436545 [details] [diff] [review]
Patch v1.1 Allow saveAs from the standalone message window for .eml files.

On code inspection only so far:

>+++ b/suite/mailnews/mail3PaneWindowCommands.js
>       case "cmd_saveAsFile":
>+        return GetNumSelectedMessages() > 0
Nit: missing ;
>       case "cmd_saveAsTemplate":
>-	      if ( GetNumSelectedMessages() > 1)
>+        if ( GetNumSelectedMessages() > 1)
Nit: remove space after (
>           return false;   // else fall thru

>       case "cmd_printpreview":
>-	      if ( GetNumSelectedMessages() == 1 && gDBView)
>+        if ( GetNumSelectedMessages() == 1 && gDBView)
Nit: remove space after (

>+++ b/suite/mailnews/mailCommands.js
>+function SaveAsFile(uris)
Nit: use variables starting with a for function arguments, so aUris.
> {
You use uris.length a few times, is it worth setting a variable to it?

>+  if (uris.length == 1 && gFolderDisplay.selectedMessageIsExternal)
>+  {
>+    saveURL(uris[0], null, "", true, false, null);
>+    return;
>+  } 
>+  var fileNames = [];
>+  for (let i = 0; i < uris.length; i++) {
>+    let subject = messenger.messageServiceFromURI(uris[i])
>+                           .messageURIToMsgHdr(uris[i])
>+                           .mime2DecodedSubject;
>+    let uniqueFileName = suggestUniqueFileName(subject.substr(0, 120), ".eml",
>+                                               fileNames);
>+    fileNames[i] = uniqueFileName;
Does using the variable uniqueFileName help in the readability? I don't think so, so unless I am missing something, just inline.

>+++ b/suite/mailnews/messageWindow.js
>+  SetupFolderDisplayWidget()
Nit: missing ;

>+  gFolderDisplay.__defineGetter__("selectedMessages", function()
>+    {
>+      return this.selectedMessage ? 
>+             [this.selectedMessage] : [];
Nit: Why is this split over two lines and longer ones not?
Comment 10 Ian Neal 2010-04-20 05:11:49 PDT
Comment on attachment 436545 [details] [diff] [review]
Patch v1.1 Allow saveAs from the standalone message window for .eml files.

r- for now (so a new patch is generated)
Comment 11 Philip Chee 2010-04-29 11:19:41 PDT
Created attachment 442451 [details] [diff] [review]
Patch v1.2 use Object.defineProperty()

Changes since the last patch:
1. Fix nits.
2. Use Object.defineProperty() instead of obj.__defineGetter__() since the latter is not in ES5 and will go away eventually in Spidermonkey.
Comment 12 Karsten Düsterloh 2010-05-02 16:02:53 PDT
Comment on attachment 442451 [details] [diff] [review]
Patch v1.2 use Object.defineProperty()

Well, first of all, I'd really love to see the distinct standalone window XUL gone, but we're not quite there yet, so we'll still have to deal with all those special hacks. *sigh*
From that pov, having the fake object to avoid special processing is okay with me.

As a sidenote, please adhere more closely to the <https://wiki.mozilla.org/SeaMonkey:MailNews:CodingStyle>. ;-)

Like here (bracing):
>+  for (let i = 0; i < num; i++) {
>+    let subject = messenger.messageServiceFromURI(aUris[i])
(And elsewhere below.)

Like here (indentation):
> function MsgSaveAsFile()
> {
>+    SaveAsFile(gFolderDisplay.selectedMessageUris);

And especially here:
>+  defineGetter("selectedCount", function()
>+    {
>+      return gCurrentMessageUri ? 1 : 0;
>+    }
>+  );

Move the 'function()' onto the next line and align it to the {.
(I leave the real functional review to those asked for it. ;-))
Comment 13 Ian Neal 2010-05-04 06:40:48 PDT
Comment on attachment 442451 [details] [diff] [review]
Patch v1.2 use Object.defineProperty()

As what Mnyromyr said basically, styling of braces and functions.
r=me with those changes.
Comment 14 Philip Chee 2010-05-05 10:08:28 PDT
Created attachment 443648 [details] [diff] [review]
Patch v1.3 Fix nits. r=IanN

Carrying forward r=IanN

Changes since the last patch:
1. Fix coding style nits.
2. Fix one typo nobody spotter :(

>  Karsten Düsterloh      2010-05-02 16:02:53 PDT

> As a sidenote, please adhere more closely to the
> <https://wiki.mozilla.org/SeaMonkey:MailNews:CodingStyle>. ;-)
> 
> Like here (bracing):
>>+  for (let i = 0; i < num; i++) {
>>+    let subject = messenger.messageServiceFromURI(aUris[i])
> (And elsewhere below.)
Fixed.

> Like here (indentation):
> > function MsgSaveAsFile()
> > {
> >+    SaveAsFile(gFolderDisplay.selectedMessageUris);
Fixed.

> And especially here:
> >+  defineGetter("selectedCount", function()
> >+    {
> >+      return gCurrentMessageUri ? 1 : 0;
> >+    }
> >+  );
> 
> Move the 'function()' onto the next line and align it to the {.
> (I leave the real functional review to those asked for it. ;-))
Fixed.

>  Ian Neal      2010-05-04 06:40:48 PDT
> 
> (From update of attachment 442451 [details] [diff] [review])
> As what Mnyromyr said basically, styling of braces and functions.
> r=me with those changes.
Thanks.
Comment 15 neil@parkwaycc.co.uk 2010-05-09 03:43:25 PDT
(In reply to comment #14)
> 2. Fix one typo nobody spotter :(
spotted ;-)
Comment 16 neil@parkwaycc.co.uk 2010-05-09 04:03:02 PDT
Comment on attachment 443648 [details] [diff] [review]
Patch v1.3 Fix nits. r=IanN

>-	      if ( GetNumSelectedMessages() == 1 && gDBView)
>+        if (GetNumSelectedMessages() == 1 && gDBView)
...mumble mumble irrelevant whitespace changes mumble mumble...

>-      filename = GenerateValidFilename(subject, ".eml");
Woohoo! Only one caller left ;-) [it's in editor/ui]

>+  if (num == 1 && gFolderDisplay.selectedMessageIsExternal)
[Not that you can have more than one selected external message!]

>+    return;
>+  } 
Nit: trailing space. Also a blank line after this block wouldn't go amiss.

>+  SaveAsFile(gFolderDisplay.selectedMessageUris);
Why can't you use GetSelectedMessages()?

>+// Emulate a fake StandaloneFolderDisplayWidget by extending gFolderDisplay
Um... not good; code sharing isn't really working here. If you can't use GetSelectedMessages() then this window needs its own gFolderDisplay object.
Comment 17 Philip Chee 2010-05-09 10:34:58 PDT
Created attachment 444313 [details] [diff] [review]
Patch v1.4 Addressed Issues. r=IanN

> (From update of attachment 443648 [details] [diff] [review])
>>-	      if ( GetNumSelectedMessages() == 1 && gDBView)
>>+        if (GetNumSelectedMessages() == 1 && gDBView)
> ...mumble mumble irrelevant whitespace changes mumble mumble...
I've put back the ugly \t(abs).

>>-      filename = GenerateValidFilename(subject, ".eml");
> Woohoo! Only one caller left ;-) [it's in editor/ui]
Yep.

>>+  if (num == 1 && gFolderDisplay.selectedMessageIsExternal)
> [Not that you can have more than one selected external message!]
Fixed. Also removed dependency on gFolderDisplay.selectedMessageIsExternal since this it doesn't work in the standalone message window with our current implementation if the message is an external .eml. I've used this instead:

if (/^file:/.test(aUris[0]))

>>+    return;
>>+  } 
> Nit: trailing space. Also a blank line after this block wouldn't go amiss.
Fixed.

>>+  SaveAsFile(gFolderDisplay.selectedMessageUris);
> Why can't you use GetSelectedMessages()?
I can. I just didn't think of it. Sorry. Fixed.

>>+// Emulate a fake StandaloneFolderDisplayWidget by extending gFolderDisplay
> Um... not good; code sharing isn't really working here. If you can't use
> GetSelectedMessages() then this window needs its own gFolderDisplay object.
Removed fake StandaloneFolderDisplayWidget.
Comment 18 neil@parkwaycc.co.uk 2010-05-09 13:09:08 PDT
Comment on attachment 444313 [details] [diff] [review]
Patch v1.4 Addressed Issues. r=IanN

>+  var num = aUris.length;
Nit: don't need this until the loop.

>+  if (/^file:/.test(aUris[0]))
Other places use /type=application\/x-message-display/. sr=me with this fixed.
Comment 19 Philip Chee 2010-05-10 02:01:28 PDT
Created attachment 444367 [details] [diff] [review]
Patch 1.4a Final nits r=IanN sr=Neil

Carrying forward r=IanN sr=Neil

> (From update of attachment 444313 [details] [diff] [review])
>>+  var num = aUris.length;
> Nit: don't need this until the loop.
Fixed.

>>+  if (/^file:/.test(aUris[0]))
> Other places use /type=application\/x-message-display/. sr=me with this fixed.
Fixed.
Comment 20 Jens Hatlak (:InvisibleSmiley) 2010-05-24 13:55:23 PDT
Moving Target Milestone forward since a1 has been released. Also removing checkin-needed now that Philip has the appropriate rights to check in himself (congrats!). If you need help, drop me a mail or grab me or anyone else on #seamonkey. If you really want someone else to check this in for you, just set checkin-needed one last time.
Comment 21 Philip Chee 2010-05-27 02:04:14 PDT
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/c3b77a8373b5

Note You need to log in before you can comment on or make changes to this bug.