Skip to content

Conversation

@gjsjohnmurray
Copy link
Contributor

@gjsjohnmurray gjsjohnmurray commented Aug 15, 2022

Second attempt at resolving #138815 to reduce the likelihood of a naive user storing their projects within the appRoot folder and then being upset when a VS Code update deletes their files.

This PR implements @bpasero's suggestion at #155443 (comment) but also makes Explorer play nice.

junk

@gjsjohnmurray
Copy link
Contributor Author

/assign @bpasero

local.updateName(disk.name);
}
local._isDirectory = disk.isDirectory;
local._readonly = disk._readonly;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the end this change wasn't necessary to make this PR work, but it looked like an omission so I have left it in. @bpasero what do you think?

@gjsjohnmurray
Copy link
Contributor Author

Submitting the PR has revealed that I've broken layering rules in the way I made Explorer work out whether to force metadata resolution so it gets to know when folders and files are read-only.

@bpasero please advise whether there's another way to achieve this.

@gjsjohnmurray
Copy link
Contributor Author

To avoid the layer breaker I have made Explorer get metadata for all file:// uris.

@bpasero bpasero added this to the Backlog milestone Aug 16, 2022
@bpasero bpasero requested review from bpasero and lramos15 August 16, 2022 05:21
@bpasero
Copy link
Member

bpasero commented Aug 16, 2022

Thanks, will review when I find time for it. @lramos15 fyi for explorer changes.

@gjsjohnmurray
Copy link
Contributor Author

Added a warning notification when workspace opens, similar to the one the previous PR had per-file:

image

@bpasero bpasero marked this pull request as draft August 31, 2022 08:05
@bpasero
Copy link
Member

bpasero commented Aug 31, 2022

Sorry, will probably not have a lot of time this month, might push this back a bit.

@gjsjohnmurray
Copy link
Contributor Author

@bpasero @lramos15 there have been two new issues raised this week by Windows users who have lost all their files. Neither yet proven to be caused by an update, but please can this PR make progress as a way of preventing future incidents of that sort?

@bpasero
Copy link
Member

bpasero commented Sep 22, 2022

@gjsjohnmurray I am not easy accepting this change, especially forcing requireMetadata will have a severe significant impact on explorer performance and this case here only impacts a handful of users.

How about a different approach where we simply open a notification if a user opens the installation dir or any child of that as a workspace folder? We could even leverage the banner for this to make it more prominent:

image

Maybe it could also be a modal dialog to be very annoying.

@gjsjohnmurray
Copy link
Contributor Author

@bpasero your verdict doesn't surprise me. I was uneasy about having to get metadata so unconditionally. Closing this. Please see #161534 instead.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2022
@gjsjohnmurray gjsjohnmurray deleted the fix-138815-v2 branch August 16, 2024 05:02
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.

2 participants