Drag / Drop functionality for Tree View

[Brainsaw]

Hello community,

as suggested by CuteAlien in my thread about my editor (Y.A.I.S.E.) I tried to implement drag / drop function for the Tree View. It works as far as I can tell, but I have the habit (or weakness) to rarely see any side-effects of what I implement (would save me a lot of work ). Here is what I have changed:

New GUI events:

• new GUI event "EGET_TREEVIEW_NODE_DRAG" which is fired when a node is dragged (i.e. left button is down and mouse is no longer on the node dragged)
• new GUI event "EGET_TREEVIEW_NODE_WILL_DROP": this is fired right before the actual movement in the tree happens. Listening to this event and calling the new method "cancelNodeDrag" will cancel dragging. This way dropping a node can be prevented
• new GUI event "EGET_TREEVIEW_NODE_DROP": an event triggered when the actual drop happens
• last new GUI event: "EGET_TREEVIEW_DRAG_CANCELED" is fired when dragging is canceled, i.e. when the programmer cancels the drag with the method mentioned before or when the node is dropped on a node which isn't possible, i.e. one of it's own children

In the "IGUITreeView" interface I have added three new virtual methods:

• draggingAllowed(bool): by default drag / drop is not active, it needs to be activated calling this method
• cancelNodeDrag(void): tell the tree that you don't want the dragging which is in progress to continue
• getNodeAt(position2di): get the tree node underneath the mouse. This method doesn't need to be exposed, but I think it might be handy for many people

In the implementation of the tree view I have done these changes:

• The "CGUITreeViewNode" got a new method "addChildNode" which takes a node given as parameter and adds it as a child of the current node
• Added new members "Dragged" (the node which is dragged), "DragCandidate" (the node clicked on which might be dragged later on) and "AllowDragDrop", a flag to (de)activate the functionality. For the next points I assume that the flag is "true"
• if a left mouse click is detected the node under the mouse pointer becomes the "DragCandidate"
• if the mouse is moved and hovers another node of the tree the "DragCandidate" becomes the "Dragged" member, "DragCandidate" is reset to "0". The event "EGET_TREEVIEW_NODE_DRAG" is fired and the "LastEventNode" member is set to the dragged node
• if the left mouse button is left up and "Dragged" is not "0" the following happens:
  • the node underneath the mouse cursor becomes the "dropCandidate"
  • if the "dropCandidate" equals the dragged node nothing happens, dragging is canceled silently
  • if the "dropCandidate" is a child of "Dragged" dragging is canceled and a "EGET_TREEVIEW_DRAG_CANCELED" event is fired. "Dragged" is reset to "0". The dragged node is "LastEventNode". This does also happen if the node is not dropped on another node.
  • if "Dragged" is still set (not "0") a "EGET_TREEVIEW_NODE_WILL_DROP" is triggered. The "LastEventNode" member is set to the drop target. Now the developer of the app can cancel the dragging if necessary
  • if "Dragged" is still set (not "0") the dragged node is moved and becomes a child of the drop target. A "EGET_TREEVIEW_NODE_DROP" event is fired with the drop node as "LastEventNode"

I have uploaded the diffs to https://dustbin-online.de/download/TreeViewDragDrop.zip.

Now it's up to you: what did I not think of, what did I break, what doesn't work? I think I'll give my changes a try next weekend and include it into "Y.A.I.S.E.".

[CuteAlien]

Cool. Will test soon. On first view interface sounds pretty good to me.

Not sure if EGET_TREEVIEW_DRAG_CANCELED should be called when users calls cancelNodeDrag. I think events are usually not send when users do the action - otherwise users might want to add code in the handler to exclude the case where the cancel was caused by the coder, but that info isn't available. While it's always easy to run the same code which you run in the event-handler in another place.

Some bikeshedding comments:
- Irrlicht uses Allman indention/bracket style, not K&R style. Please try to make it look like the other Irrlicht code.
- Mostly (except in the few cases which messed it up) all setter functions start with set so it should be setDraggingAllowed (or setAllowDragging or setCanDrag).

More comments likely after testing ;-)

[Brainsaw]

I was also not sure on whether or not to send the Cancel event when the user cancels - this only informs him of what he's doing.

I was trying to follow the code style I saw, but it's completely different from mine . I guess I can get used to it.

Looking forward to some more comments.

[CuteAlien]

Which tool did you use to create the patch? Seems to not be an svn patch, so I'm not sure right now what tool to use to apply it automatically (could do manually, but I'd prefer not to...). Or if you have full files in some open repository I could just copy them over (have no other changes to those files right now anyway).

[Brainsaw]

As I don't have SVN installed on my system at the moment I have downloaded a dump on Monday and used WinMerge to compare. I have however uploaded the complete files to http://forumdata.dustbin-online.de/TreeViewDragDrop.zip, this package also includes the little app I have written do develop the update (test_main.cpp).

Edit: and sorry for the code formatting. I need to find out if Visual Studio allows per-project code formatting settings

[CuteAlien]

Ah, always nice to have a test! Still have to test a bit more and look at code, but at least test works without obvious problems.
What's missing mainly right now is a bit feeback. Partly tricky because we don't switch cursors inside Irrlicht, I guess that part can be done by handling events (and maybe even better as it may be easier then for users to control cursors).
Maybe we could attach the node to the cursor? Probably a bit tricky (and risks of it getting stuck etc).
Or maybe avoid selection while drag&drop and instead draw some "target" line.
And one change that could mabye be done without too much trouble and could help a bit: Select the dropped element after placement and open the node above it (I think there should be enough functions for that by now, I remember I needed something similar when I used the treenode).

I'll find some more time soon.

[Brainsaw]

Thanks for the feedback, I'll try to look into it again in the next couple of days. At the moment my day-time job has me pretty exhausted after work, so I don't think I'll do anything before the weekend.

I also though about some better feedback, e.g. having the text of the dragged node attached to the cursor.

[Brainsaw]

I have uploaded a new version to the link in the original post: If a node is now dragged it is displayed at the cursor. I used two colors of the skin, EGDC_WINDOW as background and EGDC_BUTTON_TEXT as text color. I thought it may look weird that way but I think it does rather look natural.

[CuteAlien]

Nice. I'll check it out.

[Brainsaw]

Are there any news on this modification?

[CuteAlien]

Nope. Just hanging around my todo. Sorry, not enough energy left currently.

[Brainsaw]

No problem, as long as it's on your list

[CuteAlien]

I had completely forgotten how annoying it is to work without git on something together with others, no quick cheap branches :-(
So my modified version also as zip *sigh*: http://www.michaelzeilfelder.de/bin/Mic ... agDrop.zip

Changes in interface:
- Some comments added
- New function getDraggedNode
- setDraggingAllowed parameter name change

Changes in code:
- setParent removed (bit risky as it doesn't handle children unlike similar setParent function for example in ISceneNode, so I prefer avoiding it for now)
- renamed addChildNode to addChild (similar name to other places, none of the other functions uses 'node' and ISceneNode, IGUIElement also have addChild which kinda does the same)
- addChild no longer virtual (as it's restricted to CGUITreeViewNode it's not going to end up in interface anyway, if we want to make it virtual some day we can still do that)
- Some fixes so it compiles with older compilers (can't use variable construction with {} like SEvent event{};)
- moved stuff around a bit so patch is more "together" (makes it easier on diff)

Changes in example:
- Get it working with older compiler

Also - I reverted _all_ the whitespace changes and used Irrlicht style. That cost me 1-2 hours, so please do not change unrelated whitespace in future patches ;-) Reason is that I need to see in a patch which lines have changed and that becomes really, really hard if there's whitespace changes all over unrelated lines. This even messes up diff commands. If you want to propose whitespace changes in Irrlicht, always make it a separate patch (thought most probably wouldn't make it).

So... why I didn't apply it yet:
Right now you can't move nodes in the tree back to root once you moved them down into some node.
Not sure how you handle this in your editor. Do you make all "top" nodes unmovable?
There's several solutions to that, so not sure yet how to continue:
a) Make drag&drop behave like many others tools - so dropping can also happen on same level nodes and the nodes then get squeezed in between (instead of always creating sub-folders). I guess then drop needs to have another state for - dropping same level or creating new level. Or some index. Maybe check how other API's do that.
b) We just keep it as it is - but setDraggingAllowed becomes an enum or bitset with options instead of current bool. So we can extend it later and add that functionality. Easiest solution if you don't want to spend more time on it for now as it solves your problem (which is fine by me as well, thought obviously it's less nice than "a").
c) Combination of a and b - make it flags and still add the feature to drop in between other children
d) ? (in case you have other ideas)

Sadly hard to do this just later as it will force me then to break the interface in the future (which is always ugly).

Aside from that problem it workes really nicely in my tests.

edit: We probably should also test what happens if a user has 2 trees and drags an element from one to the other. I suppose we should forbid this (I think nodes have some owner so we can check that)

[Brainsaw]

Thanks for the feedback. I'll see when I find time to have another look into it and will consider all of the proposals.

I think dropping nodes in between other nodes to adjust the order is not easy to accomplish, but I'll have a look. I think I'll modify it so that an enum of flags is used for the setDraggingAllowed method as I have learnt the hard way that changing an interface later on sucks .

I also agree that moving nodes between trees should not be allowed (at least for the first iteration).

[CuteAlien]

I've been giving it some more thought (no code, just showerthoughts) and some ideas:

If we use insertChild instead of addChild with an index then adding in between is becoming trivial. Default index for the insert position can be (u32)-1 then it's always added at the end (indices larger then end just added at end). Because then you can pass the parent of the found CGUITreeViewNode + some index.

Thought it needs likely some more changes, first of all some feedback as to where it's inserted (drawing some line). So we need to know when to insert in between and then to add to a child. getNodeAt could return an additional parameter with the mouse pointers relative position in the element (edit: realizing now, that's not necessary as the returned node already knows it rectangle and the caller knows the mouse-position). Then some check for pixels (maybe using some new constant in IGUISkin?) to see if it's at upper/lower border of the CGUITreeViewNode and if so use the parent + index (might need new function to find that index).

Another change it might need is in the event handling as would need immediate feedback if a node is insertable. Maybe instead of EGET_TREEVIEW_NODE_WILL_DROP a general EGET_TREEVIEW_DRAG_CHECK and new function in IGUITreeView instead of getLastEventNode which returns a struct with all info involved you might need. Thought then we get lots of checks I guess. So could also just allow it always? Would need some tests to see what works well there.

Also independent of those changes, I wonder if getLastEventNode is maybe a bit abused right now (not intuitive it's the drop target). Should we use a new function for that? I already added one for the dragged node, so one more for drop-target wouldn't be a problem. Or maybe even replace the one I added by a function returning a struct with all info involved (drag target, drop target, maybe even event action, can then also have the insert index if useful for anything). getDragEventInfo or something like that.