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.

RESOLVED FIXED in Thunderbird 20.0

Status

MailNews Core
Account Manager
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 20.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

15.03 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

5 years ago
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.
Depends on: 525024
(Assignee)

Comment 2

5 years ago
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?
Flags: needinfo?(iann_bugzilla)

Updated

5 years ago
Flags: needinfo?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Flags: needinfo?(neil)

Comment 3

5 years ago
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.
Flags: needinfo?(neil)

Comment 4

5 years ago
Although to be fair, you probably want to lock both at once anyway.
(Assignee)

Comment 5

5 years ago
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.
Flags: needinfo?(neil)

Comment 6

5 years ago
(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.
Flags: needinfo?(neil)
(Assignee)

Comment 7

5 years ago
Thanks, I'll fix it up.
Severity: minor → normal
Flags: in-testsuite?
Summary: convert am-addressing.js to onCheckItem in am-prefs.js → 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.
(Assignee)

Comment 8

5 years ago
Created attachment 681629 [details] [diff] [review]
patch + test
Attachment #681629 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED

Comment 9

5 years ago
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;
  });
Attachment #681629 - Flags: review?(neil) → review+
(Assignee)

Comment 10

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

I don't like unreadable JS code ;)

Mconley will take care of the test.
Attachment #681629 - Flags: review?(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.
(Assignee)

Comment 12

5 years ago
Created attachment 687195 [details] [diff] [review]
patch + test v2

Ok. It looks like the try build works.
Attachment #681629 - Attachment is obsolete: true
Attachment #681629 - Flags: review?(mconley)
Attachment #687195 - Flags: review?(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.
Attachment #687195 - Flags: review?(mconley) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 687283 [details] [diff] [review]
patch + test v3

Thanks, done.
Attachment #687195 - Attachment is obsolete: true
Attachment #687283 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/118ec2f9e401
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
You need to log in before you can comment on or make changes to this bug.