856087 - Implement a pluggable store converter (mbox <-> maildir)

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Joshua Cranmer [:jcranmer]]

The basic goal here is to implement a generic component that can migrate a store (for local folders or offline caches) from one format to another. This component needs to be resilient to interruptions and other failures--the only record of users' mail may be at stake.

At a high level, the task is as follows:
1. Make a temporary directory for the new store.
2. Copy all the messages from the old store to the new store.
3. Atomically swap the stores and the pluggable store.
4. Delete the old store.

There are several complications at the low level that make this hard. A brief list of potential problems:
1. The new store and the old store should be on the same file system.
2. Free space could be an issue--we need at least twice the storage (where storage includes both the mail stores and potentially also the metadata as well).
3. What if users get new messages, delete messages, move messages, etc. while the store is being migrated? One option is completely locking down the folder while migration is going on; another option is handling changes in batches.
4. Local folders and offline storage are slightly different and may require different migration paths.
5. Notifications for the message copies/moves is tricky. This is closest in scope to a mail folder compaction (since message keys are changing but everything else remains the same--in theory).
6. What do we do about OS-integrated search indexing?
7. Progress indicators, and not blocking the main thread. It would be cool if this could be done off-main-thread, but that is almost completely impossible.
8. Our mail directories also contain other important files like popstate.dat.

If this is feasible in JS, then it would probably be better to implement this in JS than in C++. However, it is not clear that this is doable without some low-level changes to the current architecture, so it may be safer to assume C++.

Comment 1

[Atul Jangra [:atuljangra]]

Hey,
Can you elaborate the problem that will arise in Point 6?
6. What do we do about OS-integrated search indexing?

Thanks :-)

Comment 2

[:Irving Reid (No longer working on Firefox)]

Each OS has a different way of indexing the user's data. Using Mac OS's "Spotlight" as an example, if I search for "jcranmer" it may find his name in source files and in email messages. With properly integrated search indexing, Spotlight will know that when I click on an email message it should launch Thunderbird with the appropriate command line to display the message (as opposed to just displaying the message as a text file).

Comment 3

[Kent James (:rkent)]

As this is a rare operation that would always be started by a user manually, I wonder if it makes since in the interest of "just getting it done" to punt the issues of locking and notifications (which are very difficult to manage well) and just do the migration on Thunderbird startup before mailnews becomes functional.

Comment 4

[Joshua Cranmer [:jcranmer]]

(In reply to Atul Jangra [:atuljangra] from comment #1)
> Hey,
> Can you elaborate the problem that will arise in Point 6?
> 6. What do we do about OS-integrated search indexing?

I don't recall how the search indexing works offhand, but I know that at least one of the indexers creates individual file copies of the messages (the .mozmsg files). However, since maildir at least already has the separate copies, it doesn't need to have them copied over as well, but that also requires modifying logic in the search indexers which (to my knowledge) does not yet exist.

(In reply to Kent James (:rkent) from comment #3)
> As this is a rare operation that would always be started by a user manually,
> I wonder if it makes since in the interest of "just getting it done" to punt
> the issues of locking and notifications (which are very difficult to manage
> well) and just do the migration on Thunderbird startup before mailnews
> becomes functional.

To do your proposal correctly, we'd have to work before the biff manager starts up and before the messenger window is activated, and maybe more as well--it depends on what other code paths could trigger adding/moving/deleting messages.

Comment 5

[Wayne Mery (:wsmwk)]

And if conversion is done while user can interact with Thunderbird, needs to not overly impact UI responsiveness.

Comment 6

[Atul Jangra [:atuljangra]]

Yes, Wayne's point is actually important. I was wondering how are we going to handle UI. The conversion would surely be heavy, and thus user may experience some lag/ UI freeze.
Thus my question would be how are we going to handle this UI freeze? Can we somehow use thread switching for this part? I'm speaking over the head, but somehow, dividing the conversion work into chunks and then we process chunk-wise and giving control to main thread for UI between respective chunks.
jcranmer, Wayne, irving, rkent how do you guys generally handle this kind of situations?

Comment 7

[:Irving Reid (No longer working on Firefox)]

The "best" approach to this is to move all the I/O onto a background thread, so we only have to interrupt the main thread at the start and end of each message move. However, I don't think that will work very well with our architecture. I don't think the pluggable store can handle having part of a folder in one store type and part in another, so we'd need to keep the folder completely locked while it's being converted - we don't want either the user or new incoming mail to alter the folder while it's halfway between.

Within our existing architecture, the best approach would probably be to treat this like a bulk move of messages from an old folder to a new; I'm pretty sure we already have code to make sure the UI thread gets chances to interact while the messages are being moved.

Since this isn't a very common activity, I don't have that big a problem with it completely locking the UI while the folder is being converted; we'd still need to carefully design the UX but it mostly would just come down to good progress display and the ability to cancel (and have the old version available) at any time.

On the other hand, if we want to keep the UI responsive my first thought for an approach would be:

0) Lock out other activity on the folder (no UI, no checking the server for new, no filters)
1) Rename / repurpose the existing folder
2) Set up a new folder with all the appropriate properties (INBOX or other special flags, filters point here, etc)
3) Release the lock
4) Move all the old messages from the old folder to the new; new messages can also arrive during this, as long as we keep the date stamps and message IDs ordered correctly (new messages should be given higher message IDs than those moved, to preserve our "message ID order == order the messages arrived" behaviour)

Comment 8

[Joshua Cranmer [:jcranmer]]

(In reply to Atul Jangra [:atuljangra] from comment #6)
> Thus my question would be how are we going to handle this UI freeze? Can we
> somehow use thread switching for this part? I'm speaking over the head, but
> somehow, dividing the conversion work into chunks and then we process
> chunk-wise and giving control to main thread for UI between respective
> chunks.

Multithreading would be the ideal system, but our database isn't easily accessible from multiple threads.

A chunking algorithm (i.e., moving 1 message, process all pending events, move another...) is how this would need to be done on the main thread. Combined with an activity lockout (running this during startup, as rkent suggests, or developing locks on all methods), and some sort of visual progress mechanism (+ a safety mechanism if it gets interrupted), this should be sufficient for UI purposes.

Comment 9

[Magnus Melin [:mkmelin] (away until Aug 10)]

I wouldn't worry about keeping the UI responsible and such at all. Just warn the user before, and make everything disabled or something similar - it's a one time thing. Even with much effort put in to it, i don't think it's likely to be any "nice experience" for large data sets.

Comment 10

[Atul Jangra [:atuljangra]]

Maybe we do the following.
Initially lets warn the users and then do the converison, as suggested by Magnus. After we've implemented the conversion, we can then attack at the UI problem.

Also,
I am assuming that we need to do this in C++. I was looking for some code pointers to start. I was thinking of applying for this project as a part of GSoC this summer, if that's ok by Mozilla community.

Thanks :-)
P.S: Can I apply to two projects in Mozilla. I am interested in two projects, this one, and one in instantbird.

Comment 11

[Atul Jangra [:atuljangra]]

Regarding point 3, in initial comment, 
>>3. What if users get new messages, delete messages, move messages, etc. while the store is being migrated? One option is completely locking down the folder while migration is going on; another option is handling changes in batches.

I guess initial approach could be to freeze the folder when we are migrating, but later on we can improve this to handling such changes in batches. Basically waht I'm saying is that initially we should focus on creating the convertor and then we can improve the convertor and implement the batch handling.

Comment 12

[Mitesh Pathak [:mr_pathak]]

I would like to work on it!

Cheers
Mits

Comment 13

[Anirudh GP(:anirudhgp)]

Hi! I was wondering if i could help with this bug.
Thanks!

Comment 14

[Kent James (:rkent)]

This bug is currently likely as a Google Summer of Code project this summer. Were you looking at the GSOC project?

Comment 15

[Anirudh GP(:anirudhgp)]

(In reply to Kent James (:rkent) from comment #14)
> This bug is currently likely as a Google Summer of Code project this summer.
> Were you looking at the GSOC project?

Yes i was !

Comment 16

[Kent James (:rkent)]

Anirudh, if your primary interest is the GSOC project, then the path forward would be for you to apply through the GSOC project.

Comment 17

[Hrudaya Rn. Sahoo]

Hello,

I was actually browsing through the project ideas page of Mozilla and found that this project can be done by me. However, currently I have an intermediate knowledge of C++ and a (more than) beginner knowledge of javascript. Since I am very much interrested in doing this project, I actually wanted to know the level of difficulty of the project and what is the "min. knowledge" of javascript reqd. for this purpose? Thanks!

Comment 18

[Hrudaya Rn. Sahoo]

(In reply to Hrudaya Rn. Sahoo from comment #17)
> Hello,
> 
> I was actually browsing through the project ideas page of Mozilla and found

mproject ideas page of Mozilla for GSoC 2015

Comment 19

[Magnus Melin [:mkmelin] (away until Aug 10)]

This is a project that can be implemented in many ways. 

At least if you ask me you should do it mainly in JavaScript, especially as it's needs to happen in a different thread and it's just so much easier to get that right in js... 
Likely you should do it in a worker - look at https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_workers

Comment 20

[Sharad Gupta]

Hi sir, 
As i have developed many applications in javascript so i was wondering if i could contribute to this project as a GSOC project !!

Comment 21

[Vaibhav Bhosale]

Hey Josh,
Can I work on this project? I have worked on a few bugs earlier.
Thank You!

Comment 22

[Kent James (:rkent)]

Vaibhav: There is nobody currently planning to work on this project, so you are free to pursue it. I would be a more likely mentor than Joshua.

Just so you know, Mozilla did not get selected to participate in GSOC this year. So while this was one of our GSOC proposals, there is no GSOC funding available for this.

Comment 23

[Vaibhav Bhosale]

Hey Kent,
Yes, I would be willing to work on this project. Just that I have my exams till the end of this month, would like to start from the next month. Will that do?

Comment 24

[Kent James (:rkent)]

There are no current plans to work on this, so yes Vaibhav you are free to work on it.

Comment 25

[Bogdan Stefan]

Hello,

I see this project is still available, as it's listed on the GSoC 2016 ideas page.

I have some experience in JavaScript and a little C++, too.
I want to know the difficulty level of this project and if I can work on it for the GSoc 2016.
If so, can someone point me to a starting point?

Thank you!

Comment 26

[Magnus Melin [:mkmelin] (away until Aug 10)]

The pluggable store interface is here: http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgPluggableStore.idl

You'd create a new store, go through all the folders, stream messages from the old store adding them to to the new one. 
http://mxr.mozilla.org/comm-central/source/mailnews/local/test/unit/test_nsIMsgPluggableStore.js gives you an idea about how to create different stores.

Comment 27

[Magnus Melin [:mkmelin] (away until Aug 10)]

As XPCOM won't work in a worker, a solution mainly based on comment 19 might be best.

Comment 28

[Magnus Melin [:mkmelin] (away until Aug 10)]

Assigning to Anindya, as he's working on this for GSoC 2016.

Comment 29

[Anindya-Pandey]

Attachment: patch.diff

Implements mbox to maildir converter.

Comment 30

[:aceman]

How did I get selected as a reviewer? :) Yes, I can look at parts of this, but it will take some time now.

Comment 31

[Magnus Melin [:mkmelin] (away until Aug 10)]

I suggested he ask you to review :) I've been mentoring/commenting and such, so better to get some more eyes on it. 
Anyway, it seems to work in general - there are always things you can improve, but from a process perspective I think it would be good to land soon and do followups.

Comment 32

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 8783326 [details] [diff] [review]
patch.diff

Let's also have Richard have a look at the UI.

https://drive.google.com/open?id=1MySrWJ6d8rdVnev7WydnQHrYlwjn7RDrs6Gkik7hsRo has a bunch of screenshots of how it looks

Comment 33

[Richard Marti (:Paenglab)]

I think, we need a info that TB will restart after conversion to not make the user think TB crashed. This can be directly in the first dialog. The dialog needs also some UI love to look better (but this can be done in a followup bug).

I tried to convert a IMAP folder (only around 200 mails) multiple times (also with a restored profile) but it fails at different percentages.

I get this error message:

Thu Sep 08 2016 18:10:54
Error: Error: TypeError: path.startsWith is not a function
Source file: resource://app/modules/converterWorker.js
Line: 249

Comment 34

[Anindya-Pandey]

