fix #6046: workspace configuration contributed by VSCode extension an…

[a1994846931931]

…d provided by .theia/settins.json is sometimes wrongly filtered out

Signed-off-by: Cai Xuye a1994846931931@gmail.com

What it does

fix #6046

For more information about this revision, please refer to #6046 (comment)

How to test

Test according to #6046 (comment). In the fixed version, the start message should give you a correct value read from the workspace configuration.

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

[akosyakov]

I wonder how it affects native extensions which not expecting invalid values.

[a1994846931931]

I wonder how it affects native extensions which not expecting invalid values.

@akosyakov Yeah, this could cause a problem. Do you have a better idea?

But for now, even before this revision, invalid values in user configuration are not handled. In fact, only configurations under ${workspaceFolder}/.theia/settings.json would be validated because of these lines (note that this.configurations.getPaths()[0] is .theia) https://github.com/theia-ide/theia/blob/1e67e74498a369c4665b28afd64e91ec5f6e0133/packages/preferences/src/browser/abstract-resource-preference-provider.ts#L171-L177

For more information and explanation on why configurations provided by ~/.theia/settings.json are not validated: #6046 (comment)

[akosyakov]

But for now, even before this revision, invalid values in user configuration are not handled.

Yeah, sound like it is working by the pure luck. I will think how we can go about it next days.

[svenefftinge]

I think removing the check is good. Having it different when the settings its in vscode folder is clearly surprising and native extension would be faced with wrong values that way already.

[akosyakov]

We can try to move the check to statically typed APIs to avoid breaking them: https://github.com/theia-ide/theia/blob/master/packages/core/src/browser/preferences/preference-proxy.ts#L44

As you pointed out it should not be matter from which folder it comes.

[akosyakov]

@a1994846931931 Would you have time to try moving a validation check to statically typed proxies?

[a1994846931931]

@a1994846931931 Would you have time to try moving a validation check to statically typed proxies?

OK, I'll give it a try. But can you please offer some advice on how I should test on it and, maybe also on what should happen when a value is invalid?

[akosyakov]

OK, I'll give it a try. But can you please offer some advice on how I should test on it and, maybe also on what should happen when a value is invalid?

I meant to change https://github.com/theia-ide/theia/blob/78e0b2093c38a69633cfa69a728082859263ad47/packages/core/src/browser/preferences/preference-proxy.ts#L74 to something like:

        const value = preferences.get(preferenceName, defaultValue, resourceUri);
        if (preferences.validate(preferenceName, value)) {
            return value;
        }
        if (defaultValue !== undefined) {
            return defaultValue;
        }
        const values = preferences.inspect(preferenceName, resourceUri);
        return values && values.defaultValue;

and https://github.com/theia-ide/theia/blob/78e0b2093c38a69633cfa69a728082859263ad47/packages/core/src/browser/preferences/preference-proxy.ts#L79 to something like:

                const value = preferences.get(property);
                if (preferences.validate(property, value)) {
                    return value;
                }
                const values = preferences.inspect(property);
                return values && values.defaultValue;

You will need to implement validate by delegating to PreferenceSchemaProvider.

You can test it with DebugConfiguration for example, just change setting to bogus values and check that default value is provided to clients.

[a1994846931931]

@akosyakov Thanks to your comment, I just found out that validation of overridePreferenceName in getValue function should be done in place. Codes are updated. Does it look better now?

[akosyakov]

@a1994846931931 it looks good, i also tested with editor preferences and could not break it

Could you have a look at broken tests please?

[a1994846931931]

@a1994846931931 it looks good, i also tested with editor preferences and could not break it

Could you have a look at broken tests please?

@akosyakov Codes updated. both mockPreference.validate and mockPreference.get should return true now, so that the this.preferences['editor.enablePreview'] in privew-manager could actually return true.

[akosyakov]

@a1994846931931 thank you!

[akosyakov]

@a1994846931931 Are you already the official Eclipse committer or you need help with merging?

[a1994846931931]

@a1994846931931 Are you already the official Eclipse committer or you need help with merging?

@akosyakov Oh, sorry, I just found out that I missed an e-mail from eclipse. I'll check it out soon. Thanks for reminding me of that!

[a1994846931931]

@a1994846931931 Are you already the official Eclipse committer or you need help with merging?

@akosyakov Hi, I am still working on signing the document, but I think I need some help on filling ECLIPSE FOUNDATION, INC. INDIVIDUAL COMMITTER AGREEMENT.

Could you please tell me that, in a Delaware not-for-profit corporation, and __________________________ (“Committer”) an individual listed in the Eclipse Foundation’s, what should I put here? Should I write my name "Xuye Cai", or my committer ID xcaick5?

Or, can you send me an example, through e-mail a1994846931931@gmail.com, if possible? It would be very helpful.

Thank you very much!

[akosyakov]

@theia-ide/ip-help Could someone help with #6090 (comment) please?

[a1994846931931]

@akosyakov Hello, Kosyakov. My committer paperwork was declined because I wrongly put my username as "committer" in the document. I was unable to resign the document because it's already closed in "HelloSign" and I cannot create a new document because when I clicked on the "commit" button, it shows "There was an error creating the Hellosign Document. Please contact webmaster@eclipse.org for further instructions.". Clearly I was blocked in signing the document. I sent several emails to "webmaster@eclipse.org“ a week ago asking for help, but no response was received. And I just sent another one to "cydnie.smith@eclipse-foundation.org" (Cydnie Smith is the one who declined the document and asked me to resign it) this morning. I was still waiting for the reply, but I think I should inform you of what's happening in case I still need a hand from the theia community.

[akosyakov]

@a1994846931931 I think they should come back to you eventually. I will add it to tomorrow dev meeting, maybe someone has a better idea how to handle it. cc @marcdumais-work

[marcdumais-work]

@akosyakov I think the correct steps have been made on our side (thanks @a1994846931931) ; the Foundation has limited resources so we need to be patient.

If this is not resolved in a couple of weeks, we can try to open a bugzilla.

[a1994846931931]

@akosyakov @marcdumais-work Hi, Cydnie Smith eventually contacted me and re-opened a document. Thank you very much!