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.

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

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

Closed
weinand opened this issue Mar 19, 2020 · 11 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workbench-notifications Notification widget issues
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Mar 19, 2020

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).

@weinand weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Mar 19, 2020
@isidorn
Copy link
Contributor

isidorn commented Mar 19, 2020

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 isidorn added this to the March 2020 milestone Mar 19, 2020
@isidorn isidorn added the feature-request Request for new features or functionality label Mar 19, 2020
@isidorn isidorn closed this as completed Mar 19, 2020
@isidorn
Copy link
Contributor

isidorn commented Mar 24, 2020

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

@weinand
Copy link
Contributor Author

weinand commented Apr 2, 2020

I still see notifications on every step:

2020-04-02_16-53-42 (1)

see white dot on notification bell.

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

@weinand weinand reopened this Apr 2, 2020
@weinand weinand added the verification-found Issue verification failed label Apr 2, 2020
@isidorn
Copy link
Contributor

isidorn commented Apr 2, 2020

We are passing a delay to the notification service here

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

@isidorn isidorn modified the milestones: March 2020, April 2020 Apr 2, 2020
@bpasero
Copy link
Member

bpasero commented Apr 2, 2020

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:

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
Copy link
Contributor

isidorn commented Apr 2, 2020

@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
Copy link
Contributor Author

weinand commented Apr 2, 2020

@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
Copy link
Contributor

isidorn commented Apr 3, 2020

@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
Copy link
Contributor

isidorn commented Apr 3, 2020

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

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

@isidorn isidorn added workbench-notifications Notification widget issues and removed debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality on-testplan verification-found Issue verification failed labels Apr 3, 2020
@isidorn isidorn changed the title Do not show progress UI if progress sequence is short Notifications: if finished before delay starts the notifciation still shows briefly Apr 3, 2020
@weinand weinand changed the title Notifications: if finished before delay starts the notifciation still shows briefly Notifications: if finished before delay starts the notification still shows briefly Apr 3, 2020
@bpasero bpasero added the bug Issue identified by VS Code Team member as probable bug label Apr 6, 2020
@bpasero bpasero closed this as completed in 3764d68 Apr 6, 2020
@bpasero
Copy link
Member

bpasero commented Apr 6, 2020

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
Copy link
Contributor

isidorn commented Apr 6, 2020

@bpasero thanks for fixing this.

@roblourens roblourens added the verified Verification succeeded label Apr 29, 2020
@github-actions github-actions bot locked and limited conversation to collaborators May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workbench-notifications Notification widget issues
Projects
None yet
Development

No branches or pull requests

4 participants