Last Comment Bug 74959 - implement mail "back" and "forward"
: implement mail "back" and "forward"
Status: VERIFIED FIXED
: verified1.8.1, verified1.8.1.3
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: x86 All
: -- normal with 5 votes (vote)
: Thunderbird2.0
Assigned To: David :Bienvenu
: esther
:
Mentors:
: 77099 163880 169947 234841 295552 (view as bug list)
Depends on: 368177 390921
Blocks: 161374 398081 399366
  Show dependency treegraph
 
Reported: 2001-04-05 23:43 PDT by (not reading, please use seth@sspitzer.org instead)
Modified: 2007-10-11 18:28 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
work in progress (39.14 KB, patch)
2006-07-26 22:13 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
more work in progress ... (45.54 KB, patch)
2006-08-01 15:06 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
proposed fix (52.32 KB, patch)
2006-08-11 16:36 PDT, David :Bienvenu
mscott: superreview+
Details | Diff | Splinter Review
fix the dtd entities. (1.26 KB, patch)
2006-08-20 13:11 PDT, David :Bienvenu
moco: superreview+
Details | Diff | Splinter Review
fix Neil's issue (1.21 KB, patch)
2006-08-20 15:13 PDT, David :Bienvenu
moco: superreview+
Details | Diff | Splinter Review
fix navigation in stand-alone msg window (2.37 KB, patch)
2006-08-22 09:36 PDT, David :Bienvenu
mscott: superreview+
Details | Diff | Splinter Review
fix enabling/disabling of forward/back buttons in stand-alone msg window (3.94 KB, patch)
2006-08-22 11:53 PDT, David :Bienvenu
mscott: superreview+
Details | Diff | Splinter Review
SM port (25.51 KB, patch)
2007-09-30 11:32 PDT, Hartmut Figge
mnyromyr: review+
neil: superreview-
Details | Diff | Splinter Review
SM port, v2: addressed Neil's review comments (25.60 KB, patch)
2007-10-07 14:30 PDT, Karsten Düsterloh
neil: superreview+
Details | Diff | Splinter Review
SM port, v3: checked-in version (25.38 KB, patch)
2007-10-10 16:05 PDT, Karsten Düsterloh
mnyromyr: review+
mnyromyr: superreview+
Details | Diff | Splinter Review

Description (not reading, please use seth@sspitzer.org instead) 2001-04-05 23:43:58 PDT
there is now UI, it was just accelerator driven.

bienvenu was telling me how 4.x had this.  sounded useful.
Comment 1 (not reading, please use seth@sspitzer.org instead) 2001-04-10 08:53:52 PDT
future
Comment 2 Keyser Sose 2001-04-23 21:45:32 PDT
*** Bug 77099 has been marked as a duplicate of this bug. ***
Comment 3 Alex Bishop 2001-04-29 16:20:29 PDT
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.
Comment 4 David :Bienvenu 2001-09-28 10:11:17 PDT
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)
Comment 5 Ian Pottinger 2002-04-03 22:13:25 PST
adding self to cc list
Comment 6 shadytrees 2003-08-17 13:37:18 PDT
Back is now implemented in Thunderbird. Any plans of copying that code to
Mozilla's Mail?
Comment 7 David :Bienvenu 2003-08-18 16:05:15 PDT
afaik, back isn't implemented in thunderbird; previous unread is implemented.
They're not the same.
Comment 8 timeless 2004-02-18 21:10:46 PST
*** Bug 234841 has been marked as a duplicate of this bug. ***
Comment 9 timeless 2004-02-18 21:11:42 PST
*** Bug 163880 has been marked as a duplicate of this bug. ***
Comment 10 timeless 2004-02-18 21:13:27 PST
*** Bug 169947 has been marked as a duplicate of this bug. ***
Comment 11 dmitry 2004-11-04 07:42:53 PST
Will this never be implemented? 3 years later, Thunderbird still does not have this.
Comment 12 Karsten Düsterloh 2005-08-27 11:32:42 PDT
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.)
Comment 13 David :Bienvenu 2006-07-26 22:13:14 PDT
Created attachment 230842 [details] [diff] [review]
work in progress

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.
Comment 14 David :Bienvenu 2006-08-01 15:06:20 PDT
Created attachment 231660 [details] [diff] [review]
more work in progress ...

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.
Comment 15 David :Bienvenu 2006-08-11 16:36:37 PDT
Created attachment 233310 [details] [diff] [review]
proposed fix

