Skip to content
This repository has been archived by the owner. It is now read-only.

Adds a refresh menu item #3370

Merged
merged 6 commits into from May 19, 2013
Merged

Adds a refresh menu item #3370

merged 6 commits into from May 19, 2013

Conversation

@njx
Copy link
Member

@njx njx commented Apr 7, 2013

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

@njx njx mentioned this pull request Apr 7, 2013
_renderPromises = null;
} else {
// Add listeners to any new promises that have shown up.
_whenRenderedResolve(deferred);
Copy link
Contributor

@TomMalbran TomMalbran Apr 7, 2013

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.

Copy link
Member Author

@njx njx May 18, 2013

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.

Copy link
Contributor

@TomMalbran TomMalbran May 18, 2013

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.

Copy link
Member Author

@njx njx May 18, 2013

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.

Copy link
Contributor

@TuckerWhitehouse TuckerWhitehouse May 18, 2013

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

Copy link
Contributor

@TuckerWhitehouse TuckerWhitehouse May 18, 2013

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

Copy link
Member

@gruehle gruehle May 18, 2013

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.

Copy link
Member Author

@njx njx May 19, 2013

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.

Copy link
Member Author

@njx njx May 19, 2013

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

Copy link
Contributor

@TomMalbran TomMalbran May 19, 2013

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
Copy link
Member

@peterflynn peterflynn commented Apr 19, 2013

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?

Victor Gama and others added 2 commits Apr 24, 2013
* Fix selection drawing after refresh.
* Re-select selected node after refresh.
* Move Refresh command below divider in context menu since it's not file-specific.
@njx
Copy link
Member Author

@njx njx commented Apr 24, 2013

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

@njx
Copy link
Member Author

@njx njx commented May 10, 2013

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

@ghost ghost assigned gruehle May 13, 2013
@njx
Copy link
Member Author

@njx njx commented May 18, 2013

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

// Only listen to each promise once.
if (!promise._project_listening) {
promise._project_listening = true;
promise.done(function () {
Copy link
Member

@gruehle gruehle May 18, 2013

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

Copy link
Member Author

@njx njx May 19, 2013

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

Copy link
Member Author

@njx njx May 19, 2013

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
Copy link
Member Author

@njx njx commented May 19, 2013

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
Copy link
Member

@gruehle gruehle commented May 19, 2013

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
Copy link
Member Author

@njx njx commented May 19, 2013

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
Copy link
Member Author

@njx njx commented May 19, 2013

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
Copy link
Member

@gruehle gruehle commented May 19, 2013

Looks good! Merging.

gruehle added a commit that referenced this issue May 19, 2013
@gruehle gruehle merged commit d1cc272 into master May 19, 2013
1 check passed
@gruehle gruehle deleted the nj/manual-refresh branch May 19, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants