Skip to content

Commit befc284

Browse files
sandy081sandy081
committed
microsoft#100346 set conflicts status only when asked to applied
1 parent eed137f commit befc284

2 files changed

Lines changed: 44 additions & 42 deletions

File tree

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

Lines changed: 24 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,9 @@ export abstract class AbstractSynchroniser extends Disposable {
8080

8181
private _resourcePreviews: IResourcePreview[] = [];
8282
get resourcePreviews(): IResourcePreview[] { return this._resourcePreviews; }
83-
get conflicts(): IResourcePreview[] { return this._resourcePreviews.filter(({ hasConflicts }) => hasConflicts); }
83+
84+
private _conflicts: IResourcePreview[] = [];
85+
get conflicts(): IResourcePreview[] { return this._conflicts; }
8486
private _onDidChangeConflicts: Emitter<IResourcePreview[]> = this._register(new Emitter<IResourcePreview[]>());
8587
readonly onDidChangeConflicts: Event<IResourcePreview[]> = this._onDidChangeConflicts.event;
8688

@@ -146,8 +148,6 @@ export abstract class AbstractSynchroniser extends Disposable {
146148
protected setStatus(status: SyncStatus): void {
147149
if (this._status !== status) {
148150
const oldStatus = this._status;
149-
this._status = status;
150-
this._onDidChangStatus.fire(status);
151151
if (status === SyncStatus.HasConflicts) {
152152
// Log to telemetry when there is a sync conflict
153153
this.telemetryService.publicLog2<{ source: string }, SyncSourceClassification>('sync/conflictsDetected', { source: this.resource });
@@ -156,6 +156,8 @@ export abstract class AbstractSynchroniser extends Disposable {
156156
// Log to telemetry when conflicts are resolved
157157
this.telemetryService.publicLog2<{ source: string }, SyncSourceClassification>('sync/conflictsResolved', { source: this.resource });
158158
}
159+
this._status = status;
160+
this._onDidChangStatus.fire(status);
159161
}
160162
}
161163

@@ -355,15 +357,9 @@ export abstract class AbstractSynchroniser extends Disposable {
355357
if (!this.syncPreviewPromise) {
356358
this.syncPreviewPromise = createCancelablePromise(token => this.doGenerateSyncResourcePreview(remoteUserData, lastSyncUserData, token));
357359
}
358-
const preview = await this.syncPreviewPromise;
359-
360-
if (preview.resourcePreviews.some(({ hasConflicts }) => hasConflicts)) {
361-
return SyncStatus.HasConflicts;
362-
}
363360

364361
if (apply) {
365-
await this.apply(remoteUserData, lastSyncUserData, false);
366-
return SyncStatus.Idle;
362+
return await this.apply(remoteUserData, lastSyncUserData, false);
367363
} else {
368364
return SyncStatus.Syncing;
369365
}
@@ -377,15 +373,17 @@ export abstract class AbstractSynchroniser extends Disposable {
377373
}
378374
}
379375

380-
private async apply(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, force: boolean): Promise<void> {
376+
private async apply(remoteUserData: IRemoteUserData, lastSyncUserData: IRemoteUserData | null, force: boolean): Promise<SyncStatus> {
381377
if (!this.syncPreviewPromise) {
382-
return;
378+
return SyncStatus.Idle;
383379
}
384380

385381
const preview = await this.syncPreviewPromise;
386382

387-
if (preview.resourcePreviews.some(({ hasConflicts }) => hasConflicts)) {
388-
return;
383+
// update conflicts
384+
this.updateConflicts();
385+
if (this._conflicts.length) {
386+
return SyncStatus.HasConflicts;
389387
}
390388

391389
// apply preview
@@ -396,6 +394,8 @@ export abstract class AbstractSynchroniser extends Disposable {
396394

397395
// reset resource previews
398396
await this.updateResourcePreviews([], CancellationToken.None);
397+
398+
return SyncStatus.Idle;
399399
}
400400

401401
async acceptPreviewContent(resource: URI, content: string, force: boolean, headers: IHeaders = {}): Promise<ISyncResourcePreview | null> {
@@ -406,7 +406,7 @@ export abstract class AbstractSynchroniser extends Disposable {
406406
try {
407407
this.syncHeaders = { ...headers };
408408

409-
let preview = await this.syncPreviewPromise;
409+
const preview = await this.syncPreviewPromise;
410410
this.syncPreviewPromise = createCancelablePromise(async token => {
411411
const newPreview = await this.updateSyncResourcePreviewContent(preview, resource, content, token);
412412

@@ -416,18 +416,10 @@ export abstract class AbstractSynchroniser extends Disposable {
416416

417417
return newPreview;
418418
});
419-
preview = await this.syncPreviewPromise;
420-
421-
if (preview.resourcePreviews.some(({ hasConflicts }) => hasConflicts)) {
422-
return preview;
423-
}
424-
425-
await this.apply(preview.remoteUserData, preview.lastSyncUserData, force);
426419

427-
// reset status
428-
this.setStatus(SyncStatus.Idle);
429-
430-
return null;
420+
const status = await this.apply(preview.remoteUserData, preview.lastSyncUserData, force);
421+
this.setStatus(status);
422+
return this.syncPreviewPromise;
431423

432424
} finally {
433425
this.syncHeaders = {};
@@ -461,7 +453,6 @@ export abstract class AbstractSynchroniser extends Disposable {
461453
}
462454

463455
private async updateResourcePreviews(resourcePreviews: IResourcePreview[], token: CancellationToken): Promise<void> {
464-
const oldConflicts = this.conflicts;
465456
const oldPreviews = this._resourcePreviews;
466457
this._resourcePreviews = resourcePreviews;
467458

@@ -473,10 +464,13 @@ export abstract class AbstractSynchroniser extends Disposable {
473464
} catch (error) { /* Ignore */ }
474465
}
475466
}
467+
}
476468

477-
// update conflicts
478-
const newConflicts = this.conflicts;
469+
private updateConflicts(): void {
470+
const oldConflicts = this._conflicts;
471+
const newConflicts = this._resourcePreviews.filter(({ hasConflicts }) => hasConflicts);
479472
if (!equals(oldConflicts, newConflicts, (a, b) => isEqual(a.previewResource, b.previewResource))) {
473+
this._conflicts = newConflicts;
480474
this._onDidChangeConflicts.fire(newConflicts);
481475
}
482476
}
@@ -651,6 +645,7 @@ export abstract class AbstractSynchroniser extends Disposable {
651645
}
652646

653647
await this.updateResourcePreviews([], CancellationToken.None);
648+
this.updateConflicts();
654649

655650
this.setStatus(SyncStatus.Idle);
656651
this.logService.info(`${this.syncResourceLogLabel}: Stopped synchronizing ${this.resource.toLowerCase()}.`);

‎src/vs/platform/userDataSync/test/common/synchronizer.test.ts‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -147,19 +147,6 @@ suite('TestSynchronizer', () => {
147147
assert.deepEqual(testObject.status, SyncStatus.Idle);
148148
});
149149

150-
test('status is set correctly when sync has conflicts', async () => {
151-
const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
152-
testObject.syncResult = { hasConflicts: true, hasError: false };
153-
testObject.syncBarrier.open();
154-
155-
const actual: SyncStatus[] = [];
156-
disposableStore.add(testObject.onDidChangeStatus(status => actual.push(status)));
157-
await testObject.sync(await client.manifest());
158-
159-
assert.deepEqual(actual, [SyncStatus.Syncing, SyncStatus.HasConflicts]);
160-
assert.deepEqual(testObject.status, SyncStatus.HasConflicts);
161-
});
162-
163150
test('status is set correctly when sync has errors', async () => {
164151
const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
165152
testObject.syncResult = { hasError: true, hasConflicts: false };
@@ -177,6 +164,26 @@ suite('TestSynchronizer', () => {
177164
}
178165
});
179166

167+
test('status is set to hasConflicts when asked to sync if there are conflicts', async () => {
168+
const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
169+
testObject.syncResult = { hasConflicts: true, hasError: false };
170+
testObject.syncBarrier.open();
171+
172+
await testObject.sync(await client.manifest());
173+
174+
assert.deepEqual(testObject.status, SyncStatus.HasConflicts);
175+
});
176+
177+
test('status is set to syncing when asked for preview if there are conflicts', async () => {
178+
const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
179+
testObject.syncResult = { hasConflicts: true, hasError: false };
180+
testObject.syncBarrier.open();
181+
182+
await testObject.preview(await client.manifest());
183+
184+
assert.deepEqual(testObject.status, SyncStatus.Syncing);
185+
});
186+
180187
test('sync should not run if syncing already', async () => {
181188
const testObject: TestSynchroniser = client.instantiationService.createInstance(TestSynchroniser, SyncResource.Settings);
182189
const promise = Event.toPromise(testObject.onDoSyncCall.event);

0 commit comments

Comments
 (0)