Provide source info when reading configurations

[rebornix]

• VSCode Version: 1.5.0-Insiders
• OS Version: Windows 10

As discussed in #9731, we have no idea where an option is defined (default, user settings or workspace settings) when reading it by vscode.workspace.getConfiguration("section").get("optionName"). While in extensions, sometimes we may want to know whether users set options explicitly instead of using the default one but currently we don't support this kind of API.

cc @aeschli

[jrieken]

fyi @bpasero

[bpasero]

@jrieken I added IConfigurationService.lookup(key) which returns a structure of

IConfigurationValue<T>
{
   default: T
   user: T
   workspace: T
   value: T (<-- this is the merged value of them all, with higher priority to workspace and user setting)
}

I think we should expose this to extensions via some kind of new API. Do you have ideas about naming etc?

[jrieken]

Cool. Is there also some sort of key enumeration?

[bpasero]

@jrieken there is https://github.com/Microsoft/vscode/blob/master/src/vs/platform/configuration/common/model.ts#L180 which is currently not within the configuration service but can be used from anywhere.

[jrieken]

My plan is to add an inspect method to the WorkspaceConfiguration object (along with update). The only catch is that users might pick a partial key in which case both methods would bail out.

For instance, such a call won't work getConfiguration('files').update('watcherExclude.**/node_modules/**', true) whereas this does work getConfiguration('files').get('watcherExclude.**/node_modules/**') (the latter is unaware of key vs object-values).

[jrieken]

@bpasero Why is there the separation between workspace-config-service and the other config-service. I looks rather artificial, esp. with the lower-layer already knowing about user-settings. Why not merge them and have workspace-settings being undefined when not applicable?

[bpasero]

@jrieken the split between a configuration service in platform and the one in workbench was done because we use the configuration service in many more environments: CLI, shared process, main side and the workbench renderer side. Pushing the notion of workspaces down to the platform layer was very bad for those areas where we do not have nor want a notion of workspaces (which is all of them except for workbench). We used to have it down there and it took some time and was very painful to get it out of there again.

[jrieken]

I am not saying that you should push down the implementation but the interface. Now depending on where my code lives my service dependency changes which weakens the whole point of having interfaces and injections.

[bpasero]

@rebornix in your summary you ask for a way to find out if a setting is defined in user vs workspace land but it is unclear to me why that is needed? Can you share the use case for this? Would you be OK to just have a method that tells you if a setting is defined by the user or not?

@jrieken if we do not need to distinguish between user and workspace setting then maybe we can simplify this altogether and we do not need the services separation for this case.

[rebornix]

@bpasero the reason I separated them is that I'm not sure which category Workspace settings belong to. Workspace overrides User and User overrides Default, there are some similarities between Workspace and User settings, they are set by purpose. To some extent, Workspace setting is like a Super User setting which has the highest priority.

A user case for workspace info is VSCodeVim/Vim#595 , we want to set useCtrlKeys as true on Mac but false on Windows. Since Code has no support for this kind of setting so we decide to initialize the default for useCtrlKeys while the extension starts. So if there is a userCtrlKeys option in Workspace or User settings, we just use the value. If not, we set it as true on Mac, false on Windows. In this case, a method that tells you if a setting is defined by the user or not? is not enough.

Another user case is about the overlap between Code's config and Vim's. For example, Vim's tabstop and Code's tabsize. So our plan about the config overridden priority is, if the vim.tabstop is set by purpose (workspace setting or user setting), we will use it to override Code's tabSize. So if the method you suggest tells me the config is not defined by user, I still have no idea whether it's defined in Workspace or not.

With both workspace and user information, above two features can be implemented easily but there might be workarounds, let me know if you have any suggestion.

But if you guys do think separating workspace and user makes the code complicated and bad, I'm Okay with @bpasero 's proposal. These two features are not Vim blockers.

[jrieken]

@rebornix I could make WorkspaceConfiguration#inspect return something like this

{ 
  key: string; 
  value: T;
  isGlobalValue: boolean; 
  isWorkspaceValue: boolean; 
}

Still unsure about the names but the information you need should be there.

I wonder if anyone is interested in the overridden values? Like effective value is 4, global/user value is 2, and default is 8.

[rebornix]

@jrieken Your proposal looks good to me. I don't see any reason why people wants to know the default value or global value if there is already an effective one as well.

[bpasero]

@rebornix from your examples it sounds to me that all you need to know is if a setting is defined by the user or not. You do not seem to care if the setting is defined in the workspace or as a global setting.

I am fine with the suggested API in #10583 (comment) though I am not sure if an extension would need to know if a setting is defined in user vs workspace land. Maybe there is a scenario where an extension needs to know, but currently I cannot think of one. I think in most cases an extension would know exactly where to set the configuration: if the extension is useful for many workspaces, it should set the configuration on a user level. If the extension is only useful in the current workspace you are in, it would set it on that level.

[bpasero]

@jrieken let me know if something else missing here from the workbench service to add it to API for September

[jrieken]

I think everything is there, except for time... snowplowing to October

[eamodio]

@jrieken I think it would be great if an extension could get at the real values -- something like:

{ 
  key: string; 
  value: T;
  globalValue: T | undefined; 
  workspaceValue:  T | undefined; 
}

Or something along those lines

[jrieken]

why not read the entire configuration and check if the key exists?

Not so sure what that means?

[bpasero]

@jrieken hm needs thinking. today we do the opposite: read config files and flatten them into a big object. we do currently not have a way to do the opposite which is:

• collect all keys from schemas
• collect all keys as they show up in User settings.json
• collect all keys that show up in Workspace settings.json, launch.json and tasks.json

[bpasero]

@jrieken I added IConfigurationSevice.keys() that returns:

{
   default: string[];
   user: string[];
   workspace: string[]
}

similar to IConfigurationService.lookup().

[jrieken]

Thanks!

[jrieken]

@eamodio Re #10583 (comment) what would be your use-case of accessing all values (default, user, workspace)?

[jrieken]

@bpasero I have noticed that there are differences in getConfiguration and lookup and I am not sure if they are indented or not. With the snippet below I get false when drilling into the individual notes of javascript.format.insertSpaceAfterOpeningAndBeforeClosingJsxExpressionBraces but the second statement lists undefined for all of them

console.warn(configurationService.getConfiguration());

console.warn(configurationService.lookup('javascript.format.insertSpaceAfterOpeningAndBeforeClosingJsxExpressionBraces'));

[bpasero]

@jrieken I literally copied the code you wrote for this (I have noticed it as well): https://github.com/Microsoft/vscode/blob/master/src/vs/platform/configuration/common/configuration.ts#L63

From: 648681e

[eamodio]

@jrieken I was thinking that without access to the full set of values an extension couldn't be sure of how a setting could/would behave. So if an extension wanted to consolidate a setting so that if the workspace version matched the user version, then the workspace version could be removed.

I was also thinking of a specific case with visible whitespace where I want to know what the current setting is, and where to alter it. Although thinking more about my specific whitespace case -- which is more about a transient override rather than a real desire to change the saved setting, I think something like that could be better handled if there was another "scope" for any setting -- basically an in-memory only override that would not be persisted. But that is outside the scope of what we are talking about here.

So while I think the case I originally had in mind is better served through some other mechanism -- I think it could still be useful for an extension to be able to tell for a setting what the user intent is as well as the workspace one.

[jrieken]

re #10583 (comment) Wasn't me, I only added the default: https://github.com/Microsoft/vscode/blame/master/src/vs/platform/configuration/common/configuration.ts#L68

But that code is the problem because it checks for falsy and not undefined

[bpasero]

Thx for the fix.

[jrieken]

API proposal goes like this

inspect<T>(section: string): { key: string; defaultValue?: T; globalValue?: T; workspaceValue?: T };

@rebornix @eamodio @bpasero feedback, ideas?

[bpasero]

@jrieken works for me 👍

[eamodio]

@jrieken looks great to me too

[rebornix]

@jrieken it's great! 🚢

[Jakobud]

slightly off topic. I'm a bit of a noob on this subject I guess, but what does the <T> mean?

[jrieken]

The <T> is the actual type of the setting, like boolean or string