Skip to content

Commit 2e55856

Browse files
bpaserobpasero
authored
storage - scoped onDidChangeValue and adoption (microsoft#188930)
* storage - scoped `onDidChangeValue` and adoption * adopt disposables * 💄 * add tests
1 parent b3ebdfa commit 2e55856

File tree

38 files changed

+252
-238
lines changed

38 files changed

+252
-238
lines changed

‎src/vs/editor/contrib/find/test/browser/findController.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ suite('FindController', async () => {
7070
serviceCollection.set(IStorageService, {
7171
_serviceBrand: undefined,
7272
onDidChangeTarget: Event.None,
73-
onDidChangeValue: Event.None,
73+
onDidChangeValue: () => Event.None,
7474
onWillSaveState: Event.None,
7575
get: (key: string) => queryState[key],
7676
getBoolean: (key: string) => !!queryState[key],
@@ -504,7 +504,7 @@ suite('FindController query options persistence', async () => {
504504
serviceCollection.set(IStorageService, {
505505
_serviceBrand: undefined,
506506
onDidChangeTarget: Event.None,
507-
onDidChangeValue: Event.None,
507+
onDidChangeValue: () => Event.None,
508508
onWillSaveState: Event.None,
509509
get: (key: string) => queryState[key],
510510
getBoolean: (key: string) => !!queryState[key],

‎src/vs/editor/contrib/multicursor/test/browser/multicursor.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,8 @@ suite('Multicursor selection', () => {
8686
const serviceCollection = new ServiceCollection();
8787
serviceCollection.set(IStorageService, {
8888
_serviceBrand: undefined,
89-
onDidChangeValue: Event.None,
89+
onDidChangeValue: () => { throw new Error(); },
90+
onDidChangeValue2: Event.None,
9091
onDidChangeTarget: Event.None,
9192
onWillSaveState: Event.None,
9293
get: (key: string) => queryState[key],

‎src/vs/platform/actions/common/menuService.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,7 @@ class PersistedMenuHideState {
5858
this._data = Object.create(null);
5959
}
6060

61-
this._disposables.add(_storageService.onDidChangeValue(e => {
62-
if (e.key !== PersistedMenuHideState._key) {
63-
return;
64-
}
61+
this._disposables.add(_storageService.onDidChangeValue(StorageScope.PROFILE, PersistedMenuHideState._key, this._disposables)(() => {
6562
if (!this._ignoreChangeEvent) {
6663
try {
6764
const raw = _storageService.get(PersistedMenuHideState._key, StorageScope.PROFILE, '{}');

‎src/vs/platform/extensionManagement/common/extensionEnablementService.ts

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,11 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import { Emitter, Event } from 'vs/base/common/event';
7-
import { Disposable } from 'vs/base/common/lifecycle';
7+
import { Disposable, DisposableStore } from 'vs/base/common/lifecycle';
88
import { isUndefinedOrNull } from 'vs/base/common/types';
99
import { DISABLED_EXTENSIONS_STORAGE_PATH, IExtensionIdentifier, IExtensionManagementService, IGlobalExtensionEnablementService, InstallOperation } from 'vs/platform/extensionManagement/common/extensionManagement';
1010
import { areSameExtensions } from 'vs/platform/extensionManagement/common/extensionManagementUtil';
11-
import { IStorageService, IStorageValueChangeEvent, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage';
11+
import { IProfileStorageValueChangeEvent, IStorageService, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage';
1212

1313
export class GlobalExtensionEnablementService extends Disposable implements IGlobalExtensionEnablementService {
1414

@@ -102,7 +102,7 @@ export class StorageManager extends Disposable {
102102

103103
constructor(private storageService: IStorageService) {
104104
super();
105-
this._register(storageService.onDidChangeValue(e => this.onDidStorageChange(e)));
105+
this._register(storageService.onDidChangeValue(StorageScope.PROFILE, undefined, this._register(new DisposableStore()))(e => this.onDidStorageChange(e)));
106106
}
107107

108108
get(key: string, scope: StorageScope): IExtensionIdentifier[] {
@@ -133,19 +133,17 @@ export class StorageManager extends Disposable {
133133
}
134134
}
135135

136-
private onDidStorageChange(storageChangeEvent: IStorageValueChangeEvent): void {
137-
if (storageChangeEvent.scope === StorageScope.PROFILE) {
138-
if (!isUndefinedOrNull(this.storage[storageChangeEvent.key])) {
139-
const newValue = this._get(storageChangeEvent.key, storageChangeEvent.scope);
140-
if (newValue !== this.storage[storageChangeEvent.key]) {
141-
const oldValues = this.get(storageChangeEvent.key, storageChangeEvent.scope);
142-
delete this.storage[storageChangeEvent.key];
143-
const newValues = this.get(storageChangeEvent.key, storageChangeEvent.scope);
144-
const added = oldValues.filter(oldValue => !newValues.some(newValue => areSameExtensions(oldValue, newValue)));
145-
const removed = newValues.filter(newValue => !oldValues.some(oldValue => areSameExtensions(oldValue, newValue)));
146-
if (added.length || removed.length) {
147-
this._onDidChange.fire([...added, ...removed]);
148-
}
136+
private onDidStorageChange(storageChangeEvent: IProfileStorageValueChangeEvent): void {
137+
if (!isUndefinedOrNull(this.storage[storageChangeEvent.key])) {
138+
const newValue = this._get(storageChangeEvent.key, storageChangeEvent.scope);
139+
if (newValue !== this.storage[storageChangeEvent.key]) {
140+
const oldValues = this.get(storageChangeEvent.key, storageChangeEvent.scope);
141+
delete this.storage[storageChangeEvent.key];
142+
const newValues = this.get(storageChangeEvent.key, storageChangeEvent.scope);
143+
const added = oldValues.filter(oldValue => !newValues.some(newValue => areSameExtensions(oldValue, newValue)));
144+
const removed = newValues.filter(newValue => !oldValues.some(oldValue => areSameExtensions(oldValue, newValue)));
145+
if (added.length || removed.length) {
146+
this._onDidChange.fire([...added, ...removed]);
149147
}
150148
}
151149
}

‎src/vs/platform/extensionManagement/common/extensionStorage.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@
55

66
import { createDecorator } from 'vs/platform/instantiation/common/instantiation';
77
import { Emitter, Event } from 'vs/base/common/event';
8-
import { Disposable } from 'vs/base/common/lifecycle';
9-
import { IStorageService, IStorageValueChangeEvent, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage';
8+
import { Disposable, DisposableStore } from 'vs/base/common/lifecycle';
9+
import { IProfileStorageValueChangeEvent, IStorageService, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage';
1010
import { adoptToGalleryExtensionId, areSameExtensions, getExtensionId } from 'vs/platform/extensionManagement/common/extensionManagementUtil';
1111
import { IProductService } from 'vs/platform/product/common/productService';
1212
import { distinct } from 'vs/base/common/arrays';
@@ -103,13 +103,10 @@ export class ExtensionStorageService extends Disposable implements IExtensionSto
103103
) {
104104
super();
105105
this.extensionsWithKeysForSync = ExtensionStorageService.readAllExtensionsWithKeysForSync(storageService);
106-
this._register(this.storageService.onDidChangeValue(e => this.onDidChangeStorageValue(e)));
106+
this._register(this.storageService.onDidChangeValue(StorageScope.PROFILE, undefined, this._register(new DisposableStore()))(e => this.onDidChangeStorageValue(e)));
107107
}
108108

109-
private onDidChangeStorageValue(e: IStorageValueChangeEvent): void {
110-
if (e.scope !== StorageScope.PROFILE) {
111-
return;
112-
}
109+
private onDidChangeStorageValue(e: IProfileStorageValueChangeEvent): void {
113110

114111
// State of extension with keys for sync has changed
115112
if (this.extensionsWithKeysForSync.has(e.key.toLowerCase())) {

‎src/vs/platform/secrets/common/secrets.ts

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { createDecorator } from 'vs/platform/instantiation/common/instantiation'
99
import { IStorageService, InMemoryStorageService, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage';
1010
import { Emitter, Event } from 'vs/base/common/event';
1111
import { ILogService } from 'vs/platform/log/common/log';
12-
import { IDisposable } from 'vs/base/common/lifecycle';
12+
import { DisposableStore } from 'vs/base/common/lifecycle';
1313

1414
export const ISecretStorageService = createDecorator<ISecretStorageService>('secretStorageService');
1515

@@ -38,7 +38,7 @@ export abstract class BaseSecretStorageService implements ISecretStorageService
3838

3939
private _type: 'in-memory' | 'persisted' | 'unknown' = 'unknown';
4040

41-
private _onDidChangeValueDisposable: IDisposable | undefined;
41+
private readonly _onDidChangeValueDisposable = new DisposableStore();
4242

4343
constructor(
4444
@IStorageService private _storageService: IStorageService,
@@ -127,15 +127,10 @@ export abstract class BaseSecretStorageService implements ISecretStorageService
127127
storageService = new InMemoryStorageService();
128128
}
129129

130-
this._onDidChangeValueDisposable?.dispose();
131-
this._onDidChangeValueDisposable = storageService.onDidChangeValue(e => {
132-
// We only care about changes to the application scope since SecretStorage
133-
// only stores secrets in the application scope but this seems to fire
134-
// 2 events. Once for APP scope and once for PROFILE scope. ref #188460
135-
if (e.scope === StorageScope.APPLICATION) {
136-
this.onDidChangeValue(e.key);
137-
}
138-
});
130+
this._onDidChangeValueDisposable.clear();
131+
this._onDidChangeValueDisposable.add(storageService.onDidChangeValue(StorageScope.APPLICATION, undefined, this._onDidChangeValueDisposable)(e => {
132+
this.onDidChangeValue(e.key);
133+
}));
139134
return storageService;
140135
}
141136

‎src/vs/platform/storage/common/storage.ts

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
import { Promises, RunOnceScheduler, runWhenIdle } from 'vs/base/common/async';
77
import { Emitter, Event, PauseableEmitter } from 'vs/base/common/event';
8-
import { Disposable, dispose, MutableDisposable } from 'vs/base/common/lifecycle';
8+
import { Disposable, DisposableStore, dispose, MutableDisposable } from 'vs/base/common/lifecycle';
99
import { mark } from 'vs/base/common/performance';
1010
import { isUndefinedOrNull } from 'vs/base/common/types';
1111
import { InMemoryStorageDatabase, IStorage, IStorageChangeEvent, Storage, StorageHint, StorageValue } from 'vs/base/parts/storage/common/storage';
@@ -42,14 +42,34 @@ export interface IStorageEntry {
4242
readonly target: StorageTarget;
4343
}
4444

45+
export interface IWorkspaceStorageValueChangeEvent extends IStorageValueChangeEvent {
46+
readonly scope: StorageScope.WORKSPACE;
47+
}
48+
49+
export interface IProfileStorageValueChangeEvent extends IStorageValueChangeEvent {
50+
readonly scope: StorageScope.PROFILE;
51+
}
52+
53+
export interface IApplicationStorageValueChangeEvent extends IStorageValueChangeEvent {
54+
readonly scope: StorageScope.APPLICATION;
55+
}
56+
4557
export interface IStorageService {
4658

4759
readonly _serviceBrand: undefined;
4860

4961
/**
50-
* Emitted whenever data is updated or deleted.
62+
* Emitted whenever data is updated or deleted on the given
63+
* scope and optional key.
64+
*
65+
* @param scope the `StorageScope` to listen to changes
66+
* @param key the optional key to filter for or all keys of
67+
* the scope if `undefined`
5168
*/
52-
readonly onDidChangeValue: Event<IStorageValueChangeEvent>;
69+
onDidChangeValue(scope: StorageScope.WORKSPACE, key: string | undefined, disposable: DisposableStore): Event<IWorkspaceStorageValueChangeEvent>;
70+
onDidChangeValue(scope: StorageScope.PROFILE, key: string | undefined, disposable: DisposableStore): Event<IProfileStorageValueChangeEvent>;
71+
onDidChangeValue(scope: StorageScope.APPLICATION, key: string | undefined, disposable: DisposableStore): Event<IApplicationStorageValueChangeEvent>;
72+
onDidChangeValue(scope: StorageScope, key: string | undefined, disposable: DisposableStore): Event<IStorageValueChangeEvent>;
5373

5474
/**
5575
* Emitted whenever target of a storage entry changes.
@@ -294,7 +314,6 @@ export abstract class AbstractStorageService extends Disposable implements IStor
294314
private static DEFAULT_FLUSH_INTERVAL = 60 * 1000; // every minute
295315

296316
private readonly _onDidChangeValue = this._register(new PauseableEmitter<IStorageValueChangeEvent>());
297-
readonly onDidChangeValue = this._onDidChangeValue.event;
298317

299318
private readonly _onDidChangeTarget = this._register(new PauseableEmitter<IStorageTargetChangeEvent>());
300319
readonly onDidChangeTarget = this._onDidChangeTarget.event;
@@ -311,6 +330,13 @@ export abstract class AbstractStorageService extends Disposable implements IStor
311330
super();
312331
}
313332

333+
onDidChangeValue(scope: StorageScope.WORKSPACE, key: string | undefined, disposable: DisposableStore): Event<IWorkspaceStorageValueChangeEvent>;
334+
onDidChangeValue(scope: StorageScope.PROFILE, key: string | undefined, disposable: DisposableStore): Event<IProfileStorageValueChangeEvent>;
335+
onDidChangeValue(scope: StorageScope.APPLICATION, key: string | undefined, disposable: DisposableStore): Event<IApplicationStorageValueChangeEvent>;
336+
onDidChangeValue(scope: StorageScope, key: string | undefined, disposable: DisposableStore): Event<IStorageValueChangeEvent> {
337+
return Event.filter(this._onDidChangeValue.event, e => e.scope === scope && (key === undefined || e.key === key), disposable);
338+
}
339+
314340
private doFlushWhenIdle(): void {
315341
this.runFlushWhenIdle.value = runWhenIdle(() => {
316342
if (this.shouldFlushWhenIdle()) {

‎src/vs/platform/storage/test/common/storageService.test.ts

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import { deepStrictEqual, ok, strictEqual } from 'assert';
7+
import { DisposableStore } from 'vs/base/common/lifecycle';
78
import { InMemoryStorageService, IStorageService, IStorageTargetChangeEvent, IStorageValueChangeEvent, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage';
89

910
export function createSuite<T extends IStorageService>(params: { setup: () => Promise<T>; teardown: (service: T) => Promise<void> }): void {
@@ -32,7 +33,7 @@ export function createSuite<T extends IStorageService>(params: { setup: () => Pr
3233

3334
test('Storage change source', () => {
3435
const storageValueChangeEvents: IStorageValueChangeEvent[] = [];
35-
storageService.onDidChangeValue(e => storageValueChangeEvents.push(e));
36+
storageService.onDidChangeValue(StorageScope.WORKSPACE, undefined, new DisposableStore())(e => storageValueChangeEvents.push(e));
3637

3738
// Explicit external source
3839
storageService.storeAll([{ key: 'testExternalChange', value: 'foobar', scope: StorageScope.WORKSPACE, target: StorageTarget.MACHINE }], true);
@@ -49,9 +50,34 @@ export function createSuite<T extends IStorageService>(params: { setup: () => Pr
4950
strictEqual(storageValueChangeEvent?.external, false);
5051
});
5152

53+
test('Storage change event scope (all keys)', () => {
54+
const storageValueChangeEvents: IStorageValueChangeEvent[] = [];
55+
storageService.onDidChangeValue(StorageScope.WORKSPACE, undefined, new DisposableStore())(e => storageValueChangeEvents.push(e));
56+
57+
storageService.store('testChange', 'foobar', StorageScope.WORKSPACE, StorageTarget.MACHINE);
58+
storageService.store('testChange2', 'foobar', StorageScope.WORKSPACE, StorageTarget.MACHINE);
59+
storageService.store('testChange', 'foobar', StorageScope.APPLICATION, StorageTarget.MACHINE);
60+
storageService.store('testChange', 'foobar', StorageScope.PROFILE, StorageTarget.MACHINE);
61+
storageService.store('testChange2', 'foobar', StorageScope.PROFILE, StorageTarget.MACHINE);
62+
strictEqual(storageValueChangeEvents.length, 2);
63+
});
64+
65+
test('Storage change event scope (specific key)', () => {
66+
const storageValueChangeEvents: IStorageValueChangeEvent[] = [];
67+
storageService.onDidChangeValue(StorageScope.WORKSPACE, 'testChange', new DisposableStore())(e => storageValueChangeEvents.push(e));
68+
69+
storageService.store('testChange', 'foobar', StorageScope.WORKSPACE, StorageTarget.MACHINE);
70+
storageService.store('testChange', 'foobar', StorageScope.PROFILE, StorageTarget.USER);
71+
storageService.store('testChange', 'foobar', StorageScope.APPLICATION, StorageTarget.MACHINE);
72+
storageService.store('testChange2', 'foobar', StorageScope.WORKSPACE, StorageTarget.MACHINE);
73+
const storageValueChangeEvent = storageValueChangeEvents.find(e => e.key === 'testChange');
74+
ok(storageValueChangeEvent);
75+
strictEqual(storageValueChangeEvents.length, 1);
76+
});
77+
5278
function storeData(scope: StorageScope): void {
5379
let storageValueChangeEvents: IStorageValueChangeEvent[] = [];
54-
storageService.onDidChangeValue(e => storageValueChangeEvents.push(e));
80+
storageService.onDidChangeValue(scope, undefined, new DisposableStore())(e => storageValueChangeEvents.push(e));
5581

5682
strictEqual(storageService.get('test.get', scope, 'foobar'), 'foobar');
5783
strictEqual(storageService.get('test.get', scope, ''), '');
@@ -127,7 +153,7 @@ export function createSuite<T extends IStorageService>(params: { setup: () => Pr
127153

128154
function removeData(scope: StorageScope): void {
129155
const storageValueChangeEvents: IStorageValueChangeEvent[] = [];
130-
storageService.onDidChangeValue(e => storageValueChangeEvents.push(e));
156+
storageService.onDidChangeValue(scope, undefined, new DisposableStore())(e => storageValueChangeEvents.push(e));
131157

132158
storageService.store('test.remove', 'foobar', scope, StorageTarget.MACHINE);
133159
strictEqual('foobar', storageService.get('test.remove', scope, (undefined)!));
@@ -143,18 +169,19 @@ export function createSuite<T extends IStorageService>(params: { setup: () => Pr
143169
let storageTargetEvent: IStorageTargetChangeEvent | undefined = undefined;
144170
storageService.onDidChangeTarget(e => storageTargetEvent = e);
145171

146-
let storageValueChangeEvent: IStorageValueChangeEvent | undefined = undefined;
147-
storageService.onDidChangeValue(e => storageValueChangeEvent = e);
148-
149172
// Empty
150173
for (const scope of [StorageScope.WORKSPACE, StorageScope.PROFILE, StorageScope.APPLICATION]) {
151174
for (const target of [StorageTarget.MACHINE, StorageTarget.USER]) {
152175
strictEqual(storageService.keys(scope, target).length, 0);
153176
}
154177
}
155178

179+
let storageValueChangeEvent: IStorageValueChangeEvent | undefined = undefined;
180+
156181
// Add values
157182
for (const scope of [StorageScope.WORKSPACE, StorageScope.PROFILE, StorageScope.APPLICATION]) {
183+
storageService.onDidChangeValue(scope, undefined, new DisposableStore())(e => storageValueChangeEvent = e);
184+
158185
for (const target of [StorageTarget.MACHINE, StorageTarget.USER]) {
159186
storageTargetEvent = Object.create(null);
160187
storageValueChangeEvent = Object.create(null);

‎src/vs/platform/userDataSync/common/userDataSyncEnablementService.ts

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import { Emitter, Event } from 'vs/base/common/event';
7-
import { Disposable } from 'vs/base/common/lifecycle';
7+
import { Disposable, DisposableStore } from 'vs/base/common/lifecycle';
88
import { isWeb } from 'vs/base/common/platform';
99
import { IEnvironmentService } from 'vs/platform/environment/common/environment';
10-
import { IStorageService, IStorageValueChangeEvent, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage';
10+
import { IApplicationStorageValueChangeEvent, IStorageService, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage';
1111
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
1212
import { ALL_SYNC_RESOURCES, getEnablementKey, IUserDataSyncEnablementService, IUserDataSyncStoreManagementService, SyncResource } from 'vs/platform/userDataSync/common/userDataSync';
1313

@@ -36,7 +36,7 @@ export class UserDataSyncEnablementService extends Disposable implements IUserDa
3636
@IUserDataSyncStoreManagementService private readonly userDataSyncStoreManagementService: IUserDataSyncStoreManagementService,
3737
) {
3838
super();
39-
this._register(storageService.onDidChangeValue(e => this.onDidStorageChange(e)));
39+
this._register(storageService.onDidChangeValue(StorageScope.APPLICATION, undefined, this._register(new DisposableStore()))(e => this.onDidStorageChange(e)));
4040
}
4141

4242
isEnabled(): boolean {
@@ -80,11 +80,7 @@ export class UserDataSyncEnablementService extends Disposable implements IUserDa
8080
this.storageService.store(resourceEnablementKey, enabled, StorageScope.APPLICATION, isWeb ? StorageTarget.USER /* sync in web */ : StorageTarget.MACHINE);
8181
}
8282

83-
private onDidStorageChange(storageChangeEvent: IStorageValueChangeEvent): void {
84-
if (storageChangeEvent.scope !== StorageScope.APPLICATION) {
85-
return;
86-
}
87-
83+
private onDidStorageChange(storageChangeEvent: IApplicationStorageValueChangeEvent): void {
8884
if (enablementKey === storageChangeEvent.key) {
8985
this._onDidChangeEnablement.fire(this.isEnabled());
9086
return;

0 commit comments

Comments
 (0)