notifications - revised progress indication for hidden notifications (fix #91469)

src/vs/platform/notification/common/notification.ts

@@ -173,6 +173,13 @@ export interface INotificationHandle {
 	 */
 	readonly onDidClose: Event<void>;
 
+	/**
+	 * Will be fired whenever the visibility of the notification changes.
+	 * A notification can either be visible as toast or inside the notification
+	 * center if it is visible. 
+	 */
+	readonly onDidChangeVisibility: Event<boolean>;
+
 	/**
 	 * Allows to indicate progress on the notification even after the
 	 * notification is already visible.

src/vs/workbench/browser/parts/notifications/notificationsActions.ts

@@ -26,10 +26,8 @@ export class ClearNotificationAction extends Action {
 		super(id, label, 'codicon-close');
 	}
 
-	run(notification: INotificationViewItem): Promise<any> {
+	async run(notification: INotificationViewItem): Promise<any> {
 		this.commandService.executeCommand(CLEAR_NOTIFICATION, notification);
-
-		return Promise.resolve();
 	}
 }
 
@@ -46,10 +44,8 @@ export class ClearAllNotificationsAction extends Action {
 		super(id, label, 'codicon-clear-all');
 	}
 
-	run(notification: INotificationViewItem): Promise<any> {
+	async 	run(notification: INotificationViewItem): Promise<any> {
 		this.commandService.executeCommand(CLEAR_ALL_NOTIFICATIONS);
-
-		return Promise.resolve();
 	}
 }
 
@@ -66,10 +62,8 @@ export class HideNotificationsCenterAction extends Action {
 		super(id, label, 'codicon-chevron-down');
 	}
 
-	run(notification: INotificationViewItem): Promise<any> {
+	async run(notification: INotificationViewItem): Promise<any> {
 		this.commandService.executeCommand(HIDE_NOTIFICATIONS_CENTER);
-
-		return Promise.resolve();
 	}
 }
 
@@ -86,10 +80,8 @@ export class ExpandNotificationAction extends Action {
 		super(id, label, 'codicon-chevron-up');
 	}
 
-	run(notification: INotificationViewItem): Promise<any> {
+	async run(notification: INotificationViewItem): Promise<any> {
 		this.commandService.executeCommand(EXPAND_NOTIFICATION, notification);
-
-		return Promise.resolve();
 	}
 }
 
@@ -106,10 +98,8 @@ export class CollapseNotificationAction extends Action {
 		super(id, label, 'codicon-chevron-down');
 	}
 
-	run(notification: INotificationViewItem): Promise<any> {
+	async run(notification: INotificationViewItem): Promise<any> {
 		this.commandService.executeCommand(COLLAPSE_NOTIFICATION, notification);
-
-		return Promise.resolve();
 	}
 }
 

src/vs/workbench/browser/parts/notifications/notificationsCenter.ts

@@ -100,6 +100,9 @@ export class NotificationsCenter extends Themable implements INotificationsCente
 		// Theming
 		this.updateStyles();
 
+		// Mark as visible
+		this.model.notifications.forEach(notification => notification.updateVisibility(true));
+
 		// Context Key
 		this.notificationsCenterVisibleContextKey.set(true);
 
@@ -115,7 +118,7 @@ export class NotificationsCenter extends Themable implements INotificationsCente
 			clearAllAction.enabled = false;
 		} else {
 			notificationsCenterTitle.textContent = localize('notifications', "Notifications");
-			clearAllAction.enabled = true;
+			clearAllAction.enabled = this.model.notifications.some(notification => !notification.hasProgress);
 		}
 	}
 
@@ -172,20 +175,22 @@ export class NotificationsCenter extends Themable implements INotificationsCente
 			return; // only if visible
 		}
 
-		let focusGroup = false;
+		let focusEditor = false;
 
 		// Update notifications list based on event
 		const [notificationsList, notificationsCenterContainer] = assertAllDefined(this.notificationsList, this.notificationsCenterContainer);
 		switch (e.kind) {
 			case NotificationChangeType.ADD:
 				notificationsList.updateNotificationsList(e.index, 0, [e.item]);
+				e.item.updateVisibility(true);
 				break;
 			case NotificationChangeType.CHANGE:
 				notificationsList.updateNotificationsList(e.index, 1, [e.item]);
 				break;
 			case NotificationChangeType.REMOVE:
-				focusGroup = isAncestor(document.activeElement, notificationsCenterContainer);
+				focusEditor = isAncestor(document.activeElement, notificationsCenterContainer);
 				notificationsList.updateNotificationsList(e.index, 1);
+				e.item.updateVisibility(false);
 				break;
 		}
 
@@ -197,7 +202,7 @@ export class NotificationsCenter extends Themable implements INotificationsCente
 			this.hide();
 
 			// Restore focus to editor group if we had focus
-			if (focusGroup) {
+			if (focusEditor) {
 				this.editorGroupService.activeGroup.focus();
 			}
 		}
@@ -208,13 +213,16 @@ export class NotificationsCenter extends Themable implements INotificationsCente
 			return; // already hidden
 		}
 
-		const focusGroup = isAncestor(document.activeElement, this.notificationsCenterContainer);
+		const focusEditor = isAncestor(document.activeElement, this.notificationsCenterContainer);
 
 		// Hide
 		this._isVisible = false;
 		removeClass(this.notificationsCenterContainer, 'visible');
 		this.notificationsList.hide();
 
+		// Mark as hidden
+		this.model.notifications.forEach(notification => notification.updateVisibility(false));
+
 		// Context Key
 		this.notificationsCenterVisibleContextKey.set(false);
 
@@ -222,7 +230,7 @@ export class NotificationsCenter extends Themable implements INotificationsCente
 		this._onDidChangeVisibility.fire();
 
 		// Restore focus to editor group if we had focus
-		if (focusGroup) {
+		if (focusEditor) {
 			this.editorGroupService.activeGroup.focus();
 		}
 	}

src/vs/workbench/browser/parts/notifications/notificationsCommands.ts

@@ -75,6 +75,7 @@ export function registerNotificationCommands(center: INotificationsCenterControl
 
 	// Show Notifications Cneter
 	CommandsRegistry.registerCommand(SHOW_NOTIFICATIONS_CENTER, () => {
+		toasts.hide();
 		center.show();
 	});
 
@@ -92,6 +93,7 @@ export function registerNotificationCommands(center: INotificationsCenterControl
 		if (center.isVisible) {
 			center.hide();
 		} else {
+			toasts.hide();
 			center.show();
 		}
 	});

src/vs/workbench/browser/parts/notifications/notificationsToasts.ts

@@ -179,11 +179,8 @@ export class NotificationsToasts extends Themable implements INotificationsToast
 		const toast: INotificationToast = { item, list: notificationList, container: notificationToastContainer, toast: notificationToast, toDispose: itemDisposables };
 		this.mapNotificationToToast.set(item, toast);
 
-		itemDisposables.add(toDisposable(() => {
-			if (this.isToastVisible(toast) && notificationsToastsContainer) {
-				notificationsToastsContainer.removeChild(toast.container);
-			}
-		}));
+		// When disposed, remove as visible
+		itemDisposables.add(toDisposable(() => this.updateToastVisibility(toast, false)));
 
 		// Make visible
 		notificationList.show();
@@ -236,6 +233,9 @@ export class NotificationsToasts extends Themable implements INotificationsToast
 			addClass(notificationToast, 'notification-fade-in-done');
 		}));
 
+		// Mark as visible
+		item.updateVisibility(true);
+
 		// Events
 		if (!this._isVisible) {
 			this._isVisible = true;
@@ -292,12 +292,13 @@ export class NotificationsToasts extends Themable implements INotificationsToast
 	}
 
 	private removeToast(item: INotificationViewItem): void {
+		let focusEditor = false;
+
 		const notificationToast = this.mapNotificationToToast.get(item);
-		let focusGroup = false;
 		if (notificationToast) {
 			const toastHasDOMFocus = isAncestor(document.activeElement, notificationToast.container);
 			if (toastHasDOMFocus) {
-				focusGroup = !(this.focusNext() || this.focusPrevious()); // focus next if any, otherwise focus editor
+				focusEditor = !(this.focusNext() || this.focusPrevious()); // focus next if any, otherwise focus editor
 			}
 
 			// Listeners
@@ -317,7 +318,7 @@ export class NotificationsToasts extends Themable implements INotificationsToast
 			this.doHide();
 
 			// Move focus back to editor group as needed
-			if (focusGroup) {
+			if (focusEditor) {
 				this.editorGroupService.activeGroup.focus();
 			}
 		}
@@ -346,11 +347,11 @@ export class NotificationsToasts extends Themable implements INotificationsToast
 	}
 
 	hide(): void {
-		const focusGroup = this.notificationsToastsContainer ? isAncestor(document.activeElement, this.notificationsToastsContainer) : false;
+		const focusEditor = this.notificationsToastsContainer ? isAncestor(document.activeElement, this.notificationsToastsContainer) : false;
 
 		this.removeToasts();
 
-		if (focusGroup) {
+		if (focusEditor) {
 			this.editorGroupService.activeGroup.focus();
 		}
 	}
@@ -459,12 +460,12 @@ export class NotificationsToasts extends Themable implements INotificationsToast
 					notificationToasts.push(toast);
 					break;
 				case ToastVisibility.HIDDEN:
-					if (!this.isToastVisible(toast)) {
+					if (!this.isToastInDOM(toast)) {
 						notificationToasts.push(toast);
 					}
 					break;
 				case ToastVisibility.VISIBLE:
-					if (this.isToastVisible(toast)) {
+					if (this.isToastInDOM(toast)) {
 						notificationToasts.push(toast);
 					}
 					break;
@@ -530,7 +531,7 @@ export class NotificationsToasts extends Themable implements INotificationsToast
 
 			// In order to measure the client height, the element cannot have display: none
 			toast.container.style.opacity = '0';
-			this.setVisibility(toast, true);
+			this.updateToastVisibility(toast, true);
 
 			heightToGive -= toast.container.offsetHeight;
 
@@ -542,7 +543,7 @@ export class NotificationsToasts extends Themable implements INotificationsToast
 			}
 
 			// Hide or show toast based on context
-			this.setVisibility(toast, makeVisible);
+			this.updateToastVisibility(toast, makeVisible);
 			toast.container.style.opacity = '';
 
 			if (makeVisible) {
@@ -551,20 +552,24 @@ export class NotificationsToasts extends Themable implements INotificationsToast
 		});
 	}
 
-	private setVisibility(toast: INotificationToast, visible: boolean): void {
-		if (this.isToastVisible(toast) === visible) {
+	private updateToastVisibility(toast: INotificationToast, visible: boolean): void {
+		if (this.isToastInDOM(toast) === visible) {
 			return;
 		}
 
+		// Update visibility in DOM
 		const notificationsToastsContainer = assertIsDefined(this.notificationsToastsContainer);
 		if (visible) {
 			notificationsToastsContainer.appendChild(toast.container);
 		} else {
 			notificationsToastsContainer.removeChild(toast.container);
 		}
+
+		// Update visibility in model
+		toast.item.updateVisibility(visible);
 	}
 
-	private isToastVisible(toast: INotificationToast): boolean {
+	private isToastInDOM(toast: INotificationToast): boolean {
 		return !!toast.container.parentElement;
 	}
 }

src/vs/workbench/common/notifications.ts

@@ -92,6 +92,9 @@ export class NotificationHandle extends Disposable implements INotificationHandl
 	private readonly _onDidClose = this._register(new Emitter<void>());
 	readonly onDidClose = this._onDidClose.event;
 
+	private readonly _onDidChangeVisibility = this._register(new Emitter<boolean>());
+	readonly onDidChangeVisibility = this._onDidChangeVisibility.event;
+
 	constructor(private readonly item: INotificationViewItem, private readonly onClose: (item: INotificationViewItem) => void) {
 		super();
 
@@ -99,6 +102,11 @@ export class NotificationHandle extends Disposable implements INotificationHandl
 	}
 
 	private registerListeners(): void {
+
+		// Visibility
+		this._register(this.item.onDidChangeVisibility(visible => this._onDidChangeVisibility.fire(visible)));
+
+		// Closing
 		Event.once(this.item.onDidClose)(() => {
 			this._onDidClose.fire();
 
@@ -265,6 +273,7 @@ export interface INotificationViewItem {
 
 	readonly onDidChangeExpansion: Event<void>;
 	readonly onDidClose: Event<void>;
+	readonly onDidChangeVisibility: Event<boolean>;
 	readonly onDidChangeLabel: Event<INotificationViewItemLabelChangeEvent>;
 
 	expand(): void;
@@ -275,6 +284,8 @@ export interface INotificationViewItem {
 	updateMessage(message: NotificationMessage): void;
 	updateActions(actions?: INotificationActions): void;
 
+	updateVisibility(visible: boolean): void;
+
 	close(): void;
 
 	equals(item: INotificationViewItem): boolean;
@@ -398,6 +409,7 @@ export class NotificationViewItem extends Disposable implements INotificationVie
 	private static readonly MAX_MESSAGE_LENGTH = 1000;
 
 	private _expanded: boolean | undefined;
+	private _visible: boolean = false;
 
 	private _actions: INotificationActions | undefined;
 	private _progress: NotificationViewItemProgress | undefined;
@@ -411,6 +423,9 @@ export class NotificationViewItem extends Disposable implements INotificationVie
 	private readonly _onDidChangeLabel = this._register(new Emitter<INotificationViewItemLabelChangeEvent>());
 	readonly onDidChangeLabel = this._onDidChangeLabel.event;
 
+	private readonly _onDidChangeVisibility = this._register(new Emitter<boolean>());
+	readonly onDidChangeVisibility = this._onDidChangeVisibility.event;
+
 	static create(notification: INotification, filter: NotificationsFilter = NotificationsFilter.OFF): INotificationViewItem | undefined {
 		if (!notification || !notification.message || isPromiseCanceledError(notification.message)) {
 			return undefined; // we need a message to show
@@ -600,6 +615,14 @@ export class NotificationViewItem extends Disposable implements INotificationVie
 		this._onDidChangeLabel.fire({ kind: NotificationViewItemLabelKind.ACTIONS });
 	}
 
+	updateVisibility(visible: boolean): void {
+		if (this._visible !== visible) {
+			this._visible = visible;
+
+			this._onDidChangeVisibility.fire(visible);
+		}
+	}
+
 	expand(): void {
 		if (this._expanded || !this.canCollapse) {
 			return;

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

@@ -6,7 +6,7 @@
 import 'vs/css!./media/progressService';
 
 import { localize } from 'vs/nls';
-import { IDisposable, dispose, DisposableStore, MutableDisposable, Disposable } from 'vs/base/common/lifecycle';
+import { IDisposable, dispose, DisposableStore, MutableDisposable, Disposable, toDisposable } from 'vs/base/common/lifecycle';
 import { IProgressService, IProgressOptions, IProgressStep, ProgressLocation, IProgress, Progress, IProgressCompositeOptions, IProgressNotificationOptions, IProgressRunner, IProgressIndicator, IProgressWindowOptions } from 'vs/platform/progress/common/progress';
 import { IViewletService } from 'vs/workbench/services/viewlet/browser/viewlet';
 import { StatusbarAlignment, IStatusbarService } from 'vs/workbench/services/statusbar/common/statusbar';
@@ -191,6 +191,38 @@ export class ProgressService extends Disposable implements IProgressService {
 			}
 		};
 
+		const createWindowProgress = () => {
+
+			// Create a promise that we can resolve as needed
+			// when the outside calls dispose on us
+			let promiseResolve: () => void;
+			const promise = new Promise<R>(resolve => promiseResolve = resolve);
+
+			this.withWindowProgress<R>({
+				location: ProgressLocation.Window,
+				title: options.title,
+				command: 'notifications.showList'
+			}, progress => {
+
+				// Apply any progress that was made already
+				if (progressStateModel.step) {
+					progress.report(progressStateModel.step);
+				}
+
+				// Continue to report progress as it happens
+				const onDidReportListener = progressStateModel.onDidReport(step => progress.report(step));
+				promise.finally(() => onDidReportListener.dispose());
+
+				// When the progress model gets disposed, we are done as well
+				Event.once(progressStateModel.onDispose)(() => promiseResolve());
+
+				return promise;
+			});
+
+			// Dispose means completing our promise
+			return toDisposable(() => promiseResolve());
+		};
+
 		const createNotification = (message: string, increment?: number): INotificationHandle => {
 			const notificationDisposables = new DisposableStore();
 
@@ -229,7 +261,7 @@ export class ProgressService extends Disposable implements IProgressService {
 				primaryActions.push(cancelAction);
 			}
 
-			const handle = this.notificationService.notify({
+			const notification = this.notificationService.notify({
 				severity: Severity.Info,
 				message,
 				source: options.source,
@@ -237,12 +269,26 @@ export class ProgressService extends Disposable implements IProgressService {
 				progress: typeof increment === 'number' && increment >= 0 ? { total: 100, worked: increment } : { infinite: true }
 			});
 
-			updateProgress(handle, increment);
+			// Switch to window based progress once the notification
+			// changes visibility to hidden and is still ongoing.
+			// Remove that window based progress once the notification
+			// shows again.
+			let windowProgressDisposable: IDisposable | undefined = undefined;
+			notificationDisposables.add(notification.onDidChangeVisibility(visible => {
+
+				// Clear any previous running window progress
+				dispose(windowProgressDisposable);
+
+				// Create new window progress if notification got hidden
+				if (!visible && !progressStateModel.done) {
+					windowProgressDisposable = createWindowProgress();
+				}
+			}));
 
 			// Clear upon dispose
-			Event.once(handle.onDidClose)(() => notificationDisposables.dispose());
+			Event.once(notification.onDidClose)(() => notificationDisposables.dispose());
 
-			return handle;
+			return notification;
 		};
 
 		const updateProgress = (notification: INotificationHandle, increment?: number): void => {

src/vs/workbench/test/common/notifications.test.ts

@@ -98,6 +98,17 @@ suite('Notifications', () => {
 
 		assert.equal(called, 1);
 
+		called = 0;
+		item1.onDidChangeVisibility(e => {
+			called++;
+		});
+
+		item1.updateVisibility(true);
+		item1.updateVisibility(false);
+		item1.updateVisibility(false);
+
+		assert.equal(called, 2);
+
 		called = 0;
 		item1.onDidClose(() => {
 			called++;