Can't delete a folder if Trash already contains a folder of the same name

over to naving

using build2001-01-29-04 I see the following for pop
delete drafts folder
exit app
log into the account.
You will see that drafts is created again
delete this folder again
In trash you still see the first drafts folder deleted and not the second one.
The second one just goes away. Which is the opposite of what the reporter is
seeing.

confirming the bug.

We can throw an alert and inform the user that the folder will be created with
a new name. For example if there is a folder "A" under Trash and if the user
tries to delete "A" somewhere else. Then this folder will be created with a new
name "A0". This is what 4x does.

I agree. the warning message should also let the user know that the folder will
be renamed if he okays the alert.

Created an attachment (id=26204)
proposed fix

Just made it like 4x. cc bienvenu, sspitzer for review.

Sorry for marking it fixed !

a couple comments:

1) is your while loop correct?

It looks like it will generate foldernames like this:

foo1,foo12,foo123,foo1234

seems like you'd want:

foo-1,foo-2,foo-3

2) I'm not sure, but should you be using strcasecmp() instead of strcmp()?

3) ToNewCString() to get oldName and newName are wrong. what if the folder
names are in japanese? you should not be using the nsTextFormatter, you should
be using nsIStringBundle's FormatStringFromName() (or FormatStringFromID()?)

I think you can write this code without using Free() by using nsXPIDLString and
FormatStringFrom*()

>>foo1,foo12,foo123,foo1234

I would leave it this way because it is going to Trash, anyway and 4x also
does this way

>>I think you can write this code without using Free() by using nsXPIDLString
>>and
>>FormatStringFrom*()
FormatStringFromID() finally uses nsTextFormatter::smprintf

If I don't use const char *oldName *newName only the 1st character is
displayed. Any ideas?

I think UTF8String() will take care of Japanese characters.

cc nhotta. nhotta do you know whether UTF8String() will take care of
japanense characters.

I have this fix in my tree for a long time now. Can somebody tell me
what should I do for the Japenese characters ?

Seth, I was talking to you about this bug, regarding japenese characters.

*** Bug 91711 has been marked as a duplicate of this bug. ***

similar: bug 46365

changing QA contact from Sheela to Gregg

*** Bug 161876 has been marked as a duplicate of this bug. ***

*** Bug 125891 has been marked as a duplicate of this bug. ***

Reporter, I'm unable to reproduce in current build - I'm unable to delete ANY
system folders. Are you still seeing this problem?

Currently, when we delete a folder while another with the same name exists in
trash, the opertion fails and there's a confusing error messagebox: "A folder
with that name already exists. Please enter a different name".

This is so confusing - the user isn't naming anything at the moment, how can he
enter a different name?

My friend had this problem (and problem described in bug 73404) just recently in
1.2 branch nightly build when trying to manage some imported Outlook folders
(changing the mail reader from Outlook to Mozilla). Can we please check this
patch in?

this patch needs work before it can get checked in, as Seth pointed out. Since
it was written, the string classes have changed quite a bit, so I think a lot of
the copying is unneeded, and you can use the Equals method to compare
nsXPIDLStrings, so there's no need for the auto strings. To format messages with
unicode strings, you just use %S instead of %s in the format string like so:
+4023=The Trash already contained a folder named %S. The folder which you just
deleted can be found in the Trash under the new name %S.

Also,
               newFolderNameUnderTrash.AppendInt(i,10);
should just be newFolderNameUnderTrash.AppendInt(i); since 10 is the default.

mass re-assign.

*** Bug 268435 has been marked as a duplicate of this bug. ***

*** Bug 268436 has been marked as a duplicate of this bug. ***

*** Bug 268946 has been marked as a duplicate of this bug. ***

*** Bug 208114 has been marked as a duplicate of this bug. ***

Bug 71348 is for same symptom on IMAP.

*** Bug 250390 has been marked as a duplicate of this bug. ***

*** Bug 71348 has been marked as a duplicate of this bug. ***

*** Bug 274829 has been marked as a duplicate of this bug. ***

I am confused by this bug. It seems that the original problem has been fixed,
but in an unsatisfactory way which leads to the undesirable side-effect noted in
Comment 20 and in each of the bugs recently marked as duplicates of this one. It
seems that the current bug is not the original one, but a bug in the first bug
fix. If this doesn't count as a new bug, at least the situation should be
clarified - and the reporters of the bugs duplicated to this one should be
encouraged to vote for this one.

