Support opening multiple files at a time #823
Conversation
|
Is this for mac and windows? |
|
This uses the JavaScript interface for choosing files, so it should work on mac and windows when the brackets-app supports it. The pull request #121 in the brackets-app project fixes the multiple selection in the Windows app, but the JavaScript API for selecting multiple files has always existed in both the Windows and Mac version. I can't test this pull request on a mac, but I think it should work. If you have any problems, let me know! |
|
@redmunds will review since he's reviewing the brackets-app piece. |
| result.reject(); | ||
| } | ||
|
|
||
| for (i = 0; i < files.length; i++) { |
There was a problem hiding this comment.
We should use Async.doSequentially() here. This code basically reimplements the Async.doInParallel() pattern, but since each individual doOpen() could show an error dialog we shouldn't run them all in parallel. This change should also make the code easier to read.
See saveAll() for an example of similar doSequentially() usage.
There was a problem hiding this comment.
This loop has 2 other problems:
- doOpen() is only meant to open a single file because it automatically makes the file the current file, so this creates a race condition to see which of the multiple files is selected. Which is also unnecessary processing. Async.doSequentially fixes the race condition, but we still don't want to select every file in the list one at a time.
- For files inside the current project, they are not added to the working set, which defeats the purpose of opening multiple files. It looks like you only opened 1 file.
So, another code path is needed that opens files and adds them to working set, and then selects 1 file (first or last?). We don't want to update the existing code path because single-clicking a file in project tree should not add it top the working set.
There was a problem hiding this comment.
@peterflynn I can't figure out how to return the results when using Async.doSequentially. It seems that it just discards the results of the individual Deferreds. Is there any way to resolve the Deferred using the list of opened documents with Async.doSequentially? It does simplify the code nicely though.
@redmunds (regarding point 2) So are you saying that files that are opened from the open file dialog should always be added to the working set even if they are in the project? Is this the right function to do the opening multiple files in or what existing code path are you meaning?
There was a problem hiding this comment.
@conradz The "another code path" I referred to needs to be written. It would be awesome if you want to take a stab at this.
I took a look at the code, and it doesn't seem too bad. I think we should add a selectFile parameter to doOpen(). All other calls should set it to true, and set it to false here. onOpen would need to be updated accordingly.
After each file is opened, then add each to the working set. Finally, select 1 of the files (not sure if it should be the first or the last in the list, so take your pick).
The code that adds file to working set if outside project in the select code should be moved, but I'm not sure where, so it's OK for now.
There was a problem hiding this comment.
@conradz (on behalf of @peterflynn) Did you read the comment in utils/Async.js line 130? doOpen will show error dialogs for each file, so I think you just need the results from the master promise. I think you want to use failAndStopFast = false so that each file is attempted to be opened.
There was a problem hiding this comment.
Re doSequentially(): you can still attach listeners to each individual Deferred before returning it from beginProcessItem()... and then either push each doc onto a list as they get resolved, or add them at the right index if you want to keep the order the same. See the code around checkOneFile() DocumentManager for an example of the latter.
|
Conradz, this is functionality that we definitely wanted to add, so thanks for taking this on. Unfortunately, it surfaces a fundamental problem with File > Open not adding files (which are inside project) to the working folder. Please let us know if you need any help working out that part of the code. |
|
Thanks for the feedback, I added some more replies/questions to the code review. |
ghost
assigned 
| @@ -138,7 +138,7 @@ define(function (require, exports, module) { | |||
| * @return {!$.Promise} | |||
| */ | |||
| function addToWorkingSetAndSelect(fullPath) { | |||
| CommandManager.execute(Commands.FILE_ADD_TO_WORKING_SET, {fullPath: fullPath}); | |||
| CommandManager.execute(Commands.FILE_ADD_TO_WORKING_SET, { files: [fullPath] }); | |||
There was a problem hiding this comment.
Passing multiple files to this method doesn't seem right. While multiple files can be added to the working set, only a single file can be selected.
|
Re all these issues around the working set... here's an alternative way we could do this that might be simpler for now:
Opportunistically, we could still change doOpen() so it always adds to the working set. We feel like this is the right behavior anyway, but this change wouldn't be necessary for multi-file support if we implement it the way I suggested above. |
|
Thanks, @peterflynn. I think that's a better approach. |
|
Closing in favor of my new attempt, pull request #855. It uses the new approach @peterflynn suggested. |
…Url code hints (adobe#823) * Using thirdparty-pathutils to remove deprecation warning * Moved the PathUtils variable beside other module require statements
I changed the code the
FILE_OPENcommand so that the user can select multiple files to open at once, instead of having to open the files one at a time with a dialog prompting for each one. This had to change other parts of the code that used theFILE_OPENcommand. I think I got all of the locations, but please review it 😃.This probably needs pull request #121 on the brackets-app project that I created for supporting multiple file selection on the Windows app.