Closed Bug 621921 Opened 9 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: MailNews: General, defect, major)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1final

People

(Reporter: dsb, Assigned: InvisibleSmiley)

Details

Attachments

(2 files, 3 obsolete files)

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.
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.
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Summary: Delete Folder does not confirm if saved-search folder → Delete Folder does not prompt for confirmation if folder is virtual (saved search)
Version: unspecified → Trunk
Bug 335034 and bug 197439 might fix this, but I think we need a solution for this problem here now and must not wait.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #527087 - Flags: superreview?(neil)
Attachment #527087 - Flags: review?(mnyromyr)
(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.
(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 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...]
Attachment #527087 - Flags: superreview?(neil) → superreview+
Attached patch patch v1a (obsolete) — Splinter Review
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.
Attachment #527087 - Attachment is obsolete: true
Attachment #527355 - Flags: superreview+
Attachment #527355 - Flags: review?(mnyromyr)
Attachment #527087 - Flags: review?(mnyromyr)
(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 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.
Attachment #527355 - Flags: review?(mnyromyr) → review-
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #527355 - Attachment is obsolete: true
Attachment #527755 - Flags: superreview+
Attachment #527755 - Flags: review?(mnyromyr)
This is pretty much the old version since no l10n changes are allowed.
Attachment #527756 - Flags: superreview?(neil)
Attachment #527756 - Flags: review?(mnyromyr)
Actually, since Services.prompt is used in the other if clause, too, I moved it outside.
Attachment #527755 - Attachment is obsolete: true
Attachment #527757 - Flags: superreview?(neil)
Attachment #527757 - Flags: review?(mnyromyr)
Attachment #527755 - Flags: review?(mnyromyr)
Attachment #527756 - Flags: superreview?(neil) → superreview+
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.
Attachment #527757 - Flags: superreview?(neil) → superreview+
(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 on attachment 527757 [details] [diff] [review]
patch v2a [Checkin: comment 18]

r=me with Neil's last comment fixed.
Attachment #527757 - Flags: review?(mnyromyr) → review+
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. ;-)
Attachment #527756 - Flags: review?(mnyromyr) → review+
Comment on attachment 527756 [details] [diff] [review]
branch patch [Checkin: comment 17]

http://hg.mozilla.org/releases/comm-2.0/rev/9e265cc93e6a
Attachment #527756 - Attachment description: branch patch → branch patch [Checkin: comment 17]
Comment on attachment 527757 [details] [diff] [review]
patch v2a [Checkin: comment 18]

http://hg.mozilla.org/comm-central/rev/27b95c09e1ba
Attachment #527757 - Attachment description: patch v2a → patch v2a [Checkin: comment 18]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final
(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.
You need to log in before you can comment on or make changes to this bug.