Skip to content

Commit

Permalink
only apply editor settings that changed (for #21487)
Browse files Browse the repository at this point in the history
  • Loading branch information
bpasero committed Feb 28, 2017
1 parent a74e4fe commit 67924de
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 5 deletions.
30 changes: 30 additions & 0 deletions src/vs/base/common/objects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -291,4 +291,34 @@ export function safeStringify(obj: any): string {
export function getOrDefault<T, R>(obj: T, fn: (obj: T) => R, defaultValue: R = null): R {
const result = fn(obj);
return typeof result === 'undefined' ? defaultValue : result;
}

/**
* Returns an object that has keys for each value that is different in the base object. Keys
* that do not exist in the target but in the base object are not considered.
*
* Note: This is not a deep-diffing method, so the values are strictly taken into the resulting
* object if they differ.
*
* @param base the object to diff against
* @param obj the object to use for diffing
*/
export function distinct<T>(base: object, target: object): object {
const result = Object.create(null);

if (!base || !target) {
return result;
}

const targetKeys = Object.keys(target);
targetKeys.forEach(k => {
const baseValue = base[k];
const targetValue = target[k];

if (!equals(baseValue, targetValue)) {
result[k] = targetValue;
}
});

return result;
}
77 changes: 77 additions & 0 deletions src/vs/base/test/common/objects.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,81 @@ suite('Objects', () => {
someValue = 4;
assert.strictEqual(child.getter, 4);
});

test('distinct', function () {
let base = {
one: 'one',
two: 2,
three: {
3: true
},
four: false
};

let diff = objects.distinct(base, base);
assert.deepEqual(diff, {});

let obj = {};

diff = objects.distinct(base, obj);
assert.deepEqual(diff, {});

obj = {
one: 'one',
two: 2
};

diff = objects.distinct(base, obj);
assert.deepEqual(diff, {});

obj = {
three: {
3: true
},
four: false
};

diff = objects.distinct(base, obj);
assert.deepEqual(diff, {});

obj = {
one: 'two',
two: 2,
three: {
3: true
},
four: true
};

diff = objects.distinct(base, obj);
assert.deepEqual(diff, {
one: 'two',
four: true
});

obj = {
one: null,
two: 2,
three: {
3: true
},
four: void 0
};

diff = objects.distinct(base, obj);
assert.deepEqual(diff, {
one: null,
four: void 0
});

obj = {
one: 'two',
two: 3,
three: { 3: false },
four: true
};

diff = objects.distinct(base, obj);
assert.deepEqual(diff, obj);
});
});
15 changes: 10 additions & 5 deletions src/vs/workbench/browser/parts/editor/textEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,12 +302,17 @@ export abstract class BaseTextEditor extends BaseEditor {

const editorConfiguration = this.computeConfiguration(configuration);

// Apply to control if it changed. We do not want to call updateOptions() with the same options
// because it could be that from some other place editor options where changed dynamically (e.g.
// word wrapping) and configurations can change as a matter of many things, not all editor related
if (!this.lastAppliedEditorOptions || !objects.equals(editorConfiguration, this.lastAppliedEditorOptions)) {
// Try to figure out the actual editor options that changed from the last time we updated the editor.
// We do this so that we are not overwriting some dynamic editor settings (e.g. word wrap) that might
// have been applied to the editor directly.
let editorSettingsToApply = editorConfiguration;
if (this.lastAppliedEditorOptions) {
editorSettingsToApply = objects.distinct(this.lastAppliedEditorOptions, editorSettingsToApply);
}

if (Object.keys(editorSettingsToApply).length > 0) {
this.lastAppliedEditorOptions = editorConfiguration;
this.editorControl.updateOptions(editorConfiguration);
this.editorControl.updateOptions(editorSettingsToApply);

This comment has been minimized.

Copy link
@xconverge

xconverge Mar 27, 2017

Contributor

@bpasero now that we have this, is it possible to extract only what changed from the onDidConfigurationChanged event that extensions can use?

This comment has been minimized.

Copy link
@bpasero

bpasero Mar 27, 2017

Author Member

if an extension needed the real diff, could they not do this already by keeping the old settings object around?

This comment has been minimized.

Copy link
@xconverge

xconverge Mar 28, 2017

Contributor

I could do that, my concern is performance. The configuration event is triggered a lot for the Vim plugin (everytime we change the cursor type). I will need to iterate on every single setting every time the cursor changes just to know that only the cursor changed and I don't actually care about this setting update.

This comment has been minimized.

Copy link
@bpasero

bpasero Mar 28, 2017

Author Member

Sounds like a valid feature request (PR welcome).

This comment has been minimized.

Copy link
@xconverge

xconverge Mar 28, 2017

Contributor

Will do, thanks for the side convo!

}
}

Expand Down

0 comments on commit 67924de

Please sign in to comment.