Respect plot sizing metadata when present by jmcphers · Pull Request #1119 · posit-dev/ark

jmcphers · 2026-03-16T23:29:36Z

This change adds the ability to change the way plots are drawn using four new values in the positron field of execute_request

fig-width / fig-height from Quarto (takes precedence), from the cell options
output_width_px and output_pixel_ratio from the notebook and/or source editor. These are device metrics that can change at any time.

Used together, these values cause most plots in notebooks and Quarto documents to be drawn at the user's preferred size, and at the scaling appropriate for their display.

Quarto:

Jupyter:

lionel- · 2026-03-17T06:22:30Z

crates/amalthea/src/wire/execute_request.rs

+    #[serde(rename = "fig-width")]
+    pub fig_width: Option<f64>,
+
+    /// Figure height in inches, set by Quarto
+    #[serde(rename = "fig-height")]
+    pub fig_height: Option<f64>,
+
+    /// Output area width in pixels
+    pub output_width_px: Option<f64>,
+
+    /// Device pixel ratio of the output area
+    pub output_pixel_ratio: Option<f64>,


Although the dependency direction would be a little weird, I'd love if we could reuse PlotRenderSettings from the plot comm for this. Looks like we only need to add output_width_px.

At the very least, duplicating the data structure if we can't reuse it would mean that the plot settings are nicely namespaced in their own struct and keep the parent struct tidy.

Yea at the very least it feels like this should be something like

pub plot_render_settings: Option<PlotRenderSettings>

But if @lionel- is suggesting that perhaps this should not be a part of ExecuteRequestPositron at all, then I'd also be fine with that (I can't quite tell)

It's fine in the execute request, just needs some consolidation as in your snippet.

Consolidated in bed45b0

Nice! In this case though, I meant collecting all the plot setting fields in a nested struct in the execute_request.positron struct.

lionel- · 2026-03-17T06:28:49Z

crates/ark/src/console/console_repl.rs

+                    let has_sizing = p.fig_width.is_some() ||
+                        p.fig_height.is_some() ||
+                        p.output_width_px.is_some();
+                    has_sizing.then_some(graphics_device::PlotSizingMetadata {
+                        fig_width: p.fig_width,
+                        fig_height: p.fig_height,
+                        output_width_px: p.output_width_px,
+                        output_pixel_ratio: p.output_pixel_ratio,
+                    })
+                });


This is a bit domain-specific for this file. Perhaps make a from_execute_request() method on PlotSizingMetadata?

lionel- · 2026-03-17T06:41:44Z

crates/ark/src/plots/graphics_device.rs

        );

-        let settings = self.prerender_settings.get();
+        // Use sizing metadata for prerender if available, otherwise fall back to default


Would be helpful to mention that we're merging any overrides available for this plot (possibly not exhaustive) into current frontend defaults.

That feels kind of wrong to me? i.e. for plot specific metadata to affect global state in this way

The global state is copied locally from the Cell, then the overrides are merged in the copy and used in the pre-render. Not global side effect IIUC.

lionel- · 2026-03-17T06:44:46Z

crates/ark/src/plots/graphics_device.rs

