Note: There are a few cases of duplicates in user autocompletion which are being worked on.

DELete (like Shift+DELete) should warn when deleting from Trash (implement confirmation/pref mail.warn_on_delete_from_trash)

RESOLVED FIXED in Thunderbird 41.0

Status

Thunderbird
Folder and Message Lists
--
enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

({polish, ux-error-prevention})

Trunk
Thunderbird 41.0
polish, ux-error-prevention

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

7.23 KB, patch
JosiahOne
: ui-review+
Thomas D. (currently busy elsewhere; needinfo?me)
: ui-review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Shift-Delete warns also when deleting messages from Trash. I think that has no sense, because we don't warn on normal Delete there. Both commands just remove msgs from Trash for good.
(Assignee)

Comment 1

3 years ago
Created attachment 8564626 [details] [diff] [review]
patch

Does this make sense?
Attachment #8564626 - Flags: ui-review?(josiah)
Attachment #8564626 - Flags: review?(rkent)

Comment 2

3 years ago
I think we should go the other way and warn on deletion from trash in *all* cases. We could make that controlled by a separate pref, so that people could, say, have a warning enabled for shift-delete, but not for delete-from-trash.

For that matter, an off-by-default warning for regular deletion would be nice too (e.g. if you want to add some extra safety measures if your users are known for accidentally deleting stuff and not knowing about "undo").
(Assignee)

Comment 3

