1497795 - "Mark all messages of an account read" feature needs confirmation, or undo option to reverse dataloss

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #317301 +++

The "Mark all messages of an account read" feature is kind of dangerous. Press it accidentally -> unhappy user :(

I think read state can be considered data, and losing a lot of it is bad. This feature needs a confirmation dialog.

Comment 1

[Wayne Mery (:wsmwk)]

Undo may have horrible performance implications, so I suspect that won't be a viable option.

I'm suprised Richard gave UI approval in Bug #317301 on such a dangerous, horrible undoable action - IIRC we have required such confirmations for other potentially dangerous functions - and left confirmation to a follow up. Good thing the action is only available on the account name. I rarely click on account name, so I hope most users are the same.

Comment 2

[Magnus Melin [:mkmelin]]

Alex, maybe you can take this as as one of the things to get familiar with the code base.

Comment 3

[Alessandro Castellani [:aleca]]

(In reply to Magnus Melin [:mkmelin] from comment #2)

Alex, maybe you can take this as as one of the things to get familiar with the code base.

Aye aye captain!

Any guidelines or behaviors to follow in terms of dealing with a confirmation dialog?
Is there any styled component already built or something similar I should leverage instead of building something from scratch?

Comment 4

[Magnus Melin [:mkmelin]]

You should just use a standard dialog for the confirmation. There are variants and it's possible to change standard buttons just by setting flags.
Something like https://searchfox.org/comm-central/rev/8a64171478a67b80775e23c37992cdf1dbd99e54/mail/extensions/mailviews/content/mailViewList.js#49

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: mark-all-read.patch

Here's the patch with the confirmation message.
A couple of questions related to what I did:

In the example code you shared, a variable is created through the Services class in order to tap the translatable string:

var bundle = Services.strings.createBundle("chrome://messenger/locale/messenger.properties");

I noticed that in the mailWindowOverlay.js file, the strings are already available in the document and selectable with a:

var bundle = document.getElementById("bundle_messenger");

I opted for the query selector, but of course let me know if we should follow one approach and which one is better.

Also, I added the translatable strings in the messenger.properties files. Do I have to do anything else? How can I test and confirm those new strings are translatable?

Cheers

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9050167 [details] [diff] [review]
mark-all-read.patch

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

You don't need to do anything more to make the messages translatable. Whey they are used from a properties or dtd file, they are translatable

Please also provide a proper commit message in your patch. It should be like

Bug 1497795 - add confirmation for marking all folders as read. r?mkmelin (the ? would be = once reviewed)

::: mail/base/content/mailWindowOverlay.js
@@ +2260,5 @@
>   */
>  function MsgMarkAllFoldersRead() {
> +  const bundle = document.getElementById("bundle_messenger");
> +
> +  if (!Services.prompt.confirm(window, bundle.getString("confirmMarkAllFoldersReadTitle"), bundle.getString("confirmMarkAllFoldersReadMessage"))) {

we try to stay under 80ch width for the files

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +327,5 @@
>  confirmUnsubscribeText=Are you sure you want to unsubscribe from %S?
>  confirmUnsubscribeManyText=Are you sure you want to unsubscribe from these newsgroups?
>  restoreAllTabs=Restore All Tabs
> +
> +confirmMarkAllFoldersReadTitle=Confirm

This should be something like
Mark All Folders Read

@@ +328,5 @@
>  confirmUnsubscribeManyText=Are you sure you want to unsubscribe from these newsgroups?
>  restoreAllTabs=Restore All Tabs
> +
> +confirmMarkAllFoldersReadTitle=Confirm
> +confirmMarkAllFoldersReadMessage=You are about to mark all the Messages in all your Folders as Read.\nThis action cannot be undone.\n\nDo you wish to continue?

no reason to capitalize Messages, Folders or Read.

I'd suggest:

Are you sure you want to mark all messages in all folders on this account as read?

Comment 7

[Jorg K (CEST = GMT+2)]

Please make it =mkmelin straight way. I'm the poor guy who has to fix all the commit messages. Surely the patch won't land without review.

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: mark-all-read.patch

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9050840 [details] [diff] [review]
mark-all-read.patch

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

r=mkmelin with the below fixed

::: mail/base/content/mailWindowOverlay.js
@@ +2262,5 @@
> +  const bundle = document.getElementById("bundle_messenger");
> +
> +  if (!Services.prompt.confirm(window,
> +                              bundle.getString("confirmMarkAllFoldersReadTitle"),
> +                              bundle.getString("confirmMarkAllFoldersReadMessage"))) {

nit: wrong alignment, the bs should align under w

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +329,5 @@
>  restoreAllTabs=Restore All Tabs
> +
> +confirmMarkAllFoldersReadTitle=Mark All Folders Read
> +confirmMarkAllFoldersReadMessage=Are you sure you want to mark all messages in all folders on this account as read?
> +

I know this was my suggestion, but perhaps it's *of* this account?

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: mark-all-read.patch

Comment 11

[Alessandro Castellani [:aleca]]

  if (!Services.prompt.confirm(window,
                                bundle.getString("confirmMarkAllFoldersReadTitle"),
                                bundle.getString("confirmMarkAllFoldersReadMessage"))) {
    return;
  }

Due to the 2 spaces indentation style, the bundle doesn't align with window.
How do you want to approach this?
Do you prefer to always keep the 2 spaces indentation no matter what,
or is it OK to have 1 space for the sake of the alignment?

Comment 12

[Jorg K (CEST = GMT+2)]

We align, check some existing code. Multiples of 2 spaces only to the beginning of the line itself, so the if or return.

Comment 13

[Alessandro Castellani [:aleca]]

(In reply to Jorg K (GMT+1) from comment #12)

We align, check some existing code. Multiples of 2 spaces only to the beginning of the line itself, so the if or return.

Thanks for the info.

Comment 14

[Alessandro Castellani [:aleca]]

Attachment: mark-all-read.patch

Properly aligned attributes of the confirm() method.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9051076 [details] [diff] [review]
mark-all-read.patch

This already had r+. You can just re-apply r+ yourself if you addressed all the issues.

Sometimes the friendly sheriff (me) checks it before landing ;-)

First patch, hey?

Comment 16

[Alessandro Castellani [:aleca]]

(In reply to Jorg K (GMT+1) from comment #15)

This already had r+. You can just re-apply r+ yourself if you addressed all
the issues.

Good to know, thanks.

First patch, hey?

Yay!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a8263af5473c
add confirmation for marking all folders as read. r=mkmelin