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...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 20.0
Assigned To: :aceman
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


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

Description :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 :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 :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 neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 2012-11-02 03:18:14 PDT
Although to be fair, you probably want to lock both at once anyway.
Comment 5 :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 neil@parkwaycc.co.uk 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 :aceman 2012-11-02 06:37:56 PDT
Thanks, I'll fix it up.
Comment 8 :aceman 2012-11-14 12:10:54 PST
Created attachment 681629 [details] [diff] [review]
patch + test
Comment 9 neil@parkwaycc.co.uk 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 :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 Mike Conley (:mconley) - (Away until June 29th) 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 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 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 Mike Conley (:mconley) - (Away until June 29th) 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 :aceman 2012-11-30 14:20:56 PST
Created attachment 687283 [details] [diff] [review]
patch + test v3

Thanks, done.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-12-01 18:02:28 PST
https://hg.mozilla.org/comm-central/rev/118ec2f9e401

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