3 years ago
(In reply to Jim Porter (:squib) from comment #2)
> I think we should go the other way and warn on deletion from trash in *all*
> cases. We could make that controlled by a separate pref, so that people
> could, say, have a warning enabled for shift-delete, but not for
> delete-from-trash.
Yes, I could try to do that. We already have a warning for the Empty Trash, that can bu suppressed. Maybe we could just hook a plain Delete warning on the same pref?

> For that matter, an off-by-default warning for regular deletion would be
> nice too (e.g. if you want to add some extra safety measures if your users
> are known for accidentally deleting stuff and not knowing about "undo").
I think there already is a bug for this.

Comment 4

3 years ago
(In reply to :aceman from comment #3)
> Yes, I could try to do that. We already have a warning for the Empty Trash,
> that can bu suppressed. Maybe we could just hook a plain Delete warning on
> the same pref?

Sounds reasonable.
(Assignee)

Comment 5

3 years ago
Created attachment 8564768 [details] [diff] [review]
WIP patch v2

So this would be it. Open issues:
1. the wording of the message
2. whether to reuse the existing pref that shows a warning when Empty Trash is used from the menu. Maybe some users do not want the warning for deleting individual messages from trash, but still want to keep one for removing all trash (e.g. by misclick). So the prefs should be decoupled. It is trivial to do.
Attachment #8564626 - Attachment is obsolete: true
Attachment #8564626 - Flags: ui-review?(josiah)
Attachment #8564626 - Flags: review?(rkent)
Attachment #8564768 - Flags: ui-review?(josiah)
Attachment #8564768 - Flags: review?(squibblyflabbetydoo)
(Assignee)

Comment 6

3 years ago
(In reply to Jim Porter (:squib) from comment #2)
> For that matter, an off-by-default warning for regular deletion would be
> nice too (e.g. if you want to add some extra safety measures if your users
> are known for accidentally deleting stuff and not knowing about "undo").
https://bugzilla.mozilla.org/show_bug.cgi?id=895140#c9

I could surely implement it similarly to the patch here, if someone supported me :)

Comment 7

3 years ago
Comment on attachment 8564768 [details] [diff] [review]
WIP patch v2

Review of attachment 8564768 [details] [diff] [review]:
-----------------------------------------------------------------

This is probably ok, but I don't exactly *like* it. I think I'd rather we migrate away from the old pref and make a new one that works like the others. I'm not entirely sure we need to actually copy over the user's value either; maybe it's ok to just reset this pref to the default, since the behavior is changing a bit anyway...

::: mailnews/base/src/nsMsgDBView.cpp
@@ +3065,5 @@
>  
>    const char *warnCollapsedPref = "mail.warn_on_collapsed_thread_operation";
>    const char *warnShiftDelPref = "mail.warn_on_shift_delete";
>    const char *warnNewsPref = "news.warn_on_delete";
> +  const char *warnTrashDelPref = "mailnews.emptyTrash.dontAskAgain";

I almost feel like it would make more sense to migrate this pref so that it matches the others.
Attachment #8564768 - Flags: review?(squibblyflabbetydoo) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8565072 [details] [diff] [review]
patch v3

If you want to rename the mailnews.emptyTrash.dontAskAgain pref, please file a new bug as it is also shared with Seamonkey and also needs the have the same format as mailnews.emptyJunk.dontAskAgain. So it would need code changes at multiple places. I'll keep it as is for now and just make a new pref for the warning from deleting from trash so it can support the scenario I described (user wants warning for emptying trash but not for individual messages).
Attachment #8564768 - Attachment is obsolete: true
Attachment #8564768 - Flags: ui-review?(josiah)
Attachment #8565072 - Flags: ui-review?(josiah)
Attachment #8565072 - Flags: review?(squibblyflabbetydoo)

Comment 9

2 years ago
Comment on attachment 8565072 [details] [diff] [review]
patch v3

Review of attachment 8565072 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this looks good overall. See comments below.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +651,5 @@
>  
>  confirmMsgDelete.title=Confirm Deletion
>  confirmMsgDelete.collapsed.desc=This will delete messages in collapsed threads. Are you sure you want to continue?
>  confirmMsgDelete.deleteNoTrash.desc=This will delete messages immediately, without saving a copy to Trash. Are you sure you want to continue?
> +confirmMsgDelete.deleteFromTrash.desc=This will remove messages from Trash so they will be deleted permanently. Are you sure you want to continue?

We probably want to tweak this wording, but I don't have any ideas off-hand. Maybe Josiah or Blake have ideas?

::: mailnews/base/src/nsMsgDBView.cpp
@@ +3072,5 @@
>    nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  bool trashFolder = false;
> +  m_folder->GetFlag(nsMsgFolderFlags::Trash, &trashFolder);

You should probably check the return value from this to ensure that it succeeded, unless you can think of some case where this would fail but we should keep processing.

@@ +3078,5 @@
> +  if (trashFolder)
> +  {
> +    bool pref = false;
> +    prefBranch->GetBoolPref(warnTrashDelPref, &pref);
> +    if (pref) {

Nit: As much as I prefer this brace style, we should be consistent and use Allman braces:

if (pref)
{
  ...
}

@@ +3089,4 @@
>    {
>      bool pref = false;
>      prefBranch->GetBoolPref(warnCollapsedPref, &pref);
> +    if (pref) {

Ditto here re: braces.

@@ +3099,4 @@
>    {
>      bool pref = false;
>      prefBranch->GetBoolPref(warnShiftDelPref, &pref);
> +    if (pref) {

Braces

@@ +3108,5 @@
>    if (!activePref && mIsNews)
>    {
>      bool pref = false;
>      prefBranch->GetBoolPref(warnNewsPref, &pref);
> +    if (pref) {

Braces
Attachment #8565072 - Flags: review?(squibblyflabbetydoo) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8570714 [details] [diff] [review]
patch v3.1

Yes, thanks.
Attachment #8565072 - Attachment is obsolete: true
Attachment #8565072 - Flags: ui-review?(josiah)
Attachment #8570714 - Flags: ui-review?(josiah)
Attachment #8570714 - Flags: ui-review?(bwinton)
Comment on attachment 8570714 [details] [diff] [review]
patch v3.1

Review of attachment 8570714 [details] [diff] [review]:
-----------------------------------------------------------------

I do think we need a separate pref, which this does, so that component is fine. However, I'm not feeling the wording yet. After we figure that out ui-r+, in the meantime, ui-r-.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +676,5 @@
>  
>  confirmMsgDelete.title=Confirm Deletion
>  confirmMsgDelete.collapsed.desc=This will delete messages in collapsed threads. Are you sure you want to continue?
>  confirmMsgDelete.deleteNoTrash.desc=This will delete messages immediately, without saving a copy to Trash. Are you sure you want to continue?
> +confirmMsgDelete.deleteFromTrash.desc=This will remove messages from Trash so they will be deleted permanently. Are you sure you want to continue?

"This will permanently remove messages from Trash; They can not be recovered. Are you sure you want to continue?" Perhaps? Thoughts?

::: suite/locales/en-US/chrome/mailnews/messenger.properties
@@ +443,5 @@
>  
>  confirmMsgDelete.title=Confirm Deletion
>  confirmMsgDelete.collapsed.desc=This will delete messages in collapsed threads. Are you sure you want to continue?
>  confirmMsgDelete.deleteNoTrash.desc=This will delete messages immediately, without saving a copy to Trash. Are you sure you want to continue?
> +confirmMsgDelete.deleteFromTrash.desc=This will remove messages from Trash so they will be deleted permanently. Are you sure you want to continue?

Ditto.
Attachment #8570714 - Flags: ui-review?(josiah) → ui-review-
Comment on attachment 8570714 [details] [diff] [review]
patch v3.1

(I think Josiah took this one.  :)
Attachment #8570714 - Flags: ui-review?(bwinton)
(Assignee)

Comment 13

2 years ago
Yes, Josiah is on it, but needs some help with the wording (see comment 11).
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(bwinton)
I think I would go with "This will permanently delete messages from Trash.  Are you sure you want to continue?".
I would also not worry about it _too_ much, since people don't read these types of messages.  ;)
Flags: needinfo?(bwinton)

Updated

2 years ago
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 15

2 years ago
Created attachment 8585170 [details] [diff] [review]
patch v3.2

OK, thanks.
Attachment #8570714 - Attachment is obsolete: true
Attachment #8585170 - Flags: ui-review?(josiah)
Comment on attachment 8585170 [details] [diff] [review]
patch v3.2

Review of attachment 8585170 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! Both wording and functionality.
Attachment #8585170 - Flags: ui-review+
Adjusting summary / description to match what we've done here

STR

1 select a Trash folder
2 select some or all messages
3 delete selected messages using DEL; then using SHIFT+DEL

Actual result (before this bug)

no warning when using DEL; warning when using SHIFT+DEL

Expected result

add default warning for DEL (with "Don't ask me again" -> mail.warn_on_delete_from_trash)
so that we warn consistently by default for both DEL and SHIFT+DEL
Summary: Shift-Delete warns also when deleting from Trash → Delete and Shift-Delete should warn when deleting from Trash (implement pref mail.warn_on_delete_from_trash)
Keywords: ux-error-prevention
Summary: Delete and Shift-Delete should warn when deleting from Trash (implement pref mail.warn_on_delete_from_trash) → DELete (like Shift-DELete) should warn when deleting from Trash (implement pref mail.warn_on_delete_from_trash)
Duplicate of this bug: 661513
Duplicate of this bug: 660496
See Also: → bug 308690
Summary: DELete (like Shift-DELete) should warn when deleting from Trash (implement pref mail.warn_on_delete_from_trash) → DELete (like Shift+DELete) should warn when deleting from Trash (implement confirmation/pref mail.warn_on_delete_from_trash)
Comment on attachment 8585170 [details] [diff] [review]
patch v3.2

Review of attachment 8585170 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, seems fine to me. Sorry for the delay.
Attachment #8585170 - Flags: ui-review?(josiah) → ui-review+
(Assignee)

Comment 21

2 years ago
Thanks.
Keywords: checkin-needed

Comment 22

2 years ago
https://hg.mozilla.org/comm-central/rev/03e4d58e1d5c

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0
You need to log in before you can comment on or make changes to this bug.