From dd17b4cfc7e63ca75b2197fefb5a72d7e736b136 Mon Sep 17 00:00:00 2001 From: Aaron Munger Date: Mon, 22 Apr 2024 08:09:11 -0700 Subject: [PATCH] Minimal error renderer (#210767) * collapse stack trace * interaction buttons * fixup * place feature behind option * styling, more cleanup * test returned link, fix test bug * test header link, skip header link for old IPython * padding * remove inline margin --- extensions/notebook-renderers/src/index.ts | 108 ++++++++++++++++-- .../notebook-renderers/src/rendererTypes.ts | 1 + .../src/stackTraceHelper.ts | 17 ++- .../src/test/stackTraceHelper.test.ts | 23 ++-- .../notebook/browser/notebook.contribution.ts | 6 + .../notebook/browser/notebookEditorWidget.ts | 1 + .../notebook/browser/notebookOptions.ts | 21 +++- .../view/renderers/backLayerWebView.ts | 5 +- .../browser/view/renderers/webviewPreloads.ts | 2 + .../contrib/notebook/common/notebookCommon.ts | 1 + 10 files changed, 152 insertions(+), 33 deletions(-) diff --git a/extensions/notebook-renderers/src/index.ts b/extensions/notebook-renderers/src/index.ts index ef9240df2b78f3..66b6b2cb287999 100644 --- a/extensions/notebook-renderers/src/index.ts +++ b/extensions/notebook-renderers/src/index.ts @@ -184,28 +184,35 @@ function renderError( return disposableStore; } + const headerMessage = err.name && err.message ? `${err.name}: ${err.message}` : err.name || err.message; + if (err.stack) { + const minimalError = ctx.settings.minimalError && !!headerMessage?.length; outputElement.classList.add('traceback'); - const stackTrace = formatStackTrace(err.stack); + const { formattedStack, errorLocation } = formatStackTrace(err.stack); - const outputScrolling = scrollingEnabled(outputInfo, ctx.settings); - const outputOptions = { linesLimit: ctx.settings.lineLimit, scrollable: outputScrolling, trustHtml, linkifyFilePaths: ctx.settings.linkifyFilePaths }; + const outputScrolling = !minimalError && scrollingEnabled(outputInfo, ctx.settings); + const lineLimit = minimalError ? 1000 : ctx.settings.lineLimit; + const outputOptions = { linesLimit: lineLimit, scrollable: outputScrolling, trustHtml, linkifyFilePaths: false }; - const content = createOutputContent(outputInfo.id, stackTrace ?? '', outputOptions); - const contentParent = document.createElement('div'); - contentParent.classList.toggle('word-wrap', ctx.settings.outputWordWrap); + const content = createOutputContent(outputInfo.id, formattedStack, outputOptions); + const stackTraceElement = document.createElement('div'); + stackTraceElement.appendChild(content); + stackTraceElement.classList.toggle('word-wrap', ctx.settings.outputWordWrap); disposableStore.push(ctx.onDidChangeSettings(e => { - contentParent.classList.toggle('word-wrap', e.outputWordWrap); + stackTraceElement.classList.toggle('word-wrap', e.outputWordWrap); })); - contentParent.classList.toggle('scrollable', outputScrolling); - contentParent.appendChild(content); - outputElement.appendChild(contentParent); - initializeScroll(contentParent, disposableStore); + if (minimalError) { + createMinimalError(errorLocation, headerMessage, stackTraceElement, outputElement); + } else { + stackTraceElement.classList.toggle('scrollable', outputScrolling); + outputElement.appendChild(stackTraceElement); + initializeScroll(stackTraceElement, disposableStore); + } } else { const header = document.createElement('div'); - const headerMessage = err.name && err.message ? `${err.name}: ${err.message}` : err.name || err.message; if (headerMessage) { header.innerText = headerMessage; outputElement.appendChild(header); @@ -216,6 +223,54 @@ function renderError( return disposableStore; } +function createMinimalError(errorLocation: string | undefined, headerMessage: string, stackTrace: HTMLDivElement, outputElement: HTMLElement) { + const outputDiv = document.createElement('div'); + const headerSection = document.createElement('div'); + headerSection.classList.add('error-output-header'); + + if (errorLocation && errorLocation.indexOf(' { + e.preventDefault(); + const hidden = stackTrace.style.display === 'none'; + stackTrace.style.display = hidden ? '' : 'none'; + toggleStackLink.innerText = hidden ? 'Hide Details' : 'Show Details'; + }; + + outputDiv.appendChild(stackTrace); + stackTrace.style.display = 'none'; + outputElement.appendChild(outputDiv); +} + function getPreviousMatchingContentGroup(outputElement: HTMLElement) { const outputContainer = outputElement.parentElement; let match: HTMLElement | undefined = undefined; @@ -449,6 +504,35 @@ export const activate: ActivationFunction = (ctx) => { .traceback .code-underline { text-decoration: underline; } + #container ul.error-output-actions { + margin: 0px; + padding: 6px 0px 0px 6px; + padding-inline-start: 0px; + } + #container .error-output-actions li { + padding: 0px 4px 0px 4px; + border-radius: 5px; + height: 20px; + display: inline-flex; + cursor: pointer; + border: solid 1px var(--vscode-notebook-cellToolbarSeparator); + } + #container .error-output-actions li.hover { + background-color: var(--vscode-toolbar-hoverBackground); + } + #container .error-output-actions li:focus-within { + border-color: var(--theme-input-focus-border-color); + } + #container .error-output-actions a:focus { + outline: 0; + } + #container .error-output-actions li a { + color: var(--vscode-foreground); + text-decoration: none; + } + #container .error-output-header a { + padding-right: 12px; + } `; document.body.appendChild(style); diff --git a/extensions/notebook-renderers/src/rendererTypes.ts b/extensions/notebook-renderers/src/rendererTypes.ts index e1fc869a301ec4..bab7a34af9f7e6 100644 --- a/extensions/notebook-renderers/src/rendererTypes.ts +++ b/extensions/notebook-renderers/src/rendererTypes.ts @@ -33,6 +33,7 @@ export interface RenderOptions { readonly outputScrolling: boolean; readonly outputWordWrap: boolean; readonly linkifyFilePaths: boolean; + readonly minimalError: boolean; } export type IRichRenderContext = RendererContext & { readonly settings: RenderOptions; readonly onDidChangeSettings: Event }; diff --git a/extensions/notebook-renderers/src/stackTraceHelper.ts b/extensions/notebook-renderers/src/stackTraceHelper.ts index 943877ad59d0c9..ecf0eddb40ebcf 100644 --- a/extensions/notebook-renderers/src/stackTraceHelper.ts +++ b/extensions/notebook-renderers/src/stackTraceHelper.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -export function formatStackTrace(stack: string) { +export function formatStackTrace(stack: string): { formattedStack: string; errorLocation?: string } { let cleaned: string; // Ansi colors are described here: // https://en.wikipedia.org/wiki/ANSI_escape_code under the SGR section @@ -26,7 +26,7 @@ export function formatStackTrace(stack: string) { return linkifyStack(cleaned); } - return cleaned; + return { formattedStack: cleaned }; } const formatSequence = /\u001b\[.+?m/g; @@ -49,10 +49,11 @@ type fileLocation = { kind: 'file'; path: string }; type location = cellLocation | fileLocation; -function linkifyStack(stack: string) { +function linkifyStack(stack: string): { formattedStack: string; errorLocation?: string } { const lines = stack.split('\n'); let fileOrCell: location | undefined; + let locationLink = ''; for (const i in lines) { @@ -67,7 +68,9 @@ function linkifyStack(stack: string) { kind: 'cell', path: stripFormatting(original.replace(cellRegex, 'vscode-notebook-cell:?execution_count=$')) }; - lines[i] = original.replace(cellRegex, `$\'>line $`); + const link = original.replace(cellRegex, `\'>line $`); + lines[i] = original.replace(cellRegex, `$${link}`); + locationLink = locationLink || link; continue; } else if (inputRegex.test(original)) { @@ -75,7 +78,8 @@ function linkifyStack(stack: string) { kind: 'cell', path: stripFormatting(original.replace(inputRegex, 'vscode-notebook-cell:?execution_count=$')) }; - lines[i] = original.replace(inputRegex, `Input \'>$$`); + const link = original.replace(inputRegex, `$`); + lines[i] = original.replace(inputRegex, `Input ${link}$`); continue; } else if (!fileOrCell || original.trim() === '') { @@ -94,5 +98,6 @@ function linkifyStack(stack: string) { } } - return lines.join('\n'); + const errorLocation = locationLink; + return { formattedStack: lines.join('\n'), errorLocation }; } diff --git a/extensions/notebook-renderers/src/test/stackTraceHelper.test.ts b/extensions/notebook-renderers/src/test/stackTraceHelper.test.ts index c4e9917908606c..54ec15b428caa5 100644 --- a/extensions/notebook-renderers/src/test/stackTraceHelper.test.ts +++ b/extensions/notebook-renderers/src/test/stackTraceHelper.test.ts @@ -16,7 +16,7 @@ suite('StackTraceHelper', () => { '@Main c:\\src\\test\\3\\otherlanguages\\julia.ipynb: 3\n' + '[2] top - level scope\n' + '@c:\\src\\test\\3\\otherlanguages\\julia.ipynb: 1; '; - assert.equal(formatStackTrace(stack), stack); + assert.equal(formatStackTrace(stack).formattedStack, stack); }); const formatSequence = /\u001b\[.+?m/g; @@ -37,10 +37,12 @@ suite('StackTraceHelper', () => { '\u001b[1;32m----> 2\u001b[0m \u001b[38;5;28;01mraise\u001b[39;00m \u001b[38;5;167;01mException\u001b[39;00m\n\n' + '\u001b[1;31mException\u001b[0m\n:'; - const formatted = stripAsciiFormatting(formatStackTrace(stack)); - assert.ok(formatted.indexOf('Cell In[3], line 2') > 0, 'Missing line link in ' + formatted); - assert.ok(formatted.indexOf('2') > 0, 'Missing frame link in ' + formatted); - assert.ok(formatted.indexOf('2') > 0, 'Missing frame link in ' + formatted); + const { formattedStack, errorLocation } = formatStackTrace(stack); + const cleanStack = stripAsciiFormatting(formattedStack); + assert.ok(cleanStack.indexOf('Cell In[3], line 2') > 0, 'Missing line link in ' + cleanStack); + assert.ok(cleanStack.indexOf('2') > 0, 'Missing frame link in ' + cleanStack); + assert.ok(cleanStack.indexOf('2') > 0, 'Missing frame link in ' + cleanStack); + assert.equal(errorLocation, 'line 2'); }); test('IPython stack line numbers are linkified for IPython 8.3', () => { @@ -65,9 +67,10 @@ suite('StackTraceHelper', () => { '\n' + '\u001b[1;31mException\u001b[0m:\n'; - const formatted = stripAsciiFormatting(formatStackTrace(stack)); - assert.ok(formatted.indexOf('Input \'>In [2], in ') > 0, 'Missing cell link in ' + formatted); - assert.ok(formatted.indexOf('Input \'>In [1], in myfunc()') > 0, 'Missing cell link in ' + formatted); + const { formattedStack } = formatStackTrace(stack); + const formatted = stripAsciiFormatting(formattedStack); + assert.ok(formatted.indexOf('Input In [2], in ') > 0, 'Missing cell link in ' + formatted); + assert.ok(formatted.indexOf('Input In [1], in myfunc()') > 0, 'Missing cell link in ' + formatted); assert.ok(formatted.indexOf('5') > 0, 'Missing frame link in ' + formatted); }); @@ -82,7 +85,7 @@ suite('StackTraceHelper', () => { '\u001b[1;32m----> 2\u001b[0m \u001b[38;5;28;01mraise\u001b[39;00m \u001b[38;5;167;01mException\u001b[39;00m\n\n' + '\u001b[1;31mException\u001b[0m\n:'; - const formatted = formatStackTrace(stack); + const formatted = formatStackTrace(stack).formattedStack; assert.ok(!/\d<\/a>/.test(formatted), formatted); }); @@ -97,7 +100,7 @@ suite('StackTraceHelper', () => { 'a 1 print(\n' + ' 1a print(\n'; - const formattedLines = formatStackTrace(stack).split('\n'); + const formattedLines = formatStackTrace(stack).formattedStack.split('\n'); assert.ok(/ assert.ok(!//.test(line), 'line should not contain a link: ' + line)); }); diff --git a/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts b/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts index f296d16ed2d5d5..5f3754e02a7b6b 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts @@ -960,6 +960,12 @@ configurationRegistry.registerConfiguration({ default: true, tags: ['notebookOutputLayout'] }, + [NotebookSetting.minimalErrorRendering]: { + description: nls.localize('notebook.minimalErrorRendering', "Control whether to render error output in a minimal style."), + type: 'boolean', + default: false, + tags: ['notebookOutputLayout'] + }, [NotebookSetting.markupFontSize]: { markdownDescription: nls.localize('notebook.markup.fontSize', "Controls the font size in pixels of rendered markup in notebooks. When set to {0}, 120% of {1} is used.", '`0`', '`#editor.fontSize#`'), type: 'number', diff --git a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts index ba392618e9111e..3a65e207a9367b 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts @@ -374,6 +374,7 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD || e.outputWordWrap || e.outputScrolling || e.outputLinkifyFilePaths + || e.minimalError ) { this._styleElement?.remove(); this._createLayoutStyles(); diff --git a/src/vs/workbench/contrib/notebook/browser/notebookOptions.ts b/src/vs/workbench/contrib/notebook/browser/notebookOptions.ts index 6679cb33563906..6cc994b7ff6a03 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookOptions.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookOptions.ts @@ -41,6 +41,7 @@ export interface NotebookDisplayOptions { // TODO @Yoyokrazy rename to a more ge outputWordWrap: boolean; outputLineLimit: number; outputLinkifyFilePaths: boolean; + outputMinimalError: boolean; fontSize: number; outputFontSize: number; outputFontFamily: string; @@ -103,6 +104,7 @@ export interface NotebookOptionsChangeEvent { readonly outputWordWrap?: boolean; readonly outputScrolling?: boolean; readonly outputLinkifyFilePaths?: boolean; + readonly minimalError?: boolean; } const defaultConfigConstants = Object.freeze({ @@ -206,6 +208,7 @@ export class NotebookOptions extends Disposable { const outputWordWrap = this.configurationService.getValue(NotebookSetting.outputWordWrap); const outputLineLimit = this.configurationService.getValue(NotebookSetting.textOutputLineLimit) ?? 30; const linkifyFilePaths = this.configurationService.getValue(NotebookSetting.LinkifyOutputFilePaths) ?? true; + const minimalErrors = this.configurationService.getValue(NotebookSetting.minimalErrorRendering); const editorTopPadding = this._computeEditorTopPadding(); @@ -250,7 +253,8 @@ export class NotebookOptions extends Disposable { outputScrolling: outputScrolling, outputWordWrap: outputWordWrap, outputLineLimit: outputLineLimit, - outputLinkifyFilePaths: linkifyFilePaths + outputLinkifyFilePaths: linkifyFilePaths, + outputMinimalError: minimalErrors }; this._register(this.configurationService.onDidChangeConfiguration(e => { @@ -415,6 +419,7 @@ export class NotebookOptions extends Disposable { const outputScrolling = e.affectsConfiguration(NotebookSetting.outputScrolling); const outputWordWrap = e.affectsConfiguration(NotebookSetting.outputWordWrap); const outputLinkifyFilePaths = e.affectsConfiguration(NotebookSetting.LinkifyOutputFilePaths); + const minimalError = e.affectsConfiguration(NotebookSetting.minimalErrorRendering); if ( !cellStatusBarVisibility @@ -441,7 +446,8 @@ export class NotebookOptions extends Disposable { && !outputLineHeight && !outputScrolling && !outputWordWrap - && !outputLinkifyFilePaths) { + && !outputLinkifyFilePaths + && !minimalError) { return; } @@ -548,6 +554,10 @@ export class NotebookOptions extends Disposable { configuration.outputLinkifyFilePaths = this.configurationService.getValue(NotebookSetting.LinkifyOutputFilePaths); } + if (minimalError) { + configuration.outputMinimalError = this.configurationService.getValue(NotebookSetting.minimalErrorRendering); + } + this._layoutConfiguration = Object.freeze(configuration); // trigger event @@ -576,7 +586,8 @@ export class NotebookOptions extends Disposable { outputLineHeight, outputScrolling, outputWordWrap, - outputLinkifyFilePaths: outputLinkifyFilePaths + outputLinkifyFilePaths, + minimalError }); } @@ -790,6 +801,7 @@ export class NotebookOptions extends Disposable { outputWordWrap: this._layoutConfiguration.outputWordWrap, outputLineLimit: this._layoutConfiguration.outputLineLimit, outputLinkifyFilePaths: this._layoutConfiguration.outputLinkifyFilePaths, + minimalError: this._layoutConfiguration.outputMinimalError }; } @@ -811,7 +823,8 @@ export class NotebookOptions extends Disposable { outputScrolling: this._layoutConfiguration.outputScrolling, outputWordWrap: this._layoutConfiguration.outputWordWrap, outputLineLimit: this._layoutConfiguration.outputLineLimit, - outputLinkifyFilePaths: false + outputLinkifyFilePaths: false, + minimalError: false }; } diff --git a/src/vs/workbench/contrib/notebook/browser/view/renderers/backLayerWebView.ts b/src/vs/workbench/contrib/notebook/browser/view/renderers/backLayerWebView.ts index 6a6e150c4a0935..cf7965b0752f21 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/renderers/backLayerWebView.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/renderers/backLayerWebView.ts @@ -119,6 +119,7 @@ interface BacklayerWebviewOptions { readonly outputWordWrap: boolean; readonly outputLineLimit: number; readonly outputLinkifyFilePaths: boolean; + readonly minimalError: boolean; } @@ -249,6 +250,7 @@ export class BackLayerWebView extends Themable { outputScrolling: this.options.outputScrolling, outputWordWrap: this.options.outputWordWrap, linkifyFilePaths: this.options.outputLinkifyFilePaths, + minimalError: this.options.minimalError } }); } @@ -287,7 +289,8 @@ export class BackLayerWebView extends Themable { lineLimit: this.options.outputLineLimit, outputScrolling: this.options.outputScrolling, outputWordWrap: this.options.outputWordWrap, - linkifyFilePaths: this.options.outputLinkifyFilePaths + linkifyFilePaths: this.options.outputLinkifyFilePaths, + minimalError: this.options.minimalError }; const preloadScript = preloadsScriptStr( { diff --git a/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewPreloads.ts b/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewPreloads.ts index ca488737ada549..c7aa6196b5c0e2 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewPreloads.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewPreloads.ts @@ -70,6 +70,7 @@ export interface RenderOptions { readonly outputScrolling: boolean; readonly outputWordWrap: boolean; readonly linkifyFilePaths: boolean; + readonly minimalError: boolean; } interface PreloadContext { @@ -1911,6 +1912,7 @@ async function webviewPreloads(ctx: PreloadContext) { get outputScrolling() { return currentRenderOptions.outputScrolling; }, get outputWordWrap() { return currentRenderOptions.outputWordWrap; }, get linkifyFilePaths() { return currentRenderOptions.linkifyFilePaths; }, + get minimalError() { return currentRenderOptions.minimalError; }, }, get onDidChangeSettings() { return settingChange.event; } }; diff --git a/src/vs/workbench/contrib/notebook/common/notebookCommon.ts b/src/vs/workbench/contrib/notebook/common/notebookCommon.ts index cfcf9669a70717..a2225908f864a5 100644 --- a/src/vs/workbench/contrib/notebook/common/notebookCommon.ts +++ b/src/vs/workbench/contrib/notebook/common/notebookCommon.ts @@ -937,6 +937,7 @@ export const NotebookSetting = { outputScrolling: 'notebook.output.scrolling', textOutputLineLimit: 'notebook.output.textLineLimit', LinkifyOutputFilePaths: 'notebook.output.linkifyFilePaths', + minimalErrorRendering: 'notebook.output.minimalErrorRendering', formatOnSave: 'notebook.formatOnSave.enabled', insertFinalNewline: 'notebook.insertFinalNewline', formatOnCellExecution: 'notebook.formatOnCellExecution',