Currently the Archive feature as implemented in SeaMonkey only supports archiving to a single folder or categorization by year and/or month. Thunderbird implemented a per-server option to keep the original folder structure of the messages to be archived in bug 522761. It's currently only a hidden pref, bug 573336 is about adding it to the Account Settings dialog. Preferably we should support that option internally before it hits there. TB changeset: http://hg.mozilla.org/comm-central/rev/7d1528f6c171 I already have a working version locally. :-) NB: I still had the MessageIDFinder extension installed in my trunk test profile. Would you have guessed that it replaces the two-param GetMsgFolderFromUri() function by one accepting only one param? Totally broke archiving. Fun times...

This ports what TB implemented, adjusted to our needs. Some notes: - TB already added the default pref to mailnews.js - actual example pref is mail.server.server1.archive_keep_folder_structure - modified OnStopRunningUrl in an attempt to fix bug 494266 in one go (got the idea from msgMail3PaneWindow.js folderObserver.onToggleOpenState which is what gets called when using the folder twisty which worked as a workaround before) - kept our identity handling for now since the change is part of the bug 512507 discussion (and I have no intention to call for the review over there before this here is done) or bug 518204, which depends on the former.

Comment on attachment 452608 [details] [diff] [review] implement pref handling >+ let copyBatchKey = msgHdr.folder.URI + '\000'; >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer >+ .perYearArchiveFolders) >+ copyBatchKey += msgYear; >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer >+ .perMonthArchiveFolders) >+ copyBatchKey += monthFolderName; >+ let keepFolderStructure = archiveFolder.server.archiveKeepFolderStructure; >+ if (keepFolderStructure) >+ copyBatchKey += msgHdr.folder.URI; Not sure why we need to fiddle with the batch key like this. At worst all will happen is that all the messages in a given year will be split up into batches of individual months. Adding the URI twice is definitely pointless. >+ let folderURI = srcFolder.URI.split(InitialFolderLevel.URI) >+ .toString().substr(1).split("/"); >+ if (folderURI[1] == "INBOX") This is a bit confusing. I see you want to remove the server URI from the folder URI, but you can use a substr/slice for that. The substr(1) looks like it should remove the separating / so I don't understand why the INBOX would end up as the second element in the array. >+ initFolderLevel = 2; Or maybe just shift the array down by 1.

