Implement `updateValue()` method for `PreferenceService` #9178

colin-grant-work · 2021-03-10T19:02:43Z

What it does

Fixes #9152 by implementing an updateValue method parallel to that available in VSCode

In addition, an error was previously thrown if set was called with a sectionName (tasks, launch) in User scope, but that is no longer appropriate because some section configurations are available in user scope, ~~so now that check is delegated to PreferenceSchema.isValidInScope()~~. I have removed any check for scope validity, because some tests specifically relied on being able to set a preference that was not (yet) valid. It could be argued that a check should happen early, but a warning will be shown later during the processing of a preference change - and the change won't take effect - if a preference is set that is not valid.

Here is one of the tests relying on setting a preference that is only made valid later:

theia/packages/core/src/browser/preferences/preference-service.spec.ts

Lines 262 to 274 in bb17f71

    
           it('get #0', () => { 
        
               const { preferences, schema } = prepareServices(); 
        
               preferences.set('[json].editor.tabSize', 2, PreferenceScope.User); 
        
               expect(preferences.get('editor.tabSize')).to.equal(4); 
        
               expect(preferences.get('[json].editor.tabSize')).to.equal(undefined); 
        
               schema.registerOverrideIdentifier('json'); 
        
               expect(preferences.get('editor.tabSize')).to.equal(4); 
        
               expect(preferences.get('[json].editor.tabSize')).to.equal(2); 
        
           });

How to test

Run the File > Auto Save menu item.
Observe that the setting is changed in user scope.
Create a command that calls e.g. PreferenceService.set('tasks', {version: '2.0.0', tasks: []});. E.g. add this to PreferencesContribution.registerCommands():

        commands.registerCommand({id: 'test-setting-preferences', label: 'Set a preference using PreferenceService.set'}, {
            execute: () => this.preferenceService.set('tasks', {version: '2.0.0', tasks: []}),
        });

Observe that it that setting takes effect in user scope and no error is logged.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

Signed-off-by: Colin Grant [email protected]

kittaakos · 2021-03-11T12:09:39Z

Formerly, if no scope was provided, PreferenceScope.Folder was used if a URI was supplied, PreferenceScope.Workspace otherwise. Now PreferenceScope.User is used instead of PreferenceScope.Workspace.

I like the idea but VS Code uses workspace as the default if I am not mistaken:

otherwise to Workspace settings.

Could you please check the docs, @colin-grant-work. Maybe, I am overlooking something. Thank you!

update(section: string, value: any, configurationTarget?: ConfigurationTarget | boolean, overrideInLanguage?: boolean): Thenable<void>
The configuration target or a boolean value. - If true updates Global settings. - If false updates Workspace settings. - If undefined or null updates to Workspace folder settings if configuration is resource specific, otherwise to Workspace settings.

colin-grant-work · 2021-03-11T15:13:06Z

update(section: string, value: any, configurationTarget?: ConfigurationTarget | boolean, overrideInLanguage?: boolean): Thenable<void>
The configuration target or a boolean value. - If true updates Global settings. - If false updates Workspace settings. - If undefined or null updates to Workspace folder settings if configuration is resource specific, otherwise to Workspace settings.

I agree, looks like workspace is the default for VSCode, though the boolean option is a weird twist (must be for backwards compatibility to an earlier system, I guess).

I'm not committed to changing the default scope. It was my understanding that what was requested in the bug report ticket was a more general change to the behavior of .set, rather than just setting the editor.autoSave preference in user scope. If we only want to fix where the auto-save preference gets set, that's an easy fix, too. I'd still take out the check for whether we're setting a sectionName in user scope, since that check is no longer relevant, but happy to return to the old defaulting.

vince-fugnitto · 2021-03-12T15:10:42Z

packages/editor/src/browser/editor-command.ts

