Closed Bug 399031 Opened 13 years ago Closed 12 years ago

Convert xpfe prefpane to toolkit version

Categories

(Other Applications :: DOM Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sdwilsh, Assigned: bugzilla)

References

Details

Attachments

(2 files, 5 obsolete files)

Spun-off from Bug 322245 Comment 8.

Whoever does this needs to make sure they use accesskeys for things!
Flags: blocking-seamonkey2.0a1?
This depends on the first step of bug 394522, which is creation of the main new toolkit prefwindow itself for SeaMonkey. We have a WIP patch there and even a first pre-review, this should be available very soon. Once that part has landed, this panel can be reworked, with the accesskeys from bug 322245 integrated, of course.
Depends on: prefpane_migration
Depends on: 342087
No longer depends on: prefpane_migration
Attached patch Port the DOMI prefpane v1 (obsolete) — Splinter Review
First thanks KaiRo for the ground work. What did I have to do to make it work:

- outsource the JS code to its own file
- load the scripts in the script attribute of prefpane
- call enableBlinkPrefs() in onpaneload, calling it in Startup() is too early, because the preferences aren't initalized at that stage
- I also removed the flex="1" from both groupboxes to prevent the boxes from using all vertical space

Note: the patch from bug 421832 is needed for the DOMI prefpane to appear in the preferences window.
Assignee: nobody → aqualon
Status: NEW → ASSIGNED
Attachment #309287 - Flags: review?(sdwilsh)
Depends on: 421832
Comment on attachment 309287 [details] [diff] [review]
Port the DOMI prefpane v1

oops, forgot to include the access keys, so removing review request. I'll post a new patch tomorrow.
Attachment #309287 - Flags: review?(sdwilsh)
Attached patch Port the DOMI prefpane v2 (obsolete) — Splinter Review
Now with all accesskeys from the patch in bug 322245.

The onpaneload call was wrong, so I removed it.
The install button is working now, I had to insert a Startup() function into pref-sidebar.js that calls the initialization.

The DOMI sidebar isn't working, but I think that's an issue for another bug (if there isn't one already, didn't look).
Attachment #309287 - Attachment is obsolete: true
Attachment #309401 - Flags: superreview?(neil)
Attachment #309401 - Flags: review?(sdwilsh)
Comment on attachment 309401 [details] [diff] [review]
Port the DOMI prefpane v2

>@@ -0,0 +1,16 @@
No licence block?

>+function enableBlinkPrefs(aTruth)
>+{
>+  var els = [
>+    "lbElBorderColor", "cprElBorderColor",
>+    "lbElBorderWidth", "txfElBorderWidth",
>+    "lbElDuration", "txfElDuration",
>+    "lbElSpeed", "txfElSpeed",
>+    "cbElInvert"];
>+
>+  for (var i = 0; i < els.length; ++i) {
>+    if (aTruth)
>+      document.getElementById(els[i]).removeAttribute("disabled");
>+    else
>+      document.getElementById(els[i]).setAttribute("disabled", true);
>+  }
>+}
I guess this was a problem with the old code, rather than a problem with your patch, but a) it doesn't disable when you open the window b) it enables locked items.

>+        <checkbox id="cbElOn" label="&blinkSelectedElement.label;"
>+                  oncommand="enableBlinkPrefs(this.checked)"
This doesn't refresh properly if you change the pref from about:config while the preferences window is open; IIRC to fix that put <preference id="inspector.blink.on" ... onchange="enableBlinkPrefs(this.value);"/>
Attachment #309401 - Flags: superreview?(neil) → superreview+
Just a comment based on Neil's review comments (I hope to get to your review within a week):
s/var/let/

There's actually a performance benefit from using let over var, so for new code I'd like to see the use of let.
Attached patch Port the DOMI prefpane v3 (obsolete) — Splinter Review
Next version of the patch, includes fixes for Neil's comments and contains so much `let´ even Scheme would be proud of ;)
Attachment #309401 - Attachment is obsolete: true
Attachment #309701 - Flags: review?(sdwilsh)
Attachment #309401 - Flags: review?(sdwilsh)
Comment on attachment 309701 [details] [diff] [review]
Port the DOMI prefpane v3

