Propose API for [onWill|onDid][Create|Delete|Rename]

[jrieken]

This PR is for #43768. It adds a new onWillRunOperation-event to IFileService, removes the corresponding experiment from the text file service and exposes corresponding events in the API.

Open questions

• Should we enforce time limit for the onWill-events, like onWillSaveTextDocument, or should we just show a status bar message?
• Should we disable bad listeners (slow, throwing exceptions...)
• Should the onWill-events allow to do stuff, like sending off a workspace edits etc
• Using vscode.workspace.fs.delete etc will trigger this event which IMO is very correct (an extension might run this as part as a command) but needs documentation (see Propose API for [onWill|onDid][Create|Delete|Rename] #83822 (comment) and further comments)

[jrieken]

fyi @bpasero for changes in the file service

[bpasero]

@jrieken some feedback:

• the onWillRunOperation makes sense, then I would rename onAfterOperation to onDidRunOperation to be clear
• we have to be careful about what extensions can do when participating: e.g. if they are able to modify the document and leave it dirty before the operation starts, we will see a ton of issues that the respective ITextFileService methods (move/copy/del) helps with, but the file service is too low to deal with

[jrieken]

if they are able to modify the document and leave it dirty before the operation starts,

Yeah, that's the whole point of this API, e.g before renaming foo.java you need to update tweak its class definition... I have moved all of this to the file service so that we really catch all cases, e.g vscode.workspace.fs.delete etc. Tho, only rename allows to do things in API ways, collect workspace edits, all other don't allow things yet (e.g onWillCreate could be used to populate a file). Not sure how to proceed... It seems that we miss some sources of events or that we are short on some features...

[bpasero]

Jus to clarify what happens today when you move a dirty file:

• ITextFileService#move() must be used if you care about dirty state
• we find out that the file is dirty
• we use our backup service to create a backup of the dirty file at the target
• we mark the file as non-dirty so that the editor is happy
• we move it
• we restore it from that backup

Somewhere in there is the IFileService#move operation. If an extension modifies the file via text edits, the backup we made would be stale and wrongly restored.

I think in general, I would:

• only allow edits if ITextFileService is involved

(which I guess is very hard to enforce because an extension could simply write to the file on disk)

PS: I am also not sure what this means for custom editors that potentially also have dirty state that is not under the control of the workbench...

[jrieken]

Moving things to the text service is easy, it just means that those events aren't fire when using the fs-api. Unless, we wire some of the fs-call through the text file service... Not sure what that means but the textfile service likely doesn't care if non-textfile uris pass through it.

For now, I will move the will-events to the text file service. Likely also adding did-events on the text file service so that both events always come in pairs

[bpasero]

Maybe if extensions must have API to call move/copy/delete these methods should be exposed on a text document and then use the text file service under the hood. This would make it clear that the operations are considering dirty states and possibly more UI state of the document to carry over.

[jrieken]

@bpasero another challenge is that some extension would like to get bulk events, e.g when move 3 files in the explorer these should be one event listing those 3 files, not three events. This is to prevent expensive computations that need to be done. Extensions could solve this with debouncing but that's more fragile esp since we really know what's happening. Thought?

[bpasero]

Yeah I think that would require us to expose bulk methods on the text file service, or just accept an array of resources for move/copy/delete.

[jrieken]

@bpasero now it is really confusing as it seems that delete from the explorer doesn't go through the text file service but hits the file service directly. Is that the design? It seems to use the text file service to revert things but doesn't seem to use it for delete, in fact bulk edit seems to be the only user of TextFileService#delete.

[jrieken]

I have the feeling that those event might be best living in the file service and that we tackle the move-and-dirty case pragmatic, e.g by saving before applying an edit from extensions

[bpasero]

@jrieken that is correct, the explorer is doing something custom as you can see starting from here:

vscode/src/vs/workbench/contrib/files/browser/fileActions.ts

Line 160 in 7b038f1

const dirty = textFileService.getDirty().filter(d => distinctElements.some(e => resources.isEqualOrParent(d, e.resource)));

It will show a dialog asking to revert any files with dirty state. I think that makes sense to have on that layer because the bulk edit for sure does not want to bring up a dialog like that when running.

However, if we introduce the events on the level of ITextFileService, then I agree the explorer must call into that method 👍

I have the feeling that those event might be best living in the file service and that we tackle the move-and-dirty case pragmatic, e.g by saving before applying an edit from extensions

This will cause a weird flow of things:

• someone calls ITextFileService#move() with a dirty file
• a backup is made to the target to preserve the dirty contents
• we use the file service to move
• a move participant makes an edit and so we apply the edits save the file (that cannot happen from the file service because it has no knowledge about dirty files)
• the file is restored in the editor and we have a backup so we restore it
• the file is correctly still dirty, but we lost all the edits

Why can we not do the edits on the model and keep the editor dirty?

[bpasero]

Hm, maybe this could work:

• we move the events to file service as you suggest
• the text file service actually registers as listener to these events to participate in the move/copy/delete operation in the same way it does today with backups, dirty state, etc.
• we remove the move/copy/delete methods from text file service and everyone uses the file service

I am not sure just relying on the event would be enough for the text file service to do everything it does today, but it is something to investigate.

[jrieken]

Hm, that's an interesting thought. I am back and forth to where those events should live. If we move them into the file service we won't missing anything but also nothing can go unnoticed, e.g creating a log-file or deleting a temp-folder. The text-file service seems like a good abstraction of "user initiated" actions and would offer a clear boundary between the low-level and high-level world. Plus, it knows how to handle dirty move etc... Undecided.

And then custom editors are likely to have an impact on the text file service, e.g it might have to morph into a working copy service. Let's discuss Monday and in person