Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#7058 closed defect (fixed)

Tree: can't drag from Tree, drop somewhere else (tree drag source "item" invalid)

Reported by: bill Owned by: bill
Priority: high Milestone: 1.4
Component: Dijit Version: 1.1.1
Keywords: Cc:
Blocked By: Blocking:

Description

According to the (de-facto) standard implied by dojo/dnd/Source.js, getItem() must return items structured like:

{data: ...,  type: ["TreeNode"] }

That's because when you drag from the Tree and drop somewhere else, the target expects the source's getItem() to return an object w/that structure.

Similarly setItem() should take the same structure.

However, in Tree, an item is the TreeNode.domNode.

Change History (15)

comment:1 Changed 11 years ago by bill

Owner: set to bill

I believe this can be achieved simply by doing mapping in getItem() and setItem().

comment:2 Changed 11 years ago by Goran Miskovic

I've bumped at this problem when trying to implement custom creator. By all means it would be the best if done as suggested but till then workaround could be:

if(this.creator){
	// use defined creator
	this._normalizedCreator = function(node, hint){
		if(!dijit.getEnclosingWidget) {
			dojo.require("dijit.dijit");
		}
		if (null != dijit.getEnclosingWidget(node)) {
			var item = dijit.getEnclosingWidget(node).item;
			return oldCreator.call(this, item, hint);
		} else {
			return oldCreator.call(this, source.getItem(node.id).data, hint);
		}
	};
}else{

Of course checkAcceptance must be overridden as well:

var letMeDropOnYou = {

	accept: true,

	items: [],

	acceptNode : function(source, node) {
		this.accept = true;
		this.items = source.getSelectedItems();
		
		for (var item in items) {
			//do what needed and set this.accept to false if appropriate
		}
		return this.accept;
	}
};

target.checkAcceptance = dojo.hitch(letMeDropOnYou, 'acceptNode');

Further on if one manage to drop tree node(s) still there is a problem: It seams that method "delItem" is not implemented in dijit._tree.dndSource. In order to avoid pressing ctrl key property "copyOnly" should be set to true. P.S. Shall I get into move/delete issue and fill it as new bug?

comment:3 Changed 11 years ago by bill

Sure, file a new ticket for the delete problem, thanks!

comment:4 Changed 11 years ago by bill

Milestone: 1.31.4

bumping 1.4 tickets to 1.5, and most 1.3 tickets to 1.4

comment:5 Changed 11 years ago by bill

For DnD sources, getItem() returns an object like:

{data: ..., type: text? }

I'm wondering if there is/should be a standard for drag objects corresponding to dojo.data store items.

Maybe:

{type: data?, data: {store: ..., item: ...} }

(Store seems necessary since there's no official way to map from an item to what store it belongs to.)

This would allow drops from Grid to Tree etc. without having to write special glue code for every pair of widgets

comment:6 in reply to:  5 Changed 10 years ago by alle

I think

{type: data?, data: {store: ..., item: ...} }

Could be a good standard.

I propose to make it a standard over dijit and maybe dojox too.
Someone agree or disagree?

Which widgets use dnd and need to be converted?

comment:7 Changed 10 years ago by bill

The only widgets I can think of using DnD with dojo.data items are Grid and Tree, but there must be some more.

If we did have a standard format for a drag sourceItem referencing a dojo.data.item, what would happen on DnD between widgets w/different stores? I can see three options:

  • Require that the caller provide a custom an extension function to do this. That's what Tree.dndSource.itemCreator() is.
  • Generic code to copy an item from one store to another, by using the same attribute names. I guess we would just cycle through the attributes in the original dojo.data.Item (by calling oldStore.getAttributes(oldItem) and oldStore.getValue(oldItem, "foo") or getValues() for each attribute), and then create a hash like {id: foo, name: "bar", foo: "zaz"} to pass that to newStore.newItem().
  • Something more complicated, trying to preserve id and label, accounting for the fact that store1 stores the id in the "id" attribute but store2 stores the id in the "identity" attribute (and similar for label). I guess we have no automated way to do that since getIdentityAttributes() and getLabelAttributes() only work per-item, and technically speaking there's no way to tell how to setup a new item.

Also, since I wrote that comment above about having a dojo.data type for drag items, I've realized that dragged items can have multiple types. Despite the fact that the attribute is called "type", it is actually an array. So, we could mark an item as having types text and dojo.data. For text items "data" is just a String though so the dojo.data.store and dojo.data.item would have to be stored in a different attribute. (ex: {id: foo, type: ["text", "dojo.data"], data: "hello world", dojoData: {store: ..., data: ...} }. Well, that's probably not worth worrying about.

comment:8 Changed 10 years ago by alle

I think Tree.dndSource.itemCreator() is the best way. It's needed for DnD and I think is wrong changing so many stores for an optional enhancement as DnD support is. dndSource is the place for any method needed to add DnD support to a widget.
More than that I've created widgets that thought a model interact with more that one store so requiring some kind of item copy method to a store that don't know widget logic and how it interact with data can start a mess because store is no more decoupled by widget logic.

Maybe a format like this can be good

{id: foo, type: ["myBestType", "someOtherType", "dojo.data", "text"], data: {myBestType: ..., someOtherType: ..., "dojo.data": {store: ..., item: ..., parentInfo?: ...}, text: "hello world" }

Target iterate on type(s) witch are in source favorite order. First supported type is chosen and data formatted following that type is processed.
I think a string can be the data format for "text" and {store: ..., item: ..., parentInfo?: ...} the data format for "dojo.data". Other data formats can be defined later.

comment:9 Changed 10 years ago by bill

Resolution: fixed
Status: newclosed

(In [19652]) Number of fixes (!strict) around Tree DnD:

  • When dragging from a Tree and dropping somewhere, source.getItem() should return a dojo.dnd.Item, not a DomNode? (fixes #7058)
    • Pass source to itemCreator() so that the function can see the type of the dragged nodes. Also fixed documentation of itemCreator(). (fixes #9683)
  • fix dragging a random (non TreeNode?) widget and dropping onto the tree. In this case Tree should not assume the dragged widget has a dojo.data item, parentNode, etc. like TreeNode?'s do. (fixes #9550)
  • better documentation (fixes #9684)
  • make Tree drag source support forInSelectedItems(), like dojo.dnd.Source (fixes #9685)
  • Avoid memory leak on IE by using dojo.attr(node, "id", ...) rather than setting id on the node's JS object (not sure if this matters though since id is a standard attribute name, not an expando property). (refs #9614)

I wasn't sure what fields are best to set in the object returned from getItem(). For now it's just setting type == treeNode and data == the TreeNode? widget.

comment:10 in reply to:  8 Changed 10 years ago by bill

Replying to alle:

Maybe a format like this can be good

{id: foo, type: ["myBestType", "someOtherType", "dojo.data", "text"], data: {myBestType: ..., someOtherType: ..., "dojo.data": {store: ..., item: ..., parentInfo?: ...}, text: "hello world" }

The problem w/this is that if type=="text" (or one of the types is "text") then probably the receiver is expecting {data: "hello world"}, since that's how plain dojo.dnd.Source works.

It might make more sense to have {data: "hello world", dojo_data: .{store: ..., item: ...}, treeNode: ...TreeNode widget... } etc.

comment:11 Changed 10 years ago by alle

Do you want to write

{id: foo, type: ["treeNode", "dojo_data", "text"], data: "hello world", dojo_data: {store: ..., item: ...}, treeNode: ...TreeNode? widget... }

or

{id: foo, type: ["treeNode", "dojo_data", "text"], data: {data: "hello world", dojo_data: {store: ..., item: ...}, treeNode: ...TreeNode? widget... }}

In both cases text break type-key matching.
In first case available types are reduced by existing first level keys (id, type) and more important future introduction of first level keys can be a problem because of types.

If the only problem with my proposal is "text" type behavior in dojo.dnd.Source maybe changing that behavior in dojo.dnd.Source is the cleanest and future-proof way.
AFAIKS dojotoolkit is going toward more data (dojo.data.item) integration in widgets and in overall logic so make choices based on text and domNode DnDing? can limit it's rapid evolution.
My 2 euro cents :-)

comment:12 Changed 10 years ago by bill

The first one. For better or worse, dojo guarantees backwards compatibility until 2.0.

comment:13 Changed 10 years ago by alle

Sorry I forgot backwards compatibility.

What about

{id: foo, type: ["myBestType", "someOtherType", "dojo.data", "text"], data: "hello world", dataByType: {myBestType: ..., someOtherType: ..., "dojo.data": {store: ..., item: ..., parentInfo?: ...}, text: "hello world" }

?
"data" is for full backwards compatibility, "dataByType" for full enhancement support and way to go for 2.0

comment:14 Changed 10 years ago by alle

Maybe a less intrusive and better performance proposal

{id: foo, type: ["myBestType", "someOtherType", "dojo.data", "text"], data: "hello world"}

and a source method

formattedItemData(dndItem, /* string */ chosenType) -> anything

chosenType can be any type present in dndItem.type array
if chosenType=="text" return a string
if chosenType=="dojo.data" return {item: .., store: .., parentInfo?: ..}
later other standard response format may be defined.

dndItem.data is for full backwards compatibility.

This proposal is less intrusive expecially if formattedItemData(..) method is optional and has better performance because requested format is calculated only when and if needed (source can optionally cache it) so no need to calculated and keep updated any supported format for any item.

comment:15 Changed 10 years ago by bill

Yup, something like that seems better. For now I just marked the type as treeNode but supporting multiple types like that makes sense.

Note: See TracTickets for help on using tickets.