+    ///
+    /// Returns dimensions in CSS/logical pixels. The R rendering layer handles
+    /// physical pixel scaling via the separate `pixel_ratio` parameter.
+    fn intrinsic_size_to_plot_size(intrinsic: &IntrinsicSize) -> PlotSize {


IntrinsicSize belongs to Amalthea but you could make an extension trait IntrinsicSizeExt in Ark and make this IntrinsicSizeExt::to_plot_size()?

yep! 1bd3950

lionel- · 2026-03-17T06:51:52Z

crates/ark/src/plots/graphics_device.rs

+    /// If `fig_width`/`fig_height` are both set, returns an intrinsic size in inches
+    /// with source "Quarto". Otherwise returns `None` (output_width_px affects render
+    /// size but is not an intrinsic size).
+    fn intrinsic_size_from_metadata(sizing: &PlotSizingMetadata) -> Option<IntrinsicSize> {


PlotSizingMetadata::to_intrinsic_size()

Decided to just use IntrinsicSize directly

lionel- · 2026-03-17T06:52:21Z

crates/ark/src/plots/graphics_device.rs

+    /// If `fig_width`/`fig_height` are set, converts inches to logical pixels.
+    /// If only `output_width_px` is set, uses it as width with a default aspect ratio.
+    /// Otherwise returns `None` (use the default prerender settings).
+    fn prerender_overrides_from_metadata(sizing: &PlotSizingMetadata) -> Option<(PlotSize, f64)> {


PlotSizingMetadata::to_prerender_overrides()

Refactored out of PlotSizingMetadata

lionel- · 2026-03-17T06:56:04Z

crates/ark/src/plots/graphics_device.rs

    metadata: RefCell<HashMap<PlotId, PlotMetadata>>,

+    /// Mapping of plot ID to its intrinsic size (set when Quarto specifies fig-width/fig-height)
+    intrinsic_sizes: RefCell<HashMap<PlotId, IntrinsicSize>>,


We've now got 3 hashmaps over PlotId. Is it time for a struct?

Good question! The one that tracks sockets has a different lifetime, but I've combined the other two.

…-sizing

DavisVaughan · 2026-03-17T18:54:00Z

crates/ark/src/plots/graphics_device.rs

+/// When `fig_width`/`fig_height` are set, they take priority (Quarto-specified size in inches).
+/// Otherwise, `output_width_px` is used as the render width.
+#[derive(Debug, Clone)]
+pub(crate) struct PlotSizingMetadata {


Yea this does feel very close to PlotRenderSettings!

Removed in favor of PlotRenderSettings!

DavisVaughan · 2026-03-17T18:55:32Z

crates/ark/src/plots/graphics_device.rs

+                            Some(intrinsic) => Self::intrinsic_size_to_plot_size(&intrinsic),
+                            None => {
+                                return Err(anyhow!(
+                                    "Intrinsically sized plots are not yet supported."


Is that the right message? Or is it just that we somehow are missing an intrinsic size for this id?

Good catch, it's the latter one. I've updated the message.

DavisVaughan · 2026-03-17T18:57:29Z

crates/ark/src/plots/graphics_device.rs

    }

+    /// Default DPI for converting inches to pixels (matches R's default on macOS).
+    const DEFAULT_DPI: f64 = 96.0;


FWIW, in graphics.R

#' Default OS resolution in PPI (pixels per inch) #' #' Thomas thinks these are "more correct than any other numbers." Specifically, #' macOS uses 96 DPI for its internal scaling, but this is user definable on #' Windows. #' #' This corresponds to a scaling factor that tries to make things that appear #' "on screen" be as close to the size in which they are actually printed at, #' which has always been tricky. default_resolution_in_pixels_per_inch <- function() { if (Sys.info()[["sysname"]] == "Darwin") { 96L } else { 72L } }

DavisVaughan · 2026-03-17T19:04:08Z

crates/ark/src/plots/graphics_device.rs

+                width: w,
+                height: h,
+                unit: PlotUnit::Inches,
+                source: String::from("Quarto"),


Calling out that hardcoding Quarto doesn't feel quite right to me

There's no other way for these values to be set AFAIK... I think the only way to avoid this would be to give this intrinsic size a generic name (like "document"? "external"?), or to add another field to the metadata naming the source of the figure size values that Positron could set. Does either of those seem right to you?

So I'm looking at

pub struct IntrinsicSize { /// The width of the plot pub width: f64, /// The height of the plot pub height: f64, /// The unit of measurement of the plot's dimensions pub unit: PlotUnit, /// The source of the intrinsic size e.g. 'Matplotlib' pub source: String }

and mentally I'm understanding source here as "the thing that provided this width/height/unit override"

So to me it would make sense for this proposed ExecutionMetadata to have a source?: String field that is "the source of all metadata overrides", which Quarto would set to itself when it calls session.execute() and would flow through to ark.

That feels kind of right to me, and may be useful for other things in the future?

If source is not set in ExecutionMetadata, then as a fallback you could probably say that the source for the IntrinsicSize is something like "Metadata", but I also feel like it would be kind of reasonable for IntrinsicSize's source field to be optional, and we'd just not provide anything at all if ExecutionMetadata lacks the source field but does happen to provide fig_height and fig_width. Sort of a recognition of "we got these things but we don't know where they originally came from". And if Positron displays this information somehow to the user, it would just not show the source field or would show ??? if it had to show something.

Does that make any sense?

DavisVaughan · 2026-03-17T19:05:38Z

crates/ark/src/plots/graphics_device.rs

    }

-    fn create_display_data_plot(&self, id: &PlotId) -> Result<serde_json::Value, anyhow::Error> {
-        // TODO: Take these from R global options? Like `ark.plot.width`?


This TODO should stay

For example, how would, say, Jupyterlab or Zed set this?

Well, while I'm in here I guess I could just make it read from global options :-) 11542ad

DavisVaughan · 2026-03-17T19:17:52Z

crates/ark/src/plots/graphics_device.rs

+                return Some((
+                    PlotSize {
+                        width: (w * Self::DEFAULT_DPI).round() as i64,
+                        height: (h * Self::DEFAULT_DPI).round() as i64,


So, IIUC, your goal is to massage the sizing information into pixels for the R side of the graphics engine

#' ... #' @param width The plot width, in pixels. #' @param height The plot height, in pixels. #' @param pixel_ratio The device pixel ratio (e.g. 1 for standard displays, 2 #' for retina displays) #' ... with_graphics_device <- function(

...even if on the R side we possibly invert them back from pixels -> inches depending on the graphics device

finalize_device_arguments <- function(format, width, height, pixel_ratio) { if (format == "png" || format == "jpeg" || format == "tiff") { # These devices require `width` and `height` in pixels, which is what # they are provided in already. For pixel based devices, all relevant # values are upscaled by `pixel_ratio`. # # `res` is nominal resolution specified in pixels-per-inch (ppi). return(list( res = default_resolution_in_pixels_per_inch() * pixel_ratio, width = width * pixel_ratio, height = height * pixel_ratio )) } if (format == "svg" || format == "pdf") { # These devices require `width` and `height` in inches, but they are # provided to us in pixels, so we have to perform a conversion here. # For vector based devices, providing the size in inches implicitly # tells the device the relative size to use for things like text, # since that is the absolute unit (pts are based on inches). # # Thomas says the math for `width` and `height` here are correct, i.e. # we don't also multiply `default_resolution_in_pixels_per_inch()` by # `pixel_ratio` like we do above, which would have made it cancel out of # the equation below. # # There is no `res` argument for these devices. return(list( res = NULL, width = width * pixel_ratio / default_resolution_in_pixels_per_inch(), height = height * pixel_ratio / default_resolution_in_pixels_per_inch() )) } stop("Internal error: Unknown plot `format`.") }

I think that probably makes sense, but if that is the case I do think we should definitely make an attempt to keep default_resolution_in_pixels_per_inch() in sync with DEFAULT_DPI (could even give them the same name with a comment that links them together, or go all the way to making the R one just return the Rust variable)

It does feel a LITTLE weird to go through inches -> px -> inches here! But don't think there's any arrangement of this that isn't awkward somewhere.

I've updated it so that Rust is the keeper of the default DPI setting and R fetches it from there.

DavisVaughan · 2026-03-17T19:19:36Z

crates/ark/src/plots/graphics_device.rs

        );

-        let settings = self.prerender_settings.get();
+        // Use sizing metadata for prerender if available, otherwise fall back to default


That feels kind of wrong to me? i.e. for plot specific metadata to affect global state in this way

DavisVaughan · 2026-03-17T19:21:19Z

crates/ark/tests/plots.rs

+    // PNG IHDR: 8-byte signature, 4-byte chunk length, 4-byte "IHDR", then width (4) and height (4)
+    let width = u32::from_be_bytes([bytes[16], bytes[17], bytes[18], bytes[19]]);
+    let height = u32::from_be_bytes([bytes[20], bytes[21], bytes[22], bytes[23]]);
+    (width, height)


That is wild

You can thank Claude for this one, its initial run at test coverage for this feature was 200 lines of tests to be sure that DPI -> inch conversion was correct (yes Claude good job I am worried that someone might break the division operator in a future change).

I asked it to do a more meaningful higher-level test instead and it came up with the idea of checking the actual size of the emitted PNG which I would never have thought to have done!

lionel-

Looks good! but would be nice to collect plot settings in a struct nested in the positron extension of execute_request, instead of the flat structure.

lionel- · 2026-03-18T12:21:58Z

crates/amalthea/src/wire/execute_request.rs

+    #[serde(rename = "fig-width")]
+    pub fig_width: Option<f64>,
+
+    /// Figure height in inches, set by Quarto
+    #[serde(rename = "fig-height")]
+    pub fig_height: Option<f64>,
+
+    /// Output area width in pixels
+    pub output_width_px: Option<f64>,
+
+    /// Device pixel ratio of the output area
+    pub output_pixel_ratio: Option<f64>,


Nice! In this case though, I meant collecting all the plot setting fields in a nested struct in the execute_request.positron struct.

jmcphers · 2026-03-18T17:01:16Z

Yes, that'd be nicer! But Positron itself doesn't necessarily know about all these values; it allows extensions to inject additional metadata, and e.g. fig-width and fig-height can be added by Quarto without Positron knowing what they are for. We could have Quarto group them, or have Positron look for these fields specifically and promote them into a structure if we decide to; my goal was to make it possible to add info to executions without needing to add Positron plumbing every time.

DavisVaughan

Hopefully it shows you my response to your comment about IntrinsicSize's source #1119 (comment)

DavisVaughan · 2026-03-18T18:54:44Z

crates/amalthea/src/wire/execute_request.rs

 pub struct ExecuteRequestPositron {
    pub code_location: Option<JupyterPositronLocation>,
+
+    /// Figure width in inches, set by Quarto
+    #[serde(rename = "fig-width")]
+    pub fig_width: Option<f64>,
+
+    /// Figure height in inches, set by Quarto
+    #[serde(rename = "fig-height")]
+    pub fig_height: Option<f64>,
+
+    /// Output area width in pixels
+    pub output_width_px: Option<f64>,
+
+    /// Device pixel ratio of the output area
+    pub output_pixel_ratio: Option<f64>,


I believe I would be ok with keeping these nested 1 layer deeper under execution_metadata: Option<ExecutionMetadata>, mirroring your initial implementation here posit-dev/positron#12513

IIUC, we could frame it as:

code_location is "officially supported" by Positron and is Positron provided

execution_metadata is "passed through" by Positron and may contain fields that any extension feels like passing through in their session.execute() calls.

On the Ark side, we'd just "happen to recognize" some specific execution_metadata fields of fig_width, fig_height, etc, and ignore (probably?) anything else that comes over the wire that we don't recognize.

But I like the idea of containing these extension-specific-fields-that-ark-happens-to-recognize scoped to execution_metadata

DavisVaughan · 2026-03-18T19:09:49Z

crates/ark/src/plots/graphics_device.rs

+                width: w,
+                height: h,
+                unit: PlotUnit::Inches,
+                source: String::from("Quarto"),


So I'm looking at

pub struct IntrinsicSize { /// The width of the plot pub width: f64, /// The height of the plot pub height: f64, /// The unit of measurement of the plot's dimensions pub unit: PlotUnit, /// The source of the intrinsic size e.g. 'Matplotlib' pub source: String }

and mentally I'm understanding source here as "the thing that provided this width/height/unit override"

So to me it would make sense for this proposed ExecutionMetadata to have a source?: String field that is "the source of all metadata overrides", which Quarto would set to itself when it calls session.execute() and would flow through to ark.

That feels kind of right to me, and may be useful for other things in the future?

If source is not set in ExecutionMetadata, then as a fallback you could probably say that the source for the IntrinsicSize is something like "Metadata", but I also feel like it would be kind of reasonable for IntrinsicSize's source field to be optional, and we'd just not provide anything at all if ExecutionMetadata lacks the source field but does happen to provide fig_height and fig_width. Sort of a recognition of "we got these things but we don't know where they originally came from". And if Positron displays this information somehow to the user, it would just not show the source field or would show ??? if it had to show something.

Does that make any sense?

jmcphers added 4 commits March 16, 2026 12:51

pull metadata from execution request

ff5e644

fix dpi doubling

58721cf

tests or it didn't happen

2c5bc5d

tidy up a lil bit

9739f80

jmcphers requested a review from DavisVaughan March 16, 2026 23:29

clean up formatting

f4ae41b

jmcphers mentioned this pull request Mar 17, 2026

Size plots appropriately in R notebooks and Quarto inline output posit-dev/positron#12539

Open

lionel- reviewed Mar 17, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into feature/metadata-plot…

0cb3ebf

…-sizing

DavisVaughan reviewed Mar 17, 2026

View reviewed changes

jmcphers added 9 commits March 17, 2026 13:41

use PlotRenderSettings

bed45b0

update how we extract metadata from the execute request

0eb6035

consolidate two plot maps

a428e07

use OS specific DPI

3738241

get default DPI right from rust

5f8d263

respect settings

11542ad

the test runs on all operating systems!

16e5ac9

use an extension trait

1bd3950

better error message

2249aa7

jmcphers requested review from DavisVaughan and lionel- March 17, 2026 23:36

lionel- approved these changes Mar 18, 2026

View reviewed changes

DavisVaughan approved these changes Mar 18, 2026

View reviewed changes

Conversation

jmcphers commented Mar 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lionel- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmcphers commented Mar 18, 2026

Uh oh!

DavisVaughan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavisVaughan left a comment •

edited

Loading