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

'(read-only)' suffix no longer added to tab labels of files served by readonly filesystem #82964

Closed
gjsjohnmurray opened this issue Oct 21, 2019 · 18 comments
Assignees
Labels
file-explorer Explorer widget issues under-discussion Issue is under discussion for relevance, priority, approach verified Verification succeeded
Milestone

Comments

@gjsjohnmurray
Copy link
Contributor

Issue Type: Bug

Looks like the work in 54c3db1 by @isidorn maybe resulted in the '(read-only)' suffix no longer getting added to the tab label of a file opened from a readonly filesystem. This used to work correctly in 1.38, though only after an additional click (see #69651).

The issue also exists in the latest Insiders.

I will prepare a branch of the MemFS sample to repro the issue.

VS Code version: Code 1.39.2 (6ab5985, 2019-10-15T15:35:18.241Z)
OS version: Windows_NT x64 10.0.17134

@vscodebot
Copy link

vscodebot bot commented Oct 21, 2019

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

@gjsjohnmurray
Copy link
Contributor Author

Branch at https://github.com/gjsjohnmurray/vscode-extension-samples/tree/show-82964 simply amends the MemFS sample so it registers as a readonly FileSystemProvider. The files created by first running MemFS: Setup Workspace followed by MemFS: Create Files behave correctly as readonly, but ever since 1.39 their tabs no longer show the '(read-only)' suffix. Nor does that suffix appear in the window titlebar.

@isidorn
Copy link
Contributor

isidorn commented Oct 21, 2019

The issue here is as I have already mentioned in an older issue:
What is missing it seems is the textFileService should have an event which fires when the model becomes readonly. fyi @bpasero
For example when the lastResolvedFileStat is set here we could check if the readonly state changes and if yes we could fire an event.
@bpasero would be open for a PR for this?

@isidorn isidorn added file-explorer Explorer widget issues under-discussion Issue is under discussion for relevance, priority, approach labels Oct 21, 2019
@bpasero
Copy link
Member

bpasero commented Oct 21, 2019

@isidorn yeah makes sense to me. we would probably not be able to detect this change unless the tab gets loaded again because there is no file event when this happens.

@gjsjohnmurray
Copy link
Contributor Author

I'm not clear why it worked OK before 1.39. Even now it's not (yet) possible to set an individual file as read-only. All we can do is register a FileSystemProvider with readonly: true in its options.

@bpasero
Copy link
Member

bpasero commented Oct 21, 2019

Isi would maybe know under which condition this can happen.

@isidorn
Copy link
Contributor

isidorn commented Oct 22, 2019

It was working because we were caching the title less agressivly.
Now we cache it properly and drop the cache only when there is an actual change.

@gjsjohnmurray
Copy link
Contributor Author

@isidorn I hope you agree that we at least need to initialize the cached value correctly. If a file is opened from a readonly FileSystemProvider it is correctly treated as readonly in VSCode. But since this change we have lost the indicator text. Surely a regression, no?

@bpasero
Copy link
Member

bpasero commented Oct 22, 2019

@isidorn I think this is unrelated to the file model, but rather we might need to have another event that causes all file editor inputs to loose their caches when a file system provider becomes available? The way I see it:

  • user has many files opened
  • files are from readonly file system
  • user reloads window
  • we show all the files very early as part of restoring the workbench
  • after a while the file system provider is resolved
  • now we should redraw labels if the provider e.g. is readonly

The model layer is too late because most tabs that visually appear in the editor are not loaded until you click them.

@isidorn
Copy link
Contributor

isidorn commented Oct 22, 2019

This makes sense.
I have pushed a change to drop all cache when a new file system provider is registered
@gjsjohnmurray can you please try vsocde insiders from Wednesday and let us know if the issue gets fixed for you. Thank you

@isidorn isidorn added this to the October 2019 milestone Oct 22, 2019
@gjsjohnmurray
Copy link
Contributor Author

@isidorn no, that hasn't fixed the issue for me. I just tested with the following using the MemFS example branch previously notified:

Version: 1.40.0-insider (user setup)
Commit: f5d3ffa6a0d655c87e1eb0e1e90773df58f7ff25
Date: 2019-10-23T05:25:50.857Z
Electron: 6.0.12
Chrome: 76.0.3809.146
Node.js: 12.4.0
V8: 7.6.303.31-electron.0
OS: Windows_NT x64 10.0.17134

@isidorn
Copy link
Contributor

isidorn commented Oct 23, 2019

Thanks for letting us know, let me reopen and setup your reproducable steps

@isidorn isidorn reopened this Oct 23, 2019
@isidorn
Copy link
Contributor

isidorn commented Oct 25, 2019

  1. I can reproduce this
  2. It is a timing issue. When the title bar part asks for the title the textFileService is not yet aware of the model here

I am open for ideas. Maybe we need a new event when a new model is added to the textFileService.models that I should drop the cache. But that would not be enough, since on new model add the tabs title control would need to be refershed.

@bpasero
Copy link
Member

bpasero commented Nov 20, 2019

@isidorn @gjsjohnmurray this should no longer be a problem in latest because for a couple of days now editor inputs have a isReadonly method that works even when no model is loaded yet by asking the file system provider for its capability:

return model?.isReadonly() || this.fileService.hasCapability(this.resource, FileSystemProviderCapabilities.Readonly);

Can you verify?

PS: I still wonder if we need to catch the case of a file system provider suddenly becoming readonly or not.

@gjsjohnmurray
Copy link
Contributor Author

@bpasero verified using my https://github.com/gjsjohnmurray/vscode-extension-samples/tree/show-82964 branch. Thanks!

Version: 1.41.0-insider (user setup)
Commit: 4934a6f
Date: 2019-11-20T05:25:39.619Z
Electron: 6.1.4
Chrome: 76.0.3809.146
Node.js: 12.4.0
V8: 7.6.303.31-electron.0
OS: Windows_NT x64 10.0.17763

@isidorn
Copy link
Contributor

isidorn commented Nov 20, 2019

Thanks for verifiying, adding verified label

@isidorn isidorn added the verified Verification succeeded label Nov 20, 2019
@gjsjohnmurray
Copy link
Contributor Author

PS: I still wonder if we need to catch the case of a file system provider suddenly becoming readonly or not.

@bpasero I'm unclear what you mean here. IIRC a FileSystemProvider can only declare itself readonly when it registers. I guess if it registered with readOnly: false there'd be nothing stopping it from later making all of its files report themselves as readonly.

@bpasero
Copy link
Member

bpasero commented Nov 20, 2019

@gjsjohnmurray yeah nevermind, internally we allow changes to capabilities but I think that is just not surfaced as extension API.

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
file-explorer Explorer widget issues under-discussion Issue is under discussion for relevance, priority, approach verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants