Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CHANGE @W-16866345@ Spinners retain useful information in their done-state #1637

Merged
merged 1 commit into from
Oct 1, 2024
Merged

CHANGE @W-16866345@ Spinners retain useful information in their done-state #1637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions messages/progress-event-listener.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
# selection-spinner.action
Selecting rules

# selection-spinner.status
# selection-spinner.in-progress-status
Eligible engines: %s; Completion: %d%; Elapsed time: %ds

# selection-spinner.finished-status
done. Selected rules from %s.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Selected" chosen to match spinner's existing use of "Selecting rules".


# execution-spinner.action
Executing rules

Expand All @@ -13,6 +16,5 @@ Executing rules
# execution-spinner.engine-status
- %s at %d% completion.

# base-spinner.done
done

# execution-spinner.finished-status
done. Executed rules from %s.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the spinner already uses "Executing rules", it seemed correct to use "Executed rules" here instead of something like "Ran".

28 changes: 20 additions & 8 deletions src/lib/listeners/ProgressEventListener.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,28 +31,28 @@ abstract class ProgressSpinner {
return;
}
this.startTime = Date.now();
this.display.spinnerStart(action, this.createSpinnerStatus());
this.display.spinnerStart(action, this.createInProgressSpinnerStatus());
if (this.tickTime > 0) {
// `setInterval` means the callback will be called repeatedly. This allows us to automatically refresh the
// spinner on a regular interval even if nothing happened. This is primarily useful for incrementing a timer,
// so the user doesn't feel like the system is frozen.
this.tickIntervalId = setInterval(() => {
this.display.spinnerUpdate(this.createSpinnerStatus());
this.display.spinnerUpdate(this.createInProgressSpinnerStatus());
}, this.tickTime);
}
this._isSpinning = true;
}

protected updateSpinner(): void {
this.display.spinnerUpdate(this.createSpinnerStatus());
this.display.spinnerUpdate(this.createInProgressSpinnerStatus());
}

protected stopSpinning(): void {
if (this.tickIntervalId) {
clearInterval(this.tickIntervalId);
}
if (this._isSpinning) {
this.display.spinnerStop(getMessage(BundleName.ProgressEventListener, 'base-spinner.done'));
this.display.spinnerStop(this.createFinishedSpinnerStatus());
this._isSpinning = false;
}
}
Expand All @@ -69,7 +69,9 @@ abstract class ProgressSpinner {
return Math.floor((Date.now() - this.startTime) / 1000);
}

protected abstract createSpinnerStatus(): string;
protected abstract createInProgressSpinnerStatus(): string;

protected abstract createFinishedSpinnerStatus(): string;
}

export class RuleSelectionProgressSpinner extends ProgressSpinner implements ProgressEventListener {
Expand Down Expand Up @@ -128,12 +130,17 @@ export class RuleSelectionProgressSpinner extends ProgressSpinner implements Pro
}
}

