Closed Bug 1439172 Opened 2 years ago Closed 2 years ago

Port |Bug 1427364 - Remove showCommentColumn and showImageColumn| to C-C

Categories

(MailNews Core :: Composition, defect)

defect
Not set

Tracking

(thunderbird60+)

RESOLVED FIXED
Thunderbird 60.0
Tracking Status
thunderbird60 + ---

People

(Reporter: jorgk, Assigned: Paenglab)

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

The undocumented option mail.autoComplete.commentColumn has stopped working. If you set it to one, the auto-complete popup shows names of the address books, not a list of recipients.

We should remove related code or fix it:
https://dxr.mozilla.org/comm-central/search?q=commentColumn&redirect=true
Version: 52 → Trunk
Attached image autocomplete.png
Here I see no difference, maybe because I use a opt build.

But I found something different: When searching the address, the LDAP shows no names, only the LDAP directory name. I then played a bit and enabled the the .ac-separator and the .ac-url. Et voilà, I see the names too. See screenshot.

I could enable them by default, but then on normal addresses we have them two times. Good would be, we could separate the name and the email address into the two fields ac-title and ac-url. Then we would have a nice separation.

Or I could automatically enable the .ac-url when something is in the ac-comment attribute of the richlistitem (the LDAP directory name is in this comment). Then the names/addresses are shown of the LDAP.
(In reply to Richard Marti (:Paenglab) from comment #1)
> But I found something different: When searching the address, the LDAP shows
> no names, only the LDAP directory name.
Well, I only see address book names, see enclosed. That's on an (opt) Daily.
(In reply to Richard Marti (:Paenglab) from comment #1)
> I could enable them by default, but then on normal addresses we have them
> two times. Good would be, we could separate the name and the email address
> into the two fields ac-title and ac-url. Then we would have a nice
> separation.
Hmm, we could turn the order around and show the address book/directory name additionally when the option is set, I think that's how it was intended.
(In reply to Richard Marti (:Paenglab) from comment #1)
> Here I see no difference, maybe because I use a opt build.
Did you add pref mail.autoComplete.commentColumn with value 1?
Shouldn't this be a boolean?
    if (getPref("mail.autoComplete.commentColumn"))
      autoCompleteWidget.showCommentColumn = true;
Ah yes, tried it with an older Daily and mail.autoComplete.commentColumn=1 shows the AB names in a second column (and with the same icon the entry has -> looks not so good).
(In reply to Richard Marti (:Paenglab) from comment #5)
> Shouldn't this be a boolean?
>     if (getPref("mail.autoComplete.commentColumn"))
>       autoCompleteWidget.showCommentColumn = true;
Shouldn't this be documented??

But no, it's an int:
comm-central/mailnews/addrbook/src/nsAbAutoCompleteSearch.js
this._commentColumn = Services.prefs.getIntPref("mail.autoComplete.commentColumn");

1 will also test 'true' in |if (getPref("mail.autoComplete.commentColumn"))|.
Attached patch autocompleteComment.patch (obsolete) — Splinter Review
This would fix the appearance with mail.autoComplete.commentColumn=1 and also with LDAP directories. Additionally I added the highlight of the search string by bolding it in the autocomplete items.

The question is still, should we expose this pref? My patch would also be handy for LDAP with removal of the pref.
Attachment #8951969 - Flags: feedback?(jorgk)
This one applies on tip.
Attachment #8951969 - Attachment is obsolete: true
Attachment #8951969 - Flags: feedback?(jorgk)
Attachment #8951970 - Flags: feedback?(jorgk)
Comment on attachment 8951970 [details] [diff] [review]
autocompleteComment.patch

This is excellent. I love the bold(!) to show me where the match is.

Do you have a tip for testing LDAP?
Attachment #8951970 - Flags: feedback?(jorgk) → feedback+
This is a little clean-up around the edges. Add some comments, declare the preference and remove dead code.
Comment on attachment 8951970 [details] [diff] [review]
autocompleteComment.patch

I won't fiddle with LDAP unless you can give me an easy open/public server. I'm happy to r+ this based on a screenshot you can attach for LDAP.
Attachment #8951970 - Flags: review+
Comment on attachment 8952138 [details] [diff] [review]
1439172-showCommentColumn.patch (v1b)

I think this is all we need here to restore the "comment column". In fact, it works with Richard's patch alone, but this one here does a little clean-up.
Attachment #8952138 - Flags: review?(acelists)
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
I got the details fro Adam's LADP server from Aceman and from here:
https://howto.adams.edu/ASU_Staff_Email_Setup_-_Apple_Mail
Auto-complete also works fine for LDAP, so r+ again :-)
Comment on attachment 8952138 [details] [diff] [review]
1439172-showCommentColumn.patch (v1b)

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

Works nicely, thanks guys.

::: mail/components/compose/content/MsgComposeCommands.js
@@ -5579,5 @@
>      if (getPref("mail.autoComplete.highlightNonMatches"))
>        autoCompleteWidget.highlightNonMatches = true;
> -
> -    if (getPref("mail.autoComplete.commentColumn"))
> -      autoCompleteWidget.showCommentColumn = true;

Any idea why we can actually remove this? Is the feature built in now?
Attachment #8952138 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #16)
> Any idea why we can actually remove this? Is the feature built in now?

Or we ignore this and check the pref again at
https://dxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbAutoCompleteSearch.js#364
(In reply to :aceman from comment #16)
> Any idea why we can actually remove this? Is the feature built in now?
That's the whole idea of the port. This was removed in bug 1427364:
https://hg.mozilla.org/mozilla-central/rev/c8e7ab77ed3f#l8.12

Instead we now use various tricks that come with the new richlist auto-complete to which M-C switched in bug 1427366.

(In reply to :aceman from comment #17)
> Or we ignore this and check the pref again at
> https://dxr.mozilla.org/comm-central/source/mailnews/addrbook/src/
> nsAbAutoCompleteSearch.js#364
I don't understand this comment. We ignore what? We read the pref multiple times where we need it, but there is no need to set
  autoCompleteWidget.showCommentColumn = true;
any more.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b4e185898c1f
port bug 1427364: Show comments in address autocomplete when they exist. r=jorgk
https://hg.mozilla.org/comm-central/rev/f51fea20a88c
port bug 1427364: Auto-complete no longer supports showCommentColumn. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 60.0
You need to log in before you can comment on or make changes to this bug.