> (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.

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.

(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.

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)

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 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 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.

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.

(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.

> (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 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.

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.

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