Closed Bug 65775 Opened 24 years ago Closed 16 years ago

unable to undo Mark All Read

Categories

(MailNews Core :: Backend, defect, P2)

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 3.0b2

People

(Reporter: bugzilla, Assigned: nick.kreeger)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss)

Attachments

(1 file, 7 obsolete files)

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].
QA Contact: esther → nbaca
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
What is afaik?
Agree with Laurel.
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.
<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>
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.
In 4.7 "Mark All Read" on:
- Win: Shift+C
- Linux: Alt+Shift+C
- Mac: Option+Command+/
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.
QA Contact: fenella → laurel
*** Bug 249496 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
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
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. :(
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...
Assignee: sspitzer → nobody
Severity: enhancement → minor
Status: ASSIGNED → NEW
QA Contact: laurel → backend
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-
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
leaving as wanted, but will need someone to pick this up. No milestone.
Priority: -- → P2
marking rc1, which is where we put wanted bugs that don't go into an earlier milestone.
Target Milestone: Future → Thunderbird 3.0rc1
Attached patch WIP Patch V1 (obsolete) — Splinter Review
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
Attachment #347251 - Attachment description: Patch V1 → WIP Patch V1
Keywords: helpwanted
Attached patch Patch V1 (obsolete) — Splinter Review
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?
Attachment #347330 - Flags: review? → review?(bienvenu)
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 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-
Attached patch Patch V2 (obsolete) — Splinter Review
updated patch - should address all comments. Asking David for round 2...
Attachment #347330 - Attachment is obsolete: true
Attachment #348928 - Flags: review?(bienvenu)
Comment on attachment 348928 [details] [diff] [review]
Patch V2

oops, the file names look funny: /nsMsgBaseUndoTxn.cpp,h should be nsMsgReadStateTxn.cpp,h
Attached patch Patch V3 (obsolete) — Splinter Review
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 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 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+
Attached patch patch that applies to trunk... (obsolete) — Splinter Review
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.
Attached patch Patch V3 (merged) (obsolete) — Splinter Review
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
Attached patch Final Patch (obsolete) — Splinter Review
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)
> >+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.
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.
Attached patch Final Patch (v2)Splinter Review
Here is an updated patch - it fixes the issues pointed out.
Attachment #351728 - Attachment is obsolete: true
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
Closed: 16 years ago
Resolution: --- → FIXED
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
Blocks: 356015
See Also: → 356015
You need to log in before you can comment on or make changes to this bug.