342087 - extended toolkit <prefwindow> for SeaMonkey preferences

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Justin Wood (:Callek)]

My goal is to extend the toolkit prefwindow binding to appear just like the current SeaMonkey preferences display, but use the <preference> <prefwindow> <prefpane> etc elements, exactly like they are used in toolkit.

The way I implement for SeaMonkey should allow the use of [xpfe="false"] to use the toolkit prefwindow should an extension author wish.

This work is not blocked by suiterunner, but is targetted only at SeaMonkey 1.5, though work could be done to see if it is feasable in SeaMonkey 1.1

Comment 1

[Justin Wood (:Callek)]

Attachment: work in progress

this is an (obviously) not ready for review version of what I am working on.

This is for anyone _really_ interested in the code involved, thus far.

Do note, that this is before any and all cleanup I will do on the binding file itself, and without the code I am using to test this.

Comment 2

[[e-mail]]

Attachment: start to sync xpfe and toolkit WSM [checked in]

Comment 3

[Justin Wood (:Callek)]

Comment on attachment 227535 [details] [diff] [review]
start to sync xpfe and toolkit WSM [checked in]

Note, that the WSM will not be needed with the binding based off of toolkit's prefwindow, while I do agree that getting the WSM working again with our current PrefWindow may not be a bad idea, since we can then actually use Preferences in SuiteRunner before my work is complete.

Comment 4

[jag (Peter Annema)]

Justin: Neil's patch (white-space changes, s/'/"/ in a few places, replacing [] with |new Object|) is the first step in making the xpfe WSM look more like the toolkit one so we can more easily see what the differences are and what bug fixes and/or features we might have to port from xpfe's to toolkit's. I asked him to do that as part of this bug instead of creating a new bug for it.

Comment 5

[[e-mail]]

Attachment: Interim fix to get xpfe prefs to work with toolkit WSM

xpfe and toolkit have a slightly different page data format.
Calling getPageData will create one in the correct format.

This should at least make preferences usable until we convert to prefwindow.

Sorry for littering this bug.

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> Created an attachment (id=227684) [edit]
> Interim fix to get xpfe prefs to work with toolkit WSM

This doesn't get it fully working. On suiterunner I get "*** Failed to create prefs object" and then I can't set any prefs (they appear as set, you just can't change them). I think this is probably the culprit: http://lxr.mozilla.org/seamonkey/source/suite/common/pref/nsPrefWindow.js#101

Comment 7

[[e-mail]]

Attachment: Make saving prefs work in suiterunner too :-) [checked in]

Comment 8

[Justin Wood (:Callek)]

Attachment: Extend Toolkit prefwindow

This is merely the binding itself, most of the work here was implementing nsITreeView.

I can answer any questions on this if needed.

Also can provide a "test" XULRunner environ for this, (with a pane, and a few examples, etc.)

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 228104 [details]
Extend Toolkit prefwindow

