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

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, enhancement)

Description

[:aceman]

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.

Comment 1

[:aceman]

Attachment: patch

Does this make sense?

Comment 2

[Jim Porter (:squib)]

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

Comment 3

[:aceman]

(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

[Jim Porter (:squib)]

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

Comment 5

[:aceman]

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

Comment 6

[:aceman]

(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

[Jim Porter (:squib)]

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.

Comment 8

[:aceman]

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

Comment 9

[Jim Porter (:squib)]

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

Comment 10

[:aceman]

Attachment: patch v3.1

Yes, thanks.

Comment 11

[Josiah Bruner [:jsbruner]]

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.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8570714 [details] [diff] [review]
patch v3.1

(I think Josiah took this one.  :)

Comment 13

[:aceman]

Yes, Josiah is on it, but needs some help with the wording (see comment 11).

Comment 14

[Blake Winton (: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.  ;)

Comment 15

[:aceman]

Attachment: patch v3.2

OK, thanks.

Comment 16

[Thomas D. (:thomas8)]

Comment on attachment 8585170 [details] [diff] [review]
patch v3.2

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

Nice! Both wording and functionality.

Comment 17

[Thomas D. (:thomas8)]

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

Comment 20

[Josiah Bruner [:jsbruner]]

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.

Comment 21

[:aceman]

Thanks.

Comment 22

[aleth [:aleth]]

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