Naoki Hirata :nhirata (please use needinfo instead of cc) Reporter
	Description • 12 years ago

1. have 18000+ emails (ie subscribe to a bunch of bugzilla stuff) and have it filtered to a folder
2. highlight all the emails by doing a cmd-a (on the mac)

Expected: thunderbird won't lock up or give warning messages
Actual: warning message keeps appearing... the performance is horrible.

	Wayne Mery (:wsmwk)
	Comment 1 • 12 years ago

my moz local folder of 35k messages just took 30+ seconds. .msf file is 20mb. 

Memory increased about 140MB doing select all. no unresponsive script, but window did go "(not responding)".

	Kent James (:rkent)
	Comment 2 • 12 years ago

I've also never been a fan of the messages summaries that were added in TB 3, and AFAIK are the cause of the poor performance. When I am selecting a lot of messages, it is almost always because I want to select messages prior to some operation, not because I want summary information on them, or a condensed view of their content.

Wayne, how common is this complaint? Are there fans of this feature?

I believe there is a preference to turn off the message summaries, but it is not enough of an issue for me to actually try to figure that out.

	Wayne Mery (:wsmwk)
	Comment 3 • 12 years ago

(In reply to Kent James (:rkent) from comment #2)
> I've also never been a fan of the messages summaries that were added in TB
> 3, and AFAIK are the cause of the poor performance. 
Summaries are not the only cause. I have it disabled.

> When I am selecting a
> lot of messages, it is almost always because I want to select messages prior
> to some operation, not because I want summary information on them, or a
> condensed view of their content.
indeed. There might be a sanity check that I or someone else filed about that. But I couldn't find one after a quick check.

> Wayne, how common is this complaint? 
Not very. And more among techies I think than common folk.

> Are there fans of this feature?
I think so. I don't hear of many people turning it off. But, it's not exposed in the UI, so you'd have to know the hidden pref.

> I believe there is a preference to turn off the message summaries, 
mail.operate_on_msgs_in_collapsed_threads

	:aceman
	Comment 4 • 12 years ago

OK, let's make some simple UI and decouple summary display from the mail.operate_on_msgs_in_collapsed_threads pref (as that still has other effects when enabled). I propose a separate pref.

	:aceman
	Comment 5 • 12 years ago

So this puts a new button on the summary page to hide the summary as needed.
Notice if you just select a msg thread the summary is still shown. That one is controlled by the old pref.
There are still some edge cases like when bth mail.show_multiple_message_summaries and mail.operate_on_msgs_in_collapsed_threads are disabled, the summary page is no longer shown and you can't toggle at least summaries back on.
This would need to kill the logic around summarizeSelectionInFolder() in messageDisplay.js. Maybe it is fine, when the user disabled the thread pref manually in config, he does not want to enable it so easily.

However, with the patch applied I can select 600 000 messages in my test folder just fine (there are only a few seconds of CPU work to render the highlight).

	:aceman
	Comment 6 • 12 years ago

There are still some rough edges, like the button being a bit smaller in height than the Delete button and that the summary does not appear immediately after it is enabled again (but only after new selection). But I'd like to hear opinions on which things to polish and which are not worth pursuing more.

	Blake Winton (:bwinton) (:☕️)
	Comment 7 • 11 years ago

Comment on attachment 720909 [details] [diff] [review]
WIP patch

I guess my main problem with this is that it won't solve the original poster's complaint, since by the time they would see the button to turn off the message summaries, we're already in the middle of generating the message summaries, and have locked up their computer.  ;)

Splitting the pref into operating on multiple message and displaying summaries seems reasonable, though.  I just think that a button on the summary screen isn't the right way to expose it…

Thanks,
Blake.

	:aceman
	Comment 8 • 11 years ago

The reasoning is that they can toggle the button much sooner, e.g. when operating normally with TB and selecting just several messages. Then they wont be hit when they accidentally select a lot.

So what is the right way?

	Blake Winton (:bwinton) (:☕️)
	Comment 9 • 11 years ago

I doubt people would remember to do that in time.

What about auto-disabling it when someone selects more than, say, 1000 messages, and letting them show the summary if they _really_ want to?

	:aceman
	Comment 10 • 11 years ago

(In reply to Blake Winton (:bwinton) from comment #9)
> I doubt people would remember to do that in time.
They would do it preventively, because some of them do not want the summary ever. So we are just waiting for some solution like this to land so we can disable it permanently :)

> 
> What about auto-disabling it when someone selects more than, say, 1000
> messages, and letting them show the summary if they _really_ want to?

Yes, I would be OK with this. The current summary already does this partly, stops at 100 discussions. But it may have to go through many messages to find out when it has collected those.

But for this to work I need to refresh the message pane somehow. I'll see if I can do that.

	Jim Porter (:squib) Assignee
	Comment 11 • 11 years ago

I mentioned this on IRC, but for posterity and so :bwinton can comment on it, here's what I think we should do (and I say this as someone with experience doing just this for Mail Summaries):

We should truncate the list of messages we examine as early as possible. Even though we only show 100 messages in the summary, we group *all* the selected messages into threads. That's a huge waste, and all we get out of it is a single number at the top: "N conversations". It would be far better to simply examine the first 1000* selected messages, and if we selected more than that, put "At least N conversations" in the header instead. The number of times someone is selecting 10k+ messages and wants to know *exactly* how many threads are in that selection is vanishingly small, and we shouldn't bother calculating it.

This solution has the benefit of not adding any extra UI, and matches what I do with the folder summary in Mail Summaries. In fact, I perform a very similar operation on message threads. Since I only examine the 1000 newest** messages in a folder, the summary loads in about a second (and that's including all the other stuff Thunderbird does to open a folder). Of course, this isn't perfectly accurate, but it doesn't matter, since the information it provides is 1) pretty close, and 2) fast.

* Feel free to argue about the exact number.
** Newest according to when they were added to the message store.

	:aceman
	Comment 12 • 11 years ago

(In reply to Jim Porter (:squib) from comment #11)
> We should truncate the list of messages we examine as early as possible.
> Even though we only show 100 messages in the summary, we group *all* the
> selected messages into threads. That's a huge waste, and all we get out of
> it is a single number at the top: "N conversations". It would be far better
> to simply examine the first 1000* selected messages, and if we selected more
> than that, put "At least N conversations" in the header instead. The number
> of times someone is selecting 10k+ messages and wants to know *exactly* how
> many threads are in that selection is vanishingly small, and we shouldn't
> bother calculating it.

Thanks, I am going to do this. But even when showing only 100 msgs and analysing 1000 for thread count, it takes about 20 seconds (after some optimizations we came up with squib) if I select 139000 msgs (on 3.6Ghz Phenom II). So I still want to keep a pref that disables the summary entirely (threaded summary has a separate pref). But there will be no UI to toggle it.

For ~18000 messages the display should become fast enough with or without a summary.

	matteo sisti sette
	Comment 13 • 11 years ago

>  But even when showing only 100 msgs and analysing 1000 for thread count, 
> it takes about 20 seconds (after some optimizations we came up with squib)
> if I select 139000 msgs (on 3.6Ghz Phenom II). So I still want to keep
> a pref that disables the summary entirely (threaded summary has a separate
> pref). But there will be no UI to toggle it.

Not an acceptable solution at all. Suppose one day a user goes and selects 800,000 messages. Is he/she supposed to know that he/she should change a pref before? and even worse, one that can't be found in the UI?????

It must be 100% guaranteed that never, under any circumstances, the program freeazes for 20 seconds while doing a trivial  thing (from the point of view of the user) such as selecting messages. (Well, actually not even when doing a not-at-all-trivial thing, unless you issue a warning of the kind "This operation may take several minutes. Continue? [don't show this again]")

A user chooses to enable/disable the summary according to aesthetic or comfort, not in order to avoid software crashes. 

I am happy with disabling summaries because they are useless to me, but (1) i am not supposed to know that I need to disable them before doing a massive movement otherwise a disaster will happen (ESPECIALLY considered that I don't even know about there existence until I select a bunch of messages), and (2) what if one _does_ like message summaries and wants them enabled? should he remember to disable and reenable them before and after a single massive movement of messages?

	:aceman
	Comment 14 • 11 years ago

(In reply to matteo sisti sette from comment #13)

> I am happy with disabling summaries because they are useless to me, but (1)
> i am not supposed to know that I need to disable them before doing a massive
> movement otherwise a disaster will happen (ESPECIALLY considered that I
> don't even know about there existence until I select a bunch of messages),
> and (2) what if one _does_ like message summaries and wants them enabled?
> should he remember to disable and reenable them before and after a single
> massive movement of messages?

I agree with this, therefore the patch will contain some optimizations (e.g. the limiting to 1000 messages) to keep the time low even if summaries are enabled.

But I don't think selecting/working with 800 000 messages is a trivial thing any common user will do.

	matteo sisti sette
	Comment 15 • 11 years ago

You said 139,000 is enough to hang the program for 20 seconds. Right now I have 183,750 messages in a folder. I may well want to select all of them, then deselect the first 1000 or so, and move the selected ones into another foler. The actual moving is the only operation that I reasonably expect to take long.

Selecting N messages is (from the point of view of the user) exactly as trivial as selecting N-1 messages, for any N less than the number of messages that exist in a folder.
Any common user will select up to as many messages as he has in a given folder. And hey, what about the uncommon user?

By the way, if you'r only examining (in your tests, as far as I understand) the first 1000 for summary purposes, and the operation still takes long for a big number of selected messages, then where is it spending an O(N) time? Except for generating summaries, _selecting_ a bunch of adjacent messages should be O(1) (doing something with them will be the expensive part).

	:aceman
	Comment 16 • 11 years ago

(In reply to matteo sisti sette from comment #15)
> By the way, if you'r only examining (in your tests, as far as I understand)
> the first 1000 for summary purposes, and the operation still takes long for
> a big number of selected messages, then where is it spending an O(N) time?
> Except for generating summaries, _selecting_ a bunch of adjacent messages
> should be O(1) (doing something with them will be the expensive part).

The generating of the summary is slow, about 15 seconds out of those 20s.

I think even just drawing the row highlights for the selected messages takes some time, but I need to measure it more precisely.

I have a test folder of 1 million messages so I will also post some timings when the patch is polished enough.

	:aceman
	Comment 17 • 11 years ago

So with this patch I am down to 5 secs on 139 000 msgs. On 1 million msgs it is about 30 seconds. But the summary generating code is only about 3 secs out of that. My observation suggests that the rest is the selecting of the messages in the list widget (toolkit code).

	Jim Porter (:squib) Assignee
	Comment 18 • 11 years ago

Comment on attachment 745418 [details] [diff] [review]
WIP patch v2

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

Overall this looks good, and based on the performance numbers you got, this should be a huge improvement for large selections! f+ with my comments below addressed.

::: mail/base/content/messageDisplay.js
@@ +346,3 @@
>        .getChildHdrAt(0).messageKey;
>      let oneThread = true;
> +    for (let i = 0; i < selectedMessages.length; i++) {

Nit: start at i = 1 here, since you already got the thread id for the first message above.

::: mail/base/content/selectionsummaries.js
@@ +129,5 @@
>   */
>  
>  function MultiMessageSummary(aMessages, aListener) {
> +  // For performance reasons, we only process the first 1000 messages.
> +  this._msgsTruncated = aMessages.length > 1000;

Please make the 1000 a constant and put it somewhere convenient so we can change it later if we want to. Also, it would make more sense to set _msgsTruncated inside the for loop (see below).

@@ +136,5 @@
> +  for (let msgHdr in fixIterator(aMessages, Components.interfaces.nsIMsgDBHdr)) {
> +    this._msgHdrs.push(msgHdr);
> +    hdrs++;
> +    if (hdrs == 1000)
> +      break;

Here would be a better spot for setting _msgsTruncated.

@@ +216,5 @@
> +      messagesElt.removeChild(messagesElt.firstChild);
> +
> +    let heading = htmlpane.contentDocument.getElementById("heading");
> +
> +    let showSummary = Services.prefs.getBoolPref("mail.show_multiple_message_summaries");

If we got here, this should always be true, I think.

@@ +231,1 @@
>      {

Nit: this should be on the same line as the for loop.

@@ +524,5 @@
>  function ThreadSummary(aMessages, aListener)
>  {
> +  this._msgHdrs = [];
> +  for (let msgHdr in fixIterator(aMessages, Components.interfaces.nsIMsgDBHdr))
> +    this._msgHdrs.push(msgHdr);

We might want to do the truncating thing here just to be safe. I don't care strongly either way, though.

	Blake Winton (:bwinton) (:☕️)
	Comment 19 • 11 years ago

Comment on attachment 745418 [details] [diff] [review]
WIP patch v2

I'm getting some strange errors with my test data (Exception in summarizeMultipleSelection[Exception... "String contains an invalid character"  code: "5" nsresult: "0x80530005 (InvalidCharacterError)"  location: "chrome://messenger/content/selectionsummaries.js Line: 58"]), but in UI-terms I like it.  ui-r=me.

Thanks,
Blake.

	:aceman
	Comment 20 • 11 years ago

(In reply to Jim Porter (:squib) from comment #18)
> @@ +216,5 @@
> > +      messagesElt.removeChild(messagesElt.firstChild);
> > +
> > +    let heading = htmlpane.contentDocument.getElementById("heading");
> > +
> > +    let showSummary = Services.prefs.getBoolPref("mail.show_multiple_message_summaries");
> 
> If we got here, this should always be true, I think.

Not true. Try it out and you'll see the code runs here. We also set a special title in !showSummary case.

> @@ +524,5 @@
> >  function ThreadSummary(aMessages, aListener)
> >  {
> > +  this._msgHdrs = [];
> > +  for (let msgHdr in fixIterator(aMessages, Components.interfaces.nsIMsgDBHdr))
> > +    this._msgHdrs.push(msgHdr);
> 
> We might want to do the truncating thing here just to be safe. I don't care
> strongly either way, though.

OK, I can truncate here too. We actually need to define this.msgsTruncated too as common functions reference it.

	Jim Porter (:squib) Assignee
	Comment 22 • 11 years ago

Comment on attachment 745587 [details] [diff] [review]
patch v3

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

I haven't run with this yet, so leaving the review request active, but here are some comments.

::: mail/base/content/folderDisplay.js
@@ +209,5 @@
>     *     offline.
>     */
>    get summarizeSelectionInFolder() {
> +    return (Services.prefs.getBoolPref("mail.operate_on_msgs_in_collapsed_threads") ||
> +           Services.prefs.getBoolPref("mail.show_multiple_message_summaries")) &&

Ok, I see what you mean about how this pref works now. However, I don't think this is logic is useful. I think we should just have a pref that turns off the summaries *entirely* so that people can have mail.operate_on_msgs_in_collapsed_threads set to true but not to have summaries.

Since, according to your performance numbers, the summarization code should never take a long time anymore, I don't think this pref is very useful as it's currently implemented. However, I *do* think it would be useful to let people completely disable the summaries without affecting the behavior of operations on collapsed threads.

I'll let bwinton weigh in on this too, but in any case, we'll need to update the comments too.

::: mail/base/content/messageDisplay.js
@@ +353,5 @@
>          oneThread = false;
>          break;
>        }
>      }
>      if (!this.folderDisplay.summarizeSelectionInFolder) {

We should move this block earlier (before getting the selectedMessages array). That way, we're not fetching all the selected messages when we're not going to summarize things anyway.

::: mail/base/content/selectionsummaries.js
@@ +123,5 @@
> + *
> + * @param aMessages  nsIMutableArray of message headers.
> + * @param aSummary   The summary object to contain the message array.
> + */
> +function _truncate_message_array(aMessages, aSummary) {

I'd rather see this message return a pair of values: the truncated array, and a boolean if we actually did truncate it. Then we'd use it like so in MultiMessageSummary():

  [this._msgHdrs, this._msgsTruncated] = _truncate_message_array(aMessages);

Also a nit: We seem to use _mm_xyz() for private function names in this file. Let's do that here too.

::: mail/locales/en-US/chrome/messenger/multimessageview.properties
@@ +23,4 @@
>  noticeText= (Note: #1 messages are selected, the first #2 are shown)
> +approxNoticeText= (Note: More than #1 messages are selected, the first #2 are shown)
> +
> +noSummary=Multiple messages selected.

Nit: no period at the end here.

	:aceman
	Comment 23 • 11 years ago

Bwinton, as you already gave ui-r+ on the previous version can you comment on the new proposal by squib on what the effects of the pref should be?

(In reply to Jim Porter (:squib) from comment #22)
> Comment on attachment 745587 [details] [diff] [review]
> ::: mail/locales/en-US/chrome/messenger/multimessageview.properties
> @@ +23,4 @@
> >  noticeText= (Note: #1 messages are selected, the first #2 are shown)
> > +approxNoticeText= (Note: More than #1 messages are selected, the first #2 are shown)
> > +
> > +noSummary=Multiple messages selected.
> 
> Nit: no period at the end here.

I think if we implement your proposal we never get into the summary code to set this title. The message pane will be blank.

	Jim Porter (:squib) Assignee
	Comment 24 • 11 years ago

(In reply to :aceman from comment #23)
> I think if we implement your proposal we never get into the summary code to
> set this title. The message pane will be blank.

Yeah, if we go with my proposal, this won't matter.

	Blake Winton (:bwinton) (:☕️)
	Comment 25 • 11 years ago

Sorry, I haven't really been following the conversation.  What are the two proposals?

	:aceman
	Comment 26 • 11 years ago

Bwinton, start of comment 22.

	Blake Winton (:bwinton) (:☕️)
	Comment 27 • 11 years ago

Squib's proposal seems reasonable.  Let's go with that.

	Jim Porter (:squib) Assignee
	Comment 29 • 11 years ago

Comment on attachment 745587 [details] [diff] [review]
patch v3

Clearing out review, since bwinton agrees that we should change the pref logic (see comment 22 for my suggestion).

	Jim Porter (:squib) Assignee
	Comment 30 • 11 years ago

Ok, so I checked this out in the profiler (with some of my patches applied), and found that almost 70% of the time spent is actually in getIdentityForHeader(), which is called by folderDisplay.canArchiveSelectedMessages.

canArchiveSelectedMessages is called in two different places (one of them is the multi-message summary), which leads me to believe that the main thing that actually makes the patch here work is this change:

+    archiveBtn.collapsed = aForceDisableArchive || this._msgsTruncated ||
+                           !gFolderDisplay.canArchiveSelectedMessages;

Therefore, I think the most effective avenue to pursue here is to improve the performance of getIdentityForHeader (and maybe canArchiveSelectedMessages).

	Jim Porter (:squib) Assignee
	Comment 31 • 11 years ago

I have a patch in progress that helps a bunch for canArchiveSelectedMessages. I'm going to file a new bug on that.

That's not to say we should throw out the work in this patch! (Although unfortunately, I've probably bitrotted it pretty badly from some other bugs.) Even with the fix I'm working on, the summary code is pretty sluggish, and we should definitely improve that, especially since we only actually *show* the first 100 messages/threads. However, we'll probably be able to up the number of messages we process in the summaries (5000 might be a good number).

	Jim Porter (:squib) Assignee
	Comment 32 • 11 years ago

aceman: If you want, I can work on unbitrotting your patch and simplifying it a bit in light of my changes. Sorry again about bitrotting you so badly!

	:aceman
	Comment 33 • 11 years ago

Sure, no problem with that. Please make it better:)

	Jim Porter (:squib) Assignee
	Comment 34 • 11 years ago

Stealing this bug. :)

This patch isn't quite review-ready yet, since it depends on a few too many other patches of mine (bug 942638, bug 943116, and bug 975795). With everything combined, I went from taking ~5 seconds to around 0.5 seconds when selecting 22,000 messages.

	:aceman
	Comment 35 • 11 years ago

Please ask me for feedback when the patch is almost ready (and the dependecies checked in) so I can run it on my test folders:) Thanks.

	Jim Porter (:squib) Assignee
	Comment 36 • 10 years ago

Ok, this should apply to comm-central now, since I landed my multi-message summary rework.

	:aceman
	Comment 37 • 10 years ago

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

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

OK so I tried this a Phenom II 3.6Ghz, current trunk, with the preparatory patches in.
Without this patch:
18000 msgs - 6s
139000 msgs - 45s
1048000 msgs - 440s (7 minutes). Peak memory usage 2000 MB, dropping down to 860MB after summaries are calculated (but all messages still selected). Removing the selection does not lower it much. Also an error in Error console:
Error: arguments array passed to Function.prototype.apply is too large
Source file: resource://app/modules/gloda/gloda.js
Line: 337

With the patch:
18000 msgs - 3s
139000 msgs - 3s
1048000 - 24s or 11s depending whether it is first attempt or subsequent ones (probably caching).

At the 1 million testcase, memory usage (RES in top) increases from 590MB to 875MB while building the summary and stays there while all msgs are selected.
After removing the selection and choosing only one message, memory usage drop back to 590MB precisely. Is this usage caused by the selection list widget, or the summaries? Can this be improved any further?

Anyway, great improvement, thanks. Are these numbers satisfactory for us?

One last issue: while the summaries are calculated, for part of the time, the whole message view pane turns grey (as if disabled) with absolutely no content. That doesn't happen without the patch (it shows the previously selected message until summaries are ready). It looks ugly. Can you improve that?

	Jim Porter (:squib) Assignee
	Comment 38 • 10 years ago

(In reply to :aceman from comment #37)
> At the 1 million testcase, memory usage (RES in top) increases from 590MB to
> 875MB while building the summary and stays there while all msgs are selected.
> After removing the selection and choosing only one message, memory usage
> drop back to 590MB precisely. Is this usage caused by the selection list
> widget, or the summaries? Can this be improved any further?

I'd guess this is from the thread view, not the summary. You could try profiling this to see...

> One last issue: while the summaries are calculated, for part of the time,
> the whole message view pane turns grey (as if disabled) with absolutely no
> content. That doesn't happen without the patch (it shows the previously
> selected message until summaries are ready). It looks ugly. Can you improve
> that?

That's because I now reset the summary page to be about:blank whenever we stop summarizing. This is important because it unregisters all the event listeners that keep the summary up-to-date. It is a bit less pretty, but it's a lot more efficient. Do you see the grey pane when you're selecting small numbers of messages, or just for really large ones?

We could change about:blank to be white or something, but I'm not sure we could do much better than that without a lot of work.

	:aceman
	Comment 39 • 10 years ago

Does it have to be about:blank? Or can it be some placeholder text like "building summary..." ?

	Jim Porter (:squib) Assignee
	Comment 40 • 10 years ago

That pane *can* be used for pretty much anything, but yeah, in theory we could have it say that. Or maybe just a generic "loading" image.

	Jim Porter (:squib) Assignee
	Comment 41 • 10 years ago

Josiah: Thoughts on comment 38 and beyond? We could change about:blank to be white like Firefox, and that'd look a lot nicer...

	Josiah Bruner [:jsbruner]
	Comment 42 • 10 years ago

I'm surprised to find out about:blank isn't white. So yes, let's do that. In addition, we could probably use the spinner graphic with some "loading" text if that's doable just to indicate to the user something is happening.

	:aceman
	Comment 43 • 10 years ago

about:blank is also used in AccountManager.js but only for a really unexpected error state. There, it is better to have it grey (the color of the window frame). But white is fine too, as it is not expected to be shown often.

	Jim Porter (:squib) Assignee
	Comment 44 • 10 years ago

I think we should try to make about:blank white, and that's all. A loading throbber would be nice, but I don't want to enforce the idea that "nothing in the summary pane" means "it's loading", and trying to add a loading throbber right when we select the messages is already too late.

	Jim Porter (:squib) Assignee
	Comment 45 • 10 years ago

Does anyone know where I'd set the background for about:blank? It'd be nice to land this so that at least Daily users get the performance benefit.

	Wayne Mery (:wsmwk)
	Comment 46 • 10 years ago

(In reply to Jim Porter (:squib) from comment #45)
> Does anyone know where I'd set the background for about:blank? It'd be nice
> to land this so that at least Daily users get the performance benefit.

	Richard Marti (:Paenglab)
	Comment 48 • 10 years ago

I'm sorry I don't know how to set this. When the system theme is changed to a dark background, is about:blank still grey or now dark? Maybe the background is -moz-Dialog.

	Jim Porter (:squib) Assignee
	Comment 49 • 10 years ago

Ok, this fixes the background-color issue. I've been using this patch for several months on my local TB build with zero issues, so I think it's ready for a full review.

	Jim Porter (:squib) Assignee
	Comment 50 • 10 years ago

Ok, this is a bit smarter about summarizing multiple threads. It'll summarize at most 500 messages or 100 threads. That way, it'll be less likely that the summary is overly-brief if the first selected thread has a lot of messages.

	Josiah Bruner [:jsbruner]
	Comment 51 • 10 years ago

Comment on attachment 8482016 [details] [diff] [review]
Be smarter with limiting the multiple-thread summaries

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

Since I'm here, I'll steal Richard's review. Just one nit with the CSS.

::: mail/themes/linux/mail/mailWindow1.css
@@ +62,5 @@
>  }
>  
> +#multimessage {
> +  background-color: -moz-Field;
> +}

You're using this in all platforms. Please add a single rule in themes/shared/mail/mailWindow1.css instead of three separate files.

	Jim Porter (:squib) Assignee
	Comment 52 • 10 years ago

That's the main thing I wasn't sure about, thanks. I didn't know if the goal was to avoid putting color rules in the shared CSS.

	Josiah Bruner [:jsbruner]
	Comment 53 • 10 years ago

(In reply to Jim Porter (:squib) from comment #52)
> That's the main thing I wasn't sure about, thanks. I didn't know if the goal
> was to avoid putting color rules in the shared CSS.

The goal is to avoid only partially sharing rules. For example, if you had a #div with background-color: red; on all platforms, but padding: 2px; on Mac and padding: 3px; on Linux and Windows, we wouldn't share *any* of the rules, including the background-color.

If however, all rules are the same, we share the entire thing. That should make it so we don't have to search all over the place, but at the same time reduce duplication.

	Jim Porter (:squib) Assignee
	Comment 54 • 10 years ago

Ok, this fixes the CSS.

	:aceman
	Comment 55 • 10 years ago

Comment on attachment 8486026 [details] [diff] [review]
Fix the CSS

Looks good to me now, thanks. Not sure if mconley does reviews now for this area. You can try mkmelin.

	Jim Porter (:squib) Assignee
	Comment 56 • 10 years ago

(In reply to :aceman from comment #55)
> Comment on attachment 8486026 [details] [diff] [review]
> Fix the CSS
> 
> Looks good to me now, thanks. Not sure if mconley does reviews now for this
> area. You can try mkmelin.

Mike's done the past few reviews for my multi-message summary patches, so he's probably the person who knows this code the most at the moment aside from me.

	Mike Conley (:mconley) (:⚙️)
	Comment 57 • 10 years ago

Comment on attachment 8486026 [details] [diff] [review]
Fix the CSS

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

This makes sense to me - but I'm curious about that reduce thing you're doing... Please explain. :)

::: mail/base/content/multimessageview.js
@@ +62,5 @@
> +  /**
> +   * Iterate over the array until we hit the end or the maximum length,
> +   * whichever comes first.
> +   */
> +  "@@iterator": function*() {

TIL about @@iterator. Thanks. :)

@@ +368,5 @@
>     */
>    _computeSize: function(aMessages) {
>      let numBytes = 0;
> +    for (let msgHdr of aMessages)
> +      numBytes += msgHdr.messageSize; // XXX do something about news?

So _do_ we need to do something about newsgroup messages here?

@@ +510,4 @@
>      let maxCountExceeded = false;
> +    for (let [i, msgHdr] in Iterator(summarizedMessages)) {
> +      if (i > this.kMaxSummarizedMessages) {
> +        summarizedMessages.length = i;

I'm learning lots about Arrays today - I didn't realize one could truncate by writing to length. Neat trick. :)

@@ +634,3 @@
>        ]));
> +
> +      // Return only the messages for the threads we're actually showing.

Can you go into more detail about what exactly this reduce function is doing? If I didn't know better, I'd say this was dumping the contents of one array into another... am I wrong on that?

	Jim Porter (:squib) Assignee
	Comment 58 • 10 years ago

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #57)
> Comment on attachment 8486026 [details] [diff] [review]
> Fix the CSS
> 
> Review of attachment 8486026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This makes sense to me - but I'm curious about that reduce thing you're
> doing... Please explain. :)
> 
> ::: mail/base/content/multimessageview.js
> @@ +62,5 @@
> > +  /**
> > +   * Iterate over the array until we hit the end or the maximum length,
> > +   * whichever comes first.
> > +   */
> > +  "@@iterator": function*() {
> 
> TIL about @@iterator. Thanks. :)
> 
> @@ +368,5 @@
> >     */
> >    _computeSize: function(aMessages) {
> >      let numBytes = 0;
> > +    for (let msgHdr of aMessages)
> > +      numBytes += msgHdr.messageSize; // XXX do something about news?
> 
> So _do_ we need to do something about newsgroup messages here?

Maybe one day, if we ever get support for showing the multi-message summary for news, but that requires us to be able to index news posts with gloda, and *that* requires us fixing some NNTP bugs. If you're particularly interested, jcranmer probably knows a bit more about what we need for that.

> @@ +634,3 @@
> >        ]));
> > +
> > +      // Return only the messages for the threads we're actually showing.
> 
> Can you go into more detail about what exactly this reduce function is
> doing? If I didn't know better, I'd say this was dumping the contents of one
> array into another... am I wrong on that?

It's flattening an array-of-arrays into a mere array. I'll update the comment to make it clearer that that's what's happening here.

	Jim Porter (:squib) Assignee
	Comment 59 • 10 years ago

Landed with a minor change to how "@@iterator" is defined (it's Symbol.iterator now because apparently we can't just keep a single function name for this*).

https://hg.mozilla.org/comm-central/rev/ee0f0e6b36b8

* Before "@@iterator" it was __iterator__.

	:aceman
	Comment 60 • 10 years ago

Assuming this landed in TB36, if not 35.

	Onno Ekker [:nONoNonO UTC+1]
	Comment 61 • 9 years ago

Ever since this bug was fixed there is a regression in my add-on, where I can't seem to add a button to the multimessage header pane anymore. Unfortunately I don't understand why it doesn't work anymore. Could someone please help me with my issue?
https://github.com/mailredirect/mailredirect/blob/master/chrome/content/mailredirect.js#L224

	Jim Porter (:squib) Assignee
	Comment 62 • 9 years ago

I think I figured it out. You're probably getting hit by this line of the diff: <http://hg.mozilla.org/comm-central/rev/ee0f0e6b36b8#l1.19>. That clears out the summary frame when it's hidden. If that's the case, your add-on was already incompatible with Mail Summaries pre-38, since it clears out the multimessageview.xhtml when the folder summary is shown.

Probably the easiest fix would be to override some of the functions here: <http://mxr.mozilla.org/comm-central/source/mail/base/content/selectionsummaries.js>. Those functions are specifically meant for add-ons to override.

(Sorry for not seeing this earlier; it's usually easiest to get a hold of me if you needinfo me on a bug. I don't usually pay close attention to my bugmail, unfortunately.)