Clean up the multi-message summary code

RESOLVED FIXED in Thunderbird 32.0

Status

Thunderbird
Message Reader UI
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: squib, Assigned: squib)

Tracking

(Blocks: 1 bug, {perf})

Trunk
Thunderbird 32.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 19 obsolete attachments)

30.37 KB, patch
squib
: review+
Details | Diff | Splinter Review
9.76 KB, patch
squib
: review+
Details | Diff | Splinter Review
55.99 KB, patch
squib
: review+
Details | Diff | Splinter Review
36.77 KB, patch
squib
: review+
Details | Diff | Splinter Review
17.63 KB, patch
mconley
: review+
Details | Diff | Splinter Review
16.76 KB, patch
mconley
: review+
Details | Diff | Splinter Review
37.28 KB, patch
mconley
: review+
Details | Diff | Splinter Review
1.18 KB, patch
squib
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 8337462 [details] [diff] [review]
Part 1: Clean up selectionsummaries.js

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.
Attachment #8337462 - Flags: review?(mconley)
(Assignee)

Comment 2

4 years ago
Created attachment 8337464 [details] [diff] [review]
Part 2: Make messageDisplay.js know less about summaries

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.
Attachment #8337464 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Blocks: 690241
(Assignee)

Comment 3

4 years ago
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?
Attachment #8337464 - Flags: feedback?(jonathan.protzenko)
(Assignee)

Comment 4

4 years ago
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!
(Assignee)

Comment 6

4 years ago
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!
Attachment #8337464 - Flags: feedback?(jonathan.protzenko) → feedback+
(Assignee)

Comment 8

4 years ago
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.

Updated

4 years ago
Keywords: perf

Comment 10

4 years ago
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!
Attachment #8337462 - Flags: review?(mconley) → review+
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
Attachment #8337464 - Flags: review?(mconley) → review+
(Assignee)

Comment 13

