notifications - dispose markdown callbacks as needed

src/vs/base/browser/htmlContentRenderer.ts

@@ -12,11 +12,17 @@ import { removeMarkdownEscapes, IMarkdownString } from 'vs/base/common/htmlConte
 import { marked, MarkedRenderer, MarkedOptions } from 'vs/base/common/marked/marked';
 import { IMouseEvent } from 'vs/base/browser/mouseEvent';
 import { assign } from 'vs/base/common/objects';
+import { IDisposable } from 'vs/base/common/lifecycle';
+
+export interface IContentActionHandler {
+	callback: (content: string, event?: IMouseEvent) => void;
+	disposeables: IDisposable[];
+}
 
 export interface RenderOptions {
 	className?: string;
 	inline?: boolean;
-	actionCallback?: (content: string, event?: IMouseEvent) => void;
+	actionHandler?: IContentActionHandler;
 	codeBlockRenderer?: (modeId: string, value: string) => Thenable<string>;
 	codeBlockRenderCallback?: () => void;
 	joinRendererConfiguration?: (renderer: MarkedRenderer) => MarkedOptions;
@@ -39,15 +45,12 @@ export function renderText(text: string, options: RenderOptions = {}): HTMLEleme
 
 export function renderFormattedText(formattedText: string, options: RenderOptions = {}): HTMLElement {
 	const element = createElement(options);
-	_renderFormattedText(element, parseFormattedText(formattedText), options.actionCallback);
+	_renderFormattedText(element, parseFormattedText(formattedText), options.actionHandler);
 	return element;
 }
 
 /**
  * Create html nodes for the given content element.
- *
- * @param content a html element description
- * @param actionCallback a callback function for any action links in the string. Argument is the zero-based index of the clicked action.
  */
 export function renderMarkdown(markdown: IMarkdownString, options: RenderOptions = {}): HTMLElement {
 	const element = createElement(options);
@@ -141,8 +144,8 @@ export function renderMarkdown(markdown: IMarkdownString, options: RenderOptions
 		};
 	}
 
-	if (options.actionCallback) {
-		DOM.addStandardDisposableListener(element, 'click', event => {
+	if (options.actionHandler) {
+		options.actionHandler.disposeables.push(DOM.addStandardDisposableListener(element, 'click', event => {
 			let target = event.target;
 			if (target.tagName !== 'A') {
 				target = target.parentElement;
@@ -153,9 +156,9 @@ export function renderMarkdown(markdown: IMarkdownString, options: RenderOptions
 
 			const href = target.dataset['href'];
 			if (href) {
-				options.actionCallback(href, event);
+				options.actionHandler.callback(href, event);
 			}
-		});
+		}));
 	}
 
 	const markedOptions: MarkedOptions = {
@@ -224,7 +227,7 @@ interface IFormatParseTree {
 	children?: IFormatParseTree[];
 }
 
-function _renderFormattedText(element: Node, treeNode: IFormatParseTree, actionCallback?: (content: string, event?: IMouseEvent) => void) {
+function _renderFormattedText(element: Node, treeNode: IFormatParseTree, actionHandler?: IContentActionHandler) {
 	let child: Node;
 
 	if (treeNode.type === FormatType.Text) {
@@ -236,12 +239,12 @@ function _renderFormattedText(element: Node, treeNode: IFormatParseTree, actionC
 	else if (treeNode.type === FormatType.Italics) {
 		child = document.createElement('i');
 	}
-	else if (treeNode.type === FormatType.Action) {
+	else if (treeNode.type === FormatType.Action && actionHandler) {
 		const a = document.createElement('a');
 		a.href = '#';
-		DOM.addStandardDisposableListener(a, 'click', (event) => {
-			actionCallback(String(treeNode.index), event);
-		});
+		actionHandler.disposeables.push(DOM.addStandardDisposableListener(a, 'click', (event) => {
+			actionHandler.callback(String(treeNode.index), event);
+		}));
 
 		child = a;
 	}
@@ -258,7 +261,7 @@ function _renderFormattedText(element: Node, treeNode: IFormatParseTree, actionC
 
 	if (Array.isArray(treeNode.children)) {
 		treeNode.children.forEach((nodeChild) => {
-			_renderFormattedText(child, nodeChild, actionCallback);
+			_renderFormattedText(child, nodeChild, actionHandler);
 		});
 	}
 }

src/vs/base/test/browser/htmlContent.test.ts

@@ -52,9 +52,12 @@ suite('HtmlContent', () => {
 	test('action', () => {
 		var callbackCalled = false;
 		var result: HTMLElement = renderFormattedText('[[action]]', {
-			actionCallback(content) {
-				assert.strictEqual(content, '0');
-				callbackCalled = true;
+			actionHandler: {
+				callback(content) {
+					assert.strictEqual(content, '0');
+					callbackCalled = true;
+				},
+				disposeables: []
 			}
 		});
 		assert.strictEqual(result.innerHTML, '<a href="#">action</a>');
@@ -68,9 +71,12 @@ suite('HtmlContent', () => {
 	test('fancy action', () => {
 		var callbackCalled = false;
 		var result: HTMLElement = renderFormattedText('__**[[action]]**__', {
-			actionCallback(content) {
-				assert.strictEqual(content, '0');
-				callbackCalled = true;
+			actionHandler: {
+				callback(content) {
+					assert.strictEqual(content, '0');
+					callbackCalled = true;
+				},
+				disposeables: []
 			}
 		});
 		assert.strictEqual(result.innerHTML, '<i><b><a href="#">action</a></b></i>');

src/vs/editor/contrib/markdown/markdownRenderer.ts

@@ -30,9 +30,6 @@ export class MarkdownRenderer {
 		@optional(IOpenerService) private readonly _openerService: IOpenerService = NullOpenerService,
 	) {
 		this._options = {
-			actionCallback: (content) => {
-				this._openerService.open(URI.parse(content)).then(void 0, onUnexpectedError);
-			},
 			codeBlockRenderer: (languageAlias, value): TPromise<string> => {
 				// In markdown,
 				// it is possible that we stumble upon language aliases (e.g.js instead of javascript)
@@ -47,18 +44,21 @@ export class MarkdownRenderer {
 					return `<span style="font-family: ${editor.getConfiguration().fontInfo.fontFamily}">${code}</span>`;
 				});
 			},
-			codeBlockRenderCallback: () => this._onDidRenderCodeBlock.fire()
+			codeBlockRenderCallback: () => this._onDidRenderCodeBlock.fire(),
+			actionHandler: {
+				callback: (content) => {
+					this._openerService.open(URI.parse(content)).then(void 0, onUnexpectedError);
+				},
+				disposeables: [] // TODO
+			}
 		};
 	}
 
-	render(markdown: IMarkdownString, options?: RenderOptions): HTMLElement {
+	render(markdown: IMarkdownString): HTMLElement {
 		if (!markdown) {
 			return document.createElement('span');
 		}
-		if (options) {
-			return renderMarkdown(markdown, { ...options, ...this._options });
-		} else {
-			return renderMarkdown(markdown, this._options);
-		}
+
+		return renderMarkdown(markdown, this._options);
 	}
 }

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

@@ -6,7 +6,7 @@
 'use strict';
 
 import { IDelegate, IRenderer } from 'vs/base/browser/ui/list/list';
-import { renderMarkdown } from 'vs/base/browser/htmlContentRenderer';
+import { renderMarkdown, IContentActionHandler } from 'vs/base/browser/htmlContentRenderer';
 import { clearNode, addClass, removeClass, toggleClass } from 'vs/base/browser/dom';
 import { IOpenerService } from 'vs/platform/opener/common/opener';
 import URI from 'vs/base/common/uri';
@@ -132,7 +132,7 @@ class NotificationMessageMarkdownRenderer {
 		'tablerow'
 	];
 
-	public static render(markdown: IMarkdownString, actionCallback?: (content: string) => void): HTMLElement {
+	public static render(markdown: IMarkdownString, actionHandler?: IContentActionHandler): HTMLElement {
 		return renderMarkdown(markdown, {
 			inline: true,
 			joinRendererConfiguration: renderer => {
@@ -145,7 +145,7 @@ class NotificationMessageMarkdownRenderer {
 					smartypants: false // disable some text transformations
 				} as MarkedOptions;
 			},
-			actionCallback
+			actionHandler
 		});
 	}
 }
@@ -331,7 +331,10 @@ export class NotificationTemplateRenderer {
 
 	private renderMessage(notification: INotificationViewItem): boolean {
 		clearNode(this.template.message);
-		this.template.message.appendChild(NotificationMessageMarkdownRenderer.render(notification.message, (content: string) => this.openerService.open(URI.parse(content)).then(void 0, onUnexpectedError)));
+		this.template.message.appendChild(NotificationMessageMarkdownRenderer.render(notification.message, {
+			callback: (content: string) => this.openerService.open(URI.parse(content)).then(void 0, onUnexpectedError),
+			disposeables: this.inputDisposeables
+		}));
 
 		const messageOverflows = notification.canCollapse && !notification.expanded && this.template.message.scrollWidth > this.template.message.clientWidth;
 		if (messageOverflows) {