339728 - Sync xpfe/tookit config.xul and config.js

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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

[Mike Connor [:mconnor]]

Yes, 5 sounds good.

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Add int-checking to toolkit

Untested - I don't build Firefox.

Comment 3

[[e-mail]]

(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?

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(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.

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Add an id to the hbox, explicitly save the pref file (checked in on trunk & 1.8)

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

The "prefwindow stuff" is "testing that the pref window correctly deals with locked prefs".

Comment 8

[Robert Kaiser]

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

Comment 9

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

> 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

[[e-mail]]

(In reply to comment #9)
>both versions would need to be preprocessed
Why?

Comment 11

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(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.

Comment 12

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 223831 [details] [diff] [review]
Add int-checking to toolkit

Trying another reviewer after >6 mos...

Comment 13

[Asaf Romano (gone)]

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 ;)

Comment 14

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Add int-checking to toolkit

Comment 15

[Asaf Romano (gone)]

Comment on attachment 247492 [details] [diff] [review]
Add int-checking to toolkit

r=mano.

Comment 16

[Asaf Romano (gone)]

I'm not sure how can we unit-test this.

Comment 17

[Robert Sayre]

(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.

Comment 18

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

> 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.

Comment 19

[[e-mail]]

(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.