plugin: add workspace file api

[paul-marechal]

What it does

Add on(will|did)(create|rename|delete)Files to the plugin's workspace
api. This implementation does not handle the WorkspaceEdit apis.

Fixes: #7171

How to test

1. include the following plugin
2. perform add / rename / delete of files through the user-interface and and check that events are correctly fired.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[paul-marechal]

Is there a more standard/automated way of testing newly added plugin apis?

[akosyakov]

Should not we rather listen to FileSystem. With this PR we only propagate events done by one frontend. Is it correct? I think we should FS backend service as the source of truth and hook its events.

[vince-fugnitto]

Should not we rather listen to FileSystem. With this PR we only propagate events done by one frontend. Is it correct? I think we should FS backend service as the source of truth and hook its events.

@akosyakov these new APIs are only triggered by the user-interface and not the filesystem from what I understand by their definitions. For example:

[akosyakov]

@vince-fugnitto I meant by FileSystem api interface, not the disk (nsfw). It is called when a user makes a gesture. For instance, you have 2 tabs for the same workspace and delete a file using user gesture in one tab should be a plugin host process for another tab be notified? In the filesystem/workspace extension we were assuming so far that yes and we will then based on an event from FileSystem move widgets or mark widgets as deleted.

In this PR we disregard it and implement alternative logic to already existing. I don't think it is good.

[akosyakov]

I mean we already have move APIs here for instance:

theia/packages/filesystem/src/browser/filesystem-watcher.ts

Lines 130 to 133 in 00c1747

	protected readonly fileMoveEmitter = new FileOperationEmitter<FileMoveEvent>();
	readonly onWillMove = this.fileMoveEmitter.onWill;
	readonly onDidFailMove = this.fileMoveEmitter.onDidFail;
	readonly onDidMove = this.fileMoveEmitter.onDid;

Which report an event by triggered by a user from any window for the same backend. I wonder should not we add missing APIs there instead?

I am not sure what is right here, but having duplicate events does not look good. We should figure out what should be used. It could be that our use cases is different to VS Code, i.e. they cannot have multiple windows for the same workspace, but we can. Can they?

[paul-marechal]

@akosyakov ok I see what you mean now. I think I already took a look at the FileSystem api, but had two issues;

1. It seems like we need to use a DispatchingFileSystemClient instance to react to events, but it is only bound in the backend, and from what I understood, the plugin system only interfaces with browser instances?

2. I would need to break the fs client interface by adding new mandatory methods (will/did create file), is this ok?

   theia/packages/filesystem/src/common/filesystem.ts

   Lines 207 to 222 in 00c1747

   	export interface FileSystemClient {
   	/**
   	* Tests whether the given file can be overwritten
   	* in the case if it is out of sync with the given file stat.
   	*/
   	shouldOverwrite: FileShouldOverwrite;
   	willDelete(uri: string): Promise<void>;
   	didDelete(uri: string, failed: boolean): Promise<void>;
   	willMove(sourceUri: string, targetUri: string): Promise<void>;
   	didMove(sourceUri: string, targetUri: string, failed: boolean): Promise<void>;
   	}

Which report an event by triggered by a user from any window for the same backend. I wonder should not we add missing APIs there instead?

I can look into it, although using something designed with "watching" in mind confuses me a bit right now. But I agree that it would be better to avoid duplicating events.

It could be that our use cases is different to VS Code, i.e. they cannot have multiple windows for the same workspace, but we can. Can they?

No we can't open the same workspace twice in VS Code.

[akosyakov]

We have a task to redesign the FS completely that it is aligned with VS Code: #7127 I discussed it with Sven and decided to do it ourself, since it is very involving and we don't want many iterations on PR for this.

I was hoping that #7171 can be done just by binding events from

theia/packages/filesystem/src/browser/filesystem-watcher.ts

Lines 130 to 133 in 00c1747

	protected readonly fileMoveEmitter = new FileOperationEmitter<FileMoveEvent>();
	readonly onWillMove = this.fileMoveEmitter.onWill;
	readonly onDidFailMove = this.fileMoveEmitter.onDidFail;
	readonly onDidMove = this.fileMoveEmitter.onDid;

to VS Code events for now. It does not seem to be straightforward though. Also thinking about it over weekend, not even sure whether it is correct what we are doing with multiple windows right now.

I'm not sure it is good time to work on it. Maybe we are better to wait till #7127 is resolved. That's very likely that we will have to rework this part then.

If you want to pursue it still:

It seems like we need to use a DispatchingFileSystemClient instance to react to events, but it is only bound in the backend, and from what I understood, the plugin system only interfaces with browser instances?

We should not use it. Events should be exposed on FileSystemWatcher like onWillMove now and we should listen to them from the plugin system.

I would need to break the fs client interface by adding new mandatory methods (will/did create file), is this ok?

Yes, it is fine. It cannot be used anywhere else except FileSystemWatcher.

[paul-marechal]

@akosyakov thanks for the insight. I already started using the FileSystemWatcher like you mentioned, and so far it seems like it will work. I don't think the vscode fs api will be a big issue: I expect that all you'll have to do is keep emitters from our implementation fire the will/did events and the rest should fall into place as is (plugin bits).

[akosyakov]

@marechal-p ok, ping when you need another look. Please file CQs earlier if you copied code to the plugin system, it will help to merge the PR faster.

[paul-marechal]

Had issues with my Eclipse account, finally opened the CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22148

@akosyakov this PR is ready for review again, thanks.

[akosyakov]

Integration tests for documents are broken. We should make sure that they work as on master.

[paul-marechal]

@akosyakov Reverted most changes, tell me if what is left is better? CI is green now.

[akosyakov]

I'm uncomfortable to change Event APIs and propagate these changes to FS watcher APIs. Can we revert them for now till we know how the final solution will look like? I would prefer to copy as much as possible code for the extension host process from VS Code without changing to avoid diverging and it will likely remove the need for such APIs.

[paul-marechal]

@akosyakov reverted the changes to the events API. If the current changes don't make your planned changes more complicated then let's merge this :)

[akosyakov]

Code changes looks good to me, but i have not tried. @vince-fugnitto Could you verify? I think typescript and java extension make use of these events.

[vince-fugnitto]

Follow-up of #7718 (review)

I verified the following:

• the events are correctly fired using the plugin referenced in the pull-request description.
• verified that the vscode-typescript-builtin does not throw errors (logs) regarding the missing events ([TS] VS Code TS extension throws various errors #7840).