#507 Promise support callbacks

[StefanCodes]

Adds promise support (via $q) to the beforeDrop callback. The main use case is displaying a confirmation dialog when a node is dropped (with the ability to cancel/revert the drop).
This necessitated rewriting most of the dragEnd event handler to be $q friendly.

The tree node now supports 4 return types from the beforeDrop callback:

• Truthy: cancel the drop
• Rejected promise: cancel the drop
• Falsy: allow the drop
• Resolved promise: allow the drop

The use of a truthy value to cancel/revert the drop doesn't seem totally intuitive to me, but it means that this won't break anybody's existing code. Since anybody's implementation on beforeDrop may not return anything (thus falsy), I've inverted the logic for backward compatibility.
The English justification is that the callback returns a 'cancel' boolean - true means the drop is not ok, which is opposite to what accept() returns. Think that's ok? I feel like I'm caught in a backward compatibility trap.

This PR can replace #556 as a more generic solution to the same problem.

Other changes:

• Refactored the ui-tree-node $$apply scope variable to $$allowNodeDrop, which is all it did despite the scary name
• Wrote documentation for each of the callbacks

[bronielsen]

Hi Stefan. Not sure I understand why you chose to overload the beforeDrop function with all this functionality. As you rightly note, the return of false to indicate that the drop is ok - and vice versa - is not going to be easy for users to understand. This is why I chose to implement #556 with a simple change to beforeDrop and a separate callback for the confirmation dialog. If beforeDrop returns true the confirmDrop callback is called inside uiTree returning a promise to indicate whether the drop should go through or not. If beforeDrop returns false (or nothing, which is backwards compatible), then the confirmDrop callback is not called. In my mind this is a more elegant solution for the API.
With respect, Morten

[StefanCodes]

Hi Morten,

Thanks for taking a look. My main purpose was just to make beforeDrop wait for a promise chain. The rest of the code changes in beforeDrop are refactorings to clean up code usage that became obvious when the promise was added.

As you rightly note, the return of false to indicate that the drop is ok - and vice versa - is not going to be easy for users to understand.

And ya know... I think I've talked myself out of that now. My rationale for implementing it this way was just not to break anybody's apps that are in the wild right now (the same way yours does). But now I'm thinking that the beforeDrop callback is kind of useless if it doesn't wait for a promise. Does it have any practical use cases? I searched the issues and found it was mostly people attempting ugly workarounds to emulate a blocking beforeDrop.

That's why I was reluctant to add a new callback as you did - I kept ending up at "then what's the point of beforeDrop?" in my mental exercise. The two callbacks feel like they ought to be the same to me.

I keep ending up thinking that beforeDrop should function the same as accept - return true if we're good to go, or false if we want to reject the condition. That would be a change in this PR which would cause a breaking change in the wild. On the other hand, given how limited the usefulness of beforeDrop is, would that even affect any real implementations?

@Voles, do you have any opinion? Both PRs (#556 and #589) solve the same problem - the debate is really about what the tree API should look like. I don't have a particularly strong opinion either way. If you want to use @bronielsen 's I will submit a new PR with my other changes (docuementation, demo page, etc).

I'm going to be away for the rest of September. Talk to you all next month!

[bronielsen]

I agree. This is only a question about the API. And respect to you Stefan for the time you put into getting the full package (doc, demo etc) together.

I see the purpose of the current implementation of the beforeDrop to be to prepare the data structure for the actual drop (where ui-tree moves data around). I cannot think of a practical application of this scenario of the top of my head, but it is nice to have the possibility to do it. It would be natural that beforeDrop returned true if the drop could go through, but that is not the case.

So maybe the solution is just to leave beforeDrop as is, and make a new callback called confirmDrop. Not my implementation, but an inverted version of Stefan's implementation of beforeDrop, where the return values are the intuitively correct values (true / resolved promise for go-ahead, etc).

beforeDrop should be called only after confirmDrop returns true / resolved promise.

How about that? Then we can reuse all of Stefan work.

--Morten

[Voles]

@StefanCodes and @bronielsen thanks both for your work! I've looked into both PR's but I'm not yet sure about the 'best' solution.

Some thoughts:

• I don't think it's a good idea to make the API confusing in order to make the changes backwards compatible. Returning true to cancel the drop doesn't feel logical. @StefanCodes you have added clear examples and documentation already and I think we should refer to those in the release nodes and go for promises all the way.
• beforeDrop returns a promise with this change, but internally it is still part of the $callbacks property. I'm thinking about how we can separate both. Or we can try to make a promise of every current callback.

Eg. in dummy code

dragStart: function (e) {
    return $callbacks.beforeDragStart()
        .then(function () { ... }) // dragStart implementation here
        .catch(function () { ... }); // drag prevented - cancel drag
}

dragEnd: function (e) {
    return $callbacks.beforeDrop()
        .then(function () { ... }) // drop implementation here
        .catch(function () { ... }); // drop prevented - cancel drop
}

Promises make the component much more convenient, so I think we're on the right track! We can also release step by step, replacing all callbacks with promises.

I need a little more time to think this through. Any feedback on my thoughts is welcome!

[tommyminds]

I've been using angular-ui-tree quite extensively and I would love to see you make the move to the fully promise-based callback API you suggest in your comment Voles. It will make things a lot cleaner, and a lot more flexible.

Also, as I have seen mentioned before, for me it is very important to be able to easily change the data of an object to another structure when dragging and dropping between two trees, so would be nice if this would also become easier to achieve by these changes. (For example letting the object that is dropped in the target tree be whatever you resolve the beforeDrop callback promise with).

Finally, currently a tree where you drag a node from gets the dropped callback. Although in certain cases this is desired, in almost every scenario I used multiple tree's I would have rather had the dropped callback called on the tree where you drag the node to.

[Voles]

@StefanCodes do you have time to bring the PR in sync with master? I'll plan to work on this during the weekend. Thanks!

[StefanCodes]

I'm on vacation without access to a computer for another week. If you want to wait until next weekend I should be able to do the merge no problem.

Cheers,
Stefan

-----Original Message-----
From: "Niels Dequeker" notifications@github.com
Sent: ‎2015-‎09-‎24 22:23
To: "angular-ui-tree/angular-ui-tree" angular-ui-tree@noreply.github.com
Cc: "Stefan Mohr" stefan@stefanmohr.com
Subject: Re: [angular-ui-tree] #507 Promise support callbacks (#589)

@StefanCodes do you have time to bring the PR in sync with master? I'll plan to work on this during the weekend. Thanks!
—
Reply to this email directly or view it on GitHub.

[StefanCodes]

@Voles Ok, it's up to date again!

I agree with pretty well everything that's been discussed above. Full promise support for all callbacks would be a great thing to have.
I'm also unhappy with my original decision to make the API confusing in order to preserve backward compatibility. That's still present in this PR and should change before this goes into a release. Does anyone have a suggestion?

[aphujanen]

Promise support would be awesome thing. When we could expect this to be on master?

[StefanCodes]

Since the discussion on this has died down, I went ahead and changed the API like most people thought was better - now falsy/rejected will reject/cancel the drop, and truthy/resolved will accept it.

@Voles do you think this is good enough to merge into master? We still have to discuss a more general way to approaching promises for the remainder of the callbacks, but I think this particular use case accounts for the majority of the questions/requests.