Closed Bug 181973 Opened 22 years ago Closed 20 years ago

Many pref panels need to reverse the sense of boolean prefs

Categories

(SeaMonkey :: Preferences, defect)

x86
Windows 95
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

Details

(Keywords: polish)

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Proposed patch (obsolete) — Splinter Review
Create a preftype of "inverse" which is the inverse of the "boolean" preftype.
Attachment #107463 - Flags: review?(ben)
Keywords: patch, polish, review
Attachment #107463 - Flags: superreview?(jaggernaut)
I like the idea, but the name "inverse" seems too generic. Up to Ben though,
it's his code.
I did at one point consider !boolean but callion said it was worse...
FWIW, I suggested a new attribute to the effect of: prefinverse="true|false"
with false being the default value....
I've come up with a better way of handling this, via GetFields().
Because it's now possible to hook into the prefs between the front and back end
using GetFields and SetFields this simplfies the code.
Attachment #107463 - Attachment is obsolete: true
Attachment #121893 - Flags: superreview?(jaggernaut)
Attachment #121893 - Flags: review?(caillon)
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 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
Attachment #121893 - Flags: review?(caillon) → review+
Comment on attachment 107463 [details] [diff] [review]
Proposed patch

removing obsolete review requests
Attachment #107463 - Flags: superreview?(jag)
Attachment #107463 - Flags: review?(bugs)
Attachment #121893 - Flags: superreview?(jag) → superreview?(alecf)
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.
Attachment #134632 - Flags: review?(caillon)
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 on attachment 134632 [details] [diff] [review]
Define a new prefinverse="true" attribute

Great.	I like this much better.  r=caillon
Attachment #134632 - Flags: review?(caillon) → review+
Attachment #134632 - Flags: superreview?(alecf)
Comment on attachment 134632 [details] [diff] [review]
Define a new prefinverse="true" attribute

sr=alecf
Attachment #134632 - Flags: superreview?(alecf) → superreview+
Fix checked in.
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.
Attachment #121893 - Attachment is obsolete: true
Attachment #121893 - Flags: superreview?(alecf)
This caused bug 227623 
Really marking fixed.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Assignee: bugs → neil.parkwaycc.co.uk
Status: RESOLVED → ASSIGNED
Resolution: FIXED → ---
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 :-[
Attachment #146978 - Attachment description: Make reversed="true" work → Make reversed="true" work for <listitem type="checkbox"/>
Attachment #146978 - Flags: superreview?(alecf)
Attachment #146978 - Flags: review?(caillon)
Comment on attachment 146978 [details] [diff] [review]
Make reversed="true" work for <listitem type="checkbox"/>

caillon gave r= over irc.
Comment on attachment 146978 [details] [diff] [review]
Make reversed="true" work for <listitem type="checkbox"/>

heh. oops. 
sr=alecf
Attachment #146978 - Flags: superreview?(alecf) → superreview+
Really fixed this time ;-)
Status: ASSIGNED → RESOLVED
Closed: 21 years ago20 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: