Skip to content
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

[electron] Add ability to create folder in OpenDialog on MacOS #7208

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

thegecko
Copy link
Member

@thegecko thegecko commented Feb 24, 2020

Signed-off-by: Rob Moran rob.moran@arm.com

What it does

With reference to the electron docs for the version we use (4.2.12), the file dialog on MacOS doesn't show the 'New Folder' button unless properties.createDirectory has been set.

This is restrictive behaviour when a user needs to use the file dialog to select a folder (properties.openDirectory set to true).

Short of implementing this feature for all other platforms (Windows, Linux, Browser), an easier approach is to set the createDirectory property whenever openDirectory is set on MacOS only.

How to test

Ensure the New Folder button appears when using the openDialog for folders in Electron for MacOS (e.g. open a new workspace).

Screen Shot 2020-02-24 at 22 39 25

Review checklist

Reminder for reviewers

@thegecko thegecko added filesystem issues related to the filesystem electron issues related to the electron target labels Feb 24, 2020
@thegecko thegecko changed the title [electron] Add ability to create folder in OpenDialog onMacOS [electron] Add ability to create folder in OpenDialog on MacOS Feb 24, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified the changes and they work well!
In my macOS version, I was getting the new folder icon on master, but with this change I now also get it at the bottom of the dialog:

Master:

Screen Shot 2020-02-24 at 7 13 54 PM

Pull-Request:

Screen Shot 2020-02-24 at 7 17 23 PM

@vince-fugnitto vince-fugnitto added OS/Mac issues related to Mac OS CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) labels Feb 25, 2020
@thegecko
Copy link
Member Author

thegecko commented Feb 25, 2020

In my macOS version, I was getting the new folder icon on master

Older versions of MacOS don't have this icon (see my screenshot), so the new button was badly needed :)

we will require a CQ for the copied code

I disagree, this type is already brought in with the Electron dependency and partially copied here, here and here.
The types are removed at compile time and we already have a CQ for Electron anyway :)

Perhaps I should reword the comment so it doesn't say "copied"?

@marcdumais-work what do you think?

@marcdumais-work
Copy link
Contributor

Perhaps I should reword the comment so it doesn't say "copied"?

+1 ; this case IMHO we're not literally copying code - we're just using an Electron API as per their documentation.

Signed-off-by: Rob Moran <rob.moran@arm.com>
@thegecko
Copy link
Member Author

Comment updated :)

@vince-fugnitto vince-fugnitto removed the CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) label Feb 25, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified the changes on macOS and it worked great :)
I also verified on Linux (Ubuntu) and could not find any regressions.

@thegecko thegecko merged commit 68e8e24 into eclipse-theia:master Feb 25, 2020
@thegecko
Copy link
Member Author

Thanks @vince-fugnitto

@thegecko thegecko deleted the electron-dialog-defaults branch February 25, 2020 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron issues related to the electron target filesystem issues related to the filesystem OS/Mac issues related to Mac OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants