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

Allow FileSystemProvider to stat a file as readonly (#73122) #111237

Merged
merged 16 commits into from
May 12, 2021

Conversation

gjsjohnmurray
Copy link
Contributor

This draft PR addresses #73122

It adds an optional readonly boolean property to the FileStat interface, thus enabling a FileSystemProvider to control whether a file is editable or read-only.

One way to test:

  1. Clone https://github.com/gjsjohnmurray/vscode-extension-samples/tree/test-vscode-73122-fsp-stat-readonly
  2. Open fsprovider-sample folder.
  3. Add this PR's vscode.proposed.d.ts file to that folder.
  4. Build and run.
  5. Run the commands to init the MemFS workspace and add the MemFS files.
  6. Open any of the .txt files to see them read-only.
  7. Open any of the other files to see that they are editable as usual.

Submitted as a draft PR seeking comment and guidance from @jrieken about taking this through the API proposal precedure.

@jrieken
Copy link
Member

jrieken commented Nov 24, 2020

Adding @mjbvz for custom editors, adding @bpasero as feature area owner

@jrieken
Copy link
Member

jrieken commented Nov 24, 2020

Submitted as a draft PR seeking comment and guidance from @jrieken about taking this through the API proposal precedure.

The checks are in place and things are defined proposed.d.ts - so all good from that side. All API proposals are discussed during a weekly call and I can represent this issue. So far, the FS provider API has been posix-like and this isn't strictly like it anymore, tho personally I don't mind it.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Left some feedback in the code. On top of that:

  • isn't mainThreadFileSystem#$stat missing to set the readonly flag?
  • this should get a test to verify the text editor model reports readonly for readonly file stat

src/vs/platform/files/common/files.ts Outdated Show resolved Hide resolved
@bpasero bpasero removed their assignment Nov 25, 2020
@jrieken jrieken assigned bpasero and unassigned jrieken Nov 25, 2020
@bpasero bpasero added this to the Backlog milestone Nov 25, 2020
@gjsjohnmurray
Copy link
Contributor Author

* isn't `mainThreadFileSystem#$stat` missing to set the `readonly` flag?

Added in latest push.

* this should get a test to verify the text editor model reports readonly for readonly file stat

I'd appreciate some guidance on doing this.

I have updated my branch of fsprovider-sample so the readonly state of a file can be toggled while open. If I close the file tab and reopen it the readonly state of the editor changes, but I think this needs to happen dynamically. @bpasero you recently did something similar in #110854 to handle a FSP's readonly capability changing on the fly.

@gjsjohnmurray gjsjohnmurray marked this pull request as ready for review January 5, 2021 10:01
@gjsjohnmurray
Copy link
Contributor Author

I am back at my desk after the holidays, so I've removed draft status from this PR.

@joaomoreno joaomoreno changed the base branch from master to main February 15, 2021 08:51
@bpasero
Copy link
Member

bpasero commented Apr 30, 2021

@gjsjohnmurray sorry for the time this took. I tried to bring your branch up to date with main but I do not have permissions it seems to push. Anyway, maybe you can force your branch to be on this commit and then we can proceed here: cf122a2

Remaining tasks:

  • test coverage (aka ensure the readonly change event fires when a underlying provider changes readonly state for a file) [1]
  • make sure the editor control updates properly based on readonly changes, similar to how we currently do it when the provider capabilities change ([2]). A provider that after some time decides to change the readonly state for a file should reflect in the editor turning readonly or not

[1]
As for best ways how to test this, I would suggest to look at the src/vs/workbench/services/workingCopy/test/browser/fileWorkingCopy.test.ts. file working copies will eventually be the default for text file model too, so we do not have to add tests for both. I would think that the coverage should be:

  • the onDidChangeReadonly event firing properly
  • the isReadonly respecting the last resolved file stat or fallback to the system provider readonlyness

[2]

private onDidFileSystemProviderChange(scheme: string): void {
const control = this.getControl();
const input = this.input;
if (control && input?.resource.scheme === scheme) {
control.updateOptions({ readOnly: input.isReadonly() });
}
}

@bpasero bpasero modified the milestones: Backlog, May 2021 Apr 30, 2021
@gjsjohnmurray
Copy link
Contributor Author

I tried to bring your branch up to date with main but I do not have permissions it seems to push. Anyway, maybe you can force your branch to be on this commit and then we can proceed here: cf122a2

@bpasero I'm not familiar with this procedure. I have now set the PR's checkbox Allow edits and access to secrets by maintainers, so maybe you can do it yourself now. Or else please tell me the git command to issue or the option to pick from the SCM ... menu in VS Code.

@bpasero
Copy link
Member

bpasero commented May 1, 2021

@gjsjohnmurray done

@bpasero bpasero linked an issue May 3, 2021 that may be closed by this pull request
@bpasero bpasero self-requested a review May 3, 2021 12:56
@bpasero
Copy link
Member

bpasero commented May 11, 2021

@gjsjohnmurray are you planning to work on this further? Otherwise I can look into the finishing touches.

From a API discussion we had today, we picked one of the proposals from #73122 (comment). Basically it would be this:

export enum FileStatPermissions {
	/**
	 * The file is readonly.
	 */
	Readonly = 1
}

/**
 * The `FileStat`-type represents metadata about a file
 */
export interface FileStat {

	/**
	 * The permissions of the file, e.g. whether the file is readonly.
	 *
	 * *Note:* This value might be a bitmask, e.g. `FileStatPermissions.Readonly | FileStatPermissions.Other`.
	 */
	permissions?: FileStatPermissions;
}

@gjsjohnmurray
Copy link
Contributor Author

@bpasero I'll happily leave it in your safe hands. Thanks!

@bpasero bpasero merged commit 793a123 into microsoft:main May 12, 2021
@bpasero
Copy link
Member

bpasero commented May 12, 2021

Thanks 🥂

@gjsjohnmurray gjsjohnmurray deleted the 73122-fsp-stat-readonly branch May 13, 2021 11:45
@github-actions github-actions bot locked and limited conversation to collaborators Jun 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable FileSystemProvider to stat a file as readonly
3 participants