(In reply to comment #2) > (From update of attachment 452608 [details] [diff] [review]) > >+ let copyBatchKey = msgHdr.folder.URI + '\000'; > >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer > >+ .perYearArchiveFolders) > >+ copyBatchKey += msgYear; > >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer > >+ .perMonthArchiveFolders) > >+ copyBatchKey += monthFolderName; > >+ let keepFolderStructure = archiveFolder.server.archiveKeepFolderStructure; > >+ if (keepFolderStructure) > >+ copyBatchKey += msgHdr.folder.URI; > Not sure why we need to fiddle with the batch key like this. At worst all will > happen is that all the messages in a given year will be split up into batches > of individual months. Adding the URI twice is definitely pointless. > > >+ let folderURI = srcFolder.URI.split(InitialFolderLevel.URI) > >+ .toString().substr(1).split("/"); > >+ if (folderURI[1] == "INBOX") > This is a bit confusing. I see you want to remove the server URI from the > folder URI, but you can use a substr/slice for that. The substr(1) looks like > it should remove the separating / so I don't understand why the INBOX would end > up as the second element in the array. > > >+ initFolderLevel = 2; > Or maybe just shift the array down by 1.

Grrr, submitted before it should. (In reply to comment #2) > Not sure why we need to fiddle with the batch key like this. At worst all will > happen is that all the messages in a given year will be split up into batches > of individual months. Adding the URI twice is definitely pointless. Agreed on not adding the URI twice, copied blindly from TB, sorry. Otherwise I think we can just as well leave it like that. Will change it if Karsten requires that, though. > >+ let folderURI = srcFolder.URI.split(InitialFolderLevel.URI) > >+ .toString().substr(1).split("/"); > >+ if (folderURI[1] == "INBOX") > This is a bit confusing. I see you want to remove the server URI from the > folder URI, but you can use a substr/slice for that. The substr(1) looks like > it should remove the separating / so I don't understand why the INBOX would end > up as the second element in the array. > > >+ initFolderLevel = 2; > Or maybe just shift the array down by 1. I think it's much better now. :-) [I begin to lose faith in TB reviewers.] If you wonder why I did the %20 replacement: srcFolder.URI contains that instead of a space if there is one in the folder name (which cannot happen for years or months). GetMsgFolderFromUri returns an object that has no parent property if that URI is passed unchanged (maybe RDF-related?). Thus createStorageIfMissing is called over and over again for that folder. If however the escape sequence is replaced by a space character, the returned object's parent property is set, i.e. the folder is found. FWIW: My IMAP server is Courier and I was archiving a single message from "Local Folders/test2/xxx abc/" to "IMAP-Server/INBOX/Archives/2010/test2/xxx abc/". I'm all ears if someone can tell me what's going on and whether there's a better fix (maybe more characters are affected?). (In reply to comment #1) > - modified OnStopRunningUrl in an attempt to fix bug 494266 in one go (got the > idea from msgMail3PaneWindow.js folderObserver.onToggleOpenState which is what > gets called when using the folder twisty which worked as a workaround before) It turned out that another part of the twisty-triggered functionality is responsible for that. EnsureFolderIndex does the trick, bug 494266 should be fixed now with that! :-)

(In reply to comment #4) > I think it's much better now. :-) > [I begin to lose faith in TB reviewers.] Only just now? ;-) > I'm all ears if someone can tell me what's going on and whether there's a > better fix (maybe more characters are affected?). Have you investigated other magic chars such as +, / or %? > It turned out that another part of the twisty-triggered functionality is > responsible for that. EnsureFolderIndex does the trick, bug 494266 should be > fixed now with that! :-) So the problem only occurred with collapsed folders?

Comment on attachment 452815 [details] [diff] [review] patch v2 (In reply to comment #0) > NB: I still had the MessageIDFinder extension installed in my trunk test > profile. Would you have guessed that it replaces the two-param > GetMsgFolderFromUri() function by one accepting only one param? Must be a hacked version anyway. Second, the crucial parts not covered by SM trunk are in Mnenhy now. Third, the original MIDF functions got warped when Markus wrote the trunk implementation. ;-) Back to business: >+++ b/suite/mailnews/mailWindowOverlay.js > let msgYear = msgDate.getFullYear().toString(); >- let dstFolderName = msgDate.toLocaleFormat("%Y-%m"); >- let copyBatchKey = msgHdr.folder.URI + '\000' + dstFolderName; >+ let monthFolderName = msgDate.toLocaleFormat("%Y-%m"); ... >+ let archiveFolder = GetMsgFolderFromUri(archiveFolderUri, false); >+ let granularity = archiveFolder.server.archiveGranularity; >+ let keepFolderStructure = archiveFolder.server.archiveKeepFolderStructure; >+ >+ let copyBatchKey = msgHdr.folder.URI + '\000'; >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer >+ .perYearArchiveFolders) >+ copyBatchKey += msgYear; >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer >+ .perMonthArchiveFolders) >+ copyBatchKey += monthFolderName; Like Neil, I don't see the point of doing extended interface voodoo just to avoid creating 12 batches instead of one. Just use URI+year+month as the key - which would also render most of your variables unnecessary, since you'd need them only inside the this._batches[copyBatchKey] initialization. >+ this._batches[copyBatchKey] = [msgHdr.folder, archiveFolderUri, >+ granularity, keepFolderStructure, >+ msgYear, monthFolderName]; Probably worth using one line per value if you use the variable values directly here. > processNextBatch: function() ... > let srcFolder = batch[0]; >+ let archiveFolderUri = batch[1]; >+ let granularity = batch[2]; >+ let keepFolderStructure = batch[3]; >+ let msgYear = batch[4]; >+ let msgMonth = batch[5]; How old school. ;-) Just use let [srcFolder, archiveFolderUri, granularity, keepFolderStructure, msgYear, msgMonth] = batch.slice(0, 5); >+ // Create the folder structure in Archives >+ if (keepFolderStructure) { Please adhere to the usual bracket style, here and below. >+ if (folders[0] == "INBOX") >+ folders.shift(); This surely is worth an explanatory comment... >+ for (let i = 0; i < folders.length; ++i) { >+ let folderName = folders[i].replace("%20", " "); This doesn't suffice/work. (And even if it would work, you'd want to use decodeURI(Component).) Just try a folder name like "f5 äöüß Ω", which will fail miserably (at least for me under Linux and local file system). >+ // Make sure the target folder is visible in the folder tree >+ EnsureFolderIndex(GetFolderTree().builderView, dstFolder); Would this scroll the folder into view? (It shouldn't, couldn't test this atm.) r- mainly for the folder creation issue, otherwise I can't wait to it in the tree. =:)

(In reply to comment #6) > let [srcFolder, archiveFolderUri, granularity, keepFolderStructure, msgYear, > msgMonth] = batch.slice(0, 5); (0, 6), of course.

Comment on attachment 452815 [details] [diff] [review] patch v2 >+ let copyBatchKey = msgHdr.folder.URI + '\000'; >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer >+ .perYearArchiveFolders) >+ copyBatchKey += msgYear; >+ if (granularity >= Components.interfaces.nsIMsgIncomingServer >+ .perMonthArchiveFolders) >+ copyBatchKey += monthFolderName; The thing I don't like about this is that you're still duplicating the year if you're archiving into monthly folders ;-) Perhaps use == or a switch instead? >+ dstFolder2 = GetMsgFolderFromUri(archiveFolderUri, false); ... >+ dstFolder = GetMsgFolderFromUri(archiveFolderUri, false); How is this different from dstFolder2? Assuming we made it through the loop, then surely we should have the correct folder by now? (If we didn't loop, then we must have been archiving from INBOX, in which case it's still correct, since it was prefilled with the correct value earlier in the code.)

(In reply to comment #7) > (In reply to comment #6) > > let [srcFolder, archiveFolderUri, granularity, keepFolderStructure, msgYear, > > msgMonth] = batch.slice(0, 5); > (0, 6), of course. Won't let slice for you anyway? But you could be sneakier and write let msgs = this._batches[key]; let [...] = msgs.splice(0, 6); since that removes the first 6 members from the array :-) If this was Perl you would of course write my ($srcFolder, $archivefolderUri, $granularity, $keepFolderStructure, $msgYear, $msgMonth, @msgs) = @_; but I don't think let can do that ;-)

(In reply to comment #5) > > I'm all ears if someone can tell me what's going on and whether there's a > > better fix (maybe more characters are affected?). > Have you investigated other magic chars such as +, / or %? Not yet. I was pretty sure I couldn't continue in that direction code-wise anyway. ;-) > > It turned out that another part of the twisty-triggered functionality is > > responsible for that. EnsureFolderIndex does the trick, bug 494266 should be > > fixed now with that! :-) > So the problem only occurred with collapsed folders? First of all it occurred with folders that had not been there before, i.e. newly created folder hierarchies. That's what I tested, and that's what's fixed. (In reply to comment #6) > >+ // Make sure the target folder is visible in the folder tree > >+ EnsureFolderIndex(GetFolderTree().builderView, dstFolder); > Would this scroll the folder into view? (It shouldn't, couldn't test this atm.) It doesn't. It just shows (opens) the whole folder hierarchy of the target folder.

(In reply to comment #8) > The thing I don't like about this is that you're still duplicating the year if > you're archiving into monthly folders ;-) Perhaps use == or a switch instead? I simply have this now, since Karsten wants to do completely without: let archiveFolder = GetMsgFolderFromUri(archiveFolderUri, false); let server = archiveFolder.server; let copyBatchKey = msgHdr.folder.URI + '\000' + monthFolderName; if (!(copyBatchKey in this._batches)) this._batches[copyBatchKey] = [msgHdr.folder, archiveFolderUri, server.archiveGranularity, server.archiveKeepFolderStructure, msgYear, monthFolderName]; > How is this different from dstFolder2? Well, looking at it again I agree we can do completely without dstFolder2. Sigh... (In reply to comment #9) > Won't let slice for you anyway? I don't think so. Found no evidence on MDC either. > But you could be sneakier and write > let msgs = this._batches[key]; > let [...] = msgs.splice(0, 6); > since that removes the first 6 members from the array :-) Yep, will go that way. With "// leaves messages in msgs" at the end of the last line. ;-) So, all that's left is the special characters stuff.

(In reply to comment #11) > let server = archiveFolder.server; Make that afServer (and below). ;-)

Created the following folder structures below Local Folders: l1/l2/f5 äöüß Ω/me+you, makes"2 % 1 = 0 l1/l2/f5 äöüß Ω/me+you, makes 2 % 1 = 0 srcFolder.URI.substr(rootFolderURI.length + 1) is then equal to: l1/l2/63d84c37/me+you%2C%20makes070caa9d l1/l2/63d84c37/me+you%2C%20makes%202%20%25%201%20%3D%200 So any foreign language character, and the double quote (and maybe more), makes it so that all spaces in the vicinity are not encoded using %20 anymore and instead some strange different encoding is applied. Neither decodeURI nor decodeURIComponent help with that: decodeURIComponent("/l1/l2/63d84c37/me+you%2C%20makes070caa9d") decodeURI("/l1/l2/63d84c37/me+you%2C%20makes070caa9d") decodeURIComponent("63d84c37") decodeURI("63d84c37") decodeURIComponent("me+you%2C%20makes070caa9d") decodeURI("me+you%2C%20makes070caa9d") decodeURIComponent("me+you%2C%20makes%202%20%25%201%20%3D%200") decodeURI("me+you%2C%20makes%202%20%25%201%20%3D%200") /l1/l2/63d84c37/me+you, makes070caa9d /l1/l2/63d84c37/me+you%2C makes070caa9d 63d84c37 63d84c37 me+you, makes070caa9d me+you%2C makes070caa9d me+you, makes 2 % 1 = 0 me+you%2C makes 2 % 1 %3D 0 Any idea what that is and how to decode that? Also I found that space decoding needs to happen for IMAP servers and must not happen for local folders, so for now I implemented this: let folderName = folders[i]; if (isImap) folderName = decodeURIComponent(folderName); archiveFolderUri += "/" + folderName; As long as no foreign language characters or double quotes are in the folder name this works. Maybe if this gets too complex we can leave it to a follow-up bug. BTW: One evil thing that we should probably address is that createStorageIfMissing is synchronous for local folders and dstFolder.parent is not re-checked after that call (for the other cases above the keepFolderStructure-if as well, of course) so e.g. EnsureFolderIndex can throw.

Comment on attachment 452815 [details] [diff] [review] patch v2 So, if we're keeping the folder structure, we need to be much more careful. In particular, we can't use the URI, since that's an internal implementation detail. Instead, we need to walk the ancestor chain, and call containsChildNamed on each step to see if there is a subfolder with the right ancestor folder name. Note: I suspect createSubfolder is asynchronous on IMAP.

(In reply to comment #14) > (From update of attachment 452815 [details] [diff] [review]) > So, if we're keeping the folder structure, we need to be much more careful. In > particular, we can't use the URI, since that's an internal implementation > detail. So at least I can stop thinking about ways how to decode it properly, then. ;-) > Instead, we need to walk the ancestor chain, and call > containsChildNamed on each step to see if there is a subfolder with the right > ancestor folder name. Thanks for the hint, I'll see what I can do. > Note: I suspect createSubfolder is asynchronous on IMAP. Most probably, yes. I did some checking and found that it calls nsIMsgFolderNotificationService.NotifyFolderAdded which I can write an nsIMsgFolderListener for and register it using nsIMsgFolderNotificationService.addListener(obj, nsIMsgFolderNotificationService.folderAdded). Too bad this isn't obvious from the IDL, I had to find it out all by myself. :-(

(In reply to comment #11) > (In reply to comment #9) > > But you could be sneakier and write > > let msgs = this._batches[key]; > > let [...] = msgs.splice(0, 6); > > since that removes the first 6 members from the array :-) > > Yep, will go that way. With "// leaves messages in msgs" at the end of the last > line. ;-) Turns out that's a bad idea since through the assignment msgs is the same object as what is at this._batches[key] and we need that unchanged when coming back from OnStopCopy. So slice it is, which unlike splice creates a one-level deep copy.

This is so much better. :-) Tested with: - account types: IMAP, Local Folders (source and target) - folders in hierarchy: - my "me+you, makes"2 % 1 = 0" - Karsten's "f5 äöüß Ω" - my "Daß Mäuse öfter übel riechen" (semi-sensible German group of words with some umlauts) - Archive from IMAP Inbox (now checking flag rather than name, also since folder.name is "Inbox", not "INBOX", and I don't even know how it looks localized) - yearly archiving active - target folder expansion works. I know the new folder creation logic could be extended to the Archives/year/month folders but for now I refrained from doing that, also because of the RSS special case and because it might make back-porting the move from server-based toward identity-based setup that the TB guys plan harder. On the other hand, the TB guys will probably back-port the changes made here. ;-)

Comment on attachment 454141 [details] [diff] [review] patch v3 "i18n" > QueryInterface: function(aIID) > { > if (aIID.equals(Components.interfaces.nsIUrlListener) || > aIID.equals(Components.interfaces.nsIMsgCopyServiceListener) || Shouldn't you add msIMsgFolderListener?

(In reply to comment #18) > (From update of attachment 454141 [details] [diff] [review]) > > QueryInterface: function(aIID) > > { > > if (aIID.equals(Components.interfaces.nsIUrlListener) || > > aIID.equals(Components.interfaces.nsIMsgCopyServiceListener) || > Shouldn't you add msIMsgFolderListener? Hmm, maybe? What's the point of this function anyway when it's not needed? Just for consistency?

FWIW, QueryInterface is called several times in my tests, but always with an aIID with these properties: - name = "" - number = "{00000000-0000-0000-c000-000000000046}" - equals(Ci.nsIUrlListener) = false - equals(Ci.nsIMsgCopyServiceListener) = false - equals(Ci.nsIMsgFolderListener) = false - equals(Ci.nsISupports) = true

Comment on attachment 454141 [details] [diff] [review] patch v3 "i18n" Just some nits: >+++ b/suite/mailnews/mailWindowOverlay.js > processNextBatch: function() ... >+ let [srcFolder, archiveFolderUri, granularity, keepFolderStructure, >+ msgYear, msgMonth] = batch.slice(0, 6); You can even drop the call to slice (I checked ;-) - only the relevant entries will be assigned). No real need to wrap the line then. > + // Create the folder structure in Archives > + // For imap folders, we need to create the sub-folders asynchronously, > + // so we chain the actions using the listener called back from > + // createSubfolder. For local, createSubfolder is synchronous. > + if (keepFolderStructure) No need to try to create the folder structure if archiveFolder.canCreateSubfolders is false. >+ let notificationService = Components.classes["@mozilla.org/messenger/msgnotificationservice;1"] >+ .getService(Components.interfaces.nsIMsgFolderNotificationService); >+ notificationService.removeListener(this); No need for the variable notificationService. r=me with that.

Also added the new interface to QI.

Comment on attachment 454394 [details] [diff] [review] patch v3a "i18n" [Checkin: comment 24] >+ let [srcFolder, archiveFolderUri, granularity, keepFolderStructure, msgYear, msgMonth] = batch; >+ let msgs = batch.slice(6); Ah, of course, splice doesn't work in the imap case.

Comment on attachment 454394 [details] [diff] [review] patch v3a "i18n" [Checkin: comment 24] http://hg.mozilla.org/comm-central/rev/74ccec25e4a9