protected createSpinnerStatus(): string {
protected createInProgressSpinnerStatus(): string {
const aggregateCompletionPercentage: number =
Math.floor(this.completionPercentages.reduce((prevTotal, next) => prevTotal + next, 0) / this.completionPercentages.length);
return getMessage(BundleName.ProgressEventListener, 'selection-spinner.status',
return getMessage(BundleName.ProgressEventListener, 'selection-spinner.in-progress-status',
[[...this.engineNames.keys()].join(', '), aggregateCompletionPercentage, this.getSecondsSpentSpinning()]);
}

protected createFinishedSpinnerStatus(): string {
return getMessage(BundleName.ProgressEventListener, 'selection-spinner.finished-status',
[[...this.engineNames.keys()].join(', ')]);
}
}

export class EngineRunProgressSpinner extends ProgressSpinner implements ProgressEventListener {
Expand Down Expand Up @@ -189,7 +196,7 @@ export class EngineRunProgressSpinner extends ProgressSpinner implements Progres
this.updateSpinner();
}

protected createSpinnerStatus(): string {
protected createInProgressSpinnerStatus(): string {
const secondsRunning = this.getSecondsSpentSpinning();
const totalEngines = this.progressMap.size;
let unfinishedEngines = 0;
Expand All @@ -205,4 +212,9 @@ export class EngineRunProgressSpinner extends ProgressSpinner implements Progres
...engineLines
].join('\n');
}

protected createFinishedSpinnerStatus(): string {
return getMessage(BundleName.ProgressEventListener, 'execution-spinner.finished-status',
[[...this.progressMap.keys()].join(', ')]);
}
}
12 changes: 6 additions & 6 deletions test/lib/listeners/ProgressEventListener.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe('ProgressEventListener implementations', () => {
expect(percentagesInOrder).toEqual([0, 25, 50, 100]);
const endEvent = displayEvents[displayEvents.length - 1];
expect(endEvent).toHaveProperty('type', DisplayEventType.SPINNER_STOP);
expect(endEvent.data).toContain('done');
expect(endEvent.data).toContain(`done. Selected rules from stubEngine1, stubEngine2.`);
});

it('Properly aggregates percentages across multiple Cores', async () => {
Expand Down Expand Up @@ -123,7 +123,7 @@ describe('ProgressEventListener implementations', () => {
expect(percentagesInOrder).toEqual([0, 12, 25, 50, 62, 75, 100]);
const endEvent = displayEvents[displayEvents.length - 1];
expect(endEvent).toHaveProperty('type', DisplayEventType.SPINNER_STOP);
expect(endEvent.data).toContain('done');
expect(endEvent.data).toContain('done. Selected rules from stubEngine1, stubEngine2.');
});

it('Properly interleaves progress updates with ticking', async () => {
Expand Down Expand Up @@ -159,7 +159,7 @@ describe('ProgressEventListener implementations', () => {
expect(percentagesInOrder).toEqual([0, 20, 40, 50, 60, 80, 100]);
const endEvent = displayEvents[displayEvents.length - 1];
expect(endEvent).toHaveProperty('type', DisplayEventType.SPINNER_STOP);
expect(endEvent.data).toContain('done');
expect(endEvent.data).toContain('done. Selected rules from timeableEngine1, timeableEngine2.');
});
});

Expand Down Expand Up @@ -252,7 +252,7 @@ describe('ProgressEventListener implementations', () => {
expect(percentagesInOrder).toEqual([0, 50, 100]);
const endEvent = displayEvents[displayEvents.length - 1];
expect(endEvent).toHaveProperty('type', DisplayEventType.SPINNER_STOP);
expect(endEvent.data).toContain('done');
expect(endEvent.data).toContain('done. Executed rules from stubEngine1.');
});

it('Properly interleaves progress updates from multiple engines', async () => {
Expand Down Expand Up @@ -296,7 +296,7 @@ describe('ProgressEventListener implementations', () => {
// The final event should be the Stop event.
const endEvent = displayEvents[displayEvents.length - 1];
expect(endEvent).toHaveProperty('type', DisplayEventType.SPINNER_STOP);
expect(endEvent.data).toContain('done');
expect(endEvent.data).toContain('done. Executed rules from timeableEngine1, timeableEngine2.');
});

it('Properly interleaves progress updates with ticking', async () => {
Expand Down Expand Up @@ -332,7 +332,7 @@ describe('ProgressEventListener implementations', () => {
expect(percentagesInOrder).toEqual([0, 50, 100]);
const endEvent = displayEvents[displayEvents.length - 1];
expect(endEvent).toHaveProperty('type', DisplayEventType.SPINNER_STOP);
expect(endEvent.data).toContain('done');
expect(endEvent.data).toContain('done. Executed rules from timeableEngine1.');
}, 10000);

// There's currently no need for this Spinner to accept multiple Cores, so we've opted to not implement that
Expand Down