>+++ extensions/inspector/resources/content/prefs/pref-inspector.js	
>+function enableBlinkPrefs(aTruth)
>+{
>+  let els = [
>+    {
>+      "lbElBorderColor": "cprElBorderColor"
>+    },
>+    {
>+      "lbElBorderWidth": "txfElBorderWidth"
>+    },
..
>+  ];
>+
>+  for (let [, entry] in Iterator(els))
>+    for (let [label, control] in Iterator(entry))
Do you think that in the future, you'll want multiple pairs of label/control for each entry? If not, you could get the same effect by doing this:

let els = {
  lbElBorderColor: "cprElBorderColor",
  lbElBorderWidth: "txtElBorderWidth",
..
};
for (let [label, control] in Iterator(entry)) ...

(FYI, you could have used "for each (let entry in els)")
Comment on attachment 309701 [details] [diff] [review]
Port the DOMI prefpane v3

>Index: extensions/inspector/resources/content/prefs/pref-inspector.js
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
can we get the vim modeline here too?  some files in storage have the right example

>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
Really?

>+  let elts = {
A comment explaining this would be really nice

>+  for (let [, entry] in Iterator(els))
why not |for each (let entry in els)|?
nit: also, everywhere else in inspector we have the brace on the same line as the for statement.  I'm rather in favor of keeping that.

>+      var controlElem = document.getElementById(control);
s/var/let/

>+      if (aTruth && !isPrefLocked(controlElem))
>+      {
nit: same thing with bracing here

>+        if(label)
nit: |if (label)|

>+      }
>+      else
>+      {
nit: like browser and toolkit style rules please
}
else {

>+  if(!elem.hasAttribute("preference"))
nit: space after if

>Index: extensions/inspector/resources/content/prefs/pref-inspector.xul
>+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+  <prefpane id="inspector_pane"
nit: I'm much more of a fan of hyphens instead of underscores in id's

>+    <preferences id="inspector_preferences">
ditto

>+      <!-- preference id="inspector.hooks.navigator"
>+                  name="inspector.hooks.navigator"
>+                  type="bool"/ -->
why is this commented out?

Also not sure if we need the RDFU.js file for this - but not a big deal either way.

r=sdwilsh with comments addressed
Attachment #309701 - Flags: review?(sdwilsh)
(In reply to comment #9)
> (From update of attachment 309701 [details] [diff] [review])
> >Index: extensions/inspector/resources/content/prefs/pref-inspector.xul
> >+  <prefpane id="inspector_pane"
> nit: I'm much more of a fan of hyphens instead of underscores in id's
In SeaMonkey, we prefer the underscore for the pref ids. KaiRo wanted to talk about it on IRC.

> >+      <!-- preference id="inspector.hooks.navigator"
> >+                  name="inspector.hooks.navigator"
> >+                  type="bool"/ -->
> why is this commented out?
That's the case since end of 2001. I'm not sure if the feature ever worked.

> Also not sure if we need the RDFU.js file for this - but not a big deal either
> way.
That's needed in pref-sidebar.js 

I'll post a patch with all other changes soon, that's ready for checkin (subject to the clarification of the hyphen vs. underscore issue).
Attached patch Port the DOMI prefpane v4 (obsolete) — Splinter Review
Attachment #309701 - Attachment is obsolete: true
I don't care enough about the hyphen/underscore issue since this is a seamonkey only file.
Attachment #310362 - Flags: review?(sdwilsh)
I probably won't be able to get to the review on this until after April 15.  Let me know if you need it sooner and I'll try to squeeze it in.
No problem, bug 421832 has to be fixed anyway before that patch is useful.
Comment on attachment 310362 [details] [diff] [review]
Port the DOMI prefpane v4

Sorry this took so long.  r=sdwilsh, but I'd like Neil to take one more look.
Attachment #310362 - Flags: superreview?(neil)
Attachment #310362 - Flags: review?(sdwilsh)
Attachment #310362 - Flags: review+
Attached patch Port the DOMI prefpane v5 (obsolete) — Splinter Review
Taking over r+ from sdwilsh.

Patch uses Startup() function instead of onpaneload.

Note: it only works, when the pref-inspector.js is declared as last file in the script attribute of the prefpane. In any other case Startup() wasn't called when opening the prefwindow.
Attachment #310362 - Attachment is obsolete: true
Attachment #324972 - Flags: superreview?(neil)
Attachment #324972 - Flags: review+
Attachment #310362 - Flags: superreview?(neil)
Comment on attachment 324972 [details] [diff] [review]
Port the DOMI prefpane v5

>+function Startup()
>+{
>+  enableBlinkPrefs(document.getElementById("inspector.blink.on").value);
>+}
...
>+function Startup()
>+{
>+  SidebarPrefs_initialize();
>+}
This won't work, you can only have one Startup()! Just put both lines in it.

>+            script="chrome://inspector/content/prefs/pref-sidebar.js
>+                    chrome://inspector/content/extensions/wsm-colorpicker.js
>+                    chrome://inspector/content/jsutil/xpcom/XPCU.js
>+                    chrome://inspector/content/jsutil/rdf/RDFU.js
>+                    chrome://inspector/content/prefs/pref-inspector.js">
You can put these back into the original order now :-)

>+      <preference id="inspector.blink.on"
>+                  name="inspector.blink.on"
>+                  type="bool"
>+                  onchange="enableBlinkPrefs(this.value);"/>
...
>+        <checkbox id="cbElOn" label="&blinkSelectedElement.label;"
>+                  oncommand="enableBlinkPrefs(this.checked)"
>+                  preference="inspector.blink.on"
>+                  accesskey="&blinkSelectedElement.accesskey;"/>
You don't need this oncommand any more.
Attachment #324972 - Flags: superreview?(neil) → superreview+
Having two Startup() functions doesn't work, so I moved the functionality from the Startup() in pref-sidebar.js into pref-inspector.js' Startup().
Attachment #324972 - Attachment is obsolete: true
Attachment #324985 - Flags: superreview?(neil)
Attachment #324985 - Flags: review+
Comment on attachment 324985 [details] [diff] [review]
Port the DOMI prefpane v6

I overlooked that Neil already gave sr+ and since I addressed his comments from IRC, the patch is ready for checkin.
Attachment #324985 - Flags: superreview?(neil) → superreview+
> >+            script="chrome://inspector/content/prefs/pref-sidebar.js
> >+                    chrome://inspector/content/extensions/wsm-colorpicker.js
> >+                    chrome://inspector/content/jsutil/xpcom/XPCU.js
> >+                    chrome://inspector/content/jsutil/rdf/RDFU.js
> >+                    chrome://inspector/content/prefs/pref-inspector.js">
> You can put these back into the original order now :-)
Apart from this one ;-)
(In reply to comment #20)
> Apart from this one ;-)
yeah...
Attachment #325086 - Flags: superreview+
Attachment #325086 - Flags: review+
Keywords: checkin-needed
checked into both mozilla-central and cvs trunk, leaving to Bruno to mark fixed.
Keywords: checkin-needed
Comment on attachment 325086 [details] [diff] [review]
Port the DOMI prefpane v7 (ready for checkin)

>Index: extensions/inspector/resources/content/prefs/pref-inspector.xul
>===================================================================
>-              <label id="lbElBorderWidth" value="&borderWidth.label;" control="txfElBorderWidth"/>
>+              <label id="lbElBorderWidth" control="txfElBorderWidth"
>+                     value="&borderWidth.label;"
>+                     accesskey="&borderWidth.accesskey;"/>
>               <hbox align="center">
>                 <textbox id="txfElBorderWidth" style="width: 4em"
>-                  preftype="int" prefstring="inspector.blink.border-width" prefattribute="value"/>
>+                         preference="inspector.blink.border-width"/>
>                 <label value="&px.label;"/>
You might want to use aria-labelledby, MarcoZ could advise. Similarly for the other ones.
(In reply to comment #23)
> You might want to use aria-labelledby, MarcoZ could advise. Similarly for the
> other ones.
New bug please.

Resolving this bug - and it made 2.0.1.
Blocks: 420105
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: blocking-seamonkey2.0a1?
Depends on: 466223
You need to log in before you can comment on or make changes to this bug.