From 0c51035590b20878d940ceb3cac0d54c4eb54bbc Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Fri, 20 Dec 2024 13:59:08 -0800 Subject: [PATCH] testing: fix errors peeks being oversized relative to their contents (#236763) Fixes #214011 --- src/vs/base/browser/ui/list/listView.ts | 2 + .../contrib/zoneWidget/browser/zoneWidget.ts | 14 ++-- .../contrib/debug/browser/callStackWidget.ts | 8 +++ .../testResultsView/testMessageStack.ts | 49 ------------- .../testResultsView/testResultsViewContent.ts | 37 +++++++--- .../testing/browser/testingOutputPeek.ts | 70 +++++++++++-------- 6 files changed, 85 insertions(+), 95 deletions(-) delete mode 100644 src/vs/workbench/contrib/testing/browser/testResultsView/testMessageStack.ts diff --git a/src/vs/base/browser/ui/list/listView.ts b/src/vs/base/browser/ui/list/listView.ts index 36355a377ba..e305b7de4b9 100644 --- a/src/vs/base/browser/ui/list/listView.ts +++ b/src/vs/base/browser/ui/list/listView.ts @@ -567,6 +567,8 @@ export class ListView implements IListView { if (this.supportDynamicHeights) { this._rerender(this.lastRenderTop, this.lastRenderHeight); + } else { + this._onDidChangeContentHeight.fire(this.contentHeight); // otherwise fired in _rerender() } } diff --git a/src/vs/editor/contrib/zoneWidget/browser/zoneWidget.ts b/src/vs/editor/contrib/zoneWidget/browser/zoneWidget.ts index 51988af7d17..a38f1639eea 100644 --- a/src/vs/editor/contrib/zoneWidget/browser/zoneWidget.ts +++ b/src/vs/editor/contrib/zoneWidget/browser/zoneWidget.ts @@ -29,7 +29,6 @@ export interface IOptions { frameColor?: Color | string; arrowColor?: Color; keepEditorSelection?: boolean; - allowUnlimitedHeight?: boolean; ordinal?: number; showInHiddenAreas?: boolean; } @@ -376,6 +375,11 @@ export abstract class ZoneWidget implements IHorizontalSashLayoutProvider { return result; } + /** Gets the maximum widget height in lines. */ + protected _getMaximumHeightInLines(): number | undefined { + return Math.max(12, (this.editor.getLayoutInfo().height / this.editor.getOption(EditorOption.lineHeight)) * 0.8); + } + private _showImpl(where: Range, heightInLines: number): void { const position = where.getStartPosition(); const layoutInfo = this.editor.getLayoutInfo(); @@ -389,8 +393,8 @@ export abstract class ZoneWidget implements IHorizontalSashLayoutProvider { const lineHeight = this.editor.getOption(EditorOption.lineHeight); // adjust heightInLines to viewport - if (!this.options.allowUnlimitedHeight) { - const maxHeightInLines = Math.max(12, (this.editor.getLayoutInfo().height / lineHeight) * 0.8); + const maxHeightInLines = this._getMaximumHeightInLines(); + if (maxHeightInLines !== undefined) { heightInLines = Math.min(heightInLines, maxHeightInLines); } @@ -493,7 +497,9 @@ export abstract class ZoneWidget implements IHorizontalSashLayoutProvider { // implement in subclass } - protected _relayout(newHeightInLines: number): void { + protected _relayout(_newHeightInLines: number): void { + const maxHeightInLines = this._getMaximumHeightInLines(); + const newHeightInLines = maxHeightInLines === undefined ? _newHeightInLines : Math.min(maxHeightInLines, _newHeightInLines); if (this._viewZone && this._viewZone.heightInLines !== newHeightInLines) { this.editor.changeViewZones(accessor => { if (this._viewZone) { diff --git a/src/vs/workbench/contrib/debug/browser/callStackWidget.ts b/src/vs/workbench/contrib/debug/browser/callStackWidget.ts index dc2c12dd730..2948bbb715b 100644 --- a/src/vs/workbench/contrib/debug/browser/callStackWidget.ts +++ b/src/vs/workbench/contrib/debug/browser/callStackWidget.ts @@ -119,10 +119,18 @@ export class CallStackWidget extends Disposable { private readonly currentFramesDs = this._register(new DisposableStore()); private cts?: CancellationTokenSource; + public get onDidChangeContentHeight() { + return this.list.onDidChangeContentHeight; + } + public get onDidScroll() { return this.list.onDidScroll; } + public get contentHeight() { + return this.list.contentHeight; + } + constructor( container: HTMLElement, containingEditor: ICodeEditor | undefined, diff --git a/src/vs/workbench/contrib/testing/browser/testResultsView/testMessageStack.ts b/src/vs/workbench/contrib/testing/browser/testResultsView/testMessageStack.ts deleted file mode 100644 index fe0592cb567..00000000000 --- a/src/vs/workbench/contrib/testing/browser/testResultsView/testMessageStack.ts +++ /dev/null @@ -1,49 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -import { Disposable } from '../../../../../base/common/lifecycle.js'; -import { ICodeEditor } from '../../../../../editor/browser/editorBrowser.js'; -import { IInstantiationService } from '../../../../../platform/instantiation/common/instantiation.js'; -import { AnyStackFrame, CallStackFrame, CallStackWidget } from '../../../debug/browser/callStackWidget.js'; -import { ITestMessageStackFrame } from '../../common/testTypes.js'; - -export class TestResultStackWidget extends Disposable { - private readonly widget: CallStackWidget; - - public get onDidScroll() { - return this.widget.onDidScroll; - } - - constructor( - private readonly container: HTMLElement, - containingEditor: ICodeEditor | undefined, - @IInstantiationService instantiationService: IInstantiationService, - ) { - super(); - - this.widget = this._register(instantiationService.createInstance( - CallStackWidget, - container, - containingEditor, - )); - } - - public collapseAll() { - this.widget.collapseAll(); - } - - public update(messageFrame: AnyStackFrame, stack: ITestMessageStackFrame[]) { - this.widget.setFrames([messageFrame, ...stack.map(frame => new CallStackFrame( - frame.label, - frame.uri, - frame.position?.lineNumber, - frame.position?.column, - ))]); - } - - public layout(height?: number, width?: number) { - this.widget.layout(height ?? this.container.clientHeight, width); - } -} diff --git a/src/vs/workbench/contrib/testing/browser/testResultsView/testResultsViewContent.ts b/src/vs/workbench/contrib/testing/browser/testResultsView/testResultsViewContent.ts index 421afad78f2..5f2990a413a 100644 --- a/src/vs/workbench/contrib/testing/browser/testResultsView/testResultsViewContent.ts +++ b/src/vs/workbench/contrib/testing/browser/testResultsView/testResultsViewContent.ts @@ -14,7 +14,6 @@ import { Emitter, Event, Relay } from '../../../../../base/common/event.js'; import { KeyCode } from '../../../../../base/common/keyCodes.js'; import { Disposable, DisposableStore, IDisposable, toDisposable } from '../../../../../base/common/lifecycle.js'; import { observableValue } from '../../../../../base/common/observable.js'; -import './testResultsViewContent.css'; import { ICodeEditor } from '../../../../../editor/browser/editorBrowser.js'; import { ITextModelService } from '../../../../../editor/common/services/resolverService.js'; import { localize } from '../../../../../nls.js'; @@ -28,12 +27,7 @@ import { IInstantiationService, ServicesAccessor } from '../../../../../platform import { ServiceCollection } from '../../../../../platform/instantiation/common/serviceCollection.js'; import { IQuickInputService } from '../../../../../platform/quickinput/common/quickInput.js'; import { IUriIdentityService } from '../../../../../platform/uriIdentity/common/uriIdentity.js'; -import { CustomStackFrame } from '../../../debug/browser/callStackWidget.js'; -import * as icons from '../icons.js'; -import { TestResultStackWidget } from './testMessageStack.js'; -import { DiffContentProvider, IPeekOutputRenderer, MarkdownTestMessagePeek, PlainTextMessagePeek, TerminalMessagePeek } from './testResultsOutput.js'; -import { equalsSubject, getSubjectTestItem, InspectSubject, MessageSubject, TaskSubject, TestOutputSubject } from './testResultsSubject.js'; -import { OutputPeekTree } from './testResultsTree.js'; +import { AnyStackFrame, CallStackFrame, CallStackWidget, CustomStackFrame } from '../../../debug/browser/callStackWidget.js'; import { TestCommandId } from '../../common/constants.js'; import { IObservableValue } from '../../common/observableValue.js'; import { capabilityContextKeys, ITestProfileService } from '../../common/testProfileService.js'; @@ -41,6 +35,11 @@ import { LiveTestResult } from '../../common/testResult.js'; import { ITestFollowup, ITestService } from '../../common/testService.js'; import { ITestMessageStackFrame, TestRunProfileBitset } from '../../common/testTypes.js'; import { TestingContextKeys } from '../../common/testingContextKeys.js'; +import * as icons from '../icons.js'; +import { DiffContentProvider, IPeekOutputRenderer, MarkdownTestMessagePeek, PlainTextMessagePeek, TerminalMessagePeek } from './testResultsOutput.js'; +import { equalsSubject, getSubjectTestItem, InspectSubject, MessageSubject, TaskSubject, TestOutputSubject } from './testResultsSubject.js'; +import { OutputPeekTree } from './testResultsTree.js'; +import './testResultsViewContent.css'; const enum SubView { Diff = 0, @@ -183,7 +182,7 @@ export class TestResultsViewContent extends Disposable { private contextKeyTestMessage!: IContextKey; private contextKeyResultOutdated!: IContextKey; private stackContainer!: HTMLElement; - private callStackWidget!: TestResultStackWidget; + private callStackWidget!: CallStackWidget; private currentTopFrame?: MessageStackFrame; private isDoingLayoutUpdate?: boolean; @@ -209,6 +208,14 @@ export class TestResultsViewContent extends Disposable { }; } + public get onDidChangeContentHeight() { + return this.callStackWidget.onDidChangeContentHeight; + } + + public get contentHeight() { + return this.callStackWidget?.contentHeight || 0; + } + constructor( private readonly editor: ICodeEditor | undefined, private readonly options: { @@ -233,7 +240,7 @@ export class TestResultsViewContent extends Disposable { const messageContainer = this.messageContainer = dom.$('.test-output-peek-message-container'); this.stackContainer = dom.append(containerElement, dom.$('.test-output-call-stack-container')); - this.callStackWidget = this._register(this.instantiationService.createInstance(TestResultStackWidget, this.stackContainer, this.editor)); + this.callStackWidget = this._register(this.instantiationService.createInstance(CallStackWidget, this.stackContainer, this.editor)); this.followupWidget = this._register(this.instantiationService.createInstance(FollowupActionWidget, this.editor)); this.onCloseEmitter.input = this.followupWidget.onClose; @@ -315,13 +322,22 @@ export class TestResultsViewContent extends Disposable { this.currentSubjectStore.clear(); const callFrames = this.getCallFrames(opts.subject) || []; const topFrame = await this.prepareTopFrame(opts.subject, callFrames); - this.callStackWidget.update(topFrame, callFrames); + this.setCallStackFrames(topFrame, callFrames); this.followupWidget.show(opts.subject); this.populateFloatingClick(opts.subject); }); } + private setCallStackFrames(messageFrame: AnyStackFrame, stack: ITestMessageStackFrame[]) { + this.callStackWidget.setFrames([messageFrame, ...stack.map(frame => new CallStackFrame( + frame.label, + frame.uri, + frame.position?.lineNumber, + frame.position?.column, + ))]); + } + /** * Collapses all displayed stack frames. */ @@ -375,7 +391,6 @@ export class TestResultsViewContent extends Disposable { })); } - if (provider.onDidContentSizeChange) { this.currentSubjectStore.add(provider.onDidContentSizeChange(() => { if (this.dimension && !this.isDoingLayoutUpdate) { diff --git a/src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts b/src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts index 3ea9c37de5c..808e68a6ca9 100644 --- a/src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts +++ b/src/vs/workbench/contrib/testing/browser/testingOutputPeek.ts @@ -6,16 +6,16 @@ import * as dom from '../../../../base/browser/dom.js'; import { alert } from '../../../../base/browser/ui/aria/aria.js'; import { IAction } from '../../../../base/common/actions.js'; +import { RunOnceScheduler } from '../../../../base/common/async.js'; import { Codicon } from '../../../../base/common/codicons.js'; import { Color } from '../../../../base/common/color.js'; -import { Emitter, Event } from '../../../../base/common/event.js'; +import { Event } from '../../../../base/common/event.js'; import { stripIcons } from '../../../../base/common/iconLabels.js'; import { Iterable } from '../../../../base/common/iterator.js'; import { KeyCode, KeyMod } from '../../../../base/common/keyCodes.js'; import { Lazy } from '../../../../base/common/lazy.js'; import { Disposable } from '../../../../base/common/lifecycle.js'; import { derived, disposableObservableValue, observableValue } from '../../../../base/common/observable.js'; -import { count } from '../../../../base/common/strings.js'; import { URI } from '../../../../base/common/uri.js'; import { ICodeEditor, isCodeEditor } from '../../../../editor/browser/editorBrowser.js'; import { EditorAction2 } from '../../../../editor/browser/editorExtensions.js'; @@ -678,10 +678,8 @@ export class TestingOutputPeekController extends Disposable implements IEditorCo class TestResultsPeek extends PeekViewWidget { - private static lastHeightInLines?: number; - - private readonly visibilityChange = this._disposables.add(new Emitter()); public readonly current = observableValue('testPeekCurrent', undefined); + private resizeOnNextContentHeightUpdate = false; private content!: TestResultsViewContent; private scopedContextKeyService!: IContextKeyService; private dimension?: dom.Dimension; @@ -701,10 +699,22 @@ class TestResultsPeek extends PeekViewWidget { super(editor, { showFrame: true, frameWidth: 1, showArrow: true, isResizeable: true, isAccessible: true, className: 'test-output-peek' }, instantiationService); this._disposables.add(themeService.onDidColorThemeChange(this.applyTheme, this)); - this._disposables.add(this.onDidClose(() => this.visibilityChange.fire(false))); peekViewService.addExclusiveWidget(editor, this); } + protected override _getMaximumHeightInLines(): number | undefined { + const defaultMaxHeight = super._getMaximumHeightInLines(); + const contentHeight = this.content?.contentHeight; + if (!contentHeight) { // undefined or 0 + return defaultMaxHeight; + } + + const lineHeight = this.editor.getOption(EditorOption.lineHeight); + // 41 is experimentally determined to be the overhead of the peek view itself + // to avoid showing scrollbars by default in its content. + return Math.min(defaultMaxHeight || Infinity, (contentHeight + 41) / lineHeight); + } + private applyTheme() { const theme = this.themeService.getColorTheme(); const current = this.current.get(); @@ -764,6 +774,27 @@ class TestResultsPeek extends PeekViewWidget { protected override _fillBody(containerElement: HTMLElement): void { this.content.fillBody(containerElement); + + // Resize on height updates for a short time to allow any heights made + // by editor contributions to come into effect before. + const contentHeightSettleTimer = this._disposables.add(new RunOnceScheduler(() => { + this.resizeOnNextContentHeightUpdate = false; + }, 500)); + + this._disposables.add(this.content.onDidChangeContentHeight(height => { + if (!this.resizeOnNextContentHeightUpdate || !height) { + return; + } + + const displayed = this._getMaximumHeightInLines(); + if (displayed) { + this._relayout(Math.min(displayed, this.getVisibleEditorLines() / 2)); + if (!contentHeightSettleTimer.isScheduled()) { + contentHeightSettleTimer.schedule(); + } + } + })); + this._disposables.add(this.content.onDidRequestReveal(sub => { TestingOutputPeekController.get(this.editor)?.show(sub instanceof MessageSubject ? sub.messageUri @@ -780,7 +811,6 @@ class TestResultsPeek extends PeekViewWidget { return this.showInPlace(subject); } - const message = subject.message; const previous = this.current; const revealLocation = subject.revealLocation?.range.getStartPosition(); if (!revealLocation && !previous) { @@ -792,13 +822,8 @@ class TestResultsPeek extends PeekViewWidget { return this.showInPlace(subject); } - // If there is a stack we want to display, ensure the default size is large-ish - const peekLines = TestResultsPeek.lastHeightInLines || Math.max( - inspectSubjectHasStack(subject) ? Math.ceil(this.getVisibleEditorLines() / 2) : 0, - hintMessagePeekHeight(message) - ); - - this.show(revealLocation, peekLines); + this.resizeOnNextContentHeightUpdate = true; + this.show(revealLocation, 10); // 10 is just a random number, we resize once content is available this.editor.revealRangeNearTopIfOutsideViewport(Range.fromPositions(revealLocation), ScrollType.Smooth); return this.showInPlace(subject); @@ -832,11 +857,6 @@ class TestResultsPeek extends PeekViewWidget { await this.content.reveal({ subject, preserveFocus: false }); } - protected override _relayout(newHeightInLines: number): void { - super._relayout(newHeightInLines); - TestResultsPeek.lastHeightInLines = newHeightInLines; - } - /** @override */ protected override _doLayoutBody(height: number, width: number) { super._doLayoutBody(height, width); @@ -919,23 +939,11 @@ export class TestResultsView extends ViewPane { } } -const hintMessagePeekHeight = (msg: ITestMessage) => { - const msgHeight = ITestMessage.isDiffable(msg) - ? Math.max(hintPeekStrHeight(msg.actual), hintPeekStrHeight(msg.expected)) - : hintPeekStrHeight(typeof msg.message === 'string' ? msg.message : msg.message.value); - - // add 8ish lines for the size of the title and decorations in the peek. - return msgHeight + 8; -}; - const firstLine = (str: string) => { const index = str.indexOf('\n'); return index === -1 ? str : str.slice(0, index); }; - -const hintPeekStrHeight = (str: string) => Math.min(count(str, '\n'), 24); - function getOuterEditorFromDiffEditor(codeEditorService: ICodeEditorService): ICodeEditor | null { const diffEditors = codeEditorService.listDiffEditors();