Closed
Bug 65775
Opened 24 years ago
Closed 16 years ago
unable to undo Mark All Read
Categories
(MailNews Core :: Backend, defect, P2)
MailNews Core
Backend
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)
25.06 KB,
patch
|
Details | Diff | Splinter Review |
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•24 years ago
|
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
as far as I know..
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•24 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•24 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•24 years ago
|
||
In 4.7 "Mark All Read" on: - Win: Shift+C - Linux: Alt+Shift+C - Mac: Option+Command+/
Reporter | ||
Comment 9•24 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*
Comment 10•24 years ago
|
||
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•24 years ago
|
||
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
Comment 12•24 years ago
|
||
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
Comment 13•24 years ago
|
||
yes, future / helpwanted is correct, thanks.
Comment 14•20 years ago
|
||
*** Bug 249496 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 15•20 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•18 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•18 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•18 years ago
|
Assignee: sspitzer → nobody
Severity: enhancement → minor
Status: ASSIGNED → NEW
QA Contact: laurel → backend
Comment 18•17 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?
Comment 20•16 years ago
|
||
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•16 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.
Updated•16 years ago
|
Product: Core → MailNews Core
Comment 22•16 years ago
|
||
leaving as wanted, but will need someone to pick this up. No milestone.
Priority: -- → P2
Comment 23•16 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•16 years ago
|
||
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•16 years ago
|
Attachment #347251 -
Attachment description: Patch V1 → WIP Patch V1
Updated•16 years ago
|
Keywords: helpwanted
Assignee | ||
Comment 25•16 years ago
|
||
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•16 years ago
|
Attachment #347330 -
Flags: review? → review?(bienvenu)
Comment 26•16 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•16 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•16 years ago
|
||
updated patch - should address all comments. Asking David for round 2...
Attachment #347330 -
Attachment is obsolete: true
Attachment #348928 -
Flags: review?(bienvenu)
Comment 29•16 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•16 years ago
|
||
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•16 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•16 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•16 years ago
|
||
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 34•16 years ago
|
||
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•16 years ago
|
||
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 & 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•16 years ago
|
||
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 37•16 years ago
|
||
Comment on attachment 349990 [details] [diff] [review] Patch V3 >+ mMarkedMessages = *aMsgKeyArray; Better still, use SwapArrayElements
Comment 38•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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•16 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
Comment 42•16 years ago
|
||
(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•16 years ago
|
Attachment #351728 -
Flags: superreview?(neil) → superreview+
Comment 43•16 years ago
|
||
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•16 years ago
|
||
Here is an updated patch - it fixes the issues pointed out.
Attachment #351728 -
Attachment is obsolete: true
Assignee | ||
Comment 45•16 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!
Updated•16 years ago
|
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b2
Comment 46•15 years ago
|
||
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?
Comment 47•15 years ago
|
||
p.s. awesome
Updated•4 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•