Emit value change source in storage change event #185605
Conversation
src/vs/workbench/contrib/editSessions/common/workspaceStateSync.ts
Outdated
Show resolved
Hide resolved
src/vs/platform/userDataProfile/electron-main/userDataProfileStorageIpc.ts
Outdated
Show resolved
Hide resolved
src/vs/platform/userDataProfile/electron-main/userDataProfileStorageIpc.ts
Outdated
Show resolved
Hide resolved
src/vs/platform/userDataProfile/electron-main/userDataProfileStorageIpc.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/editSessions/common/workspaceStateSync.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
When I look at all the users of wasChangedExternally: true, it always seems to be from a loop over many keys and values. We could also simply have a new method storeAll that accepts an array of keys and values and that one will internally set the parameter. Might allow us to do other interesting things, such as only emitting the storage change events when all keys have been set. If a listener of storage change event depends on other state, we could then ensure the state is consistent at that point when the event is fired.
| @@ -118,8 +118,10 @@ export interface IStorageService { | |||
| * | |||
| * @param target allows to define the target of the storage operation | |||
| * to either the current machine or user. | |||
| * | |||
| * @param source allows to specify the source of the storage change. | |||
There was a problem hiding this comment.
This comment seems outdated with the new boolean
| } | ||
|
|
||
| private accept(key: string, value: string | undefined): void { | ||
| private accept(key: string, value: string | undefined, wasChangedExternally = false): void { |
There was a problem hiding this comment.
No need to set false as default as the method is only called with a boolean?
| @@ -52,9 +52,14 @@ export interface IStorageDatabase { | |||
| close(recovery?: () => Map<string, string>): Promise<void>; | |||
| } | |||
|
|
|||
| export interface IStorageChangeEvent { | |||
| readonly key: string; | |||
| readonly wasChangedExternally?: boolean; | |||
There was a problem hiding this comment.
I suggest to put the same JSDoc comment here explaining what it means.
| @@ -278,7 +283,7 @@ export class Storage extends Disposable implements IStorage { | |||
| this.pendingInserts.delete(key); | |||
|
|
|||
| // Event | |||
| this._onDidChangeStorage.fire(key); | |||
| this._onDidChangeStorage.fire({ key, wasChangedExternally: false }); | |||
There was a problem hiding this comment.
Do we need to allow to pass in wasChangedExternally also for delete? What if a settings sync removes a key because its value changed to a default value?
| @@ -607,13 +617,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, wasChangedExternally: true }); | |||
There was a problem hiding this comment.
What about newStorage.set(key, value); in line 608, shouldnt that also wasChangedExternally: true?
|
I am exploring the |
For #183449
Currently we emit a change event when a component like breakpoints writes to a key that it owns updates for. Since we now want components which may previously not have handled
onDidChangeValueto register listeners for it, which can be emitted as part of a Continue On transition, it makes adoption easier on components if they avoid handlingonDidChangeValuewhen the source is the component itself.