Last Comment Bug 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.
: fix logic in am-prefs.js::onCheckItem of disabling elements when the correspo...
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
-- normal (vote)
: Thunderbird 20.0
Assigned To: :aceman
Depends on: 525024
Blocks: 755885
  Show dependency treegraph
Reported: 2012-10-30 14:19 PDT by :aceman
Modified: 2012-12-01 18:02 PST (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch + test (14.77 KB, patch)
2012-11-14 12:10 PST, :aceman
neil: review+
Details | Diff | Splinter Review
patch + test v2 (15.00 KB, patch)
2012-11-30 11:16 PST, :aceman
mconley: review+
Details | Diff | Splinter Review
patch + test v3 (15.03 KB, patch)
2012-11-30 14:20 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description User image :aceman 2012-10-30 14:19:50 PDT
The code in am-addressing::enabling() can be reduced to 2 calls of am-prefs.js::onCheckItem nowadays.
Comment 1 User image :aceman 2012-10-30 14:23:49 PDT
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 User image :aceman 2012-10-30 14:28:01 PDT
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 User image 2012-11-02 03:16:52 PDT
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 User image 2012-11-02 03:18:14 PDT
Although to be fair, you probably want to lock both at once anyway.
Comment 5 User image :aceman 2012-11-02 04:23:26 PDT
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 User image 2012-11-02 04:33:57 PDT
(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 User image :aceman 2012-11-02 06:37:56 PDT
Thanks, I'll fix it up.
Comment 8 User image :aceman 2012-11-14 12:10:54 PST
Created attachment 681629 [details] [diff] [review]
patch + test
Comment 9 User image 2012-11-15 06:52:30 PST
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 User image :aceman 2012-11-20 08:21:44 PST
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 User image Mike Conley (:mconley) 2012-11-29 15:20:24 PST
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 - 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 User image :aceman 2012-11-30 11:16:04 PST
Created attachment 687195 [details] [diff] [review]
patch + test v2

Ok. It looks like the try build works.
Comment 13 User image Mike Conley (:mconley) 2012-11-30 14:05:53 PST
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 User image :aceman 2012-11-30 14:20:56 PST
Created attachment 687283 [details] [diff] [review]
patch + test v3

Thanks, done.
Comment 15 User image Ryan VanderMeulen [:RyanVM] 2012-12-01 18:02:28 PST

Note You need to log in before you can comment on or make changes to this bug.