The multi-message summary code is a bit of a mess. I'm going to clean it up to make it easier to work on and to modify via add-ons. My overall goals are: 1) Do some basic cleanup on the code in selectionsummaries.js to help make it more readable 2) Modify messageDisplay.js to have less specific knowledge of how summaries work 3) Move most of the code in selectionsummaries.js into a new file that runs in the context of multimessagesummary.xhtml

This is mainly just style fixes. I also removed some irrelevant functions like _mm_addClass and _mm_removeClass, which did precisely nothing that classList.add and classList.remove don't already do.

This patch moves some of the summarization logic to selectionsummaries.js and also treats the mail start page as a folder-level "summary". The latter part is especially nice for my Mail Summaries add-on because it means I have to do quite a bit less work to hook up my folder summary.

Comment on attachment 8337464 [details] [diff] [review] Part 2: Make messageDisplay.js know less about summaries protz: Do the changes here in MessageDisplayWidget._showSummary help/hurt Conversations?

Oh, I guess I should explain some of the reasons why this bug actually matters. The big one, aside from just making this code more readable/maintainable, is that this is the first of many steps to land Mail Summaries in Thunderbird proper. A lot of the changes I plan on making in this bug are things that will help make the thread summaries work a little more like the folder summaries so that landing the actual folder summaries code is easier. I can't guarantee that we'll get Mail Summaries in TB for 31, but at the very least, the Mail Summaries add-on will be much simpler, and won't use quite so many ugly monkeypatches.

Hi Jim, Thanks for the ping! The first patch doesn't impact me since, as far as I can tell, it's mostly about cleaning up selectionsummaries.js. The second patch, however, impacts me quite badly. I'm currently hooking up my Conversation display by short-circuiting the `summarizeThread` function. This allows me to just monkey-patch the case where there's just one thread to be summarized (meaning, conversation view). See https://github.com/protz/GMail-Conversation-View/blob/master/modules/monkeypatch.js#L453 for details. Your patch seems to remove that function, meaning that I'd have to either replace the ThreadSummary function, or monkey-patch summarizeSelection and replicate the logic determining whether there's a single thread or not (I think that would be worse). I don't know if I can monkey-patch ThreadSummary... Do you have builds that I could try out? Thanks again for notifying me in advance!

Hm, if I keep summarizeThread and summarizeMultipleSelection around, would that be sufficient for you? My next step is actually to move MultiMessageSummary and ThreadSummary to a separate file that's loaded in multimessagewindow.xhtml instead of in messenger.xul, which would make it considerably harder to monkey-patch ThreadSummary. It shouldn't be much trouble to keep summarizeThread and summarizeMultipleSelection and just have them delegate to the new JS file mentioned above. I'm not sure if there are other things you use from selectionsummaries.js, (e.g. _mm_FormatDisplayName), but I do plan on moving that to a separate module you can import.

Yes, keeping summarizeThread as a separate function would be excellent, as it would allow you to move ThreadSummary to a separate file and would allow me to keep my monkey-patching point easily within reach. I'm not using anything from selectionsummaries.js ; since I'm loading my own file into the multimessage pane, you have total freedom here. Thanks again!

protz: I may be making some changes to the API for summarizeThread and summarizeMultipleSelection to help with performance (see bug 778907 and bug 943116). I'm not sure of the exact details yet, but I wanted to give you an early heads-up. I'll be sure to keep you updated as I know more, and I'll do my best to keep the changes from being too obnoxious. :)

As long as you keep me CC'd, I'm totally fine with it :). I don't try to maintain backwards compatibility for Conversations : I issue new development builds whenever I fix some breakage, and people usually know where to find them.

Isn't this making bug 778907 obsolete in some way?

