Notifications: if finished before delay starts the notification still shows briefly

[weinand]

If I want to use progress events for operations that may take long in some cases, but are mostly short running, I run into the problem that I do not know upfront whether to send progress events or not.

That means I always have to use progress so that progress is displayed if I need them.

This results in flicker in the notification "bell".

To repro replace the "nextRequest" method of mockDebug by the following:

protected nextRequest(response: DebugProtocol.NextResponse, args: 
 DebugProtocol.NextArguments): void {
		const ID = '' + this._progressId++;
		this.sendEvent(new ProgressStartEvent(ID, "Step"));
		this.sendEvent(new ProgressUpdateEvent(ID, "update"));
		this._runtime.step();
		this.sendEvent(new ProgressEndEvent(ID, "end"));
		this.sendResponse(response);
}

If you now step through a Readme.md the little red dot on the bell flashes and if notifications are open I even see a notification popping up and disappearing immediately.

I suggest that we only show the progress UI if the progress runs longer than some threshold (e.g. 500 ms).

[isidorn]

Makes sense. I actually went with 500ms. Note that explorer shows progress on startup only after 800ms delay and search shows progress only after 300ms. So just for reference.

Fixed via a47badb

[isidorn]

This can be verified via the test plan item #92914 , thus adding label on-testplan

[weinand]

I still see notifications on every step:

see white dot on notification bell.

When opening notifications, the "step" notification is seen.

[isidorn]

We are passing a delay to the notification service here

vscode/src/vs/workbench/contrib/debug/browser/debugProgress.ts

Line 44 in 56df825

delay: 500

@bpasero is there something I am missing in the notification service? delay should be just respected?

[bpasero]

I am not sure I can reproduce an issue with regards to the delay, I tried the following:

Have an extension with the following code:

vscode.window.withProgress({
	location: vscode.ProgressLocation.Notification,
	title: 'Progress Title'
}, step => {
	step.report({
		message: 'Sub message'
	});

	return new Promise(resolve => {
		setTimeout(() => {
			resolve();
		}, 5000);
	});
});

Add an artificial delay of 3000ms here:

vscode/src/vs/workbench/services/progress/browser/progressService.ts

Line 173 in b720350

private withNotificationProgress<P extends Promise<R>, R = unknown>(options: IProgressNotificationOptions, callback: (progress: IProgress<IProgressStep>) => P, onDidCancel?: (choice?: number) => void): P {

e.g.

options = {
	...options,
	delay: 3000
};

The notification with progress only appears after 3 seconds.

[isidorn]

@bpasero thanks for trying it out. Sorry for pinging you before I tried it out - I can also not reproduce this.
I wrote a fake adapter that firest a debug start event, and increased the delay to 5000 ms. I can cleary see the progress starting with the delay of 5000ms.
@weinand can you please create a fork or branch of mock debug that shows this issue so we can investigate further. Thank you

[weinand]

@isidorn I was under the impression that my initial comment for this issue explains the problem in a reproducible way:

To repro replace the "nextRequest" method of mockDebug by the following:

protected nextRequest(response: DebugProtocol.NextResponse, args: 
 DebugProtocol.NextArguments): void {
		const ID = '' + this._progressId++;
		this.sendEvent(new ProgressStartEvent(ID, "Step"));
		this.sendEvent(new ProgressUpdateEvent(ID, "update"));
		this._runtime.step();
		this.sendEvent(new ProgressEndEvent(ID, "end"));
		this.sendResponse(response);
}

Did you actually try to reproduce the issue with this step?

[isidorn]

@weinand no I did not try to repro with that step, I did try now and the issue is the following:
If the progress is reported with a delay, and the progress ends before the delay starts the progress is briefly shown, it should not be shown.

@bpasero to reproduce in your example from your comment above in progressService on line 174 add the following

(<any>options).delay = 500;

In your extension sample, use a timeout of 0, not 5000 for the progressEnd event. So the following:

vscode.window.withProgress({
	location: vscode.ProgressLocation.Notification,
	title: 'Progress Title'
}, step => {
	step.report({
		message: 'Sub message'
	});

	return new Promise(resolve => {
		setTimeout(() => {
			resolve();
		}, 0);
	});
});

[isidorn]

@bpasero a potential solution: here in the setTimeout you need to check if the notification is finished before creating it

vscode/src/vs/workbench/services/progress/browser/progressService.ts

Line 360 in b9413f6

notificationTimeout = setTimeout(() => notificationHandle = createNotification(titleAndMessage!, !!options.silent, step?.increment), options.delay);

[bpasero]

I think this is a consequence of always waiting for 800ms before clearing the timeout. I now only force the notification to show for 800ms unless a delay is provided.

[isidorn]

@bpasero thanks for fixing this.