LiveShare: Improve support for dirty readonly files

[IlyaBiryukov]

Issue Type: Bug

When an extension applies edits to the documents opened from a read-only file system provider, the document becomes marked as 'dirty' but saving it always fails with an error "Failed to save 'file.ext': Insufficient permissions. Select 'Retry as Admin' to retry as administrator." Retrying as administrator does nothing and repeats the error.

VS Code version: Code - Insiders 1.25.0-insider (5d6156a, 2018-06-25T09:40:10.343Z)
OS version: Windows_NT x64 10.0.16299

System Info

Item	Value
CPUs	Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz (8 x 3408)
GPU Status	2d_canvas: enabled flash_3d: enabled flash_stage3d: enabled flash_stage3d_baseline: enabled gpu_compositing: enabled multiple_raster_threads: enabled_on native_gpu_memory_buffers: disabled_software rasterization: disabled_software video_decode: enabled video_encode: enabled vpx_decode: enabled webgl: enabled webgl2: enabled
Memory (System)	31.92GB (12.16GB free)
Process Argv	C:\Program Files\Microsoft VS Code Insiders\Code - Insiders.exe C:\src\cascade\vscode\extension
Screen Reader	no
VM	0%

Extensions (3)

Extension	Author (truncated)	Version
cpptools	ms-	0.17.5
csharp	ms-	1.15.2
vsliveshare	ms-	0.4.0-dev

[bpasero]

@isidorn I wonder if a better approach would be to mark the underlying editor model as readonly instead of making the editor readonly and then having to fight for all these corner cases. I think a readonly model is the only way to prevent this from happening.

[isidorn]

@bpasero I was not aware that we have readonly models. I will look into that as it makes more sense to make the model, not the editor readonly

[bpasero]

@isidorn we do not have this currently as far as I know, I would ping @alexandrudima to ask if this is feasable

[alexdima]

@isidorn @bpasero We have no readonly models, we have only readonly editors.

The reason is simple: most likely you want to prevent the user from editing the model than the system. e.g. the output channel is a readonly editor with a buffer that can be modified; if we are to display a readonly file from disk, then we might want to listen to file change events and update the contents of the buffer from disk, even if we prevent the user from editing.

So I believe the current abstraction where an editor can be readonly, but that concept does not exist on a text buffer is a good abstraction.

[bpasero]

@alexandrudima but we have a lot more places where a model is not shown in an editor but potentially still can be edited programmatically, to give some examples:

• search & replace (across files) operates only on the model, not on the editor
• extensions can instantiate models (openTextDocument) without showing them and make edits to it through our API

So we need to carry the notion of the model being readonly on without the need for an editor somehow.

[alexdima]

@bpasero that should live in the layer above the TextModel, as the TextModel is not aware of file systems (reading, writing), dirty state, read only file systems, etc.

btw, it is trivial to create a readonly TextModel that throws every time its text is attempted to be edited or when setValue is called, but then you are going to need to deal with disposing TextModels correctly and creating them again (with all memory usage implications -- i.e. doubling the memory usage) every time such read only files change on disk. i.e. a readonly TextModel is readonly for everybody, including the workbench file event handling code.

[bpasero]

@isidorn talked with Alex, makes sense to not push this down to the code editor model. I believe we have to check all our usage patterns where we modify a model and guard against that:

• workbench places like search & replace that operate on the model
• extensions that use edits on a visible text document
• extensions that apply workspace edits

For each we need to see how to react, e.g. after discussion:

• search & replace could simply ignore the readonly models instead of throwing an error
• workspace edits should probably either apply in total or not at all raising an error

[isidorn]

@bpasero makes sense, I can look into this. Is there some good way to find all references of model mutating clients? I can go over all getModel references but that sounds like too many

[bpasero]

@isidorn clients of createModelReference would be a good start, these will create a workbench editor model and potentially modify it.

Search & replace is a bit different because I think we do not instantiate models for those unless already opened, but I could be wrong (@sandy081 would know).

[IlyaBiryukov]

About extension applying changes to a document opened from a read-only file system provider, we do need that working. The issue is the behavior of save. I think saving a document should just be disabled for read-only provider.

[bpasero]

@IlyaBiryukov yeah good point, extensions still need to be able to make changes to the model in case it updates. We could maybe change our model in a way that a readonly model never shows up as dirty to the user.

[sandy081]

Search & replace is a bit different because I think we do not instantiate models for those unless already opened, but I could be wrong (@sandy081 would know).

Search & replace does not instantiate any model directly, but uses bulk edit service API from @jrieken.

[jrieken]

Search & replace does not instantiate any model directly, but uses bulk edit service API from

Which uses the TextFileService from @bpasero

[bpasero]

Yeah, it is rather the ITextModelService which is being used to resolve the model and apply the edits. Maybe somewhere here it should throw an error if the model is readonly (we know that at that point).

[isidorn]

