Deleting a folder fails when there is already a folder with the same name in Trash

RESOLVED FIXED in Thunderbird 31.0

Status

RESOLVED FIXED
18 years ago
2 years ago

People

(Reporter: venur, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 31.0

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: see comment #20 for current behaviour)

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

18 years ago
From Bugzilla Helper:
User-Agent: Mozilla/4.73 [en] (WinNT; I)
BuildID:    2001012404

Delete any of the system folders like Inbox, Drafts, templates. deleted folder( 
say Sent folder) and it's contents will be moved to Trash.. Restart the Browser 
, a folder by name Sent will be created automatically and previously deleted 
Sent folder and it's contents are still there in Trash. At this moment if we 
delete the Sent folder again, it will just replace the already existing folder 
in Trash. 

Is this is the way it is designed? It will be better if implemeted as what 
Netscape 4.6 does. N4.6 renames the already existing folder(Sent1) and adds 
"Sent"  to trash, hence retaining all.

Reproducible: Always
over to naving
Assignee: sspitzer → naving

Updated

18 years ago
QA Contact: esther → sheelar

Comment 2

18 years ago
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.  
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

18 years ago
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.

Comment 4

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

Comment 5

18 years ago
Created attachment 26204 [details] [diff] [review]
proposed fix

Comment 6

18 years ago
Just made it like 4x. cc bienvenu, sspitzer for review.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 7

18 years ago
Sorry for marking it fixed !
Status: RESOLVED → REOPENED
Resolution: 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*()

Comment 9

18 years ago
>>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?


Comment 10

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

Comment 11

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

Comment 12

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

Comment 13

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

Comment 14

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

Comment 15

17 years ago
similar: bug 46365

Comment 16

17 years ago
changing QA contact from Sheela to Gregg
QA Contact: sheelar → meehansqa

Comment 17

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

Comment 18

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

Comment 19

16 years ago
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?

Comment 21

16 years ago
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.

Updated

16 years ago
Blocks: 125864
mass re-assign.
Assignee: naving → sspitzer
Status: REOPENED → NEW

Updated

15 years ago
Severity: normal → critical
Keywords: dataloss
*** 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. ***
Product: Browser → Seamonkey
*** Bug 208114 has been marked as a duplicate of this bug. ***
Bug 71348 is for same symptom on IMAP.

Updated

14 years ago
Component: MailNews: Main Mail Window → MailNews: Backend
Product: Mozilla Application Suite → Core
*** 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. ***

Comment 31

14 years ago
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.

Comment 32

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

Comment 33

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

Comment 34

13 years ago
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.
Severity: critical → normal
Keywords: dataloss
OS: Windows NT → All
Hardware: PC → All
Summary: Sending a folder to trash, when already a folder by the same name is there in Trash.. → Deleting a folder fails when there is already a folder with the same name in Trash
Whiteboard: see comment #20 for current behaviour
Assignee: sspitzer → nobody
QA Contact: meehansqa → backend
*** Bug 281236 has been marked as a duplicate of this bug. ***
*** Bug 357702 has been marked as a duplicate of this bug. ***
Duplicate of this bug: 397991

Updated

11 years ago
Duplicate of this bug: 46365
Duplicate of this bug: 405493

Comment 40

11 years ago
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.
Duplicate of this bug: 431314
Duplicate of this bug: 448101
Product: Core → MailNews Core
Duplicate of this bug: 458123

Comment 44

10 years ago
This is still an issue in thunderbird 2.0.0.19 and 3.0~b2. here is the bug from Ubuntu for any info that might be needed.
https://bugs.launchpad.net/ubuntu/+source/mozilla-thunderbird/+bug/214366

This happens on seamonkey as well as Tunderbird

Comment 45

10 years ago
This is still an issue in thunderbird 2.0.0.19 and 3.0~b2. here is the bug from Ubuntu for any info that might be needed.
https://bugs.launchpad.net/ubuntu/+source/mozilla-thunderbird/+bug/214366

This happens on seamonkey as well as Tunderbird
Duplicate of this bug: 477602
Flags: wanted-thunderbird3?
Duplicate of this bug: 520145

Comment 48

9 years ago
Thunderbird 3.0
Can't move folder to "Deleted" if a folder with the same name already exists.

	Move "test" to "Deleted"
	Create new folder "test"
	Move second "test" to "Deleted"
	Error: Folder already exists

Ideally, the second "test" folder should be moved to "deleted" as "test(2)" or something.
Duplicate of this bug: 537561
Duplicate of this bug: 550836

Comment 51

9 years ago
(In reply to comment #48)
> Thunderbird 3.0
> Can't move folder to "Deleted" if a folder with the same name already exists.
> 
>     Move "test" to "Deleted"
>     Create new folder "test"
>     Move second "test" to "Deleted"
>     Error: Folder already exists
> 
> Ideally, the second "test" folder should be moved to "deleted" as "test(2)" or
> something.

This is not an issue. It should not have to change the name to be able to delete it.

Comment 52

9 years ago
> This is not an issue. It should not have to change the name to be able to
> delete it.

Strange requirement: make additional processing on item which user want to trash.

This is an issue: user dont want to worry about what already placed in Trash - user want to delete this folder. Delete this folder. Just now.

I mean this TB's behaviour is a sample of stupidity: program don't let user make their work because of 'violation' some sort of 'logical structure'.
As user mark this as  trash do something with folder name for moving it to trash -- add unique postfix, add random number etc.

Comment 53

9 years ago
I have noticed this too, just the other day, in fact.  Renaming the folder (a subfolder of "Local Folders\Personal Folders" before deleting it is a workaround.  

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.1.9) Gecko/20100317 Thunderbird/3.0.4
Duplicate of this bug: 581780
Duplicate of this bug: 598053
Fout: uncaught exception: [Exception... "Component returned failure code: 0x80550013 [nsIMsgFolder.deleteSubFolders]"  nsresult: "0x80550013 (<unknown>)"  location: "JS frame :: chrome://messenger/content/folderPane.js :: ftc_delete :: line 2201"  data: no]
Duplicate of this bug: 653696
(Assignee)

Updated

7 years ago
Duplicate of this bug: 708625
(Assignee)

Comment 59

7 years ago
Would comment 52 be an acceptable solution for everyone?
It seems the old patch is attempting to do that.
I could try to pick it up.
Assignee: nobody → acelists
Flags: wanted-thunderbird3?

Comment 60

7 years ago
(In reply to :aceman from comment #59)
> Would comment 52 be an acceptable solution for everyone?
> It seems the old patch is attempting to do that.
> I could try to pick it up.

Yes, that's fine, similar to what windows file system explorer does. That would be great if you could fix this!
(Assignee)

Comment 61

7 years ago
It seems the file has changed too much. I have no idea where to put this code. The flow is too different now.
Assignee: acelists → nobody
Duplicate of this bug: 712912
(Assignee)

Updated

7 years ago
Blocks: 92165
(Assignee)

Comment 63

7 years ago
Created attachment 609152 [details] [diff] [review]
proof of concept patch

I managed to strip the patch down and update it for the current architecture.
I need feedback from David Bienvenu if this is a good direction and reaction to the points below.

What it does:
- it does work to fix the problem as reported.
- if the folder 'Folder' is already existing in Trash, this will try to move it as 'Folder2', 'Folder3' (to fix comment 8).

What it is missing for now:
- if CopyFolderLocal is used not only for moving to trash then this renaming functionality must be done only if isChildOfTrash is true.
- it doesn't have the alert dialog informing the user the folder was renamed.

Problems:
- for some reason when a folder is deleted and then a new folder with the same name is created in its place, then it can't be deleted, without any error. It that is some other known bug or a bug in this code? After TB restart that same folder can be deleted fine.
Assignee: nobody → acelists
Attachment #26204 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #609152 - Flags: feedback?(dbienvenu)
> - it doesn't have the alert dialog informing the user the folder was renamed.

That doesn't sound to me like something we'd want.

I'd too (like comment 8) prefer a folder-N, not folderN naming scheme.
(Assignee)

Comment 65

7 years ago
(In reply to Magnus Melin from comment #64)
> > - it doesn't have the alert dialog informing the user the folder was renamed.
> That doesn't sound to me like something we'd want.
It would be easier without it, but let's ask bwinton too.
 
> I'd too (like comment 8) prefer a folder-N, not folderN naming scheme.

The dash in comment 8 can be probably added trivially if we decide on it. The main point from comment 8 is that the original patch tried folder names like this: Folder0, Folder01, Folder012, etc... My patch tries Folder2, Folder3, Folder4, etc...
(Assignee)

Updated

7 years ago
Attachment #609152 - Flags: feedback?(bwinton)

Comment 66

7 years ago
(In reply to :aceman from comment #65)
> (In reply to Magnus Melin from comment #64)
> > I'd too (like comment 8) prefer a folder-N, not folderN naming scheme.
> 
> The dash in comment 8 can be probably added trivially if we decide on it.
> The main point from comment 8 is that the original patch tried folder names
> like this: Folder0, Folder01, Folder012, etc... My patch tries Folder2,
> Folder3, Folder4, etc...

I would prefer a space before the number:
My Folder 1
Or better (like in Windows Explorer):
My Folder (1)
Copy(1) of My Folder
Duplicate(1) of My Folder
(Assignee)

Comment 67

7 years ago
For those interested, Outlook 2007 renames the folder to 'Folder1' (no space) if 'Folder' is already existing. At least in the SK language version.

Comment 68

7 years ago
(In reply to :aceman from comment #67)
> For those interested, Outlook 2007 renames the folder to 'Folder1' (no
> space) if 'Folder' is already existing. At least in the SK language version.

I would say: "Bug" in MS's corporate Identity ;-)
Imagine, someone just has Folder1. It would result in Folder11. Do you want this?
(Assignee)

Comment 69

7 years ago
Yes, that does really happen :) Delete 'Folder', 'Folder', and 'Folder1'.

I do not want that but I do not care much. I implement whatever the UI guys decide. I just provide options :)
Keywords: uiwanted

Comment 70

7 years ago
Comment on attachment 609152 [details] [diff] [review]
proof of concept patch

thx for the patch - I think we really want to do this, but there are issues with the patch.

I don't like the rename of the src folder before the move - that might fail as well, if the name exists in the source folder's directory. I'd prefer to move to the right name initially. And I don't think CopyFolderLocal is the right place for this either - I suspect it wants to be at a higher level, though I'd need to poke around to find the right place.

I'd also like to fix this for imap, which has the same issue.
Attachment #609152 - Flags: feedback?(dbienvenu) → feedback-
(Assignee)

Comment 71

7 years ago
Thanks David. If you find the right place I can look at it again. Otherwise it is again beyond me.
Comment on attachment 609152 [details] [diff] [review]
proof of concept patch

This seems like reasonable behaviour to me.  I agree with Magnus that I'ld prefer it to end in "-1", "-2", etc…  (Sorry, Ulf. :)

(As a side note, OS X, when you delete the same thing twice, names the second "filename.txt" as "filename 14:59:27 .txt" (if you deleted it at 2:59pm ;).)

Later,
Blake.
Attachment #609152 - Flags: feedback?(bwinton) → feedback+
(Assignee)

Updated

7 years ago
No longer blocks: 92165

Comment 73

7 years ago
Maybe you could rename Folder-1 back to Folder, if Folder would be deleted from trash.

Comment 74

7 years ago
can You please fix it with Your favorite solution !

this issue is already very very old without any progress.
reported Jan-2001

and if there is time it can be udated later ?


: if already exists in trash it could also be moved to
- trash/1/foldername
- trash/2/foldername
so name must not be touch for move.


but no matter how,
please help with Your favorite solution/fix

thanks, Bernhard

Comment 75

7 years ago
It might be better use aliases for folders (and files) in the trash I think.
And add a file with pair of data: alias:real_name.

I meen like this:
In the TB window:
Trash
  Folder
  Folder
  File1
  Folder
  File2
  File1

On the FS:
Trash
  UfatVuHy
  tynkEejRyp
  3jofmamOo
  HajbymFo
  faydfehacs
  RureegojDa

In the additional file:
UfatVuHy:Folder
tynkEejRyp:Folder
3jofmamOo:File1
HajbymFo:Folder
faydfehacs:File2
RureegojDa:File1

Something like this.
(Assignee)

Comment 76

7 years ago
The folder name is not a problem. There are technical issues with the implementation so we are waiting on David Bienvenu.
(In reply to :aceman from comment #71)
> Thanks David. If you find the right place I can look at it again. Otherwise
> it is again beyond me.

ref comment 70
Flags: needinfo?(irving)
I agree with David's remark in comment 70, that we should not rename the folder before copying - we should just copy it directly into the new name. I don't think we need to do anything complicated to deal with duplicate names; appending a number is probably enough.

I don't have any insight into where would be the best place to implement this; if David doesn't know off the top of his head, somebody's going to have to do the hard work of reverse engineering what's already there.
Flags: needinfo?(irving)

Comment 80

5 years ago
The fact that this still hasn't been resolved in the public release after almost 15 years is, quite frankly, disgusting. 

These kind of amateurish bugs and the utter inability of anyone to fix them years, or in this case, DECADES later is exactly why nobody takes open-source software seriously.

Rename either folder in some way and be done with it. This does not need to be debated until our eyes and ears bleed. Make a command decision and fix it one way or the other. Delete should just work.
(Assignee)

Comment 81

5 years ago
Created attachment 8370331 [details] [diff] [review]
WIP patch 2

This is an updated version that implements the atomic move of the folder to the final destination, as requested.

The search for a new free folder name is not inside the block where we check if this move is to the Trash folder. That means this automatic renaming works also when copying/moving/dragging to any other folder. That could be a nice feature. Do we want it? Or do we only want this when moving a folder to Trash?

Neil is this still a wrong place to do it and can you propose a better one that David Bienvenu was talking about?
Attachment #609152 - Attachment is obsolete: true
Attachment #8370331 - Flags: ui-review?(josiah)
Attachment #8370331 - Flags: feedback?(neil)
Attachment #8370331 - Flags: feedback?(irving)
Comment on attachment 8370331 [details] [diff] [review]
WIP patch 2

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

So this seems like a fine approach to me. I think that we should call each folder "folder(x)" instead of "folder-x", as it more clearly indicates duplicates though. People may already have folders called Name-Subname or the like, so another reason to use parenthesis.

I also definitely think we should expand this to other folders, not just trash.
Attachment #8370331 - Flags: ui-review?(josiah) → ui-review+

Comment 83

5 years ago
To avoid future bug reports I would suggest changing the existing Dialogue to:
A folder with name "Child" is already existing in folder "Parent". Do you want to rename it to "Child(2)" or fill it's content into existing folder "Child"?
[Yes] [No] [Cancel]
(Assignee)

Comment 84

5 years ago
I wouldn't try to implement merging in this bug (it is much harder). But I think I could make that dialog with Yes/Cancel options meaning Rename/Cancel. For Trash, the rename would happen without question. If UI guys like that :)
Yeah, for trash moving automatically is fine.

Comment 86

5 years ago
(In reply to :aceman from comment #84)
> I wouldn't try to implement merging in this bug (it is much harder).

What's the problem, moving a (bunch of) message(s) to another folder is already implemented and the suggested dialogue is well known from file managers. I just mean adding, not merging with existing messages.

Comment 87

5 years ago
(In reply to Ulf Zibis from comment #86)
> (In reply to :aceman from comment #84)
> > I wouldn't try to implement merging in this bug (it is much harder).
> 
> What's the problem, moving a (bunch of) message(s) to another folder is
> already implemented and the suggested dialogue is well known from file
> managers. I just mean adding, not merging with existing messages.

You seem to ignore that folders can (and will) not only contain messages, but subfolders as well (where we will run into the same problem again if the subfolder has the same name as an existing folder in the destination folder). Certainly doable, but way out of scope of this bug in my opinion.

My opinion: Can we please fix this bug quickly (I regularly rename folders just to be able to delete them...) instead of building the ideal bike shed for everybody? Auto-renaming folders when moved elsewhere is fine for me, asking the user about it is fine too, but if we try to add merging functionality (with its own problems) this bug will not be fixed within the next 10 years, I suppose.
Comment on attachment 8370331 [details] [diff] [review]
WIP patch 2

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

I'm OK with this approach. I'd like to see test cases for both a single folder and a nested folder - for example:

make folder A
move A to trash
make new A
move A to trash
make new A
make new A/B
move A to trash
make sure trash has A and A-1 and A-2 and A-2/B

We don't need to test all those combinations for every message store type, but we should at least test the copyFolder(...aNewName) parameter works once on each store type.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +1732,5 @@
> +  while (1) {
> +    rv = CheckIfFolderExists(newFolderNameUnderTrash, this, NULL);
> +    if (rv == NS_MSG_FOLDER_EXISTS) {
> +      newFolderNameUnderTrash.Assign(folderName);
> +      newFolderNameUnderTrash.AppendLiteral("-"); // do we want this localizable? "<folder>-<SeqNo>"

I don't think this needs to be localizable.
Attachment #8370331 - Flags: feedback?(irving) → feedback+
(Assignee)

Comment 89

5 years ago
(In reply to :Irving Reid from comment #88)
> I'm OK with this approach. I'd like to see test cases for both a single
> folder and a nested folder - for example:
> 
I'll try.

> We don't need to test all those combinations for every message store type,
> but we should at least test the copyFolder(...aNewName) parameter works once
> on each store type.
Store type as in mbox and maildir? Do we actually have tests differentiating the stores?
Can't bug 765926 cover maildir by itself?

> ::: mailnews/local/src/nsLocalMailFolder.cpp
> @@ +1732,5 @@
> > +  while (1) {
> > +    rv = CheckIfFolderExists(newFolderNameUnderTrash, this, NULL);
> > +    if (rv == NS_MSG_FOLDER_EXISTS) {
> > +      newFolderNameUnderTrash.Assign(folderName);
> > +      newFolderNameUnderTrash.AppendLiteral("-"); // do we want this localizable? "<folder>-<SeqNo>"
> 
> I don't think this needs to be localizable.
I think it should be localizable if it is not complicated.

And I agree with comment 87.
(In reply to :aceman from comment #89)

> > We don't need to test all those combinations for every message store type,
> > but we should at least test the copyFolder(...aNewName) parameter works once
> > on each store type.
> Store type as in mbox and maildir? Do we actually have tests differentiating
> the stores?
> 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.
(In reply to aceman from comment #89)
> (In reply to Irving Reid from comment #88)
> > I don't think this needs to be localizable.
> I think it should be localizable if it is not complicated.
It doesn't need to be localisable (compare uniqueFile from contentAreaUtils.js).
(In reply to aceman from comment #81)
> Neil is this still a wrong place to do it and can you propose a better one
> that David Bienvenu was talking about?
Well, as I see it there are two options:
1. Make moving a folder automatically rename if the destination already exists, but allow copies to continue to merge the folders. (This was a lifesaver for me when I was moving mail between two accounts and hadn't provided sufficient quota on the destination account first.) Also make this work for IMAP.
2. Propagate the optional destination folder name throughout the mailnews codebase, including the copy service and the imap service. Then you would update the two implementations of DeleteSubFolders to handle the duplicate folder name case.
(Assignee)

Comment 93

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #92)
> Well, as I see it there are two options:
> 1. Make moving a folder automatically rename if the destination already
> exists, but allow copies to continue to merge the folders. (This was a
> lifesaver for me when I was moving mail between two accounts and hadn't
> provided sufficient quota on the destination account first.) Also make this
> work for IMAP.
In which case does copy merge folders already?

> 2. Propagate the optional destination folder name throughout the mailnews
> codebase, including the copy service and the imap service. Then you would
> update the two implementations of DeleteSubFolders to handle the duplicate
> folder name case.
If I did it in DeleteSubFolders we would probably loose the automatic renaming when a normal copy occurs outside of Trash, which was of interest to users in the previous comments.
Comment on attachment 8370331 [details] [diff] [review]
WIP patch 2

>+  PRUint32 i = 2;
>+  nsAutoString newFolderNameUnderTrash(folderName);
>+  while (1) {
>+    rv = CheckIfFolderExists(newFolderNameUnderTrash, this, NULL);
>+    if (rv == NS_MSG_FOLDER_EXISTS) {
>+      newFolderNameUnderTrash.Assign(folderName);
>+      newFolderNameUnderTrash.AppendLiteral("-"); // do we want this localizable? "<folder>-<SeqNo>"
>+      newFolderNameUnderTrash.AppendInt(i);
>+      i++;
>+    } else if (NS_SUCCEEDED(rv)) {
>+      // NS_OK so the tried folder name doesn't exist
>+      break;
>+    } else {
>+      return rv;
>+    }
>+  }
CheckIfFolderExists is overkill if you don't want to alert, use ContainsChildNamed instead. Also, the loop is horribly written, though maybe you might think my version is going too far the other way:
bool containsChild;
for (int32_t i = 2; ContainsChildNamed(destFolderName, &containsChild), containsChild; i++) {
  // compute new folder name
}
Attachment #8370331 - Flags: feedback?(neil)
(Assignee)

Comment 95

5 years ago
(In reply to :aceman from comment #93)
> (In reply to neil@parkwaycc.co.uk from comment #92)
> > Well, as I see it there are two options:
> > 1. Make moving a folder automatically rename if the destination already
> > exists, but allow copies to continue to merge the folders. (This was a
> > lifesaver for me when I was moving mail between two accounts and hadn't
> > provided sufficient quota on the destination account first.) Also make this
> > work for IMAP.
> In which case does copy merge folders already?
> 
In nsMsgLocalMailFolder::CopyFolder it seems when we are moving folder then it calls CopyFolderLocal (which I am touching). If we are copying then it calls CopyFolderAcrossServer (which I do not touch). So if CopyFolderAcrossServer merges, then all is fine. Is that what you meant?
Flags: needinfo?(neil)
(In reply to aceman from comment #95)
> In nsMsgLocalMailFolder::CopyFolder it seems when we are moving folder then
> it calls CopyFolderLocal (which I am touching). If we are copying then it
> calls CopyFolderAcrossServer (which I do not touch). So if
> CopyFolderAcrossServer merges, then all is fine. Is that what you meant?

Technically yes, but that's more by luck than design.
Flags: needinfo?(neil)

Comment 97

5 years ago
(In reply to Josiah Bruner [:JosiahOne] from comment #82)
> So this seems like a fine approach to me. I think that we should call each
> folder "folder(x)" instead of "folder-x", as it more clearly indicates
> duplicates though. People may already have folders called Name-Subname or
> the like, so another reason to use parenthesis.
+1
The "(x)" pattern is well known from file managers, so why differ here?
There is already another mechanism using the "-x" pattern e.g. in case of lost synchronisation via IMAP where panacea.dat resolves the redirection. Using the same pattern for different reasons could raise ambiguity problems.

> I also definitely think we should expand this to other folders, not just trash.
(In reply to :aceman from comment #84)
> But I think I could make that dialog with Yes/Cancel options meaning
> Rename/Cancel. For Trash, the rename would happen without question.
(In reply to Josiah Bruner [:JosiahOne] from comment #85)
> Yeah, for trash moving automatically is fine.
I think, the decision if a dialogue is shown should not be upon destination folder Trash, but upon the involved action:
Delete action     ->  automatically rename to Folder(x)
Move/Copy action  ->  ask user for renaming or cancel
Reasons:
- If user uses Move/Copy instead of the easier Delete he may have some special reason e.g. using the Trash as temporary location, so user should be warned as for any other Move/Copy.
- If we later extend with merge functionality, user is guaranteed to have the merge functionality even for Trash when using the Move/Copy actions.
(Assignee)

Comment 98

5 years ago
Created attachment 8371072 [details] [diff] [review]
WIP patch 3

This adds the requested test (as xpcshell so Seamonkey gets it too).
Also covers all the feedback from Neil, Irving, Josiah and Ulf.
When moving a folder outside Trash, it asks about the rename. It does not ask when moving to Trash = deleting. I can't distinguish between clicking Delete and manually dragging to Trash. That would need sending the distinguishing flag through all the layers of functions from UI to backend and I'd not want to do that in this bug.

Josiah, decide if we want to note the proposed final folder name in the renaming question as Ulf proposes.

I'll look at IMAP after we settle on what layer this needs to be. Seems the patch here does option 1. from Neil's comment.
Attachment #8370331 - Attachment is obsolete: true
Attachment #8371072 - Flags: ui-review?(josiah)
Attachment #8371072 - Flags: feedback?(neil)
Attachment #8371072 - Flags: feedback?(irving)
(Assignee)

Comment 99

5 years ago
(In reply to Ulf Zibis from comment #97)
> (In reply to Josiah Bruner [:JosiahOne] from comment #82)
> > So this seems like a fine approach to me. I think that we should call each
> > folder "folder(x)" instead of "folder-x", as it more clearly indicates
> > duplicates though. People may already have folders called Name-Subname or
> > the like, so another reason to use parenthesis.
> +1
> The "(x)" pattern is well known from file managers, so why differ here?
Josiah does not propose to differ. The last patch does rename to (x).

> > I also definitely think we should expand this to other folders, not just trash.
> (In reply to :aceman from comment #84)
> > But I think I could make that dialog with Yes/Cancel options meaning
> > Rename/Cancel. For Trash, the rename would happen without question.
> (In reply to Josiah Bruner [:JosiahOne] from comment #85)
> > Yeah, for trash moving automatically is fine.
> I think, the decision if a dialogue is shown should not be upon destination
> folder Trash, but upon the involved action:
> Delete action     ->  automatically rename to Folder(x)
> Move/Copy action  ->  ask user for renaming or cancel
> Reasons:
> - If user uses Move/Copy instead of the easier Delete he may have some
> special reason e.g. using the Trash as temporary location, so user should be
> warned as for any other Move/Copy.
I think user should never move to Trash as temporary measure. Notice may have the automatic mechanisms enabled that remove messages from trash (retention settings e.g. old messages, only x last messages, etc.) so he may easily loose those temporarily moved messages. He gets the warning about "Really delete" in both of the cases you describe. That should be a clue for him.
(Assignee)

Comment 100

5 years ago
(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 :)
Flags: needinfo?(mbanner)

Comment 101

5 years ago
(In reply to :aceman from comment #99)
> Josiah does not propose to differ. The last patch does rename to (x).
Great!

> I think user should never move to Trash as temporary measure.
Well, You'll never know, what user does, ...and there is the other reason mentioned above. Additionally it looks more clean to me, if the user is warned upon normal Move/Copy action.
Comment on attachment 8371072 [details] [diff] [review]
WIP patch 3

Quick Q: Has someone tested what happens in the undo case? Does the folder get named to what it was previously.
(Assignee)

Comment 103

5 years ago
(In reply to Mark Banner (:standard8) from comment #102)
> Quick Q: Has someone tested what happens in the undo case? Does the folder
> get named to what it was previously.

I'll check that :)

We'd like to know from you if you have any idea about comment 88, 90 and 100.

Comment 104

5 years ago
(In reply to Mark Banner (:standard8) from comment #102)
> Quick Q: Has someone tested what happens in the undo case? Does the folder
> get named to what it was previously.

Great Q !
I too had this Q in my synapses last night before falling into dreams.
(Assignee)

Comment 105

5 years ago
(In reply to Mark Banner (:standard8) from comment #102)
> Quick Q: Has someone tested what happens in the undo case? Does the folder
> get named to what it was previously.
Not sure what I am doing wrong but I get no option to Undo after drag-and-dropping a folder. Undo is disabled in Edit menu. If I copy a message (using the context menu and Copy to, I get Undo possibility).
Comment on attachment 8371072 [details] [diff] [review]
WIP patch 3

>+  nsAutoString newFolderName(EmptyString());
[Strings default to empty...]

>+    while (containsChild) {
>+      rv = ContainsChildNamed(newFolderName, &containsChild);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      if (!containsChild)
>+        break;
[Weird use of loop condition.]

>+      newFolderName.AppendLiteral("(");
[Could use Append('(');]

>+confirmDuplicateFolderRename=A folder with that name already exists. Would you like to copy the folder under a different name?
You should include the suggested new folder name in the message, otherwise the user will expect to choose the new name.
Attachment #8371072 - Flags: feedback?(neil) → feedback+
Comment on attachment 8371072 [details] [diff] [review]
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);
}

Comment 108

5 years ago
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 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).

Comment 110

5 years ago
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.

Comment 113

5 years ago
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?

Comment 114

5 years ago
(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
(Assignee)

Comment 115

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #107)
> Comment on attachment 8371072 [details] [diff] [review]
> 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 :)
(Assignee)

Comment 116

5 years ago
(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.
Duplicate of this bug: 305728
Comment on attachment 8371072 [details] [diff] [review]
WIP patch 3

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

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"));
Attachment #8371072 - Flags: feedback?(irving) → feedback+
(Assignee)

Comment 119

5 years ago
Created attachment 8374439 [details] [diff] [review]
WIP patch 4

Thanks, addressed all comments and added a new test segment to check for the prompt when moving duplicate folder outside Trash.
Attachment #8371072 - Attachment is obsolete: true
Attachment #8371072 - Flags: ui-review?(josiah)
Attachment #8374439 - Flags: feedback?(neil)
Attachment #8374439 - Flags: feedback?(irving)
Comment on attachment 8374439 [details] [diff] [review]
WIP patch 4

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

::: 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?
Attachment #8374439 - Flags: feedback?(irving) → feedback+
(Assignee)

Comment 121

5 years ago
(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 [details] [diff] [review]
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'?
(Assignee)

Comment 123

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #122)
> Comment on attachment 8374439 [details] [diff] [review]
> 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 [details] [diff] [review])
> 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.)
(Assignee)

Comment 125

5 years ago
Created attachment 8377817 [details] [diff] [review]
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.
Attachment #8374439 - Attachment is obsolete: true
Attachment #8374439 - Flags: feedback?(neil)
Attachment #8377817 - Flags: review?(neil)
Attachment #8377817 - Flags: review?(irving)
Comment on attachment 8377817 [details] [diff] [review]
patch v5

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

(In reply to neil@parkwaycc.co.uk from comment #124)
> (In reply to aceman from comment #123)
> > (In reply to comment #122)
> > > (From update of attachment 8374439 [details] [diff] [review])
> > 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.
Attachment #8377817 - Flags: review?(irving) → review+
(Assignee)

Comment 127

5 years ago
Comment on attachment 8377817 [details] [diff] [review]
patch v5

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

Josiah, please see the discussion about the wording of the message and decide on ui-r :)
Attachment #8377817 - Flags: ui-review?(josiah)
(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 [details] [diff] [review]
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!
Attachment #8377817 - Flags: review?(neil) → review+
(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.
Flags: needinfo?(mbanner)
Comment on attachment 8377817 [details] [diff] [review]
patch v5

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

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)
Attachment #8377817 - Flags: ui-review?(josiah) → ui-review+
(Assignee)

Comment 132

5 years ago
Created attachment 8390075 [details] [diff] [review]
patch v6 - local folders

Oh, looks like this got all the reviews. Marking for superreview if one is needed for the changed idl.
Attachment #8377817 - Attachment is obsolete: true
Attachment #8390075 - Flags: superreview?(standard8)
Attachment #8390075 - Flags: review+
Comment on attachment 8390075 [details] [diff] [review]
patch v6 - local folders

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

::: 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.
Attachment #8390075 - Flags: superreview?(standard8) → superreview-
(Assignee)

Comment 134

5 years ago
Created attachment 8390810 [details] [diff] [review]
patch v7 - local folders

Thanks, converted the function to bool.
Attachment #8390075 - Attachment is obsolete: true
Attachment #8390810 - Flags: superreview?(standard8)
Attachment #8390810 - Flags: superreview?(standard8) → superreview+
(Assignee)

Updated

5 years ago
Keywords: uiwanted → checkin-needed
Whiteboard: see comment #20 for current behaviour → see comment #20 for current behaviour, [leave open after checkin]

Comment 135

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

Updated

5 years ago
Duplicate of this bug: 987608
Duplicate of this bug: 998862
should we not do the rest in a followup bug?  ref: [leave open after checkin]
Flags: needinfo?(acelists)
(Assignee)

Comment 140

4 years ago
Yes, I am currently not working on the remaining parts. You can close this for ease of tracking in which TB version this went.
Flags: needinfo?(acelists)
Bug 125864 is open, Trash does not maintain deleted folder's hierarchy location

Is anything else left that needs a new bug report?
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: see comment #20 for current behaviour, [leave open after checkin] → see comment #20 for current behaviour
Target Milestone: --- → Thunderbird 31.0
(Assignee)

Comment 142

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

Comment 143

4 years ago
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.
(Assignee)

Comment 144

4 years ago
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.

Comment 145

4 years ago
(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...

Updated

2 years ago
Duplicate of this bug: 1321634
You need to log in before you can comment on or make changes to this bug.