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

File operation events support multiple resources #98988

Merged
merged 34 commits into from
Jun 24, 2020

Conversation

pfongkye
Copy link
Contributor

@pfongkye pfongkye commented Jun 1, 2020

This PR fixes #98309

Hi @bpasero
I made some changes. Can you tell me if I'm on the right track?
If I am, I will continue with the following:

  • add support move/copy multiple resources in workingCopyFileService (wcfs)
  • add support delete multiple resources in wcfs
  • support for onWillRunFileOperation in wcfs
  • support for onDidRunFileOperation in wcfs?
  • adoption in explorerViewer
  • adoption in fileActions?
    - [ ] adoption in bulkEditService?
  • add tests if any
  • come up with a better name than UriComponentsPair?
  • check if it fixes given repro

Let me know if there is anything missing on the above list.
Thanks.

@bpasero bpasero requested review from jrieken and bpasero June 2, 2020 07:13
@bpasero bpasero self-assigned this Jun 2, 2020
@bpasero bpasero added this to the On Deck milestone Jun 2, 2020
@bpasero
Copy link
Member

bpasero commented Jun 2, 2020

@jrieken for this code change that touches the extension side of things.

@pfongkye in your list I am not sure you are aware that changes have to go into the working copy file service too, which is the main service used from e.g. the explorer when you move files.

@pfongkye
Copy link
Contributor Author

pfongkye commented Jun 2, 2020

Indeed, I'll make the necessary changes in working copy file service also. Added to the list above. Thanks for the feedback.

@bpasero
Copy link
Member

bpasero commented Jun 6, 2020

One quick feedback that I noticed, I think it is fine to have just one method in the working copy file service that accepts an array instead of having a move and moveMany method.

@pfongkye
Copy link
Contributor Author

pfongkye commented Jun 6, 2020

Yes. I was thinking the same thing.

@pfongkye pfongkye force-pushed the fix/#98309 branch 2 times, most recently from e99c695 to 4f5d4c4 Compare June 9, 2020 23:00
@pfongkye
Copy link
Contributor Author

pfongkye commented Jun 9, 2020

I think the PR is ready for review. I still need to work on supporting onDidRunFileOperation though.

@pfongkye pfongkye marked this pull request as ready for review June 9, 2020 23:22
@pfongkye
Copy link
Contributor Author

Btw I think we can support the same operations for delete also. What do you think?

For the bulkEdit, I'm not sure if options.overwrite will be the same for all the resources.

@pfongkye pfongkye marked this pull request as draft June 10, 2020 09:05
@bpasero
Copy link
Member

bpasero commented Jun 10, 2020

@pfongkye I had a quick look and think you are on the right path. Once you think this PR is ready for review, I would add maybe also Isidor to review the changes for where this needs to be adopted (file explorer land).

Yes, I think for consistency I would also do this for delete 👍

@bpasero bpasero modified the milestones: On Deck, June 2020 Jun 10, 2020
@pfongkye pfongkye force-pushed the fix/#98309 branch 3 times, most recently from 6f9d224 to e0cd188 Compare June 12, 2020 22:18
@pfongkye pfongkye marked this pull request as ready for review June 12, 2020 22:27
@bpasero bpasero removed the request for review from jrieken June 15, 2020 08:58
@bpasero
Copy link
Member

bpasero commented Jun 15, 2020

Assigning some folks to review:

  • @jrieken for changes in ext host and bulk edit
  • @isidorn for the adoption in explorer land (file actions, explorer)
  • me for the working copy changes

@bpasero bpasero requested review from bpasero, isidorn and jrieken June 15, 2020 08:59
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that maybe the file operations inside workingCopyFileService should run in sequence and not in parallel to reduce the chance of overlapping file system operations? I think that would also make the code cleaner.

@bpasero bpasero merged commit e3033fa into microsoft:master Jun 24, 2020
@bpasero
Copy link
Member

bpasero commented Jun 24, 2020

Thanks 👍

@pfongkye pfongkye deleted the fix/#98309 branch June 24, 2020 08:41
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple files in working copy file service
4 participants