1631201 - Showing comma (,) instead of fall back names in address book for |View > Show Name As > Last, First|

Status: VERIFIED FIXED

(Thunderbird :: Address Book, defect)

Description

[Thomas D. (:thomas8)]

Something broke the fallback algorithm of address book's 'Name' column for View > Show Name As > Last, First when Last name and First name are not present on the contact's card.

STR

1. Have these contacts in your local AB:

First	Last	Display	Email	Company
		Anne Adams	[e-mail]
			[e-mail]	Computer-Company
			[e-mail]
Tom	Doe	Tom Doe	[e-mail]

2. In main AB, select View > Show Name As > Last, First (required step).

Actual Result

Your list of AB entries (name column) now looks like this:

,                         (that's Anne, don't you see?) 
,                         (that's Computer-Company)
,                         (that's John)
Doe, Tom

• Visually odd: Loads of meaningless commas (,) in the name column.
• Lots of valid contacts harder to distinguish as they are not showing anything in the "Name" column.
• Regression: Existing fall back algorithm fails (compare release version, which will always display something in the name field; see expected result).
• Easy to identify contacts without Last and First name; however, it's like forcing users to fill in those fields for a useful names experience, which they may not be ready to do. Instead, we should offer optional colums for First name and Last name fields.

Expected Result

Anne Adams        (RFE: first use display name as fallback)
Computer-Company  (regression-fix: use company field as fallback)
Doe, John         (regular entry: Last, First)
johnxxx           (regression-fix: use local part of email address as fallback)

Imo, we should fix the existing fallback algorithm which regressed:
When Last Name and First Name are not filled, show data from other fields in the name column instead:

• Company Name
• Local part of email address

I suggest to enhance the fallback and start with display name first (as it's technically possible to have display name without Last and First name filled):

• Display Name
• Company Name
• Local part of email address

For polishing:

• Can we have some way of identifying list entries where Last name, First name fields are empty and the fallback is used? In current release, we are just sorting fallback names into the Last, First sequence. Options:
  • some visual hint where fallback names have been used, e.g.:
    • use italics for fallback names (whilst keeping them in alphabetical order)
    • prepend/overlay some other visual indication
  • sort all fallbacks to the top, e.g. by prepending some special character to the fallback
    name:
    ~ Anne Adams
    ~ Computer Company
    ~ johnxxx
    Doe, Tom
  • no indication in Names colum; instead, allow user to display Last name and First name columns

Maybe italics could work.

Comment 1

[Thomas D. (:thomas8)]

Fallback algorithm lives here:
https://searchfox.org/comm-central/rev/eb1e74fdbf991d849543c36e376b22c5e72228b8/mailnews/addrbook/jsaddrbook/AddrBookCard.jsm#42-63

AddrBookCard.prototype = {
...
  generateName(generateFormat, bundle) {
...
    case Ci.nsIAbCard.GENERATE_LAST_FIRST_ORDER:
        format = bundle
          ? bundle.GetStringFromName("lastFirstFormat")
          : "%S, %S";
        result = format
          .replace("%S", this.lastName)
          .replace("%S", this.firstName);
        break;
...
    if (result == "") {
      result = this.getProperty("Company", "");
    }
    if (result == "") {
      result = this.primaryEmail.split("@", 1)[0];
    }

    return result;
  },

Comment 2

[Wayne Mery (:wsmwk)]

I saw some strangeness, I think it was in corespondent column, for a > or + character. Does that sound related?

Comment 3

[Wayne Mery (:wsmwk)]

Which version did it start?

Comment 4

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk) from comment #2)

I saw some strangeness, I think it was in corespondent column, for a > or + character. Does that sound related?

Not for me.

(In reply to Wayne Mery (:wsmwk) from comment #3)

Which version did it start?

TB68 still works. Not sure when exactly this broke.
Should recheck STR on TB78 release candidate.

On daily 82.0a1 (2020-08-25) (64-bit), win10, now also broken for the other variants, essentially showing the display name always regardless of your settings Show name as: First Last | Last, First | Display name. Iow, first and last name are now ignored. check like this:
First name: JohnFirst
Last name: DoeLast
Display name: John Doe Display

And simple (round brackets) in display name also make things fail :-//

Comment 5

[Alice0775 White]

About STR step1,
I couldn't find "Company" field in AB new contact dialog.
I couldn't find "First" "Last" "Display" and "Company" in column name as well as column picker of AB top-right pane.

So I don't understand STR.

Comment 6

[Alice0775 White]

Attachment: screenshot good .vs. bad

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=e7476e934b191ffac787208323191c9c034049ce&tochange=72b1cec1260e68511a8b99d0e72fd8b0cefd44dc
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3fa65bda1e506a314ea90d936f763c7e840ab98a&tochange=3fa65bda1e506a314ea90d936f763c7e840ab98a

Comment 7

[Geoff Lankow (:darktrojan)]

Oh no. This never would've happened if the regression test for it actually tested the new code. Instead it only tests the old code, which is still hanging around for various reasons. Now I feel really stupid. :-(

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1631201 - Fix the nsIAbCard regression test to run on the new code, and the regressions it would've found. r?mkmelin

This test only ever tested nsAbCardProperty, it's never tested AddrBookCard. Now it tests both.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/79591a29a58c
Fix the nsIAbCard regression test to run on the new code, and the regressions it would've found. r=mkmelin

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 9189090 [details]
Bug 1631201 - Fix the nsIAbCard regression test to run on the new code, and the regressions it would've found. r?mkmelin

[Approval Request Comment]
Regression caused by (bug #): bug 1581765
User impact if declined: parts of address book list blank when they should have information, potentially other problems
Testing completed (on c-c, etc.): on c-c for a week
Risk to taking this patch (and alternatives if risky): not risky

Comment 11

[Wayne Mery (:wsmwk)]

Comment on attachment 9189090 [details]
Bug 1631201 - Fix the nsIAbCard regression test to run on the new code, and the regressions it would've found. r?mkmelin

[Triage Comment]
Approved for beta

Comment 12

[Dave Miller [:justdave]]

Thunderbird 84.0b3
https://hg.mozilla.org/releases/comm-beta/rev/5297f856d9fb

Comment 13

[WaltS48 [:walts48]]

Looks good to me testing the 84.0b3 release candidate on Ubuntu 18.04 LTS.

Created a new address book I called Bug Test, added the test names in the above STR, selected View > Show Name As > Last, First and the result looks like the Good screenshot in comment 6

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9189090 [details]
Bug 1631201 - Fix the nsIAbCard regression test to run on the new code, and the regressions it would've found. r?mkmelin

[Triage Comment]
Approved for esr78

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 78.6.0:
https://hg.mozilla.org/releases/comm-esr78/rev/82bc2e5efaa6

Comment 16

[WaltS48 [:walts48]]

Verified in testing of the Thunderbird 78.6.0 release candidate on Windows 10.
Same as my comment in comment 13.