Should send a single FileWillRenameEvent instead of separate events when moving multiple files at once

[testforstephen]

Currently, if I drag and drop multiple files to a new location at once, the VS Code File Event API workspace.onWillRenameFiles will send a separate FileWillRenameEvent for each file to be moved. If i choose to use FileWillRenameEvent.waitUtil() to return the refactored edit, that means each event will return a separate edit and the client will call the applyEdit operation multiple times. This is inconvenient for undo operation.

It's better to send a single FileWillRenameEvent instead of separate events when moving multiple files at once.

[jrieken]

Very good find. We have this loop which goes over all resource edits and instead of the going one-by-one it should create buckets of equal operation types (likely without changing the order) and then apply a bucket at once

[jrieken]

This needs support from the working copy service. For instance, delete only supports a single options bag but multiple resources. Today, we ask the file- & config-service for a single resource whether to use trash or not but I don't see how that's possible with different resources from potentially different schemes/providers. There are similar issues with copying multiple files but using different overwrite settings etc

[bpasero]

@isidorn let me know if you are interested in a PR to change the working copy file service to support options per resource. I think the support for bulk came in later (via a PR) but we should have also allowed the options to be specified per resource. Instead of adding another options array I would probably just allow to pass in resource and options in one object.

Overall this is not a big change glancing at the existing code, shouldn't be hard.

[isidorn]

@bpasero yeah, I can look into a PR. Will do it early next week.

[jrieken]

@isidorn can you ping when you have done that PR

[isidorn]

Will do, started to look into it now...

[isidorn]

@jrieken @bpasero Created PR #112760

[bpasero]

Is this done?

[jrieken]

Nope

[jrieken]

@isidorn I am confused why creating doesn't support multiple resources at once while everything else does. Like, undoing a "multi delete" is now separate create operations

[isidorn]

@jrieken originally I did the create to support multiple resources, however after discussion with @bpasero I reverted back to only support a single operation.

I can move back to multi operation support. Though let's see if @bpasero is fine with that

[bpasero]

I have no feelings either way is fine for me.

[isidorn]

Okey so I will bring back the support for multi create. I will push directly to master and ping you Ben on the commit.

[isidorn]

I have just pushed a commit that tackles this d51e1fa
I did not adopt this fully in the bulkFileEdits since I thought you are wokring on it @jrieken Let me know if you would like me to look into that

[jrieken]

Thanks @isidorn. I am almost done with the adoption

[bpasero]

@isidorn checked out the change, it seems that the API now allows for multiple create operations, but within, we are not wiring this in properly (I think that was my initial feedback too that made us go back to supporting just 1 operation).

Specifically, shouldn't we use the array of things to create when passing on to the participants and eventing:

vscode/src/vs/workbench/services/workingCopy/common/workingCopyFileService.ts

Line 293 in d51e1fa

await this.runFileOperationParticipants([{ target: operation.resource }], FileOperation.CREATE, undoInfo, token);

vscode/src/vs/workbench/services/workingCopy/common/workingCopyFileService.ts

Line 296 in d51e1fa

const event = { correlationId: this.correlationIds++, operation: FileOperation.CREATE, files: [{ target: operation.resource }] };

Maybe @jrieken can clarify if this kind of change is still needed or not.

[jrieken]

Yes - the event should be fired for many resources. There is a skipped/failing test here:

vscode/extensions/vscode-api-tests/src/singlefolder-tests/workspace.test.ts

Lines 1008 to 1009 in 0cb7dad

	test('Should send a single FileWillRenameEvent instead of separate events when moving multiple files at once#111867', async function () {
	this.skip();

[isidorn]

@bpasero thanks for the feedback, I have created this PR #113919 to tackle it

[jrieken]

Closing. This is now done. @testforstephen please try this out with next insiders. Thanks

[testforstephen]

I have played with the insider on 2021-01-08, moving multiple files will be grouped to one FileWillRenameEvent, which works well for me. Thanks for this great fix.

[isidorn]

Thanks a lot for trying it out. Adding verified label based on your comment.