181973 - Many pref panels need to reverse the sense of boolean prefs

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[[e-mail]]

The scripts & plugins page is a good example of this; the checkbox listitems
control prefs that need to be set to the inverse of the checkbox value.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Create a preftype of "inverse" which is the inverse of the "boolean" preftype.

Comment 2

[jag (Peter Annema)]

I like the idea, but the name "inverse" seems too generic. Up to Ben though,
it's his code.

Comment 3

[[e-mail]]

I did at one point consider !boolean but callion said it was worse...

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

FWIW, I suggested a new attribute to the effect of: prefinverse="true|false"
with false being the default value....

Comment 5

[[e-mail]]

I've come up with a better way of handling this, via GetFields().

Comment 6

[[e-mail]]

Attachment: Use GetFields() and SetFields() to invert the prefs

Because it's now possible to hook into the prefs between the front and back end
using GetFields and SetFields this simplfies the code.

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 121893 [details] [diff] [review]
Use GetFields() and SetFields() to invert the prefs

>Index: pref-scripts.js
>===================================================================

>+function InvertBoolPrefs(aDataObject){
>+  var listitems = document.getElementById('allowScripts').childNodes;
>+  for (var i = 0; i < listitems.length; ++i) {

I know bz checked in a fix some time ago for normal DOM content lists to not
re-iterate all the elements each time .length is called, but I don't think
anything was ever done like that for XUL since it has it's own content model
(ie, doesn't inherit from nsGenericElement).  Could you please double check on
that?  And switch to |var len = listitems.length; for (var i = 0; i <
listitems.length; ++i)| if it's not lazy counting?

>+    var id = listitems[i].id;
>+    aDataObject[id].checked = !aDataObject[id].checked;
>+  }
> }

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 121893 [details] [diff] [review]
Use GetFields() and SetFields() to invert the prefs

Okay, I found out your usage of .length is fine.  r=caillon

Comment 9

[Matthias Versen [:Matti]]

Comment on attachment 107463 [details] [diff] [review]
Proposed patch

removing obsolete review requests

Comment 10

[Alec Flett]

Comment on attachment 121893 [details] [diff] [review]
Use GetFields() and SetFields() to invert the prefs

I've got to be honest, I think the InvertBoolPrefs to be kind of a hack. I much
prefer prefinverse=true (or prefreverse=true)

I don't feel like GetFields and SetFields was not meant to be used this way -
the list of fields is easy to get and set, and to add this odd layer where
you're blindly flippping values in the aDataObject.. you're depending on the
data object being set up in just the right way.. it seems fragile. At the very
least, instead of InvertBoolPrefs, you should have a SetInvertedBoolPrefs() and
GetInvertedBoolPrefs() which retrieve the values from the dom nodes. That would
be far less fragile.. but still the simplest solution in my mind is the
prefinverted=true - frankly I'm amazed we don't have that already.

Comment 11

[[e-mail]]

Attachment: Define a new prefinverse="true" attribute

Comment 12

[Alec Flett]

nice, see how much cleaner that made it? :) I'll let neil review first, but I
love all that code that's been yanked.

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 134632 [details] [diff] [review]
Define a new prefinverse="true" attribute

Great.	I like this much better.  r=caillon

Comment 14

[Alec Flett]

Comment on attachment 134632 [details] [diff] [review]
Define a new prefinverse="true" attribute

sr=alecf

Comment 15

[[e-mail]]

Fix checked in.

Comment 16

[Alec Flett]

Comment on attachment 121893 [details] [diff] [review]
Use GetFields() and SetFields() to invert the prefs

I'm marking this obsolete since we went with a different option.

Comment 17

[Boris Zbarsky [:bzbarsky]]

This caused bug 227623 

Comment 18

[[e-mail]]

Really marking fixed.

Comment 19

[[e-mail]]

Attachment: Make reversed="true" work for <listitem type="checkbox"/>

Comment 20

[[e-mail]]

Comment on attachment 146978 [details] [diff] [review]
Make reversed="true" work for <listitem type="checkbox"/>

So, after all that, it turns out that ordinary checkboxes had this
reversed="true" attribute all along :-[

Comment 21

[[e-mail]]

Comment on attachment 146978 [details] [diff] [review]
Make reversed="true" work for <listitem type="checkbox"/>

caillon gave r= over irc.

Comment 22

[Alec Flett]

Comment on attachment 146978 [details] [diff] [review]
Make reversed="true" work for <listitem type="checkbox"/>

heh. oops. 
sr=alecf

Comment 23

[[e-mail]]

Really fixed this time ;-)