Skip to content

Commit

Permalink
[preferences] defer change events in the same tick
Browse files Browse the repository at this point in the history 
Signed-off-by: Anton Kosyakov <[email protected]>
  • Loading branch information
@akosyakov
akosyakov committed Apr 30, 2020
1 parent c9eabc8 commit 9286ffe
Show file tree
Hide file tree
Showing 6 changed files with 123 additions and 71 deletions.
2 changes: 2 additions & 0 deletions packages/core/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"@types/fs-extra": "^4.0.2",
"@types/lodash.debounce": "4.0.3",
"@types/lodash.throttle": "^4.1.3",
"@types/p-debounce": "^1.0.1",

This comment has been minimized.

Sign in to view

Copy link
@kittaakos

kittaakos Apr 30, 2020

Contributor

It seems, p-debounce has typings in it: node_modules/p-debounce/index.d.ts. It makes @types/p-debounce redundant.

Also, when you consume Theia from a downstream, you can see the warning when running yarn:

theia/core > @types/[email protected]: This is a stub types definition. p-debounce provides its own type definitions, so you do not need this installed.
"@types/react": "^16.4.1",
"@types/react-dom": "^16.0.6",
"@types/react-virtualized": "^9.18.3",
Expand All @@ -34,6 +35,7 @@
"lodash.debounce": "^4.0.8",
"lodash.throttle": "^4.1.1",
"nsfw": "^1.2.9",
"p-debounce": "^2.1.0",
"perfect-scrollbar": "^1.3.0",
"react": "^16.4.1",
"react-dom": "^16.4.1",
Expand Down
52 changes: 47 additions & 5 deletions packages/core/src/browser/preferences/preference-provider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

/* eslint-disable @typescript-eslint/no-explicit-any */

import debounce = require('p-debounce');
import { injectable } from 'inversify';
import { JSONExt, JSONValue } from '@phosphor/coreutils/lib/json';
import URI from '../../common/uri';
Expand Down Expand Up @@ -58,22 +59,57 @@ export abstract class PreferenceProvider implements Disposable {
this.toDispose.dispose();
}

protected deferredChanges: PreferenceProviderDataChanges | undefined;
protected _pendingChanges: Promise<boolean> = Promise.resolve(false);
get pendingChanges(): Promise<boolean> {
return this._pendingChanges;
}

