Using aug06 commercial trunk build, win98 (assume other platforms same) Related bug 41720 If I delete a folder which is named as destination to a filter, I should be warned when I take the delete action. As it stands now, I see nothing until I empty the trash after having deleted the folder. I think it is more valuable to the user to know they're mucking with a filter destination folder at the time they're deleting it, not after it's in the trash. Another issue (may need a separate bug) is that the current alert tells the user (when emptying trash) that the filters are now disabled. If we were to alert them when the delete action is taking place, we should also provide a Cancel option -- this is build in to delete folder confirmation. We should definitely provide a warning which has some way of cancelling the action.

Well moving to trash is not a physical delete and for imap it is just like renaming. However when you delete under trash or empty trash I can ask for confirmation before deletion and stop the deletion.

I would agree with Laurel that the dialog is more useful to users when the folder is being moved into the Trash than when the Trash is being emptied. The user may move the folder into the Trash (not realizing they created a filter associated with the folder) and then weeks later decide the empty the Trash. Weeks later, the warning dialog may not make sense. In 4.x, when you move an IMAP folder to the Trash, you get a confirmation dialog (Are you sure you want to move the selected folder into the Trash?). (mozilla doesn't current do this, another bug?)

ok, I have made the fix as desired by jglick and laurel. The folder deletion confirm happens at the time of deletion. The fix is to detect if the destination folder is trash and then ask for confirmation. While fixing this part I found some problems w/ D&D of folders and I fixed them as well. I have made to make quite a few changes to pass msgWindow around. This fix also includes the fix for bug 100037 where an alert w/ checkbox will appear telling user that the messages will go renamed/moved folder. The checkbox will allow users to turn it off if they do not wish to be prompted. cc bienvenu for code review robin, please review alert wording changes in messenger.properties (proposed fix).

Some suggestions. >Do you wish to delete folder '%S' because filter(s) are set on it. "Deleting the folder "%S" will disable its associated filter(s). Are you sure you want to delete the folder?" >Filters that are affected by renaming/moving folder '%S' will go to the new >destination. "Filters associated with this folder will be updated." Or as Robin suggested in bug 41720, two dialogs: "Filters that are affected by renaming this folder will be updated." "Filters that are affected by moving this folder will be updated."

david, need review.

Is this the minimum diff for this bug? It looks like it contains diffs I've already reviewed for other bugs. Is that just deja vu, or has code been copied? It looks OK, I guess, but there's a lot to look at and I want to make sure I'm looking at the right thing before diving into it.

The diffs are just for this bug. Lot of code to throw confirm and alert dialog are similar to just what you reviewed recently for another bug.

Now that you have that nice GetBaseBundle method, can you use it in your nsMsgDBFolder::AutoCompact(nsIMsgWindow *aWindow) routine? I don't need to re-review that. In base/util/nsMsgFolder.h, you don't need to include "nsIStringBundle.h", you can just forward declare "class nsIStringBundle". That might prevent a dependency (it might not, but it doesn't hurt to try). You only have to include the actual header file if you're going to use an nsCOMPtr as a member var of a class in a header file. (Of course, you'll probably have to add the #include to nsMsgFolder.cpp) You're not checking if aBundle is null in GetBaseStringBundle(nsIStringBundle **aBundle) but since this isn't a method of the interface, that's OK as long as you know no one's passing in null. I hate to see all the cloned code to put up confirmation dialogs about a folder, since it leads to code bloat (which is partly why this all seems so familiar). Would it be possible to put that in a utility method? Something like nsMsgFolder::ConfirmationPrompt(nsIMsgWindow *window, const char *stringName) that would put up the comfirmation dialog for that folder and string resource name?

>Now that you have that nice GetBaseBundle method, can you use it in your >nsMsgDBFolder::AutoCompact(nsIMsgWindow *aWindow) routine? I don't need to >re-review that. If you look at my patch more closely, I have already done that. >I hate to see all the cloned code to put up confirmation dialogs about a >folder, >since it leads to code bloat (which is partly why this all seems so familiar). >Would it be possible to put that in a utility method? Something like >nsMsgFolder::ConfirmationPrompt(nsIMsgWindow *window, const char *stringName) >that would put up the comfirmation dialog for that folder and string resource >name? I have at present only one method for throwing alert and another for throwing confirmation dialog in nsMsgFolder and I have already tried to take out the common code as much as possible. I will see if I can do it further..

sorry, you're right, you have done that first thing. Re getting rid of the code bloat, I could be missing something, but I don't see a routine like I described, that would take the propery string and a msg window, get the doc shell, the base string bundle, format the string with the folder name, put up the confirmation dialog, and return whether the dialog was confirmed or not.

>+confirmFolderDeletionForFilter=Do you wish to delete folder '%S' because >filter(s) are set on it. I prefer jglick's suggestion: "Deleting the folder "%S" will disable its associated filter(s). Are you sure you want to delete the folder?" >+alertFilterChanged=Filters that are affected by renaming/moving folder '%S' >will go to the new destination. I prefer jglick's suggestion: "Filters associated with this folder will be updated." Can you update your patch to use jglick's suggested wording? Thanks.

hey david, this piece of code, i believe addresses your concern. +NS_IMETHODIMP nsMsgFolder::ConfirmFolderDeletionForFilter(nsIMsgWindow *msgWindow, PRBool *confirmed) { nsresult rv = NS_OK; - PRBool changed =PR_FALSE; - rv = ChangeFilterDestination(nsnull, PR_FALSE, &changed); - if (msgWindow && changed) + if (msgWindow) { nsCOMPtr <nsIDocShell> docShell; msgWindow->GetRootDocShell(getter_AddRefs(docShell)); - nsCOMPtr<nsIStringBundleService> bundleService = - do_GetService(NS_STRINGBUNDLE_CONTRACTID); - if (bundleService) + nsCOMPtr <nsIStringBundle> bundle; + rv = GetBaseStringBundle(getter_AddRefs(bundle)); + if (NS_SUCCEEDED(rv) && bundle) + { + nsXPIDLString confirmString; + nsXPIDLString folderName; + GetName(getter_Copies(folderName)); + const PRUnichar *formatStrings[] = + { + folderName + }; + nsAutoString formatTemplate; + formatTemplate.AssignWithConversion("confirmFolderDeletionForFilter"); + bundle->FormatStringFromName(formatTemplate.get(), + formatStrings, 1, + getter_Copies(confirmString)); + if (docShell) + { + nsCOMPtr<nsIPrompt> dialog(do_GetInterface(docShell)); + if (dialog && confirmString) + dialog->Confirm(nsnull, confirmString, confirmed); + } + } + } + return rv; +} +

that routine puts up a specific confirmation dialog, with the prompt "confirmFolderDeletionForFilter". I'm suggesting writing a routine that puts up a general confirmation dialog, with the prompt property string name passsed in as a paramater. The autocompact code could use this routine, for example, just by passing in "autoCompactAllFolders" as the property string for the confirmation dialog.

cleaned as much possible, with all changes incorporated, looking for review.

- nsCOMPtr<nsISupports> parentSupports = do_QueryInterface(destFolder); + nsCOMPtr <nsISupports> parentSupports = do_QueryInterface(NS_STATIC_CAST(nsIMsgLocalMailFolder*, this)); Is the static cast required here? if (supports && parentSupports) { @@ -1991,17 +2011,21 @@ nsCOMPtr<nsIMsgFolder>folder; nsCOMPtr<nsISupports> supports; rv = aEnumerator->First(); + nsresult copyStatus = NS_OK; while (NS_SUCCEEDED(rv)) { rv = aEnumerator->CurrentItem(getter_AddRefs(supports)); folder = do_QueryInterface(supports); rv = aEnumerator->Next(); if (folder) - CopyFolderLocal(newMsgFolder,folder, PR_FALSE, msgWindow, listener); // PR_FALSE needed to avoid un-necessary deletions - + { + nsCOMPtr <nsIMsgLocalMailFolder> localFolder = do_QueryInterface(newMsgFolder); + if (localFolder) + copyStatus = localFolder->CopyFolderLocal(folder, PR_FALSE, msgWindow, listener); // PR_FALSE needed to avoid un-necessary deletions + } } looks to me like there are tabs here, messing up the indentation, perhaps from copied code. Also, in the place where you're removing the .msf file for the db, are you sure that the db has been closed? What happens if the db is open? I think you might need to force closed that db. It could be open, for example, if you have two three pane windows open, or you'd ever opened that folder.

Is the static cast required here? A cast is required to get nsISupports from nsIMsgLocalMailFolder, it works w/o static Also, in the place where you're removing the .msf file for the db, are you sure that the db has been closed? What happens if the db is open? I think you might need to force closed that db. It could be open, for example, if you have two three pane windows open, or you'd ever opened that folder. dbs are closed before we do a copy/move. srcFolder->ForceDBClosed(); fixed tabs as well.

ok, thx, r=bienvenu

two minor nits: in nsIMsgLocalMailFolder.idl, DoNextSubFolder should be doNextSubFolder +class nsIStringBundle; should be #include "nsIStringBundle.h"

Well david suggested the other way >In base/util/nsMsgFolder.h, you don't need to include "nsIStringBundle.h", you >can just forward declare "class nsIStringBundle". That might prevent a >dependency (it might not, but it doesn't hurt to try). You only have to include >the actual header file if you're going to use an nsCOMPtr as a member var of a >class in a header file. (Of course, you'll probably have to add the #include to >nsMsgFolder.cpp) I will make intercaps change.

I'm nervous about GetStringFromBundle(). Every time we call it, it calls GetBaseStringBundle() which creates a new bundle instance every time from messenger.properties. I'm not sure how expensive that is. It looks like your new GetStringFromBundle() isn't called too often, but I'd add a comment warning developers not to use it for something that happens often.

you and david right about "Class nsIStringBundle;" we do the same thing (in spirit) when we do "interface nsIStringBundle;" in an idl file.

Comment on attachment 50988 [details] [diff] [review] proposed fix,v3 sr=sspitzer after you add that comment and fix the interCaps issue.

fixed

> I'm nervous about GetStringFromBundle(). Every time we call it, it calls > GetBaseStringBundle() which creates a new bundle instance every time from > messenger.properties. > >I'm not sure how expensive that is. follow up: while working on something else, I found myself in the string bundle code. calling CreateBundle() isn't a big deal, assuming the bundle you are looking for is already in the string bundle service's cache. in this case, we are asking for the messenger.properties bundle, which should be there. (if not, we'll be loading it back it any day now for something else) if you ask the string bundle service for bundle that isn't cache, part of the process of creating a bundle involves strdup() every string in the bundle.

OK using nov2 commercial trunk build: win98, mac OS X, linux rh6.2 Alert displays at time of folder deletion (IMAP or local folders) and also provides for cancelling the alert which will neither complete the folder deletion or the disabling of the filter. Any other specific issues found with this alert/feature will be logged separately.