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.

Sign up for GitHub

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

Jump to bottom

Allow extenders to modify preferences #8626

Closed
wants to merge 1 commit into from
Closed

Allow extenders to modify preferences #8626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
@@ -1,5 +1,9 @@
# Change Log

## v1.11.0

- [preferences] preferences schemas can now be modified or completely removed from the UI by downstream packages [#8626](https://github.com/eclipse-theia/theia/pull/8626)

## v1.10.0 - 1/28/2021

- [api-samples] added example on how to contribute toggleable toolbar items [#8968](https://github.com/eclipse-theia/theia/pull/8968)
Expand Down
95 changes: 88 additions & 7 deletions packages/core/src/browser/preferences/preference-contribution.ts
View file
Open in desktop
Expand Up @@ -20,12 +20,16 @@ import { ContributionProvider, bindContributionProvider, escapeRegExpCharacters,
import { PreferenceScope } from './preference-scope';
import { PreferenceProvider, PreferenceProviderDataChange } from './preference-provider';
import {
PreferenceSchema, PreferenceSchemaProperties, PreferenceDataSchema, PreferenceItem, PreferenceSchemaProperty, PreferenceDataProperty, JsonType
PreferenceSchema, PreferenceSchemaProperties, PreferenceDataSchema, PreferenceItem, PreferenceSchemaProperty, PreferenceDataProperty, JsonType,
PreferenceSchemaModification, PreferenceSchemaPropertyModifications, PreferenceSchemaPropertyModification
} from '../../common/preferences/preference-schema';
import { FrontendApplicationConfigProvider } from '../frontend-application-config-provider';
import { FrontendApplicationConfig } from '@theia/application-package/lib/application-props';
import { bindPreferenceConfigurations, PreferenceConfigurations } from './preference-configurations';
export { PreferenceSchema, PreferenceSchemaProperties, PreferenceDataSchema, PreferenceItem, PreferenceSchemaProperty, PreferenceDataProperty, JsonType };
export {
PreferenceSchema, PreferenceSchemaProperties, PreferenceDataSchema, PreferenceItem, PreferenceSchemaProperty, PreferenceDataProperty, JsonType,
PreferenceSchemaModification
};
import { Mutable } from '../../common/types';

/* eslint-disable guard-for-in, @typescript-eslint/no-explicit-any */
Expand Down Expand Up @@ -60,10 +64,16 @@ export interface PreferenceContribution {
readonly schema: PreferenceSchema;
}

export const PreferenceModificationContribution = Symbol('PreferenceModificationContribution');
export interface PreferenceModificationContribution {
readonly schemaModification: PreferenceSchemaModification;
}

export function bindPreferenceSchemaProvider(bind: interfaces.Bind): void {
bindPreferenceConfigurations(bind);
bind(PreferenceSchemaProvider).toSelf().inSingletonScope();
bindContributionProvider(bind, PreferenceContribution);
bindContributionProvider(bind, PreferenceModificationContribution);
}

export interface OverridePreferenceName {
Expand Down Expand Up @@ -108,10 +118,14 @@ export class PreferenceSchemaProvider extends PreferenceProvider {
protected readonly combinedSchema: PreferenceDataSchema = { properties: {}, patternProperties: {} };
protected readonly workspaceSchema: PreferenceDataSchema = { properties: {}, patternProperties: {} };
protected readonly folderSchema: PreferenceDataSchema = { properties: {}, patternProperties: {} };
protected readonly modifications: PreferenceSchemaPropertyModifications = {};

@inject(ContributionProvider) @named(PreferenceContribution)
protected readonly preferenceContributions: ContributionProvider<PreferenceContribution>;

@inject(ContributionProvider) @named(PreferenceModificationContribution)
protected readonly preferenceModificationContributions: ContributionProvider<PreferenceModificationContribution>;

@inject(PreferenceConfigurations)
protected readonly configurations: PreferenceConfigurations;

Expand All @@ -123,6 +137,9 @@ export class PreferenceSchemaProvider extends PreferenceProvider {

@postConstruct()
protected init(): void {
this.preferenceModificationContributions.getContributions().forEach(contrib => {
this.doSetSchemaModification(contrib.schemaModification);
});
this.preferenceContributions.getContributions().forEach(contrib => {
this.doSetSchema(contrib.schema);
});
Expand Down Expand Up @@ -233,7 +250,8 @@ export class PreferenceSchemaProvider extends PreferenceProvider {
}
this.updateSchemaProps(preferenceName, schemaProps);

const value = schemaProps.defaultValue = this.getDefaultValue(schemaProps, preferenceName);
const modifiedProperties = this.schema(preferenceName, schemaProps);
const value = schemaProps.defaultValue = this.getDefaultValue(modifiedProperties, preferenceName);
if (this.testOverrideValue(preferenceName, value)) {
for (const overriddenPreferenceName in value) {
const overrideValue = value[overriddenPreferenceName];
Expand Down Expand Up @@ -289,8 +307,66 @@ export class PreferenceSchemaProvider extends PreferenceProvider {
return null;
}

protected doSetSchemaModification(schemaModification: PreferenceSchemaModification): void {
for (const preferenceName of Object.keys(schemaModification.properties)) {
const modifiableProperties = this.combinedSchema.properties[preferenceName];
const previousModifications = this.modifications[preferenceName] ?? {};
const newModifications: PreferenceSchemaPropertyModification = schemaModification.properties[preferenceName];
this.modifications[preferenceName] = { ...previousModifications, ...newModifications };

// Validate that the modifications only constrain the schema, cannot allow extra values
const modifiedEnum = newModifications.enum;
if (modifiedEnum !== undefined) {
if (modifiableProperties) {
if (modifiableProperties.type !== 'string') {
console.warn(`Override of preference ${preferenceName} cannot constrain to enum values because the property is not string type.`);
continue;
}
if (modifiableProperties.enum && modifiableProperties.enum.some(v => !modifiedEnum.includes(v))) {
console.warn(`Override of preference enum ${preferenceName} cannot add enum values, it can only constrain the set of values.`);
continue;
}
}
}
const modifiedMinimum = newModifications.minimum;
if (modifiedMinimum !== undefined) {
if (modifiableProperties) {
if (modifiableProperties.minimum && modifiedMinimum < modifiableProperties.minimum) {
console.warn(`Override of preference minimum ${preferenceName} cannot reduce the minimum, it can only increase it.`);
continue;
}
}
}
}
}

protected schema(preferenceName: string, propertySchema: PreferenceDataProperty): PreferenceDataProperty {
const modifications = this.modifications[preferenceName];
return modifications ? { ...propertySchema, ...modifications } : propertySchema;
}

getCombinedSchema(): PreferenceDataSchema {
return this.combinedSchema;
const properties: { [key: string]: PreferenceDataProperty; } = {};
for (const preferenceName of Object.keys(this.combinedSchema.properties)) {
const value = this.combinedSchema.properties[preferenceName];
const modifications = this.modifications[preferenceName];
if (modifications) {
if (!modifications.hidden) {
const modifiedValue = this.schema(preferenceName, value);
properties[preferenceName] = modifiedValue;
}
} else {
properties[preferenceName] = value;
}
}
return { ...this.combinedSchema, properties };
}

getPropertySchema(preferenceName: string): PreferenceDataProperty | undefined {
const property = this.combinedSchema.properties[preferenceName];
if (property) {
return this.schema(preferenceName, property);
}
}

getSchema(scope: PreferenceScope): PreferenceDataSchema {
Expand Down Expand Up @@ -343,10 +419,10 @@ export class PreferenceSchemaProvider extends PreferenceProvider {
}
if (!property) {
// try from overridden value
property = this.combinedSchema.properties[overridden.preferenceName];
property = this.getPropertySchema(overridden.preferenceName);
}
} else {
property = this.combinedSchema.properties[preferenceName];
property = this.getPropertySchema(preferenceName);
}
return property && property.scope! >= scope;
}
Expand All @@ -361,7 +437,7 @@ export class PreferenceSchemaProvider extends PreferenceProvider {
}

*getOverridePreferenceNames(preferenceName: string): IterableIterator<string> {
const preference = this.combinedSchema.properties[preferenceName];
const preference = this.getPropertySchema(preferenceName);
if (preference && preference.overridable) {
for (const overrideIdentifier of this.overrideIdentifiers) {
yield this.overridePreferenceName({ preferenceName, overrideIdentifier });
Expand Down Expand Up @@ -416,4 +492,9 @@ export class PreferenceSchemaProvider extends PreferenceProvider {
break;
}
}

isPropertyHidden(preferenceName: string): boolean {
const modifications = this.modifications[preferenceName];
return modifications && !!modifications.hidden;
}
}
23 changes: 15 additions & 8 deletions packages/core/src/browser/preferences/preference-service.ts
View file
Open in desktop
Expand Up @@ -494,14 +494,21 @@ export class PreferenceServiceImpl implements PreferenceService {
}
protected doResolve<T>(preferenceName: string, defaultValue?: T, resourceUri?: string): PreferenceResolveResult<T> {
const result: PreferenceResolveResult<T> = {};
for (const scope of PreferenceScope.getScopes()) {
if (this.schema.isValidInScope(preferenceName, scope)) {
const provider = this.getProvider(scope);
if (provider) {
const { configUri, value } = provider.resolve<T>(preferenceName, resourceUri);
if (value !== undefined) {
result.configUri = configUri;
result.value = PreferenceProvider.merge(result.value as any, value as any) as any;
const propertyIsHidden = this.schema.isPropertyHidden(preferenceName);
if (propertyIsHidden) {
const { configUri, value } = this.schema.resolve<T>(preferenceName, resourceUri);
result.configUri = configUri;
result.value = value;
} else {
for (const scope of PreferenceScope.getScopes()) {
if (this.schema.isValidInScope(preferenceName, scope)) {
const provider = this.getProvider(scope);
if (provider) {
const { configUri, value } = provider.resolve<T>(preferenceName, resourceUri);
if (value !== undefined) {
result.configUri = configUri;
result.value = PreferenceProvider.merge(result.value as any, value as any) as any;
}
}
}
}
Expand Down
17 changes: 17 additions & 0 deletions packages/core/src/common/preferences/preference-schema.ts
View file
Open in desktop
Expand Up @@ -76,6 +76,7 @@ export interface PreferenceItem {
additionalProperties?: object | boolean;
[name: string]: any;
overridable?: boolean;
hidden?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westbury the property is used to hide elements in the view but it still allows users to add the preference in their settings (with or without the use of auto-completion). Is there a reason you want to hide the preference rather than make it non-editable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you want to hide the preference rather than make it non-editable?

Consider a custom IDE that is targeted to a very specific set of users using a specific tool chain for a specific purpose. There will be preferences that must be set to a particular value and setting the preference to any other value will always break the user. For example, a couple of days ago, after I had put in this PR, we had a bug report from a user. They had set on the 'Support Multi Root Workspace' preference. Our IDE works to a specific directory layout and does not support multi-root workspaces. So, in this situation, should we show the preference but make it non-editable, or not show it? I would suggest that not showing it is a better user experience. Mentioning 'multi-root workspaces' in a custom IDE that has no concept of multi-root workspaces will only cause confusion. If users know what multi-root workspaces are then they will think they must be supported and send in support requests complaining that they can't get them to work. A large part of providing a clean IDE is that we do not expose stuff that does not apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should be removed from auto-complete in settings too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westbury I'm not arguing the use-case, in fact I believe it is something I brought up in the past: https://spectrum.chat/theia/general/is-there-a-need-to-set-specific-preferences-as-non-overridable~2cf3bb68-59b2-4f4b-8070-b2839c35ba4d.

The issue with the approach is while the preference is hidden, the preference is still editable which can still break applications (for instance if they do not expect to support multi-root workspaces).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how we can stop the user from editing the settings file. If the requirement is that we do not want to give the user any way of changing the preference (short of hacking javascript) then we would have to ignore any value in the settings file, always using the default value. That would provide a consistent approach but it does remove the ability to change the preference value programatically.

The current implementation leaves the hidden property in the schema. The property would be better hidden if it were not in the schema, as it would not then appear in auto-complete. A disadvantage of removing hidden properties from the schema is that if the preference has been set programatically then the user will see it with squiggly underlining.

If we decide it is okay that hidden properties cannot be changed programatically (we have no use case) then it should be removed from the schema.

}

export interface PreferenceSchemaProperty extends PreferenceItem {
Expand All @@ -100,4 +101,20 @@ export namespace PreferenceDataProperty {
}
}

export interface PreferenceSchemaModification {
properties: PreferenceSchemaPropertyModifications
}

export interface PreferenceSchemaPropertyModifications {
[name: string]: PreferenceSchemaPropertyModification
}

export interface PreferenceSchemaPropertyModification {
minimum?: number;
default?: any;
enum?: string[];
description?: string;
hidden?: boolean;
}

export type JsonType = 'string' | 'array' | 'number' | 'integer' | 'object' | 'boolean' | 'null';
2 changes: 1 addition & 1 deletion packages/preferences/src/browser/abstract-resource-preference-provider.ts
View file
Open in desktop
Expand Up @@ -229,7 +229,7 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
for (const prefName of prefNames.values()) {
const oldValue = oldPrefs[prefName];
const newValue = newPrefs[prefName];
const schemaProperties = this.schemaProvider.getCombinedSchema().properties[prefName];
const schemaProperties = this.schemaProvider.getPropertySchema(prefName);
if (schemaProperties) {
const scope = schemaProperties.scope;
// do not emit the change event if the change is made out of the defined preference scope
Expand Down