Skip to content

Respect plot sizing metadata when present #1119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jmcphers merged 15 commits into from
Mar 19, 2026
Merged

Respect plot sizing metadata when present #1119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ff5e644
pull metadata from execution request
jmcphers Mar 16, 2026
58721cf
fix dpi doubling
jmcphers Mar 16, 2026
2c5bc5d
tests or it didn't happen
jmcphers Mar 16, 2026
9739f80
tidy up a lil bit
jmcphers Mar 16, 2026
f4ae41b
clean up formatting
jmcphers Mar 16, 2026
0cb3ebf
Merge remote-tracking branch 'origin/main' into feature/metadata-plot…
jmcphers Mar 17, 2026
bed45b0
use PlotRenderSettings
jmcphers Mar 17, 2026
0eb6035
update how we extract metadata from the execute request
jmcphers Mar 17, 2026
a428e07
consolidate two plot maps
jmcphers Mar 17, 2026
3738241
use OS specific DPI
jmcphers Mar 17, 2026
5f8d263
get default DPI right from rust
jmcphers Mar 17, 2026
11542ad
respect settings
jmcphers Mar 17, 2026
16e5ac9
the test runs on all operating systems!
jmcphers Mar 17, 2026
1bd3950
use an extension trait
jmcphers Mar 17, 2026
2249aa7
better error message
jmcphers Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion crates/amalthea/src/wire/execute_request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,23 @@ pub struct ExecuteRequest {
}

#[serde_with::skip_serializing_none]
#[derive(Debug, Serialize, Deserialize, Clone)]
#[derive(Debug, Serialize, Deserialize, Clone, Default)]
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>,
Comment on lines +49 to +60
Copy link
Copy Markdown
Contributor

@lionel- lionel- Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@DavisVaughan DavisVaughan Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

@lionel- lionel- Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@jmcphers jmcphers Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consolidated in bed45b0

Copy link
Copy Markdown
Contributor

@lionel- lionel- Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 45 to +60
Copy link
Copy Markdown
Contributor

@DavisVaughan DavisVaughan Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@lionel- lionel- Mar 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think there is still an agreed upon protocol here. In this case, it seems it's one that is owned by Ark: Ark knows about these fields and Quarto is using them.

So, should it be in execute_request.ark rather than execute_request.positron? And then we can nest in plot_render_settings: this is no longer pass through, this is part of an established and documented data structure.

On the frontend side, passthrough metadata is taken with a namespace string (in this case set to 'ark') that represents the field to inject in the execute request.

wdyt @jmcphers @DavisVaughan?

Copy link
Copy Markdown
Contributor

@lionel- lionel- Mar 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or the other way around, execute_request.quarto.

On the frontend side, passthrough metadata is taken with a namespace string (in this case set to 'ark') that represents the field to inject in the execute request.

There could be multiple namespaced sets of params, so I guess it's just a passthrough object and caller makes sure the params are appropriately namespaced (like { quarto: { ...}, ark: { ... } }, then Positron injects these in the execute request.

}

#[derive(Debug, Serialize, Deserialize, Clone)]
Expand Down
10 changes: 9 additions & 1 deletion crates/ark/src/console/console_repl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1288,12 +1288,20 @@ impl Console {
reply_tx,
});

// Push execution context to graphics device for plot attribution.
// Push execution context to graphics device for plot attribution
// and optional sizing overrides from Quarto.
let code_location = exec_req.code_location().log_err().flatten();
let (render_settings, intrinsic_size) = exec_req
.positron
.as_ref()
.map(graphics_device::compute_plot_overrides)
.unwrap_or((None, None));
graphics_device::on_execute_request(
originator.header.msg_id.clone(),
exec_req.code.clone(),
code_location,
render_settings,
intrinsic_size,
);

input
Expand Down
6 changes: 1 addition & 5 deletions crates/ark/src/modules/positron/graphics.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,11 +421,7 @@ finalize_device_arguments <- function(format, width, height, pixel_ratio) {
#' "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
}
.ps.Call("ps_graphics_default_dpi")
}

#' Determines the default device `type` for png, jpeg, and tiff
Expand Down
Loading
Loading