Skip to content

File operation events support multiple resources#98988

Merged
bpasero merged 34 commits intomicrosoft:masterfrom
pfongkye:fix/#98309
Jun 24, 2020
Merged

File operation events support multiple resources#98988
bpasero merged 34 commits intomicrosoft:masterfrom
pfongkye:fix/#98309

Conversation

@pfongkye
Copy link
Contributor

@pfongkye pfongkye commented Jun 1, 2020 •
edited
Loading

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 bpasero and jrieken 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 •
edited
Loading

@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 •
edited
Loading

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 •
edited
Loading

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

pfongkye commented Jun 10, 2020

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
bpasero requested changes Jun 10, 2020
View reviewed changes
@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
bpasero
bpasero requested changes Jun 15, 2020
View reviewed changes
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.

isidorn
isidorn requested changes Jun 15, 2020
View reviewed changes
Pascal Fong Kye and others added 22 commits June 24, 2020 00:20
refactor: rename uricomponentspair
c85b38f
support multiple files on WorkingCopyFileEvent
270fd94
feat: support multiple resources
0af49f2 
onWillRunWorkingCopyFIleOperation onDidRunWorkingCopyFileOperation
refactor: make source optional for consistency
5efff11
refactor: support resources for delete
7cf8c1a
test: isolate tests
cd1664f
fix: iterate over resources
db5e4dc
feat: support operations on delete
8c13021
feat: adopt deleting multiple resources
4ad934a
fix: typing and sequential flow of copyservice
e615a80
fix: typing and naming
ff63341
fix: typing and naming
51b6fb7
fix: use different message for multiple overwrites
a74eb2d
refactor: naming consistency
73f27f7
fix: use array resources
545ef80
fix: message for multiple overwrites
3aebc9b
@bpasero
fix format
5b50555
@bpasero
clean up working copy file service
ee0ba2b
refactor multiple overwrites message helper
e98a9a5
use openeditors to bulk open
b68b71b
split drop copy and move
497c934
add returns
073936a
@pfongkye pfongkye requested a review from isidorn June 24, 2020 06:34
@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.

Reviewers

@jrieken jrieken jrieken left review comments

@bpasero bpasero bpasero approved these changes

@isidorn isidorn isidorn approved these changes

Assignees

@bpasero bpasero

Labels

None yet

Projects

None yet

Milestone

June 2020

Development

Successfully merging this pull request may close these issues.

Support multiple files in working copy file service

4 participants

@pfongkye @bpasero @isidorn @jrieken