Last Comment Bug 65775 - unable to undo Mark All Read
: unable to undo Mark All Read
Status: VERIFIED FIXED
: dataloss
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: P2 major with 6 votes (vote)
: Thunderbird 3.0b2
Assigned To: Nick Kreeger
:
Mentors:
: 249496 (view as bug list)
Depends on: 66495
Blocks:
  Show dependency treegraph
 
Reported: 2001-01-17 14:21 PST by sairuh (rarely reading bugmail)
Modified: 2009-01-06 17:54 PST (History)
24 users (show)
davida: blocking‑thunderbird3-
davida: wanted‑thunderbird3+
vseerror: in‑testsuite?
vseerror: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP Patch V1 (11.22 KB, patch)
2008-11-10 00:15 PST, Nick Kreeger
no flags Details | Diff | Splinter Review
Patch V1 (19.01 KB, patch)
2008-11-10 10:48 PST, Nick Kreeger
mozilla: review-
Details | Diff | Splinter Review
Patch V2 (25.70 KB, patch)
2008-11-18 21:57 PST, Nick Kreeger
no flags Details | Diff | Splinter Review
Patch V3 (25.74 KB, patch)
2008-11-25 09:25 PST, Nick Kreeger
mozilla: review+
Details | Diff | Splinter Review
patch that applies to trunk... (17.62 KB, patch)
2008-12-04 13:35 PST, David :Bienvenu
neil: superreview+
Details | Diff | Splinter Review
Patch V3 (merged) (18.66 KB, patch)
2008-12-04 18:00 PST, Nick Kreeger
no flags Details | Diff | Splinter Review
Final Patch (26.27 KB, patch)
2008-12-06 18:40 PST, Nick Kreeger
neil: superreview+
Details | Diff | Splinter Review
Final Patch (v2) (25.06 KB, patch)
2008-12-08 18:03 PST, Nick Kreeger
no flags Details | Diff | Splinter Review

Description sairuh (rarely reading bugmail) 2001-01-17 14:21:09 PST
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].
Comment 1 laurel 2001-01-18 11:19:08 PST
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.
Comment 2 laurel 2001-01-18 11:21:21 PST
What is afaik?
Comment 3 stephend@netscape.com (gone - use stephen.donner@gmail.com instead) 2001-01-18 11:26:19 PST
as far as I know..
Comment 4 fenella 2001-01-18 11:32:28 PST
Agree with Laurel.
Comment 5 msanz 2001-01-20 22:06:25 PST
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.
Comment 6 sairuh (rarely reading bugmail) 2001-01-22 11:31:06 PST
<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 Matthew Paul Thomas 2001-01-22 21:33:44 PST
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 Ninoschka Baca 2001-01-23 11:28:50 PST
In 4.7 "Mark All Read" on:
- Win: Shift+C
- Linux: Alt+Shift+C
- Mac: Option+Command+/
Comment 9 sairuh (rarely reading bugmail) 2001-01-23 13:39:52 PST
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*
Comment 10 (not reading, please use seth@sspitzer.org instead) 2001-01-24 20:00:11 PST
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?)
Comment 11 (not reading, please use seth@sspitzer.org instead) 2001-01-24 20:03:01 PST
accepting.  fixing #66495 makes this less important.

it would be nice to do someday, but don't expect it any time soon.
Comment 12 Brendan Eich [:brendan] 2001-01-24 23:11:56 PST
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
Comment 13 (not reading, please use seth@sspitzer.org instead) 2001-01-24 23:48:03 PST
yes, future / helpwanted is correct, thanks.
Comment 14 Mike Cowperthwaite 2004-08-25 08:42:48 PDT
*** Bug 249496 has been marked as a duplicate of this bug. ***
Comment 15 Aleksey Nogin 2004-12-01 21:57:43 PST
Sine the TBird bug 249496 is duped against this one, this should probably be
"Product: Core"
Comment 16 Seungbeom Kim 2006-01-20 11:32:58 PST
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 turningoff 2006-10-09 15:50:28 PDT
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...
Comment 18 Felix Wiemann 2007-01-15 15:11:24 PST
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).
Comment 19 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-09 06:18:04 PDT
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.
Comment 20 David Ascher (:davida) 2008-04-29 10:29:10 PDT
I don't think this blocks tb3, but I certainly think it'd be a fine bug to have fixed in tb3.
Comment 21 bgz 2008-05-18 17:14:30 PDT
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.
Comment 22 David :Bienvenu 2008-08-21 10:24:32 PDT
leaving as wanted, but will need someone to pick this up. No milestone.
Comment 23 David :Bienvenu 2008-08-21 11:21:53 PDT
marking rc1, which is where we put wanted bugs that don't go into an earlier milestone.
Comment 24 Nick Kreeger 2008-11-10 00:15:41 PST
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")
Comment 25 Nick Kreeger 2008-11-10 10:48:07 PST
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.
Comment 26 David :Bienvenu 2008-11-17 14:38:51 PST
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 David :Bienvenu 2008-11-17 14:39:45 PST
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.
Comment 28 Nick Kreeger 2008-11-18 21:57:48 PST
Created attachment 348928 [details] [diff] [review]
Patch V2

updated patch - should address all comments. Asking David for round 2...
Comment 29 David :Bienvenu 2008-11-24 10:37:17 PST
Comment on attachment 348928 [details] [diff] [review]
Patch V2

oops, the file names look funny: /nsMsgBaseUndoTxn.cpp,h should be nsMsgReadStateTxn.cpp,h
Comment 30 Nick Kreeger 2008-11-25 09:25:10 PST
Created attachment 349990 [details] [diff] [review]
Patch V3

Here is an updated patch that has the revised file names.
Comment 31 David :Bienvenu 2008-12-04 11:49:41 PST
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 David :Bienvenu 2008-12-04 12:54:13 PST
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...
Comment 33 David :Bienvenu 2008-12-04 13:35:04 PST
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...)
Comment 34 neil@parkwaycc.co.uk 2008-12-04 14:05:09 PST
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 35 neil@parkwaycc.co.uk 2008-12-04 14:10:28 PST
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 36 neil@parkwaycc.co.uk 2008-12-04 14:11:28 PST
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.
Comment 37 neil@parkwaycc.co.uk 2008-12-04 14:14:40 PST
Comment on attachment 349990 [details] [diff] [review]
Patch V3

>+  mMarkedMessages = *aMsgKeyArray; 
Better still, use SwapArrayElements
Comment 38 neil@parkwaycc.co.uk 2008-12-04 16:08:21 PST
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.
Comment 39 Nick Kreeger 2008-12-04 18:00:28 PST
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.
Comment 40 Nick Kreeger 2008-12-06 18:40:29 PST
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...
Comment 41 Nick Kreeger 2008-12-06 18:50:27 PST
> >+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
Comment 42 neil@parkwaycc.co.uk 2008-12-07 13:06:21 PST
(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.
Comment 43 neil@parkwaycc.co.uk 2008-12-07 13:39:06 PST
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.
Comment 44 Nick Kreeger 2008-12-08 18:03:39 PST
Created attachment 352022 [details] [diff] [review]
Final Patch (v2)

Here is an updated patch - it fixes the issues pointed out.
Comment 45 Nick Kreeger 2008-12-08 18:11:02 PST
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!
Comment 46 Wayne Mery (:wsmwk, NI for questions) 2009-01-06 17:51:20 PST
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)
Comment 47 Wayne Mery (:wsmwk, NI for questions) 2009-01-06 17:54:22 PST
p.s. awesome

Note You need to log in before you can comment on or make changes to this bug.