Adds a refresh menu item

[njx]

This is a continuation of @VictorGama's pull request #2786 to add manual refresh to the project tree (addresses #3124).

Changes since that pull:

• null out _lastSelected when refreshing (avoids an exception)
• restore selection after refreshing
• more accurately determine when tree is done refreshing (this is the tricky bit)
• move the "Refresh" context menu item to the bottom below a divider, since it's not specific to the currently selected file/folder as the other items are

Not urgent, but would be nice to get this functionality in so extensions can request the tree to manually refresh as well (e.g. #3252).

Updated: removed it from the project dropdown since we're no longer putting functionality there

[TomMalbran]

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.

[njx]

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.

[TomMalbran]

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.

[njx]

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.

[TuckerWhitehouse]

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();
    });
  }
}

[TuckerWhitehouse]

@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);
            }
        });
    }
}

[gruehle]

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.

[njx]

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.

[njx]

@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.

[TomMalbran]

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.

[peterflynn]

I wonder if this sort of "Promise queue" construct is something we should add to the Async utils module... would that help to simplify the code in ProjectManager?

[njx]

Rebased onto most recent master. Haven't addressed code review comments yet.

[njx]

Reviewed. Open for committers to review, though I should address @TomMalbran's feedback first.

[njx]

@gruehle - see my comment above. Ready for review.

[gruehle]

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

[njx]

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

[njx]

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.

[njx]

Ready for re-review. One last thought...do you think we should be paranoid and have a timeout in case the render promises don't all complete within some amount of time? I'm sufficiently worried about the complexity of the codepaths involved (and the possibility of bugs in jstree), and things are likely to break if projectOpen never gets sent out.

[gruehle]

Yeah, it would be a good idea to have a timeout, just in case. The Async module has a withTimout() method for easily adding a timeout to any promise.

[njx]

Added the timeout (withTimeout() is super handy!). However, now I'm noticing that even in master (without my changes), there's a problem with restoring open folders--if I open a bunch of different folders, then quit and restart Brackets, it seems like only the first folder is reopened properly--filed as #3905. Do you think we should track that down before merging this?

[njx]

Actually...it looks like that functionality has never worked properly for more than one open top-level folder :( So I guess we can go ahead and merge this :)

[gruehle]

Looks good! Merging.