@@ -333,6 +333,6 @@ export class EditorCommandContribution implements CommandContribution {
        return autoSave === 'on' || autoSave === undefined;
    }
    private async toggleAutoSave(): Promise<void> {
-        this.preferencesService.set(EditorCommandContribution.AUTOSAVE_PREFERENCE, this.isAutoSaveOn() ? 'off' : 'on');
+        this.preferencesService.set(EditorCommandContribution.AUTOSAVE_PREFERENCE, this.isAutoSaveOn() ? 'off' : 'on', PreferenceScope.User);


From what I understood, the issue with setting the preference with PreferenceScope.User means that it will not work for the following use-case which is supported in vscode:

editor.autoSave: false in the workspace settings ({workspace}/.theia/settings.json).

attempt to toggle the autoSave with the command.

the preference value ({workspace}/.theia/settings.json) is not updated while in vscode it is (.vscode).

Hm... so that clarifies where the problem with the current implementation really is. editor.autoSave should not be available in folder scope - it should only be set in a local settings.json in a single-root workspace, and in a multi-root workspace, only the workspace value should be picked up, not any folder scopes. If we're picking up a folder value in a multi-root workspace, that's a bug, and I'll look into that. Otherwise, it looks like setting it at workspace level is VSCode's behavior, and would avoid any awkward override situations.

@colin-grant-work I might have confused the workspace vs folder scopes, what I mean is the value under {workspace}/.vscode/settings.json is updated when toggling the command which does not happen in Theia if we are to specifically set the scope to User.

Yes, if there is a bug that is making us pick up folder-scoped preferences, that's the fix to do. Then we should not set it in user scope.

From what I understood, the issue with setting the preference with PreferenceScope.User means that it will not work for the following use-case which is supported in vscode:

editor.autoSave: false in the workspace settings ({workspace}/.theia/settings.json).

attempt to toggle the autoSave with the command.

the preference value ({workspace}/.theia/settings.json) is not updated while in vscode it is (.vscode).

First, we can check if the workspace config contains editor.autoSave. If yes, we change the value there. If no, we change it in the user config. Can this work?

Users get the Code behavior via the menu or command palette.

If users want to modify it manually via the settings.json, they know what they're doing.

If you're an extension developer, you either want to trigger the default functionality via the command, so we are good. Or you are doing custom things via the preferences service API.

Thoughts? Am I overlooking something?

So the code that actually performs this update in VSCode is not the update method to which you posted the docs earlier in this thread. It's the updateValue method, for which I don't think we have an equivalent. That code looks pretty similar to what you're describing: it loops through all scopes, finding the lowest one in which the value of the given preference isn't currently undefined (with user scope as default), and sets the value there. If the value is being deleted, it deletes it in all scopes in which it's defined.

https://github.com/microsoft/vscode/blob/257fbae3f34804eeb427c10bceb4c0ed95870552/src/vs/workbench/services/filesConfiguration/common/filesConfigurationService.ts#L185-L196

https://github.com/microsoft/vscode/blob/257fbae3f34804eeb427c10bceb4c0ed95870552/src/vs/workbench/services/configuration/browser/configurationService.ts#L295-L317

https://github.com/microsoft/vscode/blob/257fbae3f34804eeb427c10bceb4c0ed95870552/src/vs/workbench/services/configuration/browser/configurationService.ts#L783-L808

If we want to have that logic available for this command, we can implement methods to mirror that in the PreferenceService, or we can just do it manually for this command.

we can implement methods to mirror that in the PreferenceService

Good idea. Let's keep the new API aligned to the updateValue from Code. We can use this new API for auto-save and maybe for others too (theming?).

As an extension developer, I would prefer to use an API that has the same semantics as a VS Code extension API. What do you think?

I've implemented this. I'll add some tests next week to make sure it's doing what it should.

I've implemented this. I'll add some tests next week to make sure it's doing what it should.

@vince-fugnitto, the tests are still planned 😃

@colin-grant-work sorry about that :)

kittaakos

I tried it; it works very well. The code looks great. Thank you so much, @colin-grant-work 👍

packages/core/src/browser/preferences/preference-service.ts

vince-fugnitto

@colin-grant-work I believe the tests will need adjusting, they currently fail when testing the preferences.

colin-grant-work · 2021-03-19T19:36:24Z

@vince-fugnitto, I've implemented some tests here and modified the tests that were failing. If you wouldn't mind taking a look, I think it should be just about good to go.

vince-fugnitto

The changes look good to me, and I confirmed that autoSave now behaves like expected.

I only have a minor comment about the api-test where the import has an error but does not affect functionality.

vince-fugnitto · 2021-03-22T13:59:09Z

examples/api-tests/src/launch-preferences.spec.js

