Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#9379 closed defect (fixed)

FileStore: Tree displays a leaf.gif instead of a folderClosed.gif

Reported by: Simon Speich Owned by: Jared Jurkiewicz
Priority: high Milestone: 1.4
Component: Data Version: 1.3.0
Keywords: FileStore, data, tree, empty children array Cc:
Blocked By: Blocking:

Description

When I use the FileStore? to remotely access a filesystem and then display it as tree, an empty folder is not displayed as folder but as a file. This is the case because the data items children array is empty and the tree displays a leaf.gif instead of a folderClosed.gif

It think there should be a way do differentiate between no children array and an empty children array, especially with FileStore?, e.g.

no children array = leaf.gif
empty children array = no +- expando but folderClosed/Open icon.
children array = +- expando and folder icon

Attachments (3)

tree.htm (2.0 KB) - added by Simon Speich 11 years ago.
simple example showing that an empty children array is not displayed as a folder (e.g. South America)
ItemFileReadStore_hasAttribute.patch (1.7 KB) - added by Jared Jurkiewicz 11 years ago.
Patch to IFS + minor tweak to better trap errors in a testcase.
FileStore.patch (1.4 KB) - added by Jared Jurkiewicz 11 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 11 years ago by bill

This is possible already. Just override the model's mayHaveChildren() method to return true whenever the item is a folder. Alternately override the Tree's getIconClass() method.

The scenario you described above:

no children array = leaf.gif empty children array = no +- expando but folderClosed/Open icon.

... that's how TreeStoreModel works already: {{[ mayHaveChildren: function(/*dojo.data.Item*/ item){

summary: Tells if an item has or may have children. Implementing logic here avoids showing +/- expando icon for nodes that we know don't have children. (For efficiency reasons we may not want to check if an element actually has children until user clicks the expando node) return dojo.some(this.childrenAttrs, function(attr){

return this.store.hasAttribute(item, attr);

}, this);

}, }}}

So I'm guessing that FileStore? doesn't have an empty children array for empty folders?

comment:2 Changed 11 years ago by Simon Speich

Hi thank you for your explanation. I'm pretty new to dojo, so it will take me a while to understand.

This is the data item of the empty folder that gets returned from the server response by filestore_dojotree.php (among other data items) :

{"name":"emptyfolder"
,"parentDir":".","path":".\/emptyfolder","directory":true,"size":0,"modified":1242992137,"children":
[]}

, but it's not displayed as a folder, even though children is an empty array.

comment:3 Changed 11 years ago by bill

Hmm that is weird, it looks like it has a children array... well can you attach a small testcase using the attach file button? That's our procedure (ie, requirement) before looking at bugs.

comment:4 in reply to:  3 Changed 11 years ago by Simon Speich

Replying to bill:

Hmm that is weird, it looks like it has a children array... well can you attach a small testcase using the attach file button? That's our procedure (ie, requirement) before looking at bugs.

ok, I will produce a test case.

Changed 11 years ago by Simon Speich

Attachment: tree.htm added

simple example showing that an empty children array is not displayed as a folder (e.g. South America)

comment:5 Changed 11 years ago by Simon Speich

That path to the dojo library has to be adapted in the example.

comment:6 Changed 11 years ago by bill

Component: DijitData
Owner: set to Jared Jurkiewicz

Ah OK, thanks for the example.

Jared - I traced this through and both FileStore and ItemFileReadStore have a bug that hasAttribute() returns false if the attribute exists but is an empty array.

Looks like FileStore.getValue() also has a bug in that it returns defaultValue rather than []. (Maybe ItemFileReadStore has the same bug but I didn't trace it down.)


Simon -

I think you could work around this with some code like:

store.hasAttribute = function(item, attribute){ return (attribute in item); };

Actually that's probably how hasAttribute() should be implemented in both FileStore and ItemFileReadStore.

comment:7 Changed 11 years ago by Simon Speich

Ok, I'll try that. Thank you.

Changed 11 years ago by Jared Jurkiewicz

Patch to IFS + minor tweak to better trap errors in a testcase.

comment:8 Changed 11 years ago by Jared Jurkiewicz

IFS Store returns correctly for getValue, an empty array, as does FileStore?. getValue is supposed to return a singleton value, not an array. getValues() returns an array.

The hasAttribute check, though, does need fixing.

comment:9 Changed 11 years ago by Jared Jurkiewicz

(In [17772]) Minor fix to IFS with regards to hasAttribute, allows identifying empty multi-value attributes. \!strict refs #9379

Changed 11 years ago by Jared Jurkiewicz

Attachment: FileStore.patch added

comment:10 Changed 11 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [17774]) FileStore? patches for empty child dir list and same fix for AORS. \!strict fixes #9379

comment:11 Changed 10 years ago by Jared Jurkiewicz

Clarifying my comment:

FileStore? and IFS work correctly for getValue. getValue is defined to return a *singleton* value. An array as value internally usually means (and in these cases do mean), the attribute is a multi-valued attribute. Therefore, getValue should return either the first entry of the array, or defaultValue. getValues should return the array itself.

