Implement `files.dialog.defaultPath` setting (fix #115730) by gjsjohnmurray · Pull Request #182908 · microsoft/vscode

gjsjohnmurray · 2023-05-18T19:54:55Z

This PR resolves #115730 by adding a machine-level setting workbench.fileDialog.homePath that will override the user home folder, which is what the file and folder dialogs currently fall back to when no better choice is derived.

gjsjohnmurray · 2023-06-23T06:02:35Z

@bpasero any chance you could take a look at this? I don't think it's controversial, and reckon it will please a lot of people.

bpasero · 2023-06-23T06:38:19Z

Since this impacts simple file dialogs as well, adding @alexr00 for a look.

@gjsjohnmurray some high level feedback:

we have an existing setting files.simpleDialog.enable so maybe this setting should be called files.dialog.homePath
there is another getUserHome() method that probably needs to adopt the setting too
I find the code labeled as Normalize homePath setting so simple file dialog works correctly strange, why is this.pathService.fileURI not sufficient alone?

alexr00 · 2023-06-23T08:03:16Z

I find the code labeled as Normalize homePath setting so simple file dialog works correctly strange, why is this.pathService.fileURI not sufficient alone?

+1 I would like to understand this too.

gjsjohnmurray · 2023-06-23T08:30:45Z

I find the code labeled as Normalize homePath setting so simple file dialog works correctly strange, why is this.pathService.fileURI not sufficient alone?

+1 I would like to understand this too.

IIRC the 59c9323 commit was required to fix test failure. I'll investigate exactly which (the relevant CI log has already been purged).

gjsjohnmurray · 2023-06-23T10:30:06Z

I had misremembered. I added this when testing how my change behaved in the remote scenario, based on this advice:

#162874 (comment)

When debugging Code - OSS I used this command to open a remote:

Then I used Settings Editor to set a value for my setting (in screenshot below I have already renamed it as suggested by @bpasero ):

As my workstation is Windows, and the TestResolver remote treats my Windows filesystem as though on a remote, the intuitive format for the value I set is a Windows path.

But in the preferredHome() method this PR adds, userHomePromise resolves to a Uri whose _formatted property is 'vscode-remote://test%2Btest/c%3A/Users/JohnM' and calling its with method with the argument { path: dialogHomePath } throws:

[UriError]: If a URI contains an authority component, then the path component must either be empty or begin with a slash ("/") character

because I had set homePath to C:\Users\JohnM\Documents\GitHub

I'm not currently catching this, so the dialog never opens and the user doesn't get told why.

alexr00

Thank you for the details on Normalize homePath setting so simple file dialog works correctly.

there is another getUserHome() method that probably needs to adopt the setting too

userHome is used in 2 places in the simple file picker. I think we should only use the setting value in one of those places:

https://github.com/gjsjohnmurray/vscode/blob/b7034b80526c5b8ca5c27e1419868919898fceaa/src/vs/workbench/services/dialogs/browser/simpleFileDialog.ts#L204-L209

The other place is about replacing ~ and I don't think we should override that ever.

src/vs/workbench/contrib/files/browser/files.contribution.ts

…se it

alexr00

Looks good to me!

src/vs/workbench/services/dialogs/browser/abstractFileDialogService.ts

…mote

bpasero · 2023-06-25T06:40:54Z

I just found this method in the file picker:

vscode/src/vs/workbench/services/dialogs/browser/simpleFileDialog.ts

Lines 219 to 230 in 2bd04a1

    
           private remoteUriFrom(path: string, hintUri?: URI): URI { 
        
           	if (!path.startsWith('\\\\')) { 
        
           		path = path.replace(/\\/g, '/'); 
        
           	} 
        
           	const uri: URI = this.scheme === Schemas.file ? URI.file(path) : URI.from({ scheme: this.scheme, path, query: hintUri?.query, fragment: hintUri?.fragment }); 
        
           	// If the default scheme is file, then we don't care about the remote authority or the hint authority 
        
           	const authority = (uri.scheme === Schemas.file) ? undefined : (this.remoteAuthority ?? hintUri?.authority); 
        
           	return resources.toLocalResource(uri, authority, 
        
           		// If there is a remote authority, then we should use the system's default URI as the local scheme. 
        
           		// If there is *no* remote authority, then we should use the default scheme for this dialog as that is already local. 
        
           		authority ? this.pathService.defaultUriScheme : uri.scheme); 
        
           }

Which looks like it is doing a similar thing. Maybe @alexr00 could comment and maybe this needs to be used in this case here? It seems to do a bit more what I had suggested though.

src/vs/workbench/contrib/files/browser/files.contribution.ts

alexr00 · 2023-06-26T09:31:34Z

Which looks like it is doing a similar thing. Maybe @alexr00 could comment and maybe this needs to be used in this case here? It seems to do a bit more what I had suggested though.

I don't think that full method is needed here. The simple file picker has to do two extra things that the setting doesn't need to:

Display uniform slashes (all / or all \)
Switch between the remote file system and local file system.

alexr00

Switch between the remote file system and local file system.

Actually, looks like this is needed, and you've already done it ✅

bpasero · 2023-06-26T11:09:08Z

@gjsjohnmurray any thoughts on the name of the setting, only now I realise its called files.dialog.homePath, but isn't this rather files.dialog.defaultPath?

gjsjohnmurray · 2023-06-26T11:47:42Z

@bpasero I don't feel strongly about the current name. Fine with me if you want to rename it. Thanks for accepting the PR.

gjsjohnmurray · 2023-06-26T13:45:50Z

I updated the title of this PR to reflect the setting rename in #186156

@alexr00

…rosoft#182908) * Implement `workbench.fileDialog.homePath` setting (fix microsoft#115730) * Make it work with TestResolver remote * Rename setting to `files.dialog.homePath` * Add `preferredHome` to `IFileDialogService` so SimpleFileDialog can use it * Improve setting description following feedback from @alexr00 * Ignore home override when doing ~ replacement * Stop preferredHome falling back to user-local setting when working remote * Eliminate preferredHome hack

Implement workbench.fileDialog.homePath setting (fix microsoft#115730)

f96768c

gjsjohnmurray mentioned this pull request May 18, 2023

Default save / open location #115730

Closed

vscodenpa added the triage-needed label May 18, 2023

vscodenpa assigned hediet May 18, 2023

Make it work with TestResolver remote

59c9323

hediet assigned bpasero and unassigned hediet May 22, 2023

bpasero removed the triage-needed label May 22, 2023

bpasero added this to the Backlog milestone May 22, 2023

gjsjohnmurray added 4 commits May 25, 2023 16:40

Merge branch 'main' into fix-115730

101985f

Merge branch 'main' into fix-115730

71f530d

Merge branch 'main' into fix-115730

8e2eb48

Merge branch 'main' into fix-115730

1197d33

bpasero requested a review from alexr00 June 23, 2023 06:35

Rename setting to files.dialog.homePath

b7034b8

alexr00 requested changes Jun 23, 2023

View reviewed changes

src/vs/workbench/contrib/files/browser/files.contribution.ts Outdated Show resolved Hide resolved

gjsjohnmurray added 3 commits June 23, 2023 13:17

Add preferredHome to IFileDialogService so SimpleFileDialog can u…

05918e6

…se it

Improve setting description following feedback from @alexr00

1cc2bae

Ignore home override when doing ~ replacement

d5c2639

alexr00 previously approved these changes Jun 23, 2023

View reviewed changes

bpasero requested changes Jun 23, 2023

View reviewed changes

src/vs/workbench/services/dialogs/browser/abstractFileDialogService.ts Outdated Show resolved Hide resolved

Stop preferredHome falling back to user-local setting when working re…

165bfb7

…mote

gjsjohnmurray dismissed alexr00’s stale review via 165bfb7 June 23, 2023 22:10

gjsjohnmurray changed the title ~~Implement workbench.fileDialog.homePath setting (fix #115730)~~ Implement files.dialog.homePath setting (fix #115730) Jun 24, 2023

Eliminate preferredHome hack

ddaa876

bpasero requested review from alexr00 and bpasero June 25, 2023 06:36

bpasero reviewed Jun 25, 2023

View reviewed changes

src/vs/workbench/contrib/files/browser/files.contribution.ts Show resolved Hide resolved

alexr00 approved these changes Jun 26, 2023

View reviewed changes

alexr00 modified the milestones: Backlog, June 2023 Jun 26, 2023

bpasero approved these changes Jun 26, 2023

View reviewed changes

bpasero enabled auto-merge (squash) June 26, 2023 11:03

bpasero merged commit c1d726b into microsoft:main Jun 26, 2023

gjsjohnmurray changed the title ~~Implement files.dialog.homePath setting (fix #115730)~~ Implement files.dialog.defaultPath setting (fix #115730) Jun 26, 2023

gjsjohnmurray deleted the fix-115730 branch June 26, 2023 13:45

github-actions bot locked and limited conversation to collaborators Aug 10, 2023

Conversation

gjsjohnmurray commented May 18, 2023

Uh oh!

gjsjohnmurray commented Jun 23, 2023

Uh oh!

bpasero commented Jun 23, 2023

Uh oh!

alexr00 commented Jun 23, 2023

Uh oh!

gjsjohnmurray commented Jun 23, 2023

Uh oh!

gjsjohnmurray commented Jun 23, 2023

Uh oh!

alexr00 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexr00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bpasero commented Jun 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alexr00 commented Jun 26, 2023

Uh oh!

alexr00 left a comment

Choose a reason for hiding this comment

Uh oh!

bpasero commented Jun 26, 2023

Uh oh!

gjsjohnmurray commented Jun 26, 2023

Uh oh!

gjsjohnmurray commented Jun 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexr00 left a comment •

edited

Loading

bpasero commented Jun 25, 2023 •

edited

Loading