4 years ago
(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.
(Assignee)

Comment 14

4 years ago
Created attachment 8360853 [details] [diff] [review]
Part 1 v2: Clean up selectionsummaries.js [checked in]
Attachment #8337462 - Attachment is obsolete: true
Attachment #8360853 - Flags: review+
(Assignee)

Comment 15

4 years ago
Created attachment 8360854 [details] [diff] [review]
Part 2 v2: Make messageDisplay.js know less about summaries [checked in]
Attachment #8337464 - Attachment is obsolete: true
Attachment #8360854 - Flags: review+
(Assignee)

Comment 16

4 years ago
Created attachment 8360855 [details] [diff] [review]
Part 3 v1: Move summary-specific code to multimessageview.js

Sorry for the huge patch, but it's mostly just moving things from one file to another! :)
Attachment #8360855 - Flags: review?(mconley)
(Assignee)

Comment 17

4 years ago
Created attachment 8360856 [details] [diff] [review]
Part 4: v1 Create a single function for making summary rows

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.
Attachment #8360856 - Flags: review?(mconley)
(Assignee)

Comment 18

4 years ago
Created attachment 8360857 [details] [diff] [review]
Part 5 v1: Make the MultiMessageSummary object stick around forever

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.
Attachment #8360857 - Flags: review?(mconley)
(Assignee)

Comment 19

4 years ago
Created attachment 8360858 [details] [diff] [review]
Part 6 v1: Improve the localization

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.
Attachment #8360858 - Flags: review?(mconley)
(Assignee)

Comment 20

4 years ago
Created attachment 8360859 [details] [diff] [review]
Part 7 v1: Move FormatDisplayName and friends to a JS module (to help Mail Summaries)

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!
Attachment #8360859 - Flags: review?(mconley)
(Assignee)

Comment 21

4 years ago
Created attachment 8360860 [details] [diff] [review]
Part 3 interdiff: Diff between selectionsummaries.js and multimessageview.js

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.
(Assignee)

Comment 22

4 years ago
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.
(Assignee)

Comment 23

4 years ago
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!
(Assignee)

Comment 24

4 years ago
Parts 1 and 2 landed: https://hg.mozilla.org/comm-central/rev/f29c718dd3d8
                      https://hg.mozilla.org/comm-central/rev/9d8c31ac7972
(Assignee)

Updated

4 years ago
Attachment #8360853 - Attachment description: Part 1 v2: Clean up selectionsummaries.js → Part 1 v2: Clean up selectionsummaries.js [checked in]
(Assignee)

Updated

4 years ago
Attachment #8360854 - Attachment description: Part 2 v2: Make messageDisplay.js know less about summaries → Part 2 v2: Make messageDisplay.js know less about summaries [checked in]
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.
Attachment #8360855 - Flags: review?(mconley) → review+
(Assignee)

Comment 26

4 years ago
(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.
Attachment #8360856 - Flags: review?(mconley) → review+
(Assignee)

Comment 28

4 years ago
Created attachment 8380287 [details] [diff] [review]
Part 3 v2: Move summary-specific code to multimessageview.js
Attachment #8360855 - Attachment is obsolete: true
Attachment #8360860 - Attachment is obsolete: true
Attachment #8380287 - Flags: review+
(Assignee)

Comment 29

4 years ago
Created attachment 8380288 [details] [diff] [review]
Part 4 v2: Create a single function for making summary rows
Attachment #8360856 - Attachment is obsolete: true
Attachment #8380288 - Flags: review+
(Assignee)

Comment 30

4 years ago
Created attachment 8380289 [details] [diff] [review]
Part 5 v2: Make the MultiMessageSummary object stick around forever
Attachment #8360857 - Attachment is obsolete: true
Attachment #8360857 - Flags: review?(mconley)
Attachment #8380289 - Flags: review?(mconley)
(Assignee)

Comment 31

4 years ago
Created attachment 8380290 [details] [diff] [review]
Part 6 v2: Improve the localization
Attachment #8360858 - Attachment is obsolete: true
Attachment #8360858 - Flags: review?(mconley)
Attachment #8380290 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Attachment #8380290 - Attachment description: 8360858: Part 6 v2: Improve the localization → Part 6 v2: Improve the localization
(Assignee)

Comment 32

4 years ago
Created attachment 8380291 [details] [diff] [review]
Part 7 v2: Move FormatDisplayName and friends to a JS module (to help Mail Summaries)
Attachment #8360859 - Attachment is obsolete: true
Attachment #8360859 - Flags: review?(mconley)
Attachment #8380291 - Flags: review?(mconley)
(Assignee)

Comment 33

4 years ago
Sorry for the bugspam! The review comments bitrotted the successor patches, so I've updated all of them.
(Assignee)

Comment 34

4 years ago
Note to self: part 6 needs reuploaded.
(Assignee)

Comment 35

4 years ago
Created attachment 8380400 [details] [diff] [review]
Part 6 v3: Improve the localization
Attachment #8380290 - Attachment is obsolete: true
Attachment #8380290 - Flags: review?(mconley)
Attachment #8380400 - Flags: review?(mconley)
(Assignee)

Comment 36

4 years ago
Created attachment 8381019 [details] [diff] [review]
Part 4 v3: Create a single function for making summary rows

I forgot to change a couple of references to the global window state, which broke opening individual messages.
Attachment #8380288 - Attachment is obsolete: true
Attachment #8381019 - Flags: review+
(Assignee)

Comment 37

4 years ago
Created attachment 8381020 [details] [diff] [review]
Part 5 v3: Make the MultiMessageSummary object stick around forever

De-bitrotting
Attachment #8380289 - Attachment is obsolete: true
Attachment #8380289 - Flags: review?(mconley)
Attachment #8381020 - Flags: review?(mconley)
(Assignee)

Comment 38

4 years ago
Created attachment 8381023 [details] [diff] [review]
Part 6 v4: Improve the localization

De-bitrotting
Attachment #8380400 - Attachment is obsolete: true
Attachment #8380400 - Flags: review?(mconley)
Attachment #8381023 - Flags: review?(mconley)
(Assignee)

Comment 39

4 years ago
Created attachment 8381025 [details] [diff] [review]
Part 7 v3: Move FormatDisplayName and friends to a JS module (to help Mail Summaries)

De-bitrotting
Attachment #8380291 - Attachment is obsolete: true
Attachment #8380291 - Flags: review?(mconley)
Attachment #8381025 - Flags: review?(mconley)
(Assignee)

Comment 40

4 years ago
Created attachment 8384045 [details] [diff] [review]
Part 3 v3: Move summary-specific code to multimessageview.js [checked in]

Fix bitrot
Attachment #8380287 - Attachment is obsolete: true
Attachment #8384045 - Flags: review+
(Assignee)

Comment 41

4 years ago
Created attachment 8384047 [details] [diff] [review]
Part 4 v4: Create a single function for making summary rows [checked in]

Fix bitrot
Attachment #8381019 - Attachment is obsolete: true
Attachment #8384047 - Flags: review+
(Assignee)

Comment 42

4 years ago
Created attachment 8384049 [details] [diff] [review]
Part 5 v4: Make the MultiMessageSummary object stick around forever

Fix bitrot and improve interface of summarizers
Attachment #8381020 - Attachment is obsolete: true
Attachment #8381020 - Flags: review?(mconley)
Attachment #8384049 - Flags: review?(mconley)
(Assignee)

Comment 43

4 years ago
Created attachment 8384050 [details] [diff] [review]
Part 6 v5: Improve the localization [checked in]

Fix bitrot
Attachment #8381023 - Attachment is obsolete: true
Attachment #8381023 - Flags: review?(mconley)
Attachment #8384050 - Flags: review?(mconley)
(Assignee)

Comment 44

4 years ago
Created attachment 8384052 [details] [diff] [review]
Part 7 v4: Move FormatDisplayName and friends to a JS module (to help Mail Summaries) [checked in]

Fix bitrot
Attachment #8381025 - Attachment is obsolete: true
Attachment #8381025 - Flags: review?(mconley)
Attachment #8384052 - Flags: review?(mconley)
(Assignee)

Comment 45

4 years ago
Created attachment 8384059 [details] [diff] [review]
Part 5 v5: Make the MultiMessageSummary object stick around forever [checked in]

Forgot to change one part of the file...
Attachment #8384049 - Attachment is obsolete: true
Attachment #8384049 - Flags: review?(mconley)
Attachment #8384059 - Flags: review?(mconley)
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.
Attachment #8384050 - Flags: review?(mconley) → review+
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?
Attachment #8384052 - Flags: review?(mconley) → review+
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. :)
Attachment #8384059 - Flags: review?(mconley) → review+
(Assignee)

Comment 49

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

Comment 50

4 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Attachment #8384045 - Attachment description: Part 3 v3: Move summary-specific code to multimessageview.js → Part 3 v3: Move summary-specific code to multimessageview.js [checked in]
(Assignee)

Updated

4 years ago
Attachment #8384047 - Attachment description: Part 4 v4: Create a single function for making summary rows → Part 4 v4: Create a single function for making summary rows [checked in]
(Assignee)

Updated

4 years ago
Attachment #8384050 - Attachment description: Part 6 v5: Improve the localization → Part 6 v5: Improve the localization [checked in]
(Assignee)

Updated

4 years ago
Attachment #8384052 - Attachment description: Part 7 v4: Move FormatDisplayName and friends to a JS module (to help Mail Summaries) → Part 7 v4: Move FormatDisplayName and friends to a JS module (to help Mail Summaries) [checked in]
(Assignee)

Updated

4 years ago
Attachment #8384059 - Attachment description: Part 5 v5: Make the MultiMessageSummary object stick around forever → Part 5 v5: Make the MultiMessageSummary object stick around forever [checked in]

Comment 51

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

Comment 52

4 years ago
Created attachment 8436337 [details] [diff] [review]
fix typo

Thanks.
Attachment #8436337 - Flags: review?(squibblyflabbetydoo)

Updated

4 years ago
Target Milestone: --- → Thunderbird 32.0
Version: unspecified → Trunk
(Assignee)

Comment 53

4 years ago
Comment on attachment 8436337 [details] [diff] [review]
fix typo

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

rs=me
Attachment #8436337 - Flags: review?(squibblyflabbetydoo) → review+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/2be89f58a671
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.