93968 - Deleted folder alert should happen at time of deletion, not when emptying trash.

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[laurel]

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.

Comment 1

[Navin Gupta]

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.  

Comment 2

[jglick]

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?)

Comment 3

[Navin Gupta]

Attachment: proposed fix

Comment 4

[Navin Gupta]

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). 

Comment 5

[jglick]

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."

Comment 6

[Navin Gupta]

david, need review. 

Comment 7

[David :Bienvenu]

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.

Comment 8

[Navin Gupta]

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. 

Comment 9

[David :Bienvenu]

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?

Comment 10

[Navin Gupta]

>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..

Comment 11

[David :Bienvenu]

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.

Comment 12

[robinf]

>+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.

Comment 13

[Navin Gupta]

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;
+}
+

Comment 14

[David :Bienvenu]

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.

Comment 15

[Navin Gupta]

Attachment: proposed fix, v2

Comment 16

[Navin Gupta]

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

Comment 17

[David :Bienvenu]

-      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.

Comment 18

[Navin Gupta]

 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.

Comment 19

[Navin Gupta]

Attachment: proposed fix,v3

Comment 20

[David :Bienvenu]

ok, thx, r=bienvenu

Comment 21

[(not reading, please use [e-mail] instead)]

two minor nits:

in nsIMsgLocalMailFolder.idl, DoNextSubFolder should be doNextSubFolder

+class nsIStringBundle;

should be

#include "nsIStringBundle.h"

Comment 22

[Navin Gupta]

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. 

Comment 23

[(not reading, please use [e-mail] instead)]

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.

Comment 24

[(not reading, please use [e-mail] instead)]

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

Comment 25

[(not reading, please use [e-mail] instead)]

Comment on attachment 50988 [details] [diff] [review]
proposed fix,v3

sr=sspitzer after you add that comment and fix the interCaps issue.

Comment 26

[Navin Gupta]

fixed

Comment 27

[(not reading, please use [e-mail] instead)]

> 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.

Comment 28

[laurel]

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.