Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Back/forward buttons do not work (semi-working patch in there!) #393

Open
ghost opened this issue Aug 23, 2011 · 30 comments
Open

Back/forward buttons do not work (semi-working patch in there!) #393

ghost opened this issue Aug 23, 2011 · 30 comments

Comments

@ghost
Copy link

ghost commented Aug 23, 2011

Normally, you can use the back/forward buttons to navigate between previously viewed messages, which is nice if they were way down in the list or in different folders so that you don't have to spend a lot of time finding them repeatedly. They do not work with conversations installed.

@protz
Copy link
Collaborator

protz commented Aug 23, 2011

Hi,

Thanks for reporting. I'm too busy with other stuff so I'm not going to implement this, but any patch would of course be most welcome.

@blerner
Copy link
Contributor

blerner commented Sep 2, 2011

+1 for this. I might be able to help; do you have a sense of what's involved, or which bits of TBConversations internals are relevant starting points?

@protz
Copy link
Collaborator

protz commented Sep 2, 2011

Sure! Here's some guidance if you want to fix this. I'm talking only about navigating in the mail:3pane, and I'm not taking into account other cases, such as the conversation-in-a-tab case.

  • The place where we actually figure out if we're switching to a different conversation is https://github.com/protz/GMail-Conversation-View/blob/master/modules/monkeypatch.js#L457 ; at this very line, window.Conversations.currentConversation is the old Conversation (Conversations as in modules/conversation.js), and we replace it with the newer conversation a couple lines later.
  • I don't know how the back/forward buttons work. I added the back button to my toolbar, and the DOM Inspector told me that it was wired to the command cmd_goBack
  • a mxr search http://mxr.mozilla.org/comm-central/search?string=cmd_goBack&filter=^[^\0]*%24&tree=comm-central told me that this was related to gFolderDisplay.navigate (reading the definition of GoNextMessage
  • reading the implementation of navigate, it turns out that this is actually implemented by this.view.dbView.viewNavigate. I know that dbView happens to be nsIMsgDBView,
  • on to the source of mailnews/base/src/nsMsgDBView.cpp: I search for ::back
  PRBool enableGoForward = PR_FALSE;
  PRBool enableGoBack = PR_FALSE;

  NavigateStatus(nsMsgNavigationType::forward, &enableGoForward);
  NavigateStatus(nsMsgNavigationType::back, &enableGoBack);
  if (!summaryStateChanged &&
      (numSelected == mNumSelectedRows ||
      (numSelected > 1 && mNumSelectedRows > 1)) && (commandsNeedDisablingBecauseOfSelec
      && enableGoForward == mGoForwardEnabled && enableGoBack == mGoBackEnabled)
  {
  }
  // don't update commands if we're suppressing them, or if we're removing rows, unless 
  else if (!mSuppressCommandUpdating && mCommandUpdater && (!mRemovingRow || GetSize() =
  {
    mCommandUpdater->UpdateCommandStatus();
  }

The code above is the first hit. Something interesting seems to be happening in NavigateStatus. I read a reference to nsMessenger::GetNavigateHistory. I then find out about nsMessenger::mLoadedMsgHistory. AddMsgUrlToNavigateHistory seems to be doing all the stuff, and is called by LoadURL which conversations should end up calling when it displays a message...

At that point, I stopped digging into the code. I added a return true in Thunderbird's mail/base/content/mail3PaneWindowCommands.js on line 447, to pretend that the back command is always enabled, but then I got errors in the error console. Which confirmed my initial feeling that this is waaaaay too complicated for Thunderbird Conversations to interact with.

Anyway, if you're motivated, what you can do is wrap your head around nsMsgDBView's implementation, and figure out exactly in which cases it will call nsMessenger's relevant functions; Thunderbird Conversations confuses nsMsgDBView a lot, because when there's more than one message selected, it appears to nsMsgDBView that we're in a thread summary, which nsMsgDBView obviously doesn't want to navigate. In that case, I imagine it will refuse to add a message url (which message by the way, since there's so many of them selected?) to the navigation history...

This is horrible C++ code and there's little changes you can "do the right thing" in a clean way. The alternative is to add a hook to gFolderDisplay.navigate, which is Javascript, and wonderfully written, hence much more easy to interact with. However, you're likely to break a lot of stuff that way. Fortunately, this is an area that is well-covered by tests.

Anyway, I guess my point is, if I'm not doing it, there's a reason for it: it's too hard, and not worth spending a week refactoring the C++ code just so that I have that feature working with Conversations. If you want to contribute, I can suggest much easier bugs!

Cheers,

jonathan

@blerner
Copy link
Contributor

blerner commented Sep 6, 2011

Yowch! This looks tricky, but I find myself missing this feature a lot,
so I'll give it a go. (FWIW, I'm starting a new post-doc at the moment,
so I have to focus more on research than on this. If/when I get stuck,
I'll be sure to ping you...)

~ben

On 9/2/2011 1:02 AM, protz wrote:

Sure! Here's some guidance if you want to fix this. I'm talking only about navigating in the mail:3pane, and I'm not taking into account other cases, such as the conversation-in-a-tab case.

  • The place where we actually figure out if we're switching to a different conversation is https://github.com/protz/GMail-Conversation-View/blob/master/modules/monkeypatch.js#L457 ; at this very line, window.Conversations.currentConversation is the old Conversation (Conversations as in modules/conversation.js), and we replace it with the newer conversation a couple lines later.
  • I don't know how the back/forward buttons work. I added the back button to my toolbar, and the DOM Inspector told me that it was wired to the command cmd_goBack
  • a mxr search http://mxr.mozilla.org/comm-central/search?string=cmd_goBack&filter=^[^\0]*%24&tree=comm-central told me that this was related to gFolderDisplay.navigate (reading the definition of GoNextMessage
  • reading the implementation of navigate, it turns out that this is actually implemented by this.view.dbView.viewNavigate. I know that dbView happens to be nsIMsgDBView,
  • on to the source of mailnews/base/src/nsMsgDBView.cpp: I search for ::back
   PRBool enableGoForward = PR_FALSE;
   PRBool enableGoBack = PR_FALSE;

   NavigateStatus(nsMsgNavigationType::forward,&enableGoForward);
   NavigateStatus(nsMsgNavigationType::back,&enableGoBack);
   if (!summaryStateChanged&&
       (numSelected == mNumSelectedRows ||
       (numSelected>  1&&  mNumSelectedRows>  1))&&  (commandsNeedDisablingBecauseOfSelec
       &&  enableGoForward == mGoForwardEnabled&&  enableGoBack == mGoBackEnabled)
   {
   }
   // don't update commands if we're suppressing them, or if we're removing rows, unless
   else if (!mSuppressCommandUpdating&&  mCommandUpdater&&  (!mRemovingRow || GetSize() =
   {
     mCommandUpdater->UpdateCommandStatus();
   }

The code above is the first hit. Something interesting seems to be happening in NavigateStatus. I read a reference to nsMessenger::GetNavigateHistory. I then find out about nsMessenger::mLoadedMsgHistory. AddMsgUrlToNavigateHistory seems to be doing all the stuff, and is called by LoadURL which conversations should end up calling when it displays a message...

At that point, I stopped digging into the code. I added a return true in Thunderbird's mail/base/content/mail3PaneWindowCommands.js on line 447, to pretend that the back command is always enabled, but then I got errors in the error console. Which confirmed my initial feeling that this is waaaaay too complicated for Thunderbird Conversations to interact with.

Anyway, if you're motivated, what you can do is wrap your head around nsMsgDBView's implementation, and figure out exactly in which cases it will call nsMessenger's relevant functions; Thunderbird Conversations confuses nsMsgDBView a lot, because when there's more than one message selected, it appears to nsMsgDBView that we're in a thread summary, which nsMsgDBView obviously doesn't want to navigate. In that case, I imagine it will refuse to add a message url (which message by the way, since there's so many of them selected?) to the navigation history...

This is horrible C++ code and there's little changes you can "do the right thing" in a clean way. The alternative is to add a hook to gFolderDisplay.navigate, which is Javascript, and wonderfully written, hence much more easy to interact with. However, you're likely to break a lot of stuff that way. Fortunately, this is an area that is well-covered by tests.

Anyway, I guess my point is, if I'm not doing it, there's a reason for it: it's too hard, and not worth spending a week refactoring the C++ code just so that I have that feature working with Conversations. If you want to contribute, I can suggest much easier bugs!

Cheers,

jonathan

@blerner
Copy link
Contributor

blerner commented Sep 7, 2011

tl;dr: The following code seems to work reasonably well for me:

Message.prototype.expand = function () {
...

// NEW CODE
let msgUri = this._msgHdr.folder.getUriForMsg(this._msgHdr);
try {
let gDBView = topMail3Pane(this).gDBView;
var viewIndex = gDBView.findIndexFromKey(this._msgHdr.messageKey,
true);
const nsMsgViewIndex_None = 0xFFFFFFFF;
if (viewIndex != nsMsgViewIndex_None) // selected message is
possibly from another folder
gDBView.loadMessageByViewIndex(viewIndex);
else {
// UNSURE HOW TO GET THIS TO WORK -- CHANGING FOLDERS IS SUBTLE AND
BROKEN FOR NOW
// let gMessenger =
Cc["@mozilla.org/messenger;1"].createInstance(Ci.nsIMessenger);
// gMessenger.setWindow(mailPane, mailPane.msgWindow);
// gMessenger.openURL(msgUri);
}
} catch (e) {
Components.utils.reportError("Error for URI " + msgUri + "\n" + e);
}

}

More details: Ok, I think I've reconstructed the call stack that you're
describing here, and traced through some more of it.

  • mail/base/content/mail3PaneWindowCommands.js#706, function
    doCommand(cmd_goBack) calls GoNextMessage
    • mail/base/content/msgViewNavigation.js#260, function
      GoNextMessage calls gFolderDisplay.navigate(nsMsgNavigationType.back)
      • mail/base/content/folderDisplay.js#1838: function
        FolderDisplayWidget_navigate eventually calls
        this.view.dbView.viewNavigate. dbView is an nsIMsgDBView, implemented
        by nsMsgDBView so...
        • mailnews/base/src/nsMsgDBView.cpp#6290:
          nsMsgDBView::ViewNavigate calls nsMsgDBView::NavigateFromPos
          • mailnews/base/src/nsMsgDBView.cpp#6505:
            nsMsgDBView::NavigateFromPos, handling back and forward, eventually
            calls nsMessenger::SetNavigatePos. This ultimately returns a viewIndex
            (by way of the resultIndexObj out-param).
      • FolderDisplayWidget_navigate eventually calls selectViewIndex
        when a new message is selected
        • mail/base/content/folderDisplay.js#2227: function
          FolderDisplayWidget_selectViewIndex eventually calls
          this.view.dbView.selectionChanged
          • mailnews/base/src/nsMsgDBView.cpp#1302: function
            nsMsgDBView::SelectionChanged calls either LoadMessageByViewIndex or
            UpdateDisplayMessage
            • mailnews/base/src/nsMsgDBView.cpp#1265: function
              nsMsgDBViw::LoadMessageByViewIndex eventually calls OpenURL and
              UpdateDisplayMessage
              • OpenURL eventually calls
                AddMsgUrlToNavigateHistory (phew!)
            • SelectionChanged eventually triggers
              FolderDisplayWidget_summarizeSelection, which calls
              MessageDisplayWidget.onSelectedMessagesChanged, which you've already
              monkey-patched (phew!)

So the only thing missing in all this mess is to get message urls into
the navigate-history list, right? I think what I want to have happen is
that whenever message.expand() is called (message.js#1107), somehow we
need to call
nsMessenger::AddMsgUrlToNavigateHistory(message.toTmplData().uri) -- and
yes, I know I just crossed languages :-p It seems like all that's
needed is to call messenger.openURL(message.toTmplData().uri)

Then I think that if you don't monkeypatch anything to do with
back/forward history, pressing back or forward will cause a navigation
to the message url, which will call the whole chain above and ultimately
display the right message.

The only challenge is when conversations skip around between folders --
then calling openURL just doesn't seem to work. Dunno why, but
regardless the code above is way better than nothing, and hopefully you
know the relevant code better than me and can suss out whatever I'm
missing :) (I do know that the call to setWindow is crucial, otherwise
the call to openURL will completely crash Thunderbird, despite the
try/catch. I suspect that there's a smaller repro that can be pulled
out of here, because I think this is a potential security bug...)

~ben

@protz
Copy link
Collaborator

protz commented Sep 10, 2011

I think you need to file a bug on Bugzilla if you managed to crash Thunderbird. That's important.

Re your patch, what worries me is that loadMsgByViewIndex will trigger a message loading in the background (i.e. in the regular message pane), which

  1. will add more stress on Thunderbird and will require unnecessary actions (possible performance hit),
  2. will trigger side-effects: enigmail asking for a decryption, "do you want to send the return receipt", etc.

It's cool that it works, and I'm impressed that you found a way to do this! However, this seems like a huge hack. I'm kinda tolerant w.r.t. hacks, as there's lots of them in Conversations, but this really makes the hack-o-meter explode. Can you just patch nsMessenger and add a hook there that Thunderbird Conversations could use? With the rapid release cycle, that means that the patch would be integrated in, at most, 3*6 weeks, and Thunderbird Conversations could take advantage of it pretty soon.

Thanks for your work! This is definitely not one of the easiest bugs...

@protz
Copy link
Collaborator

protz commented Sep 10, 2011

I think my comment above boils down to: I think we need to make nsMessenger::AddMsgUrlToNavigateHistory(message.toTmplData().uri) scriptable. Really.

This isn't hard to do. If you want, I could provide guidance on how to make that happen, and I could review your patch on the bugzilla side of things.

@blerner
Copy link
Contributor

blerner commented Sep 10, 2011

Yes. That would by FAR be the easiest and most correct way to do things
(but see ramble below). In the code I took, the whole goal was to find
a scriptable way to trigger that call with "minimal" other processing.
I guess I figured that "the message was already loaded, so it ought to
be cached or really cheap to 'load' again," but I didn't think about
side effects like enigmail... Hey, as I said, "The following code seems
to work reasonably well for me" :-) My current setup doesn't use
enigmail...

I deliberately avoided patching nsMessenger because 1) I was trying to
stay in JS as much as I could, and 2) I don't currently have a machine
where I can build mozilla programs (laptop is underpowered for that, one
work-related laptop can't have open-source source code on it, and I'm
waiting for a new desktop machine at school). Hopefully that'll get
fixed soon, and I can work on this patch some more.

Regarding making Add...History() scriptable -- I'm leery of it for one unfortunate reason: the way the back/forward history works in TB is _not_ a true back/forward history like in Fx, and it's very brittle. If you view a bunch of messages, go back a few, and then view a new one, it doesn't clear your forward history, but rather slots the new one into the middle of the history. The nsMessenger::mLoadedMsgHistory array is (I think!) conceptually an array of pairs (folder, msgUrl), but is implemented as [folder, url, folder, url, ...], and all history-shifting operations should always work by offsets of 2...except when it seems sometimes they don't. I'm pretty sure I've filed a bug where if (without Conversations) you view 1 new message in each of several folders (A, B, C), and try to go back from C to B to A, the history navigation fails with a no-op. If you view 2 messages each (A1, A2, B1, B2, C1, C2) and try to go back repeatedly, you'll see "C2, no effect, B2, no effect, A2, no effect" because somewhere there's an off-by-one bug. All this is to say that mLoadedMsgHistory is not something I'd play around with lightly, and it'd likely be better to figure out who wrote it originally, get the full design from them, and then re-write or at least document almost all of it :(

A much cleaner design, probably, might take after HTML5's pushState()
and popState(), as well as navigate(). Then, for instance,
Conversations could pushState each interim view of a conversation (e.g.
which messages are open) without confusing nsMsgDBView quite so badly.
Another extension might implement something like Outlook's quick
attachment preview, which gives multiple views of the same message
without navigating to any new message.

On 9/10/2011 9:47 AM, Jonathan Protzenko wrote:

I think my comment above boils down to: I think we need to make nsMessenger::AddMsgUrlToNavigateHistory(message.toTmplData().uri) scriptable. Really.

This isn't hard to do. If you want, I could provide guidance on how to make that happen, and I could review your patch on the bugzilla side of things.

@blerner
Copy link
Contributor

blerner commented Sep 10, 2011

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=686123

On 9/10/2011 9:45 AM, Jonathan Protzenko wrote:

I think you need to file a bug on Bugzilla if you managed to crash Thunderbird. That's important.

@protz
Copy link
Collaborator

protz commented Sep 12, 2011

Cool. Let me know if you need help for the patch to make the right
function scriptable; I usually hang out in #maildev as protz (see
https://wiki.mozilla.org/Thunderbird/CommunicationChannels).

On Sat 10 Sep 2011 04:56:44 PM CEST, Ben Lerner wrote:

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=686123

On 9/10/2011 9:45 AM, Jonathan Protzenko wrote:

I think you need to file a bug on Bugzilla if you managed to crash Thunderbird. That's important.

@blerner
Copy link
Contributor

blerner commented Jan 25, 2012

I haven't had a chance to work on making the function above scriptable (very busy at school), but maybe there's a shorter-term fix: could you add a hook, in hooks.js, called onMessageExpanded(aMsgHeader, aMessage), that gets called as the last action of Message.prototype.expand? If so, I could implement the hack above as a standalone extension that uses your hooks, until one of us gets TB to make the function above scriptable... Keeps your code clean, and is easy to replace later :)

@ghost
Copy link
Author

ghost commented Jan 25, 2012

Hi,

I'm not a conversations/mozilla programmer unforuntately.

Jon

On Wednesday, January 25, 2012 15:45:27, Ben Lerner wrote:

I haven't had a chance to work on making the function above scriptable (very busy at school), but maybe there's a shorter-term fix: could you add a hook, in hooks.js, called onMessageExpanded(aMsgHeader, aMessage), that gets called as the last action of Message.prototype.expand? If so, I could implement the hack above as a standalone extension that uses your hooks, until one of us gets TB to make the function above scriptable... Keeps your code clean, and is easy to replace later :)


Reply to this email directly or view it on GitHub:
#393 (comment)

@protz
Copy link
Collaborator

protz commented Jan 26, 2012

There's a function called onMessageStreamed and another one called
onMessageSelected. I think the latter is what you want, it basically
notifies you as soon as a message is "active".

On Wed 25 Jan 2012 09:47:27 PM CET, 86smopuiM wrote:

Hi,

I'm not a conversations/mozilla programmer unforuntately.

Jon

On Wednesday, January 25, 2012 15:45:27, Ben Lerner wrote:

I haven't had a chance to work on making the function above scriptable (very busy at school), but maybe there's a shorter-term fix: could you add a hook, in hooks.js, called onMessageExpanded(aMsgHeader, aMessage), that gets called as the last action of Message.prototype.expand? If so, I could implement the hack above as a standalone extension that uses your hooks, until one of us gets TB to make the function above scriptable... Keeps your code clean, and is easy to replace later :)


Reply to this email directly or view it on GitHub:
#393 (comment)


Reply to this email directly or view it on GitHub:
#393 (comment)

@blerner
Copy link
Contributor

blerner commented Jan 26, 2012

In the latest version of Conversations I have (2.2.4), I don't see
onMessageSelected anywhere. But, putting my hook into onMessageStreamed
seems to be working so far. The following is the entirety of my
extension (ignoring the boilerplate overlay.xul and stuff to get it to
load):

Components.utils.import("resource://conversations/misc.js");
Components.utils.import("resource://conversations/hook.js");

let enableBackForward = {
     onMessageStreamed : function(msgHdr, aDomNode, msgWindow, message) {
         let msgUri = msgHdr.folder.getUriForMsg(msgHdr);
         try {
             let gDBView = topMail3Pane(message).gDBView;
             var viewIndex = gDBView.findIndexFromKey(msgHdr.messageKey, 
true);
             const nsMsgViewIndex_None = 0xFFFFFFFF;
             if (viewIndex != nsMsgViewIndex_None) // selected message 
is possibly from another folder
                 gDBView.loadMessageByViewIndex(viewIndex);
             else {
                 // UNSURE HOW TO GET THIS TO WORK -- CHANGING FOLDERS 
IS SUBTLE AND BROKEN FOR NOW
                 // let gMessenger = 
Cc["@mozilla.org/messenger;1"].createInstance(Ci.nsIMessenger);
                 // gMessenger.setWindow(mailPane, mailPane.msgWindow);
                 // gMessenger.openURL(msgUri);
             }
         } catch (e) {
             Components.utils.reportError("Error for URI " + msgUri + 
"\n" + e);
         }
     },
     onMessageBeforeStreaming : function(aMessage) { },
     onMessageBeforeSend : function(aAddress, aEditor, aStatus) { },
     onReplyComposed : function(aMessage, aBody) { },
     onFocusMessage : function(aMessage) { }
}
registerHook(enableBackForward);

If you feel like including this code in future versions of
Conversations, go right ahead. :)
~ben

@protz
Copy link
Collaborator

protz commented Jan 26, 2012

Hi Ben, it's awesome you got this to work! Here's some miscellaneous remarks:

  • onMessageSelected has been added after the 2.2 series, it's in the -master branch of this repository. I erroneously assumed you were using the git version of Conversations since you were hacking with it.
  • I would be very, very happy to integrate this as one of the core plugins (just like enigmail for instance). You could submit pull requests for this file. There's one contributor taking care of the enigmail plugin, and that model is working very well.
  • However, the main problem I have is that loadMessageByViewIndex (see http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#1264) triggers loadURL from nsMessenger (see http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessenger.cpp#515), which does an awful lot of useless work, including displaying and rendering (if I'm not mistaken) the message into the hidden "classic viewer". I was about to say that we just need to make AddMsgUrlToNavigateHistory scriptable, but then I realized I've said all of that already :).

I'll try to see how difficult it is to make nsMessenger::AddMsgUrlToNavigateHistory scriptable.

@blerner
Copy link
Contributor

blerner commented Jan 26, 2012

Yep :) I hadn't been using the -master branch because until this hook,
I had to keep patching your code to keep this feature, and I tried to
avoid doing that too frequently. Now that it's a standalone hook,
though, I can keep up with the newer versions. And I'll see if it makes
more sense as the onMessageSelected hook (it sounds like it should) than
onMessageStreamed.

This is very much a stopgap measure until nsMessenger::AMUTNH is
scriptable. LoadMessageByViewIndex was just the way for me to,
expensively but mostly innocuously (except for enigmail and such) cause
AMUTNH to be called.

On 1/26/2012 9:00 AM, Jonathan Protzenko wrote:

Hi Ben, it's awesome you got this to work! Here's some miscellaneous remarks:

  • onMessageSelected has been added after the 2.2 series, it's in the -master branch of this repository. I erroneously assumed you were using the git version of Conversations since you were hacking with it.
  • I would be very, very happy to integrate this as one of the core plugins (just like enigmail for instance). You could submit pull requests for this file. There's one contributor taking care of the enigmail plugin, and that model is working very well.
  • However, the main problem I have is that loadMessageByViewIndex (see http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#1264) triggers loadURL from nsMessenger (see http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessenger.cpp#515), which does an awful lot of useless work, including displaying and rendering (if I'm not mistaken) the message into the hidden "classic viewer". I was about to say that we just need to make AddMsgUrlToNavigateHistory scriptable, but then I realized I've said all of that already :).

I'll try to see how difficult it is to make nsMessenger::AddMsgUrlToNavigateHistory scriptable.


Reply to this email directly or view it on GitHub:
#393 (comment)

@protz
Copy link
Collaborator

protz commented Jan 26, 2012

https://bugzilla.mozilla.org/show_bug.cgi?id=721365

I just compiled Thunderbird with this, and then I'm able to add entries to the back/forward menu properly. There's some extra hacking needed in order to trick Thunderbird into thinking the commands are enabled, but that should be simple enough.

@protz
Copy link
Collaborator

protz commented Jan 26, 2012

Also, you still need to monkey-patch isCommandEnabled. Snippet:

+    let oldFunc = window.DefaultController.isCommandEnabled;
+    window.DefaultController.isCommandEnabled = function(command) {
+      switch (command) {
+        case "cmd_goBack":
+        case "cmd_goForward":
+          return true;
+        default:
+          return oldFunc(command);
+      }
+    }

You could add this as a separate function in monkeypatch.js; then, you could add the call in message.js, both in onSelected and after the msgLoaded stuff in streamMessage. After that, you could submit a pull request and I'd be happy to integrate the stuff in Conversations.

@blerner
Copy link
Contributor

blerner commented Jan 26, 2012

I don't have that currently patched, because IIRC, I'd tried that once,
and it just made back/forward permanently enabled, even if you were at
the beginning or end of the b/f history. And I don't recall whether I
found APIs to use to detect whether we were at the beginning or end, and
enable/disable accordingly :) I'll look into it again, though it may
take me a little while.

On 1/26/2012 9:20 AM, Jonathan Protzenko wrote:

Also, you still need to monkey-patch isCommandEnabled. Snippet:

+    let oldFunc = window.DefaultController.isCommandEnabled;
+    window.DefaultController.isCommandEnabled = function(command) {
+      switch (command) {
+        case "cmd_goBack":
+        case "cmd_goForward":
+          return true;
+        default:
+          return oldFunc(command);
+      }
+    }

You could add this as a separate function in monkeypatch.js; then, you could add the call in message.js, both in onSelected and after the msgLoaded stuff in streamMessage. After that, you could submit a pull request and I'd be happy to integrate the stuff in Conversations.


Reply to this email directly or view it on GitHub:
#393 (comment)

@protz
Copy link
Collaborator

protz commented Jan 26, 2012

Yes, that's a good remark. If there's extra hacking needed on the Thunderbird side, please let me know (e.g. do I need to make more functions scriptable for you to properly detect whether there's anything left in the history ?).

@protz
Copy link
Collaborator

protz commented Jan 26, 2012

If I create a build for you that contains my patch, will it help you figure out the remaining parts?

@blerner
Copy link
Contributor

blerner commented Jan 26, 2012

It might, but at the moment I don't have a dedicated dev box, just my
all-purpose laptop, which is running TB9.0.1 on Windows. It'll take me
a bit of time to set up a VM where I could test other versions -- I had
a bit of a lull in schoolwork, so I could get back to this patch. Now
the lull is over ;-) What I need to do is trace through the code paths
we figured out earlier in this thread, remind myself how back/forward
history actually works, and then I'll know better whether you need to
make other APIs scriptable or not...

On 1/26/2012 9:31 AM, Jonathan Protzenko wrote:

If I create a build for you that contains my patch, will it help you figure out the remaining parts?


Reply to this email directly or view it on GitHub:
#393 (comment)

@protz
Copy link
Collaborator

protz commented Jan 26, 2012

Alright, it's just that it's not super-hard for me to generate a build
with the patch applied, you could then play with it, and at least not
have to setup a build environment. Just edit the .js files with, as
long as you know the right flags, don't require you to recompile
Thunderbird, just restart it...

On Thu 26 Jan 2012 03:49:59 PM CET, Ben Lerner wrote:

It might, but at the moment I don't have a dedicated dev box, just my
all-purpose laptop, which is running TB9.0.1 on Windows. It'll take me
a bit of time to set up a VM where I could test other versions -- I had
a bit of a lull in schoolwork, so I could get back to this patch. Now
the lull is over ;-) What I need to do is trace through the code paths
we figured out earlier in this thread, remind myself how back/forward
history actually works, and then I'll know better whether you need to
make other APIs scriptable or not...

On 1/26/2012 9:31 AM, Jonathan Protzenko wrote:

If I create a build for you that contains my patch, will it help you figure out the remaining parts?


Reply to this email directly or view it on GitHub:
#393 (comment)


Reply to this email directly or view it on GitHub:
#393 (comment)

@blerner
Copy link
Contributor

blerner commented Jan 26, 2012

My only concern is that I'd need a side-by-side installation of two TB
versions. I mean, I doubt dropping a TB11-or-so custom build into a TB9
install directory will work...

On 1/26/2012 9:53 AM, Jonathan Protzenko wrote:

Alright, it's just that it's not super-hard for me to generate a build
with the patch applied, you could then play with it, and at least not
have to setup a build environment. Just edit the .js files with, as
long as you know the right flags, don't require you to recompile
Thunderbird, just restart it...

On Thu 26 Jan 2012 03:49:59 PM CET, Ben Lerner wrote:

It might, but at the moment I don't have a dedicated dev box, just my
all-purpose laptop, which is running TB9.0.1 on Windows. It'll take me
a bit of time to set up a VM where I could test other versions -- I had
a bit of a lull in schoolwork, so I could get back to this patch. Now
the lull is over ;-) What I need to do is trace through the code paths
we figured out earlier in this thread, remind myself how back/forward
history actually works, and then I'll know better whether you need to
make other APIs scriptable or not...

On 1/26/2012 9:31 AM, Jonathan Protzenko wrote:

If I create a build for you that contains my patch, will it help you figure out the remaining parts?


Reply to this email directly or view it on GitHub:
#393 (comment)


Reply to this email directly or view it on GitHub:

#393 (comment)

Reply to this email directly or view it on GitHub:
#393 (comment)

@protz
Copy link
Collaborator

protz commented Jan 26, 2012

https://developer.mozilla.org/en/Setting_up_extension_development_environment
contains all the information you need to get started. All sections are
important, especially the part where you create a new profile and learn
to use it for development only, so that you can keep the two versions
side by side. The dev build I'm offering to build for you will be a big
zip file that you'll have to unpack somewhere.

On Thu 26 Jan 2012 04:01:09 PM CET, Ben Lerner wrote:

My only concern is that I'd need a side-by-side installation of two TB
versions. I mean, I doubt dropping a TB11-or-so custom build into a TB9
install directory will work...

On 1/26/2012 9:53 AM, Jonathan Protzenko wrote:

Alright, it's just that it's not super-hard for me to generate a build
with the patch applied, you could then play with it, and at least not
have to setup a build environment. Just edit the .js files with, as
long as you know the right flags, don't require you to recompile
Thunderbird, just restart it...

On Thu 26 Jan 2012 03:49:59 PM CET, Ben Lerner wrote:

It might, but at the moment I don't have a dedicated dev box, just my
all-purpose laptop, which is running TB9.0.1 on Windows. It'll take me
a bit of time to set up a VM where I could test other versions -- I had
a bit of a lull in schoolwork, so I could get back to this patch. Now
the lull is over ;-) What I need to do is trace through the code paths
we figured out earlier in this thread, remind myself how back/forward
history actually works, and then I'll know better whether you need to
make other APIs scriptable or not...

