Improve Plugin Preference Access #11393

colin-grant-work · 2022-07-07T20:54:57Z

What it does

Fixes #10004
Depends on #11331
This PR updates various aspects of preference access and setting from plugins.

Adjusts PreferenceRegistry to use Monaco code for Configuration and ConfiguratioModel.
Enables access and setting of language-overridden preferences from plugins according to VSCode API (Introduce Type ConfigurationScope #10004)
Updates affectsConfiguration check to respect override options (language and URI)

How to test

Assert that the tests pass - most of the functionality related to preference access and affectsConfiguration has new tests.
Install this plugin (source)
Open a single-root workspace.
Run the command Listen for config changes
Add and remove tasks, observe that a toast informs you that you have done so.
Delete your tasks.json file.
Observe that a toast registers for changes to tasks and tasks.tasks

On master, the deletion of the file would not be registered by plugins as a change in the section.

Modify the value of editor.fontSize in your workspace; modify it for language overrides, including [rust]
Observe that your changes are accurately reported.
Clear the values of editor.fontSize in your workspace.

Run the command 'Stop listening for config changes' now if you want to see fewer toasts in the next step.

Run the command 'Run config test (no language override)'
It should modify the value for editor.fontSize in your workspace and report that the effective value has changed for non-overridden and for rust.
Run the command 'Run config test (with language override)'
Same as 10., but with different numerical values.
Run the command 'Run config test (with forced language override)'
It should create an overridden value for editor.fontSize for the rust language, and the report should show that only rust and not un-overridden editor.fontSize was affected
Run the command 'Run config test (with language override)' again.
This time, because an override already exists for rust after (13), you should see a toast similar to 14 - only the value for rust should be modified.

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

colin-grant-work · 2022-07-07T20:58:03Z

@tsmaeder, in the past, I believe you expressed concern about using Monaco code in the plugin host. This PR does so: it uses the Monaco versions of Configuration and ConfigurationModel rather than versions that we had copied when the plugin system was first written. Those classes reside in common folders of Monaco and they, or extensions of them, are also used in the VSCode plugin host. Do you believe that this use is reasonable, or that we should avoid importing directly from Monaco and copy / reimplement these classes?

tsmaeder · 2022-07-08T09:08:12Z

@colin-grant-work like all interesting questions, this one does not have a straightforward answer. I don't mind having VS Code stuff in the plugin host per se. The problem is that as we use more code outside the editor proper, the larger the change becomes when we uplift Monaco to a new version. The reason this is more of a problem with Monaco than with other libraries is that we have to update all Monaco modules at the same time. Is this the direction we want to move the code in? The upside is that we can reuse stuff from VS Code.
As for this PR, I would propose to keep the copied classes unless that's a problem. Now that we have more experience with the improved uplift procedures (with the new way we consume VS Code modules), we should discuss whether we think it's proper to use stuff from VS code in our code base more broadly with the community. If we do, the two classes can be easily nuked in a follow-up PR.

tsmaeder

Just some comments after a superficial look at the change.

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

tsmaeder · 2022-07-08T11:46:58Z

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

-        const globalValue = this.inspectInScope<T>(preferenceName, PreferenceScope.User, resourceUri);
-        const workspaceValue = this.inspectInScope<T>(preferenceName, PreferenceScope.Workspace, resourceUri);
-        const workspaceFolderValue = this.inspectInScope<T>(preferenceName, PreferenceScope.Folder, resourceUri);
+    inspect<T>(preferenceName: string, resourceUri?: string, forceLanguageOverride?: boolean): PreferenceInspection<T> | undefined {


The name forceLanguageOverride confused me a bit at first. What it means is "don't return the general value if the key is language specific", right? I haven't really thought of a good name, though.

I could reverse the polarity and rename it something like acceptDefault. There's no exactly antonym for language overridden, though...

packages/monaco/README.md

packages/monaco/src/browser/textmate/monaco-textmate-frontend-bindings.ts

- Adjusts PreferenceRegistry to use Monaco code for `Configuration` and `ConfiguratioModel`. - Enables access and setting of language-overridden preferences from plugins according to VSCode API - Updates `affectsConfiguration` check to respect override options (language and URI)

JonasHelming · 2022-07-29T14:25:17Z

@colin-grant-work Is this ready for a re-review?

colin-grant-work · 2022-07-29T14:26:58Z

@JonasHelming, yes - I believe I have addressed Thomas' comments, apart from the naming, and that review did not check the functionality, as far as I'm aware.

msujew

I'm happy with the changes, good job 👍

Language scoped preferences work as expected (i.e. doesn't bleed into the "normal" preferences, are applied correctly and instantly)
Adding a tasks.json and changes within are correctly transmitted to the plugin
Running the different plugin commands for (non)-language-override preferences works as expected

colin-grant-work added preferences issues related to preferences plug-in system issues related to the plug-in system labels Jul 7, 2022

tsmaeder reviewed Jul 8, 2022

View reviewed changes

colin-grant-work added 2 commits July 12, 2022 17:47

T extends JSONValue

15b4fe2

colin-grant-work force-pushed the bugfix/plugin-affects-check branch from 77b0fa6 to 15b4fe2 Compare July 12, 2022 23:47

Update doc for getConfiguration

5966f69

colin-grant-work mentioned this pull request Jul 15, 2022

Introduce Type ConfigurationScope #10004

Closed

JonasHelming requested a review from tsmaeder July 29, 2022 14:27

msujew self-requested a review August 2, 2022 11:36

msujew approved these changes Aug 2, 2022

View reviewed changes

colin-grant-work merged commit 7e177f9 into eclipse-theia:master Aug 2, 2022

colin-grant-work deleted the bugfix/plugin-affects-check branch August 2, 2022 14:30

colin-grant-work added this to the 1.29.0 milestone Aug 2, 2022

msujew mentioned this pull request Aug 3, 2022

JS Proxy error in plugin preference access #11501

Closed

thegecko mentioned this pull request Nov 19, 2022

Use environment-safe platform check in preferences #11888

Merged

1 task

t1m0thyj mentioned this pull request Jan 5, 2023

WorkspaceConfiguration update and inspect methods fail if section is undefined #12043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Plugin Preference Access #11393

Improve Plugin Preference Access #11393

colin-grant-work commented Jul 7, 2022

colin-grant-work commented Jul 7, 2022

tsmaeder commented Jul 8, 2022

tsmaeder left a comment

tsmaeder Jul 8, 2022

colin-grant-work Jul 12, 2022

JonasHelming commented Jul 29, 2022

colin-grant-work commented Jul 29, 2022

msujew left a comment

Improve Plugin Preference Access #11393

Improve Plugin Preference Access #11393

Conversation

colin-grant-work commented Jul 7, 2022

What it does

How to test

Review checklist

Reminder for reviewers

colin-grant-work commented Jul 7, 2022

tsmaeder commented Jul 8, 2022

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder Jul 8, 2022

Choose a reason for hiding this comment

colin-grant-work Jul 12, 2022

Choose a reason for hiding this comment

JonasHelming commented Jul 29, 2022

colin-grant-work commented Jul 29, 2022

msujew left a comment

Choose a reason for hiding this comment