Merge branch 'notebook/dev' into main

src/vs/base/common/mime.ts

@@ -316,3 +316,20 @@ export function getExtensionForMimeType(mimeType: string): string | undefined {
 
 	return undefined;
 }
+
+const _simplePattern = /^(.+)\/(.+?)(;.+)?$/;
+
+export function normalizeMimeType(mimeType: string): string;
+export function normalizeMimeType(mimeType: string, strict: true): string | undefined;
+export function normalizeMimeType(mimeType: string, strict?: true): string | undefined {
+
+	const match = _simplePattern.exec(mimeType);
+	if (!match) {
+		return strict
+			? undefined
+			: mimeType;
+	}
+	// https://datatracker.ietf.org/doc/html/rfc2045#section-5.1
+	// media and subtype must ALWAYS be lowercase, parameter not
+	return `${match[1].toLowerCase()}/${match[2].toLowerCase()}${match[3] ?? ''}`;
+}

src/vs/base/test/common/mime.test.ts

@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as assert from 'assert';
-import { guessMimeTypes, registerTextMime } from 'vs/base/common/mime';
+import { guessMimeTypes, normalizeMimeType, registerTextMime } from 'vs/base/common/mime';
 import { URI } from 'vs/base/common/uri';
 
 suite('Mime', () => {
@@ -126,4 +126,13 @@ suite('Mime', () => {
 
 		assert.deepStrictEqual(guessMimeTypes(URI.parse(`data:;label:something.data;description:data,`)), ['text/data', 'text/plain']);
 	});
+
+	test('normalize', () => {
+		assert.strictEqual(normalizeMimeType('invalid'), 'invalid');
+		assert.strictEqual(normalizeMimeType('invalid', true), undefined);
+		assert.strictEqual(normalizeMimeType('Text/plain'), 'text/plain');
+		assert.strictEqual(normalizeMimeType('Text/pläin'), 'text/pläin');
+		assert.strictEqual(normalizeMimeType('Text/plain;UPPER'), 'text/plain;UPPER');
+		assert.strictEqual(normalizeMimeType('Text/plain;lower'), 'text/plain;lower');
+	});
 });

src/vs/vscode.proposed.d.ts

@@ -1238,6 +1238,9 @@ declare module 'vscode' {
 		with(change: { start?: number, end?: number }): NotebookRange;
 	}
 
