Closed Bug 555972 Opened 14 years ago Closed 14 years ago

Save multiple messages as individual files in directory

Categories

(SeaMonkey :: MailNews: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a2

People

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

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

> (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.
Depends on: 537448
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.
Attachment #435896 - Flags: review?(iann_bugzilla)
Attachment #435896 - Flags: feedback?(mnyromyr)
(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>
>> 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;
  }
);
(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! :-)
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
(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.
> 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.
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.
Attachment #435896 - Attachment is obsolete: true
Attachment #436545 - Flags: review?(iann_bugzilla)
Attachment #436545 - Flags: feedback?(mnyromyr)
Attachment #435896 - Flags: review?(iann_bugzilla)
Attachment #435896 - Flags: feedback?(mnyromyr)
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 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)
Attachment #436545 - Flags: review?(iann_bugzilla) → review-
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.
Attachment #436545 - Attachment is obsolete: true
Attachment #442451 - Flags: review?(iann_bugzilla)
Attachment #442451 - Flags: feedback?(mnyromyr)
Attachment #436545 - Flags: feedback?(mnyromyr)
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. ;-))
Attachment #442451 - Flags: feedback?(mnyromyr) → feedback+
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.
Attachment #442451 - Flags: review?(iann_bugzilla) → review+
Attached patch Patch v1.3 Fix nits. r=IanN (obsolete) — Splinter Review
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.
Attachment #442451 - Attachment is obsolete: true
Attachment #443648 - Flags: superreview?(neil)
Attachment #443648 - Flags: review+
(In reply to comment #14)
> 2. Fix one typo nobody spotter :(
spotted ;-)
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.
Attachment #443648 - Flags: superreview?(neil) → superreview-
> (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.
Attachment #443648 - Attachment is obsolete: true
Attachment #444313 - Flags: superreview?(neil)
Attachment #444313 - Flags: review+
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.
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.
Attachment #444313 - Attachment is obsolete: true
Attachment #444367 - Flags: superreview+
Attachment #444367 - Flags: review+
Attachment #444313 - Flags: superreview?(neil)
Keywords: checkin-needed
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.
Keywords: checkin-needed
Target Milestone: seamonkey2.1a1 → seamonkey2.1a2
Attachment #444367 - Attachment description: [for checkin] Patch 1.4a Final nits r=IanN sr=Neil → Patch 1.4a Final nits r=IanN sr=Neil
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/c3b77a8373b5
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.