807101 - fix logic in am-prefs.js::onCheckItem of disabling elements when the corresponding pref is locked and convert am-addressing.js to use this common function.

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

The code in am-addressing::enabling() can be reduced to 2 calls of am-prefs.js::onCheckItem nowadays.

Comment 1

[:aceman]

I can make an easy test for this in the test file created in bug 525024.
The onCheckItem function is evolving often (by me:)) so it would be good to have a test for it.

Comment 2

[:aceman]

I have found a logic strangeness in am-prefs.js::onCheckItem() (but the same can be seen in the original version in am-offline.js::onCheckItem()):

Suppose there is a checkbox, that controls another element. If the checkbox is checked, the element get enabled.
Now if the checkbox is locked (via the prefstring mechanism) but still checked, we also disable the other element and do not allow to change its value.

Why is that so? Why do we not allow to change the element when its own prefstring is not locked?

Comment 3

[[e-mail]]

Originally onCheckItem didn't know about locked preferences. Then, support was added in bug 85335. But as you point out, the interaction between locked preferences and the checkboxes is completely broken because the locked state of the checkbox is used instead of the textbox. Oops.

Comment 4

[[e-mail]]

Although to be fair, you probably want to lock both at once anyway.

Comment 5

[:aceman]

I can fix it we just need to decide what the behaviour should be.

I think only the pref belonging to the element being decided is to be checked. E.g. we may want to support the use case when only the 
checkbox is locked, but not the textbox. This is currently not possible. If somebody really wants to disable both, then he can lock both prefs.

Comment 6

[[e-mail]]

(In reply to aceman from comment #5)
> I think only the pref belonging to the element being decided is to be checked.
Yes, that's what SeaMonkey does in Preferences.

> This is currently not possible.
Worse, it's broken, because it allows the textbox to be editable although the underlying preference is locked so the change never gets saved.

Comment 7

[:aceman]

Thanks, I'll fix it up.

Comment 8

[:aceman]

Attachment: patch + test

Comment 9

[[e-mail]]

Comment on attachment 681629 [details] [diff] [review]
patch + test

Code seems reasonable* but I can't comment on the test.

*Well, you could go for something arcane like
elementToControl.disabled = getAccountValueIsLocked(elementToControl) ||
  aCheckElementIds.any(function(aNotifyId) {
    var notifyElement = document.getElementById(notifyId);
    return notifyElement.checked || notifyElement.selected;
  });

Comment 10

[:aceman]

Comment on attachment 681629 [details] [diff] [review]
patch + test

I don't like unreadable JS code ;)

Mconley will take care of the test.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 681629 [details] [diff] [review]
patch + test

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

This looks really good! Just one nit below.

If this try build looks OK https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=f936c5be84d9 - you've got my r+.

::: mail/test/mozmill/account/test-account-settings-infrastructure.js
@@ +275,5 @@
>      subtest_check_account_dot_IDs(amc);
>    });
>  }
> +
> +function test_account_locked_prefs() {

The subtests should be immediately beneath this function.

Comment 12

[:aceman]

Attachment: patch + test v2

Ok. It looks like the try build works.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 687195 [details] [diff] [review]
patch + test v2

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

r=me with a few more nits I found with your latest version fixed. Thanks aceman!

::: mail/test/mozmill/account/test-account-settings-infrastructure.js
@@ +51,5 @@
>    // There should be only the original accounts left.
>    assert_equals(MailServices.accounts.allServers.Count(), gOriginalAccountCount);
>  }
>  
> +/*

Nit: /**

@@ +207,5 @@
> +  Services.prefs.unlockPref(controlPref);
> +  Services.prefs.getDefaultBranch("").deleteBranch(controlPref);
> +}
> +
> +

Nit - just one newline here.

::: mailnews/base/prefs/content/am-addressing.js
@@ +9,5 @@
>  {
>    parent.onPanelLoaded('am-addressing.xul');
>  }
>  
>  function onInit(aPageId, aServerId) 

Strip that trailing whitespace while you're here.

Comment 14

[:aceman]

Attachment: patch + test v3

Thanks, done.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/118ec2f9e401