Add option to disable Show Local button

[jeffmax]

Fixes #5118

The PR does two things: It hides the "Show Local" button for Save/"Save As"/"Save Workspace As" when the --disable-file-downloads flag is used and it also introduces a new --disable-show-local flag that disables the "Show Local" button for Save/"Save As"/"Save Workspace As" and Open File. I also had to modify the navigateMenus test code so that I could target "Save" specifically. Without the changes it was choosing "Save Workspace As" when asked to navigate to "Save".

[codecov]

Codecov Report

Merging #6529 (e36b5c5) into main (897e0ae) will not change coverage.
Report is 16 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6529   +/-   ##
=======================================
  Coverage   73.13%   73.13%           
=======================================
  Files          31       31           
  Lines        1906     1906           
  Branches      409      409           
=======================================
  Hits         1394     1394           
  Misses        433      433           
  Partials       79       79           

Files	Coverage Δ
src/node/cli.ts	90.90% <ø> (ø)

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11e6e65...e36b5c5. Read the comment docs.

[code-asher]

Thank you, this is awesome! I should be able to review more thoroughly this week but at first glance it looks good. You can ignore the error on the Build code-server step just fyi, there is some issue right now where in CI not all the dependencies are being installed which I can hopefully fix soon.

One thought in the meantime, do we need the disable-show-local flag? Or to ask it another way, is there a reasonable case where someone would want to disable the "show local" button but still allow downloads? My current perception is that they will always be disabled together or not at all, so the extra flag might not be necessary.

Thinking out loud, I could see a use for something like a disable-local-filesystem (and disable-file-downloads becomes deprecated, maybe), and that one would disable uploads, downloads, saving to local, reading from local, any interaction with the local filesystem at all.

[jeffmax]

@code-asher Thanks! I agree that the --disable-show-local use-case is a bit confusing. I added it for users that wanted to begin to disable uploads or just generally wanted to disable "Show Local" functionality (I found it generally confusing and unexpected). I was actually planning on tackling a full --disable-file-uploads feature next, but I had not dug into that yet. I do have a use case where folks might need to enable/disable uploads and downloads independently of one another, so keeping two separate flags would be preferable to me. I agree though, fewer, clearer flags is better.

Another, related feature that I was hoping to add was the option to log uploads and downloads. It would involve something like adding

this.logService.info(`readFile ${resource.path}`)

to:

https://github.com/microsoft/vscode/blob/1a5daa3a0231a0fbba4f14db7ec463cf99d7768e/src/vs/platform/files/node/diskFileSystemProviderServer.ts#L95

Is that something you would consider accepting?

[code-asher]

I do have a use case where folks might need to enable/disable uploads and downloads independently of one another

Got it, so what we want in the end is --disable-file-uploads and --disable-file-downloads. What do you think about removing --disable-show-local for now, and we hold off disabling the "show local" button for opening until we implement --disable-file-uploads (or we partially implement that flag now and hide it for the time being)? That way we can avoid having to support that extra flag indefinitely.

Another, related feature that I was hoping to add was the option to log uploads and downloads

To me this feels like something that should be logged at trace level, would that meet your needs? Or, depending on your goals, maybe you could use the telemetry data.

Also, that will log more than just uploads/downloads, right? Is that OK for your use case?

[jeffmax]

@code-asher Sure, I can remove the --disable-show-local flag and standalone tests for now and follow this PR up with one for --disable-file-uploads (I am actually digging through the upload code for this now). If I do that, can I roll download related "Show Local" changes on this PR into disable-file-downloads.diff?

Is trace another log level like info and debug? If so, that makes sense to me. I know it would catch more than uploads and downloads but I was looking for the best server-side place to record those actions - is there another spot within the server code that would be more specific to uploads and downloads? I know the client has a logger but it seems to log to the browser console, I didn't see that information going to the server.

[code-asher]

can I roll download related "Show Local" changes on this PR into disable-file-downloads.diff?

Yup, that sounds perfect to me. Feel free to use it for the uploads diff as well if you want (although we should probably rename it if we do).

Is trace another log level like info and debug?

Exactly, it would be this.logService.trace I think. It is the most verbose logging level in VS Code.

I was looking for the best server-side place to record those actions

Ahhh yeah that makes sense, I think only the browser-side code knows that it is an upload/download. I thought the browser logs also got stored server-side in a file but it seems I am wrong about that, or at least when I checked just now I did not see any, so readFile is looking like the closest place to me as well right now.

[jeffmax]

@code-asher Is it ok to roll this into one PR? I was going to do two separate ones for download/upload but if I put them in one diff file its simpler as one. Would it be ok to put the trace statement in there as well?

[code-asher]

Is it ok to roll this into one PR
Would it be ok to put the trace statement in there as well

Yup both things sound good to me!

[jeffmax]

I am closing this in favor of PR #6557