storage - storeAll and external (microsoft#185837)

* Emit source in storage change event

* Address some comments

* Fix failed rename

* Part -> Self

* JSDoc for new param

* Add tests

* Fix tests

* Switch from enum to boolean

* Fix tests

* first cut

* adopt storeAll in more places

* more adoptions

* renames

* more adoptions

* extract interface

* more renames

* more tests

* fix test

* more tests

* 💄 jsdoc

---------

Co-authored-by: Joyce Er <[email protected]>

Author: bpasero

src/vs/base/parts/storage/common/storage.ts

@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { ThrottledDelayer } from 'vs/base/common/async';
-import { Emitter, Event } from 'vs/base/common/event';
+import { Event, PauseableEmitter } from 'vs/base/common/event';
 import { Disposable, IDisposable } from 'vs/base/common/lifecycle';
 import { parse, stringify } from 'vs/base/common/marshalling';
 import { isObject, isUndefinedOrNull } from 'vs/base/common/types';
@@ -52,9 +52,29 @@ export interface IStorageDatabase {
 	close(recovery?: () => Map<string, string>): Promise<void>;
 }
 
+export interface IStorageChangeEvent {
+
+	/**
+	 * The `key` of the storage entry that was changed
+	 * or was removed.
+	 */
+	readonly key: string;
+
+	/**
+	 * A hint how the storage change event was triggered. If
+	 * `true`, the storage change was triggered by an external
+	 * source, such as:
+	 * - another process (for example another window)
+	 * - operations such as settings sync or profiles change
+	 */
+	readonly external?: boolean;
+}
+
+export type StorageValue = string | boolean | number | undefined | null | object;
+
 export interface IStorage extends IDisposable {
 
-	readonly onDidChangeStorage: Event<string>;
+	readonly onDidChangeStorage: Event<IStorageChangeEvent>;
 
 	readonly items: Map<string, string>;
 	readonly size: number;
@@ -73,8 +93,8 @@ export interface IStorage extends IDisposable {
 	getObject<T extends object>(key: string, fallbackValue: T): T;
 	getObject<T extends object>(key: string, fallbackValue?: T): T | undefined;
 
-	set(key: string, value: string | boolean | number | undefined | null | object): Promise<void>;
-	delete(key: string): Promise<void>;
+	set(key: string, value: StorageValue, external?: boolean): Promise<void>;
+	delete(key: string, external?: boolean): Promise<void>;
 
 	flush(delay?: number): Promise<void>;
 	whenFlushed(): Promise<void>;
@@ -92,7 +112,7 @@ export class Storage extends Disposable implements IStorage {
 
 	private static readonly DEFAULT_FLUSH_DELAY = 100;
 
-	private readonly _onDidChangeStorage = this._register(new Emitter<string>());
+	private readonly _onDidChangeStorage = this._register(new PauseableEmitter<IStorageChangeEvent>());
 	readonly onDidChangeStorage = this._onDidChangeStorage.event;
 
 	private state = StorageState.None;
@@ -122,14 +142,22 @@ export class Storage extends Disposable implements IStorage {
 	}
 
 	private onDidChangeItemsExternal(e: IStorageItemsChangeEvent): void {
-		// items that change external require us to update our
-		// caches with the values. we just accept the value and
-		// emit an event if there is a change.
-		e.changed?.forEach((value, key) => this.accept(key, value));
-		e.deleted?.forEach(key => this.accept(key, undefined));
+		this._onDidChangeStorage.pause();
+
+		try {
+			// items that change external require us to update our
+			// caches with the values. we just accept the value and
+			// emit an event if there is a change.
+
+			e.changed?.forEach((value, key) => this.acceptExternal(key, value));
+			e.deleted?.forEach(key => this.acceptExternal(key, undefined));
+
+		} finally {
+			this._onDidChangeStorage.resume();
+		}
 	}
 
-	private accept(key: string, value: string | undefined): void {
+	private acceptExternal(key: string, value: string | undefined): void {
 		if (this.state === StorageState.Closed) {
 			return; // Return early if we are already closed
 		}
@@ -152,7 +180,7 @@ export class Storage extends Disposable implements IStorage {
 
 		// Signal to outside listeners
 		if (changed) {
-			this._onDidChangeStorage.fire(key);
+			this._onDidChangeStorage.fire({ key, external: true });
 		}
 	}
 
@@ -229,14 +257,14 @@ export class Storage extends Disposable implements IStorage {
 		return parse(value);
 	}
 
-	async set(key: string, value: string | boolean | number | null | undefined | object): Promise<void> {
+	async set(key: string, value: string | boolean | number | null | undefined | object, external = false): Promise<void> {
 		if (this.state === StorageState.Closed) {
 			return; // Return early if we are already closed
 		}
 
 		// We remove the key for undefined/null values
 		if (isUndefinedOrNull(value)) {
-			return this.delete(key);
+			return this.delete(key, external);
 		}
 
 		// Otherwise, convert to String and store
@@ -254,13 +282,13 @@ export class Storage extends Disposable implements IStorage {
 		this.pendingDeletes.delete(key);
 
 		// Event
-		this._onDidChangeStorage.fire(key);
+		this._onDidChangeStorage.fire({ key, external });
 
 		// Accumulate work by scheduling after timeout
 		return this.doFlush();
 	}
 
-	async delete(key: string): Promise<void> {
+	async delete(key: string, external = false): Promise<void> {
 		if (this.state === StorageState.Closed) {
 			return; // Return early if we are already closed
 		}
@@ -278,7 +306,7 @@ export class Storage extends Disposable implements IStorage {
 		this.pendingInserts.delete(key);
 
 		// Event
-		this._onDidChangeStorage.fire(key);
+		this._onDidChangeStorage.fire({ key, external });
 
 		// Accumulate work by scheduling after timeout
 		return this.doFlush();

src/vs/base/parts/storage/test/node/storage.integrationTest.ts

@@ -59,8 +59,8 @@ flakySuite('Storage Library', function () {
 			deepStrictEqual(storage.getObject('foo', { 'bar': 'baz' }), { 'bar': 'baz' });
 
 			let changes = new Set<string>();
-			storage.onDidChangeStorage(key => {
-				changes.add(key);
+			storage.onDidChangeStorage(e => {
+				changes.add(e.key);
 			});
 
 			await storage.whenFlushed(); // returns immediately when no pending updates
@@ -149,8 +149,8 @@ flakySuite('Storage Library', function () {
 			const storage = new Storage(database);
 
 			const changes = new Set<string>();
-			storage.onDidChangeStorage(key => {
-				changes.add(key);
+			storage.onDidChangeStorage(e => {
+				changes.add(e.key);
 			});
 
 			await storage.init();
@@ -272,8 +272,8 @@ flakySuite('Storage Library', function () {
 			await storage.init();
 
 			let changes = new Set<string>();
-			storage.onDidChangeStorage(key => {
-				changes.add(key);
+			storage.onDidChangeStorage(e => {
+				changes.add(e.key);
 			});
 
 			const set1Promise = storage.set('foo', 'bar1');

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

@@ -77,6 +77,7 @@ suite('FindController', async () => {
 		getNumber: (key: string) => undefined!,
 		getObject: (key: string) => undefined!,
 		store: (key: string, value: any) => { queryState[key] = value; return Promise.resolve(); },
+		storeAll: () => { throw new Error(); },
 		remove: () => undefined,
 		isNew: () => false,
 		flush: () => { return Promise.resolve(); },
@@ -510,6 +511,7 @@ suite('FindController query options persistence', async () => {
 		getNumber: (key: string) => undefined!,
 		getObject: (key: string) => undefined!,
 		store: (key: string, value: any) => { queryState[key] = value; return Promise.resolve(); },
+		storeAll: () => { throw new Error(); },
 		remove: () => undefined,
 		isNew: () => false,
 		flush: () => { return Promise.resolve(); },

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

@@ -94,6 +94,7 @@ suite('Multicursor selection', () => {
 		getNumber: (key: string) => undefined!,
 		getObject: (key: string) => undefined!,
 		store: (key: string, value: any) => { queryState[key] = value; return Promise.resolve(); },
+		storeAll: () => { throw new Error(); },
 		remove: (key) => undefined,
 		log: () => undefined,
 		switch: () => Promise.resolve(undefined),

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

@@ -8,7 +8,7 @@ import { Emitter, Event, PauseableEmitter } from 'vs/base/common/event';
 import { Disposable, dispose, MutableDisposable } from 'vs/base/common/lifecycle';
 import { mark } from 'vs/base/common/performance';
 import { isUndefinedOrNull } from 'vs/base/common/types';
-import { InMemoryStorageDatabase, IStorage, Storage, StorageHint } from 'vs/base/parts/storage/common/storage';
+import { InMemoryStorageDatabase, IStorage, IStorageChangeEvent, Storage, StorageHint, StorageValue } from 'vs/base/parts/storage/common/storage';
 import { createDecorator } from 'vs/platform/instantiation/common/instantiation';
 import { isUserDataProfile, IUserDataProfile } from 'vs/platform/userDataProfile/common/userDataProfile';
 import { IAnyWorkspaceIdentifier } from 'vs/platform/workspace/common/workspace';
@@ -35,6 +35,13 @@ export interface IWillSaveStateEvent {
 	readonly reason: WillSaveStateReason;
 }
 
+export interface IStorageEntry {
+	readonly key: string;
+	readonly value: StorageValue;
+	readonly scope: StorageScope;
+	readonly target: StorageTarget;
+}
+
 export interface IStorageService {
 
 	readonly _serviceBrand: undefined;
@@ -119,7 +126,16 @@ export interface IStorageService {
 	 * @param target allows to define the target of the storage operation
 	 * to either the current machine or user.
 	 */
-	store(key: string, value: string | boolean | number | undefined | null | object, scope: StorageScope, target: StorageTarget): void;
+	store(key: string, value: StorageValue, scope: StorageScope, target: StorageTarget): void;
+
+	/**
+	 * Allows to store multiple values in a bulk operation. Events will only
+	 * be emitted when all values have been stored.
+	 *
+	 * @param external a hint to indicate the source of the operation is external,
+	 * such as settings sync or profile changes.
+	 */
+	storeAll(entries: Array<IStorageEntry>, external: boolean): void;
 
 	/**
 	 * Delete an element stored under the provided key from storage.
@@ -229,6 +245,15 @@ export interface IStorageValueChangeEvent {
 	 * removed.
 	 */
 	readonly target: StorageTarget | undefined;
+
+	/**
+	 * A hint how the storage change event was triggered. If
+	 * `true`, the storage change was triggered by an external
+	 * source, such as:
+	 * - another process (for example another window)
+	 * - operations such as settings sync or profiles change
+	 */
+	readonly external?: boolean;
 }
 
 export interface IStorageTargetChangeEvent {
@@ -332,7 +357,8 @@ export abstract class AbstractStorageService extends Disposable implements IStor
 		return this.initializationPromise;
 	}
 
-	protected emitDidChangeValue(scope: StorageScope, key: string): void {
+	protected emitDidChangeValue(scope: StorageScope, event: IStorageChangeEvent): void {
+		const { key, external } = event;
 
 		// Specially handle `TARGET_KEY`
 		if (key === TARGET_KEY) {
@@ -356,7 +382,7 @@ export abstract class AbstractStorageService extends Disposable implements IStor
 
 		// Emit any other key to outside
 		else {
-			this._onDidChangeValue.fire({ scope, key, target: this.getKeyTargets(scope)[key] });
+			this._onDidChangeValue.fire({ scope, key, target: this.getKeyTargets(scope)[key], external });
 		}
 	}
 
@@ -388,11 +414,19 @@ export abstract class AbstractStorageService extends Disposable implements IStor
 		return this.getStorage(scope)?.getObject(key, fallbackValue);
 	}
 
-	store(key: string, value: string | boolean | number | undefined | null | object, scope: StorageScope, target: StorageTarget): void {
+	storeAll(entries: Array<IStorageEntry>, external: boolean): void {
+		this.withPausedEmitters(() => {
+			for (const entry of entries) {
+				this.store(entry.key, entry.value, entry.scope, entry.target, external);
+			}
+		});
+	}
+
+	store(key: string, value: StorageValue, scope: StorageScope, target: StorageTarget, external = false): void {
 
 		// We remove the key for undefined/null values
 		if (isUndefinedOrNull(value)) {
-			this.remove(key, scope);
+			this.remove(key, scope, external);
 			return;
 		}
 
@@ -403,11 +437,11 @@ export abstract class AbstractStorageService extends Disposable implements IStor
 			this.updateKeyTarget(key, scope, target);
 
 			// Store actual value
-			this.getStorage(scope)?.set(key, value);
+			this.getStorage(scope)?.set(key, value, external);
 		});
 	}
 
-	remove(key: string, scope: StorageScope): void {
+	remove(key: string, scope: StorageScope, external = false): void {
 
 		// Update our datastructures but send events only after
 		this.withPausedEmitters(() => {
@@ -416,7 +450,7 @@ export abstract class AbstractStorageService extends Disposable implements IStor
 			this.updateKeyTarget(key, scope, undefined);
 
 			// Remove actual key
-			this.getStorage(scope)?.delete(key);
+			this.getStorage(scope)?.delete(key, external);
 		});
 	}
 
@@ -450,22 +484,22 @@ export abstract class AbstractStorageService extends Disposable implements IStor
 		return keys;
 	}
 
-	private updateKeyTarget(key: string, scope: StorageScope, target: StorageTarget | undefined): void {
+	private updateKeyTarget(key: string, scope: StorageScope, target: StorageTarget | undefined, external = false): void {
 
 		// Add
 		const keyTargets = this.getKeyTargets(scope);
 		if (typeof target === 'number') {
 			if (keyTargets[key] !== target) {
 				keyTargets[key] = target;
-				this.getStorage(scope)?.set(TARGET_KEY, JSON.stringify(keyTargets));
+				this.getStorage(scope)?.set(TARGET_KEY, JSON.stringify(keyTargets), external);
 			}
 		}
 
 		// Remove
 		else {
 			if (typeof keyTargets[key] === 'number') {
 				delete keyTargets[key];
-				this.getStorage(scope)?.set(TARGET_KEY, JSON.stringify(keyTargets));
+				this.getStorage(scope)?.set(TARGET_KEY, JSON.stringify(keyTargets), external);
 			}
 		}
 	}
@@ -595,7 +629,7 @@ export abstract class AbstractStorageService extends Disposable implements IStor
 			// Copy over previous keys if `preserveData`
 			if (preserveData) {
 				for (const [key, value] of oldStorage) {
-					newStorage.set(key, value);
+					newStorage.set(key, value, true);
 				}
 			}
 
@@ -607,13 +641,13 @@ export abstract class AbstractStorageService extends Disposable implements IStor
 
 					const newValue = newStorage.get(key);
 					if (newValue !== oldValue) {
-						this.emitDidChangeValue(scope, key);
+						this.emitDidChangeValue(scope, { key, external: true });
 					}
 				}
 
 				for (const [key] of newStorage.items) {
 					if (!handledkeys.has(key)) {
-						this.emitDidChangeValue(scope, key);
+						this.emitDidChangeValue(scope, { key, external: true });
 					}
 				}
 			}
@@ -647,9 +681,9 @@ export class InMemoryStorageService extends AbstractStorageService {
 	constructor() {
 		super();
 
-		this._register(this.workspaceStorage.onDidChangeStorage(key => this.emitDidChangeValue(StorageScope.WORKSPACE, key)));
-		this._register(this.profileStorage.onDidChangeStorage(key => this.emitDidChangeValue(StorageScope.PROFILE, key)));
-		this._register(this.applicationStorage.onDidChangeStorage(key => this.emitDidChangeValue(StorageScope.APPLICATION, key)));
+		this._register(this.workspaceStorage.onDidChangeStorage(e => this.emitDidChangeValue(StorageScope.WORKSPACE, e)));
+		this._register(this.profileStorage.onDidChangeStorage(e => this.emitDidChangeValue(StorageScope.PROFILE, e)));
+		this._register(this.applicationStorage.onDidChangeStorage(e => this.emitDidChangeValue(StorageScope.APPLICATION, e)));
 	}
 
 	protected getStorage(scope: StorageScope): IStorage {