-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dialogs - Allow multiple selection on Open... #12923
Conversation
b2ce1c0
to
7e8f500
Compare
if (destination.isDirectory) { | ||
this.workspaceService.open(destinationUri); | ||
if (destinationUri) { | ||
if (Array.isArray(destinationUri)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be simpler by doing
if (!Array.isArray(destinationUri)) {
destinationUri= [ destinationUri]
}
and omitting the else
branch and the added method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a general note, I think APIs that return T | T[]
don't make sense in many cases: if you care about the "single element" case, you can still check for length === 1
and if you don't care, you don't have to handle the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hint to simplify algorithm! Indeed, that's better to handle all cases at once.
I made explicit in the signature of the doOpen... methods that an array of elements will be returned, getting rid of the MaybeArray. I kept however the signature of the showOpenDialog service with this MaybeArray, to avoid compatibility issues.
packages/workspace/src/browser/workspace-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
a116c9e
to
8f60558
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, now
fixes eclipse-theia#12905 - allow multiple selection in doOpen dialog - handle an array or a single URI or undefined when closing dialog Contributed on behalf of ST Microelectronics Signed-off-by: Remi Schnekenburger <[email protected]> Address review comments - remove checks for files against workspace URI - update variable name to a more representative one - Simplify algorithm for single selected URI case - update signatures to indicate an array of files or folders can be opened rather than a single element
8788785
to
6ee97b3
Compare
What it does
fixes #12905 - Allow multiple selection on Open...
Contributed on behalf of ST Microelectronics
How to test
When running Theia in browser, select the Open... dialog:
Electron has 2 different menus to select either Folder or Files:
Open file...
selection dialog to also be able to select multiple items. If the selection has several files and a folder selected in the dialog, the open button of the dialog won't react. It is supposed to browse the folder if there is only one folder selected.open folder
command already supports multiple folder selection, creating a new workspace containing all folders. No updates were done to this command.Follow-ups
No Follow-up was identified during this task development.
Review checklist
Reminder for reviewers