ezraroi / ngJsTree

Angular Directive for the famous JS Tree
http://ezraroi.github.io/ngJsTree/
MIT License
270 stars 98 forks source link

controller scope lost in callback events #75

Closed aroder closed 8 years ago

aroder commented 8 years ago

On line 125: s.tree.on(evt, cb); and line 119 it looks like you are mapping events from jsTree to functions on the controller. The "this" context points to the div that is the jsTree, rather than pointing to the controller scope.

One possible answer is to use call or apply to set the context directly. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/apply

I'd like to help with this, but I am not familiar with the testing setup that you have, nor have I done pull requests before. If you can help me get going, I can try working on these issues I logged.

aroder commented 8 years ago

I'm developing in TypeScript. In the compiled javascript, the "this" context for the jsTree callback events is the jsTree dom element. I'm diggin in now to see what I can understand about the compiled javascript that is causing this.

aroder commented 8 years ago

This is the method produced by the TS compiler

    ProjectController.prototype.onTreeReady = function () {
        console.log(this);
        // treeInstance should be populated at this point, but is not. https://github.com/ezraroi/ngJsTree/issues/74
        // the this context points to the DOM element
        // in the demo code from this library, it works
    };
aroder commented 8 years ago

Line 119 uses jQuery's "on" function to map an event from jsTree to its corresponding event on the controller.

s.tree.on(evt, s.$parent.$eval(cb));

From the docs http://api.jquery.com/on/:

When jQuery calls a handler, the this keyword is a reference to the element where the event is being delivered;

This explains the behavior I am seeing. And I understand now how you are mapping events.

What I disagree on is that the callbacks should execute in the context of the controller, not in the context of the tree.

aroder commented 8 years ago

OK, so it is a TypeScript scoping thing. In objects like controllers, TypeScript controls the "this" keyword and causes it to refer to the current object at design time. I see in your example you capture the controller's context in the variable "vm." I can't replicate that in TypeScript, but ultimately that is not the root of the problem.

In Angular, all calls to controller functions execute in the controller object's context. Mapping the jsTree events to callback functions from outside of the controller and under the tree instance's context is outside of what the framework recommends, from what I understand..

I can see the logic in using the tree instance's context in the callback functions. This would be the jQuery way of doing things IMO. Since we are plugging jsTree into Angular, the Angular method should trump.

I see you used $eval. I will learn about that and https://docs.angularjs.org/api/ng/type/$rootScope.Scope#$apply

ezraroi commented 8 years ago

Thanks for sharing.. update if you think that something needs to be changed

SuricateCan commented 8 years ago

I faced the same issue today. The best option would be not to bind directly to the scope function, but to write a proxy function that check if a digest is in progress. If it is not, call apply with another proxy to do a function call on the scope function, if it is in digest, just call the function using the scope as this.

Here is my "fix" for this:

                function manageEvents(s, e, a) {
                    if (a.treeEvents) {
                        var evMap = a.treeEvents.split(';');
                        for (var i = 0; i < evMap.length; i++) {
                            if (evMap[i].length > 0) {
                                var name = evMap[i].split(':')[0];
                                var cb = evMap[i].split(':')[1];
                                events.push(name);
                                if (name.indexOf(".vakata")) {
                                    $(document).on(name, function () {
                                        var args = arguments;
                                        var fn = s.$parent.$eval(cb);
                                        if (!s.$root.$$phase) s.$parent.$apply(function () { fn.apply(s.$parent, args); });
                                        else fn.apply(s.$parent, args);
                                    });
                                } else {
                                    s.tree.on(name, function () {
                                        var args = arguments;
                                        var fn = s.$parent.$eval(cb);
                                        if (!s.$root.$$phase) s.$parent.$apply(function () { fn.apply(s.$parent, args); });
                                        else fn.apply(s.$parent, args);
                                    });
                                }
                            }
                        }
                    }
                    if (angular.isObject(s.treeEventsObj)) {
                        angular.forEach(s.treeEventsObj, function (cb, name) {
                            events.push(name);
                            if (name.indexOf(".vakata")) {
                                $(document).on(name, function () {
                                    var args = arguments;
                                    if (!s.$root.$$phase) s.$parent.$apply(function () { cb.apply(s.$parent, args); });
                                    else cb.apply(s.$parent, args);
                                });
                            } else {
                                s.tree.on(name, function () {
                                    var args = arguments;
                                    if (!s.$root.$$phase) s.$parent.$apply(function () { cb.apply(s.$parent, args); });
                                    else cb.apply(s.$parent, args);
                                });
                            }
                        });
                    }
                }

As you can see, the function is called using the scope as this and any arguments passed from jstree is forwarded to it, and now, the event is inside the digest loop.

Also note that I removed the ".jstree" append and left the name as specified in the attribute, so we can specify other events like "dnd_stop.vakata", which is part of the core plugins. I also had to check if the namespace is ".vakata", cause these events are triggered on the document, and not the element.

This fix may work to handle the events from inside the digest loop (as angular recommend), however, on the controller, this is the controller, and not the scope. But as we cannot access the controller from here, I this is the closest solution.

SuricateCan commented 8 years ago

A good thing would be to have ALL events from jstree to trigger a digest, even if not handled by the scope.

A good example of use for this is if you are watching for selection changes on your scope. Watch functions only triggers when a digest is made, so your view can get outdated.

I wrote a plugin to proxy ALL events triggered by jstree:

(function ($, undefined) {
    "use strict";
    $.jstree.defaults.alltrigger = null;
    $.jstree.plugins.alltrigger = function (options, parent) {
        this.init = function (el, opts) {
            if (options) {
                this.trigger = function (ev, data) {
                    parent.trigger.call(this, ev, data);
                    options(ev.replace('.jstree', '') + '.jstree', data);
                };
                var contextTrigger = $.vakata.context._trigger;
                $.vakata.context._trigger = function (event_name) {
                    contextTrigger(event_name);
                    options("context_" + event_name + ".vakata");
                };
                var dndTrigger = $.vakata.dnd._trigger;
                $.vakata.dnd._trigger = function (event_name, e, data) {
                    dndTrigger(event_name, e, data);
                    options("dnd_" + event_name + ".vakata", e, data);
                };
            }
            parent.init.call(this, el, opts);
        }
    };
})(jQuery);

With this plugin, I changed a small portion of the NgJsTree directive:

                function getOptions() {
                    var jsTreeSettings = attrs.jsTree ? scope.$parent.$eval(attrs.jsTree) : {};
                    config = {};
                    angular.copy(jsTreeSettings, config);
                    var result = JSON.stringify(config);
                    if (config.core) {
                        config.core.data = scope.treeData;
                    }
                    else {
                        config.core = { data: scope.treeData };
                    }
                    //start of my modifications
                    if (config.plugins) {
                        config.plugins.push("alltrigger");
                    } else {
                        config.plugins = ["alltrigger"];
                    }
                    config.alltrigger = function () {
                        if (!scope.$root.$$phase) scope.$apply();
                    };
                    //end of my modifications
                    return result;
                }

With this code, ALL events triggered by jsTree gets a check for digest and a call to apply if not digesting already.

EDIT: I did some local changes to prevent a digest for events already handled by the treeEvents portion.