@@ -30,7 +30,7 @@ describe('Launch Preferences', function () {

    const { assert } = chai;

-    const { PreferenceService, PreferenceScope } = require('@theia/core/lib/browser/preferences/preference-service');
+    const { PreferenceService, PreferenceScope, PreferenceInspection } = require('@theia/core/lib/browser/preferences/preference-service');


I wonder if there is a way to avoid the error we get by this import:

Property 'PreferenceInspection' does not exist on type 'typeof import("/home/evinfug/workspaces/theia/packages/core/src/browser/preferences/preference-service")

The behavior there seems to be a bit inconsistent between environments, but I've modified the code in a way that seems to make the IDE happy in all of the environments available to me.

Signed-off-by: Colin Grant <[email protected]>

colin-grant-work added the preferences issues related to preferences label Mar 10, 2021

colin-grant-work force-pushed the bugfix/default-scope branch from 1b94533 to 8be966b Compare March 10, 2021 20:19

colin-grant-work force-pushed the bugfix/default-scope branch from 8be966b to 34603ac Compare March 11, 2021 15:19

colin-grant-work changed the title ~~Change default scope for PreferenceService.set()~~ Change setting of autosave preference to PreferenceScope.User Mar 11, 2021

vince-fugnitto reviewed Mar 12, 2021

View reviewed changes

colin-grant-work force-pushed the bugfix/default-scope branch from 34603ac to 400d483 Compare March 12, 2021 23:32

kittaakos approved these changes Mar 16, 2021

View reviewed changes

packages/core/src/browser/preferences/preference-service.ts Outdated Show resolved Hide resolved

vince-fugnitto reviewed Mar 16, 2021

View reviewed changes

colin-grant-work changed the title ~~Change setting of autosave preference to PreferenceScope.User~~ Implement updateValue() method for PreferenceService Mar 19, 2021

colin-grant-work force-pushed the bugfix/default-scope branch 3 times, most recently from ba28e45 to 865e92d Compare March 19, 2021 16:20

vince-fugnitto approved these changes Mar 22, 2021

View reviewed changes

Implement updateValue method for PreferenceService

ce50a61

Signed-off-by: Colin Grant <[email protected]>

colin-grant-work force-pushed the bugfix/default-scope branch from 865e92d to ce50a61 Compare March 23, 2021 14:39

colin-grant-work merged commit edc346b into eclipse-theia:master Mar 23, 2021

colin-grant-work deleted the bugfix/default-scope branch March 23, 2021 15:00

vince-fugnitto added this to the 1.12.0 milestone Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `updateValue()` method for `PreferenceService` #9178

Implement `updateValue()` method for `PreferenceService` #9178

colin-grant-work commented Mar 10, 2021 •

edited

Loading

kittaakos commented Mar 11, 2021

colin-grant-work commented Mar 11, 2021

vince-fugnitto Mar 12, 2021 •

edited

Loading

colin-grant-work Mar 12, 2021 •

edited

Loading

vince-fugnitto Mar 12, 2021

colin-grant-work Mar 12, 2021

kittaakos Mar 12, 2021

colin-grant-work Mar 12, 2021 •

edited

Loading

kittaakos Mar 12, 2021

colin-grant-work Mar 12, 2021

colin-grant-work Mar 16, 2021

vince-fugnitto Mar 16, 2021

kittaakos left a comment

vince-fugnitto left a comment

colin-grant-work commented Mar 19, 2021

vince-fugnitto left a comment

vince-fugnitto Mar 22, 2021

colin-grant-work Mar 23, 2021

	it('get #0', () => {
	const { preferences, schema } = prepareServices();

	preferences.set('[json].editor.tabSize', 2, PreferenceScope.User);

	expect(preferences.get('editor.tabSize')).to.equal(4);
	expect(preferences.get('[json].editor.tabSize')).to.equal(undefined);

	schema.registerOverrideIdentifier('json');

	expect(preferences.get('editor.tabSize')).to.equal(4);
	expect(preferences.get('[json].editor.tabSize')).to.equal(2);
	});

Implement updateValue() method for PreferenceService #9178

Implement updateValue() method for PreferenceService #9178

Conversation

colin-grant-work commented Mar 10, 2021 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

kittaakos commented Mar 11, 2021

colin-grant-work commented Mar 11, 2021

vince-fugnitto Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

colin-grant-work Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-grant-work Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittaakos left a comment

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

colin-grant-work commented Mar 19, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement `updateValue()` method for `PreferenceService` #9178

Implement `updateValue()` method for `PreferenceService` #9178

colin-grant-work commented Mar 10, 2021 •

edited

Loading

vince-fugnitto Mar 12, 2021 •

edited

Loading

colin-grant-work Mar 12, 2021 •

edited

Loading

colin-grant-work Mar 12, 2021 •

edited

Loading