That's why there is a getValue and getValues in the first place. Single versus multi-valued attributes.

comment:12 Changed 10 years ago by Jared Jurkiewicz

And FileStore?'s docs even denote that children is multi-value and state it should be accessed through the multivalued accessor:

http://docs.dojocampus.org/dojox/data/FileStore#item-structure

comment:13 Changed 10 years ago by Simon Speich

Hi jaredj
I tested the patches for IFS and FS with a tree. Now the empty folder is displayed correctly with the icon folderClosed.gif, but the expando icon treeExpand_plus.gif is still displayed instead of a treeExpand_leaf.gif. Only after clicking on the + icon the correct icon is set. Should I file a new bug for the tree?

comment:14 Changed 10 years ago by bill

Hi again Simon,

I think that's a question for me :-). This behavior is intentional... the reason is that we can't know to use treeExpand_leaf.gif unless we query the item for children, which in the general case could be expensive. (In the general case it might require an XHR request to the server.) FWIW you can see the same "problem" in Windows explorer when you explore network drives.

Having said that though, with FileStore that information is already there, at no cost.

So, I could change the default mayHaveChildren() implementation to return true or false to mean:

  • definitely does have children
  • definitely doesn't have children

rather than the current return code of:

  • might have children
  • definitely doesn't have children

However, ironically, that would require you to override the Tree's getIconClass() method to be smarter about picking the icon depending on whether the item is a directory or not, like this:

new Tree({ ... getIconClass: function(item){
		return (!item || store.getValue(item, 'directory')) ? (opened ? "dijitFolderOpened" : "dijitFolderClosed") : "dijitLeaf"
} ... })

Would be nice if that was automatic but given the API's involved (that the tree model can connect to any model which connect to any data store), I don't see a way to make it automatic.

What do you think?

comment:15 Changed 10 years ago by bill

PS: the change to mayHaveChildren() is to simply check both that the children attribute exists, and it has one or more entries, something like:

mayHaveChildren: function(/*dojo.data.Item*/ item){
	return dojo.some(this.childrenAttrs, function(attr){
		return this.store.hasAttribute(item, attr) &&
                            this.store.getValues(item, attr).length > 0;
	}, this);
},

comment:16 Changed 10 years ago by Simon Speich

Hi bill

Thank you for taking the time to explain. So saying it with my own words, the point is, that even if children array is empty there might be children, we just don't know that at this point.

So all i can think of is that each data stores has to identify itself with an property: i am of the type definitelyHaveChildren=true or definitelyHaveChildren=false. But that's probably not generic enough?

comment:17 Changed 10 years ago by bill

Sorry, that's not quite it.... this is hard to explain and probably a much longer explanation than you wanted. The basic question comes down to whether or not it's worth fixing this "bug" about the expando-icon of empty folders, or considering the current behavior as expected.

First of all, in FileStore, if the children array is empty, there are no children. But in some other data formats from other stores, we can't tell whether an item has children without doing another fetch. For example, consider this data, which you might see in an ItemFileReadStore:

{name: 'folder1', type: 'directory'},
{name: 'file1', type: file, parent: 'folder1'}

Just looking at the folder1 item doesn't tell you whether it has children; you need to query for items with parent==folder1. (In the above example it turns out that folder1 has one child.)

Tree can display data like above by using a different model instead of TreeStoreModel or ForestStoreModel. See for example source:demos/trunk/i18n/model.js. (Remember that dijit.tree.model is a *interface* that has different implementations)

Currently, dijit.tree.model has a method called mayHaveChildren(), which dijit.Tree is unfortunately using for two separate (conflicting) purposes:

  • test if the item is a folder
  • test if the item either has children, or may have children but we don't know

Testing if an item is a folder is best done by checking for some other attribute, like type=folder or directory=true, but to make things generic we can check if there is a children attribute or not:

	return dojo.some(this.childrenAttrs, function(attr){
		return this.store.hasAttribute(item, attr);
	}, this);

That's the current implementation of mayHaveChilden(). OTOH, for data stores like dijit.tree.TreeStoreModel, with a "children" attribute, the way to test if an item has children or not is to check if there's a children array that's non-empty:

	return dojo.some(this.childrenAttrs, function(attr){
		return this.store.hasAttribute(item, attr) &&
                            this.store.getValues(item, attr).length > 0;
	}, this);

In summary, to make empty folders initially have the right expando icon, and to make folders have the right icon probably requires fixing TreeStoreModel.mayHaveChildren() to the second implementation above, and then adding a new method to the dijit.tree.model interface called isFolder(). And I was just wondering if that was worth it, or if we should leave things as is.

comment:18 Changed 10 years ago by bill

Milestone: tbd1.4

Anyway, the dojo.data problem has been fixed and the only remaining issue is w/the expando icon, which I've filed as a separate ticket, #9379.

comment:19 Changed 10 years ago by bill

Oops, this is ticket #9379 :-). I meant #9413...

Note: See TracTickets for help on using tickets.