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

Fixes cannot disable file auto save when configuration target is other than user #109278

Merged
merged 3 commits into from Oct 26, 2020

Conversation

jeanp413
Copy link
Contributor

This PR fixes #108932

@bpasero
Copy link
Member

bpasero commented Oct 24, 2020

@jeanp413 thanks, this change made me look closer into the configurationService.updateValue method. As you can see, it seems to already do exactly what you do manually from within the implementation when you leave out the target:

private deriveConfigurationTarget(key: string, value: any, overrides: IConfigurationOverrides | undefined, target: ConfigurationTarget): ConfigurationTarget | undefined {
if (target) {
return target;
}
if (value === undefined) {
// Ignore. But expected is to remove the value from all targets
return undefined;
}
const inspect = this.inspect(key, overrides);
if (equals(value, inspect.value)) {
// No change. So ignore.
return undefined;
}
if (inspect.workspaceFolderValue !== undefined) {
return ConfigurationTarget.WORKSPACE_FOLDER;
}
if (inspect.workspaceValue !== undefined) {
return ConfigurationTarget.WORKSPACE;
}
return ConfigurationTarget.USER;
}

I actually think that we should not have code outside the configuration service that tries to find the right target, it should be handled consistently within the configuration service.

@sandy081 need your advice here: searching for references to ConfigurationTarget.USER I see a ton of uses, e.g. in the editor to toggle things, such as minimap:

public run(): Promise<any> {
const newValue = !this._configurationService.getValue<boolean>('editor.minimap.enabled');
return this._configurationService.updateValue('editor.minimap.enabled', newValue, ConfigurationTarget.USER);
}

If a user has configured the editor.minimap.enabled setting as a workspace setting, these actions actually do nothing.

To summarise, I think the auto save toggle is just the tip of the ice berg, any updateValue with ConfigurationTarget.USER will fail unless the setting is not allowed per workspace.

@bpasero bpasero self-requested a review October 24, 2020 09:48
@jeanp413
Copy link
Contributor Author

Removed the duplicated code, let me know if fixing the other places that you pointed out will be part of this pr or will be tackle apart.

@bpasero
Copy link
Member

bpasero commented Oct 26, 2020

Thanks, I have extracted #109373 for follow up on the other locations.

@sandy081
Copy link
Member

As a core service IConfigurationService provides multiple options for the consumer to update - like updating in specific target or resource etc., Its up to the caller to use the variant it needed. I agree that most of the toggle actions does not need to send the target and just use the updateValue(key, value) and the service does the right thing.

@jeanp413 jeanp413 deleted the fix-108932 branch October 26, 2020 15:08
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 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.

Cannot disable file auto save if configured in workspace
3 participants