On 1/26/2012 9:31 AM, Jonathan Protzenko wrote:

If I create a build for you that contains my patch, will it help you figure out the remaining parts?


Reply to this email directly or view it on GitHub:
#393 (comment)


Reply to this email directly or view it on GitHub:

#393 (comment)

Reply to this email directly or view it on GitHub:
#393 (comment)


Reply to this email directly or view it on GitHub:
#393 (comment)

@blerner
Copy link
Contributor

blerner commented Jan 26, 2012

Yep, I'm familiar with it. And I have a test profile set up, too. I
just recall some frustration during the Firefox 3.6->4.0 transition
where I had to keep reinstalling both versions, rather than just
unpacking one of them into a new directory. If I can just unpack your
zip, without needing to install it (and thereby confuse Windows), then
I'm good to go.

On 1/26/2012 10:02 AM, Jonathan Protzenko wrote:

https://developer.mozilla.org/en/Setting_up_extension_development_environment
contains all the information you need to get started. All sections are
important, especially the part where you create a new profile and learn
to use it for development only, so that you can keep the two versions
side by side. The dev build I'm offering to build for you will be a big
zip file that you'll have to unpack somewhere.

On Thu 26 Jan 2012 04:01:09 PM CET, Ben Lerner wrote:

My only concern is that I'd need a side-by-side installation of two TB
versions. I mean, I doubt dropping a TB11-or-so custom build into a TB9
install directory will work...