+	/**
+	 * One representation of a {@link NotebookCellOutput notebook output}, defined by MIME type and data.
+	 */
 	// todo@API document which mime types are supported out of the box and
 	// which are considered secure
 	export class NotebookCellOutputItem {
@@ -1331,12 +1334,41 @@ declare module 'vscode' {
 		constructor(data: Uint8Array, mime: string, metadata?: { [key: string]: any });
 	}
 
-	// @jrieken transient
+	/**
+	 * Notebook cell output represents a result of executing a cell. It is a container type for multiple
+	 * {@link NotebookCellOutputItem output items} where contained items represent the same result but
+	 * use different MIME types.
+	 */
+	//todo@API - add sugar function to add more outputs
 	export class NotebookCellOutput {
+
+		/**
+		 * Identifier for this output. Using the identifier allows a subsequent execution to modify
+		 * existing output. Defaults to a fresh UUID.
+		 */
 		id: string;
+
+		/**
+		 * The output items of this output. Each item must represent the same result. _Note_ that repeated
+		 * MIME types per output is invalid and that the editor will just pick one of them.
+		 *
+		 * ```ts
+		 * new vscode.NotebookCellOutput([
+		 * 	vscode.NotebookCellOutputItem.text('Hello', 'text/plain'),
+		 * 	vscode.NotebookCellOutputItem.text('<i>Hello</i>', 'text/html'),
+		 * 	vscode.NotebookCellOutputItem.text('_Hello_', 'text/markdown'),
+		 * 	vscode.NotebookCellOutputItem.text('Hey', 'text/plain'), // INVALID: repeated type, editor will pick just one
+		 * ])
+		 * ```
+		 */
+		//todo@API rename to items
 		outputs: NotebookCellOutputItem[];
+
+		//todo@API
 		metadata?: { [key: string]: any };
+
 		constructor(outputs: NotebookCellOutputItem[], metadata?: { [key: string]: any });
+
 		constructor(outputs: NotebookCellOutputItem[], id: string, metadata?: { [key: string]: any });
 	}
 

src/vs/workbench/api/common/extHostNotebookKernels.ts

@@ -16,10 +16,11 @@ import { asWebviewUri } from 'vs/workbench/api/common/shared/webview';
 import { ResourceMap } from 'vs/base/common/map';
 import { timeout } from 'vs/base/common/async';
 import { ExtHostCell, ExtHostNotebookDocument } from 'vs/workbench/api/common/extHostNotebookDocument';
-import { CellEditType, IImmediateCellEditOperation, NotebookCellExecutionState, NullablePartialNotebookCellInternalMetadata } from 'vs/workbench/contrib/notebook/common/notebookCommon';
+import { CellEditType, IImmediateCellEditOperation, IOutputDto, NotebookCellExecutionState, NullablePartialNotebookCellInternalMetadata } from 'vs/workbench/contrib/notebook/common/notebookCommon';
 import { CancellationTokenSource } from 'vs/base/common/cancellation';
 import { asArray } from 'vs/base/common/arrays';
 import { ILogService } from 'vs/platform/log/common/log';
+import { NotebookCellOutput } from 'vs/workbench/api/common/extHostTypes';
 
 interface IKernelData {
 	extensionId: ExtensionIdentifier,
@@ -388,9 +389,24 @@ class NotebookCellExecutionTask extends Disposable {
 		return cell.handle;
 	}
 
+	private validateAndConvertOutputs(items: vscode.NotebookCellOutput[]): IOutputDto[] {
+		return items.map(output => {
+			const newOutput = NotebookCellOutput.ensureUniqueMimeTypes(output.outputs, true);
+			if (newOutput === output.outputs) {
+				return extHostTypeConverters.NotebookCellOutput.from(output);
+			}
+			return extHostTypeConverters.NotebookCellOutput.from({
+				outputs: newOutput,
+				id: output.id,
+				metadata: output.metadata
+			});
+		});
+	}
+
 	asApiObject(): vscode.NotebookCellExecutionTask {
 		const that = this;
 		return Object.freeze(<vscode.NotebookCellExecutionTask>{
+			get token() { return that._tokenSource.token; },
 			get document() { return that._document.apiNotebook; },
 			get cell() { return that._cell.apiCell; },
 
@@ -439,30 +455,28 @@ class NotebookCellExecutionTask extends Disposable {
 			async appendOutput(outputs: vscode.NotebookCellOutput | vscode.NotebookCellOutput[], cellIndex?: number): Promise<void> {
 				that.verifyStateForOutput();
 				const handle = that.cellIndexToHandle(cellIndex);
-				outputs = asArray(outputs);
-				return that.applyEditSoon({ editType: CellEditType.Output, handle, append: true, outputs: outputs.map(extHostTypeConverters.NotebookCellOutput.from) });
+				const outputDtos = that.validateAndConvertOutputs(asArray(outputs));
+				return that.applyEditSoon({ editType: CellEditType.Output, handle, append: true, outputs: outputDtos });
 			},
 
 			async replaceOutput(outputs: vscode.NotebookCellOutput | vscode.NotebookCellOutput[], cellIndex?: number): Promise<void> {
 				that.verifyStateForOutput();
 				const handle = that.cellIndexToHandle(cellIndex);
-				outputs = asArray(outputs);
-				return that.applyEditSoon({ editType: CellEditType.Output, handle, outputs: outputs.map(extHostTypeConverters.NotebookCellOutput.from) });
+				const outputDtos = that.validateAndConvertOutputs(asArray(outputs));
+				return that.applyEditSoon({ editType: CellEditType.Output, handle, outputs: outputDtos });
 			},
 
 			async appendOutputItems(items: vscode.NotebookCellOutputItem | vscode.NotebookCellOutputItem[], outputId: string): Promise<void> {
 				that.verifyStateForOutput();
-				items = asArray(items);
+				items = NotebookCellOutput.ensureUniqueMimeTypes(asArray(items), true);
 				return that.applyEditSoon({ editType: CellEditType.OutputItems, append: true, items: items.map(extHostTypeConverters.NotebookCellOutputItem.from), outputId });
 			},
 
 			async replaceOutputItems(items: vscode.NotebookCellOutputItem | vscode.NotebookCellOutputItem[], outputId: string): Promise<void> {
 				that.verifyStateForOutput();
-				items = asArray(items);
+				items = NotebookCellOutput.ensureUniqueMimeTypes(asArray(items), true);
 				return that.applyEditSoon({ editType: CellEditType.OutputItems, items: items.map(extHostTypeConverters.NotebookCellOutputItem.from), outputId });
-			},
-
-			token: that._tokenSource.token
+			}
 		});
 	}
 }

src/vs/workbench/api/common/extHostTypes.ts

@@ -8,7 +8,7 @@ import { illegalArgument } from 'vs/base/common/errors';
 import { IRelativePattern } from 'vs/base/common/glob';
 import { isMarkdownString, MarkdownString as BaseMarkdownString } from 'vs/base/common/htmlContent';
 import { ReadonlyMapView, ResourceMap } from 'vs/base/common/map';
-import { isFalsyOrWhitespace } from 'vs/base/common/strings';
+import { normalizeMimeType } from 'vs/base/common/mime';
 import { isStringArray } from 'vs/base/common/types';
 import { URI } from 'vs/base/common/uri';
 import { generateUuid } from 'vs/base/common/uuid';
@@ -3156,15 +3156,39 @@ export class NotebookCellOutputItem {
 		if (!(data instanceof Uint8Array)) {
 			this.value = data;
 		}
-		if (isFalsyOrWhitespace(this.mime)) {
-			throw new Error('INVALID mime type, must not be empty or falsy');
+
+		const mimeNormalized = normalizeMimeType(mime, true);
+		if (!mimeNormalized) {
+			throw new Error('INVALID mime type, must not be empty or falsy: ' + mime);
 		}
-		// todo@joh stringify and check metadata and throw when not JSONable
+		this.mime = mimeNormalized;
 	}
 }
 
 export class NotebookCellOutput {
 
+	static ensureUniqueMimeTypes(items: NotebookCellOutputItem[], warn: boolean = false): NotebookCellOutputItem[] {
+		const seen = new Set<string>();
+		const removeIdx = new Set<number>();
+		for (let i = 0; i < items.length; i++) {
+			const item = items[i];
+			const normalMime = normalizeMimeType(item.mime);
+			if (!seen.has(normalMime)) {
+				seen.add(normalMime);
+				continue;
+			}
+			// duplicated mime types... first has won
+			removeIdx.add(i);
+			if (warn) {
+				console.warn(`DUPLICATED mime type '${item.mime}' will be dropped`);
+			}
+		}
+		if (removeIdx.size === 0) {
+			return items;
+		}
+		return items.filter((_item, index) => !removeIdx.has(index));
+	}
+
 	id: string;
 	outputs: NotebookCellOutputItem[];
 	metadata?: Record<string, any>;
@@ -3174,7 +3198,7 @@ export class NotebookCellOutput {
 		idOrMetadata?: string | Record<string, any>,
 		metadata?: Record<string, any>
 	) {
-		this.outputs = outputs;
+		this.outputs = NotebookCellOutput.ensureUniqueMimeTypes(outputs, true);
 		if (typeof idOrMetadata === 'string') {
 			this.id = idOrMetadata;
 			this.metadata = metadata;

src/vs/workbench/contrib/notebook/browser/contrib/coreActions.ts

@@ -1590,7 +1590,10 @@ registerAction2(class ClearAllCellOutputsAction extends NotebookAction {
 				},
 				{
 					id: MenuId.NotebookToolbar,
-					when: ContextKeyExpr.equals('config.notebook.globalToolbar', true),
+					when: ContextKeyExpr.and(
+						executeNotebookCondition,
+						ContextKeyExpr.equals('config.notebook.globalToolbar', true)
+					),
 					group: 'navigation/execute',
 					order: 0
 				}

src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts

@@ -613,7 +613,7 @@ configurationRegistry.registerConfiguration({
 		[CompactView]: {
 			description: nls.localize('notebook.compactView.description', "Control whether the notebook editor should be rendered in a compact form. "),
 			type: 'boolean',
-			default: false,
+			default: true,
 			tags: ['notebookLayout']
 		},
 		[FocusIndicator]: {

src/vs/workbench/contrib/notebook/browser/view/output/outputRenderer.ts

@@ -8,79 +8,66 @@ import { OutputRendererRegistry } from 'vs/workbench/contrib/notebook/browser/vi
 import { onUnexpectedError } from 'vs/base/common/errors';
 import { ICellOutputViewModel, ICommonNotebookEditor, IOutputTransformContribution, IRenderOutput, RenderOutputType } from 'vs/workbench/contrib/notebook/browser/notebookBrowser';
 import { URI } from 'vs/base/common/uri';
-import { Disposable } from 'vs/base/common/lifecycle';
+import { dispose } from 'vs/base/common/lifecycle';
 import { localize } from 'vs/nls';
 
-export class OutputRenderer extends Disposable {
+export class OutputRenderer {
 
 	private readonly _richMimeTypeRenderers = new Map<string, IOutputTransformContribution>();
 
 	constructor(
 		notebookEditor: ICommonNotebookEditor,
-		private readonly instantiationService: IInstantiationService
+		instantiationService: IInstantiationService
 	) {
-		super();
 		for (const desc of OutputRendererRegistry.getOutputTransformContributions()) {
 			try {
-				const contribution = this.instantiationService.createInstance(desc.ctor, notebookEditor);
-				contribution.getMimetypes().forEach(mimetype => {
-					this._richMimeTypeRenderers.set(mimetype, contribution);
-				});
-				this._register(contribution);
+				const contribution = instantiationService.createInstance(desc.ctor, notebookEditor);
+				contribution.getMimetypes().forEach(mimetype => { this._richMimeTypeRenderers.set(mimetype, contribution); });
 			} catch (err) {
 				onUnexpectedError(err);
 			}
 		}
 	}
 
-	override dispose(): void {
-		super.dispose();
+	dispose(): void {
+		dispose(this._richMimeTypeRenderers.values());
 		this._richMimeTypeRenderers.clear();
 	}
 
-	getContribution(preferredMimeType: string | undefined): IOutputTransformContribution | undefined {
-		if (preferredMimeType) {
-			return this._richMimeTypeRenderers.get(preferredMimeType);
-		}
-
-		return undefined;
+	getContribution(preferredMimeType: string): IOutputTransformContribution | undefined {
+		return this._richMimeTypeRenderers.get(preferredMimeType);
 	}
 
-	private _renderNoop(viewModel: ICellOutputViewModel, container: HTMLElement): IRenderOutput {
+	private _renderMessage(container: HTMLElement, message: string): IRenderOutput {
 		const contentNode = document.createElement('p');
-		contentNode.innerText = localize('empty', "No renderer could be found for output.");
+		contentNode.innerText = message;
 		container.appendChild(contentNode);
 		return { type: RenderOutputType.Mainframe };
 	}
 
 	render(viewModel: ICellOutputViewModel, container: HTMLElement, preferredMimeType: string | undefined, notebookUri: URI): IRenderOutput {
 		if (!viewModel.model.outputs.length) {
-			return this._renderNoop(viewModel, container);
+			return this._renderMessage(container, localize('empty', "Cell has no output"));
 		}
-
-		if (!preferredMimeType || !this._richMimeTypeRenderers.has(preferredMimeType)) {
-			const contentNode = document.createElement('p');
+		if (!preferredMimeType) {
 			const mimeTypes = viewModel.model.outputs.map(op => op.mime);
-
 			const mimeTypesMessage = mimeTypes.join(', ');
-
+			return this._renderMessage(container, localize('noRenderer.2', "No renderer could be found for output. It has the following MIME types: {0}", mimeTypesMessage));
+		}
+		if (!preferredMimeType || !this._richMimeTypeRenderers.has(preferredMimeType)) {
 			if (preferredMimeType) {
-				contentNode.innerText = localize('noRenderer.1', "No renderer could be found for MIME type: {0}", preferredMimeType);
-			} else {
-				contentNode.innerText = localize('noRenderer.2', "No renderer could be found for output. It has the following MIME types: {0}", mimeTypesMessage);
+				return this._renderMessage(container, localize('noRenderer.1', "No renderer could be found for MIME type: {0}", preferredMimeType));
 			}
-
-			container.appendChild(contentNode);
-			return { type: RenderOutputType.Mainframe };
 		}
-
 		const renderer = this._richMimeTypeRenderers.get(preferredMimeType);
-		const items = viewModel.model.outputs.filter(op => op.mime === preferredMimeType);
-
-		if (items.length && renderer) {
-			return renderer.render(viewModel, items, container, notebookUri);
-		} else {
-			return this._renderNoop(viewModel, container);
+		if (!renderer) {
+			return this._renderMessage(container, localize('noRenderer.1', "No renderer could be found for MIME type: {0}", preferredMimeType));
 		}
+		const first = viewModel.model.outputs.find(op => op.mime === preferredMimeType);
+		if (!first) {
+			return this._renderMessage(container, localize('empty', "Cell has no output"));
+		}
+
+		return renderer.render(viewModel, [first], container, notebookUri);
 	}
 }

src/vs/workbench/contrib/notebook/common/notebookOptions.ts

@@ -111,7 +111,7 @@ export class NotebookOptions {
 		const dragAndDropEnabled = this.configurationService.getValue<boolean | undefined>(DragAndDropEnabled) ?? true;
 		const cellToolbarLocation = this.configurationService.getValue<string | { [key: string]: string }>(CellToolbarLocKey);
 		const cellToolbarInteraction = this.configurationService.getValue<string>(CellToolbarVisibility);
-		const compactView = this.configurationService.getValue<boolean>(CompactView);
+		const compactView = this.configurationService.getValue<boolean | undefined>(CompactView) ?? true;
 		const focusIndicator = this._computeFocusIndicatorOption();
 		const insertToolbarPosition = this._computeInsertToolbarPositionOption();
 		const insertToolbarAlignment = this._computeInsertToolbarAlignmentOption();
@@ -217,7 +217,7 @@ export class NotebookOptions {
 		}
 
 		if (compactView) {
-			const compactViewValue = this.configurationService.getValue<boolean>(CompactView);
+			const compactViewValue = this.configurationService.getValue<boolean | undefined>(CompactView) ?? true;
 			configuration = Object.assign(configuration, {
 				...(compactViewValue ? compactConfigConstants : defaultConfigConstants),
 			});

src/vs/workbench/test/browser/api/extHostTypes.test.ts

@@ -685,6 +685,11 @@ suite('ExtHostTypes', function () {
 
 	test('NotebookCellOutputItem - factories', function () {
 
+		assert.throws(() => {
+			// invalid mime type
+			new types.NotebookCellOutputItem(new Uint8Array(), 'invalid');
+		});
+
 		// --- err
 
 		let item = types.NotebookCellOutputItem.error(new Error());
@@ -698,8 +703,8 @@ suite('ExtHostTypes', function () {
 		assert.strictEqual(item.mime, 'application/json');
 		assert.deepStrictEqual(item.data, new TextEncoder().encode(JSON.stringify(1)));
 
-		item = types.NotebookCellOutputItem.json(1, 'foo');
-		assert.strictEqual(item.mime, 'foo');
+		item = types.NotebookCellOutputItem.json(1, 'foo/bar');
+		assert.strictEqual(item.mime, 'foo/bar');
 		assert.deepStrictEqual(item.data, new TextEncoder().encode(JSON.stringify(1)));
 
 		item = types.NotebookCellOutputItem.json(true);