Comment on attachment 8337462 [details] [diff] [review] Part 1: Clean up selectionsummaries.js Review of attachment 8337462 [details] [diff] [review]: ----------------------------------------------------------------- Oh my - there's quite a few other things in selectionsummaries.js that I think could be cleaned up / re-done a bit better... But in the interests of not scope creeping, and landing a thing that puts us in a better state, I'm cool to r+ this, if you change over the for loops I highlighted to use the more straight-forward for of pattern. ::: mail/base/content/selectionsummaries.js @@ -37,5 @@ > gSelectionSummaryStrings[name] = typeof value == "string" ? > getStr(value) : value.map(gSelectionSummaryStrings); > } > > -// Ah, wouldn't it be nice if there was platform code to do the following... The future is now! @@ +330,5 @@ > _addTagNodes: function(msgs, tagsNode) { > + // For tags, stars, and read/unread status, we want to map from all > + // messages to one node. > + let tags = {}; > + for (let [, msgHdr] in Iterator(msgs)) { Both msgs and msgTags are just vanilla Arrays here, so we can just use: for (let msgHdr of msgs) { // ... for (let tag of msgTags) { // ... } } Same with the other loop below. @@ +433,4 @@ > tagsNode); > } > > + for (let [,headerNode] in Iterator(knownMessageNodes)) { for (let headerNode of knownMessageNodes) @@ +567,5 @@ > snippetNode.textContent = "..."; > } > let tagsNode = msgNode.querySelector(".tags"); > let tags = this.getTagsForMsg(msgHdr); > + for (let [,tag] in Iterator(tags)) { for (let tag of tags) @@ +570,5 @@ > let tags = this.getTagsForMsg(msgHdr); > + for (let [,tag] in Iterator(tags)) { > + let tagNode = tagsNode.ownerDocument.createElement("span"); > + // See tagColors.css. > + let colorClass = "blc-" + this._msgTagService.getColorForKey(tag.key) I think this could be made more readable as: let colorClass = "blc-" + this._msgTagService .getColorForKey(tag.key) .substr(1); @@ -708,5 @@ > - * @param s > - * input text > - * @return The string with <, >, and & replaced by the corresponding entities. > - */ > -function escapeXMLchars(s) Good riddance!

Comment on attachment 8337464 [details] [diff] [review] Part 2: Make messageDisplay.js know less about summaries Review of attachment 8337464 [details] [diff] [review]: ----------------------------------------------------------------- I think this is OK, but again, this is my first foray into summaries code, so... take that feedback with a grain of salt. ::: mail/base/content/selectionsummaries.js @@ +641,5 @@ > + let folderDisplay = messageDisplay.folderDisplay; > + let selectedMessages = folderDisplay.selectedMessages; > + let selectedIndices = folderDisplay.selectedIndices; > + let dbView = folderDisplay.view.dbView; > + let firstThreadId = dbView.getThreadContainingIndex(selectedIndices[0]) This is my first real foray into message summary code, but isn't it possible that there were no selectedIndices, and that selectedIndices[0] will be undefined? Is that a thing to be worried about? @@ +642,5 @@ > + let selectedMessages = folderDisplay.selectedMessages; > + let selectedIndices = folderDisplay.selectedIndices; > + let dbView = folderDisplay.view.dbView; > + let firstThreadId = dbView.getThreadContainingIndex(selectedIndices[0]) > + .getChildHdrAt(0).messageKey; Nit - I think I'd prefer this way of breaking things up: let firstThreadId = dbView.getThreadContainingIndex(selectedIndices[0]) .getChildHdrAt(0) .messageKey; @@ +644,5 @@ > + let dbView = folderDisplay.view.dbView; > + let firstThreadId = dbView.getThreadContainingIndex(selectedIndices[0]) > + .getChildHdrAt(0).messageKey; > + > + let oneThread = true; I think I'd personally prefer: let oneThread = selectedIndices.every(function(aSelectedIndex) { return threadId == dbView.getThreadContainingIndex(aSelectedIndex) .getChildHdrAt(0) .messageKey; }); @@ +646,5 @@ > + .getChildHdrAt(0).messageKey; > + > + let oneThread = true; > + for (let i = 0; i < selectedIndices.length; i++) { > + let threadId = dbView.getThreadContainingIndex(selectedIndices[i]) Same nit as above for the breaking up between .getChildHdrAt(0) and .messageKey