On 1/26/2012 9:53 AM, Jonathan Protzenko wrote:

Alright, it's just that it's not super-hard for me to generate a build
with the patch applied, you could then play with it, and at least not
have to setup a build environment. Just edit the .js files with, as
long as you know the right flags, don't require you to recompile
Thunderbird, just restart it...

On Thu 26 Jan 2012 03:49:59 PM CET, Ben Lerner wrote:

It might, but at the moment I don't have a dedicated dev box, just my
all-purpose laptop, which is running TB9.0.1 on Windows. It'll take me
a bit of time to set up a VM where I could test other versions -- I had
a bit of a lull in schoolwork, so I could get back to this patch. Now
the lull is over ;-) What I need to do is trace through the code paths
we figured out earlier in this thread, remind myself how back/forward
history actually works, and then I'll know better whether you need to
make other APIs scriptable or not...

On 1/26/2012 9:31 AM, Jonathan Protzenko wrote:

If I create a build for you that contains my patch, will it help you figure out the remaining parts?


Reply to this email directly or view it on GitHub:
#393 (comment)


Reply to this email directly or view it on GitHub:

#393 (comment)

Reply to this email directly or view it on GitHub:
#393 (comment)


Reply to this email directly or view it on GitHub:

#393 (comment)

Reply to this email directly or view it on GitHub:
#393 (comment)

