595654 - MultiMessageSummary and ThreadSummary should ensure multimessageview.xhtml is loaded before proceeding

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Jonathan Protzenko [:protz]]

So I was working on "Thunderbird Conversations" the other day, and it turned out I had broken the MultiMessageSummary. Actually, I just realized that MultiMessageSummary assumes that the right URL is loaded in the multimessage pane, and proceeds without even checking that multimessageview.xhtml is actually loaded.

What I did was make a monkey-patch on top of the summarizeMultipleSelection function that first restores the right url before handing control flow over the original summarizeMultipleSelection once multimessageview.xhtml is loaded.

I think the original implementations of summarizeThread and summarizeMultipleSelection should take care of this. I don't think this is just a case of "patch Thunderbird to suit my needs", because I'm pretty sure the extension that does account summary and folder summary has to workaround this too, and this might simplify its life if it could just assume that whenever someones actually needs to display something in the multimessage pane, then that someone takes care of having the right URL loaded.

davida, mixedpuppy, thoughts?

(I have a patch waiting somewhere so if you think that's a good idea, I'll just assign to me and submit a patch as soon as I have a decent development environment and work machine).

Comment 1

[Blake Winton (:bwinton) (:☕️)]

As the nominal owner of the Message Reader sub-module, this proposal makes sense to me, and I would love to see the patch when you get a chance to post it.

Thanks,
Blake.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Lets see a patch, I'll look at how it affects the addons and whether there is something that would help it work for them.

Comment 3

[Jim Porter (:squib)]

Hm, I bet this is really the "SummaryFrameManager" from bug 492158. Protz and I have discussed this recently, and I'm guessing that's the sort of thing he means in comment 0. Assuming that's the case, then I definitely agree that this would be nice.

The first step would probably be to see if the SummaryFrameManager can be used to make both Mail Summaries and Conversations happy.

Comment 4

[Jonathan Protzenko [:protz]]

My current stance on this is: as soon as you land the SummaryFrameManager (maybe give me a heads up a couple days before you do), I'll make sure Conversations is compatible with it, and submit patches if it doesn't expose the required functionality. I'm all for landing the SummaryFrameManager in Thunderbird :-).

Comment 5

[Jim Porter (:squib)]

Attachment: Add the SummaryFrameManager

Here's a patch to do this. I also set the nsMsgDBView to trigger a selection change event when entering a folder so that the summary code gets called then. This is necessary for the folder summary to load properly (previously, I used an annoying hack to make it happen in the add-on).

I've verified that this works with and without (a modified version of) the Mail Summaries add-on.

Comment 6

[Jim Porter (:squib)]

Attachment: Add missing file

Hmm, it would help if I added the necessary files to the patch...

Comment 7

[Jonathan Protzenko [:protz]]

Comment on attachment 575115 [details] [diff] [review]
Add missing file

Review of attachment 575115 [details] [diff] [review]:
-----------------------------------------------------------------

So the patch looks fine. I just gave it a try, and it works for me with a few minor changes in my code (that's cool). However, there's a few minor details that I think are worth ironing out.
- I'd like to be notified if the SummaryFrameManager had to actually change the URL before calling me. That's minor, however, and I can easily detect that myself by checking what is the current URL before calling loadAndCallback so I'm not sure it should be part of the API. What do you think?
- Do you think we should cancel a request to load something in the multimessage if the message pane is hidden, or is that something that we expect the clients to check for themselves? If I call window.gSummaryFrameManager.loadAndCallback with the multimessage hidden, it will un-hide it and confuse the command (need to hit F8 twice).

Apart from that, the patch looks fine enough. I still do think we should have a couple tests for that, though, esp. the thing in nsMsgDBView.cpp which, I suppose, allows you to show the Folder Summary easily when a folder is selected. So, r=me with the nits above fixed, and a proper test (even if it's a very small one, I wouldn't want this to regress).

::: mail/base/modules/summaryFrameManager.js
@@ +49,5 @@
> + * @param aFrame the iframe that we're managing
> + */
> +function SummaryFrameManager(aFrame) {
> +  this.iframe = aFrame;
> +  this.iframe.addEventListener("DOMContentLoaded", this._onLoad.bind(this),

So I know there's a reason why we need DOMContentLoaded and not "loaded", but I was unable to find it off the top of my head. Can you add a comment?

@@ +70,5 @@
> +  loadAndCallback: function(aUrl, aCallback) {
> +    this.url = aUrl;
> +    if (this.iframe.contentDocument.location.href != aUrl) {
> +      // We're changing the document -- so we need to set the src attribute,
> +      // and stash the callback that we want to call when it's done loading

This comment is in complete contradiction with what's happening in the code. You're not changing the src attribute.

@@ +99,5 @@
> +    try {
> +      if (!event.originalTarget.location ||
> +          event.originalTarget.location.href != this.url)
> +        return;
> +

Is it to catch other (unrelated) DOMContentLoaded events bubbling up the tree? If so, a comment would be appropriate I think :).

Comment 8

[Jim Porter (:squib)]

(In reply to Jonathan Protzenko [:protz] from comment #7)
> Comment on attachment 575115 [details] [diff] [review] [diff] [details] [review]
> Add missing file
> 
> Review of attachment 575115 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> So the patch looks fine. I just gave it a try, and it works for me with a
> few minor changes in my code (that's cool). However, there's a few minor
> details that I think are worth ironing out.
> - I'd like to be notified if the SummaryFrameManager had to actually change
> the URL before calling me. That's minor, however, and I can easily detect
> that myself by checking what is the current URL before calling
> loadAndCallback so I'm not sure it should be part of the API. What do you
> think?

That seems reasonable. We could just pass it as a parameter to the callback.

> - Do you think we should cancel a request to load something in the
> multimessage if the message pane is hidden, or is that something that we
> expect the clients to check for themselves? If I call
> window.gSummaryFrameManager.loadAndCallback with the multimessage hidden, it
> will un-hide it and confuse the command (need to hit F8 twice).

I don't see this when I try it. Right now, we need to load the multimessage page even when it's hidden so that the right thing is loaded when you unhide it. We could probably come up with a way to load the right page when unhiding too, but that would be more complicated.

> Apart from that, the patch looks fine enough. I still do think we should
> have a couple tests for that, though, esp. the thing in nsMsgDBView.cpp
> which, I suppose, allows you to show the Folder Summary easily when a folder
> is selected. So, r=me with the nits above fixed, and a proper test (even if
> it's a very small one, I wouldn't want this to regress).

Do you have any idea of how to test this without actually having the Folder Summary in the Thunderbird tree?

Comment 9

[Jim Porter (:squib)]

Hm, it looks like the nsMsgDBView.cpp part breaks the folder summary for virtual folders. Since that's only needed for the folder summary, I'll probably wait on that bit and handle it in bug 492158. Then I can hopefully get help from some folder guru to figure out the best way of doing that. :)

Comment 10

[Jim Porter (:squib)]

Attachment: Updated patch

Just asking for review again to make sure the "did the document change" argument to the callback will work for Conversations.

Comment 11

[Jonathan Protzenko [:protz]]

Comment on attachment 576384 [details] [diff] [review]
Updated patch

I just tried this and it works fine.

Sorry for the delay, I tend to be real busy these days, and I usually have to wait until the week-end for reviews.

Comment 12

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/f1cec15376be

Existing tests should cover this, but be on the lookout if anything bad happens. I've run with this patch for a week or so now and things look good here.