221619 - [Meta] Tree widget refactoring and enhancement

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jan Varga [:janv]]

Comment 1

[Jan Varga [:janv]]

Attachment: Initial patch for layout

Comment 2

[Jan Varga [:janv]]

Attachment: Initial patch for the rest

Comment 3

[[e-mail]]

Comment on attachment 139993 [details] [diff] [review]
Initial patch for layout

My eyes somewhat glazed over the miscellaneous cleanup which probably means
that I missed something...

>-  void getCellAt(in long x, in long y, out long row, out wstring colID, out wstring childElt);
>+  void getCellAt(in long x, in long y, out long row, out nsITreeColumn col, out AString childElt);
Could this be made an ACString?

> nsTreeBodyFrame::nsTreeBodyFrame(nsIPresShell* aPresShell)
> :nsLeafBoxFrame(aPresShell), mPresContext(nsnull), mTreeBoxObject(nsnull), mImageCache(nsnull),
>- mColumns(nsnull), mScrollbar(nsnull), mTopRowIndex(0), mRowHeight(0), mIndentation(0), mStringWidth(-1),
>- mFocused(PR_FALSE), mColumnsDirty(PR_TRUE), mDropAllowed(PR_FALSE), mHasFixedRowCount(PR_FALSE),
>- mVerticalOverflow(PR_FALSE), mImageGuard(PR_FALSE), mReflowCallbackPosted(PR_FALSE),
>- mDropRow(-1), mDropOrient(-1), mScrollLines(0), mTimer(nsnull), mValueArray(~PRInt32(0)),
>- mUpdateBatchNest(0), mRowCount(0)
>+ mScrollbar(nsnull), mTopRowIndex(0), mRowHeight(0), mIndentation(0), mStringWidth(-1),
>+ mFocused(PR_FALSE), mHasFixedRowCount(PR_FALSE),
>+ mVerticalOverflow(PR_FALSE), mReflowCallbackPosted(PR_FALSE),
>+ mUpdateBatchNest(0), mRowCount(0), mSlots(nsnull)
> {
>+  mColumns = new nsTreeColumns(nsnull);
Can't use use mColumns(new nsTreeColumns(nsnull))?

>-  nsIView* ourView = nsLeafBoxFrame::GetView();
>+  nsIView* view = nsLeafBoxFrame::GetView();
>+  view->CreateWidget(kWidgetCID);
Might as well GetView()->CreateWidget().

>-NS_IMETHODIMP nsTreeBodyFrame::GetLastVisibleRow(PRInt32 *_retval)
>+NS_IMETHODIMP
>+nsTreeBodyFrame::GetLastVisibleRow(PRInt32 *_retval)
> {
>-  *_retval = mTopRowIndex + mPageCount;
>+  *_retval = mTopRowIndex + mPageLength;
>   return NS_OK;
> }
I don't see any useful consumers of this function - everyone who calls it
already knows the first visible row, they could request the page length
instead. Or, you could alter this function not to return a row greater than the
row count. (I say greater because this function actually returns the first
not-wholly-visible row.) At least inline the internal callers...

>+  if (aCol) {
>+    nsTreeColumn* col = NS_STATIC_CAST(nsTreeColumn*, aCol);
Isn't it safer to do this the other way around:
nsTreeColumn* col = NS_STATIC_CAST(nsTreeColumn*, aCol);
if (col) {

>-  if (aIndex < mTopRowIndex || aIndex > mTopRowIndex + mPageCount + 1)
>+  if (aIndex < mTopRowIndex || aIndex > mTopRowIndex + mPageLength + 1)
As the row mTopRowIndex + mPageLength is guaranteed to be the last (partially)
visible row, the + 1 is unnecessary. Also, aIndex -= mTopRowIndex might help
you calculate the y coordinate.

>   nsRect rowRect(mInnerBox.x, mInnerBox.y+mRowHeight*(aIndex-mTopRowIndex), mInnerBox.width, mRowHeight);
>-  if (!rowRect.IsEmpty())
>+  if (!rowRect.IsEmpty()) {
Q. How likely is this?

>+PRInt32
>+nsTreeBodyFrame::GetRowAt(PRInt32 aX, PRInt32 aY)
Variable "aX" never used...

>+void
>+nsTreeBodyFrame::InvalidateDropFeedback(PRInt32 aRow, PRInt16 aOrientation)
>+{
>+  InvalidateRow(aRow);
>+  if (aOrientation == nsITreeView::DROP_BEFORE)
>+    InvalidateRow(aRow - 1);
>+  else if (aOrientation == nsITreeView::DROP_AFTER)
>+    InvalidateRow(aRow + 1);
>+}
Now if only DROP_BEFORE was -1 and DROP_AFTER was +1 ...
>+    for (PRInt32 i = mTopRowIndex; i < mRowCount && i < mTopRowIndex+mPageLength+1; i++) {
Might as well use i <= mTopRowIndex + mPageLength;

>+  virtual ~nsTreeBodyFrame();
Is that correct? Who destroys it? (Same goes for your other objects).

>-NS_IMETHODIMP nsTreeBoxObject::GetPageCount(PRInt32 *_retval)
>+NS_IMETHODIMP nsTreeBoxObject::GetPageLength(PRInt32 *_retval)
> {
>   nsITreeBoxObject* body = GetTreeBody();
>   if (body)
>-    return body->GetPageCount(_retval);
>+    return body->GetPageLength(_retval);
Might as well rename _retval to aLength or something more meaningful.

>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2003
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   David W. Hyatt <[e-mail]> (Original Author)
>+ *   Jan Varga <[e-mail]>
This looks wrong...

>+    nsCOMPtr<nsIBoxObject> boxObject = do_QueryInterface(mTree);
mTree is already a box object...

>+  virtual ~nsTreeColumn() { NS_IF_RELEASE(mNext); };
Clear mNext->mPrevious and mNext->mColumns first. Consider this:
1. Store the second tree column in an nsCOMPtr
2. Destroy the tree, removing the last ref to the first column
3. Call GetPrevious(nsITreeColumn**) or GetCoumns(nsITreeColumns**)
nsTreeColumns also needs to clear mFirst->mColumns appropriately.

>   if (tag == nsXULAtoms::treecol) {
>     if (aAttribute == nsXULAtoms::properties) {
>-      nsAutoString id;
>-      aContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::id, id);
>-      if (mBoxObject)
>-        mBoxObject->InvalidateColumn(id.get());
>+      if (mBoxObject) {
>+        nsCOMPtr<nsITreeColumns> cols;
>+        mBoxObject->GetColumns(getter_AddRefs(cols));
>+        if (cols) {
>+          nsCOMPtr<nsIDOMElement> element = do_QueryInterface(aContent);
>+          nsCOMPtr<nsITreeColumn> col;
>+          cols->GetColumnFor(element, getter_AddRefs(col));
>+          mBoxObject->InvalidateColumn(col);
>+        }
>+      }
>     }
>   }
Doesn't the treecol element know how to invalidate itself already?

Comment 4

[[e-mail]]

Or, as an alternative, make a new nsITreeColumnBoxObject which would hold all
the properties for a column; the tree columns object could update the column
objects as necessary and clear the columns variable on the column objects when
it was destroyed, and each column object would have to unlink itself from the
doubly weak linked list when it was destroyed.

Comment 5

[[e-mail]]

Comment on attachment 139995 [details] [diff] [review]
Initial patch for the rest

This list of errors is not intended to be exhaustive.

>+CSS_ANON_BOX(moztreecolumn, ":-moz-tree-column")
> CSS_ANON_BOX(moztreerow, ":-moz-tree-row")
>+CSS_ANON_BOX(moztreeseparator, ":-moz-tree-separator")
> CSS_ANON_BOX(moztreecell, ":-moz-tree-cell")
>-CSS_ANON_BOX(moztreecolumn, ":-moz-tree-column")
>-CSS_ANON_BOX(moztreecelltext, ":-moz-tree-cell-text")
>-CSS_ANON_BOX(moztreetwisty, ":-moz-tree-twisty")
> CSS_ANON_BOX(moztreeindentation, ":-moz-tree-indentation")
> CSS_ANON_BOX(moztreeline, ":-moz-tree-line")
>+CSS_ANON_BOX(moztreetwisty, ":-moz-tree-twisty")
> CSS_ANON_BOX(moztreeimage, ":-moz-tree-image")
>-CSS_ANON_BOX(moztreeseparator, ":-moz-tree-separator")
>-CSS_ANON_BOX(moztreedropfeedback, ":-moz-tree-drop-feedback")
>+CSS_ANON_BOX(moztreecelltext, ":-moz-tree-cell-text")
>+CSS_ANON_BOX(moztreecheckbox, ":-moz-tree-checkbox")
> CSS_ANON_BOX(moztreeprogressmeter, ":-moz-tree-progressmeter")
>+CSS_ANON_BOX(moztreedropfeedback, ":-moz-tree-drop-feedback")
Any particular point to the moves?

>-
>-    /** 
>-     * APIs for inline editing.  isEditable is called to ask the view if the 
>-     * cell contents are editable.  A value of true will result in the 
>-     * tree popping up a text field when the user tries to inline edit 
>-     * the cell. 
>-     */
>-    boolean isEditable(in long row, in wstring colID);
>-
>-    /** 
>-     * onSetCellText is called when the contents of the cell have been edited by the user.
>-     */
>-    void onSetCellText(in long row, in wstring colID, in wstring value);
Why are you removing these? You went to the trouble of adding editable support
to content views...

>Index: dom/public/idl/xul/nsIDOMXULMultSelectCntrlEl.idl
>===================================================================
>RCS file: /cvsroot/mozilla/dom/public/idl/xul/nsIDOMXULMultSelectCntrlEl.idl,v
>retrieving revision 1.1
>diff -u -r1.1 nsIDOMXULMultSelectCntrlEl.idl
>--- dom/public/idl/xul/nsIDOMXULMultSelectCntrlEl.idl	12 Jan 2002 01:20:19 -0000	1.1
>+++ dom/public/idl/xul/nsIDOMXULMultSelectCntrlEl.idl	27 Jan 2004 17:08:45 -0000
>@@ -46,7 +46,8 @@
>   attribute DOMString selType;
> 
>   attribute nsIDOMXULSelectControlItemElement currentItem;
>-  
But replace it with a really blank line...

>+  attribute long currentIndex;
>+
You'll need to update listbox.xml to support this.

>Index: editor/ui/dialogs/content/EdSelectProps.js
You'll need to teach me how to use the new checkbox cell.
(Drag'n'drop would be nice too!)

>   var index = gDialog.charsetTree.builderView.getIndexOfResource(RDF.GetResource(gCharset));
>   if (index >= 0) {
>     var treeBox = gDialog.charsetTree.treeBoxObject;
>-    treeBox.selection.select(index);
>+    treeBox.view.selection.select(index);
>     treeBox.ensureRowIsVisible(index);
>   }
Cache builderView perhaps?

>-    var index = cview.componentView.tree.selection.currentIndex;
>+    var index = cview.componentView.tree.view.selection.currentIndex;
"Listen very carefully, I shall say this only once." rginda didn't know that
the view actually stores the selection object, and the the treeBoxObject gets
it from the view. So all of his code gets the treeBoxObject from the
componentView, interfaceView (et al.), and the selection from the
treeBoxObject... his views probably need a null initial selection (heavily
commented!)

>-  nsAutoString col(colID);
>-  if (col.Equals(NS_LITERAL_STRING("colNodeName")))
>+  nsAutoString colID;
>+  col->GetId(colID);
>+  if (colID.Equals(NS_LITERAL_STRING("colNodeName")))
>     domNode->GetNodeName(_retval);
>-  else if (col.Equals(NS_LITERAL_STRING("colLocalName")))
>+  else if (colID.Equals(NS_LITERAL_STRING("colLocalName")))
>     domNode->GetLocalName(_retval);
>-  else if (col.Equals(NS_LITERAL_STRING("colPrefix")))
>+  else if (colID.Equals(NS_LITERAL_STRING("colPrefix")))
>     domNode->GetPrefix(_retval);
>-  else if (col.Equals(NS_LITERAL_STRING("colNamespaceURI")))
>+  else if (colID.Equals(NS_LITERAL_STRING("colNamespaceURI")))
>     domNode->GetNamespaceURI(_retval);
>-  else if (col.Equals(NS_LITERAL_STRING("colNodeType"))) {
>+  else if (colID.Equals(NS_LITERAL_STRING("colNodeType"))) {
>     PRUint16 nodeType;
>     domNode->GetNodeType(&nodeType);
>     nsAutoString temp;
>     temp.AppendInt(PRInt32(nodeType));
>     _retval = temp;
>-  } else if (col.Equals(NS_LITERAL_STRING("colNodeValue")))
>+  } else if (colID.Equals(NS_LITERAL_STRING("colNodeValue")))
This is just so ugly. Can't you define some static atoms somewhere?
btw, I'd name your readonly attribute nsIAtom atom; rather than idAtom and
possibly even make it [noscript] - I can't see it being useful for script.

>     var bx = this.mDOMTree.treeBoxObject;
> 
>     if (!aEl) {
>-      bx.selection.select(null);
>+      bx.view.selection.select(null);
>       return false;      
>     }
>       
>@@ -608,7 +608,7 @@
>     }
> 
>     if (!aQuickie && lastIndex >= 0) {
>-      bx.selection.select(lastIndex);
>+      bx.view.selection.select(lastIndex);
>       bx.ensureRowIsVisible(lastIndex);
>     }
Except what you don't see is a var view = bx.view; ...

>       if (aObject != this.mDOMView.rootNode) {
>         this.mDOMView.rootNode = aObject;
>-        this.mAttrTree.treeBoxObject.selection.select(-1);
>+        this.mAttrTree.view.selection.select(-1);
>       }
Very similar case... this.mAttrTree.view == this.mDOMView

>     this.mView = new StyleSheetsView(aObject);
>     this.mOlBox.view = this.mView;
>     this.mObsMan.dispatchEvent("subjectChange", { subject: aObject });
>-    this.mOlBox.selection.select(0);
>+    this.mOlBox.view.selection.select(0);
Sigh...

>Index: extensions/sql/base/src/Makefile.in
As well as this, you included browser/ mail/ and toolkit/ none of which I
bothered to review.

>         treeContent.treeBoxObject.view = treeView;
>-        if (treeContent.treeBoxObject.selection)
>+        if (treeContent.treeBoxObject.view.selection)
>         {
>-            treeContent.treeBoxObject.selection.tree =
>+            treeContent.treeBoxObject.view.selection.tree =
>                 treeContent.treeBoxObject;
Sigh...

> function ThreadPaneSelectionChanged()
> {
>   var treeBoxObj = GetThreadTree().treeBoxObject;
>-  var treeSelection = treeBoxObj.selection;
>+  var treeSelection = treeBoxObj.view.selection;
>   if (!gRightMouseButtonDown)
>     treeBoxObj.view.selectionChanged();
> }
I don't think that the + was necessary ;-)

>-  switch (aColID[0])
>+  const PRUnichar* colID;
>+  aCol->GetIdConst(&colID);
>+  switch (colID[0])
More atoms here, please.

> NS_IMETHODIMP 
>-nsNSSASN1Tree::GetCellText(PRInt32 row, const PRUnichar *colID, 
>-                               nsAString& _retval)
>+nsNSSASN1Tree::GetCellText(PRInt32 row, nsITreeColumn* col, 
>+                           nsAString& _retval)
> {
>-  nsCOMPtr<nsIASN1Object> object;
>-  _retval.SetCapacity(0);
>-  NS_ConvertUCS2toUTF8 aUtf8ColID(colID);
>-  const char *col = aUtf8ColID.get();
>+  _retval.Truncate();
>+  const PRUnichar* colID;
>+  col->GetIdConst(&colID);
>   nsresult rv = NS_OK;
>-  if (strcmp(col, "certDataCol") == 0) {
>-    myNode *n = FindNodeFromIndex(row);
>+
>+  if (NS_LITERAL_STRING("certDataCol").Equals(colID)) {
>+    myNode* n = FindNodeFromIndex(row);
>     if (!n)
>       return NS_ERROR_FAILURE;
> 
>-    //There's only one column for ASN1 dump.
>+    // There's only one column for ASN1 dump.
>     rv = n->obj->GetDisplayName(_retval);
>   }
>+
>   return rv;
> }
It hardly seems worth checking the column id for a single column...

>-// CanDropBeforeAfter
>+// CanDrop
> //
> // Can't drop on the thread pane.
> //
>-NS_IMETHODIMP nsNSSASN1Tree::CanDropBeforeAfter(PRInt32 index, PRBool before, PRBool *_retval)
>+NS_IMETHODIMP nsNSSASN1Tree::CanDrop(PRInt32 index, PRInt32 orientation, PRBool *_retval)
Might as well get right of the whole comment ;-)

>Index: security/manager/pki/src/nsASN1Tree.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/manager/pki/src/nsASN1Tree.h,v
>retrieving revision 1.4
>diff -u -r1.4 nsASN1Tree.h
>--- security/manager/pki/src/nsASN1Tree.h	17 Sep 2002 18:50:48 -0000	1.4
>+++ security/manager/pki/src/nsASN1Tree.h	27 Jan 2004 17:08:57 -0000
>@@ -42,6 +42,7 @@
> #include "nsITreeView.h"
> #include "nsITreeBoxObject.h"
> #include "nsITreeSelection.h"
>+#include "nsITreeColumns.h"
You only need the (existing) forward declaration here,
you could move the include to the .cpp file instead.

>+  const PRUnichar* colID;
>+  col->GetIdConst(&colID);
>+
>   treeArrayEl *el = GetThreadDescAtIndex(row);
>   if (el != nsnull) {
>-    if (strcmp(col, "certcol") == 0)
>+    if (NS_LITERAL_STRING("certcol").Equals(colID))
More atoms perhaps?

>-// CanDropBeforeAfter
>+// CanDrop
> //
> // Can't drop on the thread pane.
> //
>-NS_IMETHODIMP nsCertTree::CanDropBeforeAfter(PRInt32 index, PRBool before, PRBool *_retval)
>+NS_IMETHODIMP nsCertTree::CanDrop(PRInt32 index, PRInt32 orientation, PRBool *_retval)
Again :-)

>Index: security/manager/ssl/src/nsCertTree.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsCertTree.h,v
>retrieving revision 1.9
>diff -u -r1.9 nsCertTree.h
>--- security/manager/ssl/src/nsCertTree.h	11 Sep 2003 01:59:21 -0000	1.9
>+++ security/manager/ssl/src/nsCertTree.h	27 Jan 2004 17:08:57 -0000
>@@ -42,6 +42,7 @@
> #include "nsITreeView.h"
> #include "nsITreeBoxObject.h"
> #include "nsITreeSelection.h"
>+#include "nsITreeColumns.h"
Again...

>+  skin/classic/global/tree/checkbox.gif                         (tree/checkbox.gif)
>+  skin/classic/global/tree/checkbox-checked.gif                 (tree/checkbox-checked.gif)
Don't we already have some suitable images... checkbox/cbox.gif etc?

>-          return this.treeBoxObject.selection.currentIndex;
>+          return this.treeBoxObject.view.selection.currentIndex;
Possibly this.tree.view.selection.currentIndex; ?

>-      <method name="sortBookmarks">
I don't think you meant to remove this method...

>+        canDrop: function(index, orientation)
>         {
>           var dragSession = DS && DS.getCurrentSession();
>           if (!dragSession)
>             return false;
>           var selection = BookmarksUtils.getSelectionFromXferData(dragSession);
>-          if (selection.containsRF)
>-            return false;
You could probably have left this in, and just added
	  if (orientation == BookmarksUtils.DROP_ON) {
	    return true;
just after it.

>Index: xpfe/global/resources/content/xul.css
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/global/resources/content/xul.css,v
>retrieving revision 1.130
>diff -u -r1.130 xul.css
>--- xpfe/global/resources/content/xul.css	15 Dec 2003 22:47:52 -0000	1.130
>+++ xpfe/global/resources/content/xul.css	27 Jan 2004 17:09:05 -0000
>@@ -495,6 +495,7 @@
>   -moz-user-focus: none;
>   -moz-user-select: none;
>   -moz-box-flex: 1;
>+/*  overflow: auto; */
> }
Part of another patch?

>-      <handler event="click" button="0" phase="target" action="this.parentNode.parentNode.treeBoxObject.view.cycleHeader(this.id, this);"/>
>+      <handler event="click" button="0" phase="target">
>+        <![CDATA[
>+          var tree = this.parentNode.parentNode;
>+          var column = tree.columns.getColumnFor(this);
>+          tree.treeBoxObject.view.cycleHeader(column);
>+        ]]>
>+      </handler>
I've always wondered why this handler wasn't on the treecol-base.

Comment 6

[Jan Varga [:janv]]

Attachment: Updated patch for layout

Comment 7

[Jan Varga [:janv]]

> >+  void getCellAt(in long x, in long y, out long row, out nsITreeColumn col,
out AString childElt);
> Could this be made an ACString?

done

> >+  mColumns = new nsTreeColumns(nsnull);
> Can't use use mColumns(new nsTreeColumns(nsnull))?

as we discussed, we might want to check for null in the future :)

> >+  view->CreateWidget(kWidgetCID);
> Might as well GetView()->CreateWidget().

done

> I don't see any useful consumers of this function - everyone who calls it
> already knows the first visible row, they could request the page length
> instead. Or, you could alter this function not to return a row greater than the
> row count. (I say greater because this function actually returns the first
> not-wholly-visible row.) At least inline the internal callers...

inlined and added a check for row count

> >+  if (aCol) {
> >+    nsTreeColumn* col = NS_STATIC_CAST(nsTreeColumn*, aCol);
> Isn't it safer to do this the other way around:
> nsTreeColumn* col = NS_STATIC_CAST(nsTreeColumn*, aCol);
> if (col) {

done

> >+  if (aIndex < mTopRowIndex || aIndex > mTopRowIndex + mPageLength + 1)
> As the row mTopRowIndex + mPageLength is guaranteed to be the last (partially)
> visible row, the + 1 is unnecessary. Also, aIndex -= mTopRowIndex might help
> you calculate the y coordinate.

removed "+ 1" InvalidateRow() and InvalidateCell()

> >+  if (!rowRect.IsEmpty()) {
> Q. How likely is this?

removed in InvalidateRow()

> >+nsTreeBodyFrame::GetRowAt(PRInt32 aX, PRInt32 aY)
> Variable "aX" never used...

I've decided to not remove it yet, we might want to check the x coordinatio to
be valid.

> Now if only DROP_BEFORE was -1 and DROP_AFTER was +1 ...

done, I also inlined InvalidateDropFeedback()

> >+    for (PRInt32 i = mTopRowIndex; i < mRowCount && i <
mTopRowIndex+mPageLength+1; i++) {
> Might as well use i <= mTopRowIndex + mPageLength;

done

> >+  virtual ~nsTreeBodyFrame();
> Is that correct? Who destroys it? (Same goes for your other objects).

fixed all objects except frames (nsTreeBodyFrame and nsTreeColFrame) which may
be destroyed through an ancestor pointer

> >+    return body->GetPageLength(_retval);
> Might as well rename _retval to aLength or something more meaningful.

as we discussed, I prefer _retval, this way I know it's not a parameter in IDL

> >+ *   David W. Hyatt <[e-mail]> (Original Author)
> >+ *   Jan Varga <[e-mail]>
> This looks wrong...

as we discussed, I used some code by hyatt

> >+    nsCOMPtr<nsIBoxObject> boxObject = do_QueryInterface(mTree);
> mTree is already a box object...

mTree is an nsITreeBoxObject

> >+  virtual ~nsTreeColumn() { NS_IF_RELEASE(mNext); };
> Clear mNext->mPrevious and mNext->mColumns first. Consider this:
> 1. Store the second tree column in an nsCOMPtr
> 2. Destroy the tree, removing the last ref to the first column
> 3. Call GetPrevious(nsITreeColumn**) or GetCoumns(nsITreeColumns**)
> nsTreeColumns also needs to clear mFirst->mColumns appropriately.

fixed in nsTreeColumn dtor

> Doesn't the treecol element know how to invalidate itself already?

No, nsITreeColumn::Invalidate() is intended for invalidation of cached col
attributes. Needed in the future for more fine grained invalidation.

Comment 8

[Jan Varga [:janv]]

Please ignore the printfs in dtors.
The diff between those two diffs is not accurate since the revisions are not the
same.

Comment 9

[Jan Varga [:janv]]

Attachment: Updated patch for the rest

Comment 10

[Jan Varga [:janv]]

> >+CSS_ANON_BOX(moztreedropfeedback, ":-moz-tree-drop-feedback")
> Any particular point to the moves?

I want them to be in a logical order, actually it's painting order in this case

> >-    void onSetCellText(in long row, in wstring colID, in wstring value);
> Why are you removing these? You went to the trouble of adding editable support
> to content views...

isEditable is now implemented by both views, setCellText is implemented by the
content view and I plan to add support for setCellText to the builder view, too

> >+  attribute long currentIndex;
> >+
> You'll need to update listbox.xml to support this.

done

> You'll need to teach me how to use the new checkbox cell.

1. Add editable="true" on a <tree>
2. Add editable="true" and type="checkbox" on a <treecol>

> (Drag'n'drop would be nice too!)

yeah, I've been thinking about it

> >-    treeBox.selection.select(index);
> >+    treeBox.view.selection.select(index);
> >     treeBox.ensureRowIsVisible(index);
> >   }
> Cache builderView perhaps?

done

> >-    var index = cview.componentView.tree.selection.currentIndex;
> >+    var index = cview.componentView.tree.view.selection.currentIndex;
> "Listen very carefully, I shall say this only once." rginda didn't know that
> the view actually stores the selection object, and the the treeBoxObject gets
> it from the view. So all of his code gets the treeBoxObject from the
> componentView, interfaceView (et al.), and the selection from the
> treeBoxObject... his views probably need a null initial selection (heavily
> commented!)

done

> This is just so ugly. Can't you define some static atoms somewhere?
> btw, I'd name your readonly attribute nsIAtom atom; rather than idAtom and
> possibly even make it [noscript] - I can't see it being useful for script.

as we discussed, let's do it in a different bug

> >     if (!aQuickie && lastIndex >= 0) {
> >-      bx.selection.select(lastIndex);
> >+      bx.view.selection.select(lastIndex);
> >       bx.ensureRowIsVisible(lastIndex);
> >     }
> Except what you don't see is a var view = bx.view; ...

fixed

> >     this.mView = new StyleSheetsView(aObject);
> >     this.mOlBox.view = this.mView;
> >     this.mObsMan.dispatchEvent("subjectChange", { subject: aObject });
> >-    this.mOlBox.selection.select(0);
> >+    this.mOlBox.view.selection.select(0);

fixed

> As well as this, you included browser/ mail/ and toolkit/ none of which I
> bothered to review.

ok, I'll find someone else for these

> >-        if (treeContent.treeBoxObject.selection)
> >+        if (treeContent.treeBoxObject.view.selection)
> >         {
> >-            treeContent.treeBoxObject.selection.tree =
> >+            treeContent.treeBoxObject.view.selection.tree =
> >                 treeContent.treeBoxObject;
> Sigh...

fixed

> > function ThreadPaneSelectionChanged()
> > {
> >   var treeBoxObj = GetThreadTree().treeBoxObject;
> >-  var treeSelection = treeBoxObj.selection;
> >+  var treeSelection = treeBoxObj.view.selection;
> >   if (!gRightMouseButtonDown)
> >     treeBoxObj.view.selectionChanged();
> > }
> I don't think that the + was necessary ;-)

simplified even more

> >-  switch (aColID[0])
> >+  const PRUnichar* colID;
> >+  aCol->GetIdConst(&colID);
> >+  switch (colID[0])
> More atoms here, please.

in other bug

> >-    //There's only one column for ASN1 dump.
> >+    // There's only one column for ASN1 dump.
> >     rv = n->obj->GetDisplayName(_retval);
> >   }
> >+
> >   return rv;
> > }
> It hardly seems worth checking the column id for a single column...

fixed

> Might as well get right of the whole comment ;-)

done

> >+#include "nsITreeColumns.h"
> You only need the (existing) forward declaration here,
> you could move the include to the .cpp file instead.

fixed

> >+  skin/classic/global/tree/checkbox-checked.gif                
(tree/checkbox-checked.gif)
> Don't we already have some suitable images... checkbox/cbox.gif etc?

we have only checked checkbox

> >-          return this.treeBoxObject.selection.currentIndex;
> >+          return this.treeBoxObject.view.selection.currentIndex;
> Possibly this.tree.view.selection.currentIndex; ?

fixed

> >-      <method name="sortBookmarks">
> I don't think you meant to remove this method...

no, I meant to remove it, I want to change the sorting a bit
actually, I originally implemented it this way, that is, having 2 sorting
methods - view sort (not persistent) and data source sort (persistent)
We just need a better wording for them

> You could probably have left this in, and just added
> 	  if (orientation == BookmarksUtils.DROP_ON) {
> 	    return true;
> just after it.

done

> >+/*  overflow: auto; */
> > }
> Part of another patch?

removed

> >+      <handler event="click" button="0" phase="target">
> >+        <![CDATA[
> >+          var tree = this.parentNode.parentNode;
> >+          var column = tree.columns.getColumnFor(this);
> >+          tree.treeBoxObject.view.cycleHeader(column);
> >+        ]]>
> >+      </handler>
> I've always wondered why this handler wasn't on the treecol-base.

moved to the base

Comment 11

[Jan Varga [:janv]]

Guys, please take a look at those patches. I know they're big, but they should
be cleaned up after the first review.
It would be really great if I could catch the freeze.

Comment 12

[[e-mail]]

Comment on attachment 140659 [details] [diff] [review]
Updated patch for layout

r=me with the changes as discussed on irc.

Comment 13

[[e-mail]]

Comment on attachment 140664 [details] [diff] [review]
Updated patch for the rest

r=me for xpfe.

Comment 14

[Brendan Eich [:brendan]]

I'm wondering if XUL 1.0 compatibility is being broken here.  I have no time
today to read the patch and try to assess that for myself, but I'd like Jan and
anyone else who can to comment on the question.

/be

Comment 15

[Jan Varga [:janv]]

(In reply to comment #14)
> I'm wondering if XUL 1.0 compatibility is being broken here.  I have no time
> today to read the patch and try to assess that for myself, but I'd like Jan and
> anyone else who can to comment on the question.

Well, my answer depends on the term XUL 1.0. If it's just the XML syntax we use,
then my patch doesn't break compatibility at all.
However I modified/added several interfaces that are not frozen yet. I'll attach
a patch that contains only these changes.
I fixed all implementations in our source code, including firefox, thunderbird
and sunbird. And if I forgot something I'd fix it ASAP after my landing.
So for a XUL developer it means that he has to fix his JS code too.
I plan to post a message to n.p.m.xpfe/n.p.d.xul of course that will contain a
complete description of the changes and how to fix existing code.
I know it's a pain, but these changes will make the tree widget simplier, faster
and it will unblock my other development that depends on it.
Honestly, some idl changes are not so critical, but I included them because when
there's a chance to do it, do it at once rather than mess things again and again
There's a little proof that requiring ids on tree cols (bug 151986) might have
been confusing for XUL developers is at
http://www.xulplanet.com/ndeakin/archive/2003/12/15/
I'll go through my patch again and if I find something that breaks the XUL 1.0
syntax I'll let you know.

Comment 16

[Jan Varga [:janv]]

Attachment: IDL changes

Comment 17

[Nigel McFarlane]

A mighty patch. Feedback on comment 14: does this break XUL 1.0?

On the XML side: No. Not having to specify id= on <column> is one less thing for
app developers to trip over. I support this change.

On the AOM side: Partially No. Changes there are minimal, except where IDL
changes are implied (see below).

On the IDL side: Yes, sort of. Applications built on Mozilla 1.0 will definitely
have to be migrated if they make any significant use of trees, just as Firefox
has had to be in this patch.  This is not the first such tree change though.
Various bugs in trees in the past mean that app developers have had to make
changes just to keep things working. This is another instance of that, but
driven this time by enhancements rather than by discovered bugs. A rocky road
for app developers.

Those using trees from 1.0 or nearby for apps are strong supporters of Mozilla
and it seems unwise to annoy them deliberately. mozilla.org would no doubt
prefer that they continue to upgrade from 1.6 to 1.7 rather than halt at 1.6.

Otherwise, Ben and Jan would have to add back the old 1.0 api's and mark them
deprecated if these older app users are to be supported.

The alternative (ahem) is for 1.7 to be released with a professional quality
migration document. If there's a company or org out there (anyone?) with heavy
use of trees, then they should contact me so that I can 'volunteer' to do this,
once Ben and Jan's work is fully stable.

- N.

Comment 18

[Nigel McFarlane]

This second comment is patch review. I defer to Neil's excellent close reading
for many points not covered here. The two issues I would raise are:

In pursuing the goal of a columnar/tabular navigational model, I think removal
of column ID API's has been slightly over-zealous. There needs to be a way to
move between tabular and DOM navigation models, so the scripter can still walk
the XML tree if desired, even if that is only the tree as it appears in a
template spec. Even though column ID's are not used anymore in trees, I would
still like to see a getIdForColumn() and getColumnForId() or equivalent, so that
tabular columns and DOM elements can be mapped between. That lets programmers
move between tabular and DOM navigation as they see fit.

Secondly, does nsIDOMXULTreeElement fall a little short? This new interface has
potential in my view. It could be a base class for all the non-label cell types,
like progressmeter, etc. It could be used to move trees a micron closer to a
standard rather than specialist framing model at some point in the future. I
think this i/f is forward looking, but if APIs are to break adding it, then can
they be broken with some foresight?

- N.

Comment 19

[[e-mail]]

Here are (I think) the new ways to perform familiar tasks:

tree.treeBoxObject.getColumnIndex(<id>) is now
tree.columns.getNamedColumn(<id>).index

tree.treeBoxObject.getColumnID(<index>) is now tree.columns[index].id

tree.treeBoxObject.getKeyColumnIndex() is now tree.columns.getKeyColumn().index

I don't think the current tree has a way to get directly from the column element
from the index or vice versa, for instance you have to use
tree.treeBoxObject.getColumnIndex(treecol.id) (assuming that the treecol element
has an id, of course).

Comment 20

[Jan Varga [:janv]]

Thanks for all your comments.

Neil perfectly answered to your second comment. I didn't remove col ids
completely, I just replaced them with a new structure nsITreeColumn. Actutally
there was such structure already, I just exposed some of its methods and added
new features. So an nsITreeColumn object has "index" and "id" attributes. The
index attribute is always filled, but id can be empty. There is even "atom"
attribute but it's not scriptable.

Yeah, nsIDOMXULTreeElement has a potential. Actually, bryner and I have been
even considering moving all nsITreeBoxObject methods and attributes to this new
interface. Anyway I think we need more time to reconsider such a change because
there are other box objects, etc.

The migration in most cases means something like this:

tree.treeBoxObject.selection -> tree.view.selection

if (colID == "myCol") -> if (col.id == "myCol") - in all methods that were using
colID


and for those who use trees more heavily:

tree.treeBoxObject.getPageCount() -> tree.treeBoxObject.getPageLength()

nsITreeView::CanDropOn and nsITreeView::CanDropBeforeAfter were merged into one
nsITreeView::CanDrop

and I think, the new nsITreeView interface (added setCellValue() method) is more
ready to editable cells that might come in the future

Comment 21

[Jan Varga [:janv]]

I forgot to mention that I added explicitly "@status UNDER_DEVELOPMENT" into the
nsIDOMXULTreeElement.

Comment 22

[Jan Varga [:janv]]

Attachment: An up to date patch

Comment 23

[Brian Ryner (not reading)]

So just to summarize a conversation I had with Jan, here are the benefits this
patch provides (to offset the cost of breaking compatibility):

- No need to specify id= on columns, and easy syntax for obtaining a column
object from js
- No need to iterate over the tree columns doing string compares on the id in
interface methods which take a column parameter.
- Fix some inefficiencies in event handling and drag & drop
- Groundwork for inline editing

I think this sounds like the sort of improvement we'd want for a "XUL 2.0", the
question is when an appropriate time is to land it.  My vote would be now.

Comment 24

[Brian Ryner (not reading)]

I just have a few minor comments on the interfaces:

layout/xul/base/src/tree/public/nsITreeColumns.idl:

+  void restoreNaturalOrder();

please comment this method.

layout/xul/base/src/tree/public/nsITreeView.idl:

+  /**
+   * setCellValue is called when the value of the cell have been set by the user.
+   * This method is only called for columns of type other than |text|.

"has been set"

The tree changes themselves look fine and the call site changes are very
straightforward.  sr=me but please coordinate with drivers about whether this
should land for 1.7b or wait for 1.8a.

Comment 25

[Brian Ryner (not reading)]

Comment on attachment 140659 [details] [diff] [review]
Updated patch for layout

see comment 24 for review comments

Comment 26

[Brian Ryner (not reading)]

Comment on attachment 140664 [details] [diff] [review]
Updated patch for the rest

see comment 24 for review comments

Comment 27

[Jan Varga [:janv]]

Thanks!
I think it's too late for 1.7b, I'll wait for 1.8a.

Comment 28

[Michael Lefevre]

Seems this was checked in on 16 April, along with some followup fixes for
bustage and the regression bug 240872 that it caused.

Should it be marked fixed now, along with the bugs that this bug "depends" on
(although because the patch is in this "meta" bug, the dependencies are all
backwards...)?

Comment 29

[Patrick]

Not sure if i tracked my comment to the right bug, but in the download manager
window, the last entry in the column picker now reads "restore natural order".
This is an awkward formulation, especially the word "natural" :o

Suggested phrase "restore default order".

[Posting here instead of filing a new bug since this one is not marked FIXED yet.]

Comment 30

[Jan Varga [:janv]]

We use natural, ascending and descending in the tree widget for sorting.
But I'm not a native English speaker, so I let others to decide.

Comment 31

[Jan Varga [:janv]]

Michael, bug 231847 is not completely fixed. So don't close this one yet.
Anyway, thanks for the interest.

Comment 32

[timeless]

hello world. please to be changing iid's when you change interfaces. the change
to nsITreeView was significant, broke many consumers (including failure to
update something in thunderbird) and did not change its iid.

Comment 33

[Jan Varga [:janv]]

Attachment: iid changes

Comment 34

[[e-mail]]

Comment on attachment 157813 [details] [diff] [review]
iid changes

The interfaces nsIXULTemplateBuilder and nsITreeContentView did not change,
although nsIXULTreeBuilderObserver did and is not here.

Comment 35

[Jan Varga [:janv]]

Comment on attachment 157813 [details] [diff] [review]
iid changes

Neil, you're right about nsIXULTemplateBuilder, but nsITreeContentView did
change.
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&f
ile=nsITreeContentView.idl&branch=&root=/cvsroot&subdir=mozilla/layout/xul/base
/src/tree/public&command=DIFF_FRAMESET&rev1=1.7&rev2=1.8

I'll fix nsIXULTemplateBuilder.idl before checkin

Comment 36

[Asa Dotzler [:asa]]

Comment on attachment 157813 [details] [diff] [review]
iid changes

too late for a4.

Comment 37

[Darin Fisher]

Comment on attachment 157813 [details] [diff] [review]
iid changes

sr=darin in general for bumping IIDs whenever interfaces change.

please please don't change interfaces without bumping IIDs... it's the one
saving grace of XPCOM that allows us to have an unfrozen API and still support
C++ extensions... sort of.

Comment 38

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

Attachment: port listbox.xml change to toolkit (checked in)

this patch also contains the fix for bug 236068

Comment 39

[Jan Varga [:janv]]

the patch looks good, r=varga

Comment 40

[Mike Connor [:mconnor]]

Comment on attachment 167154 [details] [diff] [review]
port listbox.xml change to toolkit (checked in)

fun fun fun

Comment 41

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

Comment on attachment 167154 [details] [diff] [review]
port listbox.xml change to toolkit (checked in)

toolkit port checked in

Comment 42

[Worcester12345]

Fixed?

Comment 43

[Worcester12345]

(In reply to comment #42)
> Fixed?