*** Bug 277959 has been marked as a duplicate of this bug. ***

*** Bug 314406 has been marked as a duplicate of this bug. ***

As noted in comment 31, this bug has morphed over time from deleting system
folders to normal folders, though I don't doubt that system folders would show
the same behaviour today if it were possible to delete them.

Since a lot of other bugs have already been duped against this bug, we might as
well morph this bug completely...

-> no longer dataloss, updating summary + whiteboard, etc.

*** Bug 281236 has been marked as a duplicate of this bug. ***

*** Bug 357702 has been marked as a duplicate of this bug. ***

*** Bug 397991 has been marked as a duplicate of this bug. ***

*** Bug 46365 has been marked as a duplicate of this bug. ***

*** Bug 405493 has been marked as a duplicate of this bug. ***

As comment #34 points out, this bug is no longer the same bug as the description states. This bug should be closed and a new bug opened that described the current problem (cannot delete folder and misleading error message). One of the many dupes will do.

Comment on attachment 8371072
WIP patch 3

>+ newFolderName.Assign(folderName);
>+ bool containsChild = true;
>+ uint32_t i = 1;
>+ while (containsChild) {
>+ rv = ContainsChildNamed(newFolderName, &containsChild);
>+ NS_ENSURE_SUCCESS(rv, rv);
>+ if (!containsChild)
>+ break;
>+ // This could be localizable but Toolkit is fine without it, see
>+ // mozilla/toolkit/content/contentAreaUtils.js::uniqueFile()
>+ i++;
>+ newFolderName.Assign(folderName);
>+ newFolderName.AppendLiteral("(");
>+ newFolderName.AppendInt(i);
>+ newFolderName.AppendLiteral(")");
>+ }
bool containsChild;
rv = ContainsChildNamed(folderName, &containsChild);
NS_ENSURE_SUCCESS(rv, rv);
while (containsChild) {
  i++;
  newFolderName.Assign(folderName);
  newFolderName.AppendLiteral("(");
  newFolderName.AppendInt(i);
  newFolderName.AppendLiteral(")");
  rv = ContainsChildNamed(newFolderName, &containsChild);
  NS_ENSURE_SUCCESS(rv, rv);
}

