Opened 9 years ago

Closed 9 years ago

Last modified 4 years ago

#9575 closed enhancement (fixed)

Tree: Add an option to the TreeStoreModel for defering loading of child items

Reported by: Kris Zyp Owned by: Kris Zyp
Priority: high Milestone: 1.4
Component: Dijit Version: 1.3.0
Keywords: Cc:
Blocked By: Blocking:

Description

Currently when you expand a node in the tree, the TreeStoreModel? will call loadItem for each child (that isn't loaded) of the expanded node, which can result in a network call for each child (which could be a large number) and can be very inefficient. An option should be added that will load the item for the current node (the parent item of the child items to be shown) when it is expanded (rather than the child items). Stores can then provide partially loaded child items that will include sufficient attributes for the label and indication of possible children for rendering. The entire full item (including a full list of children items and their labels) can then be loaded (via loadItem) when it is actually expanded, and lazy loaded trees can be built with optimal efficiency.

Attachments (3)

testDeferChildren.patch (1.8 KB) - added by Kris Zyp 9 years ago.
Lazy loading test of Tree with deferred child loading
deferChildrenLoading.patch (2.0 KB) - added by Kris Zyp 9 years ago.
test_Tree_with_JRS.patch (4.8 KB) - added by Kris Zyp 9 years ago.
automated test for Tree with JsonRestStore?

Download all attachments as: .zip

Change History (21)

Changed 9 years ago by Kris Zyp

Attachment: testDeferChildren.patch added

Lazy loading test of Tree with deferred child loading

comment:1 Changed 9 years ago by bill

We've had this discussion before in other tickets. JsonRestStore is special in that store.getLabel() etc. works on an item even though the item isn't loaded. That's not what the dojo.data spec says. I'm reluctant to add an option to Tree to account for stores that deviate from the spec like this.

I also don't see why an option needs to be added to Tree at all. Why not just subclass TreeStoreModel?

comment:2 in reply to:  1 Changed 9 years ago by Kris Zyp

Replying to bill:

We've had this discussion before in other tickets.

Maybe you could refer me to the rationale for not supporting this then?

JsonRestStore is special in that store.getLabel() etc. works on an item even though the item isn't loaded. That's not what the dojo.data spec says.

Where does say that? AFAICT, dojox.data.api.Read.js says nothing about getLabel not working for items that have not had been loaded, nor can I find any other prohibition of partial loading. By my reading, JRS is acting according to both the intent and letter of the API. Can you refer me to where it says otherwise?

I also don't see why an option needs to be added to Tree at all. Why not just subclass TreeStoreModel?

I can do that, but that much functionality improvement with that small of patch? You seriously think its worth a subclass?

comment:3 Changed 9 years ago by Jean-Rubin Leonard

Kris, thank you very much for such a fast turnaround. This is gonna have a tremendous impact on applications performance as it will drastically reduce the number of server calls. When do you think this can be committed? It would be nice if it could be done asap. The alternative is to reproduce the changes in my local files and that is subpar because I know other people could use a more efficient tree.

Thanks again, JR

comment:4 Changed 9 years ago by bill

Maybe you could refer me to the rationale for not supporting this then?

As you mentioned in your attached patch, you are talking about partially loaded items. dojo.data doesn't support this concept... the discussion we had before was about having a new dojo.data API like isAttrLoaded(), so we could detect if the attributes we needed were already loaded (into an item that was partially but not fully loaded), but neither Jared nor I wanted to complicate the dojo.data API like that.

AFAICT, dojox.data.api.Read.js says nothing about getLabel not working for items that have not had been loaded, nor can I find any other prohibition of partial loading.

Strangely the Read API doesn't seem to say anything at all about what "loaded" means, but you can see an example in the http://docs.dojocampus.org/quickstart/data/usingdatastores/lazyloading#lazy-loading manual about calling loadItem() before calling getLabel(). It's not safe to call any functions on an item that isn't [fully] loaded, unless you happen to know that for your data and data store, even though isLoaded() returns false those attributes are safe to query.

Keep in mind it's not limited to getLabel(). Tree can also inspects an item's attribute(s) to figure out which icon to use, which CSS class names to apply... basically we are talking about the item being completely loaded except for the children attribute.

Anyway, there's no reason to add a parameter to Tree; it makes more sense to add the parameter to TreeStoreModel. I'll think about it.

I agree that it's terrible to have all those unnecessary XHR's.

comment:5 Changed 9 years ago by Kris Zyp

Just to be clear, I am certainly not asking for any explicit support for partial loading in dojo.data. I agree that isAttrLoaded() is excessively complicated. The dojo.data API is (very wisely) silent on the issue (and as far as I can tell the link you gave is neither normative nor attempts to dictate otherwise), thus affording the opportunity for stores to have flexibility to extend their functionality. The primary request of this ticket is not to attempt to change the dojo.data API in anyway, but rather avoid unnecessary assumptions and allow users to take advantage of a different loading possibility (that is perfectly feasible within the constraints of dojo.data).

Anyway, there's no reason to add a parameter to Tree; it makes more sense to add the parameter to TreeStoreModel?. I'll think about it.

The parameter is in TreeStoreModel?, but I was simply following the pattern used by the Tree of passing along store/model relevant properties on to the model (in _store2model). That seems to make it much more convenient for devs (since they don't have to create a model themselves).

comment:6 Changed 9 years ago by bill

Notice though the deprecation warning in _store2model():

dojo.deprecated("Tree: from version 2.0, should specify a model object rather than a store/query");

Although it's easier (in this case) for a user to just specify a single Tree widget, rather than explicitly creating a TreeStoreModel or ForestStoreModel, the problem is that there are multiple models implementations (see the i18n demo for an example) and I don't want to hardcode Tree to use one, even as a default... in the same way that ComboBox isn't hardcoded to create an ItemFileReadStore if no store is specified. Obviously there are different philosophies about this sort of thing.

Anyway, if you can write an automated test for this option to TreeStoreModel, or rather an automated test for Tree running against JsonRestStore, then I'll be happy to add the option in to TreeStoreModel.

Changed 9 years ago by Kris Zyp

Attachment: deferChildrenLoading.patch added

comment:7 Changed 9 years ago by Kris Zyp

I attached an updated patch to keep this as only a TreeStoreModel? parameter. As far as the tests, I am not sure I understand. Is this attached test case not sufficient?

comment:8 Changed 9 years ago by bill

Oh thanks actually I didn't notice that attached test case (I see now you added it 3 days ago), but I meant something (in addition to what you already have) to automatically check the results, like http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/robot/Tree_a11y.html.

Maybe that's a bad example because it's got a bunch of tests in it and it's rather convoluted, whereas for this all we need is to open a tree a few levels, making sure that the children and grandchildren display correctly (right number of children, right labels, right expando icon). Not sure it even needs to be a robot test although that may be the path of least resistance... it does need doh.Deferred() to wait for tree to load items from the store, and for the wipeIn expansion to finish.

comment:9 Changed 9 years ago by Kris Zyp

I've never worked with robot tests before, where can I find the tests for the existing lazy loading behavior to base mine off of? Or would it make more sense to directly unit test TreeStoreModel? (since I am not changing the Tree at all anymore)? Where are the unit tests for TreeStoreModel??

comment:10 Changed 9 years ago by bill

Hi Kris,

There aren't any specific tests for TreeStoreModel; it's just being tested via the Tree tests. Might be nice to have some but anyway I'd like to first get end-to-end tests to make sure that Tree is working with JsonRestStore.

There aren't special tree tests for lazy loading behavior either, but the existing tests all assume a certain delay between a user action and the results appearing, even though they are using ItemFileReadStore. For example, in Tree_a11y.html there's the code below, which presses a key and then 500ms later checks that a certain result occurred:

{
	name:"Second 'A' key goes to Asia",
	timeout:4000,
	runTest:function(){
		var d = new doh.Deferred();
		
		// From Africa node, press "A" again.   Should go to Asia.
		doh.robot.keyPress("a", 100);
		doh.robot.sequence(d.getTestCallback(function(){
			var focus = dijit.getEnclosingWidget(dojo.global.dijit._curFocus);
			doh.t(focus, "there is a focused widget");
			doh.is("Asia", focus.label);
		}), 500);

		return d;
	}
},

Probably a better example is the non-robot test in dijit/tests/Tree.html, with code such as:

function openANode(){
	var d = new doh.Deferred;

	var asia = myTree.rootNode.getChildren()[1];

	doh.is(0, asia.getChildren().length, "no children loaded yet");

	myTree._onExpandoClick({node: asia});

	setTimeout(d.getTestCallback(function(){
		// Give the tree time to load the children, and the do checks that it
		// loaded correctly

		var children = asia.getChildren();
		doh.is(4, children.length, "four children");
		doh.is("China", children[0].label, "first child");
		doh.is("Mongolia", children[3].label, "last child");
	}), 1000);
	return d;
},

(The above code clicks an expando node and then after 1000ms checks that the children all loaded correctly.)

Note that the main difference b/w the robot tests and the non-robot tests, besides robot tests doing keyboard and mouse operations, is that the robot tests are split into two files, one with the widgets and another with the test commands to run against them. (In other words, Tree_a11y.html loads in test_Tree.html)

Changed 9 years ago by Kris Zyp

Attachment: test_Tree_with_JRS.patch added

automated test for Tree with JsonRestStore?

comment:11 Changed 9 years ago by Kris Zyp

Attached automated tests, hopefully that will be sufficient to be able to commit this patch.

comment:12 Changed 9 years ago by bill

Owner: set to Kris Zyp

Cool, looks good except that I'm getting errors on IE6 when running it (about the deferred getting called twice). I noticed Tree.html had the same problem, so I reduced the tests to run in <1000ms and it went away.

Other than that, looks great, feel free to checkin (after it's running on IE) and add the new test to dijit/tests/modules.js.

Thanks for adding this!

PS: oh the tabbing is strange in the comment for deferItemLoadingUntilExpand, and it ends in a half-finished sentence.

comment:13 Changed 9 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

(In [19971]) Adds support for deferring full loading of an item until its node is expanded (partial loading of children nodes), fixes #9575

comment:14 Changed 9 years ago by Kris Zyp

(In [19972]) Fixes documentation on deferItemLoadingUntilExpand, Refs #9575

comment:15 Changed 9 years ago by bill

Summary: Add an option to the Tree for defering loading of child itemsTree: Add an option to the TreeStoreModel for defering loading of child items

Thanks for getting this in.

comment:16 Changed 5 years ago by Bill Keese <bill@…>

In c973939b549322652ce31384e2f85c089e361493/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:17 Changed 4 years ago by Bill Keese <bill@…>

In 1346991f22464b3b30e264a63f273dba798ab282/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:18 Changed 4 years ago by Bill Keese <bill@…>

In c17a0f367ad27847e6d1ac2962635d003f690a16/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.