unable to undo Mark All Read

VERIFIED FIXED in Thunderbird 3.0b2

Status

MailNews Core
Backend
P2
major
VERIFIED FIXED
17 years ago
9 years ago

People

(Reporter: sairuh (rarely reading bugmail), Assigned: Nick Kreeger)

Tracking

({dataloss})

Trunk
Thunderbird 3.0b2
dataloss
Bug Flags:
blocking-thunderbird3 -
wanted-thunderbird3 +
in-testsuite ?
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

17 years ago
thanks to msanz for pointing this out. at the present there's no way to undo a
Mark All Read action. it'd be useful [if it's possible] to implement a undo
feature, since [for example] it can be all too easy to accidentally hit the A
key. :)

to repro:
0. have an inbox full of both read and unread email.
1. either select the menu item Message > Mark > All Read, or hit the A key.
result: all unread messages are now marked as read [expected].
2. look for a way to undo step [1]. result: you cannot [afaik].
(Reporter)

Updated

17 years ago
QA Contact: esther → nbaca

Comment 1

17 years ago
6.0 only covered rudamentary Undo covering Delete, Move or Copy messages.  Never
did implement undo for marking, I don't believe there are any plans to do so in
the next release.
QA Contact: nbaca → fenella

Comment 2

17 years ago
What is afaik?
as far as I know..

Comment 4

17 years ago
Agree with Laurel.

Comment 5

17 years ago
If the undo feature is not implemented, can we at least change the shortcut?
It's really easy to press "a" while you're reading messages, that's all it takes
to mess the read/unread status... A key combination would be much better.
(Reporter)

Comment 6

17 years ago
<digression type="mild">i spoke to lisa and nbaca about unmodified shortcuts
such as A [and others in mailnews], and they mentioned that they are rather
popular... lake, would you be able to shed more light as to whether or not we
should change this? imho, if these are popular, we should change/remove 'em
[they're used in 4.x]...</digression>

Comment 7

17 years ago
4.x did have single-key commands, but it did not have `Mark All as Read' as one 
of these -- for the very good reason that it wasn't undoable. If you did 
something as minor as typing an `A' into an HTML form field in a message, but 
forgetting to click in the field first, you would suddenly lose all your 
read/unread info for that folder.

There's another bug somewhere about single-key commands in general, which 
quotes a Mac reviewer of Netscape 6 who thought this particular shortcut (A for 
Mark All as Read) was absurd.

Comment 8

17 years ago
In 4.7 "Mark All Read" on:
- Win: Shift+C
- Linux: Alt+Shift+C
- Mac: Option+Command+/
(Reporter)

Comment 9

17 years ago
nbaca, thx for the 4.x equivalents! i spaced on my last comment above and
neglected to look at the specific Mark All Read shortcuts. *sigh*
yes, having mark all read be "a" is very bad, for all the reasons cited above.

I'll create a new bug on that.  (is that a recent regression?  I didn't add that
on purpose.  I wonder if adding Mark to the toolbar exposed this bug?)
accepting.  fixing #66495 makes this less important.

it would be nice to do someday, but don't expect it any time soon.
Status: NEW → ASSIGNED
Depends on: 66495
Seth, your words "nice to do someday, but don't expect it any time soon"
strongly imply Future target-milestone with helpwanted keyword -- right?  I hope
you don't mind my messing with your t-m of --- here.  The keywords field is fair
game in any event.

/be
Keywords: helpwanted
Target Milestone: --- → Future
yes, future / helpwanted is correct, thanks.

Updated

16 years ago
QA Contact: fenella → laurel

Comment 14

13 years ago
*** Bug 249496 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey

Comment 15

13 years ago
Sine the TBird bug 249496 is duped against this one, this should probably be
"Product: Core"
Component: MailNews: Main Mail Window → MailNews: Backend
Product: Mozilla Application Suite → Core

Comment 16

12 years ago
An undo feature for Mark All Read is still needed, because once you get used to do it in your spam folder it gets too easy to do it elsewhere as well by mistake.