(In reply to Richard Marti (:Paenglab) from comment #33)
> I think, we need a info that TB will restart after conversion to not make
> the user think TB crashed. This can be directly in the first dialog. The
> dialog needs also some UI love to look better (but this can be done in a
> followup bug).
> 
> I tried to convert a IMAP folder (only around 200 mails) multiple times
> (also with a restored profile) but it fails at different percentages.
> 
> I get this error message:
> 
> Thu Sep 08 2016 18:10:54
> Error: Error: TypeError: path.startsWith is not a function
> Source file: resource://app/modules/converterWorker.js
> Line: 249

Thanks for testing it and thanks for your feedback.
path.startsWith is not used in converterWorker.js. So I think it is being called internally by some other function. I'll try and look for the source of the error.

Comment 35

[Richard Marti (:Paenglab)]

I forgot to mention, this was on Windows.

Comment 36

[Magnus Melin [:mkmelin] (away until Aug 10)]

The error is probably coming from the OS.File functions, like https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/components/osfile/modules/ospath_win.jsm#58, and likely Windows specific. 

Richard: if you have the (native) console open you should see a bunch of debug loggings, maybe some clues from there?

One other thing comes to mind: the OS.File functions return promises, so all the calls should yield to make sure the promise was resolved before moving on.

Comment 37

[Richard Marti (:Paenglab)]

I get this:
ErROR:anonymous/winGetDrive@resource://gre/modules/osfile/ospath_win.jsm:195:7
anonymous/join@resource://gre/modules/osfile/ospath_win.jsm:153:17
@resource:///modules/converterWorker.js:161:41
EventListener.handleEvent*@resource:///modules/converterWorker.js:6:1
 converterWorker.js:249:5
ErROR:anonymous/winGetDrive@resource://gre/modules/osfile/ospath_win.jsm:195:7
anonymous/join@resource://gre/modules/osfile/ospath_win.jsm:153:17
@resource:///modules/converterWorker.js:202:41
 converterWorker.js:249:5
ErROR:anonymous/winGetDrive@resource://gre/modules/osfile/ospath_win.jsm:195:7
anonymous/join@resource://gre/modules/osfile/ospath_win.jsm:153:17
@resource:///modules/converterWorker.js:202:41
EventListener.handleEvent*@resource:///modules/converterWorker.js:6:1
 converterWorker.js:249:5
ErROR:anonymous/winGetDrive@resource://gre/modules/osfile/ospath_win.jsm:195:7
anonymous/join@resource://gre/modules/osfile/ospath_win.jsm:153:17
@resource:///modules/converterWorker.js:161:41
EventListener.handleEvent*@resource:///modules/converterWorker.js:6:1

Comment 38

[:aceman]

Comment on attachment 8783326 [details] [diff] [review]
patch.diff

Review of attachment 8783326 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to be a nice big work.
I haven't yet run with the patch, but I have some comments.

Also, it seems the .dtd and properties files are missing for Seamonkey (/suite/*) . The functionality seems enabled also for SM (account manager is shared code), but the dialogs will break without the string files.

Thanks.

::: mail/locales/en-US/chrome/messenger/converterDialog.dtd
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!ENTITY converterDialog.title "Message Store Type Converter">

Do we no longer align the strings in a column?

@@ +6,5 @@
> +<!ENTITY converterDialog.continueButton "Continue">
> +<!ENTITY converterDialog.cancelButton "Cancel">
> +<!ENTITY converterDialog.finishButton "Finish">
> +<!ENTITY converterDialog.complete "The conversion is complete. Thunderbird will now restart.">
> +<!ENTITY converterDialog.done " done">

What is the leading space?

::: mail/locales/en-US/chrome/messenger/converterDialog.properties
@@ +4,5 @@
> +
> +converterDialog.warning=The messages in the account %S will now be converted to the maildir format.
> +converterDialog.message=Converting the account %S to maildir…
> +converterDialog.warningForOneDeferredAccount=%S is deferred to %S. The messages in the accounts %S will now be converted to the maildir format.
> +converterDialog.warningForDeferredAccounts=%S are deferred to %S. The messages in the accounts %S will now be converted to the maildir format.

I think all these %S need a LOCALIZATION NOTES (see in other files for the precise format of such notes) explaining what kind of data/object the placeholder represents.

::: mailnews/base/prefs/content/am-server.js
@@ +14,5 @@
> +/**
> + * Called when the store type menu is clicked.
> + */
> +function clickStoreTypeMenu() {
> +  var storeTypeElement = document.getElementById("server.storeTypeMenulist");

If you passed the element as argument, you would need to look it up here.

@@ +15,5 @@
> + * Called when the store type menu is clicked.
> + */
> +function clickStoreTypeMenu() {
> +  var storeTypeElement = document.getElementById("server.storeTypeMenulist");
> +  if (storeTypeElement.selectedItem.value == gOriginalStoreType.value) {

Would storeTypeElement.value work?

@@ +48,5 @@
> +      var newRootFolder = response.newRootFolder;
> +      var lastSlash = newRootFolder.lastIndexOf("/");
> +      var newsrc = newRootFolder.slice(0, lastSlash) + "/newsrc-" +
> +        newRootFolder.slice(lastSlash + 1);
> +      document.getElementById("nntp.newsrcFilePath").value = newsrc;  

Trailing space.

@@ +55,5 @@
> +    document.getElementById("server.localPath").value = response.newRootFolder;
> +    gOriginalStoreType = document.getElementById("server.storeTypeMenulist")
> +                                 .selectedItem;
> +    //document.getElementById("server.storeTypeMenulist")
> +    //        .setAttribute("disabled", "true");

In the comment above you say the menu should get disabled. Why is that needed. And then why is this code commented out?

@@ +103,5 @@
>                                 .getAttribute("value");
>    let targetItem = storeTypeElement.getElementsByAttribute("value", currentStoreID);
>    storeTypeElement.selectedItem = targetItem[0];
> +  // Initialise "gOriginalStoreType" to the item that was originally selected.
> +  gOriginalStoreType = storeTypeElement.selectedItem;

Do you really need the item element, or would the value suffice? storeTypeElement.value.

::: mailnews/base/prefs/content/am-server.xul
@@ +400,4 @@
>          <label value="&storeType.label;"
>                 accesskey="&storeType.accesskey;"
>                 control="server.storeTypeMenulist"/>
> +        <menulist id="server.storeTypeMenulist" 

Trailing space.

@@ +400,5 @@
>          <label value="&storeType.label;"
>                 accesskey="&storeType.accesskey;"
>                 control="server.storeTypeMenulist"/>
> +        <menulist id="server.storeTypeMenulist" 
> +                  oncommand="clickStoreTypeMenu();">

You could pass clickStoreTypeMenu(this) and then you would need to find the element again from inside the function.

::: mailnews/base/prefs/content/am-serverwithnoidentities.js
@@ +5,5 @@
>  
> +Components.utils.import("resource:///modules/iteratorUtils.jsm");
> +Components.utils.import("resource:///modules/MailUtils.js");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/BrowserUtils.jsm");

Are all these imports needed?

@@ +13,5 @@
> +
> +/**
> + * Called when the store type menu is clicked.
> + */
> +function clickStoreTypeMenu() {

Much of this code seems duplicated from am-server.js. Could it be shared?

@@ +78,3 @@
>    // disable store type change if store has already been used
> +  // storeTypeElement.setAttribute("disabled",
> +  //  gServer.getBoolValue("canChangeStoreType") ? "false" : "true");

Why is this just commented out?

::: mailnews/base/prefs/content/converterDialog.js
@@ +30,5 @@
> +// Array to hold deferred accounts.
> +var gDeferredAccounts = [];
> +
> +/**
> + * Place account name in migration diaog modal.

"dialog"

@@ +105,5 @@
> +      if (deferredToAccount.incomingServer.hostName == "Local Folders") {
> +        if (storeContractId == "@mozilla.org/msgstore/berkeleystore;1") {
> +          document.getElementById("warningSpan").innerHTML = bundle
> +            .formatStringFromName(
> +              "converterDialog.warningForOneDeferredAccount",

There are 6 blocks like this. Can it be formatted in less lines? Also, it seems to me only the string ID is changing so maybe you could just set the ID into a variable and than have the string displayed at the end of the 'if()else' blocks.

@@ +253,5 @@
> +  document.getElementById("warning").style.display = "none";
> +  document.getElementById("progressDiv").style.display = "block";
> +  document.getElementById("progressPrcnt").innerHTML =
> +    ((document.getElementById("progress").value /
> +      parseInt(document.getElementById("progress").max)) * 100) + "%";

This could be made localizable. I'm not sure all language just append the % sign like that.

::: mailnews/base/prefs/content/converterDialog.xhtml
@@ +45,5 @@
> +      </button>
> +    </div>
> +  </div>
> +  <div id="progressDiv" hidden="hidden">
> +    <p id="messageSpan">[Messages go here]</p>

Are these string placeholders ([...]) the proper usage pattern? Are there other samples like this in existing code?

::: mailnews/base/test/unit/test_converter.js
@@ +10,5 @@
> +Components.utils.import("resource://gre/modules/osfile.jsm");
> +Components.utils.import("resource://testing-common/mailnews/" +
> +                                                       "PromiseTestUtils.jsm");
> +Components.utils.import("resource:///modules/mailstoreConverter.jsm");
> +Components.utils.import("resource://gre/modules/Log.jsm");

Are all these imports needed?

::: mailnews/base/test/unit/test_converterDeferredAccount.js
@@ +10,5 @@
> +Components.utils.import("resource://gre/modules/osfile.jsm");
> +Components.utils.import("resource://testing-common/mailnews/PromiseTestUtils.jsm");
> +Components.utils.import("resource:///modules/mailstoreConverter.jsm");
> +Components.utils.import("resource://gre/modules/Log.jsm");
> +Components.utils.import("resource:///modules/folderUtils.jsm");

So many imports again?

@@ +97,5 @@
> +}
> +
> +function run_test() {
> +  localAccountUtils.loadLocalMailAccount();
> +  

Trailing space.

@@ +148,5 @@
> +      deferredToAccount = accounts[i];
> +    }
> +    // Else if the path of the rootMsgFolder of an account is the same as
> +    // "deferredToRootFolder", then we know that this account is a deferred
> +    // account.     

Trailing space.

@@ +169,5 @@
> +  // Assign "Inbox" folder of Local Folders to "gInbox".
> +  gInbox = localAccountUtils.inboxFolder;
> +  // Assign the incoming server of the account to which other accounts are
> +  // deferred ie "deferredToAccount" to "gServer".
> +  gServer = deferredToAccount.incomingServer;  

Trailing space.

::: mailnews/base/util/converterWorker.js
@@ +20,5 @@
> +    if (mailstoreContractId == "@mozilla.org/msgstore/maildirstore;1" &&
> +        stat.isDir && sourceFile.substr(-4) != ".sbd") {
> +      // If directory whose path is in "dest" does not exist, create it.
> +      OS.File.makeDir(dest, {from: tmpDir});
> +      var mboxFile;

Could these lower level variables be declared using 'let'?

::: mailnews/base/util/mailstoreConverter.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var EXPORTED_SYMBOLS = ["convertMailStoreTo", "terminateWorkers"];

Why are there so many .js files for the functionality? Did Magnus see use for some of the functions to be in separate modules?

::: mailnews/imap/test/unit/test_converterImap.js
@@ +2,5 @@
> +Components.utils.import("resource:///modules/iteratorUtils.jsm");
> +Components.utils.import("resource:///modules/MailUtils.js");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/FileUtils.jsm");
> +Components.utils.import("resource://gre/modules/osfile.jsm");

All needed?

Comment 39

[Magnus Melin [:mkmelin] (away until Aug 10)]

I'll answer a few of these

(In reply to :aceman from comment #38)
> Do we no longer align the strings in a column?

We don't :)
You quickly get the wrong intentions as you add/remove new entities.

> > +      parseInt(document.getElementById("progress").max)) * 100) + "%";
> 
> This could be made localizable. I'm not sure all language just append the %
> sign like that.

Not sure how you'd localize that. We certainly could leave it as a follow-up.

> > +    <p id="messageSpan">[Messages go here]</p>
> 
> Are these string placeholders ([...]) the proper usage pattern? Are there
> other samples like this in existing code?

Probably not that many examples, though there are at least one or two. All the texts are replaced of course, but this way you can see something while just working on the xhtml file statically.

> > +var EXPORTED_SYMBOLS = ["convertMailStoreTo", "terminateWorkers"];
> 
> Why are there so many .js files for the functionality? Did Magnus see use
> for some of the functions to be in separate modules?

The conversion functionality is in the .jsm file, and since it uses a worker the worker needs a separate worker js file. Then there is the last javascript file for purely dialog UI related functionality. So I don't think there is any redundancies.

Comment 40

[Richard Marti (:Paenglab)]

Comment on attachment 8783326 [details] [diff] [review]
patch.diff

ui-r- because of the missing restart information.

I tried the patch on OS X and it worked flawlessly. My previous issue is also only on Windows.

Comment 41

[:aceman]

(In reply to Magnus Melin from comment #39)
> > > +      parseInt(document.getElementById("progress").max)) * 100) + "%";
> > 
> > This could be made localizable. I'm not sure all language just append the %
> > sign like that.
> 
> Not sure how you'd localize that. We certainly could leave it as a follow-up.

A $S% string in properties file should work.

> > > +    <p id="messageSpan">[Messages go here]</p>
> > 
> > Are these string placeholders ([...]) the proper usage pattern? Are there
> > other samples like this in existing code?
> 
> Probably not that many examples, though there are at least one or two. All
> the texts are replaced of course, but this way you can see something while
> just working on the xhtml file statically.

OK, an exception for xhtml. We just must be sure the placeholders never show through (for a split second) in real usage.

> > > +var EXPORTED_SYMBOLS = ["convertMailStoreTo", "terminateWorkers"];
> > 
> > Why are there so many .js files for the functionality? Did Magnus see use
> > for some of the functions to be in separate modules?
> 
> The conversion functionality is in the .jsm file, and since it uses a worker
> the worker needs a separate worker js file. Then there is the last
> javascript file for purely dialog UI related functionality. So I don't think
> there is any redundancies.

So why is it in a worker? I hope the converter dialog is modal and we want to freeze other activity in TB. So the purpose of the worker probably isn't to offload the work to some other thread and have the main thread still interact with the user. So as such the converter could work directly from the dialog JS on the main thread. So is it for future-proofing where we may want to run the convertor backend at other places?

Comment 42

[Magnus Melin [:mkmelin] (away until Aug 10)]

It's not a good idea to do io work on the main thread. It would make even the migration dialog UI feel freezed. E.g. no responsiveness of buttons and displaying progress would be hard to do properly. And of course, it's also more future proof and faster since many workers run simultaneously.

Comment 43

[:aceman]

What is the next step here? Can you refresh the patch to incorporate Paenglab's and mine comments? I can then look at testing it more.

Comment 44

[Anindya-Pandey]

Attachment: patch2.diff.

Comment 45

[Anindya-Pandey]

(In reply to :aceman from comment #43)
> What is the next step here? Can you refresh the patch to incorporate
> Paenglab's and mine comments? I can then look at testing it more.

I think I incorporated all of your comments. I added the restart message to the first dialog as suggested by Richard. Could not rectify the bug on Windows. Visual Studio just won't install on my system and without it I can't build Thunderbird on Windows.

Comment 46

[Richard Marti (:Paenglab)]

Comment on attachment 8799650 [details] [diff] [review]
patch2.diff.

Review of attachment 8799650 [details] [diff] [review]:
-----------------------------------------------------------------

ui-r=me for the text changes. Needs a followup bug for nicer dialogs.

Still doesn't work on Windows as you wrote.

::: mail/locales/en-US/chrome/messenger/converterDialog.properties
@@ +4,5 @@
> +
> +# LOCALIZATION NOTE (converterDialog.warning):
> +# %1$S will be replaced by the name of the account which is going to be converted.
> +# %2$S will be replaced by the format into which the account will be converted.
> +converterDialog.warning=The messages in the account %1$S will now be converted to the %2$S format. Thunderbird will restart after the conversion is complete.

If you could use $BrandShortName instead of Thunderbird it would use the correct names on c-c and c-a.

Comment 47

[Jorg K (CEST = GMT+2)]

Comment on attachment 8799650 [details] [diff] [review]
patch2.diff.

Review of attachment 8799650 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few drive-by comments:
- This code is *very* hard to read. We usually use blank lines to separate
  little blocks of lines which achieve a certain goal. We don't glue it
  all together with no blank lines.
- There are many comments which explain the obvious. The winner is:
    // Assign "server" to "gServer".
    gServer = server;
  Please explain why you're doing something but not what you're doing if
  it's really obvious.
  Please look at other source files to see how our code looks like.
- The indentation isn't right in a few spots.
- There are a few lines of experimental code which is commented out.

Please go through the *entire* patch to fix the issues listed above.

::: mailnews/base/prefs/content/am-serverwithnoidentities.js
@@ +21,5 @@
> +  // response.newRootFolder will hold null.
> +  var response = { newRootFolder: null };
> +  // Send "response" as an argument to converterDialog.xhtml.
> +  window.openDialog("converterDialog.xhtml","mailnews:mailstoreconverter",
> +                   "modal,centerscreen,width=800,height=180", gServer, response);

Indentation.

@@ +44,5 @@
> +    gOriginalStoreType = document.getElementById("server.storeTypeMenulist")
> +                                 .value;
> +    BrowserUtils.restartApplication();
> +  }
> +  // Else if "response.newRootFolder" is null, we know that conversion failed or was

Otherwise we know ...

::: mailnews/base/prefs/content/converterDialog.js
@@ +37,5 @@
> +function placeAccountName(server) {
> +  var bundle = Components.classes["@mozilla.org/intl/stringbundle;1"]
> +    .getService(Components.interfaces.nsIStringBundleService)
> +    .createBundle("chrome://messenger/locale/converterDialog.properties");
> +  // Assign path of rootMsgFolder of "server" to "deferredToRootFolder".

Obvious comment.

@@ +62,5 @@
> +    // same as "deferredToRootFolder" ie the path of rootMsgFolder of "server",
> +    // then that account is deferred to "deferredToAccount".
> +    else if (accounts[i].incomingServer.rootMsgFolder.filePath.path ==
> +             deferredToRootFolder) {
> +      gDeferredAccounts[gDeferredAccounts.length] = accounts[i];

Use push.

@@ +116,5 @@
> +        .formatStringFromName(
> +          "converterDialog.warningForOneDeferredAccount",
> +            [deferredAccountsString,
> +              deferredToAccountName,
> +                accountsToConvert, storeContractId], 4);

Indentation.

@@ +125,5 @@
> +        .formatStringFromName(
> +          "converterDialog.warningForDeferredAccounts",
> +            [deferredAccountsString,
> +              deferredToAccountName,
> +                accountsToConvert, storeContractId], 4);

Indentation.

@@ +159,5 @@
> +      .formatStringFromName("converterDialog.message", [tempName,
> +        storeContractId], 2);
> +
> +    // Assign "server" to "gServer".
> +    gServer = server;

Obvious comment.

@@ +175,5 @@
> +
> +  document.getElementById("progress").max = "100";
> +  document.getElementById("progress").addEventListener("progress", function(e) {
> +   document.getElementById("progress").value = e.detail;
> +   document.getElementById("progressPrcnt").innerHTML = e.detail + "%";

Indentation.

::: mailnews/base/test/unit/test_converterDeferredAccount.js
@@ +96,5 @@
> +  localAccountUtils.loadLocalMailAccount();
> +
> +  // Create server for first deferred pop account.
> +  gServer1 = MailServices.accounts.createIncomingServer("test1", "localhost1",
> +                                                                       "pop3");

Indentation.

@@ +99,5 @@
> +  gServer1 = MailServices.accounts.createIncomingServer("test1", "localhost1",
> +                                                                       "pop3");
> +  // Create server for second deferred pop account.
> +  gServer2 = MailServices.accounts.createIncomingServer("test2", "localhost2",
> +                                                                       "pop3");

Indentation.

@@ +140,5 @@
> +    // be deferred to this account.
> +    if (accounts[i].incomingServer.rootFolder.filePath.path ==
> +      deferredToRootFolder) {
> +      // Assign the account to "deferredToAccount".
> +      deferredToAccount = accounts[i];

Obvious comment.

@@ +148,5 @@
> +    // account.
> +    else if (accounts[i].incomingServer.rootMsgFolder.filePath.path ==
> +      deferredToRootFolder) {
> +      // Add the account to "deferredAccounts".
> +      deferredAccounts[deferredAccounts.length] = accounts[i];

Use push here.

@@ +162,5 @@
> +  accountsToConvert = accountsToConvert +
> +    deferredToAccount.incomingServer.username;
> +  log.info(accountsToConvert + " will be converted");
> +  // Assign "Inbox" folder of Local Folders to "gInbox".
> +  gInbox = localAccountUtils.inboxFolder;

Obvious comment.

::: mailnews/base/util/converterWorker.js
@@ +143,5 @@
> +        while (true) {
> +          // Get the current position in "sourceFile" into "position".
> +          position = sourceFileOpen.getPosition();
> +          // Read "noOfBytes" bytes from "sourceFileOpen" into "array".
> +          let array = sourceFileOpen.read(noOfBytes);

Obvious comment.

@@ +145,5 @@
> +          position = sourceFileOpen.getPosition();
> +          // Read "noOfBytes" bytes from "sourceFileOpen" into "array".
> +          let array = sourceFileOpen.read(noOfBytes);
> +          // Decode "array" into a string and assign it to "textNew".
> +          textNew = decoder.decode(array);

Obvious comment.

@@ +156,5 @@
> +          // If "nextPos==prevPos" and "nextLen=prevLen", we know that we
> +          // have reached the last message in "sourceFile".
> +          if (nextPos == prevPos && nextLen == prevLen) {
> +            // If "text" is not null.
> +            if (text !== null) {

Obvious comment.

@@ +176,5 @@
> +              // index of the first "From - " match and the end of "text".
> +              targetFile.write(encoder.encode(text.substring(lastPos[0],
> +                  text.length + 1)));
> +              // Close "targetFile".
> +              targetFile.close();

Obvious comment.

@@ +183,5 @@
> +              // in the account is more than 0 and mailstore type is mbox.
> +              self.postMessage(["copied", name, " " + position]);
> +            }
> +            // Break out of the loop.
> +            break;

Obvious comment.

@@ +188,5 @@
> +          }
> +          // Else we might have more messages in "sourceFile".
> +          else {
> +            // Set "prevPos" to "nextPos".
> +            prevPos = nextPos;

Obvious comment.

@@ +190,5 @@
> +          else {
> +            // Set "prevPos" to "nextPos".
> +            prevPos = nextPos;
> +            // Set "prevLen" to "nextLen".
> +            prevLen = nextLen;

Obvious comment.

@@ +192,5 @@
> +            prevPos = nextPos;
> +            // Set "prevLen" to "nextLen".
> +            prevLen = nextLen;
> +            // Set "text" to "textNew"
> +            text = textNew;

Obvious comment.

@@ +211,5 @@
> +            for (let i = 0; i < msgPos.length - 1; i++) {
> +              // Create and open a new file in dest/destFile/cur with name
> +              // "name" to hold the next mail and assign it to "targetFile".
> +              targetFile = OS.File.open(OS.Path.join(dest, destFile, "cur",
> +                  name), {write: true, create: true}, {});

Obvious comment.

@@ +215,5 @@
> +                  name), {write: true, create: true}, {});
> +              // Extract the text lying between consecutive indices, encode
> +              // it and write it to "targetFile".
> +              targetFile.write(encoder.encode(text.substring(msgPos[i],
> +                 msgPos[i + 1])));

Indentation.

::: mailnews/base/util/mailstoreConverter.jsm
@@ +99,5 @@
> +    var count = 0;
> +    var contents = folder.directoryEntries;
> +    while (contents.hasMoreElements()) {
> +      var content = contents.getNext().QueryInterface(Components.interfaces
> +                                                                     .nsIFile);

Indentation.

@@ +164,5 @@
> +  var isMaildir = (mailstoreContractId == "@mozilla.org/msgstore/maildirstore;1");
> +  // If mailstore type is maildir and server type is not "nntp".
> +  if (isMaildir && server.type != "nntp") {
> +    // Get no. of msgs in "count".
> +    count = countMaildirMsgs(accountRootFolder);

Obvious comment.

@@ +166,5 @@
> +  if (isMaildir && server.type != "nntp") {
> +    // Get no. of msgs in "count".
> +    count = countMaildirMsgs(accountRootFolder);
> +    // If no. of msgs is 0.
> +    if (count == 0) {

Obvious comment.

@@ +170,5 @@
> +    if (count == 0) {
> +      // Set "count" to the no. of files and folders in account root folder and
> +      // set "zeroMessages" to true.
> +      count = countMaildirZeroMsgs(accountRootFolder);
> +      zeroMessages = true;

Obvious comment.

@@ +177,5 @@
> +  // Else if server type is pop or none ( for Local Folders ).
> +  else if (server.type == "pop3" || server.type == "none" || server.type ==
> +    "movemail") {
> +    // Get no. of msgs in "count".
> +    count = server.rootFolder.getTotalMessages(true);

Obvious comment.

@@ +179,5 @@
> +    "movemail") {
> +    // Get no. of msgs in "count".
> +    count = server.rootFolder.getTotalMessages(true);
> +    // If no. of msgs is 0.
> +    if (count == 0) {

Obvious comment.

@@ +183,5 @@
> +    if (count == 0) {
> +      // Set "count" to no. of files and folders in account root folder and set
> +      // "zeroMessages" to true.
> +      count = countImapFileFolders(accountRootFolder);
> +      zeroMessages = true;

Obvious comment.

@@ +187,5 @@
> +      zeroMessages = true;
> +    }
> +  }
> +  else if (server.type == "imap" || server.type == "nntp") {
> +    //var msgWindow = MailServices.mailSession.topmostMsgWindow;

Remove experimental code left-overs.

@@ +216,5 @@
> +  var subDir = function(folder, destPath) {
> +    var contents = folder.directoryEntries;
> +    while (contents.hasMoreElements()) {
> +      var content = contents.getNext().QueryInterface(Components.interfaces
> +                                                              .nsIFile);

Indentation.

@@ +292,5 @@
> +            (responseWorker != "copied" &&
> +             (server.type != "pop3" && server.type != "none" &&
> +              server.type != "movemail" && !isMaildir)) ||
> +            (responseWorker != "copied" &&
> +             server.type == "nntp")) {

The if above is impossible to read. Please break this into lines differently or use indentation to show what's going on.

@@ +320,5 @@
> +              converterFolder.remove(true);
> +            }
> +            try {
> +              //OS.File.move(dir.path,parent.path+"/"+dir.leafName,
> +              //                                        { noOverwrite : false });

Remove experimental code left-overs.

@@ +343,5 @@
> +                converterFolderMsf.remove(true);
> +              }
> +              var oldRootFolderMsf = new FileUtils.File(OS.Path.join(
> +                parent.path,accountRootFolder.leafName + ".msf"));
> +              //OS.File.copy(oldRootFolderMsf.path, converterFolderMsf.path);

Remove experimental code left-overs.

@@ +356,5 @@
> +                converterFolderNewsrc.remove(true);
> +              }
> +              var oldNewsrc = new FileUtils.File(OS.Path.join(parent.path,
> +                "newsrc-" + accountRootFolder.leafName));
> +              //OS.File.copy(oldRootFolderMsf.path, converterFolderMsf.path);

Remove experimental code left-overs.

@@ +406,5 @@
> +                "mail.server." + server.key + ".directory-rel"));
> +            if (!isMaildir) {
> +              Services.prefs.setCharPref(
> +                "mail.server." + server.key + ".storeContractID",
> +                  "@mozilla.org/msgstore/maildirstore;1");

Indentation.

@@ +411,5 @@
> +            }
> +            else {
> +              Services.prefs.setCharPref(
> +                "mail.server." + server.key + ".storeContractID",
> +                  "@mozilla.org/msgstore/berkeleystore;1");

Indentation.

Comment 48

[Jorg K (CEST = GMT+2)]

Running this on Windows I successfully converted maildir to mbox, but when I try to convert mbox back to maildir I get:
1477264397862   MailStoreConverter      ERROR   Error at resource:///modules/converterWorker.js:261 - Error: TypeError: path.startsWith is not a function

That was discussed before. Line 261 is in a catch block. After taking the try/catch out, I get:
1477265333083   MailStoreConverter      ERROR   Error at resource://gre/modules/
workers/require.js line 139 > Function:195 - TypeError: path.startsWith is not a function

C:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\bin\modules\workers\require.js line 139 reads:
        let code = new Function("exports", "require", "module",
          source + "\n//# sourceURL=" + uri + "\n"
        );

Function:195 refers to this:
https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/components/osfile/modules/ospath_win.jsm#195

I can't work out what's going on. Looks like JS is trying to load ospath_win.jsm into a new Function??

My JS is pretty bad, so I don't know where to look further, besides, 2:35 AM here now.

Comment 49

[Magnus Melin [:mkmelin] (away until Aug 10)]

I imagine path is either undefined or unexpectedly some kind of other type. The check just checks for null - https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/components/osfile/modules/ospath_win.jsm#191 but it would be useful to know what "path" actually is. (You can console.dir(path); to output into the Error console). Of course, after that we need to figure out why it gets into the wrong state.

Hmm, looking at the code, it might be that it's a number? JS is usually relaxed with type checking, but maybe if the patch would use

let name = Date.now().toString(); 
  instead of 
let name = Date.now();

... it could work.

Comment 50

[Magnus Melin [:mkmelin] (away until Aug 10)]

Or maybe better just toString() name before passing in to OS.Path.join

Comment 51

[Jorg K (CEST = GMT+2)]

Yes, I changed the two calls like this
targetFile = OS.File.open(OS.Path.join(dest, destFile, "cur",
  name.toString()), {write: true, create: true}, {});
and it ran through. This improvement is so great that it's now 500% complete!!
Message:
  The conversion is complete. Thunderbird will now restart. - 500% done.
Oh, I ran it again, this time it was 166% done.
That's for conversion from mbox to maildir.

Can someone explain to me why this works on Linux and Mac?

One question:
Given that the code has obvious formal problems, it has obvious real problems and was actually never tested thoroughly, since until now, it didn't work on Windows, and given that the panel also doesn't look right, I don't think this has "production strength" just yet. Has anyone tried converting a dataset of - say - 2GB size, since this is what our users will be doing?

Speaking as the release manager for Tb 52 Aurora/Beta, I'd prefer not to have this in the release. Can we land this after Nov. 7th?

Comment 52

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #51)
> Can someone explain to me why this works on Linux and Mac?
Sorry, I'm tired, it was 2:30 AM last night. This of course only fails on Windows since this code is only run on Windows.

Comment 53

[Magnus Melin [:mkmelin] (away until Aug 10)]

Like I wrote earlier, there may be things to improve and bugs to fix, but it's more useful to do after landing. 

It could be useful to put this feature behind a pref until we have more beta-testing done for it, but I'd still like to see it land so people can provide real feedback. Otherwise you have the same issue for the next release.

Comment 54

[Wayne Mery (:wsmwk)]

I tend to agree with Magnus - improve what we can and put it behind a pref, then move forward. Plus strings need to land.

Comment 55

[Jorg K (CEST = GMT+2)]

Funny, Magnus is against preferences in general. Now we do one just to disable a semi-cooked feature.

(In reply to Magnus Melin from comment #53)
> Otherwise you have the same issue for the next release.
Well, the system is called the "trains". If this lands in TB 53, it will be in Daily for six weeks, then Aurora then Beta. That gives us time to address the follow-up bugs. Otherwise we will need to fix this during the TB 52 ESR cycle.

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #54)
> Plus strings need to land.
No strings need to land now if this lands on TB 53 in two weeks time. We're already talking about follow up bugs, so how do I know that those don't need new strings?

Anyway, it's not ready to land now. Once it works and doesn't show 166% complete and passes review we can talk about this more.

Comment 56

[Wayne Mery (:wsmwk)]

It is desirable to try to get this this into version 52.  If there is a process whereby this can be accomplished we should put in the effort. And prefs are accepted practice for landing things which are risky or incomplete.  Ask kent.

Comment 57

[Jorg K (CEST = GMT+2)]

OK, to put a positive spin on this, I tried it with about 2 GB of data.
Mbox->Maildir took four minutes and ended with 100% complete.
Maildir->Mbox took about five minutes, also ended 100% complete. That was on an SSD.
So it works ;-)

The 500% complete or 166% complete (reproducible) only showed up on small amounts of data. Perhaps something is miscounted.

What I don't like is that the original data is left behind, so you convert 2 GB and end up with 2x 2 GB.

If we want to accelerate, I could fix the long list of formal problems (comment #47) (and the trailing whitespace, etc.) so we can save many loops of complaining about nits while the author can focus on any remaining code issues.

Let me know, I'm great at clean-up ;-)

We should also fix the panel, it's too big mainly because the font is too big. Richard, don't you think so? Have you looked at why it's so big?

Comment 58

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #57)
> We should also fix the panel, it's too big mainly because the font is too
> big. Richard, don't you think so? Have you looked at why it's so big?

Not deeply but I think it's because it's a HTML page and the default font for HTML is bigger than the system UI font.

Comment 59

[Kent James (:rkent)]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #56)
> It is desirable to try to get this this into version 52.  If there is a
> process whereby this can be accomplished we should put in the effort. And
> prefs are accepted practice for landing things which are risky or
> incomplete.  Ask kent.

I have not really looked at this code (other than to look briefly at the diff and notice it has a number of issues). Things get ugly near string freeze time though as we try to land stuff that we hope to clean up later. Unfortunately hope rarely is realized, but there is always a chance, right? Hiding behind a pref at least gives us the option!

So I have no problems with adding a pref as part of an experimental feature. But if you do that, you should really pref it on in trunk, and pref it off later in beta or esr, particularly if we are asking for localization. If the quality is too low even for that, maybe this feature is ready.

That being said, I kinda wish this had been done in an extension initially.

Comment 60

[Magnus Melin [:mkmelin] (away until Aug 10)]

(In reply to Jorg K (GMT+2) from comment #57)
> If we want to accelerate, I could fix the long list of formal problems
> (comment #47) (and the trailing whitespace, etc.) so we can save many loops
> of complaining about nits while the author can focus on any remaining code
> issues.
> 
> Let me know, I'm great at clean-up ;-)

Anindya, let use know how you want to proceed.

Comment 61

[Anindya-Pandey]

(In reply to Magnus Melin from comment #60)
> (In reply to Jorg K (GMT+2) from comment #57)
> > If we want to accelerate, I could fix the long list of formal problems
> > (comment #47) (and the trailing whitespace, etc.) so we can save many loops
> > of complaining about nits while the author can focus on any remaining code
> > issues.
> > 
> > Let me know, I'm great at clean-up ;-)
> 
> Anindya, let use know how you want to proceed.

I think it would be better if Jorg could fix the formal problems.
If the 500% / 166% complete bugs are reproducible I would like to go through the debug loggings.

Comment 62

[Jorg K (CEST = GMT+2)]

I take it that we want to replace 'var' with 'let' where possible and replace
  for (let i = 0; i < accounts.length; i++) {
with
  for (let account of accounts) {
Right?

Comment 63

[Jorg K (CEST = GMT+2)]

BTW, what about test_converterImap.js. Does that work? It never runs:
+[test_converterImap.js]
+# Disabled until bug 870864 is resolved
+skip-if = true
Guess what, bug 870864 *is* resolved.

Comment 64

[Magnus Melin [:mkmelin] (away until Aug 10)]

You can use let if you prefer, there are occasional cases where it makes a difference.
For the for-loops, I suppose both versions are ok and a matter of taste.

Comment 65

[Jorg K (CEST = GMT+2)]

Attachment: mbox-to-maildir-160-percent.txt

Here's the log of a conversion that was 160% done.
At first, I couldn't reproduce it and always got 100%. Then I deleted some messages without compacting the folder. Now I got 160% which seems like 8 of 5 ;-)

I'll do the clean-up within the next 24 hours, so you'd have to integrate the fix with the new patch I will be sending.

Comment 66

[Jorg K (CEST = GMT+2)]

Attachment: mbox-maildir.patch

OK, I had a first stab at this. I did all files apart from:
converterDialog.js
converterWorker.js
mailstoreConverter.jsm
These three will follow later today.

Please note the logic error in test_converterDeferredAccount.js

Comment 67

[Jorg K (CEST = GMT+2)]

I ran the three tests:
mozilla/mach xpcshell-test mailnews/imap/test/unit/test_converterImap.js
That failed with:
ERROR ReferenceError: deferredToRootFolder is not defined at c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/_tests/xpcshell/mailnews/base/test/unit/test_converterDeferredAccount.js:138
I noticed that problem and marked it with XXX.

mozilla/mach xpcshell-test mailnews/base/test/unit/test_converter.js
That failed at first with not finding the bugmail10 file. I corrected that to:
add_task(function* setupMessages() {
  let msgFile = do_get_file("../../../data/bugmail10");
Now it almost works:
Ran 1008 tests (1 parents, 1007 subtests)
Expected results: 1004
Unexpected results: 4 (FAIL: 4)

mozilla/mach xpcshell-test mailnews/imap/test/unit/test_converterImap.js
That fails:
"JavaScript error: resource://testing-common/mailnews/maild.js, line 132: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsISocketTransport.openInputStream]"

Anindya, what is the status of those tests? Looks like they all fail for some reason or another. Or did I break something?

Comment 68

[Jorg K (CEST = GMT+2)]

Correction:
The first test I ran was:
mozilla/mach xpcshell-test mailnews/base/test/unit/test_converterDeferredAccount.js (not imap).
If failed with deferredToRootFolder not defined.

However, when I run the original version (after adding the .toString()) I also run into the problem that it can't find the bugmail10 file. Fixing that also, I get:

Ran 1008 tests (1 parents, 1007 subtests)
Expected results: 1004
Unexpected results: 4 (FAIL: 4)

Somehow it doesn't run into deferredToRootFolder not being set. Hmm.

Anindya, could you fix the tests before I proceed with fixing formal issues in the remaining three files.
converterDialog.js
converterWorker.js
mailstoreConverter.jsm
It would be good to be able to check my editing work with *working* tests, so I have a working base to start at. Like this, I keep going back and forward between the original and my refactored version and it's not clear whether the error already existed or my edits introduced it.

Please start with the patch I've attached and fix the file paths:
  let msgFile = do_get_file("../../../data/bugmail10");

Comment 69

[Jorg K (CEST = GMT+2)]

Sorry, I can see it now, I introduced the error with changing 'var' to 'let' and the variable goes out of scope.

Comment 70

[Jorg K (CEST = GMT+2)]

Attachment: mbox-maildir.patch (JK v2).

OK, I restored the 'var' where necessary.

Test status:
test_converterDeferredAccount.js and test_converter.js both give:
Ran 1008 tests (1 parents, 1007 subtests)
Expected results: 1004
Unexpected results: 4 (FAIL: 4)

The imap test fails in many weird an wonderful ways.

So, Anindya, can you fix the tests first or should I continue with the main three files that need clean-up?
converterDialog.js
converterWorker.js
mailstoreConverter.jsm

Comment 71

[:aceman]

(In reply to Jorg K (GMT+2) from comment #65)
> Here's the log of a conversion that was 160% done.
> At first, I couldn't reproduce it and always got 100%. Then I deleted some
> messages without compacting the folder. Now I got 160% which seems like 8 of
> 5 ;-)

Did it correctly migrate only the 5 non-deleted msgs?

Comment 72

[:aceman]

(In reply to Jorg K (GMT+2) from comment #69)
> Sorry, I can see it now, I introduced the error with changing 'var' to 'let'
> and the variable goes out of scope.

I would rather declare the variable with let some levels above, so it covers all its uses. I do not like variables escaping their scope blocks like in this case.

Comment 73

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #72)
> I would rather declare the variable with let some levels above, so it covers
> all its uses. I do not like variables escaping their scope blocks like in
> this case.
I'll let the author fix any functional and logic errors.

Comment 74

[Jorg K (CEST = GMT+2)]

Attachment: mbox-maildir.patch (JK v3).

Fixed converterDialog.js.
I tossed the restoring of preferences. If one had changed, *all* were restored. So why not just restore all of them anyway? If that's not wanted, they could be compared/restored individually.

Still to go: converterWorker.js and mailstoreConverter.jsm.

Comment 75

[Jorg K (CEST = GMT+2)]

Attachment: mbox-maildir.patch (JK v4).

Fixed converterWorker.js.

The author likes to talk about this variables in comments, like:
// If the directory with path "dest" does not exist, create it.
This is highly confusing since he doesn't mean the literal string "dest", but the value of the variable 'dest'. Another example:
// Create and open a new file in dest/destFile/cur
Here 'dest' and 'destFile' refer to variables, "cur" is a literal.

I tried to remove variable names from comments as much as possible and refer to the remaining ones using single quotes to distinguish them from literal strings which use double quotes.

Still to go mailstoreConverter.jsm.

(In reply to :aceman from comment #71)
> Did it correctly migrate only the 5 non-deleted msgs?
I let the reviewer figure that out ;-)

Comment 76

[Anindya-Pandey]

test_converterImap.js is working for me after importing mbox-maildir.patch ( JK v3). The other two are failing but I know how to correct them. I'll attach a patch after mbox-maildir.patch (JK v5 ) which should be the last patch removing the formal problems.

Comment 77

[Jorg K (CEST = GMT+2)]

Yes, v5 is coming.

Comment 78

[Anindya-Pandey]

Did the 160% problem happen with an imap or a pop account ?

Comment 79

[Jorg K (CEST = GMT+2)]

Pop.

Comment 80

[Jorg K (CEST = GMT+2)]

Attachment: mbox-maildir.patch (JK v5).

Fixed last file.

Test status: The non-IMAP tests fail as before, the IMAP test works(!!) after fixing the path to bugmail10.

I flagged a heap of issues with XXX in the code.

Also still to solve: the 8 of 5 problem (160% done).

Over to you, Anindya.

P.S.: I hope you appreciate that I invested a day's work to move this forward. I also hope that you see that the code is a whole lot more readable now. Please deliver further patches in this quality.

Comment 81

[Jorg K (CEST = GMT+2)]

Let me make some general comments about coding style and especially commenting code. The main aim of commenting code it to make it easier for other people to understand why you are doing something and not what you're doing (if its obvious). The people reading the code can - well - read code.

So
  // Assign 5 to 'a'.
  a = 5;
is unnecessary, however
  // We need to start with at small prime number since ...
  a = 5;
can add value.

It's really funny since you had some many redundant comments which I just removed, however, some stuff that wasn't obvious, like:
  let newsrcRel = Services.prefs.getCharPref(p4);
  lastSlash = newsrcRel.lastIndexOf("/");
  newsrcRel = newsrcRel.slice(0, lastSlash) + "/" + newNewsrc.leafName;
  Services.prefs.setCharPref(p4, newsrcRel);
didn't have a comment. Looking at it again the comment should possibly be:
  // Replace filename in relative path.
or words to that extent.

I removed many if/else blocks like this.

  // If a==5, do stuff.
  if (a == 5) {
    doStuff();
  }
  // Else a !=5, so bail out
  else {
    return;
  }

There is no value in repeating the condition in comment and there is also no value in explaining what the else case is (since we can all negate the original condition). There are two options here which you can use.

Explain the whole if/else block before:

  // Check for small prime, otherwise bail.
  if (a == 5) {
    doStuff();
  } else {
    return;
  }

or explain each part individually:

  if (a == 5) {
    // We found a small prime, so do it.
    doStuff();
  } else {
    // Too bad, nothing to do.
    return;
  }

You really created a lot of complicated software here even with xpcshell tests and promises (which most/some of our developers fear). The software works, even when stressed with 2 GB of data, however, the nice quality was hidden in a whole heap of very hard-to-read code.

A little off-topic: There is this initiative going on to encourage people to learn to program. They like to take the nerdy notion away and compare it to art and emphasize the creativity. So as a book author puts beautiful words together to form beautiful sentences that fill a book, we write beautiful software that is nice to look at and even works ;-)

Comment 82

[Jorg K (CEST = GMT+2)]

Comment on attachment 8804433 [details] [diff] [review]
mbox-maildir.patch (JK v5).

Review of attachment 8804433 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/test/unit/test_converterDeferredAccount.js
@@ +124,5 @@
> +    // of the accountto which other accounts are deferred.
> +    var deferredToRootFolder = gServer1.rootMsgFolder.filePath.path;
> +    log.info(gServer1.username + " is a deferred account");
> +  }
> +  // XXX TODO: Logic error here. deferredToRootFolder may not be set!!!

This should check that gServer1.rootFolder.filePath.path != gServer1.rootMsgFolder.filePath.path and then assign deferredToRootFolder, so:

// 'gServer1' should be deferred. Get the path of the root folder
// to which other accounts are deferred.
ok(gServer1.rootFolder.filePath.path != gServer1.rootMsgFolder.filePath.path);
let deferredToRootFolder = gServer1.rootMsgFolder.filePath.path;

Comment 83

[:aceman]

Comment on attachment 8804433 [details] [diff] [review]
mbox-maildir.patch (JK v5).

Review of attachment 8804433 [details] [diff] [review]:
-----------------------------------------------------------------

So in addition to the nits below there are these bigger problems:
1. the above 100% progress Jorg was seeing. I have seen it too when folders contained deleted messages.
2. you create a new directory for the new store (suffixed -maildir). After the process you do not delete the old directory. I think you should tell it to the user (after all there is private data stored in there which he will no longer see via TB UI).
3. what happens if you switch the store back from maildir to mbox? You get a dir suffixed -mbox. Then if you switch back? You put it into the -maildir again. Do you remove the directory before that? Or just put new files into it, possibly mixing with old files? TB will pick up all files it finds at next restart.
4. why do you actually care about account deferred to the converted account and tell them to the user? AFAIK you do not actually convert their original directories. And if they defer to an account it should be obvious they store to the store of that account, whatever it is (will be). Why is it important to note?
5. Please warn the user about the disk space requirement for the new store. You will need permanently the amount of disk as the old store did (in addition to the old one) so this needs to be communicated. You should even check if there is so much disk space and abort before start of conversion.
6. I tried to convert an account that had a single 150MB message in the Trash folder. It took several minutes and TB took about 1.5GB of RAM in the process. That is quite unexpected when the operation should just be to read a big file from disk and write it consecutively to another file (my disk should do that in ~3 seconds). Can you test what takes the time and RAM? Is using a JS string ineffective? Maybe some of the typed arrays would be faster? Or is it the encoder.encode() process? What is it actually encoding? You are writing the whole message in a single call here. Maybe that could be chunked too. When I copied that same message into the same account but different folder, and converted again, TB took above 2GB of RAM (can't say exactly, as it even ate all swap space). So do the memory requirements accumulate over all messages? Does the process in converterWorker.js leak memory, or not release it (via JS garbage collection) fast enough?

Please fix the comments starting with XXX. Explain why the code does what it does there.

All function arguments should be named starting with 'a'.

And please fix the tests Jorg mentioned.

But the backend of the actual conversion of the store seems to work great, thanks.

::: mail/locales/en-US/chrome/messenger/converterDialog.dtd
@@ +6,5 @@
> +<!ENTITY converterDialog.continueButton "Continue">
> +<!ENTITY converterDialog.cancelButton "Cancel">
> +<!ENTITY converterDialog.finishButton "Finish">
> +<!-- XXX Can we use $BrandShortName instead of Thunderbird? -->
> +<!ENTITY converterDialog.complete "The conversion is complete. Thunderbird will now restart.">

Yes, you have to use &brandShortName; here. But make sure the .xul file also references brand.dtd. See e.g. removeAccount.xul .

@@ +10,5 @@
> +<!ENTITY converterDialog.complete "The conversion is complete. Thunderbird will now restart.">
> +<!-- LOCALIZATION NOTE (converterDialog.done):
> +  The space before 'done' is essential since this is part of a progress display, for example: '67% done'
> +  -->
> +<!ENTITY converterDialog.done " done">

You can't compose strings in this way. You must have a string "%S%% done" in a properties file and then use it via *bundle.getFormattedString("percent_done", [percentage]) .

@@ +11,5 @@
> +<!-- LOCALIZATION NOTE (converterDialog.done):
> +  The space before 'done' is essential since this is part of a progress display, for example: '67% done'
> +  -->
> +<!ENTITY converterDialog.done " done">
> +<!ENTITY converterDialog.error "Conversion Failed.">

I think this needs lowercase 'failed'.

::: mail/locales/en-US/chrome/messenger/converterDialog.properties
@@ +4,5 @@
> +
> +# LOCALIZATION NOTE (converterDialog.warning):
> +# %1$S will be replaced by the name of the account which is going to be converted.
> +# %2$S will be replaced by the format into which the account will be converted.
> +converterDialog.warning=The messages in the account %1$S will now be converted to the %2$S format. Thunderbird will restart after the conversion is complete.

Thunderbird needs to be a placeholder.

@@ +16,5 @@
> +# %1$S will be replaced by the name of the account which is deferred.
> +# %2$S will be replaced by the name of the account to which the account is deferred.
> +# %3$S will be replaced by a comma separated list of names of accounts which are going to get converted.
> +# %4$S will be replaced by the format into which the account will be converted.
> +converterDialog.warningForOneDeferredAccount=%1$S is deferred to %2$S. The messages in the accounts %3$S will now be converted to the %4$S format. $BrandShortName will restart after the conversion is complete.

I think for '.properties' strings you need to make $BrandShortName another placeholder %5$S and replace it similar to all the other placeholders.

@@ +23,5 @@
> +# %1$S will be replaced by a comma separated list of names of accounts which are deferred.
> +# %2$S will be replaced by the name of the account to which the accounts are deferred.
> +# %3$S will be replaced by a comma separated list of names of accounts which are going to get converted.
> +# %4$S will be replaced by the format into which the account will be converted.
> +converterDialog.warningForDeferredAccounts=%1$S are deferred to %2$S. The messages in the accounts %3$S will now be converted to the %4$S format. Thunderbird will restart after the conversion is complete.

You can't have a singular and 1 plural string, that is not how plurals are done for all languages. See PluralForm.jsm and a sample in bug 1313950.

::: mailnews/base/prefs/content/am-server.js
@@ -4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -Components.utils.import("resource:///modules/iteratorUtils.jsm");
> -Components.utils.import("resource:///modules/MailUtils.js");
> -Components.utils.import("resource://gre/modules/Services.jsm");

Why got these removed?

@@ +8,5 @@
>  
> +/**
> + * Called when the store type menu is clicked.
> + */
> +function clickStoreTypeMenu(storeTypeElement) {

aStoreTypeElement

@@ +27,5 @@
> +/**
> + * Revert store type to the original store type if converter modal closes
> + * before migration is complete, otherwise change original store type to
> + * currently selected store type.
> + * @param response - response from migration dialog modal.

What is the contents of 'response'? It is an object? Write it here. And aResponse.

@@ -47,5 @@
>    let targetItem = storeTypeElement.getElementsByAttribute("value", currentStoreID);
>    storeTypeElement.selectedItem = targetItem[0];
> -  // disable store type change if store has already been used
> -  storeTypeElement.setAttribute("disabled",
> -    gServer.getBoolValue("canChangeStoreType") ? "false" : "true");

So what do you do with this server attribute now? Some backend code sets is to false but you still allow conversion now. That will be counter-intuitive. But you can remove that variable in a new bug once this migration wizard is tested in the field :)

::: mailnews/base/prefs/content/am-serverwithnoidentities.js
@@ +10,5 @@
> +
> +/**
> + * Called when the store type menu is clicked.
> + */
> +function clickStoreTypeMenu(storeTypeElement) {

Document the storeTypeElement argument. And call it aStoreTypeElement.

@@ +29,5 @@
> +/**
> + * Revert store type to the original store type if converter modal closes
> + * before migration is complete, otherwise change original store type to
> + * currently selected store type.
> + * @param response - response from migration dialog modal.

What is the contents of 'response'? It is an object? Write it here.

@@ +31,5 @@
> + * before migration is complete, otherwise change original store type to
> + * currently selected store type.
> + * @param response - response from migration dialog modal.
> + */
> +function changeStoreType(response) {

aResponse

::: mailnews/base/prefs/content/converterDialog.js
@@ +33,5 @@
> +/**
> + * Place account name in migration dialog modal.
> + * @param {nsIMsgIncomingServer} server - account server.
> + */
> +function placeAccountName(server) {

aServer

@@ +64,5 @@
> +
> +  // String to hold the names of accounts to be converted separated by commas.
> +  let accountsToConvert = "";
> +
> +  if (gDeferredAccounts.length >= 1) {

> 0 ?

@@ +78,5 @@
> +    }
> +
> +    // Username of Local Folders is "nobody". So it's better to use
> +    // its hostname which is "Local Folders".
> +    if (deferredToAccount.incomingServer.hostName == "Local Folders") {

I think there must be a better way to detect the Local folders account. Maybe
deferredToAccount.incomingServer.key != MailServices.accounts.localFoldersServer.key

@@ +81,5 @@
> +    // its hostname which is "Local Folders".
> +    if (deferredToAccount.incomingServer.hostName == "Local Folders") {
> +      accountsToConvert += ", " + deferredToAccount.incomingServer.hostName;
> +    } else {
> +      accountsToConvert += ", " + deferredToAccount.incomingServer.username;

Anyway, why do you fetch usernames in multiple places in this function? The accounts have their proper name so what do you use username for?

@@ +100,5 @@
> +    if (deferredToAccountName != "Local Folders") {
> +      deferredToAccountName = deferredToAccount.incomingServer.username;
> +    }
> +
> +    if (gDeferredAccounts.length == 1) {

This is not how you make plural forms. Please see PluralForm.jsm.

@@ +153,5 @@
> + * Start the conversion process.
> + * @param {nsIMsgIncomingServer} server - server for account to be migrated.
> + * @param {Object} response - response from the migration dialog modal.
> + */
> +function startContinue(server, response) {

aServer, aResponse

@@ +157,5 @@
> +function startContinue(server, response) {
> +  gResponse = response;
> +  gFolder = gServer.rootFolder.filePath;
> +
> +  document.getElementById("progress").max = "100";

Why is this attribute not set on the progress element in the html file?

@@ +165,5 @@
> +  });
> +
> +  document.getElementById("warning").style.display = "none";
> +  document.getElementById("progressDiv").style.display = "block";
> +  document.getElementById("progressPrcnt").innerHTML =

Why are there 2 places (one is above) that set the percentage string?

@@ +175,5 @@
> +  let p1 = "mail.server." + gServer.key + ".directory";
> +  let p2 = "mail.server." + gServer.key + ".directory-rel";
> +  let p3 = "mail.server." + gServer.key + ".newsrc.file";
> +  let p4 = "mail.server." + gServer.key + ".newsrc.file-rel";
> +  let p5 = "mail.server." + gServer.key + ".storeContractID";

I think these can be retrieved from the server directly, without this hardcoding. As last resort e.g. gServer.getCharValue("directory") should work. Or were those accessors unusable for some reason?

@@ +193,5 @@
> +    gServer, document.getElementById("progress"));
> +  pConvert.then(function(val) {
> +    let newMailstoreContractId;
> +    if (originalStoreContractID == "@mozilla.org/msgstore/berkeleystore;1") {
> +      newMailstoreContractId = "@mozilla.org/msgstore/maildirstore;1";

This works when we have only 2 store types. But why guess this here? We know what the user has chosen in the menulist so why not propagate the value to all the needed places?

@@ +205,5 @@
> +      let defServer = deferredAccount.incomingServer;
> +      defServer.rootMsgFolder.filePath = new FileUtils.File(val);
> +      if (originalStoreContractID == "@mozilla.org/msgstore/berkeleystore;1") {
> +        Services.prefs.setCharPref("mail.server." + defServer.key +
> +          ".storeContractID", "@mozilla.org/msgstore/maildirstore;1");

Again guessing the storetype.

@@ +212,5 @@
> +          ".storeContractID", "@mozilla.org/msgstore/berkeleystore;1");
> +      }
> +    }
> +
> +    Services.io.offline = false;

What does this do? What if we were offline before the conversion? You must not randomly toggle TB to offline (multiple occurrences).

@@ +216,5 @@
> +    Services.io.offline = false;
> +    document.getElementById("cancel").style.display = "none";
> +    document.getElementById("finish").style.display = "inline-block";
> +    document.getElementById("messageSpan").style.display = "none";
> +    document.getElementById("completeSpan").style.display = "block";

So if this is or new non-XUL future... :(

@@ +244,5 @@
> +/**
> + * Cancel the conversion.
> + * @param {Object} response - response param from the migration dialog modal.
> + */
> +function cancelConversion(response) {

aResponse

::: mailnews/base/prefs/content/converterDialog.xhtml
@@ +27,5 @@
> +  </style>
> +  <script src="converterDialog.js" />
> +  <script>
> +    window.addEventListener("load", function() {
> +      placeAccountName(window.arguments[0]);

Can this be put into <body onload=""> ?

@@ +51,5 @@
> +    <p id="errorSpan" hidden="hidden">&converterDialog.error;</p>
> +    <p>
> +      <progress id="progress" value="0"></progress>
> +      <span id="progressPrcnt">[XXX %]</span>
> +      <span>&converterDialog.done;</span>

Must be a single element with a single string, as already described.

::: mailnews/base/test/unit/test_converter.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please call this file test_mailstoreConverter.js. Converter is too generic.

::: mailnews/base/test/unit/test_converterDeferredAccount.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Does this need to be a separate test file? Can it be folded into test_converter.js and just run the functions twice with an argument which creates a deferred account in one of the runs? Or is there a large difference? This relates to my question above why are deferred accounts so special.

::: mailnews/base/util/converterWorker.js
@@ +63,5 @@
> +    } else if (stat.isDir) {
> +      // A directory is encountered. This is a ".sbd" directory.
> +
> +      // If directory with path 'dest' does not exist, create it.
> +      // XXX Comment suggests that there is a test.

What does this mean?

@@ +111,5 @@
> +        // no. of msgs in the account is 0 and mailstore type is maildir.
> +        self.postMessage(["msfdat", sourceFile]);
> +
> +      } else if (mailstoreContractId != "@mozilla.org/msgstore/maildirstore;1") {
> +        // XXX Comment?

What should go here?

@@ +130,5 @@
> +        let targetFile = null;
> +        // Get a timestamp for file name.
> +        let name = Date.now();
> +        // No. of bytes to be read from the source file.
> +        let noOfBytes = 10000000;

Please comment here that it wants to be a large size to read in chunks.

@@ +134,5 @@
> +        let noOfBytes = 10000000;
> +        // 'text' holds the string that was read.
> +        let text = null;
> +        // Index of last match in 'text'.
> +        let j;

So could this be named better?

@@ +181,5 @@
> +
> +              // Extract the text in 'text' between 'lastPos[0]' ie the
> +              // index of the first "From - " match and the end of 'text'.
> +              targetFile.write(encoder.encode(text.substring(lastPos[0],
> +                text.length + 1))); // XXX: + 1??

So why +1 ?

@@ +187,5 @@
> +
> +              // Send a message indicating that a message was copied.
> +              // This indicates progress for a pop account if the no. of msgs
> +              // in the account is more than 0 and mailstore type is mbox.
> +              self.postMessage(["copied", name, " " + position]);

Why the space?

@@ +210,5 @@
> +          }
> +
> +          if (msgPos.length > 1) {
> +            // More than one "From - " match is found.
> +            noOfBytes = 10000000;

If this is the same value as in the beginning of the file, make a constant for it.

@@ +219,5 @@
> +                name.toString()), {write: true, create: true}, {});
> +              // Extract the text lying between consecutive indices, encode
> +              // it and write it.
> +              targetFile.write(encoder.encode(text.substring(msgPos[i],
> +                msgPos[i + 1])));

This didn't fit on a single line?

::: mailnews/base/util/mailstoreConverter.jsm
@@ +16,5 @@
> +log.level = Log.Level.Debug;
> +log.addAppender(new Log.DumpAppender(new Log.BasicFormatter()));
> +
> +var FileUtils = Components.utils.import("resource://gre/modules/FileUtils.jsm")
> +                                .FileUtils;

Is this needed? Doesn't FileUtils.jsm already export FileUtils var?

@@ +43,5 @@
> +  var tmpDir = FileUtils.getDir("TmpD", [], false);
> +  // Array to hold path to the Converter folder in tmp dir.
> +  var pathArray;
> +  // No. of messages that have been copied in case of a pop account.
> +  // No. of files and folders that have been copied in case of an imap account.

What about other account types?

@@ +125,5 @@
> +      if (content.isDirectory() && content.leafName.substr(-4) != ".sbd") {
> +        var cur = FileUtils.File(OS.Path.join(content.path,"cur"));
> +        var curContents = cur.directoryEntries;
> +        while (curContents.hasMoreElements()) {
> +          curContent = curContents.getNext();

Undeclared variable. Do you use it for anything? Would just calling curContents.getNext() work?

@@ +227,5 @@
> +        // If it is not a ".sbd" directory we know that it is a maildir msg
> +        // folder.
> +        if (content.leafName.substr(-4) != ".sbd") {
> +          // Array to hold unsorted list of maildir msg filenames.
> +          dataArrayUnsorted = []

Declare the variable with 'let' and add missing ; .

@@ +473,5 @@
> +    }
> +  }
> +  // Go offline before conversion, so there aren't messages coming in during
> +  // the process.
> +  Services.io.offline = true;

Shouldn't we save the current value of offline to restore it after the process?

::: mailnews/imap/test/unit/test_converterImap.js
@@ +16,5 @@
> +
> +var log = Log.repository.getLogger("MailStoreConverter");
> +
> +var FileUtils = Components.utils.import("resource://gre/modules/FileUtils.jsm")
> +                                .FileUtils;

Needed?

::: mailnews/jar.mn
@@ +63,5 @@
>      content/messenger/SmtpServerEdit.js                                        (base/prefs/content/SmtpServerEdit.js)
>      content/messenger/smtpEditOverlay.xul                                      (base/prefs/content/smtpEditOverlay.xul)
>      content/messenger/smtpEditOverlay.js                                       (base/prefs/content/smtpEditOverlay.js)
> +    content/messenger/converterDialog.xhtml                                    (base/prefs/content/converterDialog.xhtml)
> +    content/messenger/converterDialog.js                                       (base/prefs/content/converterDialog.js)

This does not apply today, needs update.

Comment 84

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #83)

I'm responsible for some of this, so allow me some replies:

> > +  let p1 = "mail.server." + gServer.key + ".directory";
> > +  let p2 = "mail.server." + gServer.key + ".directory-rel";
> > +  let p3 = "mail.server." + gServer.key + ".newsrc.file";
> > +  let p4 = "mail.server." + gServer.key + ".newsrc.file-rel";
> > +  let p5 = "mail.server." + gServer.key + ".storeContractID";
> 
> I think these can be retrieved from the server directly, without this
> hardcoding. As last resort e.g. gServer.getCharValue("directory") should
> work. Or were those accessors unusable for some reason?

I introduced these variables since the RHS expressions were just copied over and over and over and over ;-)

> So if this is or new non-XUL future... :(
You mean: So if this is OUR new non-XUL future.

> > +      // If directory with path 'dest' does not exist, create it.
> > +      // XXX Comment suggests that there is a test.
> 
> What does this mean?

I added this comment. The comment says: "If directory with path 'dest' does not exist ...", but there is no test in the code. So the comment needs to change or there is something missing in the code.

> > +        // no. of msgs in the account is 0 and mailstore type is maildir.
> > +        self.postMessage(["msfdat", sourceFile]);
> > +
> > +      } else if (mailstoreContractId != "@mozilla.org/msgstore/maildirstore;1") {
> > +        // XXX Comment?
> 
> What should go here?

I think a comment explaining what the 'else if' branch does would be good.

Also, there is the XXX Logic error which I think should be fixed according to comment #82.

Comment 85

[Anindya-Pandey]

Attachment: patch6.diff

Comment 86

[Anindya-Pandey]

The 160 % completion error was because the root folder was being compacted before conversion. The compaction is working now and the conversion works properly even after deleting or moving messages.

Comment 87

[Anindya-Pandey]

(In reply to Anindya-Pandey from comment #86)
> The 160 % completion error was because the root folder was being compacted
> before conversion. The compaction is working now and the conversion works
> properly even after deleting or moving messages.

sorry, the error was because the root folder was NOT* being compacted

Comment 88

[Anindya-Pandey]

>+++ b/mail/locales/en-US/chrome/messenger/converterDialog.dtd
>@@ -0,0 +1,16 @@
>+<!-- XXX Can we use $BrandShortName instead of Thunderbird? -->
>+<!ENTITY converterDialog.complete "The conversion is complete. Thunderbird will now restart.">

$BrandShortName did not work in the dtd file. Can I transfer this string to the properties file that is being used for the converter ?

>+++ b/mailnews/base/prefs/content/am-serverwithnoidentities.js
>@@ -1,27 +1,71 @@
>+
>+/**
>+ * Revert store type to the original store type if converter modal closes
>+ * before migration is complete, otherwise change original store type to
>+ * currently selected store type.
>+ * @param response - response from migration dialog modal.
>+ */
>+function changeStoreType(response) {
>+  if (response.newRootFolder) {
>+    // The conversion is complete.
>+    // Set local path to the new account root folder which is present
>+    // in 'response.newRootFolder'.
>+    document.getElementById("server.localPath").value = response.newRootFolder;
>+    gOriginalStoreType = document.getElementById("server.storeTypeMenulist")
>+                                 .value;
>+    BrowserUtils.restartApplication();
>+  } else {
>+    // The conversion failed or was cancelled.
>+    // Restore selected item to what was selected before conversion.
>+    // XXX TODO: Cleanup.
>+    document.getElementById("server.storeTypeMenulist").value =
>+      gOriginalStoreType;
>+  }
>+}

What is supposed to be cleaned up here ? The prefs are already reverted in converterDialog.js right ?
 

>+++ b/mailnews/imap/test/unit/test_converterImap.js
>@@ -0,0 +1,152 @@
>+function run_test() {
>+  setup();
>+  // XXX TODO: Replace old asynch framework with promises.
>+  async_run_tests(tests);
>+}

I tried to make the test work using promises but I haven't been able to do that so far.

Comment 89

[Anindya-Pandey]

I still need to incorporate aceman's comments.
I'm sorry for taking so much time. There's just too much work because of my end-semester exams.

Comment 90

[Jorg K (CEST = GMT+2)]

(In reply to Anindya-Pandey from comment #88)
> $BrandShortName did not work in the dtd file.
I'm no specialist on this. Ask Aceman. I think he suggests to use &brandShortName; (see comment #83).

> What is supposed to be cleaned up here ?
That's referring to all the files on disk the process creates. Say you do a mbox to maildir conversion and create a gazillion little maildir files. Then the conversion fails. What happens? Also, in the "good" case, do you ever remove the original source data?

> I tried to make the test work using promises but I haven't been able to do
> that so far.
I know, it's terrible. Last time I had do do it, I got Kent James to do it for me, see bug 629738. Take a look at test_cacheParts.js, also in the obsolete attachments there. This started as an "async" test and Kent made it a "promise" test in no time at all.

> I'm sorry for taking so much time. There's just too much work because of
> my end-semester exams.
No problem. You missed getting into TB 52, but that's life.

Comment 91

[:aceman]

(In reply to Anindya-Pandey from comment #87)
> (In reply to Anindya-Pandey from comment #86)
> > The 160 % completion error was because the root folder was being compacted
> > before conversion. The compaction is working now and the conversion works
> > properly even after deleting or moving messages.
> 
> sorry, the error was because the root folder was NOT* being compacted

Just do not try to run compact as part of the migration.
To calculate correct percentage, use the number of messages you are going to copy (the not deleted ones), not total number of messages (including deleted). Or are you totally bypassing the msf database which could tell you this number beforehand?

Comment 92

[Jorg K (CEST = GMT+2)]

Clearly missed all deadlines.

Comment 93

[Anindya-Pandey]

Attachment: patch7.diff

Comment 94

[Anindya-Pandey]

(In reply to :aceman from comment #83)
> Comment on attachment 8804433 [details] [diff] [review]
> mbox-maildir.patch (JK v5).
> 
> Review of attachment 8804433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So in addition to the nits below there are these bigger problems:
> 1. the above 100% progress Jorg was seeing. I have seen it too when folders
> contained deleted messages.
> 2. you create a new directory for the new store (suffixed -maildir). After
> the process you do not delete the old directory. I think you should tell it
> to the user (after all there is private data stored in there which he will
> no longer see via TB UI).

Still need to fix this.

> 3. what happens if you switch the store back from maildir to mbox? You get a
> dir suffixed -mbox. Then if you switch back? You put it into the -maildir
> again. Do you remove the directory before that? Or just put new files into
> it, possibly mixing with old files? TB will pick up all files it finds at
> next restart.

Yes the directory is removed before conversion.

> 4. why do you actually care about account deferred to the converted account
> and tell them to the user? AFAIK you do not actually convert their original
> directories. And if they defer to an account it should be obvious they store
> to the store of that account, whatever it is (will be). Why is it important
> to note?

I think it is possible that a user tries to convert an account that is deferred to some other account or tries to convert an account to which other accounts are deferred. In both the cases only the deferred-to account which contains the messages is actually converted and the mailstore type of all accounts deferred to this account are changed. The only thing special about handling deferred accounts is that, the user should be notified about all the accounts which are deferred to the deferred-to account so that if the user does not want the messages of any one or more of the deferred accounts to be converted he/she can cancel the conversion because converting the deferred-to account will convert the messages belonging to every deferred account and the deferred-to account.

Please let me know if there is something wrong here.

> 5. Please warn the user about the disk space requirement for the new store.
> You will need permanently the amount of disk as the old store did (in
> addition to the old one) so this needs to be communicated. You should even
> check if there is so much disk space and abort before start of conversion.

Still need to fix this.

> 6. I tried to convert an account that had a single 150MB message in the
> Trash folder. It took several minutes and TB took about 1.5GB of RAM in the
> process. That is quite unexpected when the operation should just be to read
> a big file from disk and write it consecutively to another file (my disk
> should do that in ~3 seconds). Can you test what takes the time and RAM? Is
> using a JS string ineffective? Maybe some of the typed arrays would be
> faster? Or is it the encoder.encode() process? What is it actually encoding?
> You are writing the whole message in a single call here. Maybe that could be
> chunked too. When I copied that same message into the same account but
> different folder, and converted again, TB took above 2GB of RAM (can't say
> exactly, as it even ate all swap space). So do the memory requirements
> accumulate over all messages? Does the process in converterWorker.js leak
> memory, or not release it (via JS garbage collection) fast enough?

How should I test this ?
 
> @@ +78,5 @@
> > +    }
> > +
> > +    // Username of Local Folders is "nobody". So it's better to use
> > +    // its hostname which is "Local Folders".
> > +    if (deferredToAccount.incomingServer.hostName == "Local Folders") {
> 
> I think there must be a better way to detect the Local folders account. Maybe
> deferredToAccount.incomingServer.key !=
> MailServices.accounts.localFoldersServer.key

I still need to make this change.

> @@ +81,5 @@
> > +    // its hostname which is "Local Folders".
> > +    if (deferredToAccount.incomingServer.hostName == "Local Folders") {
> > +      accountsToConvert += ", " + deferredToAccount.incomingServer.hostName;
> > +    } else {
> > +      accountsToConvert += ", " + deferredToAccount.incomingServer.username;
> 
> Anyway, why do you fetch usernames in multiple places in this function? The
> accounts have their proper name so what do you use username for?

Could you please tell me what should be used here instead of incomingServer.username ?

> @@ +100,5 @@
> > +    if (deferredToAccountName != "Local Folders") {
> > +      deferredToAccountName = deferredToAccount.incomingServer.username;
> > +    }
> > +
> > +    if (gDeferredAccounts.length == 1) {
> 
> This is not how you make plural forms. Please see PluralForm.jsm.

I fixed this in patch7.diff.

If the user tries to convert a deferred account the following message is displayed on the dialog - 
'deferredAccountName' is deferred to 'deferredToAccountName'. Accounts deferred to 'deferredToAccountName': 'commaSeparatedListOfDeferredAccountNames'. 'commaSeparatedListOfToBeConvertedAccountNames' will now be converted to 'format' format.

If the user tries to convert a deferred-to account the following message is displayed on the dialog - 
Accounts deferred to 'deferredToAccountName': 'commaSeparatedListOfDeferredAccountNames'. 'commaSeparatedListOfToBeConvertedAccountNames' will now be converted to 'format' format.

No singular or plural forms are used. So I don't think there is a need to use PluralForm.jsm.


> @@ +216,5 @@
> > +    Services.io.offline = false;
> > +    document.getElementById("cancel").style.display = "none";
> > +    document.getElementById("finish").style.display = "inline-block";
> > +    document.getElementById("messageSpan").style.display = "none";
> > +    document.getElementById("completeSpan").style.display = "block";
> 
> So if this is or new non-XUL future... :(
>

I did not understand what this means. Sorry.. :(. Could please explain a bit.
 
> ::: mailnews/base/test/unit/test_converterDeferredAccount.js
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Does this need to be a separate test file? Can it be folded into
> test_converter.js and just run the functions twice with an argument which
> creates a deferred account in one of the runs? Or is there a large
> difference? This relates to my question above why are deferred accounts so
> special.
> 

I think it is possible that a user tries to convert an account that is deferred to some other account or tries to convert an account to which other accounts are deferred. In both the cases only the deferred-to account which contains the messages is actually converted and the mailstore type of all accounts deferred to this account are changed. The only thing special about handling deferred accounts is that, the user should be notified about all the accounts which are deferred to the deferred-to account so that if the user does not want the messages of any one or more of the deferred accounts to be converted he/she can cancel the conversion because converting the deferred-to account will convert the messages belonging to every deferred account and the deferred-to account.

Please let me know if there is something wrong here.

Now when I think about it, I don't think a separate test for deferred accounts is necessary. Do you want me to remove it ?  
> ::: mailnews/jar.mn
> @@ +63,5 @@
> >      content/messenger/SmtpServerEdit.js                                        (base/prefs/content/SmtpServerEdit.js)
> >      content/messenger/smtpEditOverlay.xul                                      (base/prefs/content/smtpEditOverlay.xul)
> >      content/messenger/smtpEditOverlay.js                                       (base/prefs/content/smtpEditOverlay.js)
> > +    content/messenger/converterDialog.xhtml                                    (base/prefs/content/converterDialog.xhtml)
> > +    content/messenger/converterDialog.js                                       (base/prefs/content/converterDialog.js)
> 
> This does not apply today, needs update.

What exactly is to be changed here ?

Comment 95

[Anindya-Pandey]

(In reply to :aceman from comment #91)
> (In reply to Anindya-Pandey from comment #87)
> > (In reply to Anindya-Pandey from comment #86)
> > > The 160 % completion error was because the root folder was being compacted
> > > before conversion. The compaction is working now and the conversion works
> > > properly even after deleting or moving messages.
> > 
> > sorry, the error was because the root folder was NOT* being compacted
> 
> Just do not try to run compact as part of the migration.
> To calculate correct percentage, use the number of messages you are going to
> copy (the not deleted ones), not total number of messages (including
> deleted). Or are you totally bypassing the msf database which could tell you
> this number beforehand?

getTotalMessages() gives the correct number of messages to be copied (the not deleted ones). But when the message files are parsed the deleted messages still exist in the files and they too get copied giving the wrong percentage. Is there a way to check if a message is deleted or not when an mbox message file is parsed using the text of the message ?

Comment 96

[:aceman]

(In reply to Anindya-Pandey from comment #95)
> Is there a way to check if a message is deleted or not when an
> mbox message file is parsed using the text of the message ?

You would have to parse the "X-Mozilla-Status" header of the message to check if it contains the nsMsgMessageFlagType.Expunged flag.

Comment 97

[Jorg K (CEST = GMT+2)]

Sorry in the delay for the review. I'll let Aceman (who has got hardware trouble currently) go first since the new patch addresses his review. Since two months have passed between Aceman's review and the new patch, I don't think there's any rush here.

Comment 98

[Joseph Reagle]

It's a new summer and I'm wondering if this GSOC will see the light of day? I'd love to switch to maildir.

Comment 99

[Jorg K (CEST = GMT+2)]

Comment on attachment 8824762 [details] [diff] [review]
patch7.diff

Looks like someone needs to pick this up and carry it through to completion, preferably before TB 59 comes around. I understand that Aceman needs to check whether his review comments were addressed and also there are some more changes required, as per comment #94.

So clearing the review request for now.

Comment 100

[:aceman]

(In reply to Anindya-Pandey from comment #94)
> I think it is possible that a user tries to convert an account that is
> deferred to some other account or tries to convert an account to which other
> accounts are deferred. In both the cases only the deferred-to account which
> contains the messages is actually converted and the mailstore type of all
> accounts deferred to this account are changed. The only thing special about
> handling deferred accounts is that, the user should be notified about all
> the accounts which are deferred to the deferred-to account so that if the
> user does not want the messages of any one or more of the deferred accounts
> to be converted he/she can cancel the conversion because converting the
> deferred-to account will convert the messages belonging to every deferred
> account and the deferred-to account.
> 
> Please let me know if there is something wrong here.

OK

> > 6. I tried to convert an account that had a single 150MB message in the
> > Trash folder. It took several minutes and TB took about 1.5GB of RAM in the
> > process. That is quite unexpected when the operation should just be to read
> > a big file from disk and write it consecutively to another file (my disk
> > should do that in ~3 seconds). Can you test what takes the time and RAM? Is
> > using a JS string ineffective? Maybe some of the typed arrays would be
> > faster? Or is it the encoder.encode() process? What is it actually encoding?
> > You are writing the whole message in a single call here. Maybe that could be
> > chunked too. When I copied that same message into the same account but
> > different folder, and converted again, TB took above 2GB of RAM (can't say
> > exactly, as it even ate all swap space). So do the memory requirements
> > accumulate over all messages? Does the process in converterWorker.js leak
> > memory, or not release it (via JS garbage collection) fast enough?
> 
> How should I test this ?

Create such a big message and do the conversion.

> > @@ +81,5 @@
> > > +    // its hostname which is "Local Folders".
> > > +    if (deferredToAccount.incomingServer.hostName == "Local Folders") {
> > > +      accountsToConvert += ", " + deferredToAccount.incomingServer.hostName;
> > > +    } else {
> > > +      accountsToConvert += ", " + deferredToAccount.incomingServer.username;
> > 
> > Anyway, why do you fetch usernames in multiple places in this function? The
> > accounts have their proper name so what do you use username for?
> 
> Could you please tell me what should be used here instead of
> incomingServer.username ?

If you really want the account names, then use <account>.incomingServer.prettyName. .username may not be unique among accounts.
 
> > @@ +216,5 @@
> > > +    Services.io.offline = false;
> > > +    document.getElementById("cancel").style.display = "none";
> > > +    document.getElementById("finish").style.display = "inline-block";
> > > +    document.getElementById("messageSpan").style.display = "none";
> > > +    document.getElementById("completeSpan").style.display = "block";
> > 
> > So if this is or new non-XUL future... :(
> I did not understand what this means. Sorry.. :(. Could please explain a bit.

This would be done more elegantly if you had a .xul file for converterDialog.xhtml. But if you have an xhtml format ordered by the mentor (understandable), you do not need to do anything with this.
  
> > ::: mailnews/base/test/unit/test_converterDeferredAccount.js
> > @@ +1,1 @@
> > > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > 
> > Does this need to be a separate test file? Can it be folded into
> > test_converter.js and just run the functions twice with an argument which
> > creates a deferred account in one of the runs? Or is there a large
> > difference? This relates to my question above why are deferred accounts so
> > special.
> > 
> Please let me know if there is something wrong here.
> 
> Now when I think about it, I don't think a separate test for deferred
> accounts is necessary. Do you want me to remove it ?  

Yes, I'd like the two test files merged. Please keep testing both cases of conversion (deferred and non-deferred account), but merge it into a single file. It seems much of the code is common to both test cases.

> > ::: mailnews/jar.mn
> > @@ +63,5 @@
> > >      content/messenger/SmtpServerEdit.js                                        (base/prefs/content/SmtpServerEdit.js)
> > >      content/messenger/smtpEditOverlay.xul                                      (base/prefs/content/smtpEditOverlay.xul)
> > >      content/messenger/smtpEditOverlay.js                                       (base/prefs/content/smtpEditOverlay.js)
> > > +    content/messenger/converterDialog.xhtml                                    (base/prefs/content/converterDialog.xhtml)
> > > +    content/messenger/converterDialog.js                                       (base/prefs/content/converterDialog.js)
> > 
> > This does not apply today, needs update.
> 
> What exactly is to be changed here ?

Probably nothing, the patch applies today so it seems you fixed this as part of updating the patches for v6 and v7.

Comment 101

[:aceman]

(In reply to Jorg K [Almost not working on Thunderbird (some bustage-fix only) due to non-renewal of contract] from comment #84)
> (In reply to :aceman from comment #83)
> 
> I'm responsible for some of this, so allow me some replies:
> 
> > > +  let p1 = "mail.server." + gServer.key + ".directory";
> > > +  let p2 = "mail.server." + gServer.key + ".directory-rel";
> > > +  let p3 = "mail.server." + gServer.key + ".newsrc.file";
> > > +  let p4 = "mail.server." + gServer.key + ".newsrc.file-rel";
> > > +  let p5 = "mail.server." + gServer.key + ".storeContractID";
> > 
> > I think these can be retrieved from the server directly, without this
> > hardcoding. As last resort e.g. gServer.getCharValue("directory") should
> > work. Or were those accessors unusable for some reason?
> 
> I introduced these variables since the RHS expressions were just copied over
> and over and over and over ;-)

The variables are OK, but the way of retrieving the values are strange. This still exists in the current patch v7.
 
> I added this comment. The comment says: "If directory with path 'dest' does
> not exist ...", but there is no test in the code. So the comment needs to
> change or there is something missing in the code.
> 
> > > +        // no. of msgs in the account is 0 and mailstore type is maildir.
> > > +        self.postMessage(["msfdat", sourceFile]);
> > > +
> > > +      } else if (mailstoreContractId != "@mozilla.org/msgstore/maildirstore;1") {
> > > +        // XXX Comment?
> > 
> > What should go here?
> 
> I think a comment explaining what the 'else if' branch does would be good.

OK, a comment is now there. It seems to be the branch for 'mbox'. Then I would like we check for mbox specifically in the branch:
      } else if (mailstoreContractId != "@mozilla.org/msgstore/maildirstore;1") {

If we encounter an unknown format, it should error out, not assume everything else is mbox.

Comment 102

[:aceman]

Anindya, will you continue here?

Comment 103

[Joseph Reagle]

Any update? Still looking forward to this.

Comment 104

[Magnus Melin [:mkmelin] (away until Aug 10)]

Attachment: patch8.diff

Refreshed, and incorporated some of the comments, leaving todo comments for some others. 

Also added a pref to enable this in the ui on nightlies. (mail.store_conversion_enabled)

Although some polishing could be good, I think we should land this now so that real testing is a possibility, and we can enable conversion for the average user for the next esr release after 60. 

If we don't land this now it's not possible to get enough testing by real users until the next release. I think it would better progress with polish in-tree.

Comment 105

[Jorg K (CEST = GMT+2)]

Wow, that's unexpected. I hope you ran the tests.

Comment 106

[Jorg K (CEST = GMT+2)]

You didn't run the tests :-(
Patch is full of Components.xxx and Components.utils.import and do_check_eq and do_check_true.
At least the latter will fail :-(

Comment 107

[Jorg K (CEST = GMT+2)]

Attachment: patch8.diff - after basic clean-up

Comment 108

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cb57855b5455
Implement a pluggable store converter (mbox <-> maildir). r=mkmelin,aceman

Comment 109

[Magnus Melin [:mkmelin] (away until Aug 10)]

Ah yes, sorry about that - ran out of time and forgot to see if they still worked. And thanks for the modifications!

Also thanks again to Anindya for the base work here! Look forward to getting this more polished. For the TB67 release we can hopefully promote maildir support, in addition to early preview for advanced users who want to check it out in version 60.

Comment 110

[:aceman]

Please don't forget to file the followup bug for the remaining problems.

Comment 111

[Joseph Reagle]

Is this expected in TB68 now? Any idea when that might be released?

Comment 112

[Magnus Melin [:mkmelin] (away until Aug 10)]

Its available since 60 if you set the mail.store_conversion_enabled pref.
There was actually a bunch of improvements to it going into 68. Bug 1306254 is tracking enabling it by default.

Comment 113

[Wayne Mery (:wsmwk)]

(In reply to Joseph Reagle from comment #111)

Is this expected in TB68 now? Any idea when that might be released?

A few changes happened for 68 - some of them are the dependencies of maildirblockers bug 845952. A lot more to be done. There is no time table.

https://support.mozilla.org/en-US/kb/maildir-thunderbird and the wiki article to which it points summarizes the current state and some of the concerns.

Comment 114

[Joseph Reagle]

I just upgrade to TB68 from TB60, is it possible to convert existing mailboxes yet? All the maildirblockers in 845952 have strike-throughs.

Comment 115

[Wayne Mery (:wsmwk)]

(In reply to Joseph Reagle from comment #114)

I just upgrade to TB68 from TB60, is it possible to convert existing mailboxes yet? All the maildirblockers in 845952 have strike-throughs.

There are certainly blockers to bug 845852 https://bugzilla.mozilla.org/showdependencytree.cgi?id=845952&hide_resolved=1 therefore maildir is not yet set as a default storage choice.

Comment 116

[Morgan Read]

Hi Folks, running TB???.
When setting mail.store_conversion_enabled=true where is the feature revealed in the UI?
Thanks

Comment 117

[Morgan Read]

Oops, those ??? were a marker for me, trigger happy - TB 91.8.0

Comment 118

[Magnus Melin [:mkmelin] (away until Aug 10)]

You can then use the Message Store Type dropdown, found under Account Settings | Server Settings to trigger migration.

Comment 119

[Morgan Read]

Thanks, I hadn't noticed that - but, I'm a little confused - this setting under Account Settings | Server Settings 'Message Store Type' looks the same as the setting under Preferences | General | Indexing 'Message Store Type for new accounts' which was active before setting mail.store_conversion_enabled=true

What's the difference in function? (I was expecting a function that actively converted mailbox type.)

I actually made use of the instructions to use ImportExportTools here:
https://www.wilderssecurity.com/threads/thunderbird-from-mbox-to-maildir.389599/
Which was with IMAP but as I was changing email host provider with mailboxes of over 1GB so I actually followed the POP advice offline.

Following on, I also have a number of multi-GB mbox archives on a headless nfs share which I was also wanting to move to maildir and I wasn't looking forward to doing it across nfs - is there any command line tool for conversion or, I was wondering about installing TB on the nfs server and running the gui (slowly) over nfs. Apologies, perhaps I've gone a little ot.

Comment 120

[Morgan Read]

Attachment: Screenshot from 2022-04-29 10-02-47.png

Comment 121

[Morgan Read]

Comment on attachment 9274400 [details]
Screenshot from 2022-04-29 10-02-47.png

I discovered the drop down menu under Settings has different behaviour to that under Preferences - I guess that's no surprise, but just in case some other green horn happens buy, persevere.

Running an instance of TB across the network to deal with v large (one 15GB!) mbox files on my headless server seems to work. Unfortunately, the conversion process hung at 14% with a failure message, which I guess is down to the the size of the mbox files TB was asked to digest:

Comment 122

[Magnus Melin [:mkmelin] (away until Aug 10)]

If you can reproduce the error, file a new bug. To seem more log info, set the mail.mailstoreconverter.loglevel preference to All. (Logs then go to the error console.)

Comment 123

[Morgan Read]

Attachment: pics refered to comment #124

Comment 124

[Morgan Read]

Apologies, it's taken a while to put things back as they were once getting everything working for me.
I ran the tests again with the set-up as before - with the massive mbox files (over 15GBs). There was not a squeak from the errout - or stdout, nothing. I ran it initially with errout sent to a file and then figured I must have done something wrong and so ran it just on the command line. On both times I ran the conversion it failed on 29% but I doubt that means much as the first I reported was on 14%. Pics attached. Now it's set-up back where it was, I'm able to run any further tests to sort this out.

Comment 125

[Morgan Read]

Attachment: Screenshot from 2022-05-16 21-13-17.png

Comment 126

[Morgan Read]

Attachment: Screenshot from 2022-05-16 17-53-08.png

Comment 127

[Morgan Read]

Attachment: Screenshot from 2022-05-16 19-49-56.png

Comment 128

[Magnus Melin [:mkmelin] (away until Aug 10)]

Oh, with Error Console I mean the Thunderbird Error Console (Ctrl+Shift+J). Though it should go to the native console as well if you put tb in the background (thunderbird -p &)