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

Enables opening read-only files in electron. #9950

Merged
merged 5 commits into from
Aug 26, 2021

Conversation

kejsitake
Copy link
Contributor

@kejsitake kejsitake commented Aug 22, 2021

Signed-off-by: Kejsi Take [email protected]

Fixes: #9860
Makes sure that a user can open a new file in the ElectronFileDialog when it has Read permissions (and not Write).

What it does

  • Adds a new canRead() method to check if the user has read permissions to the file.
  • Calls the method from .showOpenDialog()

How to test

After making the above-mentioned changes, I ran the Electron example and verified that I could open a file that had only read permissions.

Review checklist

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

The code looks good and I've tested that I can open a read-only file on Windows. There is a lot of weird behaviour around reading/writing files into read-only directories on Windows, but the PR fixes the particular problem the issue is about.

@tsmaeder
Copy link
Contributor

@colin-grant-work could you test on Linux?

Comment on lines 94 to 99
for (const uri of Array.isArray(uris) ? uris : [uris]) {
if (!(await this.fileService.access(uri, FileAccess.Constants.R_OK))) {
this.messageService.error(`Cannot read resource at ${uri.path}.`);
return false;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I could imagine that this could impact performance for large arrays of URIs because every access call is sequential. We can parallelize this fairly well like this:

Suggested change
for (const uri of Array.isArray(uris) ? uris : [uris]) {
if (!(await this.fileService.access(uri, FileAccess.Constants.R_OK))) {
this.messageService.error(`Cannot read resource at ${uri.path}.`);
return false;
}
}
const filePaths = await Promise.all((Array.isArray(uris) ? uris : [uris]).map(uri => {
return !await this.fileService.access(uri, FileAccess.Constants.R_OK) && uri.path || '';
}).filter(e => e);
if (filePaths.length) {
this.messageService.error(`Cannot read ${filePaths.length} resources: ${filePaths.join(', ')}`);
}
return !!filePaths.length;

@msujew
Copy link
Member

msujew commented Aug 23, 2021

Hi @kejsitake, thank you for your first contribution and taking care of this issue 👍

Please be sure to sign the eclipse contributor agreement (eca) with the same email as your authorship in order for us to accept the changes.

@vince-fugnitto vince-fugnitto added the electron issues related to the electron target label Aug 23, 2021
@kejsitake
Copy link
Contributor Author

I think I got the changes done and signed the eca. I am happy to look at this again if needed.

Copy link
Contributor

@colin-grant-work colin-grant-work left a 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 and works. One minor comment about naming that may prevent future confusion, but otherwise 👍

@colin-grant-work
Copy link
Contributor

@kejsitake Thank you very much for the contribution!

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

That looks good to me as well 👍

@tsmaeder tsmaeder merged commit eb9ba88 into eclipse-theia:master Aug 26, 2021
RomanNikitenko pushed a commit that referenced this pull request Sep 16, 2021
RomanNikitenko pushed a commit that referenced this pull request Sep 16, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Electron File Dialog Can't Open Read-only Files
5 participants