I once did that and got very frustrated. :(

Comment 17

11 years ago
I'm over here because I asked for a guard against mark all read, and someone pointed to this bug report.  I don't use a typein shortcut, but I have several times slipped when using the right click menu to mark an individual message as not read and accidentally invoked mark all as read, wiping out my to do list, which is the messages marked unread.  Alternatively to a guard, I would be quite happy to have mark all as read go away.

I didn't know there was an undo feature for anything...

Updated

11 years ago
Assignee: sspitzer → nobody
Severity: enhancement → minor
Status: ASSIGNED → NEW
QA Contact: laurel → backend

Comment 18

11 years ago
I agree, it would be good to have a question box before everything is marked as read.  I have just been bitten by this, too (using Thunderbird 2.0b1 FWIW).
Hoping that maybe the bold world of TB3 will have room for this tasty "omfg what messages did I still need to look at?!?" morsel.
Severity: minor → major
Keywords: dataloss
Flags: blocking-thunderbird3?
I don't think this blocks tb3, but I certainly think it'd be a fine bug to have fixed in tb3.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3-

Comment 21

9 years ago
I tried to report the same issue in https://bugzilla.mozilla.org/show_bug.cgi?id=434042
but this is the original and I vote for it to be fixed. Meantime bug 434042 now requests addition of a separator between "Compact" and "Mark All Read" in the pick list to reduce frequency of accidental "Mark All Read" when using the mouse.
Product: Core → MailNews Core

Comment 22

9 years ago
leaving as wanted, but will need someone to pick this up. No milestone.
Priority: -- → P2

Comment 23

9 years ago
marking rc1, which is where we put wanted bugs that don't go into an earlier milestone.
Target Milestone: Future → Thunderbird 3.0rc1
(Assignee)

Comment 24

9 years ago
Created attachment 347251 [details] [diff] [review]
WIP Patch V1

I got bit by this the other day - Here is an initial WIP patch. From what I whipped up tonight, it's almost review ready - just needs:

1.) local, pop3 integration
2.) update u.i. hooks (i.e. "Undo Mark All As Read")
Assignee: nobody → nick.kreeger
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Attachment #347251 - Attachment description: Patch V1 → WIP Patch V1

Updated

9 years ago
Keywords: helpwanted
(Assignee)

Comment 25

9 years ago
Created attachment 347330 [details] [diff] [review]
Patch V1

Here is an initial patch. It simply adds transaction hooks to the |MarkAllMessagesRead()| folder methods. An alternative to looking up the |nsIMsgWindow| in those functions is to either add or modify the existing |MarkAllMessagesRead()| function on the idl.
Attachment #347251 - Attachment is obsolete: true
Attachment #347330 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #347330 - Flags: review? → review?(bienvenu)

Comment 26

9 years ago
thx for doing this, Nick, this all looks pretty reasonable, modulo the existing weirdness in the way undo works...

+    nsMsgReadStateTxn *undoMarkTxn = new nsMsgReadStateTxn();
+    if (!undoMarkTxn) {
+      delete undoMarkTxn;
+      return NS_ERROR_OUT_OF_MEMORY;
+    }

no need to delete null here :-)

There are a few places where you're not following the prevailing braces style.

I think SeaMonkey might want to add these strings. I guess right now the user would just see "Undo/Redo" in the SM menu, for undo of mark all read?

I think passing in an nsIMsgWindow to MarkAllRead would be fine.

Comment 27

9 years ago
Comment on attachment 347330 [details] [diff] [review]
Patch V1

minusing based on comments, but it would be great to get a new patch and get it in post b1.
Attachment #347330 - Flags: review?(bienvenu) → review-
(Assignee)

Comment 28

9 years ago
Created attachment 348928 [details] [diff] [review]
Patch V2

updated patch - should address all comments. Asking David for round 2...
Attachment #347330 - Attachment is obsolete: true
Attachment #348928 - Flags: review?(bienvenu)

Comment 29

9 years ago
Comment on attachment 348928 [details] [diff] [review]
Patch V2

oops, the file names look funny: /nsMsgBaseUndoTxn.cpp,h should be nsMsgReadStateTxn.cpp,h
(Assignee)

Comment 30

9 years ago
Created attachment 349990 [details] [diff] [review]
Patch V3

Here is an updated patch that has the revised file names.
Attachment #348928 - Attachment is obsolete: true
Attachment #349990 - Flags: review?(bienvenu)
Attachment #348928 - Flags: review?(bienvenu)

Comment 31

9 years ago
Comment on attachment 349990 [details] [diff] [review]
Patch V3

ugh, this patch doesn't apply cleanly to the trunk. I can probably work around it, but you will need a new patch at some point.

Comment 32

9 years ago
Comment on attachment 349990 [details] [diff] [review]
Patch V3

thx, Nick. This seems to work. It would be nice if it were easier to extend our undo system :-(

I have not tried SM. Asking Neil for SR...
Attachment #349990 - Flags: superreview?(neil)
Attachment #349990 - Flags: review?(bienvenu)
Attachment #349990 - Flags: review+

Comment 33

9 years ago
Created attachment 351431 [details] [diff] [review]
patch that applies to trunk...

since I've already done the merge by hand, nick asked if I could just attach that, which I have (I had to get rid of some other changes in my tree - I hope I didn't mess anything up...)
Attachment #349990 - Attachment is obsolete: true
Attachment #351431 - Flags: superreview?(neil)
Attachment #349990 - Flags: superreview?(neil)
Comment on attachment 349990 [details] [diff] [review]
Patch V3

>diff --git a/mailnews/base/util/nsMsgReadStateTxn.cpp b/mailnews/base/util/nsMsgReadStateTxn.cpp
>new file mode 100644
This was missing from the last patch so I'll comment here instead.

>+NS_IMETHODIMP
>+nsMsgReadStateTxn::Init(nsIMsgFolder *aParentFolder, 
>+                        nsTArray<nsMsgKey> *aMsgKeyArray)
Doesn't need to be virtual, since you know what type it is. In fact, you could probably do this in the constructor, and not define your own Init, although you would want to change the NS_ENSURE_ARG_POINTER to NS_ASSERTION. Also, I have a slight preference for references rather than pointers to nsTArray.

>+    if (NS_FAILED(rv) || !curMsgHdr) {
>+      continue;
>+    }
>+
>+    messageArray->AppendElement(curMsgHdr, PR_FALSE);
Personally I'd prefer if (NS_SUCCEEDED(rv) && curMsgHdr)

>+  if (aIID.Equals(NS_GET_IID(nsMsgReadStateTxn))) {
>+    *aInstancePtr = static_cast<nsMsgReadStateTxn *>(this);
>+  }
Do you actually need this? I don't see this being used anywhere.
Comment on attachment 349990 [details] [diff] [review]
Patch V3

>diff --git a/suite/locales/en-US/chrome/mailnews/messenger.dtd b/suite/locales/en-US/chrome/mailnews/messenger.dtd
>--- a/suite/locales/en-US/chrome/mailnews/messenger.dtd
>+++ b/suite/locales/en-US/chrome/mailnews/messenger.dtd
>@@ -133,16 +133,18 @@
> <!ENTITY accountManagerCmd.label "Mail &amp; Newsgroups Account Settings…">
> <!ENTITY accountManagerCmd.accesskey "M">
> <!ENTITY undoDeleteMsgCmd.label "Undo Delete Message">
> <!ENTITY redoDeleteMsgCmd.label "Redo Delete Message">
> <!ENTITY undoMoveMsgCmd.label "Undo Move Message">
> <!ENTITY redoMoveMsgCmd.label "Redo Move Message">
> <!ENTITY undoCopyMsgCmd.label "Undo Copy Message">
> <!ENTITY redoCopyMsgCmd.label "Redo Copy Message">
>+<!ENTITY undoMarkAllCmd.label "Undo Mark All As Read">
>+<!ENTITY redoMarkAllCmd.label "Redo Mark All As Read">
Also missing from the latest patch... the label on the menuitem is "All Read" so please miss off the "As" from both of these please.
Comment on attachment 351431 [details] [diff] [review]
patch that applies to trunk...

>         case 1:
>             goSetMenuValue(command, 'valueDeleteMsg');
>             break;
>         case 2:
>             goSetMenuValue(command, 'valueMoveMsg');
>             break;
>         case 3:
>             goSetMenuValue(command, 'valueCopyMsg');
>+            break;
>+        case 4:
>+            goSetMenuValue(command, 'valueUnmarkAllMsgs');
>             break;
I guess these case labels should really use the nsIMessenger constants ;-)

>-         valueDefault="&undoDefaultCmd.label;"/>
>+         valueDefault="&undoDefaultCmd.label;"
>+         valueUnmarkAllMsgs="&undoMarkAllCmd.label;"/>
Please leave valueDefault last and put the new attribute before it (2×).

sr=me with all my nits fixed.
Attachment #351431 - Flags: superreview?(neil) → superreview+
Comment on attachment 349990 [details] [diff] [review]
Patch V3

>+  mMarkedMessages = *aMsgKeyArray; 
Better still, use SwapArrayElements
Comment on attachment 351431 [details] [diff] [review]
patch that applies to trunk...

>+      nsMsgReadStateTxn *undoMarkTxn = new nsMsgReadStateTxn();
This could leak if not made an nsRefPtr instead.
(Assignee)

Comment 39

9 years ago
Created attachment 351476 [details] [diff] [review]
Patch V3 (merged)

Here is a merged version of my old patch - posting because it sounds like some stuff was missing...

I'll spin up a new patch (maybe tonight) that matches Neil's sr nits...

Thanks for the reviews gentlemen.
Attachment #351431 - Attachment is obsolete: true
(Assignee)

Comment 40

9 years ago
Created attachment 351728 [details] [diff] [review]
Final Patch

Here is an updated patch - it addresses just about every comment - I'll address the ones that were not addressed...
Attachment #351476 - Attachment is obsolete: true
Attachment #351728 - Flags: superreview?(neil)
(Assignee)

Comment 41

9 years ago
> >+NS_IMETHODIMP
> >+nsMsgReadStateTxn::Init(nsIMsgFolder *aParentFolder, 
> >+                        nsTArray<nsMsgKey> *aMsgKeyArray)
> Doesn't need to be virtual, since you know what type it is. In fact, you could
> probably do this in the constructor, and not define your own Init, although you

I'm matching the style of |nsMsgTxn| and it's subclasses - they all use |Init()|.


> >+  if (aIID.Equals(NS_GET_IID(nsMsgReadStateTxn))) {
> >+    *aInstancePtr = static_cast<nsMsgReadStateTxn *>(this);
> >+  }
> Do you actually need this? I don't see this being used anywhere.

Not sure what you mean here - |aInstancePtr| is a in-param. The QI is needed from the quirky way that |nsMsgTxn| is implemented. See |nsLocalUndoTxn|:

http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalUndoTxn.cpp#64
(In reply to comment #41)
> I'm matching the style of |nsMsgTxn| and it's subclasses
OK, so that that explains a lot ;-)

> > >+  if (aIID.Equals(NS_GET_IID(nsMsgReadStateTxn))) {
> > >+    *aInstancePtr = static_cast<nsMsgReadStateTxn *>(this);
> > >+  }
> > Do you actually need this? I don't see this being used anywhere.
> Not sure what you mean here - |aInstancePtr| is a in-param. The QI is needed
> from the quirky way that |nsMsgTxn| is implemented. See |nsLocalUndoTxn|:
Sure, but that's because nsLocalMailFolder.cpp is really badly written.

Updated

9 years ago
Attachment #351728 - Flags: superreview?(neil) → superreview+
Comment on attachment 351728 [details] [diff] [review]
Final Patch

>-[scriptable, uuid(1B44C6EB-2C14-470F-AA5C-BDDCE89A4ECC)]
>+[scriptable, uuid(21A3F88C-9593-4C0C-8372-F23C9D918AFA)]
> interface nsIMessenger : nsISupports {
> 
>     const long eUnknown = 0;
>     const long eDeleteMsg = 1;
>     const long eMoveMsg = 2;
>     const long eCopyMsg = 3;
>+    const long eMarkAllMsg = 4;
Sorry, only just noticed this, but constant additions don't require new uuids.

>-         valueDefault="&undoDefaultCmd.label;"/>
>+         valueDefault="&undoDefaultCmd.label;"
>+         valueUnmarkAllMsgs="&undoMarkAllCmd.label;"/>
>   <command id="cmd_redo"
>          valueDeleteMsg="&redoDeleteMsgCmd.label;"
>          valueMoveMsg="&redoMoveMsgCmd.label;"
>          valueCopyMsg="&redoCopyMsgCmd.label;"
>-         valueDefault="&redoDefaultCmd.label;"/>
>+         valueDefault="&redoDefaultCmd.label;"
>+         valueUnmarkAllMsgs="&redoMarkAllCmd.label;"/>
I'm sure I asked you to keep valueDefault last when adding your attribute!
sr=me with this fixed.

>+      nsRefPtr<nsMsgReadStateTxn> undoMarkTxn = new nsMsgReadStateTxn();
This is a confusing variable name, because you're not actually undoing yet.

>+NS_IMETHODIMP
>+nsMsgReadStateTxn::QueryInterface(REFNSIID aIID, void** aInstancePtr)
You don't need this. See bug 468364 for example ;-)

>+  NS_DECL_ISUPPORTS_INHERITED;
You don't need this and it doesn't take a ; anyway.
(Assignee)

Comment 44

9 years ago
Created attachment 352022 [details] [diff] [review]
Final Patch (v2)

Here is an updated patch - it fixes the issues pointed out.
Attachment #351728 - Attachment is obsolete: true
(Assignee)

Comment 45

9 years ago
changeset:   1383:455fc2b7347b
tag:         tip
user:        Nick Kreeger <nick.kreeger@park.edu>
date:        Mon Dec 08 18:09:53 2008 -0800
summary:     Fixing bug 65775 - unable to "Mark All Read". r=bienvenu/sr=neil.

Pushed to Comm-Central. Thanks all!
URL: gg
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b2
URL: gg
verified. now just have to remember that it exists!
tested with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090105 Shredder/3.0b2pre

need test(s)
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
p.s. awesome
You need to log in before you can comment on or make changes to this bug.