><?xml version="1.0"?>
><!DOCTYPE bindings SYSTEM "chrome://global/locale/preferences.dtd">
><bindings id="seamonkeypreferencesBindings" xmlns="http://www.mozilla.org/xbl" xmlns:xbl="http://www.mozilla.org/xbl" xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>
><binding id="prefwindow" extends="chrome://global/content/bindings/preferences.xml#prefwindow">
>  <resources>
>    <stylesheet src="chrome://global/skin/preferences.css"/>
>  </resources>
>  <content dlgbuttons="accept,cancel" persist="lastSelected screenX screenY" closebuttonlabel="Close" closebuttonaccesskey="C" xmlns:xhtml2="http://www.w3.org/TR/xhtml2" xmlns:wairole="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#" xhtml2:role="wairole:dialog" title="Preferences">
>    <xul:radiogroup anonid="selector" orient="horizontal" class="paneSelector chromeclass-toolbar" xhtml2:role="wairole:list" style="display: none;"/>
>    <xul:hbox flex="1" class="paneDeckContainer">
>      <xul:listbox anonid="listboxSelector" style="display: none; width: 7em;"/>
>      <xul:tree style="width: 13em;" hidecolumnpicker="true" seltype="single" anonid="smPrefTree" onselect="this.view.selectionChanged();">
>        <xul:treecols>
>          <xul:treecol id="column" flex="1" primary="true" hideheader="true"/>
>        </xul:treecols>
>        <xul:treechildren/>
>      </xul:tree>
>      <xul:deck anonid="paneDeck" flex="1">
>        <children includes="prefpane"/>
>      </xul:deck>
>    </xul:hbox>
>    <xul:hbox anonid="dlg-buttons" class="prefWindow-dlgbuttons" pack="end">
>      <xul:button dlgtype="extra2" class="dialog-button" hidden="true"/>
>      <xul:spacer anonid="spacer" flex="1"/>
>      <xul:button dlgtype="accept" class="dialog-button" icon="accept"/>
>      <xul:button dlgtype="extra1" class="dialog-button" hidden="true"/>
>      <xul:button dlgtype="cancel" class="dialog-button" icon="cancel"/>
>      <xul:button dlgtype="help" class="dialog-button" hidden="true" icon="help"/>
>      <xul:button dlgtype="disclosure" class="dialog-button" hidden="true"/>
>    </xul:hbox>
>    <xul:hbox>
>      <children/>
>    </xul:hbox>
>  </content>
>  <implementation>
>    <constructor>
>      <![CDATA[
>        this.setAttribute("animate", !this._shouldAnimate ? "off" : "on");
>        if (this.type != "child") {
>          var psvc = Components.classes["@mozilla.org/preferences-service;1"]
>                               .getService(Components.interfaces.nsIPrefBranch);
>          this.instantApply = psvc.getBoolPref("browser.preferences.instantApply");
>          if (this.instantApply) {
>            var docElt = document.documentElement;
>            var acceptButton = docElt.getButton("accept");
>            acceptButton.hidden = true;
>            var cancelButton  = docElt.getButton("cancel");
>            // morph the Cancel button into the Close button
>            cancelButton.setAttribute ("icon", "close");
>            cancelButton.label = docElt.getAttribute("closebuttonlabel");
>            cancelButton.accesskey = docElt.getAttribute("closebuttonaccesskey");
>          }
>        }
>        var panes = this.preferencePanes;
>
>        var mytree = document.getAnonymousElementByAttribute(this, 'anonid', 'smPrefTree');
>        var myView = this.createView();        //odd syntax to use
>        myView.wrappedJSObject = myView;   //wrappedJSObject correctly
>        mytree.view = myView;
>
>        var lastPane = null;
>        if (this.lastSelected) {
>          lastPane = document.getElementById(this.lastSelected);
>          if (!lastPane) {
>            this.lastSelected = null;
>          }
>        }
>
>        var paneToLoad;
>        if ("arguments" in window && window.arguments[0] && document.getElementById(window.arguments[0]))
>          paneToLoad = document.getElementById(window.arguments[0]);
>        else if (lastPane)
>          paneToLoad = lastPane;
>        else
>          paneToLoad = panes[0];
>
>        for (var i = 0; i < panes.length; ++i) {
>          this._makePaneButton(panes[i]);
>          if (panes[i].loaded) {
>            // Inline pane content, fire load event to force initialization.
>            this._fireEvent("paneload", panes[i]);
>          }
>        }
>        this.showPane(paneToLoad);
>        myView.selectPane(paneToLoad);
>
>        if (panes.length == 1)
>          this._selector.setAttribute("collapsed", "true");
>      ]]>
>      </constructor>
>
>      <!-- Implement nsITreeView here for our tree Impl. -->
>      <method name="createView">
>        <body>
>        <![CDATA[
>          return ({
>            rowCount: 0,
>            getRowProperties: function(row,props){},
>            getCellProperties: function(row,col,props){},
>            getColumnProperties: function(colid,col,props){},
>            isContainer: function(row){
>              return this.getPaneObject(row).isContainer;
>            },
>            isContainerOpen: function(row){
>              return this.getPaneObject(row).isContOpen;
>            },
>            isContainerEmpty: function(row){
>              return this.getPaneObject(row).isContEmpty;
>            },
>            isSeparator: function(row){
>              return false;
>            },
>            isSorted: function(){
>              return false;
>            },
>            canDrop: function(row,orientation){
>              return false;
>            },
>            drop: function(row,orientation){},
>            getParentIndex: function(row) {
>              return this.mLiveTree[row].parentIdx;
>            },
>            hasNextSibling: function(parentIndex, index) {
>              var obj = this.getPaneObject(this.mLiveTree[parentIndex].parentIdx);
>              return (this.mLiveTree[parentIndex].ObjTreeIdx
>                         != obj.children.length - 1)
>              return true;
>            },
>            getLevel: function(row){
>              return this.mLiveTree[row].level;
>            },
>            getImageSrc: function(row,column){
>              return "";
>            },
>            getCellValue: function(row,column){},
>            getCellText: function(row,column){
>              return this.getPaneObject(row).label;
>            },
>            setTree: function(treebox){ this.treebox = treebox; },
>            toggleOpenState: function(row){
>              var objectToToggle = this.getPaneObject(row);
>              if (objectToToggle.isContOpen){
>                objectToToggle.isContOpen = false;
>                this.mLiveTree.splice(row + 1, objectToToggle.children.length);
>                this.rowCount = this.mLiveTree.length - 1;
>                this.treebox.rowCountChanged( row + 1,
>                                             (-1)*objectToToggle.children.length);
>              }else{
>                objectToToggle.isContOpen = true;
>                var liveTreeChildren = new Array();
>                for (var i = 0; i < objectToToggle.children.length; i++){
>                  this.mLiveTree.splice( row + i + 1, 0, {
>                        ObjTreeIdx: i,
>                        parentIdx: row,
>                        level: 1
>                      });
>                }
>                this.rowCount = this.mLiveTree.length - 1;
>                this.treebox.rowCountChanged(row + 1, i);
>              }
>            },
>            cycleHeader: function(col){},
>            selectionChanged: function() {
>              paneId = this.getPaneObject(this.selection.currentIndex).paneid;
>              this.mPrefBinding.showPane( document.getElementById(paneId) );
>            },
>            cycleCell: function(row,col){},
>            isEditable: function(row,col){ return false; },
>            isSelectable: function(rowm,col){ return true; },
>            setCellValue: function(row,col,value){},
>            setCellText: function(row,col,value){},
>            performAction: function(action){},
>            performActionOnRow: function(action,row){},
>            performActionOnCell: function(action,row,col){},
>            
>            /* Stuff Internal to our Impl */
>            mPrefBinding: this,
>            mLiveTree: new Array(),
>            mPaneObjectTree: new Array(),
>            addToPaneTree: function(paneObj){
>              if (paneObj.parent){
>                for each (var loadedPaneObj in this.mPaneObjectTree){
>                  if (loadedPaneObj.paneid != paneObj.parent)
>                    continue;
>                  if (typeof loadedPaneObj.children == 'undefined'){
>                    loadedPaneObj.children = new Array();
>                  }
>                  var objectTreeRef = loadedPaneObj.children.push(paneObj);
>                  loadedPaneObj.isContainer = true;
>                  loadedPaneObj.isContEmpty = false;
>                }
>              } else {
>                var objectTreeRef = this.mPaneObjectTree.push({
>                  isContainer: false,
>                  isContOpen: false,
>                  isContEmpty: true,
>                  label: paneObj.label,
>                  parent: paneObj.parent,
>                  paneid: paneObj.paneid
>                });
>                var liveTreeRef = this.mLiveTree.push({
>                  ObjTreeIdx: objectTreeRef - 1,
>                  parentIdx: -1,
>                  level: 0
>                });
>                this.treebox.rowCountChanged(this.rowCount++,1);
>              }
>            },
>            selectPane: function(aPaneElement){
>              var paneObjToSelect;
>              for each (var currPaneObj in this.mPaneObjectTree){
>                if (currPaneObj.paneid == aPaneElement.id){
>                  paneObjToSelect = currPaneObj;
>                  break;
>                }
>                if (typeof currPaneObj.children == 'undefined')
>                  continue;
>                for each (var childPaneObj in  currPaneObj.children){
>                  if (childPaneObj.paneid != aPaneElement.id)
>                    continue;
>                  paneObjToSelect = childPaneObj;
>                  break;
>                }
>                if (typeof paneObjToSelect != 'undefined'){
>                  if (currPaneObj.isContOpen)
>                    break;
>                  for (var row=0; row < this.mLiveTree.length; row++){
>                    if (this.getPaneObject(row) != currPaneObj)
>                      continue;
>                    this.toggleOpenState(row);
>                    break;
>                  }
>                  break;
>                }
>              }
>              for (var row=0; row < this.mLiveTree.length; row++){
>                if (this.getPaneObject(row) != paneObjToSelect)
>                  continue;
>                this.selection.select(row);
>              }
>            },                  
>            getPaneObject: function(row){
>              if (this.mLiveTree[row].level != 0){
>                parentObj = this.getPaneObject(this.mLiveTree[row].parentIdx);
>                return parentObj.children[this.mLiveTree[row].ObjTreeIdx];
>              } else {
>                return this.mPaneObjectTree[this.mLiveTree[row].ObjTreeIdx];
>              }
>            }
>          });
>        ]]>
>        </body>
>      </method>
>      
>      <method name="_makePaneButton">
>        <parameter name="aPaneElement"/>
>        <body>
>        <![CDATA[
>          var radio = document.createElement("radio");
>          radio.setAttribute("pane", aPaneElement.id);
>          radio.setAttribute("label", aPaneElement.label);
>          // Expose preference group choice to accessibility APIs as an unchecked list item
>          // The parent group is exposed to accessibility APIs as a list
>          if (aPaneElement.image)
>            radio.setAttribute("src", aPaneElement.image);
>          radio.style.listStyleImage = aPaneElement.style.listStyleImage;
>          this._selector.appendChild(radio);
>          
>          mytree = document.getAnonymousElementByAttribute(this, 'anonid', 'smPrefTree');
>          mytree.view.wrappedJSObject.addToPaneTree(
>              {
>                  label: aPaneElement.label,
>                  paneid: aPaneElement.id,
>                  parent: aPaneElement.getAttribute('parentpane')
>              }
>          );
>          return radio;
>       ]]>
>      </body>
>      </method>
>      
>      <!-- Disable Animation -->
>      <property name="_shouldAnimate">
>        <getter>false</getter>
>      </property>
>      
>    </implementation>
>  </binding>
></bindings>
><!-- ***** BEGIN LICENSE BLOCK *****
>   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
>   -
>   - The contents of this file are subject to the Mozilla Public License Version
>   - 1.1 (the "License"); you may not use this file except in compliance with
>   - the License. You may obtain a copy of the License at
>   - http://www.mozilla.org/MPL/
>   -
>   - Software distributed under the License is distributed on an "AS IS" basis,
>   - WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>   - for the specific language governing rights and limitations under the
>   - License.
>   -
>   - The Original Code is mozilla.org code.
>   -
>   - The Initial Developer of the Original Code is
>   - the mozilla.org SeaMonkey project.
>   - Portions created by the Initial Developer are Copyright (C) 2006
>   - the Initial Developer. All Rights Reserved.
>   -
>   - Contributor(s):
>   -  Justin Wood <[e-mail]>
>   -
>   - Alternatively, the contents of this file may be used under the terms of
>   - either the GNU General Public License Version 2 or later (the "GPL"), or
>   - the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>   - in which case the provisions of the GPL or the LGPL are applicable instead
>   - of those above. If you wish to allow use of your version of this file only
>   - under the terms of either the GPL or the LGPL, and not to allow others to
>   - use your version of this file under the terms of the MPL, indicate your
>   - decision by deleting the provisions above and replace them with the notice
>   - and other provisions required by the LGPL or the GPL. If you do not delete
>   - the provisions above, a recipient may use your version of this file under
>   - the terms of any one of the MPL, the GPL or the LGPL.
>   -
>   - ***** END LICENSE BLOCK ***** -->

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 228104 [details]
Extend Toolkit prefwindow

I have found a few *slight* problems with this version of the binding.

