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

RESOLVED FIXED in Thunderbird 67.0

Status

enhancement
RESOLVED FIXED
8 months ago
3 months ago

People

(Reporter: mkmelin, Assigned: aleca)

Tracking

Trunk
Thunderbird 67.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

8 months ago
+++ 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.

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.

Summary: "Mark all messages of an account read" feature needs confirmation, or undo option → "Mark all messages of an account read" feature needs confirmation, or undo option to reverse dataloss
Reporter

Comment 2

3 months ago

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

Assignee: nobody → alessandro
Assignee

Comment 3

3 months ago

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

Assignee

Updated

3 months ago
Status: NEW → ASSIGNED
Reporter

Comment 4

3 months ago

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

Assignee

Comment 5

3 months ago
Posted patch mark-all-read.patch (obsolete) — Splinter Review

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

Attachment #9050167 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 6

3 months ago
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?
Attachment #9050167 - Flags: review?(mkmelin+mozilla)

Comment 7

3 months ago

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.

Assignee

Comment 8

3 months ago
Posted patch mark-all-read.patch (obsolete) — Splinter Review
Attachment #9050167 - Attachment is obsolete: true
Attachment #9050840 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 9

3 months ago
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?
Attachment #9050840 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 10

3 months ago
Posted patch mark-all-read.patch (obsolete) — Splinter Review
Attachment #9050840 - Attachment is obsolete: true
Attachment #9051071 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 11

3 months ago
  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

3 months ago

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

Assignee

Comment 13

3 months ago

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

Assignee

Comment 14

3 months ago

Properly aligned attributes of the confirm() method.

Attachment #9051071 - Attachment is obsolete: true
Attachment #9051071 - Flags: review?(mkmelin+mozilla)
Attachment #9051076 - Flags: review?(mkmelin+mozilla)

Comment 15

3 months ago
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?
Attachment #9051076 - Flags: review?(mkmelin+mozilla) → review+

Updated

3 months ago
Keywords: checkin-needed
Assignee

Comment 16

3 months ago

(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

3 months ago

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/a8263af5473c
add confirmation for marking all folders as read. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0
You need to log in before you can comment on or make changes to this bug.