(In reply to <email address hidden> from comment #107)

Hi, why not using for statements:

> >+ newFolderName.Assign(folderName);
> >+ bool containsChild = true;
> >+ for (uint32_t i=2; ; i++) {
> >+ rv = ContainsChildNamed(newFolderName, &containsChild);
> >+ NS_ENSURE_SUCCESS(rv, rv);
> >+ if (!containsChild)
> >+ break;
> >+ // This could be localizable but Toolkit is fine without it, see
> >+ // mozilla/toolkit/content/contentAreaUtils.js::uniqueFile()
> >+ newFolderName.Assign(folderName);
> >+ newFolderName.AppendLiteral("(");
> >+ newFolderName.AppendInt(i);
> >+ newFolderName.AppendLiteral(")");
> >+ }

(In reply to Ulf Zibis from comment #108)
> (In reply to <email address hidden> from comment #107)
>
> Hi, why not using for statements:
>
> > >+ newFolderName.Assign(folderName);
> > >+ bool containsChild = true;
> > >+ for (uint32_t i=2; ; i++) {
> > >+ rv = ContainsChildNamed(newFolderName, &containsChild);
> > >+ NS_ENSURE_SUCCESS(rv, rv);
> > >+ if (!containsChild)
> > >+ break;
> > >+ // This could be localizable but Toolkit is fine without it, see
> > >+ // mozilla/toolkit/content/contentAreaUtils.js::uniqueFile()
> > >+ newFolderName.Assign(folderName);
> > >+ newFolderName.AppendLiteral("(");
> > >+ newFolderName.AppendInt(i);
> > >+ newFolderName.AppendLiteral(")");
> > >+ }

For is never used for such a purpose. That's the point of a while(condition).

IMHO:
1. while(condition) is only appropriate, if the condition is determined outside the loop.
2. define variables only in the scope, where they are used.
3. for(...) is appropriate if there is a counting variable, which is only used inside the loop
4. If you look closer, in your code snippet you could fairly use while(true) and define containsChild inside the loop.

No. The idea here is very similar to the common "priming read" concept taught in introductory CS courses. Following such a pattern the code here should be:

newFolderName.Assign(folderName);
bool containsChild = true;
int counter = 2;

//Priming Read
rv = ContainsChildNamed(newFolderName, &containsChild);
NS_ENSURE_SUCCESS(rv, rv);

while (containsChild) {
  // This could be localizable but Toolkit is fine without it, see
  // mozilla/toolkit/content/contentAreaUtils.js::uniqueFile()
  newFolderName.Assign(folderName);
  newFolderName.AppendLiteral("(");
  newFolderName.AppendInt(i);
  newFolderName.AppendLiteral(")");

  //Read new data
  rv = ContainsChildNamed(newFolderName, &containsChild);
  NS_ENSURE_SUCCESS(rv, rv);
}

Err. Except in that while statement there should be a:
  "counter++"
line right before we read new data.

Little sophisticated:

>+ newFolderName.Assign(folderName);
>+ for (uint32_t i=2, containsChild; ; i++) {
>+ rv = ContainsChildNamed(newFolderName, &containsChild);
>+ NS_ENSURE_SUCCESS(rv, rv);
>+ if (!containsChild)
>+ break;
>+ // This could be localizable but Toolkit is fine without it, see
>+ // mozilla/toolkit/content/contentAreaUtils.js::uniqueFile()
>+ newFolderName.Assign(folderName);
>+ newFolderName.AppendLiteral("(");
>+ newFolderName.AppendInt(i);
>+ newFolderName.AppendLiteral(")");
>+ }

Or really type clean:
>+ for (uint32_t i=2; ; i++) {
>+ bool containsChild;
>+ rv = ContainsChildNamed(newFolderName, &containsChild);
...

I'm not sure, if newFolderName and rv also could be drawn into the loop, are they used outside?

(In reply to Josiah Bruner [:JosiahOne] from comment #111)
> No. The idea here is very similar to the common "priming read" concept
> taught in introductory CS courses.
In advanced CS practice I learned: No code duplicates, because if there comes a change along, one of the duplicates could be overseen.

> Following such a pattern the code here should be:
If there is no other pattern with higher priority ;-)

> newFolderName.Assign(folderName);
> bool containsChild = true; // no need to preset it to true
> int counter = 2; // yes, better than starting with 1 as before

(In reply to <email address hidden> from comment #107)
> Comment on attachment 8371072
> WIP patch 3
>
> >+ newFolderName.Assign(folderName);
> >+ bool containsChild = true;
> >+ uint32_t i = 1;
> >+ while (containsChild) {
> >+ rv = ContainsChildNamed(newFolderName, &containsChild);
> >+ NS_ENSURE_SUCCESS(rv, rv);
> >+ if (!containsChild)
> >+ break;
> >+ // This could be localizable but Toolkit is fine without it, see
> >+ // mozilla/toolkit/content/contentAreaUtils.js::uniqueFile()
> >+ i++;
> >+ newFolderName.Assign(folderName);
> >+ newFolderName.AppendLiteral("(");
> >+ newFolderName.AppendInt(i);
> >+ newFolderName.AppendLiteral(")");
> >+ }
> bool containsChild;
> rv = ContainsChildNamed(folderName, &containsChild);
> NS_ENSURE_SUCCESS(rv, rv);
> while (containsChild) {
> i++;
> newFolderName.Assign(folderName);
> newFolderName.AppendLiteral("(");
> newFolderName.AppendInt(i);
> newFolderName.AppendLiteral(")");
> rv = ContainsChildNamed(newFolderName, &containsChild);
> NS_ENSURE_SUCCESS(rv, rv);
> }

Well, I always try to avoid the duplication of the main logic (rv = ContainsChildNamed(folderName, &containsChild);) that is why I did not write it this way :)

(In reply to Ulf Zibis from comment #114)
> (In reply to Josiah Bruner [:JosiahOne] from comment #111)
> > No. The idea here is very similar to the common "priming read" concept
> > taught in introductory CS courses.
> In advanced CS practice I learned: No code duplicates, because if there
> comes a change along, one of the duplicates could be overseen.
Exactly.

>
> > newFolderName.Assign(folderName);
> > bool containsChild = true; // no need to preset it to true
> > int counter = 2; // yes, better than starting with 1 as before

But later below the loop I check whether i (counter) > 1 so the variable exists outside the loop and value of 1 is also significant to the code.

So what about this:
    uint32_t i;
    for (i=1;;i++)) {
      newFolderName.Assign(folderName);
      if (i>1) {
        newFolderName.AppendLiteral("(");
        newFolderName.AppendInt(i);
        newFolderName.AppendLiteral(")");
      }
      rv = ContainsChildNamed(newFolderName, &containsChild);
      NS_ENSURE_SUCCESS(rv, rv);
      if (!containsChild)
        break;
    }

Of course, instead of (i>1) test later on we could do !newFolderName.Equals(folderName) but that is probably much more expensive.

*** Bug 305728 has been marked as a duplicate of this bug. ***

Comment on attachment 8371072
WIP patch 3

Review of attachment 8371072:
-----------------------------------------------------------------

Nice test.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +1723,5 @@
> return NS_MSG_ERROR_COPY_FOLDER_ABORTED;
> }
> }
> +
> + nsAutoString newFolderName(EmptyString());

Shouldn't need the EmptyString() initializer; the constructor makes an empty string by default.

@@ +1726,5 @@
> +
> + nsAutoString newFolderName(EmptyString());
> + nsAutoString folderName;
> + rv = srcFolder->GetName(folderName);
> + NS_ENSURE_SUCCESS(rv, rv);

Mozilla core code is deprecating NS_ENSURE_SUCCESS, because the macro hides control flow - see https://groups.google.com/d/topic/mozilla.dev.platform/1clMLuuhtWQ/discussion . Use

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

if the condition deserves a warning message (as this one does)

::: mailnews/local/test/unit/test_nsIMsgLocalMailFolder.js
@@ +50,5 @@
> + do_check_eq(trash.numSubFolders, 3);
> + // The folder should be automatically renamed as the same name already is in Trash
> + // but the subfolder should be untouched.
> + let folderDeleted3 = trash.getChildNamed("folder(3)");
> + let subfolderDeleted = folderDeleted3.getChildNamed("subfolder");

do_check_true(trash.containsChildNamed("folder(3)");
do_check_true(trash.getChildNamed("folder(3)").containsChildNamed("subfolder"));

Created attachment 8374439
WIP patch 4

Thanks, addressed all comments and added a new test segment to check for the prompt when moving duplicate folder outside Trash.

Comment on attachment 8374439
WIP patch 4

Review of attachment 8374439:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +58,5 @@
> sendingUnsent=Sending Unsent Messages
>
> folderExists=A folder with that name already exists. Please enter a different name.
> +# LOCALIZATION NOTE(confirmDuplicateFolderRename): %1$S is parent folder name, %2$S is proposed child folder name
> +confirmDuplicateFolderRename=A folder with that name already exists under folder '%1$S'. Would you like to copy the folder under a new name of '%2$S'?

Does this message come up with folder copies and moves, or only with moves? If it's only move, I suggest changing the wording to:

A folder named %1%S already exists in %2$S. Do you wish to move the folder to %2$S / %3$S?

(In reply to :Irving Reid from comment #120)
> Does this message come up with folder copies and moves, or only with moves?
> If it's only move, I suggest changing the wording to:
>
> A folder named %1%S already exists in %2$S. Do you wish to move the folder
> to %2$S / %3$S?

In the change to nsLocalMailFolder.cpp it can be seen I try to do rename only on move (we should not do a copy in CopyFolderLocal() as said earlier, but I put an if there to be sure).

Comment on attachment 8374439
WIP patch 4

>+ bool containsChild = true;
>+ uint32_t i;
>+ for (i = 1; ; i++) {
>+ newFolderName.Assign(folderName);
>+ if (i > 1) {
>+ // This could be localizable but Toolkit is fine without it, see
>+ // mozilla/toolkit/content/contentAreaUtils.js::uniqueFile()
>+ newFolderName.Append('(');
>+ newFolderName.AppendInt(i);
>+ newFolderName.Append(')');
>+ }
>+ rv = ContainsChildNamed(newFolderName, &containsChild);
>+ if (NS_WARN_IF(NS_FAILED(rv))) {
>+ return rv;
>+ }
>+ if (!containsChild)
>+ break;
>+ }
Did you mean for (i = 1; containsChild; i++) ? Note that if you do this then i ends the loop being at least 2, so you'd need to change the subsequent check. Alternatively, you would have to rewrite the loop something like this (sorry if this is what the code did in one of the earlier patches):

uint32_t i = 1;
newFolderName.Assign(folderName);
for (;;) {
  bool containsChild;
  rv = ContainsChildNamed(newFolderName, &containsChild);
  NS_ENSURE_SUCCESS(rv, rv);
  if (!containsChild)
    break;

  i++;
  newFolderName.Assign(folderName);
  newFolderName.Append('(');
  newFolderName.AppendInt(i);
  newFolderName.Append(')');
}

>+ try {
>+ root.copyFolderLocal(folderDeleted3, true, null, null);
>+ do_throw("copyFolderLocal() should have failed here due to user prompt!");
I'm not sure why you're trying to throw from inside this try block, but I don't know enough about writing tests to know whether there's a better way.

>+ } catch (e) {
>+ do_check_eq(e.result, parseInt("0x8055001a", 16));
Why not just write 0x8055001a?

>+confirmDuplicateFolderRename=A folder with that name already exists under folder '%1$S'. Would you like to copy the folder under a new name of '%2$S'?
A subfolder with that name already exists in the folder '%1$S'. Would you like to move the folder using the new name of '%2$S'?

(In reply to <email address hidden> from comment #122)
> Comment on attachment 8374439
> WIP patch 4
>
> >+ bool containsChild = true;
> >+ uint32_t i;
> >+ for (i = 1; ; i++) {
> >+ newFolderName.Assign(folderName);
> >+ if (i > 1) {
> >+ // This could be localizable but Toolkit is fine without it, see
> >+ // mozilla/toolkit/content/contentAreaUtils.js::uniqueFile()
> >+ newFolderName.Append('(');
> >+ newFolderName.AppendInt(i);
> >+ newFolderName.Append(')');
> >+ }
> >+ rv = ContainsChildNamed(newFolderName, &containsChild);
> >+ if (NS_WARN_IF(NS_FAILED(rv))) {
> >+ return rv;
> >+ }
> >+ if (!containsChild)
> >+ break;
> >+ }
> Did you mean for (i = 1; containsChild; i++) ? Note that if you do this then
> i ends the loop being at least 2, so you'd need to change the subsequent
> check. Alternatively, you would have to rewrite the loop something like this
> (sorry if this is what the code did in one of the earlier patches):
>
> uint32_t i = 1;
> newFolderName.Assign(folderName);
> for (;;) {
> bool containsChild;
> rv = ContainsChildNamed(newFolderName, &containsChild);
> NS_ENSURE_SUCCESS(rv, rv);
> if (!containsChild)
> break;
>
> i++;
> newFolderName.Assign(folderName);
> newFolderName.Append('(');
> newFolderName.AppendInt(i);
> newFolderName.Append(')');
> }
Yes, this is what the previous version did, just with a 'while'.
Is this issue really that important that we make tens of comments in such a long thread? It is not a perf critical path.

> >+ try {
> >+ root.copyFolderLocal(folderDeleted3, true, null, null);
> >+ do_throw("copyFolderLocal() should have failed here due to user prompt!");
> I'm not sure why you're trying to throw from inside this try block, but I
> don't know enough about writing tests to know whether there's a better way.
I expect .copyFolderLocal to throw and be catched by my catch. But if it does not throw (wrong behaviour), the next throw is not catched and fails the test.

>
> >+ } catch (e) {
> >+ do_check_eq(e.result, parseInt("0x8055001a", 16));
> Why not just write 0x8055001a?
Because e.result returns the code in decimal. Is there other method of 'e' I can use?

> >+confirmDuplicateFolderRename=A folder with that name already exists under folder '%1$S'. Would you like to copy the folder under a new name of '%2$S'?
> A subfolder with that name already exists in the folder '%1$S'. Would you
> like to move the folder using the new name of '%2$S'?

Irving proposed some other wording with more arguments. So what should I do now?

(In reply to aceman from comment #123)
> (In reply to comment #122)
> > (From update of attachment 8374439)
> Is this issue really that important that we make tens of comments in such a
> long thread? It is not a perf critical path.

I'm after readability here, not perf, in particular I don't like breaking at the end of a for loop, because it's easy to miss that this skips the increment.

> > >+ try {
> > >+ root.copyFolderLocal(folderDeleted3, true, null, null);
> > >+ do_throw("copyFolderLocal() should have failed here due to user prompt!");
> > I'm not sure why you're trying to throw from inside this try block, but I
> > don't know enough about writing tests to know whether there's a better way.
> I expect .copyFolderLocal to throw and be catched by my catch. But if it
> does not throw (wrong behaviour), the next throw is not catched and fails
> the test.
Sure, but my question is is it safe to put do_throw in a try block?

> > >+ } catch (e) {
> > >+ do_check_eq(e.result, parseInt("0x8055001a", 16));
> > Why not just write 0x8055001a?
> Because e.result returns the code in decimal. Is there other method of 'e' I
> can use?

nsIException.idl (if that's the right one) says that result is an unsigned long, not a decimal. So I don't see why you can't compare it to any other integer.

> > >+confirmDuplicateFolderRename=A folder with that name already exists under folder '%1$S'. Would you like to copy the folder under a new name of '%2$S'?
> > A subfolder with that name already exists in the folder '%1$S'. Would you
> > like to move the folder using the new name of '%2$S'?
>
> Irving proposed some other wording with more arguments. So what should I do
> now?

Sorry, I hadn't seen his wording. How about:
A subfolder with the name '%$1S' already exists in the folder '%2$S'. Would you like to move this folder using the new name of '%3$S'?
(I don't really want to use a / because that's an implementation detail.)

Created attachment 8377817
patch v5

> > > >+ try {
> > > >+ root.copyFolderLocal(folderDeleted3, true, null, null);
> > > >+ do_throw("copyFolderLocal() should have failed here due to user prompt!");
> > > I'm not sure why you're trying to throw from inside this try block, but I
> > > don't know enough about writing tests to know whether there's a better way.
> > I expect .copyFolderLocal to throw and be catched by my catch. But if it
> > does not throw (wrong behaviour), the next throw is not catched and fails
> > the test.
> Sure, but my question is is it safe to put do_throw in a try block?
I use the same pattern in test_fileName.js. I don't know if it is safe enough. I now changed the catch a bit to only catch the specific exception. Is is safer now?

> > > >+ } catch (e) {
> > > >+ do_check_eq(e.result, parseInt("0x8055001a", 16));
> > > Why not just write 0x8055001a?
> > Because e.result returns the code in decimal. Is there other method of 'e' I
> > can use?
> nsIException.idl (if that's the right one) says that result is an unsigned
> long, not a decimal. So I don't see why you can't compare it to any other
> integer.
I knew octal literals are deprecated so somehow I extended that to hex too. Thanks.

Comment on attachment 8377817
patch v5

Review of attachment 8377817:
-----------------------------------------------------------------

(In reply to <email address hidden> from comment #124)
> (In reply to aceman from comment #123)
> > (In reply to comment #122)
> > > (From update of attachment 8374439)
> > Is this issue really that important that we make tens of comments in such a
> > long thread? It is not a perf critical path.
>
> I'm after readability here, not perf, in particular I don't like breaking at
> the end of a for loop, because it's easy to miss that this skips the
> increment.
>
> > > >+ try {
> > > >+ root.copyFolderLocal(folderDeleted3, true, null, null);
> > > >+ do_throw("copyFolderLocal() should have failed here due to user prompt!");
> > > I'm not sure why you're trying to throw from inside this try block, but I
> > > don't know enough about writing tests to know whether there's a better way.
> > I expect .copyFolderLocal to throw and be catched by my catch. But if it
> > does not throw (wrong behaviour), the next throw is not catched and fails
> > the test.
> Sure, but my question is is it safe to put do_throw in a try block?

You can call do_throw() inside a try; whether you catch it or not it still registers as a test failure (see https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#XPCShell_test_utility_functions)

> Sorry, I hadn't seen his wording. How about:
> A subfolder with the name '%$1S' already exists in the folder '%2$S'. Would
> you like to move this folder using the new name of '%3$S'?
> (I don't really want to use a / because that's an implementation detail.)

This is OK with me; whoever has ui-r? gets final say. r+ aside from that wording change.

Comment on attachment 8377817
patch v5

Review of attachment 8377817:
-----------------------------------------------------------------

Josiah, please see the discussion about the wording of the message and decide on ui-r :)

(In reply to aceman from comment #125)
> I knew octal literals are deprecated so somehow I extended that to hex too.
> Thanks.

The only reason old-style octal literals are deprecated is because it's too easy to confuse them with decimal literals e.g. 160 - 060 != 100. Hex literals and new-style octal literals (0o60) are fine, of course.

Comment on attachment 8377817
patch v5

>+ rv = bundle->FormatStringFromName(MOZ_UTF16("confirmDuplicateFolderRename"),
>+ formatStrings, 3, getter_Copies(confirmString));
If you'd used MOZ_ARRAY_LENGTH(formatStrings) it would have saved you the trouble of changing it each time ;-)

>+ } catch (e if e.result == 0x8055001a) {
>+ // catch only the expected error, otherwise fail the test.
Nice!

(In reply to :aceman from comment #100)
> (In reply to :Irving Reid from comment #90)
> > (In reply to :aceman from comment #89)
> > > Can't bug 765926 cover maildir by itself?
> > As far as I know, maildir is mostly untested aside from what's discussed in
> > bug 765926. I don't know whether the long term plan is to run the entire
> > suite twice, once for mbox and once for maildir - if so then yes, we'd be
> > covered. That's a huge waste of testing run time, but might be easier than
> > refactoring our test suite into separate tests for the higher level and the
> > store.
> I am sure we or somebody else will run the whole testsuite under maildir in
> the future. I understood bug 765926 as a metabug to fix found issues. If we
> do not have any other individual tests running both store versions NOW, I do
> not see a reason implementing it in this test (it would be an anomaly). But
> it would probably not be hard to add, so please decide :)

I know David implemented some maildir tests. I can't remember if they ran both twice or not, or if it was just some additional tests in places.

I believe the intent was to do a complete switch over at some stage, and hence this wouldn't have been running the suites twice (although there would likely be something vague in the middle where both existed, and we probably would need the tests).

I would suggest having a quick look around. For now, mbox should be enough I think.

Comment on attachment 8377817
patch v5

Review of attachment 8377817:
-----------------------------------------------------------------

I'm good with:

A subfolder with the name '%$1S' already exists in the folder '%2$S'. Would you like to move this folder using the new name '%3$S'?

(So sans the 'of' part of Neil's suggestion)

Created attachment 8390075
patch v6 - local folders

Oh, looks like this got all the reviews. Marking for superreview if one is needed for the changed idl.

Comment on attachment 8390075
patch v6 - local folders

Review of attachment 8390075:
-----------------------------------------------------------------

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +3843,5 @@
>
> +nsresult
> +nsMsgDBFolder::ConfirmAutoFolderRename(nsIMsgWindow *msgWindow,
> + const nsString aOldName,
> + const nsString aNewName,

These should be const nsString &... to avoid unnecessary string copies.

@@ +3846,5 @@
> + const nsString aOldName,
> + const nsString aNewName,
> + bool *confirmed)
> +{
> + NS_ENSURE_ARG_POINTER(confirmed);

It is possible that confirmed never gets set, so you might then end up with a bad value.

Additionally I don't really see the point of returning nsresult here, when you're already warning for errors, and you treat a failure as not confirmed anyway.

So I think it would be simpler just to change this function to return the bool.

Created attachment 8390810
patch v7 - local folders

Thanks, converted the function to bool.

Why don't You allow double/multiple entries, may be
- add the delete timestamp
- add the different parent folders

*** Bug 987608 has been marked as a duplicate of this bug. ***

https://hg.mozilla.org/comm-central/rev/3320c766cf5b

*** Bug 998862 has been marked as a duplicate of this bug. ***

should we not do the rest in a followup bug? ref: [leave open after checkin]

Yes, I am currently not working on the remaining parts. You can close this for ease of tracking in which TB version this went.

Bug 125864 is open, Trash does not maintain deleted folder's hierarchy location

Is anything else left that needs a new bug report?

Other types of accounts (other than "local" ones), e.g. IMAP need testing and if they don't work then they need a bug.

I Don't know if this is the same old bug again (or something completely new), but today I noticed that my TB 31.2.0 under RHEL6.6 (64-bit) didn't delete an IMAP folder (In Exchange) if a folder with the same name already existed in the Trash. It just silently didn't delete it, until I renamed the same name folder in Trash to something else. In this case the folder to be deleted was in "IMAP:/.../Archive/", as if that should make any difference.

See comment 142. If you see the problem on IMAP, please open a new bug. We knowingly fixed it only for POP3/Local accounts in this bug.

(In reply to :aceman from comment #144)
> See comment 142. If you see the problem on IMAP, please open a new bug. We
> knowingly fixed it only for POP3/Local accounts in this bug.

Ok, will do...