I will attach it as a real patch with stuff to tie it in to SeaMonkey's build system directly next actually, and request review on that lot. Not much will change before then, so IanN if you want to get a head start on the review, feel free to look here.

Comment 11

[Karsten Düsterloh]

1. One of the key differences between an XPFE prefwindow and a toolkit on is the ability to have prefpane hierarchies in XPFE, which you are trying to simulate by introducing the 'parentpane' attribute on an otherwise flat prefpane structure:
  <prefwindow>
    <prefpane id="a"/>
    <prefpane id="b"/>
    <prefpane id="c" parentpane="a"/>
  </prefwindow>
Despite all the nastiness you already had to work around (like checking for cyclic references), this is also hurting the structural logic a lot - after all, XUL is XML, we can _have_ hierarchical elements!
So, I toyed around with real hierarchical prefpanes like
  <prefwindow>
    <prefpane id="a">
      <prefpane id="c"/>
    </prefpane>
    <prefpane id="b"/>
  </prefwindow>
to see what toolkit thinks of this.
Well, toolkit's not amused. ;-)
But that's merely a problem of preferences.xml's
   <property name="preferencePanes"
             onget="return this.getElementsByTagName('prefpane');"/>
and the assumption of <method name="_selectPane"> that these prefpanes are all on the top level.
So, given that only SM will use hierarchical prefpanes anyway, we could just ignore that issue - only if we would show toolkit's horizontal prefpane selector strip, we had to overwrite _selectPane. Anyone using SM as a XULRunner-like app base wouldn't add to the SM prefs and won't see any issues with a plain toolkit prefwindow.

2. What specific problems did you have that make a custom tree view necessary? Would those be solved by using nested prefpanes as in (1)?

Comment 12

[Justin Wood (:Callek)]

In my opinion extending (1) to do that is asking for trouble, we would then be breaking toolkit assumptions... I can think of people who would like to do stuff like:

<prefwindow id="foo">
</prefwindow>

and then use the *same* overlay for both SeaMonkey prefwindow and their own, allowing their own to be used in TB/FF/XULRunner versions.

as it stands now I only override xbl:content, xbl:constructor and xbl:method name="_makePaneButton", with a slight override of xbl:property name="_shouldAnimate", as I feel that SeaMonkey's panel animating is "just wrong".

Using a sub-pane system like that, also, would _require_ all top-level <pane .../> to actually have their content inline, this removing much of the gain of the toolkit prefwindow system, (the dynamic overlay); Our [current] system gets this gain with the WSM+IFrame's.

as far as (2), I was having problems based on the shear number of elements to generate as I encountered <pane> stuff, essentially causing unneeded slowdown in  creation of each of the DOM elements to add to the tree. Using the custom treeView "keeps it speedy". I don't know about you, but with a full assortment of pref-panes, I'd hate to wait 30-90 seconds for the prefwindow to "appear"

Comment 13

[Justin Wood (:Callek)]

Attachment: Binding + Implement it in Suite

this should do it, I now correctly preprocess the binding (how toolkit does), and use dtd's ;-)

This merely implements <prefwindow> and does not actually use it, further patches on "using" <prefwindow> coming.

Comment 14

[Justin Wood (:Callek)]

Attachment: Binding + Implement it in Suite (take 2)

correct a typo pointed out by Mnyromyr, grrr.

Comment 15

[Justin Wood (:Callek)]

Comment on attachment 228277 [details] [diff] [review]
Binding + Implement it in Suite (take 2)

>@@ -81,4 +86,9 @@
> 
> menubar {
>   -moz-binding: url("chrome://communicator/content/bindings/toolbar.xml#grippymenubar");
>-}
>\ No newline at end of file
>+}
>+
>+prefwindow[xpfe="false"] {
>+  -moz-binding: url("chrome://communicator/content/bindings/prefwindow.xml#prefwindow");  
>+  -moz-box-orient: vertical;
>+}


Note to self, sleep before making any more patch cycles... should be

