835916 - Add a TreeViews.jsm helper module for XUL trees

Status: RESOLVED WONTFIX

(Toolkit :: UI Widgets, defect)

Description

[Alex Vincent [:WeirdAl]]

Tree views are notoriously complicated and difficult to get right.  I am working on a TreeViews.jsm module to provide helpers for this, and plenty of chrome mochitests to confirm that the tree views it provides work correctly.

In particular, I'm looking to build a reusable "tree object model", where objects map to rows (one object to at least one row), and the cells' values, texts and properties (as well as row properties) are defined on the model itself.  In the next few days I should have an early prototype with tests for cell texts only.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: very early work in progress

I need some help here.  Whichever XUL iframe (reference or test) appears first, the tree inside has a horizontal scrollbar.  The second iframe's tree does not have a horizontal scrollbar.  This is true even when I swap the iframes in test_grandchildren.xul.

At a glance, the iframes appear identical except for that scrollbar.

Steps to reproduce:
(1) Apply the patch from bug 833879
(2) Apply this patch
(3) Build with --enable-tests
(4) TEST_PATH=layout/xul/tree/ pymake mochitest-chrome

Test fails, but you can capture the output from SnapshotWindow.js in the debugging output.  Rendering those data:image/png images shows the first iframe's tree always has a scrollbar and the second iframe's tree never does.

roc: feedback request is simply to confirm this bug.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry Alex, but honestly I don't care about XUL trees at all. If you find a bug and come up with a fix, I'll review it though.

Comment 3

[Asaf Romano (gone)]

You could ask either Neil or me for review when you're ready.

By the way, the module should go in toolkit/content.

Comment 4

[Alex Vincent [:WeirdAl]]

:( A timeout of 100 milliseconds seems to have resolved it, for now.  But I don't like that:  timeout-based tests have a way of failing randomly.  Is there a XUL event I can listen for when a tree or iframe is redrawn?

Comment 5

[Alex Vincent [:WeirdAl]]

I figured it out - a little mutation observer glue on tree@hidehscroll and I'm back in business.

Comment 6

[Alex Vincent [:WeirdAl]]

I'm looking at the list of nsITreeView features, and I have quite a bit to support.  Mano, I'm wondering if I should do this in incremental stages, or have one big patch with lots of testcases and lots of support in play first.

The TreeObjectModel feature list includes:
(+) Reading an object's properties for cell texts
( ) Reading an object's properties for cell values
(+) Generating hierarchical trees based on an user-provided .getChildObjects method
(+) Toggling whether a row is open or not
( ) Selecting cells and rows
( ) Editing cells
( ) Providing methods for notifying the tree view the objects shown have changed
( ) Styling of cells, rows and columns (depends on bug 407956)
( ) Progressmeter and checkbox column types
( ) Adding separator rows
( ) Cycling headers and cells
( ) .getImageSrc (whatever that does)
( ) performAction* notifications

What's marked (+) I have implemented with tests.

Comment 7

[Alex Vincent [:WeirdAl]]

Attachment: patch with tests

This should be a pretty good start.  :)  My goal was for parity with nsTreeContentView, at least as a starting point.

(+) Reading an object's properties for cell texts
(+) Reading an object's properties for cell values
(+) Generating hierarchical trees based on an user-provided .getChildObjects method
(+) Toggling whether a row is open or not
(+) Selecting cells and rows
(+) Editing cells
(+) Providing methods for notifying the tree view the objects shown have changed
(+) Styling of cells, rows and columns (depends on bug 407956)
(+) Progressmeter and checkbox column types
(+) Adding separator rows
(+) Showing icons in cells
( ) Sorting columns
( ) performAction* notifications
( ) Cycling cells

I tried taking a stab at sorting by column, but I just couldn't figure out what nsTreeContentView's patterns were for that.

performAction* methods are currently unused in the tree (bug 406588 ).

.cycleCell I don't understand yet.

On the mutations test, I probably went a little overboard with the combinations.  My goal was to show that the model could safely make adjustments at the beginning, middle and end of a tree.

Comment 8

[Alex Vincent [:WeirdAl]]

Comment on attachment 715071 [details] [diff] [review]
patch with tests

Apparently I haven't worked out all of the kinks yet... :(

Comment 9

[Alex Vincent [:WeirdAl]]

Something changed in the last three weeks:  my custom tree views no longer have their open/closed twisty icons on them.  The reference views (from nsTreeContentView) do, and the CSS & XUL markup are the same as far as the twisty icons should be concerned.

My code never did anything special regarding the twisty icons, so this doesn't make sense.

Comment 10

[Alex Vincent [:WeirdAl]]

Attachment: revised patch with tests

I've worked out the kinks, and updated my tests to reflect that.

Comment 11

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 732692 [details] [diff] [review]
revised patch with tests