@bpasero that makes sense, however at that point we have a ITextEditorModel not a ITextFileEditorModel. Just casting feels like a hack, so not sure what to do there.
If we throw an error there should it catch most of the cases or we should still go through all the usages of createModelReference?

[bpasero]

Maybe we need to put this property one level up then?

[isidorn]

@IlyaBiryukov can you please try this out in tomorrow's vscode insiders when you find the time and let us know if it works fine for you. Thanks a lot
Now it should not longer attempt saving but immediatly throw an error

[dakowebrex]

hello,
you should try this for saving issue in ubantu
sudo chmod -R 777 PATH
LIKE..
sudo chmod -R 777 /opt/lampp/htdocs/potrider

[talhashahab786]

@dakowebrex worked for me! THANKS

[talhashahab786]

@dakowebrex First got to the folder using cd and ls.
Then :
sudo chmod -R 777 *

[bpasero]

@isidorn fyi with my latest changes around supporting custom editor save, there is also an impact here: an editor that shows up dirty but is marked readonly from the filesystem provider will no longer participate in the save. For example, the file appears as dirty but Cmd+S or Save All will ignore it.

Editors can now signal back if they are readonly (IEditorInput#isReadonly()) and the file editor input implements this via the FileSystemProviderCapabilities.Readonly capability:

vscode/src/vs/workbench/contrib/files/common/editors/fileEditorInput.ts

Line 227 in 90a0eb3

return this.fileService.hasCapability(this.resource, FileSystemProviderCapabilities.Readonly);

Now there is still probably quite more places where edits might end up on the document, so it is not 100% readonly from that perspective, but better.

[bpasero]

@IlyaBiryukov here is how latest behaves (note the dirty file that is readonly).

The editor is still considered to be dirty in the workbench and that means indicators show up as usual, however Cmd+S and Save All for example ignore this editor.

Still, the user cannot close the editor in this state without deciding on reverting the file. Doing so will not result in an error because it will fail but we ignore it since the editor is closing anyway.

I think this behaviour is better than before but wondering if you think it can be improved further.

[IlyaBiryukov]

It's definitely better than before.
Will the save prompt pop up when you close workspace / VSCode?
Can we not show the save prompt for a dirty file if the file system is read-only? Save does nothing there anyway.

[bpasero]

Yeah, there are still things missing. Specifically:

• closing such an editor will still ask for save/revert
• closing the application might ask for save/revert if hot exit is disabled

I think we need to come up with a new kind of dirty state specifically for readonly resources. The situation got a bit better with the changes around Save & Revert commands, but not fully there yet.

[bpasero]

One idea would be to never show any dirty indicator for readonly documents. Assuming that the user cannot cause the dirty change of a readonly file system anyway, that would probably be an OK change to do.

[bpasero]

@IlyaBiryukov given issues I recently filed such as microsoft/live-share#3090, microsoft/live-share#3092 and microsoft/live-share#3093 it would probably be a good idea to find a solution that works for all these cases.

My understanding is that the LiveShare extension directly modifies the editor of the guest for each keystroke the host does. As such, the guest will see a dirty indicator as soon as that happens because every change to the model will turn it dirty until it gets saved or reverted. This can happen if:

• the user saves the file (or it get's auto saved)
• the user reverts the file (e.g by invoking the revert command or closing the dirty editor without saving it)
• the user presses undo until the last saved state is reached again

I think my suggestion in #53257 (comment) is probably not a good one because we still want to indicate to the guest that a document has not been saved yet. Even if readonly, it may be interesting to know if the document is still dirty or not.

Given that, trying to find out what is missing in API today for LiveShare to handle these cases:

• an event for when a document changes its dirty state, this would allow to detect all the cases of save and revert properly
• a command or API to indicate that a document's dirty state should be cleared. I think a revert() method would come close here as it would a) clear the dirty indicator and b) load the contents as they are persisted by the host

Does that sound reasonable?

[bpasero]

Related: #7532 and #50969

[bpasero]

With d108195 I pushed a change to not mark an editor as dirty when its model changes and the editor is readonly (issue #89405).

The rationale is that we have an increasing number of custom file system providers built-in that register as readonly (for example Git). I think these providers are fine to make changes to the underlying model directly as needed, but should never result in dirty indicator to the user, nor play a role in hot-exit.

I tested this in todays insider build and when I join a readonly LiveShare session I am no longer seeing the dirty indicator when the host is typing. I think that is a reasonable change, but let me know otherwise.

[IlyaBiryukov]

Thanks! Appreciate it.

[bpasero]

@IlyaBiryukov I tested this and it seems to work fine, with one catch: with default settings, editors in VSCode open in preview mode unless you make changes (you see an italic title). Every editor you open replaces that preview editor. Now, since the guest no longer sees the editor as dirty in readonly sessions, it stays in preview mode (it used to get "pinned" automatically when becoming dirty), even if the host sees it not in preview. As such, any other editor the host opens will replace the preview editor in the guest session.

I felt this was an OK experience because it is up to the guest to e.g. double click the editor title to get it out of preview.