There might be a better way to do this, right now it might go many times through the same promises. Since the promises are pushed to the array, you could save the length of the array in a separate variable and initialize it the first time _whenRenderedResolve is called. Decrease the value by one when splice is called and just check if the length of the array is different to the saved length before calling _whenRenderedResolve here. If it is, you know that new elements where added to the array, you would then saved the new length and call _whenRenderedResolve where you could pass the last index, and do a normal for to just call the last new elements, or just do a forEach like now.

Finally got back to looking at this. I spent some time trying to follow your suggestion, but it ended up complicating the logic quite a bit. Stepping back, it seems to me like it's not really necessary to optimize this, because (1) the number of render promises is only going to be as large as the number of open folders in the tree, which is likely not going to be very big, and (2) when it goes through the loop, it fails quickly for each promise that already has a listener attached. So it doesn't really seem worth it.

@gruehle - feel free to add your input if you disagree.

That idea might have been a bit complex, but I still think we could simplify this code. If we just want to create a promise and resolve it once all the other promises are done, maybe we could simplify it using a counter. We initialize the counter in 0 on line 433. On line 733 if the counter is greater than 0, we add 1 to it and call done over the promise, with a simple function that reduces the counter by 1 and an if that if the counter is equal to 0, then it would resolve the promise from the reopen call.

It could then be part of the Async utils as an object that gets initialized giving it a deferred object and has a method to add new promises, and more stuff if required.

Note: Sorry I couldn't review, I've haven't had much time this past 2 weeks.

That makes sense. I was sort of hung up on the fact that the counter would have to be kept outside the function and was thinking that might screw up somehow if the function were called multiple times. But the render promise array is already global, so if there's some reentrancy problem, we already have it.

I'll take a look at simplifying this weekend.

Maybe i'm not following what's going on entirely, but if my understanding is correct, this function is used to resolve a deferred after a bunch of other deferred's have completed. (yes?)

jQuery has a built in function for this $.when, which accepts an unlimited number of parameters, or can be called $.when.apply($, array), so this function could be simplified to:

function _whenRenderedResolve(deferred) {
  if (_renderPromises) {
    $.when.apply($, _renderPromises).done(function() {
        deferred.resolve();
    });
  }
}

@njx, I figured it couldn't just be that easy :/ Could you recursively call _whenRenderedResolve? For example:

function _whenRenderedResolve(deferred) {
    if (_renderPromises) {
        var len = _renderPromises;
        $.when.apply($, _renderPromises).done(function() {
            if (len === _renderPromises.length) {
                deferred.resolve();
            } else {
                _whenRenderedResolve(deferred);
            }
        });
    }
}

I'm not worried about efficiency here. As NJ pointed out, there will not be many pending promises. For code cleanup and re-use, I agree with @peterflynn that we should consider making a utility class to handle this. It would be easy to encapsulate this into a small class, and that would be a good time to optimize.

Actually @TomMalbran's idea makes sense--we can just count the number of promises that need to be resolved. I'll see if I can make that change easily.

@TuckerWhitehouse - I'm not sure if your second example would work because (depending on how $.when is implemented) it might end up listening to the same promises multiple times.

@TomMalbran - turns out that doesn't quite work either. I think the reason is that if we add a done() handler for the render promise immediately after creating it, there's a danger that it will resolve before all the nodes we need to open are actually open. In the current version, that doesn't happen because _whenRenderedResolve() doesn't get called until after we've at least kicked off the reopening of all the previously open nodes in the reopen.jstree handler in _renderTree(). So we do need to store the render promises until then (without attaching their done handlers), and at that point we basically have to deal with something like the current _whenRenderedResolve(), although (as you mentioned before) there might be some clever way to avoid running through the array multiple times.

So, after all that :), I'm again inclined to merge this as is, given that I've tested it a fair amount and I know it works, and there's a fair amount of potential fragility here.

I thought that that might happen. Anyway I don't see any problem of having the the promises be resolved before the call to _whenRenderedResolve(). It could easily happen that when we get to _whenRenderedResolve() all the promises have been resolved and we are done. The only change would be to replace the proposed if asking for the counter to be greater than 0 before when adding a done callback to the promises for a flag that while true lets promises get a done callback will be set to false after _whenRenderedResolve() is called and all the promises are resolved (counter == 0). With this we can keep adding the done callbacks until we are ready to call _whenRenderedResolve().

Anyway, if this works this can be merged like this and optimized later, or make it an Utils function/class.

Shouldn't this be always() instead of done()? If one of the render promises fails, the deferred wouldn't get resolved.

The deferreds behind those promises are never rejected (see _treeDataProvider()).

However, that does bring up an issue...if for some reason one of the promises doesn't complete, then projectOpen will never get sent out. From what I can tell, the only way that would happen would be if the call to readEntries() fails and no entries were actually returned. So perhaps I should reject the render promise in that case, and then turn the above into an always() as you suggest.