/**
* Informs the listeners that one or more preferences of this provider are changed.
* The listeners are able to find what was changed from the emitted event.
*/
protected emitPreferencesChangedEvent(changes: PreferenceProviderDataChanges | PreferenceProviderDataChange[]): void {
protected emitPreferencesChangedEvent(changes: PreferenceProviderDataChanges | PreferenceProviderDataChange[]): Promise<boolean> {
if (Array.isArray(changes)) {
const prefChanges: PreferenceProviderDataChanges = {};
for (const change of changes) {
prefChanges[change.preferenceName] = change;
this.mergePreferenceProviderDataChange(change);
}
this.onDidPreferencesChangedEmitter.fire(prefChanges);
} else {
this.onDidPreferencesChangedEmitter.fire(changes);
for (const preferenceName of Object.keys(changes)) {
this.mergePreferenceProviderDataChange(changes[preferenceName]);
}
}
return this._pendingChanges = this.fireDidPreferencesChanged();
}

protected mergePreferenceProviderDataChange(change: PreferenceProviderDataChange): void {
if (!this.deferredChanges) {
this.deferredChanges = {};
}
const current = this.deferredChanges[change.preferenceName];
const { newValue, scope, domain } = change;
if (!current) {
// new
this.deferredChanges[change.preferenceName] = change;
} else if (current.oldValue === newValue) {
// delete
delete this.deferredChanges[change.preferenceName];
} else {
// update
Object.assign(current, { newValue, scope, domain });
}
}

protected fireDidPreferencesChanged = debounce(() => {
const changes = this.deferredChanges;
this.deferredChanges = undefined;
if (changes && Object.keys(changes).length) {
this.onDidPreferencesChangedEmitter.fire(changes);
return true;
}
return false;
}, 0);

get<T>(preferenceName: string, resourceUri?: string): T | undefined {
return this.resolve<T>(preferenceName, resourceUri).value;
}
Expand All @@ -91,6 +127,12 @@ export abstract class PreferenceProvider implements Disposable {

abstract getPreferences(resourceUri?: string): { [p: string]: any };

/**
* Resolves only if all changes were delivered.
* If changes were made then implementation must either
* await on `this.emitPreferencesChangedEvent(...)` or
* `this.pendingChanges` if chnages are fired indirectly.
*/
abstract setPreference(key: string, value: any, resourceUri?: string): Promise<boolean>;

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/browser/preferences/preference-proxy.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ describe('Preference Proxy', () => {
expect(Object.keys(proxy).join()).to.equal(['my', 'my.pref'].join());
});

it('it should forward change events', () => {
it('it should forward change events', async () => {
const proxy = getProxy(undefined, { style: 'both' });
let theChange: PreferenceChangeEvent<{ [key: string]: any }>;
proxy.onPreferenceChanged(change => {
Expand All @@ -134,7 +134,7 @@ describe('Preference Proxy', () => {
theSecondChange = change;
});

getProvider(PreferenceScope.User).setPreference('my.pref', 'bar');
await getProvider(PreferenceScope.User).setPreference('my.pref', 'bar');

expect(theChange!.newValue).to.equal('bar');
expect(theChange!.oldValue).to.equal(undefined);
Expand Down
124 changes: 69 additions & 55 deletions packages/core/src/browser/preferences/preference-service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import * as assert from 'assert';
import { Container } from 'inversify';
import { bindPreferenceService } from '../frontend-application-bindings';
import { bindMockPreferenceProviders, MockPreferenceProvider } from './test';
import { PreferenceService, PreferenceServiceImpl, PreferenceChange } from './preference-service';
import { PreferenceService, PreferenceServiceImpl, PreferenceChange, PreferenceChanges } from './preference-service';
import { PreferenceSchemaProvider, PreferenceSchema } from './preference-contribution';
import { PreferenceScope } from './preference-scope';
import { PreferenceProvider } from './preference-provider';
Expand Down Expand Up @@ -87,38 +87,6 @@ describe('Preference Service', () => {
return testContainer.getNamed(PreferenceProvider, scope) as MockPreferenceProvider;
}

it('should get notified if a provider emits a change', done => {
prefSchema.setSchema({
properties: {
'testPref': {
type: 'string'
}
}
});
const userProvider = getProvider(PreferenceScope.User);
userProvider.setPreference('testPref', 'oldVal');
prefService.onPreferenceChanged(pref => {
if (pref) {
expect(pref.preferenceName).eq('testPref');
expect(pref.newValue).eq('newVal');
done();
return;
}
done(new Error('onPreferenceChanged() fails to return any preference change information'));
return;
});

userProvider.emitPreferencesChangedEvent({
testPref: {
preferenceName: 'testPref',
newValue: 'newVal',
oldValue: 'oldVal',
scope: PreferenceScope.User,
domain: []
}
});
}).timeout(2000);

it('should return the preference from the more specific scope (user > workspace)', () => {
prefSchema.setSchema({
properties: {
Expand Down Expand Up @@ -185,12 +153,56 @@ describe('Preference Service', () => {
expect(prefService.get('test.number')).equals(0);
});

it('should unset preference schema', () => {
it('should not fire events if preference schema is unset in the same tick ', async () => {
const events: PreferenceChange[] = [];
prefService.onPreferenceChanged(event => events.push(event));
prefSchema.registerOverrideIdentifier('go');

const toUnset = prefSchema.setSchema({
properties: {
'editor.insertSpaces': {
type: 'boolean',
default: true,
overridable: true
},
'[go]': {
type: 'object',
default: {
'editor.insertSpaces': false
}
}
}
});
assert.deepStrictEqual([], events.map(e => ({
preferenceName: e.preferenceName,
newValue: e.newValue,
oldValue: e.oldValue
})), 'events after set in the same tick');
assert.strictEqual(prefService.get('editor.insertSpaces'), true, 'get before');
assert.strictEqual(prefService.get('[go].editor.insertSpaces'), false, 'get before overridden');

toUnset.dispose();

assert.deepStrictEqual([], events.map(e => ({
preferenceName: e.preferenceName,
newValue: e.newValue,
oldValue: e.oldValue
})), 'events after unset in the same tick');
assert.strictEqual(prefService.get('editor.insertSpaces'), undefined, 'get after');
assert.strictEqual(prefService.get('[go].editor.insertSpaces'), undefined, 'get after overridden');

assert.strictEqual(await prefSchema.pendingChanges, false);
assert.deepStrictEqual([], events.map(e => ({
preferenceName: e.preferenceName,
newValue: e.newValue,
oldValue: e.oldValue
})), 'events in next tick');
});

it('should fire events if preference schema is unset in another tick', async () => {
prefSchema.registerOverrideIdentifier('go');

let pending = new Promise<PreferenceChanges>(resolve => prefService.onPreferencesChanged(resolve));
const toUnset = prefSchema.setSchema({
properties: {
'editor.insertSpaces': {
Expand All @@ -206,6 +218,7 @@ describe('Preference Service', () => {
}
}
});
let changes = await pending;

assert.deepStrictEqual([{
preferenceName: 'editor.insertSpaces',
Expand All @@ -215,16 +228,16 @@ describe('Preference Service', () => {
preferenceName: '[go].editor.insertSpaces',
newValue: false,
oldValue: undefined
}], events.map(e => ({
preferenceName: e.preferenceName,
newValue: e.newValue,
oldValue: e.oldValue
})), 'events before');
}], Object.keys(changes).map(key => {
const { preferenceName, newValue, oldValue } = changes[key];
return { preferenceName, newValue, oldValue };
}), 'events before');
assert.strictEqual(prefService.get('editor.insertSpaces'), true, 'get before');
assert.strictEqual(prefService.get('[go].editor.insertSpaces'), false, 'get before overridden');

events.length = 0;
pending = new Promise<PreferenceChanges>(resolve => prefService.onPreferencesChanged(resolve));
toUnset.dispose();
changes = await pending;

assert.deepStrictEqual([{
preferenceName: 'editor.insertSpaces',
Expand All @@ -234,11 +247,10 @@ describe('Preference Service', () => {
preferenceName: '[go].editor.insertSpaces',
newValue: undefined,
oldValue: false
}], events.map(e => ({
preferenceName: e.preferenceName,
newValue: e.newValue,
oldValue: e.oldValue
})), 'events after');
}], Object.keys(changes).map(key => {
const { preferenceName, newValue, oldValue } = changes[key];
return { preferenceName, newValue, oldValue };
}), 'events after');
assert.strictEqual(prefService.get('editor.insertSpaces'), undefined, 'get after');
assert.strictEqual(prefService.get('[go].editor.insertSpaces'), undefined, 'get after overridden');
});
Expand Down Expand Up @@ -367,15 +379,16 @@ describe('Preference Service', () => {
}, preferences.inspect('[json].editor.tabSize'));
});

it('onPreferenceChanged #0', () => {
it('onPreferenceChanged #0', async () => {
const { preferences, schema } = prepareServices();
await schema.pendingChanges;

const events: PreferenceChange[] = [];
preferences.onPreferenceChanged(event => events.push(event));

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

assert.deepStrictEqual([{
preferenceName: '[json].editor.tabSize',
Expand All @@ -389,14 +402,15 @@ describe('Preference Service', () => {
})));
});

it('onPreferenceChanged #1', () => {
it('onPreferenceChanged #1', async () => {
const { preferences, schema } = prepareServices();
await schema.pendingChanges;

const events: PreferenceChange[] = [];
preferences.onPreferenceChanged(event => events.push(event));

schema.registerOverrideIdentifier('json');
preferences.set('editor.tabSize', 2, PreferenceScope.User);
await preferences.set('editor.tabSize', 2, PreferenceScope.User);

assert.deepStrictEqual([{
preferenceName: 'editor.tabSize',
Expand All @@ -410,37 +424,37 @@ describe('Preference Service', () => {
})));
});

it('onPreferenceChanged #2', () => {
it('onPreferenceChanged #2', async function (): Promise<void> {
const { preferences, schema } = prepareServices();

schema.registerOverrideIdentifier('json');
schema.registerOverrideIdentifier('javascript');
preferences.set('[json].editor.tabSize', 2, PreferenceScope.User);
preferences.set('editor.tabSize', 3, PreferenceScope.User);
await preferences.set('editor.tabSize', 3, PreferenceScope.User);

const events: PreferenceChangeEvent<{ [key: string]: any }>[] = [];
const proxy = createPreferenceProxy<{ [key: string]: any }>(preferences, schema.getCombinedSchema(), { overrideIdentifier: 'json' });
proxy.onPreferenceChanged(event => events.push(event));

preferences.set('[javascript].editor.tabSize', 4, PreferenceScope.User);
await preferences.set('[javascript].editor.tabSize', 4, PreferenceScope.User);

assert.deepStrictEqual([], events.map(e => ({
preferenceName: e.preferenceName,
newValue: e.newValue
})), 'changes not relevant to json override should be ignored');
});

it('onPreferenceChanged #3', () => {
it('onPreferenceChanged #3', async () => {
const { preferences, schema } = prepareServices();

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

const events: PreferenceChange[] = [];
preferences.onPreferenceChanged(event => events.push(event));

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

assert.deepStrictEqual([{
preferenceName: '[json].editor.tabSize',
Expand Down
10 changes: 2 additions & 8 deletions packages/core/src/browser/preferences/test/mock-preference-provider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import { interfaces } from 'inversify';
import { PreferenceProvider } from '../';
import { PreferenceScope } from '../preference-scope';
import { PreferenceProviderDataChanges, PreferenceProviderDataChange } from '../preference-provider';

export class MockPreferenceProvider extends PreferenceProvider {
readonly prefs: { [p: string]: any } = {};
Expand All @@ -28,22 +27,17 @@ export class MockPreferenceProvider extends PreferenceProvider {
super();
}

public emitPreferencesChangedEvent(changes: PreferenceProviderDataChanges | PreferenceProviderDataChange[]): void {
super.emitPreferencesChangedEvent(changes);
}

public markReady(): void {
this._ready.resolve();
}

getPreferences(): { [p: string]: any } {
return this.prefs;
}
async setPreference(preferenceName: string, newValue: any, resourceUri?: string): Promise<boolean> {
setPreference(preferenceName: string, newValue: any, resourceUri?: string): Promise<boolean> {
const oldValue = this.prefs[preferenceName];
this.prefs[preferenceName] = newValue;
this.emitPreferencesChangedEvent([{ preferenceName, oldValue, newValue, scope: this.scope, domain: [] }]);
return true;
return this.emitPreferencesChangedEvent([{ preferenceName, oldValue, newValue, scope: this.scope, domain: [] }]);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/preferences/src/browser/abstract-resource-preference-provider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
});
}
await this.workspace.applyBackgroundEdit(this.model, editOperations);
return true;
return await this.pendingChanges;
} catch (e) {
const message = `Failed to update the value of '${key}' in '${this.getUri()}'.`;
this.messageService.error(`${message} Please check if it is corrupted.`);
Expand Down

0 comments on commit 9286ffe

Please sign in to comment.