Skip to content

Commit 9cf5c6e

Browse files
author
Benjamin Pasero
committed
notifications - do not add a "close" action for extensions
1 parent 0d05c78 commit 9cf5c6e

6 files changed

Lines changed: 73 additions & 90 deletions

File tree

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,9 @@ export interface INotificationProgress {
9696
export interface INotificationHandle extends IDisposable {
9797

9898
/**
99-
* Will be fired once the notification hides.
99+
* Will be fired once the notification is disposed.
100100
*/
101-
readonly onDidHide: Event<void>;
101+
readonly onDidDispose: Event<void>;
102102

103103
/**
104104
* Allows to indicate progress on the notification even after the
@@ -160,18 +160,18 @@ export interface INotificationService {
160160
export class NoOpNotification implements INotificationHandle {
161161
readonly progress = new NoOpProgress();
162162

163-
private _onDidHide: Emitter<void> = new Emitter();
163+
private _onDidDispose: Emitter<void> = new Emitter();
164164

165-
public get onDidHide(): Event<void> {
166-
return this._onDidHide.event;
165+
public get onDidDispose(): Event<void> {
166+
return this._onDidDispose.event;
167167
}
168168

169169
updateSeverity(severity: Severity): void { }
170170
updateMessage(message: NotificationMessage): void { }
171171
updateActions(actions?: INotificationActions): void { }
172172

173173
dispose(): void {
174-
this._onDidHide.dispose();
174+
this._onDidDispose.dispose();
175175
}
176176
}
177177

‎src/vs/workbench/api/electron-browser/mainThreadMessageService.ts‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,17 @@ import { MainThreadMessageServiceShape, MainContext, IExtHostContext, MainThread
1111
import { extHostNamedCustomer } from 'vs/workbench/api/electron-browser/extHostCustomers';
1212
import { IExtensionDescription } from 'vs/platform/extensions/common/extensions';
1313
import { IChoiceService } from 'vs/platform/dialogs/common/dialogs';
14-
import { INotificationService, INotificationHandle } from 'vs/platform/notification/common/notification';
14+
import { INotificationService } from 'vs/platform/notification/common/notification';
15+
import { once } from 'vs/base/common/event';
16+
import { ICommandService } from 'vs/platform/commands/common/commands';
1517

1618
@extHostNamedCustomer(MainContext.MainThreadMessageService)
1719
export class MainThreadMessageService implements MainThreadMessageServiceShape {
1820

1921
constructor(
2022
extHostContext: IExtHostContext,
2123
@INotificationService private readonly _notificationService: INotificationService,
24+
@ICommandService private readonly _commandService: ICommandService,
2225
@IChoiceService private readonly _choiceService: IChoiceService
2326
) {
2427
//
@@ -40,40 +43,41 @@ export class MainThreadMessageService implements MainThreadMessageServiceShape {
4043

4144
return new Promise<number>(resolve => {
4245

43-
let messageHandle: INotificationHandle;
4446
let actions: MessageItemAction[] = [];
45-
let hasCloseAffordance = false;
4647

4748
class MessageItemAction extends Action {
4849
constructor(id: string, label: string, handle: number) {
4950
super(id, label, undefined, true, () => {
5051
resolve(handle);
51-
messageHandle.dispose(); // triggers dispose! make sure promise is already resolved
5252
return undefined;
5353
});
5454
}
55-
dispose(): void {
56-
resolve(undefined);
55+
}
56+
57+
class ManageExtensionAction extends Action {
58+
constructor(id: string, label: string, commandService: ICommandService) {
59+
super(id, label, undefined, true, () => {
60+
return commandService.executeCommand('_extensions.manage', id);
61+
});
5762
}
5863
}
5964

6065
commands.forEach(command => {
61-
if (command.isCloseAffordance === true) {
62-
hasCloseAffordance = true;
63-
}
6466
actions.push(new MessageItemAction('_extension_message_handle_' + command.handle, command.title, command.handle));
6567
});
6668

67-
if (!hasCloseAffordance) {
68-
actions.push(new MessageItemAction('__close', nls.localize('close', "Close"), undefined));
69-
}
70-
71-
messageHandle = this._notificationService.notify({
69+
const messageHandle = this._notificationService.notify({
7270
severity,
7371
message,
74-
actions: { primary: actions },
72+
actions: { primary: actions, secondary: extension ? [new ManageExtensionAction(extension.id, nls.localize('manageExtension', "Manage Extension"), this._commandService)] : [] },
7573
source: extension && `${extension.displayName || extension.name}`
7674
});
75+
76+
// if promise has not been resolved yet, now is the time to ensure a return value
77+
// otherwise if already resolved it means the user clicked one of the buttons
78+
once(messageHandle.onDidDispose)(() => {
79+
resolve(undefined);
80+
});
7781
});
7882
}
7983

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,21 +33,21 @@ export interface INotificationChangeEvent {
3333
}
3434

3535
export class NotificationHandle implements INotificationHandle {
36-
private _onDidHide: Emitter<void> = new Emitter();
36+
private _onDidDispose: Emitter<void> = new Emitter();
3737

3838
constructor(private item: INotificationViewItem, private disposeItem: (item: INotificationViewItem) => void) {
3939
this.registerListeners();
4040
}
4141

4242
private registerListeners(): void {
4343
once(this.item.onDidDispose)(() => {
44-
this._onDidHide.fire();
45-
this._onDidHide.dispose();
44+
this._onDidDispose.fire();
45+
this._onDidDispose.dispose();
4646
});
4747
}
4848

49-
public get onDidHide(): Event<void> {
50-
return this._onDidHide.event;
49+
public get onDidDispose(): Event<void> {
50+
return this._onDidDispose.event;
5151
}
5252

5353
public get progress(): INotificationProgress {
@@ -68,7 +68,7 @@ export class NotificationHandle implements INotificationHandle {
6868

6969
public dispose(): void {
7070
this.disposeItem(this.item);
71-
this._onDidHide.dispose();
71+
this._onDidDispose.dispose();
7272
}
7373
}
7474

‎src/vs/workbench/services/dialogs/electron-browser/dialogs.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ export class DialogService implements IChoiceService, IConfirmationService {
150150
handle = this.notificationService.notify({ severity, message, actions });
151151

152152
// Cancel promise when notification gets disposed
153-
once(handle.onDidHide)(() => promise.cancel());
153+
once(handle.onDidDispose)(() => promise.cancel());
154154

155155
}, () => handle.dispose());
156156

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ suite('Notifications', () => {
143143
assert.equal(model.notifications.length, 3);
144144

145145
let called = 0;
146-
item1Handle.onDidHide(() => {
146+
item1Handle.onDidDispose(() => {
147147
called++;
148148
});
149149

‎src/vs/workbench/test/electron-browser/api/extHostMessagerService.test.ts‎

Lines changed: 40 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66
'use strict';
77

88
import * as assert from 'assert';
9-
import { IAction } from 'vs/base/common/actions';
109
import { MainThreadMessageService } from 'vs/workbench/api/electron-browser/mainThreadMessageService';
11-
import { TPromise as Promise } from 'vs/base/common/winjs.base';
10+
import { TPromise as Promise, TPromise } from 'vs/base/common/winjs.base';
1211
import { IChoiceService } from 'vs/platform/dialogs/common/dialogs';
13-
import { INotificationService, INotification } from 'vs/platform/notification/common/notification';
12+
import { INotificationService, INotification, NoOpNotification, INotificationHandle } from 'vs/platform/notification/common/notification';
13+
import { ICommandService } from 'vs/platform/commands/common/commands';
1414

1515
const emptyChoiceService = new class implements IChoiceService {
1616
_serviceBrand: 'choiceService';
@@ -19,6 +19,13 @@ const emptyChoiceService = new class implements IChoiceService {
1919
}
2020
};
2121

22+
const emptyCommandService: ICommandService = {
23+
_serviceBrand: undefined,
24+
onWillExecuteCommand: () => ({ dispose: () => { } }),
25+
executeCommand: (commandId: string, ...args: any[]): TPromise<any> => {
26+
return TPromise.as(void 0);
27+
}
28+
};
2229

2330
const emptyNotificationService = new class implements INotificationService {
2431
_serviceBrand: 'notificiationService';
@@ -36,74 +43,46 @@ const emptyNotificationService = new class implements INotificationService {
3643
}
3744
};
3845

46+
class EmptyNotificationService implements INotificationService {
47+
48+
_serviceBrand: any;
49+
50+
constructor(private withNotify: (notification: INotification) => void) {
51+
}
52+
53+
notify(notification: INotification): INotificationHandle {
54+
this.withNotify(notification);
55+
56+
return new NoOpNotification();
57+
}
58+
info(message: any): void {
59+
throw new Error('Method not implemented.');
60+
}
61+
warn(message: any): void {
62+
throw new Error('Method not implemented.');
63+
}
64+
error(message: any): void {
65+
throw new Error('Method not implemented.');
66+
}
67+
}
68+
3969
suite('ExtHostMessageService', function () {
4070

4171
test('propagte handle on select', function () {
4272

43-
let service = new MainThreadMessageService(null, {
44-
notify(m: INotification) {
45-
assert.equal(m.actions.primary.length, 1);
46-
setImmediate(() => m.actions.primary[0].run());
47-
return undefined;
48-
}
49-
} as INotificationService, emptyChoiceService);
73+
let service = new MainThreadMessageService(null, new EmptyNotificationService(notification => {
74+
assert.equal(notification.actions.primary.length, 1);
75+
setImmediate(() => notification.actions.primary[0].run());
76+
}), emptyCommandService, emptyChoiceService);
5077

5178
return service.$showMessage(1, 'h', {}, [{ handle: 42, title: 'a thing', isCloseAffordance: true }]).then(handle => {
5279
assert.equal(handle, 42);
5380
});
5481
});
5582

56-
test('isCloseAffordance', function () {
57-
58-
let actions: IAction[];
59-
let service = new MainThreadMessageService(null, {
60-
notify(m: INotification) {
61-
actions = m.actions.primary;
62-
63-
return undefined;
64-
}
65-
} as INotificationService, emptyChoiceService);
66-
67-
// default close action
68-
service.$showMessage(1, '', {}, [{ title: 'a thing', isCloseAffordance: false, handle: 0 }]);
69-
assert.equal(actions.length, 2);
70-
let [first, second] = actions;
71-
assert.equal(first.label, 'a thing');
72-
assert.equal(second.label, 'Close');
73-
74-
// override close action
75-
service.$showMessage(1, '', {}, [{ title: 'a thing', isCloseAffordance: true, handle: 0 }]);
76-
assert.equal(actions.length, 1);
77-
first = actions[0];
78-
assert.equal(first.label, 'a thing');
79-
});
80-
81-
test('hide on select', function () {
82-
83-
let actions: IAction[];
84-
let c: number;
85-
let service = new MainThreadMessageService(null, {
86-
notify(m: INotification) {
87-
c = 0;
88-
actions = m.actions.primary;
89-
return {
90-
dispose: () => {
91-
c += 1;
92-
}
93-
};
94-
}
95-
} as INotificationService, emptyChoiceService);
96-
97-
service.$showMessage(1, '', {}, [{ title: 'a thing', isCloseAffordance: true, handle: 0 }]);
98-
assert.equal(actions.length, 1);
99-
100-
actions[0].run();
101-
assert.equal(c, 1);
102-
});
103-
10483
suite('modal', () => {
10584
test('calls choice service', () => {
106-
const service = new MainThreadMessageService(null, emptyNotificationService, {
85+
const service = new MainThreadMessageService(null, emptyNotificationService, emptyCommandService, {
10786
choose(severity, message, options, modal) {
10887
assert.equal(severity, 1);
10988
assert.equal(message, 'h');
@@ -119,7 +98,7 @@ suite('ExtHostMessageService', function () {
11998
});
12099

121100
test('returns undefined when cancelled', () => {
122-
const service = new MainThreadMessageService(null, emptyNotificationService, {
101+
const service = new MainThreadMessageService(null, emptyNotificationService, emptyCommandService, {
123102
choose(severity, message, options, modal) {
124103
return Promise.as(1);
125104
}
@@ -131,7 +110,7 @@ suite('ExtHostMessageService', function () {
131110
});
132111

133112
test('hides Cancel button when not needed', () => {
134-
const service = new MainThreadMessageService(null, emptyNotificationService, {
113+
const service = new MainThreadMessageService(null, emptyNotificationService, emptyCommandService, {
135114
choose(severity, message, options, modal) {
136115
assert.equal(options.length, 1);
137116
return Promise.as(0);

0 commit comments

Comments
 (0)