Closed
Bug 74959
Opened 24 years ago
Closed 18 years ago
implement mail "back" and "forward"
Categories
(Thunderbird :: Mail Window Front End, defect)
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mscott
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
moco
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
moco
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mscott
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mscott
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mnyromyr
:
review+
mnyromyr
:
superreview+
|
Details | Diff | Splinter Review |
there is now UI, it was just accelerator driven.
bienvenu was telling me how 4.x had this. sounded useful.
Comment 3•24 years ago
|
||
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.
Assignee | ||
Comment 4•23 years ago
|
||
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
Comment 5•23 years ago
|
||
adding self to cc list
Comment 6•21 years ago
|
||
Back is now implemented in Thunderbird. Any plans of copying that code to
Mozilla's Mail?
Assignee | ||
Comment 7•21 years ago
|
||
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. ***
Comment 10•21 years ago
|
||
*** Bug 169947 has been marked as a duplicate of this bug. ***
Comment 11•20 years ago
|
||
Will this never be implemented? 3 years later, Thunderbird still does not have this.
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 12•19 years ago
|
||
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.)
Assignee | ||
Comment 13•18 years ago
|
||
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.
Assignee | ||
Comment 14•18 years ago
|
||
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
Assignee | ||
Comment 15•18 years ago
|
||
this is getting to where I'd like to get it reviewed and landed...
Attachment #233310 -
Flags: superreview?(mscott)
Comment 16•18 years ago
|
||
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 17•18 years ago
|
||
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•18 years ago
|
||
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.
Assignee | ||
Comment 19•18 years ago
|
||
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.
Assignee | ||
Comment 20•18 years ago
|
||
use consistent names for the go nav menu item entities.
Attachment #234697 -
Flags: superreview?(sspitzer)
Comment 21•18 years ago
|
||
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+
Assignee | ||
Comment 22•18 years ago
|
||
remove unneeded code as pointed out by Neil.
Attachment #234707 -
Flags: superreview?(sspitzer)
Assignee | ||
Comment 23•18 years ago
|
||
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 24•18 years ago
|
||
Comment on attachment 234707 [details] [diff] [review]
fix Neil's issue
sr=sspitzer, acting sr for mscott.
Attachment #234707 -
Flags: superreview?(sspitzer) → superreview+
Comment 25•18 years ago
|
||
> 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...
Assignee | ||
Comment 26•18 years ago
|
||
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•18 years ago
|
||
Are these buttons supposed to work in the standalone message window? They don't appear to be.
Assignee | ||
Comment 28•18 years ago
|
||
I'll fix that - it's supposed to work...
Comment 29•18 years ago
|
||
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.
Assignee | ||
Comment 30•18 years ago
|
||
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)
Assignee | ||
Comment 31•18 years ago
|
||
Attachment #234956 -
Flags: superreview?(mscott)
Updated•18 years ago
|
Attachment #234936 -
Flags: superreview?(mscott) → superreview+
Updated•18 years ago
|
Attachment #234956 -
Flags: superreview?(mscott) → superreview+
Assignee | ||
Comment 32•18 years ago
|
||
all patches so far landed on trunk and 2.0 branch
Comment 33•18 years ago
|
||
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 :)
Assignee | ||
Comment 34•18 years ago
|
||
I tried quick search on [ and ] and it worked...ah, the mysteries of the mac. Have ou tried it?
Comment 35•18 years ago
|
||
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•18 years ago
|
||
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.
>
Assignee | ||
Comment 37•18 years ago
|
||
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•18 years ago
|
||
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
Comment 40•18 years ago
|
||
Verified on 2.0.0.0 rc2 builds on Win XP and Mac OS 10.4.9
Status: RESOLVED → VERIFIED
Updated•18 years ago
|
Keywords: fixed1.8.1 → verified1.8.1
Comment 41•17 years ago
|
||
Adaption for SM suiterunner
Attachment #282902 -
Flags: superreview?(neil)
Attachment #282902 -
Flags: review?(mnyromyr)
Comment 42•17 years ago
|
||
(In reply to comment #41)
> Created an attachment (id=282902) [details]
Application/octet-stream? Hm, don't know, why this happened.
Updated•17 years ago
|
Attachment #282902 -
Attachment is patch: true
Attachment #282902 -
Attachment mime type: application/octet-stream → text/plain
Comment 43•17 years ago
|
||
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+
Comment 44•17 years ago
|
||
(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•17 years ago
|
||
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-
Comment 46•17 years ago
|
||
(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•17 years ago
|
||
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)
Comment 48•17 years ago
|
||
(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•17 years ago
|
||
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+
Comment 50•17 years ago
|
||
(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•17 years ago
|
||
(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+
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•