Closed
Bug 339728
Opened 19 years ago
Closed 18 years ago
Sync xpfe/tookit config.xul and config.js
Categories
(SeaMonkey :: UI Design, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: csthomas, Assigned: csthomas)
References
Details
(Whiteboard: [cst: 1f 2? 3? 4f 5f 6f])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
neil
:
review+
neil
:
superreview+
kairo
:
approval-seamonkey1.1a+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
1. Toolkit adds an id to the hbox containing the filter search box.
2. XPFE sets the tree's class to "plain focusring", toolkit doesn't set a class.
3. Toolkit allows locking/unlocking prefs.
4. XPFE has a title attribute on the window, Toolkit sets document.title in onConfigLoad()
5. XPFE verifies that values entered for int prefs are really ints
6. Toolkit writes out the pref file explicitly
Do we want to pick up 1, 3, 6? What's the story on 2? Which is better for 4?
Toolkit probably wants 5. mconnor?
Comment 1•19 years ago
|
||
Yes, 5 sounds good.
Assignee | ||
Comment 2•19 years ago
|
||
Untested - I don't build Firefox.
Comment 3•19 years ago
|
||
(In reply to comment #0)
>Do we want to pick up 1, 3, 6?
1. Don't mind 3. Scary 6. Don't mind
>What's the story on 2?
XPFE accessibility has us draw a border on the tree body when it is focused.
Toolkit accessibility only forces a current item.
>Which is better for 4?
XPFE of course. Need you ask?
Assignee | ||
Comment 4•19 years ago
|
||
(In reply to comment #3)
> (In reply to comment #0)
> >Do we want to pick up 1, 3, 6?
> 1. Don't mind 3. Scary 6. Don't mind
I'll do 1 and 6 in a bit.
> >What's the story on 2?
> XPFE accessibility has us draw a border on the tree body when it is focused.
> Toolkit accessibility only forces a current item.
mconnor, do you want the border on the tree body as well? I see ben removed it in revision 1.5...
> >Which is better for 4?
> XPFE of course. Need you ask?
Just being polite ;). Anyway, standard8 fixed toolkit's version in bug 336894.
Depends on: 336894
Assignee | ||
Comment 5•19 years ago
|
||
Attachment #224277 -
Flags: review?(neil)
Assignee | ||
Comment 6•19 years ago
|
||
Neil, would it be ok to add pref locking in a |#ifdef debug|? mconnor said it's useful for some prefwindow stuff they do in toolkit.
Blocks: 339720
Comment 7•19 years ago
|
||
The "prefwindow stuff" is "testing that the pref window correctly deals with locked prefs".
Updated•19 years ago
|
Attachment #224277 -
Flags: review?(neil) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #224277 -
Flags: approval-seamonkey1.1a?
Assignee | ||
Updated•19 years ago
|
Attachment #224277 -
Flags: superreview?(neil)
Updated•19 years ago
|
Attachment #224277 -
Flags: superreview?(neil) → superreview+
Comment 8•19 years ago
|
||
Comment on attachment 224277 [details] [diff] [review]
Add an id to the hbox, explicitly save the pref file (checked in on trunk & 1.8)
a=me for SeaMonkey 1.1
Attachment #224277 -
Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
Assignee | ||
Updated•19 years ago
|
Whiteboard: [cst: 1f 2p? 3p? 4f 5r? 6f]
Assignee | ||
Updated•19 years ago
|
Attachment #224277 -
Attachment description: Add an id to the hbox, explicitly save the pref file → Add an id to the hbox, explicitly save the pref file (checked in on trunk & 1.8)
Assignee | ||
Comment 9•19 years ago
|
||
> 2. XPFE sets the tree's class to "plain focusring", toolkit doesn't set a
> class.
> 3. Toolkit allows locking/unlocking prefs.
For 3, would a #if DEBUG be ok (allow lock/unlock only in debug builds, which is where people would be testing prefwindow's handling of them)? Then we could address 2 with another #if to make it SeaMonkey-only (since both versions would need to be preprocessed).
Comment 10•19 years ago
|
||
(In reply to comment #9)
>both versions would need to be preprocessed
Why?
Assignee | ||
Comment 11•19 years ago
|
||
(In reply to comment #10)
> (In reply to comment #9)
> >both versions would need to be preprocessed
> Why?
>
If they're the same, both would have the #if DEBUG, and the xpfe version would need the focusring class.
Assignee | ||
Comment 12•18 years ago
|
||
Comment on attachment 223831 [details] [diff] [review]
Add int-checking to toolkit
Trying another reviewer after >6 mos...
Attachment #223831 -
Flags: review?(mconnor) → review?(mano)
Comment 13•18 years ago
|
||
Comment on attachment 223831 [details] [diff] [review]
Add int-checking to toolkit
>Index: toolkit/components/viewconfig/content/config.js
>===================================================================
>@@ -553,11 +553,20 @@ function ModifyPref(entry)
> var result = { value: entry.valueCol };
> var dummy = { value: 0 };
> if (!gPromptService.prompt(window, title, entry.prefCol, result, null, dummy))
> return false;
> if (entry.typeCol == nsIPrefBranch.PREF_INT) {
>- gPrefBranch.setIntPref(entry.prefCol, parseInt(result.value, 10));
>+ // | 0 converts to integer or 0; - 0 to float or NaN.
>+ // Thus, this check should catch all cases.
>+ var val = result.value | 0;
>+ if (val != result.value - 0) {
>+ var err_title = gConfigBundle.getString("nan_title");
>+ var err_text = gConfigBundle.getString("nan_text");
>+ gPromptService.alert(window, err_title, err_text);
You should copy over the strings as well ;)
Attachment #223831 -
Flags: review?(mano) → review-
Assignee | ||
Comment 14•18 years ago
|
||
Attachment #223831 -
Attachment is obsolete: true
Attachment #247492 -
Flags: review?(mano)
Comment 15•18 years ago
|
||
Comment on attachment 247492 [details] [diff] [review]
Add int-checking to toolkit
r=mano.
Attachment #247492 -
Flags: review?(mano) → review+
Comment 17•18 years ago
|
||
(In reply to comment #16)
> I'm not sure how can we unit-test this.
>
recapping IRC conversations: this looks pretty testable via the XPCShell harness, and loading the script in. All the functions that touch the prefs but not the document should be easy to test.
Assignee | ||
Updated•18 years ago
|
Whiteboard: [cst: 1f 2p? 3p? 4f 5r? 6f] → [cst: 1f 2? 3? 4f 5f 6f]
Assignee | ||
Comment 18•18 years ago
|
||
> 2. XPFE sets the tree's class to "plain focusring", toolkit doesn't set a
> class.
> 3. Toolkit allows locking/unlocking prefs.
...are not fixed. The rest are. #2 may be an issue, but it's probably affecting suiterunner in more places than this now. Maybe SM should ditch the focusring everywhere to match toolkit...
Resolving since we switched to toolkit.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 19•17 years ago
|
||
(In reply to comment #18)
> > 2. XPFE sets the tree's class to "plain focusring", toolkit doesn't set a
> > class.
> > 3. Toolkit allows locking/unlocking prefs.
> ...are not fixed.
The focusring was fixed in bug 339720, and the pref locking/unlocking was removed in bug 289136.
Updated•11 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•