(In reply to Mike Conley (:mconley) from comment #11) > Oh my - there's quite a few other things in selectionsummaries.js that I > think could be cleaned up / re-done a bit better... Yeah, this is by no means all of what I plan to do here! :) The two patches I've uploaded are just there to get the framework in a slightly better place and to simplify the review process, since part 3 is going to involve moving a lot of stuff to new files. Subsequent parts will clean up the logic a lot more. There are a lot of lessons I've learned in working on Mail Summaries that I hope to apply to the message summaries in this bug.

Sorry for the huge patch, but it's mostly just moving things from one file to another! :)

This patch cleans up some CSS and unused functions, and also make a single helper function for creating message/thread summary items. This way, there's not quite so much duplicated code.

This patch makes MultiMessageSummary into a singleton that sticks around forever, and which you can call summarize() on repeatedly to summarize different selections. In theory, you could register different kinds of summarizers using this, although I don't suspect we'll use that much; I just thought this would be the clearest way to separate things.

Here's a simple one. :) This just improves the localization code so that it's easier to follow and adds comments to help out localizers.

And another simple one. This just moves some utility functions to a separate file to make it easier for the summaries (both TB's multi-message summary and Mail Summaries' folder summary) to format display names. Sorry for putting you down as the reviewer for all of these! :( I'm not sure how easy it would be to have other folks look at it, since most of these patches depend pretty heavily on the previous one in the queue. Feel free to do these as slow or as quickly as you like!

Here's a diff of selectionsummaries.js as of part 2 and multimessageview.js as of part 3. This should make it a little clearer what the actual changes are.

Comment on attachment 8360855 [details] [diff] [review] Part 3 v1: Move summary-specific code to multimessageview.js Review of attachment 8360855 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/jar.mn @@ +88,5 @@ > content/messenger/multimessageview_print.css (content/multimessageview_print.css) > content/messenger/sharedsummary.css (content/sharedsummary.css) > content/messenger/multimessageview.xhtml (content/multimessageview.xhtml) > + content/messenger/multimessageview.js (content/multimessageview.js) > + Whoops, an extra newline snuck in! This is fixed in my local version of the patch.

Try build for parts 1 and 2 (v2): https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=e831e4d2bee5 Assuming the results of the try run look good, I'll land those two parts soon!

Parts 1 and 2 landed: https://hg.mozilla.org/comm-central/rev/f29c718dd3d8 https://hg.mozilla.org/comm-central/rev/9d8c31ac7972

Comment on attachment 8360855 [details] [diff] [review] Part 3 v1: Move summary-specific code to multimessageview.js Review of attachment 8360855 [details] [diff] [review]: ----------------------------------------------------------------- Yes, this is much better. Thanks for tackling this! And sorry for taking forever to get through these. ::: mail/base/content/selectionsummaries.js @@ +44,5 @@ > * @param aMessageDisplay The MessageDisplayWidget object responsible for > * showing messages. > */ > function summarizeThread(aSelectedMessages, aMessageDisplay) { > + const summaryURL = "chrome://messenger/content/multimessageview.xhtml"; consts should start with a k, like kSummaryURL, or be in ALL CAPS. @@ +67,5 @@ > * @param aMessageDisplay The MessageDisplayWidget object responsible for > * showing messages. > */ > function summarizeMultipleSelection(aSelectedMessages, aMessageDisplay) { > + const summaryURL = "chrome://messenger/content/multimessageview.xhtml"; Same as above, re: consts.

(In reply to Mike Conley (:mconley) from comment #25) > Yes, this is much better. Thanks for tackling this! And sorry for taking > forever to get through these. No worries! They're big patches, and taken together, they rewrite nearly everything in the multi-message summary code. If we have time before TB31 after this lands, I have a few smaller things I intend to add to the summaries, like context menus, visual accessibility (read: using platform-derived colors), and keyboard accessibility. :)

Comment on attachment 8360856 [details] [diff] [review] Part 4: v1 Create a single function for making summary rows Review of attachment 8360856 [details] [diff] [review]: ----------------------------------------------------------------- Good to see the clean-up in here, and good to see some newer Harmony constructs coming in. :) I don't have much to complain about here. I can't say I'm the expert at the multimessageview code, but I don't see anything wrong with this - just some nits and suggestions (see below). ::: mail/base/content/multimessageview.js @@ +123,5 @@ > MultiMessageSummary.prototype = { > /** > + * The maximum number of messages to summarize. > + */ > + maxMessages: 100, If these are constants, perhaps we should call them kMaxMessages, kSnippetLength. If you want to make double-y sure they won't be accidentally overwritten, perhaps make them getters. @@ +141,3 @@ > */ > getTagsForMsg: function(aMsgHdr) { > + let keywords = new Set(aMsgHdr.getStringProperty("keywords").split(" ")); Yes, this is much better! Good to see the Harmony stuff coming in. @@ +236,2 @@ > */ > + _makeSummaryItem: function(messageOrThread, options) { Let's stick with the convention of prefixing function arguments with a, so aMessageOrThread, and aOptions. @@ +311,5 @@ > + } > + else { > + let dateNode = document.createElement("span"); > + dateNode.classList.add("date", "right"); > + dateNode.textContent = makeFriendlyDateAgo(new Date(message.date/1000)); Spaces on either side of the / please. @@ +329,5 @@ > + itemHeaderNode.appendChild(tagNode); > + > + let snippetNode = row.querySelector(".snippet"); > + try { > + const snippetLength = this.snippetLength; consts should be prefixed with k, or be in all caps. @@ +341,5 @@ > + snippetNode.textContent = text; > + if (meta.author) > + authorNode.textContent = meta.author; > + }, false, {saneBodySize: true}); > + } catch (e if e.result == Components.results.NS_ERROR_FAILURE) { Whoaaa - never seen this construct before. Very interesting. @@ +343,5 @@ > + authorNode.textContent = meta.author; > + }, false, {saneBodySize: true}); > + } catch (e if e.result == Components.results.NS_ERROR_FAILURE) { > + // Offline messages generate exceptions, which is unfortunate. When > + // that's fixed, this code should adapt. XXX Is there a bug for fixing that? If not, please file one - and perhaps either comment (or file a blocker) for adapting this code. @@ +358,5 @@ > + * @param aTagsNode The DOM node to contain the list of tags. > + */ > + _addTagNodes: function(aTags, aTagsNode) { > + // Make sure the tags are sorted in their natural order. > + let sortedTags = [x for (x of aTags)]; I believe the spread operator, like [...aTags] might be workable here instead. ::: mail/test/mozmill/shared-modules/test-folder-display-helpers.js @@ +2738,5 @@ > let htmlframe = mc.e('multimessage'); > + let matches = htmlframe.contentDocument.querySelectorAll(aSelector); > + if (matches.length != aNumElts) { > + throw new Error( > + "Expected to find " + aNumElts + " elements with selector '" + Please fix the alignment for the second half of the string.

Sorry for the bugspam! The review comments bitrotted the successor patches, so I've updated all of them.

Note to self: part 6 needs reuploaded.

I forgot to change a couple of references to the global window state, which broke opening individual messages.

De-bitrotting

De-bitrotting

De-bitrotting

Fix bitrot

Fix bitrot

Fix bitrot and improve interface of summarizers

Fix bitrot

Fix bitrot

Forgot to change one part of the file...

Comment on attachment 8384050 [details] [diff] [review] Part 6 v5: Improve the localization [checked in] Review of attachment 8384050 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: mail/locales/en-US/chrome/messenger/multimessageview.properties @@ +1,5 @@ > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +# LOCALIZATION NOTE (numConversations): Semi-colon list of plural forms. Nice documentation in here. ::: mailnews/base/util/templateUtils.js @@ +1,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +let EXPORTED_SYMBOLS = ["PluralStringFormatter", "makeFriendlyDateAgo"]; Shouldn't EXPORTED_SYMBOLS be a const? @@ +28,5 @@ > + return str; > + }, > +}; > + > + Nit - I don't think we need the extra newline here.

Comment on attachment 8384052 [details] [diff] [review] Part 7 v4: Move FormatDisplayName and friends to a JS module (to help Mail Summaries) [checked in] Review of attachment 8384052 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/modules/displayNameUtils.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Seems funny to perpetuate creating modules and giving them .js extensions instead of .jsm. If you put a .jsm on the end of this instead, I think that'd be good, but I won't harp on it. :) @@ +23,5 @@ > + * > + * @param aEmailAddress The address to look for. > + * @return An object with two properties, .book and .card. > + */ > +function GetCardForEmail(aEmailAddress) { Hrm. This is the third (?) thing called GetCardForEmail in comm-central. A shame we have to keep reimplementing it all over the place... @@ +64,5 @@ > + * @param aContext The field being formatted (e.g. "to", "from"). > + * @param aCard The address book card, if any. > + * @return The formatted display name, or null. > + */ > +function FormatDisplayName(aEmailAddress, aHeaderDisplayName, aContext, aCard) Are we sure these sorts of functions don't exist elsewhere? They look hauntingly familiar. If similar functions already exist, we might want to file a bug to de-duplicate... ::: mail/installer/removed-files.in @@ +576,5 @@ > modules/appIdleManager.js > modules/attachmentChecker.js > modules/ctypes.jsm > modules/dbViewWrapper.js > + modules/displayNameutils.js Utils, with a capital U?

Comment on attachment 8384059 [details] [diff] [review] Part 5 v5: Make the MultiMessageSummary object stick around forever [checked in] Review of attachment 8384059 [details] [diff] [review]: ----------------------------------------------------------------- Again, sorry for taking about forever to get to this. I can't find anything wrong here. All looks very sane. Let's pull the trigger. :)

Here's a try run to make sure nothing broke in the meantime: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=92cf035870a3

Landed: https://hg.mozilla.org/comm-central/rev/cb607c912206 https://hg.mozilla.org/comm-central/rev/8a65cd57e679 https://hg.mozilla.org/comm-central/rev/626d1149552c https://hg.mozilla.org/comm-central/rev/d5cc7852fef9 https://hg.mozilla.org/comm-central/rev/8b1266430b26

there's a throw typo "gMessenberBundle" at: http://hg.mozilla.org/comm-central/diff/8b1266430b26/mail/base/modules/displayNameUtils.js

Thanks.

Comment on attachment 8436337 [details] [diff] [review] fix typo Review of attachment 8436337 [details] [diff] [review]: ----------------------------------------------------------------- rs=me