Closed Bug 1017939 Opened 10 years ago Closed 10 years ago

newly created maildir subfolders folders are created under INBOX instead of INBOX.sbd

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: rkent, Assigned: rkent)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

STR:

1) Enable maildir and offline storage
2) Create (using UI) a subfolder of INBOX

Subfolder gets created under INBOX instead of INBOX.sbd.  I suspect this is because in nsMsgDBFolder::CreateDirectoryForFolder the folder path is checked to see if it is a directory (which it is in maildir, but not in mbox) and that is used to determine whether the .sbd directory needs to be created.
Attached patch test that shows the failure (obsolete) — Splinter Review
Assignee: nobody → kent
Status: NEW → ASSIGNED
Attached patch make the directory end in .sbd (obsolete) — Splinter Review
The reason this was failing is that folder code was checking to see if a folder was a directory, and if it was assuming that it could put subfolders there. This does not work for maildir.

I really don't like embedding storage details like "subfolder parents end in .sbd" in the folders instead of in the message store, but that seems to be ubiquitous and fixing it is a much bigger effort.

The changes to nsMsgMaildirStore.cpp were needed because under many cases the message files were being created as folders, making it hard to even test this code.
Attachment #8431304 - Attachment is obsolete: true
Attachment #8431900 - Flags: review?(irving)
FYI. Bug 857436 is for similar problem upon "rename folder".
WADA: I did this patch because I could not make the STR in bug 857436 work - because I could not even create a folder without having issues. It is quite possible that the patch in this bug is all that is needed for bug 857436, so for now I will just mark a dependency.
Blocks: 857436
Blocks: 855950
Depends on: 856387
Comment on attachment 8431900 [details] [diff] [review]
make the directory end in .sbd

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

The only blocker is the change from 'Promise.defer()' to 'new Promise(function...)'; everything else is nits or optional if you disagree strongly with my suggestion.

::: mailnews/imap/test/unit/test_subfolderLocation.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> + 

There are a few trailing white spaces in this file

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +142,1 @@
>   * but no "new" subfolder, because it's not sensical in the mail client context.

existing text, but s/it's not sensical/it doesn't make sense/

@@ +149,5 @@
>    nsresult rv = path->Create(nsIFile::DIRECTORY_TYPE, 0700);
> +  if (NS_FAILED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS)
> +  {
> +    NS_WARNING("Could not create directory for message folder");
> +    return rv;

Include the folder path and failure reason in the error message if it's not too difficult.

@@ +162,5 @@
>    leaf->AppendNative(NS_LITERAL_CSTRING("tmp"));
>    rv = leaf->Create(nsIFile::DIRECTORY_TYPE, 0700);
> +  if (NS_FAILED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS)
> +  {
> +    NS_WARNING("Could not create directory for message folder");

same, except that it's nice to have a slightly different error message so we can tell which failure we hit (actually, if NS_WARNING includes the line number that's enough).

@@ +170,5 @@
>    leaf->SetNativeLeafName(NS_LITERAL_CSTRING("cur"));
>    rv = leaf->Create(nsIFile::DIRECTORY_TYPE, 0700);
> +  if (NS_FAILED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS)
> +  {
> +    NS_WARNING("Could not create directory for message folder");

same again

::: mailnews/test/resources/PromiseTestUtils.jsm
@@ +51,5 @@
>    get promise() { return this._deferred.promise; },
>  };
> +
> +/**
> + * Folder listener to resolves a promise when a folder with a certain

"Create and register a folder listener and return a promise that resolves when a folder with the specified name..."

@@ +55,5 @@
> + * Folder listener to resolves a promise when a folder with a certain
> + * name is added.
> + *
> + * @param name   folder name to listen for
> + * @param promise  promise that resolves when folder added

@return promise{folder} promise that resolves with the new folder when the folder add completes (in case somebody can use it)

@@ +57,5 @@
> + *
> + * @param name   folder name to listen for
> + * @param promise  promise that resolves when folder added
> + */
> + 

trailing white space

@@ +60,5 @@
> + */
> + 
> +PromiseTestUtils.PromiseFolderAdded = function PromiseFolderAdded(name)
> +{
> +  this._deferred = Promise.defer();

Promise.defer() isn't part of the ES6 Promises spec, and may eventually be deprecated. Please use the syntax:

promise = Promise.new((resolve, reject) => {
  ... code that eventually resolves or rejects the promise
});

For this example, rather than creating an object holding a Promise, I'd recommend a function that just returns the promise directly:

function PromiseFolderAdded (folderName) {
  return new Promise((resolve, reject) => {
    listener = {
       folderAdded: aFolder => {
         if (aFolder.name == folderName) {
           MailServices.mfn.removeListener(listener);
           resolve(aFolder);
         }
       }
    };
    MailServices.mfn.addListener(listener);
  });
}
Attachment #8431900 - Flags: review?(irving) → review-
Attached patch Use ES6 Promise constructior (obsolete) — Splinter Review
I fixed the issues except for "Include the folder path and failure reason in the error message if it's not too difficult." While I agree in principle that it would be a good idea, it is a significant amount of code for a relatively unimportant warning. Perhaps we need to develop some macros to make this kind of thing easier.

We'll also fix the PromiseTestUtils' urlListener to match this pattern once we get this all resolved.
Attachment #8431900 - Attachment is obsolete: true
Attachment #8435360 - Flags: review?(irving)
Comment on attachment 8435360 [details] [diff] [review]
Use ES6 Promise constructior

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

::: mailnews/imap/test/unit/test_subfolderLocation.js
@@ +59,5 @@
> +/**/
> +// use a folder method to add a subfolder
> +add_task(function* addSubfolder() {
> +  IMAPPump.inbox.createSubfolder(folderName1, null);
> +  yield PromiseTestUtils.PromiseFolderAdded(folderName1);

There's a race condition here; you need to set up the listener (and get your promise) before you call createSubfolder, and then yield that promise after:

let pFolderCreated = PTU.PromiseFolderAdded(folderName1);
I.i.createSubFolder(...);
yield pFolderCreated;
Attachment #8435360 - Flags: review?(irving) → review-
Attached patch 1017939_v3.patchSplinter Review
Hopefully without race condition.

I renamed to (small p) PromiseTestUtils.promiseFolderAdded because I found myself wondering if a "new" was needed with this.
Attachment #8435360 - Attachment is obsolete: true
Attachment #8437928 - Flags: review?(irving)
Attachment #8437928 - Flags: review?(irving) → review+
Checked in https://hg.mozilla.org/comm-central/rev/58347faf6340
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
OS: Windows 7 → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
You need to log in before you can comment on or make changes to this bug.