74959 - implement mail "back" and "forward"

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[(not reading, please use [e-mail] instead)]

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 [e-mail] instead)]

future

Comment 2

[Keyser Sose]

*** Bug 77099 has been marked as a duplicate of this bug. ***

Comment 3

[Alex Bishop]

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 [e-mail] (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]

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]

adding self to cc list

Comment 6

[shadytrees]

Back is now implemented in Thunderbird. Any plans of copying that code to
Mozilla's Mail?

Comment 7

[David :Bienvenu]

afaik, back isn't implemented in thunderbird; previous unread is implemented.
They're not the same.

Comment 8

[timeless]

*** Bug 234841 has been marked as a duplicate of this bug. ***

Comment 9

[timeless]

*** Bug 163880 has been marked as a duplicate of this bug. ***

Comment 10

[timeless]

*** Bug 169947 has been marked as a duplicate of this bug. ***

Comment 11

[dmitry]

Will this never be implemented? 3 years later, Thunderbird still does not have this.

Comment 12

[Karsten Düsterloh]

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]

Attachment: 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]

Attachment: 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]

Attachment: proposed fix

this is getting to where I'd like to get it reviewed and landed...

Comment 16

[Scott MacGregor]

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

[[e-mail]]

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]

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]

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]

Attachment: fix the dtd entities.

use consistent names for the go nav menu item entities.

Comment 21

[(not reading, please use [e-mail] instead)]

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]

Attachment: fix Neil's issue

remove unneeded code as pointed out by Neil.

Comment 23

[David :Bienvenu]

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 [e-mail] instead)]

Comment on attachment 234707 [details] [diff] [review]
fix Neil's issue

sr=sspitzer, acting sr for mscott.

Comment 25

[Karsten Düsterloh]

> 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]

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]

Are these buttons supposed to work in the standalone message window?  They don't appear to be.

Comment 28

[David :Bienvenu]

I'll fix that - it's supposed to work...

Comment 29

[Alex Ihrig [:Thunderbird_Mail_DE]]

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]

Attachment: 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]

Attachment: fix enabling/disabling of forward/back buttons in stand-alone msg window

Comment 32

[David :Bienvenu]

all patches so far landed on trunk and 2.0 branch

Comment 33

[Scott MacGregor]

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]

I tried quick search on [ and ] and it worked...ah, the mysteries of the mac. Have ou tried it?

Comment 35

[Marcia Knous [:marcia]]

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]]

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]

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 39

[Carsten Book [:Tomcat]]

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]]

Verified on 2.0.0.0 rc2 builds on Win XP and Mac OS 10.4.9

Comment 41

[Hartmut Figge]

Attachment: SM port

Adaption for SM suiterunner

Comment 42

[Hartmut Figge]

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

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

[[e-mail]]

(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

[[e-mail]]

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]

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

Attachment: 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

[[e-mail]]

(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

[[e-mail]]

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

[[e-mail]]

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

Attachment: 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.