Skip to content

Commit 35d4a17

Browse files
authored
Size plots appropriately in R notebooks and Quarto inline output (#12539)
This change adds metadata to execution requests so that it is possible for kernels to size plots to fit the viewport/DPI. For Quarto, we add cell options (such as `fig-width` and `fig-height`) to the execution metadata; for both Quarto inline outputs and Jupyter editors, we add the width of the output field and the device pixel ratio to the execution request. In notebooks, this means that your plots render crisply on Retina displays and are sized appropriately to your viewport: <img width="768" height="831" alt="image" src="https://github.com/user-attachments/assets/d2a5ac46-afb3-4a88-9b79-deab6d842b41" /> In Quarto inline outputs, this means that figure width/height are respected, too, so you get a preview with the correct aspect ratio: <img width="758" height="363" alt="image" src="https://github.com/user-attachments/assets/98a1505a-8620-43b0-ba0a-bec65ec01daa" /> Requires posit-dev/ark#1119 Requires posit-dev/qa-example-content#116 Related to quarto-dev/quarto#938 Addresses #8104 Addresses #12150 ### Release Notes <!-- Optionally, replace `N/A` with text to be included in the next release notes. The `N/A` bullets are ignored. If you refer to one or more Positron issues, these issues are used to collect information about the feature or bugfix, such as the relevant language pack as determined by Github labels of type `lang: `. The note will automatically be tagged with the language. These notes are typically filled by the Positron team. If you are an external contributor, you may ignore this section. --> #### New Features - Quarto inline output for R now respects `fig-width` and `fig-height` options (#12150) #### Bug Fixes - Plots in R notebooks are now sized correctly (#8104) ### QA Notes This PR doesn't attempt to size Python plots, though it takes the first step towards doing so by adding the right metadata. I added a hidden magic you can use to see what is being sent to Python: ```python %_positron_exec_metadata ``` The Ark equivalent is `.ps.internal(active_request())`. It also doesn't apply to the Plots pane, just notebook-style execution. We'll need to wait for a new Quarto extension to do that (quarto-dev/quarto#938). Test tags: `@:quarto` `@positron-notebooks` `@notebooks`
1 parent f234e05 commit 35d4a17

File tree

14 files changed

+486
-13
lines changed

14 files changed

+486
-13
lines changed

extensions/positron-python/python_files/posit/positron/positron_ipkernel.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,17 @@ def connection_show(self, line: str) -> None:
222222
except TypeError as e:
223223
raise UsageError(f"cannot show object of type '{get_qualname(info.obj)}'") from e
224224

225+
@line_magic
226+
def _positron_exec_metadata(self, line: str) -> None: # noqa: ARG002
227+
"""Print the positron execution metadata from the current execute_request as JSON."""
228+
import json as _json
229+
230+
kernel = self.shell.kernel
231+
parent = kernel.get_parent("shell")
232+
content: dict = cast("dict", parent.get("content", {}))
233+
positron_meta: dict = cast("dict", content.get("positron", {}))
234+
print(_json.dumps(positron_meta, sort_keys=True))
235+
225236

226237
_traceback_file_link_re = re.compile(r"^(File \x1b\[\d+;\d+m)(.+):(\d+)")
227238

extensions/positron-r/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1094,7 +1094,7 @@
10941094
},
10951095
"positron": {
10961096
"binaryDependencies": {
1097-
"ark": "0.1.242"
1097+
"ark": "0.1.244"
10981098
},
10991099
"minimumRVersion": "4.2.0",
11001100
"minimumRenvVersion": "1.0.9"

src/vs/workbench/contrib/notebook/test/browser/testNotebookEditor.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ export function setupInstantiationService(disposables: Pick<DisposableStore, 'ad
254254
function _createTestNotebookEditor(instantiationService: TestInstantiationService, disposables: DisposableStore, cells: MockNotebookCell[]): { editor: IActiveNotebookEditorDelegate; viewModel: NotebookViewModel } {
255255

256256
const viewType = 'notebook';
257-
const notebook = disposables.add(instantiationService.createInstance(NotebookTextModel, viewType, URI.parse('test://test'), cells.map((cell): ICellDto2 => {
257+
const notebook = disposables.add(instantiationService.createInstance(NotebookTextModel, viewType, URI.parse('test://test/notebook'), cells.map((cell): ICellDto2 => {
258258
return {
259259
source: cell[0],
260260
mime: undefined,

src/vs/workbench/contrib/positronNotebook/browser/IPositronNotebookInstance.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,18 @@ import { IHoverManager } from '../../../../platform/hover/browser/hoverManager.j
1818
import { IPositronNotebookContribution } from './positronNotebookExtensions.js';
1919
import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js';
2020

21+
/**
22+
* Metadata about the editor layout that may be relevant for execution, such as
23+
* output sizing.
24+
*/
25+
export interface EditorLayoutMetadata {
26+
/** The width of the output area in pixels */
27+
output_width_px: number;
28+
29+
/** The pixel ratio of the output area (monitor-dependent) */
30+
output_pixel_ratio: number;
31+
}
32+
2133
/**
2234
* Represents a deletion sentinel - a temporary placeholder shown where cells were deleted.
2335
* Sentinels display a red fade animation and provide a restore button.
@@ -439,6 +451,13 @@ export interface IPositronNotebookInstance extends IPositronNotebookEditor {
439451
*/
440452
getContribution<T extends IPositronNotebookContribution>(id: string): T | undefined;
441453

454+
/**
455+
* Computes the output area dimensions for execution metadata.
456+
* Returns undefined if the notebook is not connected to an editor or the
457+
* layout cannot be measured.
458+
*/
459+
getOutputLayoutInfo(): EditorLayoutMetadata | undefined;
460+
442461
/**
443462
* Fire the scroll event for the cells container.
444463
* Called by React when scroll or DOM mutations occur.

src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookInstance.ts

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import { IPositronNotebookCell } from './PositronNotebookCells/IPositronNotebook
2525
import { CellSelectionType, getActiveCell, getEditingCell, getSelectedCells, SelectionState, SelectionStateMachine, toCellRanges } from '../../../contrib/positronNotebook/browser/selectionMachine.js';
2626
import { PositronNotebookContextKeyManager } from './ContextKeysManager.js';
2727
import { IPositronNotebookService } from './positronNotebookService.js';
28-
import { IDeletionSentinel, IPositronNotebookInstance, KernelStatus, NotebookOperationType } from './IPositronNotebookInstance.js';
28+
import { EditorLayoutMetadata, IDeletionSentinel, IPositronNotebookInstance, KernelStatus, NotebookOperationType } from './IPositronNotebookInstance.js';
2929
import { POSITRON_NOTEBOOK_ASSISTANT_AUTO_FOLLOW_KEY } from '../common/positronNotebookConfig.js';
3030
import { getAssistantSettings } from '../common/notebookAssistantMetadata.js';
3131
import { NotebookCellTextModel } from '../../notebook/common/model/notebookCellTextModel.js';
@@ -652,6 +652,71 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
652652
return this.currentContainer;
653653
}
654654

655+
/**
656+
* Computes the output area dimensions for execution metadata.
657+
*/
658+
getOutputLayoutInfo(): EditorLayoutMetadata | undefined {
659+
if (!this.connectedToEditor || !this.currentContainer) {
660+
return undefined;
661+
}
662+
663+
const domNode = this.currentContainer;
664+
const win = DOM.getWindow(domNode);
665+
const output_pixel_ratio = win.devicePixelRatio;
666+
667+
// Best case: measure an existing outputs-inner element directly.
668+
const outputsInner = domNode.querySelector<HTMLElement>(
669+
'.positron-notebook-code-cell-outputs-inner'
670+
);
671+
if (outputsInner && outputsInner.clientWidth > 0) {
672+
const style = win.getComputedStyle(outputsInner);
673+
const paddingLeft = parseFloat(style.paddingLeft) || 0;
674+
const paddingRight = parseFloat(style.paddingRight) || 0;
675+
return {
676+
output_width_px: outputsInner.clientWidth - paddingLeft - paddingRight,
677+
output_pixel_ratio,
678+
};
679+
}
680+
681+
// Fallback: compute from the cells container and intermediate
682+
// elements by reading their computed CSS offsets.
683+
const container = this.cellsContainer ?? domNode;
684+
if (container.clientWidth > 0) {
685+
const containerStyle = win.getComputedStyle(container);
686+
const containerPadding = (parseFloat(containerStyle.paddingLeft) || 0)
687+
+ (parseFloat(containerStyle.paddingRight) || 0);
688+
689+
const cell = container.querySelector<HTMLElement>('.positron-notebook-cell');
690+
const cellMarginLeft = cell
691+
? parseFloat(win.getComputedStyle(cell).marginLeft) || 0
692+
: 0;
693+
694+
// The outputs-inner padding is defined but may not have a
695+
// rendered element yet (first execution). Read from CSS if
696+
// an element exists; otherwise approximate from the
697+
// .positron-notebook-code-cell-outputs-inner rule (0.5rem
698+
// inline = 8px each side at default font size).
699+
const innerEl = container.querySelector<HTMLElement>(
700+
'.positron-notebook-code-cell-outputs-inner'
701+
);
702+
const outputPadding = innerEl
703+
? (parseFloat(win.getComputedStyle(innerEl).paddingLeft) || 0)
704+
+ (parseFloat(win.getComputedStyle(innerEl).paddingRight) || 0)
705+
: 16;
706+
707+
const output_width_px = container.clientWidth
708+
- containerPadding - cellMarginLeft - outputPadding;
709+
if (output_width_px > 0) {
710+
return {
711+
output_width_px,
712+
output_pixel_ratio,
713+
};
714+
}
715+
}
716+
717+
return undefined;
718+
}
719+
655720
/**
656721
* Event fired when the notebook editor widget or a cell editor within it gains focus.
657722
*/

src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookCodeCell.css

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,5 +96,12 @@
9696
.show-hidden-output-button:focus-visible {
9797
outline-offset: 2px !important;
9898
}
99+
100+
/* Scale plot images to fit the output viewport without exceeding
101+
their natural size or changing their aspect ratio. */
102+
& > img {
103+
max-width: 100%;
104+
height: auto;
105+
}
99106
}
100107
}

src/vs/workbench/contrib/positronQuarto/browser/quartoExecutionManager.ts

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,9 @@ import { IRuntimeSessionService, ILanguageRuntimeSession } from '../../../servic
3838
import { ITerminalService } from '../../terminal/browser/terminal.js';
3939
import { TerminalCapability, ICommandDetectionCapability } from '../../../../platform/terminal/common/capabilities/capabilities.js';
4040
import { parseCellExecutionOptions, QuartoCellExecutionOptions, DEFAULT_CELL_EXECUTION_OPTIONS } from '../common/quartoExecutionOptions.js';
41+
import { isCodeEditor } from '../../../../editor/browser/editorBrowser.js';
42+
import { getWindow } from '../../../../base/browser/dom.js';
43+
import { EditorLayoutMetadata } from '../../runtimeNotebookKernel/browser/runtimeNotebookKernel.js';
4144

4245
// Re-export for convenience
4346
export { IQuartoExecutionManager } from '../common/quartoExecutionTypes.js';
@@ -327,6 +330,9 @@ export class QuartoExecutionManager extends Disposable implements IQuartoExecuti
327330
const documentModel = this._documentModelService.getModel(textModel);
328331
const quartoCells = documentModel.cells;
329332

333+
// Compute layout metadata from the active editor for output sizing.
334+
const layoutMetadata = this._getEditorLayoutMetadata();
335+
330336
// For each range, find the containing cell and prepare execution info
331337
interface InlineExecution {
332338
cell: QuartoCodeCell;
@@ -373,7 +379,7 @@ export class QuartoExecutionManager extends Disposable implements IQuartoExecuti
373379

374380
// Get the code content and parse execution options
375381
const code = textModel.getValueInRange(clampedRange);
376-
const { options, optionLineCount } = parseCellExecutionOptions(code);
382+
const { options, optionLineCount, metadata } = parseCellExecutionOptions(code);
377383

378384
// Calculate effective code range (excluding option lines at the start)
379385
let effectiveCodeRange = clampedRange;
@@ -386,12 +392,20 @@ export class QuartoExecutionManager extends Disposable implements IQuartoExecuti
386392
);
387393
}
388394

395+
// Merge metadata from layout, cell YAML options, and external sources.
396+
// Cell YAML options take highest precedence, then external, then layout.
397+
const cellMetadata = Object.keys(metadata).length > 0 ? metadata : undefined;
398+
const externalMetadata = executionMetadata?.[rangeIdx];
399+
const mergedMetadata = cellMetadata || externalMetadata || layoutMetadata
400+
? { ...layoutMetadata, ...externalMetadata, ...cellMetadata }
401+
: undefined;
402+
389403
executions.push({
390404
cell: containingCell,
391405
codeRange: clampedRange,
392406
effectiveCodeRange,
393407
options,
394-
executionMetadata: executionMetadata?.[rangeIdx],
408+
executionMetadata: mergedMetadata,
395409
});
396410
}
397411

@@ -1646,7 +1660,7 @@ export class QuartoExecutionManager extends Disposable implements IQuartoExecuti
16461660

16471661
// Parse execution options from cell code
16481662
const code = textModel.getValueInRange(codeRange);
1649-
const { options, optionLineCount } = parseCellExecutionOptions(code);
1663+
const { options, optionLineCount, metadata } = parseCellExecutionOptions(code);
16501664

16511665
// Calculate effective code range (excluding option lines at the start)
16521666
let effectiveCodeRange = codeRange;
@@ -1663,8 +1677,14 @@ export class QuartoExecutionManager extends Disposable implements IQuartoExecuti
16631677
this._removeFromQueue(documentUri, cell.id);
16641678
await this._persistQueueState(documentUri);
16651679

1666-
// Delegate to the unified range execution with parsed options
1667-
const hadError = await this._executeRange(documentUri, currentCell, effectiveCodeRange, options, token);
1680+
// Delegate to the unified range execution with parsed options.
1681+
// Cell YAML options take highest precedence over layout metadata.
1682+
const cellMetadata = Object.keys(metadata).length > 0 ? metadata : undefined;
1683+
const layoutMetadata = this._getEditorLayoutMetadata();
1684+
const executionMetadata = cellMetadata || layoutMetadata
1685+
? { ...layoutMetadata, ...cellMetadata }
1686+
: undefined;
1687+
const hadError = await this._executeRange(documentUri, currentCell, effectiveCodeRange, options, token, executionMetadata);
16681688
return { hadError, options };
16691689
}
16701690

@@ -2036,6 +2056,23 @@ export class QuartoExecutionManager extends Disposable implements IQuartoExecuti
20362056
}
20372057
}
20382058

2059+
/**
2060+
* Compute output layout metadata from the active text editor.
2061+
* Returns dimensions used by the kernel to size outputs (e.g. plot
2062+
* rendering). Uses the same formula as QuartoOutputViewZone._getWidth().
2063+
*/
2064+
private _getEditorLayoutMetadata(): EditorLayoutMetadata | undefined {
2065+
const editorControl = this._editorService.activeTextEditorControl;
2066+
if (isCodeEditor(editorControl)) {
2067+
const layoutInfo = editorControl.getLayoutInfo();
2068+
return {
2069+
output_width_px: layoutInfo.contentWidth - layoutInfo.verticalScrollbarWidth - 4,
2070+
output_pixel_ratio: getWindow(editorControl.getContainerDomNode()).devicePixelRatio,
2071+
};
2072+
}
2073+
return undefined;
2074+
}
2075+
20392076
/**
20402077
* Restore queue state from ephemeral storage.
20412078
* This is called on startup to restore queued cells that survived a reload.

src/vs/workbench/contrib/positronQuarto/common/quartoExecutionOptions.ts

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import { parse as parseYaml, YamlObjectNode } from '../../../../base/common/yaml.js';
6+
import { parse as parseYaml, YamlNode, YamlObjectNode } from '../../../../base/common/yaml.js';
77

88
/**
99
* Execution options parsed from cell YAML comments.
@@ -55,8 +55,35 @@ export interface ParsedCellOptions {
5555
options: QuartoCellExecutionOptions;
5656
/** Number of option lines at the start of the code */
5757
optionLineCount: number;
58+
/** All non-execution cell options as a plain key-value record */
59+
metadata: Record<string, unknown>;
5860
}
5961

62+
/**
63+
* Recursively convert a YAML AST node to a plain JavaScript value.
64+
*/
65+
function yamlNodeToValue(node: YamlNode): unknown {
66+
switch (node.type) {
67+
case 'string': return node.value;
68+
case 'number': return node.value;
69+
case 'boolean': return node.value;
70+
case 'null': return null;
71+
case 'array': return node.items.map(yamlNodeToValue);
72+
case 'object': {
73+
const result: Record<string, unknown> = {};
74+
for (const prop of node.properties) {
75+
result[prop.key.value] = yamlNodeToValue(prop.value);
76+
}
77+
return result;
78+
}
79+
}
80+
}
81+
82+
/**
83+
* Keys that are handled as execution options and should not appear in metadata.
84+
*/
85+
const EXECUTION_OPTION_KEYS = new Set(['eval', 'error']);
86+
6087
/**
6188
* Parse execution options from cell code content.
6289
*
@@ -75,7 +102,7 @@ export interface ParsedCellOptions {
75102
* @returns Parsed options and the number of option lines at the start
76103
*/
77104
export function parseCellExecutionOptions(code: string): ParsedCellOptions {
78-
const lines = code.split('\n');
105+
const lines = code.replace(/\r\n?/g, '\n').split('\n');
79106
const optionLines: string[] = [];
80107
let optionLineCount = 0;
81108

@@ -93,6 +120,7 @@ export function parseCellExecutionOptions(code: string): ParsedCellOptions {
93120

94121
// Start with defaults
95122
const options: QuartoCellExecutionOptions = { ...DEFAULT_CELL_EXECUTION_OPTIONS };
123+
const metadata: Record<string, unknown> = {};
96124

97125
if (optionLines.length > 0) {
98126
// Parse accumulated option lines as YAML
@@ -112,11 +140,16 @@ export function parseCellExecutionOptions(code: string): ParsedCellOptions {
112140
if (key === 'error' && value.type === 'boolean') {
113141
(options as { error: boolean }).error = value.value;
114142
}
143+
144+
// Collect non-execution options as metadata
145+
if (!EXECUTION_OPTION_KEYS.has(key)) {
146+
metadata[key] = yamlNodeToValue(value);
147+
}
115148
}
116149
}
117150
}
118151

119-
return { options, optionLineCount };
152+
return { options, optionLineCount, metadata };
120153
}
121154

122155
/**
@@ -127,7 +160,7 @@ export function parseCellExecutionOptions(code: string): ParsedCellOptions {
127160
*/
128161
export function extractExecutableCode(code: string): string {
129162
const { optionLineCount } = parseCellExecutionOptions(code);
130-
const lines = code.split('\n');
163+
const lines = code.replace(/\r\n?/g, '\n').split('\n');
131164
return lines.slice(optionLineCount).join('\n');
132165
}
133166

0 commit comments

Comments
 (0)