Closed Bug 66763 Opened 24 years ago Closed 10 years ago

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

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 31.0

People

(Reporter: venur, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Whiteboard: see comment #20 for current behaviour)

Attachments

(1 file, 7 obsolete files)

patch v7 - local folders
21.62 KB, patch
standard8
: superreview+
Details | Diff | Splinter Review
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
QA Contact: esther → sheelar
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
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.
Attached patch proposed fix (obsolete) — — Splinter Review
Just made it like 4x. cc bienvenu, sspitzer for review.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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*()
>>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
QA Contact: sheelar → meehansqa
*** 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.
Blocks: 125864
mass re-assign.
Assignee: naving → sspitzer
Status: REOPENED → NEW
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.
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. ***
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.
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. ***
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.
Product: Core → MailNews Core
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
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
Flags: wanted-thunderbird3?
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.
(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.
> 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.
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
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]
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?
(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!
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
Blocks: 92165
Attached patch proof of concept patch (obsolete) — — Splinter Review
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.
(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...
Attachment #609152 - Flags: feedback?(bwinton)
(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
For those interested, Outlook 2007 renames the folder to 'Folder1' (no space) if 'Folder' is already existing. At least in the SK language version.
(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?
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 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-
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+
No longer blocks: 92165
Maybe you could rename Folder-1 back to Folder, if Folder would be deleted from trash.
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
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.
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)
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.
Attached patch WIP patch 2 (obsolete) — — Splinter Review
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+
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]
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.
(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.
(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+
(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.
(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)
(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)
(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.
Attached patch WIP patch 3 (obsolete) — — Splinter Review
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)
(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.
(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)
(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.
(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.
(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.
(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); }
(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).
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 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 :)
(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.
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+
Attached patch WIP patch 4 (obsolete) — — Splinter Review
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+
(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'?
(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.)
Attached patch patch v5 (obsolete) — — Splinter Review
> > > >+ 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+
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+
Attached patch patch v6 - local folders (obsolete) — — Splinter Review
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-
Thanks, converted the function to bool.
Attachment #8390075 - Attachment is obsolete: true
Attachment #8390810 - Flags: superreview?(standard8)
Attachment #8390810 - Flags: superreview?(standard8) → superreview+
Keywords: uiwanted → checkin-needed
Whiteboard: see comment #20 for current behaviour → see comment #20 for current behaviour, [leave open after checkin]
Why don't You allow double/multiple entries, may be - add the delete timestamp - add the different parent folders
should we not do the rest in a followup bug? ref: [leave open after checkin]
Flags: needinfo?(acelists)
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
Closed: 24 years ago → 10 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
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...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: