Closed Bug 1546425 Opened 2 years ago Closed 2 years ago

Allow to disable autocomplete per directory

Categories

(MailNews Core :: Address Book, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird_esr6067+ fixed, thunderbird67 fixed, thunderbird68 fixed)

RESOLVED FIXED
Thunderbird 68.0
Tracking Status
thunderbird_esr60 67+ fixed
thunderbird67 --- fixed
thunderbird68 --- fixed

People

(Reporter: TbSync, Assigned: TbSync)

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/73.0.3683.103 Safari/537.36

Steps to reproduce:

Currently autocomplete can only be enabled/disabled for ALL local address books. This behavior is defined here:
https://searchfox.org/comm-central/source/mailnews/addrbook/src/nsAbDirProperty.cpp#464-476

I would like to add a directory property, to disable autocomplete for individual directories.

Type: defect → enhancement
Attached patch autocomplete.patch (obsolete) — Splinter Review
Attachment #9060125 - Flags: review?(acelists)
Comment on attachment 9060125 [details] [diff] [review]
autocomplete.patch

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

::: mailnews/addrbook/src/nsAbDirProperty.cpp
@@ +474,5 @@
>  
> +  rv = prefBranch->GetBoolPref("mail.enable_autocomplete", aResult);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  //If autocomplete is generally enabled, check if it has been disabled explicitly for this directory

Nits: Space after // and full stop at the end.

@@ +477,5 @@
> +
> +  //If autocomplete is generally enabled, check if it has been disabled explicitly for this directory
> +  if (*aResult == true)
> +  {
> +    GetBoolValue("enable_autocomplete", true, aResult);

Where can that be set?

Nits: will change that.

It can be set programatically. I do not want to add an UI option.

It is intended for advanced address books (addons) that extend the behaviour of the thunderbird address book. In some cases the standard autocomplete function produces wrong results and should be disabled (by the addon managing that address book).

Attached patch autocomplete.patch (obsolete) — Splinter Review
Attachment #9060125 - Attachment is obsolete: true
Attachment #9060125 - Flags: review?(acelists)
Attachment #9060236 - Flags: review?(acelists)
Comment on attachment 9060236 [details] [diff] [review]
autocomplete.patch

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

Thanks, works for me.

The pref can also be set manually in pref editor or prefs.js [as user_pref("ldap_2.servers.<Abname>.enable_autocomplete", false);].

::: mailnews/addrbook/src/nsAbDirProperty.cpp
@@ +475,5 @@
> +  rv = prefBranch->GetBoolPref("mail.enable_autocomplete", aResult);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // If autocomplete is generally enabled, check if it has been disabled explicitly for this directory.
> +  if (*aResult == true)

if (*aResult) ?

@@ +477,5 @@
> +
> +  // If autocomplete is generally enabled, check if it has been disabled explicitly for this directory.
> +  if (*aResult == true)
> +  {
> +    GetBoolValue("enable_autocomplete", true, aResult);

Also check the return value of GetBoolValue().

@@ +480,5 @@
> +  {
> +    GetBoolValue("enable_autocomplete", true, aResult);
> +  }
> +
> +  return rv;

You could actually do:
rv = prefBranch->GetBoolPref("mail.enable_autocomplete", aResult);
NS_ENSURE_SUCCESS(rv, rv);

if (!*aResult)
  return NS_OK;

// If autocomplete is generally enabled, check if it has been disabled explicitly for this directory.
return GetBoolValue("enable_autocomplete", true, aResult);
Attachment #9060236 - Flags: review?(acelists) → review+

Thanks!

I did not check the return value of GetBoolValue() because I did not want to change the final return value by this patch.

The return value is only influenced by the primary check as before and I am only doing the second check, if the first one succeeded (so we have an answer we can work with). The second check is a bonus and if that fails for some reason, your version would change the return value to NS_ERROR compared to a system without this patch, which just gets NS_OK from the first check.

What do you think?

I can change the if (*aResult) of course.

What would be the next step? I am still new to all of this :-)

OK, sounds reasonable, but then make it '(void) GetBoolValue(...)' so indicate we intentionally ignore the return value.

Attached patch autocomplete.patch (obsolete) — Splinter Review
Attachment #9060236 - Attachment is obsolete: true
Attachment #9060263 - Flags: review?(acelists)

Now with correct header, I hope

Attachment #9060263 - Attachment is obsolete: true
Attachment #9060263 - Flags: review?(acelists)
Attachment #9060267 - Flags: review?(acelists)
Comment on attachment 9060267 [details] [diff] [review]
autocomplete.patch

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

Yes, thanks.
Attachment #9060267 - Flags: review?(acelists) → review+
Assignee: nobody → john.bieling
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: checkin-needed
OS: Unspecified → All
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/27ccda1bf87f
Allow to disable autocomplete per directory; r=aceman

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0

Could I have this backported to ESR? Thanks!

Attachment #9060267 - Flags: approval-comm-beta?
Attachment #9060267 - Flags: approval-comm-esr60?
Attachment #9060267 - Flags: approval-comm-beta?
Attachment #9060267 - Flags: approval-comm-beta+

But that does not make a difference, or? rv is NS_OK as it passed NS_ENSURE_SUCCESS(rv, rv). Right? Do you want me to update the patch?

TB 67 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/e305f7284ec8eb2449ba95f82042ca7075767ae2

It makes no difference, but is easier to read. Let's leave it as it is now.

Attachment #9060267 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.