@protz
Copy link
Collaborator

protz commented Jan 26, 2012

The windows build will be available from
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/jonathan.protzenko@gmail.com-6ad39627006a
in a couple hours. Enjoy!

On Thu 26 Jan 2012 04:06:14 PM CET, Ben Lerner wrote:

Yep, I'm familiar with it. And I have a test profile set up, too. I
just recall some frustration during the Firefox 3.6->4.0 transition
where I had to keep reinstalling both versions, rather than just
unpacking one of them into a new directory. If I can just unpack your
zip, without needing to install it (and thereby confuse Windows), then
I'm good to go.

On 1/26/2012 10:02 AM, Jonathan Protzenko wrote:

https://developer.mozilla.org/en/Setting_up_extension_development_environment
contains all the information you need to get started. All sections are
important, especially the part where you create a new profile and learn
to use it for development only, so that you can keep the two versions
side by side. The dev build I'm offering to build for you will be a big
zip file that you'll have to unpack somewhere.

On Thu 26 Jan 2012 04:01:09 PM CET, Ben Lerner wrote:

My only concern is that I'd need a side-by-side installation of two TB
versions. I mean, I doubt dropping a TB11-or-so custom build into a TB9
install directory will work...

On 1/26/2012 9:53 AM, Jonathan Protzenko wrote:

Alright, it's just that it's not super-hard for me to generate a build
with the patch applied, you could then play with it, and at least not
have to setup a build environment. Just edit the .js files with, as
long as you know the right flags, don't require you to recompile
Thunderbird, just restart it...

On Thu 26 Jan 2012 03:49:59 PM CET, Ben Lerner wrote:

It might, but at the moment I don't have a dedicated dev box, just my
all-purpose laptop, which is running TB9.0.1 on Windows. It'll take me
a bit of time to set up a VM where I could test other versions -- I had
a bit of a lull in schoolwork, so I could get back to this patch. Now
the lull is over ;-) What I need to do is trace through the code paths
we figured out earlier in this thread, remind myself how back/forward
history actually works, and then I'll know better whether you need to
make other APIs scriptable or not...

On 1/26/2012 9:31 AM, Jonathan Protzenko wrote:

If I create a build for you that contains my patch, will it help you figure out the remaining parts?


Reply to this email directly or view it on GitHub:
#393 (comment)


Reply to this email directly or view it on GitHub:

#393 (comment)

Reply to this email directly or view it on GitHub:
#393 (comment)


Reply to this email directly or view it on GitHub:

#393 (comment)

Reply to this email directly or view it on GitHub:
#393 (comment)


Reply to this email directly or view it on GitHub:
#393 (comment)

@protz
Copy link
Collaborator

protz commented Jan 27, 2012

So the test build didn't succeed but hopefully tomorrow's nightly should have the changed baked in.

ftp://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-central/

@klint
Copy link

klint commented Oct 28, 2013

Hello
Any news on that issue? Still here in TBC 2.7 unfortunately. The buttons seem to work actually, but browsed messages are not recorded in the history (so there is nothing for the buttons to work for).
Thanks

@protz
Copy link
Collaborator

protz commented Nov 10, 2013

Here's a summary of the situation.

  • The patch I mentioned a few messages earlier has been committed on Thunderbird, so it should be relatively easy to get an 80% patch, that is, a patch that is imperfect but somehow enables navigation .
  • The plugin Ben wrote a posted a few messages before could thus be simplified and reworked to work even better.
  • We still need to determine whether more functions need to be opened up to addons to get the corner cases working (end of the history, viewing a message from another folder).

There's probably quite a few corner cases but I'd certainly be willing to add even an imperfect patch. However, the story remains the same: busy with the PhD and little time / incentive to hack on this particular bug since I'm not using the back/forward feature myself...

@protz protz changed the title Back/forward buttons do not work Back/forward buttons do not work (semi-working patch in there!) Mar 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants