1526985 - Add support for multiple authors (from) to gloda search

Status: NEW

(MailNews Core :: Search, enhancement)

Comment 1

[alta88]

Attachment: authorsGloda.patch

this wip seems to work, except for some reason the faceted results UI doesn't display the author as GlodaMessage instance doesn't get the string for the from property. asuth, any advice in general (other than lasciate ogni speranza..).

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9043001 [details] [diff] [review]
authorsGloda.patch

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

I'm not sure I understand the problem you're seeing.  Are you saying the object doesn't query from disk right, or that the DOM doesn't get built right, or that the resulting DOM doesn't visually seem to display correctly?

Gloda has unit tests around indexing and querying, I would suggest starting with updating those and then moving up the stack.  Honestly, I'm not able to spend many/any cycles on Thunderbird related things, but it's definitely a prerequisite that tests be updated and I can look at a log of them passing or failing to understand what's happening.

Comment 3

[alta88]

What I'm seeing is that GlodaMessage.from is not populated, and it should be done in GlodaFundAttr.process when getOrCreateMailIdentities() assigns to it. Of course the tests need to be updated for .from now being an array of identities rather than an identity. In general, I'm just wondering if there's anything else architecturally than needs to be considered for multiple authors, if you can recall. Thanks for any help you have time for.

Comment 4

[alta88]

It seems gloda results (Tb 60) don't correctly display non ascii From names, while at the same time there's a comment not to use MIME decoded variants on msgHdr (mime2DecodedAuthor) because comma parsing, which has likely been fixed with jsmime that postdates gloda. Hard to believe, and yet no bug for it.

Comment 5

[alta88]

Meaning RFC6532 headers aren't handled.

Comment 6

[alta88]

Attachment: authorsGloda.patch

wip2. fix failure to display to: if both from and to are the same.

Comment 7

[alta88]

Attachment: authorsGloda.patch

wip3, this is fairly complete

• trim leading \n and whitespace in message content to not look sloppy
• display real identity name for myContact identities in from/to rather than rolled up contact name; that's just ultra confusing
• sometimes gloda message array properties are null and sometimes a zero length array; handle em both.
• a db built in Tb60 seems to have no ill effects after upgrading to a version with this patch and friends

still to investigate: sometimes a utf8 multiauthor from order is not as stored, ie it is of great importance to display the first author first (an academic will get it).

Comment 8

[alta88]

Attachment: authorsGloda.patch

wip4, fix bug 1429747, Bug 1223951.

Comment 9

[alta88]

Attachment: authorsEtAl.patch

Part 1, consistency with message list strings in summary views.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9064055 [details] [diff] [review]
authorsEtAl.patch

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

Looks good to me, r=mkmelin

::: mail/base/content/foldersummary.js
@@ +108,5 @@
>          return false;
>        }
>  
>        foundNewMsg = true;
> +      let andOthersStr = this.messengerBundle.GetStringFromName("andOthers");

given this is a bit of an edge case, could move this declaration down to where it's used (if it's used)

::: mail/base/content/multimessageview.js
@@ +252,1 @@
>      );

move ); to the earlier line

Comment 11

[alta88]

Attachment: authorsEtAl.patch

updated for comments.

Comment 12

[Jorg K (CEST = GMT+2)]

Are you sure this won't break any tests? Last time we added multiple authors in bug 1423487 we had various test failures. Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cd5a2aadf0437a43e91e322563b3ec8b16f374ee

Comment 13

[Jorg K (CEST = GMT+2)]

I'd say these failures come from here:
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/folder-display/test-summarization.js | test-summarization.js::test_display_name_no_abook
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/folder-display/test-summarization.js | test-summarization.js::test_display_name_abook
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/folder-display/test-summarization.js | test-summarization.js::test_display_name_abook_no_pdn

The orange X's are known intermittent failures.

EDIT:
EXCEPTION: Expected to find sender named 'Andy Ulvaeus <[e-mail]>', found 'Andy Ulvaeus'
EXCEPTION: Expected to find sender named 'My Friend', found 'Andy Ulvaeus'
EXCEPTION: Expected to find sender named 'Andy Ulvaeus <[e-mail]>', found 'Andy Ulvaeus'

Comment 14

[alta88]

Attachment: authorsEtAl.patch

fixed for tests, thanks for checking.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9065186 [details] [diff] [review]
authorsEtAl.patch

Looks like a completely different patch now ;-) - I'll add myself to the reviewers then ;-)

Just out of interest: Is "et al." a commonly used abbreviation in Anglo-Saxonia? Or do only people with a Latin degree understand it?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5806b067a6a1
Part 1: Add support for multiple authors (add "et al" to summary views). r=mkmelin,jorgk

Comment 17

[alta88]

(In reply to Jorg K (GMT+2) from comment #15)

Comment on attachment 9065186 [details] [diff] [review]
authorsEtAl.patch

Looks like a completely different patch now ;-) - I'll add myself to the
reviewers then ;-)

Just out of interest: Is "et al." a commonly used abbreviation in
Anglo-Saxonia? Or do only people with a Latin degree understand it?

It is the accepted and well known representation in academic and legal writing. There are style guides to its usage, which most common is no comma before 'et al.' and no period after 'et' and violating that is akin to code syntax religious wars. Here, we are not exactly following printed usage, which dictates that there must be more than 3 or 6 (depending on guide) authors named before et al. can be used (since horizontal space is at a premium in From threadpane column). But 1 can be used if the others have already been referred to previously. Etc.