Closed Bug 74959 Opened 24 years ago Closed 18 years ago

implement mail "back" and "forward"

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird2.0

People

(Reporter: sspitzer, Assigned: Bienvenu)

References

Details

(Keywords: verified1.8.1, verified1.8.1.3)

Attachments

(7 files, 3 obsolete files)

there is now UI, it was just accelerator driven. bienvenu was telling me how 4.x had this. sounded useful.
future
Target Milestone: --- → Future
*** Bug 77099 has been marked as a duplicate of this bug. ***
Bug 77099 has recently been marked as a duplicate of this one. However, its scope is slightly different (it requests a list of recently read messages), so, to make sure no one forgets about these requests, I'll quote the relevant bits here: Opened: 2001-04-22 10:00 by lasse@lama.no (Lasse Marøen) Description: Sometimes I have missed the possibility of navigating between messages in mailnews based on the order which they were read. It would be great to have the opportunity of going back to read messages without having to recreate sorting method and scrolling up and down. What I'd like is something like the History Sidebar, only for Mailnews. That way one could easily find a message based on search criteria such as "I'm sure I read this half an hour ago" or "someone said something about that in some newsgroup I read yesterday". ------- Additional Comments From Alex Bishop 2001-04-22 10:37 ------- So basically you want a list of say, the ten last messages you read, a little like the most recently accessed pages in Navigator's Go menu (but persistent across sessions). That's a nice idea and I don't think it would be too hard to implement it. Probably wouldn't make 1.0 though. ------- Additional Comments From Lasse Marøen 2001-04-22 12:30 ------- Ideally I would have all the navigation features of navigator in Mailnews - Back and forward buttons, a go menu, and the History Sidebar. I would settle for the latter though, I guess it's a feature that wouldn't be used enough to justify a new menu or new buttons in the mail UI.
I really miss this from 4.x - I'll take this bug and maybe find some time to do it (back and forward, not the rest of the stuff described here)
Assignee: sspitzer → bienvenu
adding self to cc list
Blocks: 161374
Back is now implemented in Thunderbird. Any plans of copying that code to Mozilla's Mail?
afaik, back isn't implemented in thunderbird; previous unread is implemented. They're not the same.
*** Bug 234841 has been marked as a duplicate of this bug. ***
*** Bug 163880 has been marked as a duplicate of this bug. ***
*** Bug 169947 has been marked as a duplicate of this bug. ***
Will this never be implemented? 3 years later, Thunderbird still does not have this.
Product: Browser → Seamonkey
There's an extension (now) named 'Mistory' that implements this for Mozilla, SM and TB: <http://www.heise.de/ix/artikel/2005/09/154/mistory-1.0.tar.gz>, but it's more of a proof of concept. (It's part of an article about XUL.)
Attached patch work in progress (obsolete) (deleted) — Splinter Review
work in progress. The back menu seems to be working - the forward menu isn't quite right yet. The other remaining thing I want to do is make back do the right thing when you read a message and then go to a different folder w/o selecting a message - back should go back to the message you were reading in the previous folder.
Attached patch more work in progress ... (deleted) — Splinter Review
this is quite a bit closer to being done - I still need to prune the global history at some point (maybe 100 messages?), and fix a problem where an empty entry gets into the dropdown menu. I also need to add code to enable/disable forward and back correctly.
Attachment #230842 - Attachment is obsolete: true
Attached patch proposed fix (deleted) — Splinter Review
this is getting to where I'd like to get it reviewed and landed...
Attachment #233310 - Flags: superreview?(mscott)
Comment on attachment 233310 [details] [diff] [review] proposed fix woot! you should be able to use the new icons which are now on the branch and trunk for forward / back. The last two icons in: http://lxr.mozilla.org/seamonkey/source/mail/themes/qute/mail/icons/mail-toolbar.png and http://lxr.mozilla.org/seamonkey/source/mail/themes/qute/mail/icons/mail-toolbar-small.png We don't have artwork for the mac yet. We should ditch the dumps statement: + dump("msg uri = " + msgUriStr.toString + "\n"); Is this messenger object the right place to put this stuff or should it be part of the mail window? Browser page history is window specific I think... can you do a quick leak test to make sure the messenger object still gets released properly now that it's a registered folder listener?
Attachment #233310 - Flags: superreview?(mscott) → superreview+
Comment on attachment 233310 [details] [diff] [review] proposed fix Earlier you have >+ var folderUri = messenger.getFolderUriAtNavigatePos(historyIndex); >+ var msgUri = messenger.getMsgUriAtNavigatePos(historyIndex); >+ var folder = RDF.GetResource(folderUri).QueryInterface(Components.interfaces.nsIMsgFolder); >+ var msgHdr = messenger.msgHdrFromURI(msgUri); But then later you have >+ var folderUri = messenger.getFolderUriAtNavigatePos(relPos); >+ var msgUri = messenger.getMsgUriAtNavigatePos(relPos); >+ dump("msg uri = " + msgUri.toString() + " folder Uri = " + folderUri.toString() + "\n"); >+ // want to get rid of "-message:" and replace it with ":" >+ var msgUriStr = new String(""); >+ msgUriStr = msgUri; >+ msgUriStr.replace("-message:", ":"); >+ dump("msg uri = " + msgUriStr.toString + "\n"); >+ var msgHdr = messenger.msgHdrFromURI(msgUriStr); Which is right?
After this patch checked in, there are duplicate entities <!ENTITY forwardMsgCmd.label "Forward">. See http://lxr.mozilla.org/mozilla/source/mail/locales/en-US/chrome/messenger/messenger.dtd#282 and http://lxr.mozilla.org/mozilla/source/mail/locales/en-US/chrome/messenger/messenger.dtd#306 That looks wrong.
Re Neil's question - I believe they both work, in which case the second one should be fixed. I'll see if that works. Re the duplicate entity, I'll look into that as well.
Attached patch fix the dtd entities. (deleted) — Splinter Review
use consistent names for the go nav menu item entities.
Attachment #234697 - Flags: superreview?(sspitzer)
Comment on attachment 234697 [details] [diff] [review] fix the dtd entities. sr=sspitzer, acting sr for mailnews (while mscott is away)
Attachment #234697 - Flags: superreview?(sspitzer) → superreview+
Attached patch fix Neil's issue (deleted) — Splinter Review
remove unneeded code as pointed out by Neil.
Attachment #234707 - Flags: superreview?(sspitzer)
fixed on trunk and branch (I've hijacked this bug from the suite - I don't know if the suite will want this or not...)
Status: NEW → RESOLVED
Closed: 18 years ago
Component: MailNews: Main Mail Window → Mail Window Front End
Keywords: fixed1.8.1
Product: Mozilla Application Suite → Thunderbird
Resolution: --- → FIXED
Target Milestone: Future → Thunderbird2.0
Comment on attachment 234707 [details] [diff] [review] fix Neil's issue sr=sspitzer, acting sr for mscott.
Attachment #234707 - Flags: superreview?(sspitzer) → superreview+
> I don't know if the suite will want this Yes, we do. Luckily, attachment 233310 [details] [diff] [review] doesn't seem to break anything if this feature isn't used...
no, it shouldn't break anything - unless you add the ui for the toolbar button or the menu item, none of the new code should be invoked.
Are these buttons supposed to work in the standalone message window? They don't appear to be.
I'll fix that - it's supposed to work...
One more thing: the "Go Back" button (and the corresponding menu item) isn't disabled, if there is no more item in history to go back.
this gets navigation working in a stand-alone msg window (for both normal folders and saved searches). I'll fix updating the enabling/disabling next.
Attachment #234936 - Flags: superreview?(mscott)
Attachment #234956 - Flags: superreview?(mscott)
Attachment #234936 - Flags: superreview?(mscott) → superreview+
Attachment #234956 - Flags: superreview?(mscott) → superreview+
all patches so far landed on trunk and 2.0 branch
Hey David, You'll want to wrap the key commands: <menuitem label="&goForwardCmd.label;" key="key_goForward" observes="cmd_goForward"/> <menuitem label="&goBackCmd.label;" key="key_goBack" observes="cmd_goBack"/> in xp_macosx ifndefs otherwise you won't be able to type [ or ] in the quick search bar on the mac :)
I tried quick search on [ and ] and it worked...ah, the mysteries of the mac. Have ou tried it?
Blocks: TB2SM
david: Using version 2 beta 1 (20070115, I don't see the functionality present in a standalone mail window (buttons are greyed out on toolbar and I can't navigate thru the Go menu). In the mail pane, it seems to work fine.
Sorry, forgot to mention I was running on Win XP, not the Mac. (In reply to comment #35) > david: Using version 2 beta 1 (20070115, I don't see the functionality present > in a standalone mail window (buttons are greyed out on toolbar and I can't > navigate thru the Go menu). In the mail pane, it seems to work fine. >
Marcia, try this: Select a folder with multiple unread messages. double click on a message to open a stand-alone msg window hit 'n' to go next unread. Does the back button enable then? It does for me...
Depends on: 368177
verified fixed 1.8.1.3 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.3) Gecko/20070326 Thunderbird/2.0.0.0 Mnenhy/0.7.5.0 ID:2007032620 (Thunderbird 2 RC1) and tested with david`s steps from comment #37
Keywords: verified1.8.1.3
Verified on 2.0.0.0 rc2 builds on Win XP and Mac OS 10.4.9
Status: RESOLVED → VERIFIED
Depends on: 390921
Attached patch SM port (obsolete) (deleted) — Splinter Review
Adaption for SM suiterunner
Attachment #282902 - Flags: superreview?(neil)
Attachment #282902 - Flags: review?(mnyromyr)
(In reply to comment #41) > Created an attachment (id=282902) [details] Application/octet-stream? Hm, don't know, why this happened.
Attachment #282902 - Attachment is patch: true
Attachment #282902 - Attachment mime type: application/octet-stream → text/plain
Blocks: 398081
Comment on attachment 282902 [details] [diff] [review] SM port I had the chance to prereview this before attaching, so just two remarks for others: - we need new icons for goback/goforward, this patch uses the respective browser button icons in Classic and those of reply/forward in Modern - we still need fixes for bug 368178 and bug 398081.
Attachment #282902 - Flags: review?(mnyromyr) → review+
(In reply to comment #43) > - we need new icons for goback/goforward, this patch uses the respective > browser button icons in Classic and those of reply/forward in Modern Odd, why reply/forward for Modern?
Comment on attachment 282902 [details] [diff] [review] SM port >+ case "button_goForward": >+ case "button_goBack": >+ case "cmd_goForward": >+ case "cmd_goBack": >+ if (gDBView) >+ enabled.value = gDBView.navigateStatus((command == "cmd_goBack" || command == "button_goBack") ? nsMsgNavigationType.back : nsMsgNavigationType.forward); >+ return enabled.value; Please split these up into Back/Forward. >+ case "button_goForward": >+ case "cmd_goForward": >+ MsgGoForward(); >+ break; >+ case "button_goBack": >+ case "cmd_goBack": >+ MsgGoBack(); >+ break; Back before Forward again (and similarly throughout) >+var gNavDebug = false; >+function NavDebug(str) >+{ >+ if (gNavDebug) >+ dump(str); >+} Drop this. >+ curPos.value = curPos.value * 2; Please don't reuse this, instead have two separate variables. >+ for (var i = curPos.value; isBackMenu ? (i >= 0) : (i < historyArray.length); i += (isBackMenu ? -2 : 2)) Even with the comment, this is too convoluted. Please put the back and forward cases in the _init functions (but you can leave the menuitem creation code below in a shared function). >+ var menuText = ""; >+ var msgHdr = messenger.msgHdrFromURI(historyArray[i]); >+ if (!IsCurrentLoadedFolder(folder)) >+ menuText = folder.prettyName + " - "; You don't use the msgHdr just yet, please declare it after this line. >+ menuText += msgHdr.subject; >+ menuText += ":"; >+ menuText += msgHdr.author; These don't need to be separate concatenations. (Should we actually use the description attribute for the author?) >+ relPos += isBackMenu ? -1 : 1; Sigh, what a useless API we have to deal with :-( >+ newMenuItem.setAttribute('oncommand', 'NavigateToUri(event.target); event.stopPropagation();'); Please define this on the menupopups in the XUL. >+ if (! (relPos % 50)) 50? That's rather a big menu... how about 15? >+function forwardToolbarMenu_init(menuPopup) >+{ >+ PopulateHistoryMenu(menuPopup, false); >+} This belongs directly after the back menu, not with the other stuff in between. >+<!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd" > >+%globalDTD; Don't bother with this, we're nowhere near RTL-aware yet. > </menu> >+ <menuitem label="&goForwardCmd.label;" key="key_goForward" observes="cmd_goForward"/> >+ <menuitem label="&goBackCmd.label;" key="key_goBack" observes="cmd_goBack"/> > <menuseparator observes="mailHideMenus"/> Indentation seems to be wrong here. >+ <menuitem label="&goBackCmd.label;" >+ observes="cmd_goBack"/> >+ <menuitem label="&goForwardCmd.label;" >+ observes="cmd_goForward"/> These get deleted immediately. Don't bother with them. >+#button-goforward { Is there any chance you can use the browser buttons in the Modern theme?
Attachment #282902 - Flags: superreview?(neil) → superreview-
(In reply to comment #45) > >+ if (! (relPos % 50)) > 50? That's rather a big menu... how about 15? That is my fault/wish. The original TB patch has 20 items, which I feel are too *few*, so I recommended 50. > >+#button-goforward { > Is there any chance you can use the browser buttons in the Modern theme? Not really, they simple don't fit into the very special Modern mailnews theming, so I was fine with using the reply/forward icons here.
Attached patch SM port, v2: addressed Neil's review comments (obsolete) (deleted) — Splinter Review
Hartmut did most of the changes Neil requested, but asked me to fill in the PopulateHistoryMenu stuff where he lacked intimate mailnews XUL/JS knowledge. Hartmut, many thanks for putting together all these cumbersome details this patch requires! Changes to hafi's patch: - reordered any back/forward stuff, so that the back code is always before the forward one - clean-up of the PopulateHistoryMenu method, so that I don't think that drawing the loop out into the Init functions is necessary anymore; also ported over a TB fix for MIME-encoded subjects and authors
Attachment #282902 - Attachment is obsolete: true
Attachment #283929 - Flags: superreview?(neil)
(In reply to comment #46) >(In reply to comment #45) >>>+ if (! (relPos % 50)) >>50? That's rather a big menu... how about 15? >That is my fault/wish. The original TB patch has 20 items, which I feel are too >*few*, so I recommended 50. Well, I'm not convinced... I can't actually fit more than about 25 on my screen, and I shudder to think what BenoitRen will think ;-) >>>+#button-goforward { >>Is there any chance you can use the browser buttons in the Modern theme? >Not really, they simple don't fit into the very special Modern mailnews >theming, so I was fine with using the reply/forward icons here. Well, I guess we could take the back/forward buttons, cut out the circle, and paste over the stop button (which we already have in both versions).
Comment on attachment 283929 [details] [diff] [review] SM port, v2: addressed Neil's review comments >Index: mailnews/base/resources/content/mail3PaneWindowCommands.js >+ case "button_goBack": >+ case "cmd_goBack": >+ if (gDBView) >+ enabled.value = gDBView.navigateStatus(nsMsgNavigationType.back); >+ return enabled.value; I prefer the versions of the code in messageWindow.js (back and forward). >+ (i >= 0) && (i < maxPos) && (itemCount < 30); Nit: the ()s aren't strictly necessary. >+function NavigateToUri(target) >+{ >+ var historyIndex = target.getAttribute('value'); >+ var folderUri = messenger.getFolderUriAtNavigatePos(historyIndex); >+ var msgUri = messenger.getMsgUriAtNavigatePos(historyIndex); >+ var folder = RDF.GetResource(folderUri).QueryInterface(Components.interfaces.nsIMsgFolder); >+ var msgHdr = messenger.msgHdrFromURI(msgUri); >+ messenger.navigatePos += Number(historyIndex); >+ if (IsCurrentLoadedFolder(folder)) I had another look at the backend. Would this work? newMenuItem.setAttribute('value', i); ... messenger.navigatePos = target.getAttribute('value'); var msgUri = messenger.getMsgUriAtNavigatePos(0); var msgHdr = messenger.msgHdrFromURI(msgUri); if (IsCurrentLoadedFolder(msgHdr.folder)) The "Go Forward" button didn't have an icon at all in the Modern theme, did some of the style rules get misplaced somewhere? It gave me the chance to try using the navigation buttons, at least as placeholders; for forward I used: list-style-image: url(chrome://navigator/skin/icons/btn1.gif); -moz-image-region: rect(42px, 41px, 75px, 0px);
Attachment #283929 - Flags: superreview?(neil) → superreview+
(In reply to comment #49) >var msgHdr = messenger.msgHdrFromURI(msgUri); >if (IsCurrentLoadedFolder(msgHdr.folder)) No, beacuse this is a virtual folder, but it might be quicker to get the loaded folder's URI and do a string compare than get the resource of the URI.
Blocks: 399366
Attached patch SM port, v3: checked-in version (deleted) — Splinter Review
(In reply to comment #48) > >That is my fault/wish. The original TB patch has 20 items, which I feel are too > >*few*, so I recommended 50. > Well, I'm not convinced... I can't actually fit more than about 25 on my > screen, So 25 it is. 25 rows must be C= 64 style, right? ;-) > Well, I guess we could take the back/forward buttons, cut out the circle, and > paste over the stop button (which we already have in both versions). Filed bug 399366 for the icons. (In reply to comment #49) > I prefer the versions of the code in messageWindow.js (back and forward). Done. > >+ (i >= 0) && (i < maxPos) && (itemCount < 30); > Nit: the ()s aren't strictly necessary. Yeah, but it's much more readable this way. > The "Go Forward" button didn't have an icon at all in the Modern theme, did > some of the style rules get misplaced somewhere? Yes, sorry, left some stray list-style-image rule in the patch. (In reply to comment #50) > it might be quicker to get the loaded > folder's URI and do a string compare than get the resource of the URI. Done. Taking over review flags. Landed on trunk.
Attachment #283929 - Attachment is obsolete: true
Attachment #284384 - Flags: superreview+
Attachment #284384 - Flags: review+
No longer blocks: TB2SM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: