From ab97b0d0f652af3d78abc23a4cce651821ffc5b0 Mon Sep 17 00:00:00 2001 From: Arseniy Terekhin Date: Sun, 1 Dec 2024 16:53:56 +0300 Subject: [PATCH] fix: rework sync bar moments Let the `SyncBar` decide what to do with the fossil output, thus simplifying logic separation. Plus add tests. --- src/repository.ts | 64 ++++++++++++++--------------- src/statusbar.ts | 66 ++++++++++++++++++++---------- src/test/suite/common.ts | 2 +- src/test/suite/stateSuite.ts | 79 ++++++++++++++++++++++++++++++++---- 4 files changed, 148 insertions(+), 63 deletions(-) diff --git a/src/repository.ts b/src/repository.ts index 60c7162..061172b 100644 --- a/src/repository.ts +++ b/src/repository.ts @@ -21,7 +21,6 @@ import { Commit, CommitDetails, ConfigKey, - Distinct, FossilBranch, FossilCheckin, FossilClass, @@ -71,8 +70,6 @@ import { localize } from './main'; import type { ExecFailure, ExecResult, Reason } from './fossilExecutable'; const iconsRootPath = path.join(path.dirname(__dirname), 'resources', 'icons'); -export type Changes = Distinct; - type AvailableIcons = | 'status-added' | 'status-clean' @@ -279,6 +276,9 @@ export class Repository implements IDisposable, InteractionAPI { return this._sourceControl; } + /** + * An operation started or stopped running + */ @memoize private get onDidChangeOperations(): Event { return anyEvent( @@ -715,8 +715,9 @@ export class Repository implements IDisposable, InteractionAPI { async update(checkin?: FossilCheckin): Promise { // Update command can change everything - await this.runWithProgress(UpdateAll, () => - this.repository.update(checkin) + await this.runWithProgress( + { syncText: 'Updating...', ...UpdateAll }, + () => this.repository.update(checkin) ); } @@ -886,7 +887,8 @@ export class Repository implements IDisposable, InteractionAPI { private async runWithProgress( sideEffects: SideEffects, - runOperation: () => Promise = () => Promise.resolve(null) + runOperation: () => Promise = () => Promise.resolve(null), + runSideEffects: (arg0: T) => boolean = () => true ): Promise { if (this.state !== RepositoryState.Idle) { throw new Error('Repository not initialized'); @@ -901,10 +903,12 @@ export class Repository implements IDisposable, InteractionAPI { try { const operationResult = await runOperation(); - await this.updateModelState( - sideEffects, - 'Triggered by previous operation' as Reason - ); + if (runSideEffects(operationResult)) { + await this.updateModelState( + sideEffects, + 'Triggered by previous operation' as Reason + ); + } return operationResult; } finally { this._operations.delete(key); @@ -1083,7 +1087,7 @@ export class Repository implements IDisposable, InteractionAPI { clearTimeout(this.autoSyncTimer); const nextSyncTime = new Date(Date.now() + interval); this.statusBar.onSyncTimeUpdated(nextSyncTime); - this.autoSyncTimer = setTimeout(() => this.sync(), interval); + this.autoSyncTimer = setTimeout(() => this.periodicSync(), interval); } /** @@ -1091,38 +1095,32 @@ export class Repository implements IDisposable, InteractionAPI { * and updates the status bar */ private async updateChanges(reason: Reason): Promise { - const updateOutput = await this.repository.update( + const updateResult = await this.repository.update( undefined, true, reason ); - if (updateOutput.exitCode) { - this.statusBar.onChangesUpdated( - 'error' as Changes, - updateOutput.stderr - ); - } else { - const match = updateOutput.stdout.match(/^changes:\s*(.*)/m); - const changes = (match ? match[1] : 'unknown status') as Changes; - this.statusBar.onChangesUpdated(changes); - } + this.statusBar.onChangesReady(updateResult); } - /** - * runs `fossil sync`. when 'auto' is set to true, errors are ignored - * and exec reason is set to 'periodic sync'. - * after that the `changes` are updated and auto sync is rescheduled - */ - async sync(auto?: true): Promise { - await this.repository.exec( - ['sync'], - auto ? ('periodic sync' as Reason) : undefined, - { logErrors: !auto } - ); + async periodicSync(): Promise { + await this.repository.exec(['sync'], 'periodic sync' as Reason, { + logErrors: false, + }); await this.updateChanges('sync happened' as Reason); this.updateAutoSyncInterval(typedConfig.autoSyncIntervalMs); } + async sync(): Promise { + const res = await this.runWithProgress( + { changes: true, syncText: 'Syncing' }, + async () => this.repository.exec(['sync']), + res => !res.exitCode + ); + this.statusBar.onSyncReady(res); + this.updateAutoSyncInterval(typedConfig.autoSyncIntervalMs); + } + dispose(): void { dispose(this.disposables); } diff --git a/src/statusbar.ts b/src/statusbar.ts index ae022a9..4d61e19 100644 --- a/src/statusbar.ts +++ b/src/statusbar.ts @@ -4,10 +4,13 @@ *--------------------------------------------------------------------------------------------*/ import type { Command, SourceControl } from 'vscode'; -import { Repository, Changes } from './repository'; +import { Repository } from './repository'; import { ageFromNow, Old } from './humanise'; import { localize } from './main'; -import { FossilStdErr } from './fossilExecutable'; +import { ExecResult } from './fossilExecutable'; +import { Distinct } from './openedRepository'; + +type Changes = Distinct; /** * A bar with 'sync' icon; @@ -19,21 +22,41 @@ import { FossilStdErr } from './fossilExecutable'; * - handle case when there's no sync URL */ class SyncBar { + private icon: 'sync' | 'warning' = 'sync'; // 'sync-ignored' is nice, but not intuitive private text = ''; - private errorText: FossilStdErr | undefined; - private changes: Changes = '' as Changes; + private syncMessage: `${string}\n` | '' = ''; + /** + * match for /changes:\s*(string)/ + * like `17 files modified.` or ` None. Already up-to-date` + */ + private changes: string = ''; // private nextSyncTime: Date | undefined; // undefined = no auto syncing constructor(private repository: Repository) {} - /** - * @param changes like `17 files modified.` or ` None. Already up-to-date` - */ - public onChangesUpdated(changes: Changes, errorText?: FossilStdErr) { - const numChanges = changes.match(/\d+/); - this.text = numChanges && errorText === undefined ? numChanges[0] : ''; - this.changes = changes; - this.errorText = errorText; + public onChangesReady(updateResult: ExecResult) { + if (!updateResult.exitCode) { + const match = updateResult.stdout.match(/^changes:\s*((\d*).*)/m); + this.changes = (match?.[1] ?? 'unknown changes') as Changes; + this.text = match?.[2] ?? ''; // digits of nothing + } else { + this.changes = this.text = ''; + } + } + + public onSyncReady(result: ExecResult) { + this.icon = 'sync'; + if (!result.exitCode) { + this.syncMessage = ''; + } else { + if (/^Usage: /.test(result.stderr)) { + // likely only local repo + this.syncMessage = 'repository with no remote\n'; + } else { + this.icon = 'warning'; + this.syncMessage = `Sync error: ${result.stderr}\n`; + } + } } public onSyncTimeUpdated(date: Date | undefined) { @@ -41,17 +64,13 @@ class SyncBar { } public get command(): Command { - const message = this.nextSyncTime + const timeMessage = this.nextSyncTime ? `Next sync ${this.nextSyncTime.toTimeString().split(' ')[0]}` : `Auto sync disabled`; - const tooltip = - this.errorText === undefined - ? `${this.changes}\n${message}\nUpdate` - : `Error: ${this.errorText}`; return { command: 'fossil.update', - title: `$(sync) ${this.text}`.trim(), - tooltip: tooltip, + title: `$(${this.icon}) ${this.text}`.trim(), + tooltip: `${timeMessage}\n${this.syncMessage}${this.changes}\nUpdate`, arguments: [this.repository satisfies Repository], }; } @@ -104,8 +123,8 @@ export class StatusBarCommands { this.update(); } - public onChangesUpdated(changes: Changes, errorText?: FossilStdErr) { - this.syncBar.onChangesUpdated(changes, errorText); + public onChangesReady(updateResult: ExecResult) { + this.syncBar.onChangesReady(updateResult); this.update(); } @@ -114,6 +133,11 @@ export class StatusBarCommands { this.update(); } + public onSyncReady(syncResult: ExecResult) { + this.syncBar.onSyncReady(syncResult); + this.update(); + } + /** * Should be called whenever commands text/actions/tooltips * are updated diff --git a/src/test/suite/common.ts b/src/test/suite/common.ts index a2b7b52..0bbf768 100644 --- a/src/test/suite/common.ts +++ b/src/test/suite/common.ts @@ -190,7 +190,7 @@ export function fakeFossilStatus(execStub: ExecStub, status: string): ExecStub { export function fakeFossilBranch( execStub: ExecStub, - branch: 'refresh' + branch: 'refresh' | 'trunk' ): ExecStub { return execStub .withArgs(['branch', 'current']) diff --git a/src/test/suite/stateSuite.ts b/src/test/suite/stateSuite.ts index d4ec357..ecf1182 100644 --- a/src/test/suite/stateSuite.ts +++ b/src/test/suite/stateSuite.ts @@ -4,6 +4,7 @@ import { assertGroups, cleanupFossil, fakeExecutionResult, + fakeFossilBranch, fakeFossilChanges, fakeFossilStatus, getExecStub, @@ -173,25 +174,26 @@ export function StatusBarSuite(this: Suite): void { assert.deepEqual(branchBar.arguments, [getRepository()]); assert.equal(syncBar.command, 'fossil.update'); assert.equal(syncBar.title, '$(sync)'); + assert.ok(syncBar.tooltip); assert.match( - syncBar.tooltip!, - /^None. Already up-to-date\nNext sync \d\d:\d\d:\d\d\nUpdate$/ + syncBar.tooltip, + /^Next sync \d\d:\d\d:\d\d\nNone\. Already up-to-date\nUpdate$/ ); assert.deepEqual(syncBar.arguments, [getRepository()]); }); test('Sync', async () => { const execStub = getExecStub(this.ctx.sandbox); - const syncCall = execStub.withArgs(['sync']).resolves(); + const syncCall = execStub + .withArgs(['sync']) + .resolves(fakeExecutionResult()); const changesCall = fakeFossilChanges(execStub, '18 files modified.'); await commands.executeCommand('fossil.sync'); - sinon.assert.calledOnceWithExactly(syncCall, ['sync'], undefined, { - logErrors: true, - }); + sinon.assert.calledOnceWithExactly(syncCall, ['sync']); sinon.assert.calledOnceWithExactly( changesCall, ['update', '--dry-run', '--latest'], - 'sync happened' as Reason, + 'Triggered by previous operation' as Reason, { logErrors: false } ); const nextSyncString = new Date(N.getTime() + 180 * 1000) @@ -202,7 +204,68 @@ export function StatusBarSuite(this: Suite): void { assert.equal(syncBar.title, '$(sync) 18'); assert.equal( syncBar.tooltip, - `18 files modified.\nNext sync ${nextSyncString}\nUpdate` + `Next sync ${nextSyncString}\n18 files modified.\nUpdate` + ); + }); + + test('Icon spins when sync is in progress', async () => { + const execStub = getExecStub(this.ctx.sandbox); + const syncStub = execStub.withArgs(['sync']).callsFake(async () => { + const syncBar = statusBarCommands()[1]; + assert.equal(syncBar.title, '$(sync~spin)'); + return fakeExecutionResult(); + }); + const changeStub = fakeFossilChanges( + execStub, + 'None. Already up-to-date' + ); + await commands.executeCommand('fossil.sync'); + sinon.assert.calledOnce(syncStub); + sinon.assert.calledOnce(changeStub); + sinon.assert.calledTwice(execStub); + }); + + test('Icon spins when update is in progress', async () => { + const execStub = getExecStub(this.ctx.sandbox); + const syncStub = execStub.withArgs(['update']).callsFake(async () => { + const syncBar = statusBarCommands()[1]; + assert.equal(syncBar.title, '$(sync~spin)'); + return fakeExecutionResult(); + }); + const changeStub = fakeFossilChanges( + execStub, + 'None. Already up-to-date' + ); + const statusStub = fakeFossilStatus(execStub, ''); + const branchStub = fakeFossilBranch(execStub, 'trunk'); + await commands.executeCommand('fossil.update'); + sinon.assert.calledOnce(syncStub); + sinon.assert.calledOnce(changeStub); + sinon.assert.calledOnce(statusStub); + sinon.assert.calledOnce(branchStub); + sinon.assert.callCount(execStub, 4); + }); + + test('Error in tooltip when `sync` failed', async () => { + const execStub = getExecStub(this.ctx.sandbox); + const syncStub = execStub + .withArgs(['sync']) + .resolves( + fakeExecutionResult({ stderr: 'test failure', exitCode: 1 }) + ); + const changeStub = fakeFossilChanges( + execStub, + 'None. Already up-to-date' + ); + await commands.executeCommand('fossil.sync'); + sinon.assert.calledOnce(syncStub); + sinon.assert.notCalled(changeStub); // sync failed, nothing changed + sinon.assert.calledOnce(execStub); + const syncBar = statusBarCommands()[1]; + assert.ok(syncBar.tooltip); + assert.match( + syncBar.tooltip, + /Next sync \d\d:\d\d:\d\d\nSync error: test failure\nNone\. Already up-to-date\nUpdate/ ); }); }