From 44673c2129d4bb1294478f84be5f4fa9c9aa4355 Mon Sep 17 00:00:00 2001 From: Sandeep Somavarapu Date: Wed, 12 Oct 2022 12:38:48 +0200 Subject: [PATCH] add more logging (#163419) * add more logging * fix tests: check remote.all for remote change --- .../userDataSync/common/globalStateMerge.ts | 14 ++---- .../userDataSync/common/globalStateSync.ts | 14 +++--- .../test/common/globalStateMerge.test.ts | 50 +++++++++---------- .../views/common/viewContainerModel.ts | 22 ++++++-- 4 files changed, 55 insertions(+), 45 deletions(-) diff --git a/src/vs/platform/userDataSync/common/globalStateMerge.ts b/src/vs/platform/userDataSync/common/globalStateMerge.ts index 8f2e51bb67d..930706f3c15 100644 --- a/src/vs/platform/userDataSync/common/globalStateMerge.ts +++ b/src/vs/platform/userDataSync/common/globalStateMerge.ts @@ -10,18 +10,18 @@ import { IStorageValue, SYNC_SERVICE_URL_TYPE } from 'vs/platform/userDataSync/c export interface IMergeResult { local: { added: IStringDictionary; removed: string[]; updated: IStringDictionary }; - remote: IStringDictionary | null; + remote: { added: string[]; removed: string[]; updated: string[]; all: IStringDictionary | null }; } export function merge(localStorage: IStringDictionary, remoteStorage: IStringDictionary | null, baseStorage: IStringDictionary | null, storageKeys: { machine: ReadonlyArray; unregistered: ReadonlyArray }, logService: ILogService): IMergeResult { if (!remoteStorage) { - return { remote: Object.keys(localStorage).length > 0 ? localStorage : null, local: { added: {}, removed: [], updated: {} } }; + return { remote: { added: Object.keys(localStorage), removed: [], updated: [], all: Object.keys(localStorage).length > 0 ? localStorage : null }, local: { added: {}, removed: [], updated: {} } }; } const localToRemote = compare(localStorage, remoteStorage); if (localToRemote.added.size === 0 && localToRemote.removed.size === 0 && localToRemote.updated.size === 0) { // No changes found between local and remote. - return { remote: null, local: { added: {}, removed: [], updated: {} } }; + return { remote: { added: [], removed: [], updated: [], all: null }, local: { added: {}, removed: [], updated: {} } }; } const baseToRemote = baseStorage ? compare(baseStorage, remoteStorage) : { added: Object.keys(remoteStorage).reduce((r, k) => { r.add(k); return r; }, new Set()), removed: new Set(), updated: new Set() }; @@ -116,7 +116,8 @@ export function merge(localStorage: IStringDictionary, remoteStor local.removed.push(key); } - return { local, remote: areSame(remote, remoteStorage) ? null : remote }; + const result = compare(remoteStorage, remote); + return { local, remote: { added: [...result.added], updated: [...result.updated], removed: [...result.removed], all: result.added.size === 0 && result.removed.size === 0 && result.updated.size === 0 ? null : remote } }; } function compare(from: IStringDictionary, to: IStringDictionary): { added: Set; removed: Set; updated: Set } { @@ -140,8 +141,3 @@ function compare(from: IStringDictionary, to: IStringDictionary): { ad return { added, removed, updated }; } -function areSame(a: IStringDictionary, b: IStringDictionary): boolean { - const { added, removed, updated } = compare(a, b); - return added.size === 0 && removed.size === 0 && updated.size === 0; -} - diff --git a/src/vs/platform/userDataSync/common/globalStateSync.ts b/src/vs/platform/userDataSync/common/globalStateSync.ts index e10896d7bf8..eabf5326051 100644 --- a/src/vs/platform/userDataSync/common/globalStateSync.ts +++ b/src/vs/platform/userDataSync/common/globalStateSync.ts @@ -37,7 +37,7 @@ type StorageKeys = { machine: string[]; user: string[]; unregistered: string[] } interface IGlobalStateResourceMergeResult extends IAcceptResult { readonly local: { added: IStringDictionary; removed: string[]; updated: IStringDictionary }; - readonly remote: IStringDictionary | null; + readonly remote: { added: string[]; removed: string[]; updated: string[]; all: IStringDictionary | null }; } export interface IGlobalStateResourcePreview extends IResourcePreview { @@ -133,7 +133,7 @@ export class GlobalStateSynchroniser extends AbstractSynchroniser implements IUs local, remote, localChange: Object.keys(local.added).length > 0 || Object.keys(local.updated).length > 0 || local.removed.length > 0 ? Change.Modified : Change.None, - remoteChange: remote !== null ? Change.Modified : Change.None, + remoteChange: remote.all !== null ? Change.Modified : Change.None, }; const localContent = stringify(localGlobalState, false); @@ -162,7 +162,7 @@ export class GlobalStateSynchroniser extends AbstractSynchroniser implements IUs const localGlobalState = await this.getLocalGlobalState(); const storageKeys = await this.getStorageKeys(lastSyncGlobalState); const { remote } = merge(localGlobalState.storage, lastSyncGlobalState.storage, lastSyncGlobalState.storage, storageKeys, this.logService); - return remote !== null; + return remote.all !== null; } protected async getMergeResult(resourcePreview: IGlobalStateResourcePreview, token: CancellationToken): Promise { @@ -193,7 +193,7 @@ export class GlobalStateSynchroniser extends AbstractSynchroniser implements IUs return { content: resourcePreview.localContent, local: { added: {}, removed: [], updated: {} }, - remote: resourcePreview.localUserData.storage, + remote: { added: Object.keys(resourcePreview.localUserData.storage), removed: [], updated: [], all: resourcePreview.localUserData.storage }, localChange: Change.None, remoteChange: Change.Modified, }; @@ -214,7 +214,7 @@ export class GlobalStateSynchroniser extends AbstractSynchroniser implements IUs return { content: resourcePreview.remoteContent, local: { added: {}, removed: [], updated: {} }, - remote: null, + remote: { added: [], removed: [], updated: [], all: null }, localChange: Change.None, remoteChange: Change.None, }; @@ -240,9 +240,9 @@ export class GlobalStateSynchroniser extends AbstractSynchroniser implements IUs if (remoteChange !== Change.None) { // update remote this.logService.trace(`${this.syncResourceLogLabel}: Updating remote ui state...`); - const content = JSON.stringify({ storage: remote }); + const content = JSON.stringify({ storage: remote.all }); remoteUserData = await this.updateRemoteUserData(content, force ? null : remoteUserData.ref); - this.logService.info(`${this.syncResourceLogLabel}: Updated remote ui state`); + this.logService.info(`${this.syncResourceLogLabel}: Updated remote ui state.${remote.added.length ? ` Added: ${remote.added}.` : ''}${remote.updated.length ? ` Updated: ${remote.updated}.` : ''}${remote.removed.length ? ` Removed: ${remote.removed}.` : ''}`); } if (lastSyncUserData?.ref !== remoteUserData.ref) { diff --git a/src/vs/platform/userDataSync/test/common/globalStateMerge.test.ts b/src/vs/platform/userDataSync/test/common/globalStateMerge.test.ts index 50ff413ed27..be518dba470 100644 --- a/src/vs/platform/userDataSync/test/common/globalStateMerge.test.ts +++ b/src/vs/platform/userDataSync/test/common/globalStateMerge.test.ts @@ -18,7 +18,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when local and remote are same with multiple entries and local is not synced yet', async () => { @@ -30,7 +30,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when local and remote are same with multiple entries in different order and local is not synced yet', async () => { @@ -42,7 +42,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when local and remote are same with different base content', async () => { @@ -55,7 +55,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when a new entry is added to remote and local has not synced yet', async () => { @@ -67,7 +67,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, { 'b': { version: 1, value: 'b' } }); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when multiple new entries are added to remote and local is not synced yet', async () => { @@ -79,7 +79,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, { 'b': { version: 1, value: 'b' }, 'a': { version: 1, value: 'a' } }); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when new entry is added to remote from base and local has not changed', async () => { @@ -91,7 +91,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, { 'b': { version: 1, value: 'b' } }); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when an entry is removed from remote from base and local has not changed', async () => { @@ -103,7 +103,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, ['b']); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when all entries are removed from base and local has not changed', async () => { @@ -115,7 +115,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, ['b', 'a']); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when an entry is updated in remote from base and local has not changed', async () => { @@ -127,7 +127,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, { 'a': { version: 1, value: 'b' } }); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when remote has moved forwarded with multiple changes and local stays with base', async () => { @@ -139,7 +139,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, { 'c': { version: 1, value: 'c' } }); assert.deepStrictEqual(actual.local.updated, { 'a': { version: 1, value: 'd' } }); assert.deepStrictEqual(actual.local.removed, ['b']); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when new entries are added to local and local is not synced yet', async () => { @@ -151,7 +151,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, local); + assert.deepStrictEqual(actual.remote.all, local); }); test('merge when multiple new entries are added to local from base and remote is not changed', async () => { @@ -163,7 +163,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, local); + assert.deepStrictEqual(actual.remote.all, local); }); test('merge when an entry is removed from local from base and remote has not changed', async () => { @@ -175,7 +175,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, local); + assert.deepStrictEqual(actual.remote.all, local); }); test('merge when an entry is updated in local from base and remote has not changed', async () => { @@ -187,7 +187,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, local); + assert.deepStrictEqual(actual.remote.all, local); }); test('merge when local has moved forwarded with multiple changes and remote stays with base', async () => { @@ -199,7 +199,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, local); + assert.deepStrictEqual(actual.remote.all, local); }); test('merge when local and remote with one entry but different value and local is not synced yet', async () => { @@ -211,7 +211,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, { 'a': { version: 1, value: 'b' } }); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when the entry is removed in remote but updated in local and a new entry is added in remote', async () => { @@ -224,7 +224,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, { 'c': { version: 1, value: 'c' } }); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, { 'a': { version: 1, value: 'a' }, 'c': { version: 1, value: 'c' }, 'b': { version: 1, value: 'd' } }); + assert.deepStrictEqual(actual.remote.all, { 'a': { version: 1, value: 'a' }, 'c': { version: 1, value: 'c' }, 'b': { version: 1, value: 'd' } }); }); test('merge with single entry and local is empty', async () => { @@ -237,7 +237,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, local); + assert.deepStrictEqual(actual.remote.all, local); }); test('merge when local and remote has moved forward with conflicts', async () => { @@ -250,7 +250,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, local); + assert.deepStrictEqual(actual.remote.all, local); }); test('merge when a new entry is added to remote but scoped to machine locally and local is not synced yet', async () => { @@ -262,7 +262,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when an entry is updated to remote but scoped to machine locally', async () => { @@ -274,7 +274,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, null); + assert.deepStrictEqual(actual.remote.all, null); }); test('merge when a local value is removed and scoped to machine locally', async () => { @@ -287,7 +287,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, local); + assert.deepStrictEqual(actual.remote.all, local); }); test('merge when local moved forwared by changing a key to machine scope', async () => { @@ -300,7 +300,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, local); + assert.deepStrictEqual(actual.remote.all, local); }); test('merge should not remove remote keys if not registered', async () => { @@ -313,7 +313,7 @@ suite('GlobalStateMerge', () => { assert.deepStrictEqual(actual.local.added, {}); assert.deepStrictEqual(actual.local.updated, {}); assert.deepStrictEqual(actual.local.removed, []); - assert.deepStrictEqual(actual.remote, { 'a': { version: 1, value: 'a' }, 'b': { version: 1, value: 'b' }, 'c': { version: 1, value: 'c' } }); + assert.deepStrictEqual(actual.remote.all, { 'a': { version: 1, value: 'a' }, 'b': { version: 1, value: 'b' }, 'c': { version: 1, value: 'c' } }); }); }); diff --git a/src/vs/workbench/services/views/common/viewContainerModel.ts b/src/vs/workbench/services/views/common/viewContainerModel.ts index 0418f5ee6cd..4bbed6f5ffd 100644 --- a/src/vs/workbench/services/views/common/viewContainerModel.ts +++ b/src/vs/workbench/services/views/common/viewContainerModel.ts @@ -18,6 +18,7 @@ import { ThemeIcon } from 'vs/platform/theme/common/themeService'; import { IStringDictionary } from 'vs/base/common/collections'; import { Extensions, IProfileStorageRegistry } from 'vs/workbench/services/userDataProfile/common/userDataProfileStorageRegistry'; import { localize } from 'vs/nls'; +import { ILogService } from 'vs/platform/log/common/log'; export function getViewsStateStorageId(viewContainerStorageId: string): string { return `${viewContainerStorageId}.hidden`; } @@ -88,6 +89,7 @@ class ViewDescriptorsState extends Disposable { viewContainerStorageId: string, viewContainerName: string, @IStorageService private readonly storageService: IStorageService, + @ILogService private readonly logService: ILogService, ) { super(); @@ -162,6 +164,9 @@ class ViewDescriptorsState extends Disposable { const state = this.get(id); if (state) { if (state.visibleGlobal !== !storedState.isHidden) { + if (!storedState.isHidden) { + this.logService.info(`View visibility state changed: ${id} is now visible`); + } changedStates.push({ id, visible: !storedState.isHidden }); } } else { @@ -348,6 +353,7 @@ export class ViewContainerModel extends Disposable implements IViewContainerMode readonly viewContainer: ViewContainer, @IInstantiationService instantiationService: IInstantiationService, @IContextKeyService private readonly contextKeyService: IContextKeyService, + @ILogService private readonly logService: ILogService, ) { super(); @@ -356,10 +362,11 @@ export class ViewContainerModel extends Disposable implements IViewContainerMode this._register(this.viewDescriptorsState.onDidChangeStoredState(items => this.updateVisibility(items))); this._register(Event.any( - this.onDidAddVisibleViewDescriptors, - this.onDidRemoveVisibleViewDescriptors, - this.onDidMoveVisibleViewDescriptors) - (() => { + Event.map(this.onDidAddVisibleViewDescriptors, added => `Added views:${added.map(v => v.viewDescriptor.id).join(',')}`), + Event.map(this.onDidRemoveVisibleViewDescriptors, removed => `Removed views:${removed.map(v => v.viewDescriptor.id).join(',')}`), + Event.map(this.onDidMoveVisibleViewDescriptors, ({ from, to }) => `Moved view ${from.viewDescriptor.id} to ${to.viewDescriptor.id}`)) + (message => { + this.logService.info(message); this.viewDescriptorsState.updateState(this.allViewDescriptors); this.updateContainerInfo(); })); @@ -464,6 +471,9 @@ export class ViewContainerModel extends Disposable implements IViewContainerMode viewDescriptorItem.state.visibleWorkspace = visible; } else { viewDescriptorItem.state.visibleGlobal = visible; + if (visible) { + this.logService.info(`Showing view ${viewDescriptorItem.viewDescriptor.id} in the container ${this.viewContainer.id}`); + } } // return `true` only if visibility is changed @@ -532,7 +542,11 @@ export class ViewContainerModel extends Disposable implements IViewContainerMode if (viewDescriptor.workspace) { state.visibleWorkspace = isUndefinedOrNull(addedViewDescriptorState.visible) ? (isUndefinedOrNull(state.visibleWorkspace) ? !viewDescriptor.hideByDefault : state.visibleWorkspace) : addedViewDescriptorState.visible; } else { + const isVisible = state.visibleGlobal; state.visibleGlobal = isUndefinedOrNull(addedViewDescriptorState.visible) ? (isUndefinedOrNull(state.visibleGlobal) ? !viewDescriptor.hideByDefault : state.visibleGlobal) : addedViewDescriptorState.visible; + if (state.visibleGlobal && !isVisible) { + this.logService.info(`Added view ${viewDescriptor.id} in the container ${this.viewContainer.id} and showing it`); + } } state.collapsed = isUndefinedOrNull(addedViewDescriptorState.collapsed) ? (isUndefinedOrNull(state.collapsed) ? !!viewDescriptor.collapsed : state.collapsed) : addedViewDescriptorState.collapsed; } else {