PDA

View Full Version : Breaking change in Treegrid



bseddon
21 Apr 2013, 7:58 AM
I'm logging the issue described in this post as a bug because 4.2 introduces a backwards incompatibility which is apparent when adding nodes programmatically (rather than, say, through a store proxy). I'm reporting it so others who encounter a hung application when using 4.2 don't have to spend a day working out why.

Fortunately the workaround is relatively easy (see 'workaround' below) though impeded by the other breaking change (http://www.sencha.com/forum/showthread.php?261805-Breaking-change-in-TreeStore-event-handling-behavior).

I apologize for not trying the beta version. Before I could use 4.2 I had learn how to compile the theme I am using and make it work with 4.2.

Problem

The getPath() method of the NodeInterface which is used to decorate tree store records returns a handy path to any given node in the form:

/root/my-level-1-node-id/my-level-2-node-id/etc-id

The value of the components of the path ('my-level-1-node-id', 'my-level-2-node-id', 'etc-id') are the node.data.id values of the nodes along path.

In versions before 4.2 it didn't matter whether the node.data.id is set explicitly or as part of a configuration object. These two are equivalent:

var newNode = node.appendChild({name: 'xxx', icon: '/icon/myicon.png', id: 'xxx'});

var newNode = node.appendChild({name: 'xxx', icon: '/icon/myicon.png'});
neeNode.data.id = 'xxx';

In 4.2 these two are also equivalent however the first case generates a problem when any two nodes on the path have the same id. That is, the result of node.getPath() is something like:

/root/xxx/yyy/xxx

The configuration might look like:

{name: 'Xs', id: 'xxx', leaf: false, expanded: true,
children: [
{name: 'Ys', id: 'yyy', leaf: false, expanded: false,
children: [
{name: 'Xs', id: 'xxx', }
]}
]}

When using 4.2, if the node configuration object is used to set the id and the same id is repeated, the application will hang. I've attached a sample application which illustrates this point and that when using the same application but with 4.1 the application does not hang.

To use the application with 4.2, unzip it then navigate to the app.html file and follow the instructions in the text area at the bottom of the application window. To use the application with 4.1 navigate to the app410.html file and, again, follow the instructions in the text area.

The issue seems to be that when the ids are set using a configuration object the row checking algorithm of doSelect() becomes confused. It is unable to determine which row with id 'xxx' to use and flip-flops between them.

Workaround

Simple: never set the id in the configuration object, always set it explicitly.

For me, this is a bug for two reasons. 1) its backward incompatible and an existing application will hang without any explanation.; 2) there's no explicit warning that setting the id via configuration is not allowed.

evant
21 Apr 2013, 5:27 PM
There are 2 major problems with the test case:

1) As you said, you're reaching into the data object and changing a property so that the model has no way to react.
2) There's duplicate id's being generated in the tree.

The id needs to be unique throughout the tree, so that certainly needs to be resolved in the application code.

For #1, the id of the node is used to keep a relation to the underlying DOM elements, so by changing it in this fashion it's causing the association to get clobbered.

For example, if you change the code to generate unique id's and use model::setId:



/*
* File: app/controller/MyController.js
*
* This file was generated by Sencha Architect version 2.2.1.
* http://www.sencha.com/products/architect/
*
* This file requires use of the Ext JS 4.2.x library, under independent license.
* License of Sencha Architect does not include license for Ext JS 4.2.x. For more
* details see http://www.sencha.com/license or contact license@sencha.com.
*
* This file will be auto-generated each and everytime you save your project.
*
* Do NOT hand edit this file.
*/

var foo = 0;
Ext.define('MyApp.controller.MyController', {
extend: 'Ext.app.Controller',

refs: [{
ref: 'TextArea',
selector: '#textarea',
xtype: 'Ext.form.field.TextArea'
}],

init: function(application) {
var app = this.getApplication === undefined ? this.application : this.getApplication();

var store = app.getMyTreeStoreStore();
// return;

store.on({
append: this.myAppended,
remove: this.myRemoved,
scope: this
});

var root = store.getRootNode();
root.removeAll(false);

var nodes = {
expanded: true,
children: [{
node: 'A',
info: 'Start of all As',
datetime: new Date(),
id: 'a',
leaf: false,
expanded: true,
children: [{
node: 'A.A',
info: 'A.As',
datetime: new Date(),
id: 'aa',
leaf: true
}, {
node: 'A.B',
info: 'A.Bs',
datetime: new Date(),
id: 'ab',
leaf: true
}, {
node: 'A.C',
info: 'A.Cs',
datetime: new Date(),
id: 'ac',
leaf: false,
expanded: true,
children: [{
node: 'A.C.A',
info: 'A.C.As',
datetime: new Date(),
id: 'aca',
leaf: true
}, {
node: 'A.C.B',
info: 'A.C.Bs',
datetime: new Date(),
id: 'acb',
leaf: true
}]
}, {
node: 'A.D',
info: 'A.Ds',
datetime: new Date(),
id: 'ad',
leaf: true
}]
}]
};

store.setRootNode(nodes);

},

myAppended: function(thisNode, newChildNode, index, eOpts) {

if (newChildNode.isRoot()) {
console.log("Appending root");
newChildNode.data.id = 'root';
} else {
newChildNode.setId(newChildNode.data.node.toLowerCase() + (++foo));
console.log("Node " + newChildNode.getPath() + " appended to " + thisNode.getPath());
}
},

myRemoved: function(thisNode, removedNode, isMoved, opts) {
console.log("Node " + removedNode.data.nodes + " removed from " + thisNode.getPath());

},

TestRule: function(includeId) {
var ta = this.getTextArea();

if (includeId) {
ta.setValue("Now select a node. Any node. Now select another. See it works OK. But next expand the 'Ts' node. Now select the 'incl' node. See, you are stuck.");
} else {
ta.setValue("Now select a node. Any node. Now select another. See it works OK. Expand the 'Ts' node. Now select the 'excl' node. It still works OK. This is expected.\n\nNow press the 'Test with id' button.");
}

var name = includeId ? 'incl' : 'excl';
var description = name + ' desc';

var app = this.getApplication === undefined ? this.application : this.getApplication();
var node = app.getMyTreeStoreStore().getRootNode();

// Look to see if a node of the name already exists
var nodes = node.findChildBy(function(child) {
return child.data.node.toLowerCase() == name.toLowerCase();
});

if (nodes !== null) {
console.log(Ext.String.format("A rule called {0} already exists", name));
return;
}

node.expand();
var newNode = node.appendChild({
node: name,
description: description,
leaf: false,
id: name.toLowerCase(),
datetime: new Date()
});
node.set('leaf', false);

console.log(newNode.getPath());

// Add the child type, scope and assertion nodes
var nodes = [{
name: "Ts",
desc: "Node to contain Ts"
}, {
name: "Ss",
desc: "Node to contain Ss"
}, {
name: "Rs",
desc: "Node to contain Rs"
}];

var nodeObjects = [];

Ext.Array.forEach(nodes, function(node, i) {
var nodeObject = {
node: node.name,
info: node.desc,
leaf: node.name !== "Ts",
id: includeId ? node.name.toLowerCase() : undefined,
datetime: new Date(),
qtip: node.desc
};

if (nodeObject.node === "Ts") {
nodeObject.children = [];

nodeObject.children.push({
node: name,
info: description,
builtin: false,
leaf: true,
id: includeId ? name.toLowerCase() : undefined,
datetime: new Date(),
qtip: description
});
}

nodeObjects.push(nodeObject);

});

var ruleNode = newNode.appendChild(nodeObjects);

newNode.expand();

}
});

bseddon
22 Apr 2013, 1:28 AM
Evan, thanks for your response. I have to be careful replying to you because you work on the code however I have to draw issue with some of your comments. The problem seems to be that the tree code has changed a lot between 4.1 and 4.2. The tree (and grids) seem more responsive and perhaps this improvement is a consequence of changing the tree.

<<The id needs to be unique throughout the tree, so that certainly needs to be resolved in the application code.>>

I appreciate that each dom element needs a unique id. In versions before 4.2 this did not rely on and was not impacted by the value of node.data.id.

In versions earlier than 4.2. The 'path' built from node.data.id had to be unique. A node.data.id within the childnodes of a single node had to be unique. However node.data.id did not have to be unique across the tree.

If the node.data.id (vs node.id) is being used to generate a DOM element id it surely will be a problem. The way you appear to describe it seems to mean the semantics of the node are tightly coupled with the semantics of the tree. However, trees are not grids and node associations are described by node.childnodes and node.parentNode and the implicit node paths.

What's been the motivation for the change made?

<<For #1, the id of the node is used to keep a relation to the underlying DOM elements, so by changing it in this fashion it's causing the association to get clobbered.>>

Sure, but this is a change in behavior (a backwards incompatible change).

<<For example, if you change the code to generate unique id's and use model: getId>>

Unfortunately in our case this is not an option. The example only tries to highlight the problem. In the real application the nodes ids are not hard coded. The result of NodeInterface.getPath() is used to identify changes made to the tree by a user so their representation on the server can be updated and persisted. If there were one user using the application, it might be possible to auto-generate ids but because the nodes can be edited by multiple people there needs to be some consistency to the ids of the nodes. As a result, we have a naming convention which is id to generate node.data.id values.

Regards

Bill

evant
22 Apr 2013, 3:18 AM
In versions earlier than 4.2. The 'path' built from node.data.id had to be unique. A node.data.id within the childnodes of a single node had to be unique. However node.data.id did not have to be unique across the tree.


This isn't correct. Since Ext 4.0 (& 3.x), there's been several elements that have required the node ids to be unique. For 4.x, specifically:

1) The Ext.data.Tree class keys nodes by their id in a single hash, so any type of id style lookup for a node relies on them being unique.
2) To display in the same manner as a grid, the nodes are flattened into an internal NodeStore class, so essentially it acts the same as a grid with a store. As such, that flat store requires unique identifiers for each record in the store.

There are other general things about the tree, for example when you're lazy loading nodes, it sends the id of the parent node along. There is no context provided. Same things happens with the parentId value that is stored on each node.

The suggestion I made about auto-generating the id was more just to demonstrate the issue with duplicates, you could certainly follow a predictable structure for generating identifiers that are unique.

bseddon
22 Apr 2013, 3:55 AM
I think my mental model of what's going on has been wrong so, perhaps, I've been missing your point.

There's 'node.id' and 'node.data.id'. So far as I can see, 'node.id' has always had to be unique and holds the ExtJS generated value which, I believe is used, as the DOM element id. By contrast, 'node.data.id' has been undefined by default (and is undefined in 4.2) but is used by node.getPath() to generate a path representation of a node hierarchy.

In my head I've been seeing the data from the configuration object as being the node.data values. But this probably isn't right. ExtJS is taking these values and applying them to the node.data object where appropriate based on the model specification and other times using them to fill values on the node object itself. The 'id' value is ambiguous as it could appear in both places. Some change in 4.2 means the id will *definitely* be used by the node object not just the node.data object causing my problem.

Anyway, the post was alert others to a change that can cause a problem. Assigning an 'id' value using a configuration parameter worked prior to 4.2 but no longer works in 4.2. It took a day to work out why our application stopped responding in some circumstances when we used 4.2. Maybe the information will save others the time and the workaround is easy enough.