this is getting to where I'd like to get it reviewed and landed...
Comment 16 Scott MacGregor 2006-08-17 17:03:19 PDT
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?
Comment 17 neil@parkwaycc.co.uk 2006-08-19 16:35:34 PDT
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?
Comment 18 Alexander L. Slovesnik 2006-08-20 07:44:49 PDT
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.
Comment 19 David :Bienvenu 2006-08-20 12:27:56 PDT
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.
Comment 20 David :Bienvenu 2006-08-20 13:11:37 PDT
Created attachment 234697 [details] [diff] [review]
fix the dtd entities.

use consistent names for the go nav menu item entities.
Comment 21 (not reading, please use seth@sspitzer.org instead) 2006-08-20 14:36:41 PDT
Comment on attachment 234697 [details] [diff] [review]
fix the dtd entities.

sr=sspitzer, acting sr for mailnews (while mscott is away)
Comment 22 David :Bienvenu 2006-08-20 15:13:50 PDT
Created attachment 234707 [details] [diff] [review]
fix Neil's issue

remove unneeded code as pointed out by Neil.
Comment 23 David :Bienvenu 2006-08-20 15:15:11 PDT
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...)
Comment 24 (not reading, please use seth@sspitzer.org instead) 2006-08-20 15:18:49 PDT
Comment on attachment 234707 [details] [diff] [review]
fix Neil's issue

sr=sspitzer, acting sr for mscott.
Comment 25 Karsten Düsterloh 2006-08-20 17:17:37 PDT
> 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...

Comment 26 David :Bienvenu 2006-08-20 20:59:37 PDT
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.
Comment 27 Mike Cowperthwaite 2006-08-21 13:23:46 PDT
Are these buttons supposed to work in the standalone message window?  They don't appear to be.
Comment 28 David :Bienvenu 2006-08-21 13:26:46 PDT
I'll fix that - it's supposed to work...
Comment 29 AlexIhrig 2006-08-21 14:16:53 PDT
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.
Comment 30 David :Bienvenu 2006-08-22 09:36:24 PDT
Created attachment 234936 [details] [diff] [review]
fix navigation in stand-alone msg window

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.
Comment 31 David :Bienvenu 2006-08-22 11:53:02 PDT
Created attachment 234956 [details] [diff] [review]
fix enabling/disabling of forward/back buttons in stand-alone msg window
Comment 32 David :Bienvenu 2006-08-22 15:21:49 PDT
all patches so far landed on trunk and 2.0 branch
Comment 33 Scott MacGregor 2006-08-22 22:31:18 PDT
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 :)
Comment 34 David :Bienvenu 2006-08-23 11:00:05 PDT
I tried quick search on [ and ] and it worked...ah, the mysteries of the mac. Have ou tried it?
Comment 35 Marcia Knous [:marcia - use ni] 2007-01-15 16:09:42 PST
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.
Comment 36 Marcia Knous [:marcia - use ni] 2007-01-15 16:12:15 PST
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.
> 

Comment 37 David :Bienvenu 2007-01-15 16:18:13 PST
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...
Comment 38 Mike Cowperthwaite 2007-02-16 09:32:19 PST
*** Bug 295552 has been marked as a duplicate of this bug. ***
Comment 39 Carsten Book [:Tomcat] 2007-03-30 18:02:27 PDT
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
Comment 40 Tracy Walker [:tracy] 2007-04-05 16:14:19 PDT
Verified on 2.0.0.0 rc2 builds on Win XP and Mac OS 10.4.9
Comment 41 Hartmut Figge 2007-09-30 11:32:04 PDT
Created attachment 282902 [details] [diff] [review]
SM port

Adaption for SM suiterunner
Comment 42 Hartmut Figge 2007-09-30 11:40:47 PDT
(In reply to comment #41)

> Created an attachment (id=282902) [details]

Application/octet-stream? Hm, don't know, why this happened.

Comment 43 Karsten Düsterloh 2007-09-30 14:01:13 PDT
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.
Comment 44 neil@parkwaycc.co.uk 2007-10-01 08:59:42 PDT
(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 45 neil@parkwaycc.co.uk 2007-10-01 09:42:21 PDT
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?
Comment 46 Karsten Düsterloh 2007-10-01 10:14:59 PDT
(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.
Comment 47 Karsten Düsterloh 2007-10-07 14:30:05 PDT
Created attachment 283929 [details] [diff] [review]
SM port, v2: addressed Neil's review comments

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
Comment 48 neil@parkwaycc.co.uk 2007-10-08 06:02:02 PDT
(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 49 neil@parkwaycc.co.uk 2007-10-08 07:49:19 PDT
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);
Comment 50 neil@parkwaycc.co.uk 2007-10-08 14:03:58 PDT
(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.
Comment 51 Karsten Düsterloh 2007-10-10 16:05:48 PDT
Created attachment 284384 [details] [diff] [review]
SM port, v3: checked-in version

(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.

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