621921 - Delete Folder does not prompt for confirmation if folder is virtual (saved search)

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Daniel B.]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.16) Gecko/20101123 SeaMonkey/2.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.16) Gecko/20101123 SeaMonkey/2.0.11

The Delete Folder command does not confirm the deletion with the user if
the folder is a folder that is a saved search (rather than a regular mail
folder).

Reproducible: Always

Steps to Reproduce:
1. Create a new mail folder.
2. Create a saved-search folder.  
3. Delete the folder.  

Actual Results:  
4. There is no confirmation before the folder is deleted.

Expected Results:  
4. There should be a confirmation dialog asking whether to delete
   the folder.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Confirming and raising severity since there is no Undo, i.e. the possibly complex saved search criteria are easily lost with no way back. Happened to me several times when focus miraculously switched to the folder pane.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Bug 335034 and bug 197439 might fix this, but I think we need a solution for this problem here now and must not wait.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

TB fixed this through bug 468081. From bug 468081 comment 4:
"we used to do this here:
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgFolderDataSource.cpp#2013

but now we don't go through the folder data source, so we'll need to do it in
folderPane.js, or the backend somewhere..."

The above reads like this is a regression for us, too, but I haven't actually checked. What I do know is that this bug is in both 2.0.x and 2.1pre/trunk and I really want to have this fixed before 2.1 goes gold.

Comment 4

[[e-mail]]

(In reply to comment #3)
> The above reads like this is a regression for us, too, but I haven't actually
> checked. What I do know is that this bug is in both 2.0.x and 2.1pre/trunk and
> I really want to have this fixed before 2.1 goes gold.
It's sort of a regression; I think 1.5a had the prompt.

Comment 5

[[e-mail]]

(In reply to comment #3)
> I really want to have this fixed before 2.1 goes gold.
Not with that string change you won't.

Comment 6

[[e-mail]]

Comment on attachment 527087 [details] [diff] [review]
patch

>+                var confirmation = gMessengerBundle.getString("confirmSavedSearchDeleteMessage");
>+                var title = gMessengerBundle.getString("confirmSavedSearchTitle");
Nit: should be SearchDeleteTitle to match.

>+                var IPS = Components.interfaces.nsIPromptService;
Not a fan of this. Can we use var nsIPromptService, or how about Services.prompt.STD_YES_NO_BUTTONS etc?

>+                if (promptService.confirmEx(window, title, confirmation,
Worth using Services.prompt here?

>                 if (gCurrentVirtualFolderUri == selectedFolder.URI)
>                   gCurrentVirtualFolderUri = null;
[Bah, mix of indentation styles...]

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v1a

I'll be bold and carry forward the sr+ even though I made more changes than requested (replaced one more occurrence of promptService, which allowed me to remove the variable altogether).

Karsten/Neil, would it be OK to land this on c-2.0 if I reused the "confirmation" variable for the confirmEx title (or using "confirmSavedSearchDeleteMessage" twice; maybe with an explanatory comment)? This would allow us to not break the l10n freeze but still fix this dataloss regression on the branch.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #7)
> Karsten/Neil, would it be OK to land this on c-2.0 if I reused the
> "confirmation" variable for the confirmEx title

Actually I found that one can pass "null" for the title, which will result in "Confirm" being the dialog title for an en-US build. I only found that when searching for the other occurrence of "confirmSavedSearchDeleteMessage", which is in nsMsgFolderDataSource.cpp (see line 2011 there). Sadly the IDL for nsIPromptService doesn't tell you that, implying that it's not possible. I wasn't successful trying to find where this fallback logic exists in the source either (must be some magic init through some magically called functions; MXR left me in the cold here) so that explains why no non-back-end developer would ever have guessed and corrected the IDL (or MDC for that matter). This just confirms (heh) why I keep as far away from the back-end as possible.

Anyway, long story short: For branch we can just pass "null" for the title and be done with it. Please consider that as part of the review request.

Comment 9

[Karsten Düsterloh]

Comment on attachment 527355 [details] [diff] [review]
patch v1a

> # for message views
> confirmViewDeleteTitle=Confirm
> confirmViewDeleteMessage=Are you sure you want to delete this view?

The "Delete View" dialog is not exactly a good example to follow.

> # for virtual folders
>+confirmSavedSearchDeleteTitle=Confirm Delete
> confirmSavedSearchDeleteMessage=Are you sure you want to delete this saved search?

You should mimic the normal "Delete Folder" dialog as close as possible, i.e. use "Delete Saved Search" as the title.

>+                if (Services.prompt.confirmEx(window, title, confirmation,
>+                        Services.prompt.STD_YES_NO_BUTTONS + Services.prompt.BUTTON_POS_1_DEFAULT,
>+                        "", "", "", "", {}) != 0) /* the yes button is in position 0 */

Yikes. STD_YES_NO_BUTTONS is usually not really helpful. Syncing in with the "Delete Folder" dialog would call for "Cancel" and "Delete Saved Search".
(See for example <http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#2499> ff.)

Passing null for string frozen branch would be okay by me.

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

BTW I get this error when canceling the deletion of a normal folder:

Error: An error occurred executing the cmd_delete command: [Exception... "Component returned failure code: 0x8055001a [nsIMsgFolder.deleteSubFolders]"  nsresult: "0x8055001a (<unknown>)"  location: "JS frame :: chrome://messenger/content/mail3PaneWindowCommands.js :: MsgDeleteFolder :: line 990"  data: no]
Source File: chrome://global/content/globalOverlay.js
Line: 100

I'd file a bug if anyone sees this, too.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Attachment: branch patch [Checkin: comment 17]

This is pretty much the old version since no l10n changes are allowed.

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2a [Checkin: comment 18]

Actually, since Services.prompt is used in the other if clause, too, I moved it outside.

Comment 13

[[e-mail]]

Comment on attachment 527757 [details] [diff] [review]
patch v2a [Checkin: comment 18]

>+                    return;
Bah, I just noticed that this should be continue; for consistency with the rest of the loop (in both patches). sr=me with that fixed.

Comment 14

[Karsten Düsterloh]

(In reply to comment #10)
> Error: An error occurred executing the cmd_delete command: [Exception...
> "Component returned failure code: 0x8055001a [nsIMsgFolder.deleteSubFolders]" 
> nsresult: "0x8055001a (<unknown>)"  location: "JS frame ::
> chrome://messenger/content/mail3PaneWindowCommands.js :: MsgDeleteFolder ::
> line 990"  data: no]
> Source File: chrome://global/content/globalOverlay.js
> Line: 100
> 
> I'd file a bug if anyone sees this, too.

I see this, too. Go ahead. ;-)

Comment 15

[Karsten Düsterloh]

Comment on attachment 527757 [details] [diff] [review]
patch v2a [Checkin: comment 18]

r=me with Neil's last comment fixed.

Comment 16

[Karsten Düsterloh]

Comment on attachment 527756 [details] [diff] [review]
branch patch [Checkin: comment 17]

r=me by code inspection since I don't have a branch at hand currently — I trust that you have tested your patch with it. ;-)

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 527756 [details] [diff] [review]
branch patch [Checkin: comment 17]

http://hg.mozilla.org/releases/comm-2.0/rev/9e265cc93e6a

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 527757 [details] [diff] [review]
patch v2a [Checkin: comment 18]

http://hg.mozilla.org/comm-central/rev/27b95c09e1ba

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #14)
> (In reply to comment #10)
> > (...)
> > I'd file a bug if anyone sees this, too.
> 
> I see this, too. Go ahead. ;-)

Filed bug 652913.