Review of attachment 732692 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/TreeViews.jsm
@@ +744,5 @@
> +  },
> +
> +  hasNextSibling: function(index, afterIndex) {
> +    var thisLevel = this.getLevel(index);
> +    for (var t = after + 1; t < this.visibleData.length; t++) {

What's visibleData?

Comment 12

[Alex Vincent [:WeirdAl]]

(In reply to Colby Russell :crussell from comment #11)
> > +    for (var t = after + 1; t < this.visibleData.length; t++) {
> 
> What's visibleData?

Whoops, that whole line is wrong.  It should say:

for (var t = afterIndex + 1; t < this.visibleRows.length; t++) {

Comment 13

[Asaf Romano (gone)]

Comment on attachment 732692 [details] [diff] [review]
revised patch with tests

I think that in order for use to evaluate this API, it would be good to take some existing complex tree usage we have
and convert it. Then we'll be able to tell whether or not this is the right approach. It seems so to me, but "seems" is never enough
with regard to new APIs.

I think the places tree view is an excellent candidate.

***
And now to the patch. This is the first batch of comments. They will probably result in a lot of refactoring so I'll hold with the rest of my nitty comments until they are addressed.

First of all, I think this should be exposed through tree.xml somehow, preferably through a lazy getter.

>diff --git a/toolkit/content/Makefile.in b/toolkit/content/Makefile.in
>--- a/toolkit/content/Makefile.in
>+++ b/toolkit/content/Makefile.in

toolkit/modules now.

>diff --git a/toolkit/content/TreeViews.jsm b/toolkit/content/TreeViews.jsm
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/content/TreeViews.jsm

>+function returnFalse() {
>+  return false;
>+}
>+
>+function returnEmptyString() {
>+  return "";
>+}
>+

Remove these please.

>+/**
>+ * A generic nsITreeView implementation for dumping to the stderr console.
..
>+function TreeViewLog(next, id) {
..
>+TreeViewLog.prototype = {
>+  log: function(methodName, args) {
>+    dump((new Date).toLocaleTimeString() + " ");
>+    dump("TreeViewLog " + this.id + " " + methodName + "(");

|dump| isn't always the right choice. Sometimes one would prefer console.log, some other times we may want to save to a file.

A serialization-like concept would work better here, I think.

>+    for (var i = 0; i < args.length; i++) {

In toolkit & browser, |let| should always be preferred over |var|, esp. in local scopes like this.
But in cases like this you should just use for (...of...).


The comment about |let| vs. |var| applies to the entire patch.


>+// methods
>+["getRowProperties", "getCellProperties", "getColumnProperties", "isContainer",
>+ "isContainerOpen", "isContainerEmpty", "isSeparator", "isSorted", "canDrop",
>+ "drop", "getParentIndex", "hasNextSibling", "getLevel", "getImageSrc",
>+ "getProgressMode", "getCellValue", "getCellText", "setTree", "toggleOpenState",
>+ "cycleHeader", "selectionChanged", "cycleCell", "isEditable", "isSelectable",
>+ "setCellValue", "setCellText", "performAction", "performActionOnRow",
>+ "performActionOnCell"].forEach(function(methodName) {
>+    this[methodName] = function() {
>+      return this.logWithCallback(methodName, arguments);
>+    };
>+}, TreeViewLog.prototype);
>+

But if what you want is for the main object methods to LOG some output only in debug mode. What we usually do for that
is: (*) declare some LOG method in the global scope, that loga conditionally (see other modules and components). (*) Call it unconditionally.


>+  reflectProperties: function(colElement, userFields, userSetter) {

I think that the column should be defined _once_, preferably in the constructor, but either way in a single method that takes something of this form: 
[
 {
   columnElement: THE TREECOL ELEMENT,
   userFields: A JS MAP,
   userSetter: OPTIONAL, A JS FUNCTION
 },
 {
   columnElement: THE TREECOL ELEMENT,
   userFields: A JS MAP,
   userSetter: OPTIONAL, A JS FUNCTION
 },
 ...
}


>+    var fields = {
>+      text:  this.propConverter(userFields.text),
>+      image: this.propConverter(userFields.image),
>+      mode:  this.propConverter(userFields.mode),
>+      value: this.propConverter(userFields.value),
>+      isSelectable: this.propConverter(userFields.isSelectable),
>+      isEditable: this.propConverter(userFields.isEditable),
>+      properties: []
>+    };
>+

Use Array.Map or (even better) array comprehensions syntax (check other cases across the patch please).

>+    if (Array.isArray(userFields.properties)) {
>+      userFields.properties.forEach(function(p) {
>+        fields.properties.push(this.propConverter(p));
>+      }, this);

ditto.


>+  defineRowStyling: function(userStyles) {
>+  defineColumnStyling: function(colElement, userStyles) {
The current setup is misleading, by passing in a function nothing "live" happens... The
function(s) runs only once. Since we cannot make it live, I suggest a simpler apis

styleRows(either-one-row-or-more, one-or-more-styling-selectors)
styleColumns ...
styleCell ...


>+  /**
>+   * Get the child objects of a parent object per its own object model.
>+   *
>+   * @param obj {Object} The parent object in question.
>+   *
>+   * @note This method should be overridden and defined by the user!
>+   */
>+  listChildObjects: function(obj) {
>+    return {
>+      isOpen: false,
>+      children: []
>+    };
>+  },
>+

s/list/get/

>+  /**
>+   * Get an object representing a generic tree separator row.
>+   *
>+   * You should add getSeparator() instances into listChildObjects().children to
>+   * force a tree separator row to render.
>+   */
>+  getSeparator: function() {
>+    return new TreeSeparator;
>+  },
>+

A insertSeperatorAt method is probably better.

but s/get/new if it says.

Please use JS lamdas when you can.

Comment 14

[Alex Vincent [:WeirdAl]]

(In reply to Mano from comment #13)
> Comment on attachment 732692 [details] [diff] [review]
> revised patch with tests
...
> Please use JS lamdas when you can.

Example, please?  I've never used them before.

(It'll likely be mid-August before I can revise this patch.)

Comment 15

[Alex Vincent [:WeirdAl]]

[11:33]	Mano	WeirdAl: https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.8#Expression_closures_%28Merge_into_own_page.2Fsection%29
[11:33]	Mano	btw I think we want something like
[11:33]	Mano	<tree objectmodel="true">

Comment 16

[Alex Vincent [:WeirdAl]]

XUL trees are going away, per bug 1446335.