1546425 - Allow to disable autocomplete per directory

Status: RESOLVED FIXED

(MailNews Core :: Address Book, enhancement)

Description

[John Bieling]

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.

Comment 1

[John Bieling]

Attachment: autocomplete.patch

Comment 2

[Jorg K (CEST = GMT+2)]

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?

Comment 3

[John Bieling]

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

Comment 4

[John Bieling]

Attachment: autocomplete.patch

Comment 5

[:aceman]

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);

Comment 6

[John Bieling]

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 :-)

Comment 7

[:aceman]

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

Comment 8

[John Bieling]

Attachment: autocomplete.patch

Comment 9

[John Bieling]

Attachment: autocomplete.patch

Now with correct header, I hope

Comment 10

[:aceman]

Comment on attachment 9060267 [details] [diff] [review]
autocomplete.patch

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

Yes, thanks.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/27ccda1bf87f
Allow to disable autocomplete per directory; r=aceman

Comment 12

[John Bieling]

Could I have this backported to ESR? Thanks!

Comment 13

[Jorg K (CEST = GMT+2)]

You should have returned NS_OK here: https://hg.mozilla.org/comm-central/rev/27ccda1bf87f#l1.22

Comment 14

[John Bieling]

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?

Comment 15

[Jorg K (CEST = GMT+2)]

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.

Comment 16

[Jorg K (CEST = GMT+2)]

TB 60.7 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/0ba26e8ccd61fc0092d36f412cd1be0c0baf362f