+prefwindow {

the xpfe="false" stuff was above.

Comment 16

[[e-mail]]

Comment on attachment 228277 [details] [diff] [review]
Binding + Implement it in Suite (take 2)

>+    <resources>
>+      <stylesheet src="chrome://global/skin/preferences.css"/>
>+    </resources>
This should already inherit.

>+      <xul:radiogroup anonid="selector" orient="horizontal" class="paneSelector chromeclass-toolbar"
>+                      xhtml2:role="wairole:list" style="display:none"/>
Why didn't you remove this?

>+          <xul:label value="&categoryHeader;"/>
I've always wanted the header on the treecol (with sortLocked="true")

>+      <xul:hbox anonid="dlg-buttons" class="prefWindow-dlgbuttons"
>+#ifdef XP_UNIX
>+                >
#ifdefs are bad enough but I particularly hate them in the middle of a tag... can you convert this to #ifdef the whole box?

>+      <constructor>
>+        <![CDATA[
>+          this.setAttribute("animate", !this._shouldAnimate ? "off" : "on");
Constructors inherit too, don't they?

>+          var mytree = document.getAnonymousElementByAttribute(this, 'anonid', 'smPrefTree');
>+          var myView = this.createView();        //odd syntax to use
>+          myView.wrappedJSObject = myView;   //wrappedJSObject correctly
>+          mytree.view = myView;
You might find it easier to use <implementation implements="nsITreeView"> and define all the properties and methods in XBL instead.

>+            getRowProperties: function(row,props){},
>+            getCellProperties: function(row,col,props){},
>+            getColumnProperties: function(colid,col,props){},
Nit: use proper spacing even for dummy functions

>+              return this.getPaneObject(row).isContOpen;
>+              return this.getPaneObject(row).isContEmpty;
Might as well abbreviate these to isOpen/isEmpty

>+            hasNextSibling: function(parentIndex, index) {
>+              var obj = this.getPaneObject(this.mLiveTree[parentIndex].parentIdx);
>+              return (this.mLiveTree[parentIndex].ObjTreeIdx
>+                         != obj.children.length - 1)
>+              return true;
This just... looks wrong. Two returns for a start ;-)
FYI, hasNextSibling actually means is "should I draw a vertical line at the level of the item with index parentIndex past the item with index index?"

>+                this.rowCount = this.mLiveTree.length - 1;
Is that -1 correct? Also, if this.rowCount is always equal to this.mLiveTree.length then your rowCount getter should just return that length to save you having to update the property each time.

>+                this.treebox.rowCountChanged( row + 1,
>+                                             (-1)*objectToToggle.children.length);
just use the unary - operator

>+              }else{
Nit: spaces around else

>+                var liveTreeChildren = new Array();
Unused.

>+                for (var i = 0; i < objectToToggle.children.length; i++){
>+                  this.mLiveTree.splice( row + i + 1, 0, {
>+                        ObjTreeIdx: i,
>+                        parentIdx: row,
>+                        level: 1
>+                      });
>+                }
You don't seem to update the indices of existing rows.

>+            mLiveTree: new Array(),
>+            mPaneObjectTree: new Array(),
Nit: I prefer []

>+                for each (var loadedPaneObj in this.mPaneObjectTree){
Please don't use for each. Some extension could modify Array.prototype :-(

>+                  if (typeof loadedPaneObj.children == 'undefined'){
I prefer to declare all properties in advance where possible.

My review got cut off here for some reason. I'll try again later.

Comment 17

[Karsten Düsterloh]

> So, given that only SM will use hierarchical prefpanes anyway, we could just
> ignore that issue - only if we would show toolkit's horizontal prefpane
> selector strip, we had to overwrite _selectPane. Anyone using SM as a
> XULRunner-like app base wouldn't add to the SM prefs and won't see any issues
> with a plain toolkit prefwindow.

Well, not really. The changes to get that to work seem to be even more
complex than your cyclic approach. So, your IRC proposal of using a SM
only nested tree structure tag set (which gets ignored by toolkit) looks rather decent to me now - and much less complicated than the parentpane attribute.
 ;-) 

Comment 18

[[e-mail]]

Comment on attachment 228277 [details] [diff] [review]
Binding + Implement it in Suite (take 2)

>+            selectPane: function(aPaneElement){
This function seems overly complex. I think it seems to be trying to find the parent pane so that it can be opened if necessary, but very inefficiently.

>+                return parentObj.children[this.mLiveTree[row].ObjTreeIdx];
How does this differ from this.mLiveTree[row]?

>+          var radio = document.createElement("radio");
Why do we need to create radiobuttons?

>\ No newline at end of file
>+}
>+
>+prefwindow[xpfe="false"] {
This one shouldn't have xpfe="false"

Comment 19

[Justin Wood (:Callek)]

(In reply to comment #16)
> (From update of attachment 228277 [details] [diff] [review] [edit])
> >+    <resources>
> >+      <stylesheet src="chrome://global/skin/preferences.css"/>
> >+    </resources>
> This should already inherit.
> 

fixed.

> >+      <xul:radiogroup anonid="selector" orient="horizontal" class="paneSelector chromeclass-toolbar"
> >+                      xhtml2:role="wairole:list" style="display:none"/>
> Why didn't you remove this?

because doing so would cause other inherited methods to break, causing me to need to override them as well, while the only parts I would be changing were references to this radiogroup, so instead of re-implementing (nearly) all of prefwindow (while needing to keep most of it in sync), I chose to leave this in.  Though aaronlev did conclude with me, that I do not need _most_ of the attributes here, anonid and style="display:none" should suffice.

> >+          <xul:label value="&categoryHeader;"/>
> I've always wanted the header on the treecol (with sortLocked="true")
> 

Easily fixed, done.

> >+      <xul:hbox anonid="dlg-buttons" class="prefWindow-dlgbuttons"
> >+#ifdef XP_UNIX
> >+                >
> #ifdefs are bad enough but I particularly hate them in the middle of a tag...
> can you convert this to #ifdef the whole box?

no problem, done. (For the record I kept toolkit's choice of how to ifdef this initially, though it is _our_ impl of it ;-) )

> >+      <constructor>
> >+        <![CDATA[
> >+          this.setAttribute("animate", !this._shouldAnimate ? "off" : "on");
> Constructors inherit too, don't they?

Yes, but I am adding features to the constructor.  As in, I can't call ParentBinding::constructor in any way if I override it (that I know of), besides I need to do stuff at varying places in the constructor.

> >+          var mytree = document.getAnonymousElementByAttribute(this, 'anonid', 'smPrefTree');
> >+          var myView = this.createView();        //odd syntax to use
> >+          myView.wrappedJSObject = myView;   //wrappedJSObject correctly
> >+          mytree.view = myView;
> You might find it easier to use <implementation implements="nsITreeView"> and
> define all the properties and methods in XBL instead.

I did it this way to know (more easily) what is truely part of nsITreeView, and to keep the implimentation/loading of this as simple and quick as possible, a JS Object just seems to have less perf hit (ass-u-me-d) than an XBL implements, if this really makes a difference to you, I'll do it in XBL methods.

> >+            getRowProperties: function(row,props){},
> >+            getCellProperties: function(row,col,props){},
> >+            getColumnProperties: function(colid,col,props){},
> Nit: use proper spacing even for dummy functions

Proper spacing is what?

+            getColumnProperties: function(colid, col, props){},

or

+            getColumnProperties: function(colid, col, props) {
+            },

(or something else)?

> >+              return this.getPaneObject(row).isContOpen;
> >+              return this.getPaneObject(row).isContEmpty;
> Might as well abbreviate these to isOpen/isEmpty

Sure, why not. done.

> >+            hasNextSibling: function(parentIndex, index) {
> >+              var obj = this.getPaneObject(this.mLiveTree[parentIndex].parentIdx);
> >+              return (this.mLiveTree[parentIndex].ObjTreeIdx
> >+                         != obj.children.length - 1)
> >+              return true;
> This just... looks wrong. Two returns for a start ;-)

removed |return true;|

> FYI, hasNextSibling actually means is "should I draw a vertical line at the
> level of the item with index parentIndex past the item with index index?"

Yes, but the code here never has a |level > 1|, so I don't have to worry about it here.

> >+                this.rowCount = this.mLiveTree.length - 1;
> Is that -1 correct? Also, if this.rowCount is always equal to
> this.mLiveTree.length then your rowCount getter should just return that length
> to save you having to update the property each time.

It does correlate, though the nsITreeView expects a number not a function for that.

> >+                this.treebox.rowCountChanged( row + 1,
> >+                                             (-1)*objectToToggle.children.length);
> just use the unary - operator

sure

> >+              }else{
> Nit: spaces around else

sure

> >+                var liveTreeChildren = new Array();
> Unused.

cruft, thanks. removed.

> >+                for (var i = 0; i < objectToToggle.children.length; i++){
> >+                  this.mLiveTree.splice( row + i + 1, 0, {
> >+                        ObjTreeIdx: i,
> >+                        parentIdx: row,
> >+                        level: 1
> >+                      });
> >+                }
> You don't seem to update the indices of existing rows.

Good catch, will work on adding this algorithm in.

> >+            mLiveTree: new Array(),
> >+            mPaneObjectTree: new Array(),
> Nit: I prefer []

Done.

> >+                for each (var loadedPaneObj in this.mPaneObjectTree){
> Please don't use for each. Some extension could modify Array.prototype :-(

Done.

> >+                  if (typeof loadedPaneObj.children == 'undefined'){
> I prefer to declare all properties in advance where possible.

Done

> My review got cut off here for some reason. I'll try again later.

I'll reply to your further review in next comment.

Comment 20

[Justin Wood (:Callek)]

(In reply to comment #18)
> (From update of attachment 228277 [details] [diff] [review] [edit])
> >+            selectPane: function(aPaneElement){
> This function seems overly complex. I think it seems to be trying to find the
> parent pane so that it can be opened if necessary, but very inefficiently.

It is, I do use for each (while I'll change as per your earlier comment), and I do use break and continue to not do more computation than needed.

The chance of it hitting all logic is nill in our default code, but if someone opens the prefwindow with showpane calling a nested tree item, we need to open it's parent and select it, which is why this is necessary.

> >+                return parentObj.children[this.mLiveTree[row].ObjTreeIdx];
> How does this differ from this.mLiveTree[row]?
> 

see |addToPaneTree| else clause, it shows it much better than I can explain here, other than "they return different objects".

> >+          var radio = document.createElement("radio");
> Why do we need to create radiobuttons?

Same reason I am keeping the radiogroup, to make it drastically easier to extend toolkit's version (though the buttons may be less needed.)

Comment 21

[[e-mail]]

(In reply to comment #19)
>>>+      <xul:radiogroup anonid="selector" orient="horizontal" class="paneSelector chromeclass-toolbar"
>>>+                      xhtml2:role="wairole:list" style="display:none"/>
>>Why didn't you remove this?
>Though aaronlev did conclude with me, that I do not need _most_ of the
>attributes here, anonid and style="display:none" should suffice.
hidden="true" in XUL, please!

>>>+      <constructor>
>>>+        <![CDATA[
>>>+          this.setAttribute("animate", !this._shouldAnimate ? "off" : "on");
>>Constructors inherit too, don't they?
>Yes, but I am adding features to the constructor.  As in, I can't call
>ParentBinding::constructor in any way if I override it (that I know of),
>besides I need to do stuff at varying places in the constructor.
It gets called before this constructor does.

>Proper spacing is what?
>+            getColumnProperties: function(colid, col, props){},
>or
>+            getColumnProperties: function(colid, col, props) {
>+            },
>(or something else)?
The second one is OK, and the first one is only missing the space before {

>the code here never has a |level > 1|, so I don't have to worry about it here.
Well I didn't try it but basically this should return true for all the level 1 items except the last item in its level.

>>>+                this.rowCount = this.mLiveTree.length - 1;
>>Is that -1 correct? Also, if this.rowCount is always equal to
>>this.mLiveTree.length then your rowCount getter should just return that length
>>to save you having to update the property each time.
>It does correlate, though the nsITreeView expects a number not a function for that.
get rowCount() { return this.mLiveTree.length; }
or in XBL:
<property name="rowCount" onget="return this.mLiveTree.length" readonly="true"/>

(In reply to comment #20)
>The chance of it hitting all logic is nill in our default code, but if someone
>opens the prefwindow with showpane calling a nested tree item, we need to open
>it's parent and select it, which is why this is necessary.
Oh, I understand why it's necessary, just not why it appears to be inefficient.

Comment 22

[Justin Wood (:Callek)]

Attachment: Binding + Implement in Suite (take 3)

this should address all review comments, (I don't think I missed anything)

Comment 23

[Justin Wood (:Callek)]

Comment on attachment 229434 [details] [diff] [review]
Binding + Implement in Suite (take 3)

>Index: mozilla/suite/common/bindings/prefwindow.xml
>+             closebuttonlabel="Close"
>+             closebuttonaccesskey="C"

ahh fooey, well that should be an entity of course

&preferencesCloseButton.label;
and
&preferencesCloseButton.accesskey;

respectively, with adding back in the dtd at "chrome://global/locale/preferences.dtd"

consider that as part of the review please.

Comment 24

[Justin Wood (:Callek)]

Attachment: Use <prefwindow> in SuiteRunner itself (WIP)

this is a WIP of what it is taking to tie this in, minus actual panes.  (Not for review)

Comment 25

[Karsten Düsterloh]

Attachment: Alternative extended binding (preftree hierarchy, no custom tree view)

This is an alternative approach, based basically on comment #17. The navigation tree is defined by a nested tag structure (which "accidentally" stays invisible when using just the basic toolkit prefwindow), so no strange cycle checking is necessary. Also, the navigation tree is created in JS, so no need for a custom tree view either - we likely don't have 10,000s of pref panes... ;-)

This is not a patch, just a different prefwindow.xml; Callek's patch is a prerequisite.

Comment 26

[Justin Wood (:Callek)]

Comment on attachment 229738 [details]
Alternative extended binding (preftree hierarchy, no custom tree view)


>      The syntax for use looks something like:
...
and
>      <method name="CreateNavigationTree">
...
>            // Only build our navigation tree if we have a <preftree> with at
>            // least one <preftreeitem>. Not every <preftreeitem> needs to have
>            // <prefpane> children, but <prefpane>s outside of <preftreeitem>s
>            // will not be accessible via tree navigation!

comments describe different syntax's, which is it?

>    <resources>
>      <stylesheet src="chrome://global/skin/preferences.css"/>
>    </resources>
Shouldn't be needed (and really isn't)

>    <content dlgbuttons="accept,cancel"
>             persist="lastSelected screenX screenY"
>             closebuttonlabel="Close"
>             closebuttonaccesskey="C"
^^^
See c#23


>          <xul:label value="&categoryHeader;"/>
>          <xul:tree style="width:13em;" 

Neil prefers the CategoryHeader as the column header, with sortLocked=true;  See my most recent patch.

>      </xul:hbox>
>      <xul:hbox anonid="dlg-buttons" class="prefWindow-dlgbuttons">
...
You want this section pre-processed like the toolkit one.

>          if (!targettree.hidden)
>          {

simply do a |if (targettree.hidden) return;| thing of some sort here, saves you a nesting level

>              treecell.setAttribute('label', walker.currentNode.getAttribute('label'));

Failing a label on the tree, I'd pull one of the prefpane itself.


>                targetitem = treechildren;
>              }
>              else 
>              {

why not a |continue| here, and avoid the else

You also seem to miss <prefpane>'s which are not in the <preftree> setup, aiui, that is by design; however what you fail to catch is the possibility of them being added (which they can be) to |this.prefPanes| on the toolkit side, and being selected by the code which loads the prefwindow (via window.arguments with the paneId), or by being the first <prefpane> in the list, with no default selection specified; and you are then left with no way to get back to it, and no matching paneId in your tree itself.  A solution would be to actually expose all <prefpane>'s as at least top-level items in the preftree (though it would be more work).

All in all, your method feels like a hack to me, and I still prefer my method, but if the above nits and one bigger problem are fixed, I would r+ anyway, (as a non-peer); lets poke jag/Neil about which API to use, I'm obviously favoring my own. ;-)

Comment 27

[Justin Wood (:Callek)]

Comment on attachment 229738 [details]
Alternative extended binding (preftree hierarchy, no custom tree view)

hmm, two more issues that I  just thought of for you to digest...

You should override <property name="_shouldAnimate"> to return false "onget", it is a nice little property which allows easy turn-off of animation, (mainly used for simplicity of preprocessing, but makes for us turning it off soo much easier) ;-)

Also, you probably want to drop the preftree into "hidden" or "collapsed" mode similar to what toolkit does with their radiogroup when this.preferencepanes.length == 1, having just one item in a tree makes little sense.

Comment 28

[[e-mail]]

Comment on attachment 229738 [details]
Alternative extended binding (preftree hierarchy, no custom tree view)

>This hierarchical structure definition has no text content and
>thus is invisible in any normal toolkit prefwindow!
Then do we need xpfe="false" ;-)

>          // the base binding ctor is called automatically before,
This comment is out of date, because you needed to ensure the tree was built before the first panel was shown...

>          var sourcetree = document.getElementsByTagName('preftree');
>          // only look at the first <preftree> element
>          targettree.hidden = !sourcetree.length || !sourcetree[0].childNodes.length;
Not that I like a separate label, but this doesn't hide it. It would be easier for you if you defaulted the tree to hidden, then returned early if there was no preftree element or it was empty, for which you should write
if (!sourcetree.item(0) || !sourcetree.item(0).hasChildNodes()) return;

>                treeitem.setAttribute('container', true);
I thought that content views detected containers automatically.

>            } // walk the hierarchy
I'm not convinced that you need to to such great lengths, wouldn't a recursive function have been simpler and easier?

>          if (!this.navigationTree)
>            this.CreateNavigationTree();
Except CreateNavigationTree doesn't guarantee to set this.navigationTree ...

>          if (this.navigationTree)
>          {
>            var index = 0;
>            if (aPaneElement && "preftreeitem" in aPaneElement)
>              index = this.navigationTree.contentView
>                          .getIndexOfItem(aPaneElement.preftreeitem);
>            this.navigationTree.view.selection.select(index);
>          }
I don't think you should select index 0 if the tree item is not found. -1 (meaning no selection) might work, if you don't want to leave it unchanged.

>          // Now do all the original toolkit stuff -
>          // alas!, this.__proto__.__proto__.showPane(aPaneElement);
>          // does not work here, it gets the this pointer wrong...
Try this.__proto__.__proto__.showPane.call(this, aPaneElement);

Comment 29

[Karsten Düsterloh]

Attachment: Alternative binding, v2

Functional, but still proof of concept, hence no real <content> entities and such. Still needs Callek's work as a basis, just exchange the binding xml file.

Re comment 26: I did most of the stuff proposed there, though:

>>      </xul:hbox>
>>      <xul:hbox anonid="dlg-buttons" class="prefWindow-dlgbuttons">
> ...
> You want this section pre-processed like the toolkit one.

Yes, I want, but my Mac suiterunner build complains about a "error evaluating ifdef: not a valid variable name: XP_UNIX", so I left that out for now.

> you are then left with no way to get back to [the prefpane], and no
> matching paneId in your tree itself.  A solution would be to actually
> expose all <prefpane>'s as at least top-level items in the preftree
> (though it would be more work).

Another solution is to avoid such selections, which I do now.


Re comment 27:

- Killing _shouldAnimate isn't a real necessity, I'd say...
- Hiding the tree stuff makes working with its contentView rather hard, so I chose collapsing.


Re comment 28:

> Then do we need xpfe="false" ;-)

Yes, the extended binding hides the toolkit top pane button bar - we could do this dynamically, though, but what's the point?

> I thought that content views detected containers automatically.

So did I at first, but I was taught the better. :-/

>>            } // walk the hierarchy
> I'm not convinced that you need to to such great lengths, wouldn't a
> recursive function have been simpler and easier?

Didn't try yet, isn't too relevant for a poc... ;-)

> Try this.__proto__.__proto__.showPane.call(this, aPaneElement);

Hmpf. Yeah, I forgot about that. :-/

Comment 30

[Justin Wood (:Callek)]

Comment on attachment 230663 [details]
Alternative binding, v2

Hmm, don't have time for my review at the moment, but skimming this and thinking about it, if an extension does a <prefwindow> call, expecting it to work like toolkits, without creating a <preftree> and has more than one <prefpane> there will be _no_ way to access any of the panes, as the tree itself will be hidden, and the radiogroup is hidden by the binding directly, is this intended, if so, imho, it's broken. ;-)

Comment 31

[Karsten Düsterloh]

Attachment: Alternative binding, v3

Some code clean-up (now using a recursive method to create the tree) and a slight change in handling the situation Callek described in comment 30: if no <preftree> structure is given, all <prefpanes> will be shown as top level items in the navigation tree. If a <preftree> is given, though, unreferenced <prefpanes< will be hidden.

Comment 32

[Robert Kaiser]

Neil, could you as UI owner decide on an approach we want to use here, so that we can move forward on this topic?

Comment 33

[Karsten Düsterloh]

Attachment: Alternative binding, v4

This a real patch of my alternative approach, for both suiterunner and XPFE. I did a slight change compared to v3, though: all prefpanes not part of the navigation are appended as top level items now, making the code even less complex.
In either case, the boolean pref browser.preferences.instantApply has yet to be set manually - and we need to rewrite all our pref panels...

Comment 34

[Karsten Düsterloh]

Comment on attachment 260253 [details] [diff] [review]
Alternative binding, v4

Oh, how I hate this "so we left the requestee field blank" message!

Anyway, taking, as Callek is AWOL and we need to get traction on this.

Comment 35

[Karsten Düsterloh]

Callek started transforming the panel calls by adding lots of ifdefs to keep XPFE as is - which I think is not necessary. We're touching all the places anyway, so we can just as well change them for both.

If we'd use the XPFE panelURL as the toolkit paneID, we could even get away without fixing any callers! But who wants pane ids like "chrome://communicator/content/pref/pref-proxies.xul"? We could also just fix the goPreferences function to just pass the itemID (instead of the panelURL) as the first parameter (= toolkit paneID) which would give "nicer" paneIDs, still without fixing any callers.
I'd propose the latter as a first step to get the preferences up and running after switching the XBL derivation, and fix the callers if we're in calm waters again.

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 260253 [details] [diff] [review]
Alternative binding, v4

Passing this to biesi - I'm not too up on this area, and I think you'd be better.

Comment 37

[[e-mail]]

Comment on attachment 260253 [details] [diff] [review]
Alternative binding, v4

>+    (Since the <preftree> hierarchy structure definition has no textual content,
>+    it will remain invisible in any normal toolkit <prefwindow>. This permits
>+    to reuse the very same <prefwindow> in both SeaMonkey and other, more
>+    toolkit dependent applications.)
I see your point, but the code to convert this to a tree is always going to be ugly... perhaps you could look for a <tree hidden="true"> and unhide it?

>+<!DOCTYPE dialog [
s/dialog/bindings/

>+                    onselect="OnSelectNavigationTree();">
That's cheating. It only works because the JS engine is passed all the ancestor nodes as scopes so that you eventually find the XBL binding.
document.documentElement.OnSelectNavigationTree(); would be acceptable.

>+      <xul:hbox anonid="dlg-buttons" class="prefWindow-dlgbuttons" pack="end">
>+        <xul:button dlgtype="extra2" class="dialog-button" hidden="true"/>
>+        <xul:spacer anonid="spacer" flex="1"/>
>+        <xul:button dlgtype="accept" class="dialog-button" icon="accept"/>
>+        <xul:button dlgtype="extra1" class="dialog-button" hidden="true"/>
>+        <xul:button dlgtype="cancel" class="dialog-button" icon="cancel"/>
>+        <xul:button dlgtype="help" class="dialog-button" hidden="true" icon="help"/>
>+        <xul:button dlgtype="disclosure" class="dialog-button" hidden="true"/>
>+      </xul:hbox>
Don't we need a Mac version of this?

>+            var filter =
>+            {
>+              acceptNode: function(node)
>+                          {
>+                            if (node.nodeName == 'preftreeitem')
>+                              return NodeFilter.FILTER_ACCEPT;
>+                            return NodeFilter.FILTER_REJECT;
>+                          }
>+            };
What other nodes are you expecting here? i.e. why not just use .firstChild and .nextSibling instead of a tree walker?

>+          var index = this._navigationTree.currentIndex;
>+          if (index < 0)
>+            return;
>+          var treeitem = this._navigationTree.contentView.getItemAtIndex(index);
>+          this.showPane(treeitem.prefpane);
I'd marginally prefer if (index >= 0) { ... }

>+          // Now do all the original toolkit stuff
>+          this.__proto__.__proto__.showPane.call(this, aPaneElement);
I rather liked Callek's idea of watching when the deck's index changed.
Obviously it would be better if toolkit was fixed to call the index setter which conveniently throws an event for us rather than having to watch for attribute changes.

>+*+ content/communicator/bindings/prefwindow.xml                     (bindings/prefwindow.xml)
Why the *+?

>-   content/communicator/pref/nsPrefWindow.js                        (pref/nsPrefWindow.js)
>-   content/communicator/pref/nsWidgetStateManager.js                (/xpfe/global/resources/content/nsWidgetStateManager.js)
Not sure it's safe to remove these just yet ;-)

>-        content/global/nsWidgetStateManager.js      (resources/content/nsWidgetStateManager.js)
And it's definitely unsafe to remove this.

Comment 38

[[e-mail]]

(In reply to comment #37)
>(From update of attachment 260253 [details] [diff] [review])
>>+    (Since the <preftree> hierarchy structure definition has no textual content,
>>+    it will remain invisible in any normal toolkit <prefwindow>. This permits
>>+    to reuse the very same <prefwindow> in both SeaMonkey and other, more
>>+    toolkit dependent applications.)
>I see your point, but the code to convert this to a tree is always going to be
>ugly... perhaps you could look for a <tree hidden="true"> and unhide it?
Actually, if we tweak nsTreeBodyFrame.cpp slightly, we can use <treechildren> here and <tree> in the binding. Other apps without the tree won't show anything.

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 260253 [details] [diff] [review]
Alternative binding, v4

please find someone else for this review

Comment 40

[mv_van_rantwijk]

Will nsPrefWindow.js and nsWidgetStateManager.js be removed completely, after this bug is fixed?  Can this new binding (new for SeaMonkey) be used in frames, like we do for MultiZilla? 

See http://multizilla.mozdev.org/features/session-manager.html#SettingsTab-Confirmations 
for an example.

Comment 41

[Karsten Düsterloh]

> Will nsPrefWindow.js and nsWidgetStateManager.js be removed completely, after
> this bug is fixed?

That's the plan. 
(The next patch will see quite some changes to cope with review comments and real use testing, so don't take attachment 260253 [details] [diff] [review] as a base for anything.)

> Can this new binding (new for SeaMonkey) be used in frames,
> like we do for MultiZilla? 

I can't tell what and how you're doing it, just by looking at pictures... ;-)

Comment 42

[mv_van_rantwijk]

(In reply to comment #41)
> > Can this new binding (new for SeaMonkey) be used in frames,
> > like we do for MultiZilla? 
> 
> I can't tell what and how you're doing it, just by looking at pictures... ;-)

Can the new binding (for SeaMonkey) be used in an <iframe> instead of a window and have tabs, no images, like we do in that screenshot?

Comment 43

[Karsten Düsterloh]

> Can the new binding (for SeaMonkey) be used in an <iframe> instead of a window

If the previous pref window could, the new should be able to as well - anything else would be a bug (at this time of writing, I don't know any particular reason why that should change).

> and have tabs, no images, like we do in that screenshot?

You mean tabs instead of the tree?
That should be even easier, because you could turn off the new binding (setting xpfe="false") and just use the toolkit prefwindow...

Comment 44

[Justin Wood (:Callek)]

Karsten, I'm not completely AWOL.

If you need help with this I am still willing, but lets just say my ability to build is greatly diminished since the last time I worked on this.  And since Neil did review your version, I am also willing to go with that, if he chooses.

Comment 45

[Karsten Düsterloh]

Attachment: Alternative binding, v5

This version abdicates the need for a new preftree element, it just sucks in a normal tree found as a child of the prefwindow. The tree's treeitems are tied to the prefpanes, untied prefpanes are added at the end.
Sorry for the long delay.

Comment 46

[Karsten Düsterloh]

As a sidenote:
Attachment 272421 [details] [diff] will depackage the xpfe files for the old pref dialogs. We probably could leave this out of this patch and do it along with the prefpanel migration in a separate bug...

Comment 47

[Philip Chee]

(In reply to comment #45)
> it just sucks in a normal tree found as a child of the prefwindow

I still think the tree should be called <preftree> or at least <tree type="preftree"> to make it easier to selectively style it differently from other trees.

Comment 48

[Karsten Düsterloh]

(In reply to comment #47)
> I still think the tree should be called <preftree> or at least <tree
> type="preftree"> to make it easier to selectively style it differently from
> other trees.

The binding doesn't know anything about the tree (and doesn't enforce anything but a few basic requirements like columns or body), you can name and/or style it however you want.
And if you really want to do global preftree theming, you can assume that any navigation tree is inside of the <prefwindow>'s navTrees-vbox:

| <xul:vbox anonid="navTrees">
|   <children includes="tree"/>
| </xul:vbox>

Comment 49

[[e-mail]]

(In reply to comment #48)
> And if you really want to do global preftree theming, you can assume that any
> navigation tree is inside of the <prefwindow>'s navTrees-vbox:
> 
> | <xul:vbox anonid="navTrees">
> |   <children includes="tree"/>
> | </xul:vbox>

Actually that's irrelevant, because the CSS rules don't cross into insertion points; instead the rule would be for prefwindow > tree.

Comment 50

[[e-mail]]

(In reply to comment #45)
>This version abdicates the need for a new preftree element, it just sucks in a
>normal tree found as a child of the prefwindow.
Actually with my fix for bug 377035 you don't even need to suck in a whole tree, a <treechildren> will do, and you can have the <tree> in the XBL.

Comment 51

[Karsten Düsterloh]

> >This version abdicates the need for a new preftree element, it just sucks in a
> >normal tree found as a child of the prefwindow.
> Actually with my fix for bug 377035 you don't even need to suck in a whole
> tree, a <treechildren> will do, and you can have the <tree> in the XBL.

Sure, and I did have a look at that. But what's the gain? I still need all those treeitem>treerow>trecell thingies and the column definitions. And it really doesn't matter much when I'm walking down the tree to connect it to the <prefpanes> whether I start from a <tree> or a <treechildren> element...

I shouldn't have written "suck in", the v5 binding just looks for a tree and tries to nail its twigs onto the prefpanes.

Comment 52

[Robert Kaiser]

(In reply to comment #51)
> But what's the gain?

The case that looks interesting for me is to be able to set a class="" attribute on that tree that is in the XBL, so it's easier to apply generic theming to it.

Comment 53

[Karsten Düsterloh]

(In reply to comment #52)
> The case that looks interesting for me is to be able to set a class=""
> attribute on that tree that is in the XBL, so it's easier to apply generic
> theming to it.

The binding could add a class (or a special attribute or whatever - would a class have advantages perf-wise?) to the tree, no problem.
But I'd rather let the actual <tree> outside of the binding so that <prefwindow> users have a certain degree of control upon the actual tree definition, eg. its dimension or number of columns.

Comment 54

[[e-mail]]

Comment on attachment 272421 [details] [diff] [review]
Alternative binding, v5

>+          // grab the first child tree and tie it to the prefpanes
>+          var tree = document.getAnonymousElementByAttribute(this, 'anonid', 'navTrees')
>+                             .boxObject.firstChild;
>+          if (tree)
>+          {
>+            this.InitNavigationTree(tree);
>+            this._paneDeck.addEventListener("DOMAttrModified",
>+                                            this.OnChangePrefpane,
>+                                            false);
>+            this._navigationTree.addEventListener("select",
>+                                            this.OnSelectNavigationTree,
>+                                            false);
>+          }
>+          // hide the toolkit pref strip if we have a tree
>+          this._selector.hidden = this._navigationTree;
initNavigationTree doesn't always set _navigationTree - in fact I'd prefer if all of this was moved into initNavigationTree. Note that an alternative way of listening to the select events is to use a <handler phase="target"> but you would then have to check the target of the event to see whether it is the tree or the deck (as per bug 389525).

>+            // load the prefpane associated with this treeitem
>+            var treeitem = event.target.contentView.getItemAtIndex(index);
>+            if ("prefpane" in treeitem)
>+              document.documentElement._selectPane(treeitem.prefpane);
I guess you need to do this for those bonus panes that don't otherwise have any way of being identified? (I was thinking that if all prefpanes had an id then you could link them up using document.getElementById(prefpane) and this._navigationTree.getElementsByAttribute("prefpane", id)).

>+          // wander through the <tree> and tie its <treeitem>s to the
>+          // corresponding <prefpane>s
Perhaps use this._navigationTree.getElementsByAttribute("prefpane", "*"); ? If not, at least this._navigationTree.getElementsByTagName("treeitem"); ?

>+          // ensure that we have a tree column
>+          // ensure that we have a tree body
[Wouldn't be necessary with an embedded tree!]

Comment 55

[mv_van_rantwijk]

(In reply to comment #54)
> Note that an alternative way of
> listening to the select events is to use a <handler phase="target"> but you
> would then have to check the target of the event to see whether it is the tree
> or the deck (as per bug 389525).

Which is still better, and cleaner, because replacing .addEventListener() {} won't break you ;)

Comment 56

[mv_van_rantwijk]

p.s. "DOMAttrModified" will probably be called a lot i.e. you might want to add a filter.

Comment 57

[[e-mail]]

Comment on attachment 229434 [details] [diff] [review]
Binding + Implement in Suite (take 3)

Cancelling this as Mnyromyr's patch is almost there.

Comment 58

[Karsten Düsterloh]

Attachment: Alternative binding, v6

Next round:
- left out removal of XPFE widget state manager (see comment #46)
- updated comment to reality: prefwindow users expecting their prefwindow to be used in other toolkit apps should hide the tree, we'll unhide it if necessary
- always add a class to the tree so that themes can "prestyle" it
- left InitNavigationTree to take a tree argument, so that prefwindow users have the possibility to exchange the tree by calling this method
- dropped the separate listeners in favour of a select handler 
- dropped insane tree walking code (while toolkit prefwindow likes all prefpanes to have ids, I need to touch all treeitems to hide those unused, so I went for getElementsByTag)

Comment 59

[[e-mail]]

Comment on attachment 275044 [details] [diff] [review]
Alternative binding, v6

>+  -moz-box-orient: vertical;
This is already true from xul.css, no need to repeat it here.
>+      <xul:hbox anonid="dlg-buttons" class="prefWindow-dlgbuttons"
>+#ifdef XP_UNIX
>+                >
>+        <xul:button dlgtype="disclosure" class="dialog-button" hidden="true"/>
>+        <xul:button dlgtype="help" class="dialog-button" hidden="true" icon="help"/>
>+        <xul:button dlgtype="extra2" class="dialog-button" hidden="true"/>
>+        <xul:button dlgtype="extra1" class="dialog-button" hidden="true"/>
>+        <xul:spacer anonid="spacer" flex="1"/>
>+        <xul:button dlgtype="cancel" class="dialog-button" icon="cancel"/>
>+        <xul:button dlgtype="accept" class="dialog-button" icon="accept"/>
>+#else
>+                pack="end">
>+        <xul:button dlgtype="extra2" class="dialog-button" hidden="true"/>
>+        <xul:spacer anonid="spacer" flex="1"/>
>+        <xul:button dlgtype="accept" class="dialog-button" icon="accept"/>
>+        <xul:button dlgtype="extra1" class="dialog-button" hidden="true"/>
>+        <xul:button dlgtype="cancel" class="dialog-button" icon="cancel"/>
>+        <xul:button dlgtype="help" class="dialog-button" hidden="true" icon="help"/>
>+        <xul:button dlgtype="disclosure" class="dialog-button" hidden="true"/>
>+#endif
>+      </xul:hbox>
It's a good thing I've stopped using a vim with syntax highlighting because this would really confuse it.

>+          this._selector.hidden = this._navigationTree;
Nit: != null (converting an object to a boolean looks odd)

>+      <method name="InitNavigationTree">
Method names should begin with a lower case letter.

>+          this._navigationTree = aTreeElement;
I don't think you should set this until after you unhide the tree.

>+          // add the class "prefnavtree", so that themes can set defaults
>+          this._navigationTree.className += ' prefnavtree';
What about width? flex? hidecolumnpicker?

>+          // ensure that we have a tree column
>+          if (!this._navigationTree.columns)
>+          {
>+            var navcols = document.createElement('treecols');
>+            var navcol  = document.createElement('treecol');
>+            navcol.setAttribute('id', 'navtreecol');
>+            navcol.setAttribute('primary', true);
>+            navcol.setAttribute('flex', 1);
>+            navcols.appendChild(navcol);
>+            this._navigationTree.appendChild(navcols);
>+          }
This didn't seem to work correctly - there may be a bug in XBL whereby this constructor runs before this._navigationTree.columns is set, but I got two sets of columns... there's also no label on this column, perhaps hideheader="true" would be a good idea?

>+        if (target == this._navigationTree)
>+        {
>+          var index = target.currentIndex;
>+          if (index >= 0)
>+          {
>+            // load the prefpane associated with this treeitem
>+            var treeitem = target.contentView.getItemAtIndex(index);
>+            if ('prefpane' in treeitem)
>+              document.documentElement._selectPane(treeitem.prefpane);
>+          }
>+        }
OK, so when I tried this (and I used DOMI to reveal the radiogroup to be sure), changing the deck index correctly selected the tree row but changing the tree row didn't always select the right pane (and the radiogroup didn't update, but I don't know whether that's expected). Also the inital row didn't seem to get selected.

Comment 60

[[e-mail]]

Comment on attachment 275044 [details] [diff] [review]
Alternative binding, v6

>+        <vbox flex="1">
>+          <dialogheader id="header"/>
Missing xul: prefixes (also on </vbox>) - and surely an id is incorrect?

>+          // grab the first child tree and tie it to the prefpanes
>+          var tree = document.getAnonymousElementByAttribute(this, 'anonid', 'navTrees')
>+                             .boxObject.firstChild;
This won't find a hidden tree...

Comment 61

[Karsten Düsterloh]

Attachment: Alternative binding, v7

Incarnation++:
- drop unneeded ex-XPFE styles 
- use slightly editor friendlier #ifdef
- use correct missing columns check 
- added a a class to the treeitems (should we do this for all tree elements?)
- fixed initial tree selection (all initial select events were fired by the base binding before we enter our derived ctor)
- fixed some other nits

Specifically didn't add width etc. for the tree, this has to be done by the tree supplier. 
And I can't get the tree selection to select a wrong pane, either...

Comment 62

[Karsten Düsterloh]

Attachment: demo prefwindow (must have chrome rights)

The prefwindow I use for testing.

Comment 63

[[e-mail]]

Comment on attachment 275507 [details] [diff] [review]
Alternative binding, v7

>+          // ensure that we have a tree body
>+          if (!this._navigationTree.body)
Should probably be !this._navigationTree.treeBoxObject.treeBody

Comment 64

[[e-mail]]

Comment on attachment 275507 [details] [diff] [review]
Alternative binding, v7

>+          // ensure that we have a tree column
>+          if (!this._navigationTree.columns.count)
...
>+          // ensure that we have a tree body
>+          if (!this._navigationTree.body)
Sorry, that wasn't the problem; the problem is that you need to ensure the tree body before you can count the columns.

Comment 65

[[e-mail]]

Comment on attachment 275507 [details] [diff] [review]
Alternative binding, v7

>+          // hide the toolkit pref strip if we have a tree
>+          this._selector.hidden = (this._navigationTree != null);
Interestingly the toolkit binding collapses the selector if there is only one pane, so maybe you should do
if (this._navigationTree)
  this._selector.collapsed = true;

>+          // the toolkit base binding fires some select events before we get
>+          // into this ctor, so we need to sync the tree manually now
>+          this.syncTreeWithPane(this._paneDeck.selectedIndex);
>+        ]]>
>+      </constructor>
>+
>+      <field name="_navigationTree">null</field>
>+
>+      <!-- <prefwindow> users can call this method to exchange the <tree> -->
In which case, wouldn't you want initNavigationTree to sync with the pane?

>+                node.className += ' prefnavtreeitem';
Can't style a <treeitem> (at least, not like this).

>+          // enforce loading the current pane
>+          document.documentElement._selectPane(this.currentPane);
This doesn't work if the first pane loads asynchronously.

>+              document.documentElement._selectPane(treeitem.prefpane);
This doesn't work if the pane loads asynchronously.

>+              this._navigationTree.view.selection.select(index);
This doesn't work if the first pane loads asynchronously!
What happens is that you sync the tree to the first pane, which fires a select event on the tree, so you try to sync the pane to the tree... strangely this doesn't seem to be a problem on other panes (with the above issue "fixed").

Comment 66

[Karsten Düsterloh]

Attachment: Alternative binding, v8

Next take:
- fix specific review comments
- fix asynchronous pane loading: listens to paneload events now

Comment 67

[[e-mail]]

Comment on attachment 277400 [details] [diff] [review]
Alternative binding, v8

>+        <xul:vbox anonid="navTrees">
>+          <children includes="tree"/>
>+        </xul:vbox>
Do we actually need a separate box here for the tree child?

>+      <xul:hbox anonid="dlg-buttons" class="prefWindow-dlgbuttons" pack="end">
I've just noticed you have a spacer flex="1" which makes pack meaningless.
This means that you put that hbox (without pack) outside the ifdef :-)

>+          var tree = this.getElementsByTagName('tree')[0];
>+          this.initNavigationTree(tree);
Do we need to check that the tree's parent is this?
(Probably in initNavigationTree actually)

>+          // hide the toolkit pref strip if we have a tree
>+          if (this._navigationTree)
>+            this._selector.hidden = true;
As I mentioned, toolkit uses collapsed; do you have a reason for hidden?

>+              document.documentElement.showPane(pane);
this.showPane suffices, no?

Comment 68

[Karsten Düsterloh]

Attachment: Alternative binding, v9 [checked in]

Final version (carrying over r+/sr+), addressing Neil's comments: 

> >+        <xul:vbox anonid="navTrees">
> >+          <children includes="tree"/>
> >+        </xul:vbox>
> Do we actually need a separate box here for the tree child?

IMO yes, because this will arrange multiple trees vertically (just in case, we still bind only the first one).

> put that hbox (without pack) outside the ifdef :-)

Done.

> Do we need to check that the tree's parent is this?

Yes. Done.

> >+            this._selector.hidden = true;
> As I mentioned, toolkit uses collapsed; do you have a reason for hidden?

Hidden completely suppresses rendering.

> >+              document.documentElement.showPane(pane);
> this.showPane suffices, no?

Yes. Done.

Comment 69

[Karsten Düsterloh]

Landed binding v9 on trunk. Yay!
The show must go on in bug 394522.

Comment 70

[[e-mail]]

(In reply to comment #67)
>>+      <xul:hbox anonid="dlg-buttons" class="prefWindow-dlgbuttons" pack="end">
>I've just noticed you have a spacer flex="1" which makes pack meaningless.
>This means that you put that hbox (without pack) outside the ifdef :-)
Not if dialog.xml removes the flex again. Sigh.

Comment 71

[Karsten Düsterloh]

revision 1.4
date: 2007/11/12 13:00:14 UTC;  author: neil%parkwaycc.co.uk;  lines: +1 -1
Windows does need pack="end" after all b=342087 r=Mnyromyr