make EnableTextbox() available in all prefpanes

RESOLVED FIXED in seamonkey2.0a1

Status

SeaMonkey
Preferences
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Bruno 'Aqualon' Escherl, Assigned: Bruno 'Aqualon' Escherl)

Tracking

Trunk
seamonkey2.0a1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

5.77 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
EnableTextbox(aTextboxId, aValue, aFocus) can be used in all prefpanes, where a checkbox is used to enable/disable a textbox, so it's useful to have it available everywhere and don't have redundant code.
(Assignee)

Comment 1

10 years ago
Created attachment 316423 [details] [diff] [review]
patch v1

Adds a new preferences.js and loads its content into preferences.xul.
Assignee: nobody → aqualon
Status: NEW → ASSIGNED
Attachment #316423 - Flags: superreview?(neil)
Attachment #316423 - Flags: review?(iann_bugzilla)

Comment 2

10 years ago
Comment on attachment 316423 [details] [diff] [review]
patch v1

Some uncalled for nits: ;-)

>Index: suite/common/pref/preferences.js
>===================================================================
>+// The contents of this file will be loaded into the scope of the
>+// prefwindow and is available in all prefpanes!

"content ... be available to ..."
But IanN and Neil should know even better. ;-)

>+function EnableTextbox(aTextboxId, aValue, aFocus)

I'd rename aValue to aCheckboxValue to make its use more clear.

>+  let enable = aValue && !pref.locked;

I'd use "enabled", because it's a state.

>Index: suite/common/pref/preferences.xul
>===================================================================
>   <stringbundle id="bundle_prefutilities"
>                 src="chrome://communicator/locale/pref/prefutilities.properties"/>
> 
>+  <script type="application/x-javascript" src="chrome://communicator/content/pref/preferences.js"/>
>+

The <script> should be the first child.

Comment 3

10 years ago
Comment on attachment 316423 [details] [diff] [review]
patch v1

r=me with mnyromyr's nits fixed
Attachment #316423 - Flags: review?(iann_bugzilla) → review+

Comment 4

10 years ago
Comment on attachment 316423 [details] [diff] [review]
patch v1

I don't quite agree with Mnyromyr!

>+// The contents of this file will be loaded into the scope of the
>+// prefwindow and is available in all prefpanes!
I'd swap things so you get "is loaded" and "will be available".

>+function EnableTextbox(aTextboxId, aValue, aFocus)
I'd call it aEnable rather than aCheckbox.
Attachment #316423 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 5

10 years ago
Created attachment 316808 [details] [diff] [review]
patch v2

Patch, addressing the review comments (I took Neil's suggestions).
Attachment #316423 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed

Comment 6

10 years ago
Landed v2 on trunk.
Keywords: checkin-needed
(Assignee)

Comment 7

10 years ago
Thx for landing -> RESOLVED FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.