diff --git a/crates/ark/src/console/console_graphics.rs b/crates/ark/src/console/console_graphics.rs index 21e364cde..6fba69937 100644 --- a/crates/ark/src/console/console_graphics.rs +++ b/crates/ark/src/console/console_graphics.rs @@ -39,6 +39,5 @@ impl Console { let dc = Rc::clone(self.device_context()); dc.process_changes(self); dc.clear_execution_context(); - dc.clear_pending_origin(); } } diff --git a/crates/ark/src/plots/graphics_device.rs b/crates/ark/src/plots/graphics_device.rs index 56b33ec75..4fe45ca3e 100644 --- a/crates/ark/src/plots/graphics_device.rs +++ b/crates/ark/src/plots/graphics_device.rs @@ -187,10 +187,10 @@ pub(crate) struct DeviceContext { /// provides the file attribution even though the execute_request came from the console. source_context_stack: RefCell>, - /// The plot origin captured eagerly when drawing starts (i.e. when `has_changes` - /// transitions from false to true). This is necessary because the source context - /// stack may be popped before `process_changes()` runs (e.g. `source()` completes - /// before the execute request finishes), so we snapshot the origin at drawing time. + /// The plot origin for the current page, captured eagerly whenever drawing + /// starts and no snapshot is pending. This is necessary because the source + /// context stack may be popped before `process_changes()` runs (e.g. `source()` + /// completes before the execute request finishes). pending_origin: RefCell>>, } @@ -269,15 +269,14 @@ impl DeviceContext { self.source_context_stack.borrow().last().cloned() } - /// Eagerly capture the plot origin so it's available when `process_changes()` runs later. - /// Called when drawing first starts for a change set, since the source context stack - /// may be popped before we get a chance to consume it. + /// Eagerly capture the plot origin so it's available when `process_changes()` + /// runs later, since the source context stack may be popped before then. fn set_pending_origin(&self, origin: Option) { self.pending_origin.replace(Some(origin)); } /// Clear any unconsumed pending origin. - pub(crate) fn clear_pending_origin(&self) { + fn clear_pending_origin(&self) { self.pending_origin.replace(None); } @@ -338,10 +337,13 @@ impl DeviceContext { let old_has_changes = self.has_changes.get(); self.has_changes.replace(old_has_changes || is_drawing); - // Eagerly capture the plot origin when drawing first starts for this - // change set. The source context stack may be popped before - // `process_changes()` runs, so we snapshot it now while it's available. - if !old_has_changes && is_drawing { + // Eagerly capture the plot origin while the source context stack is still + // available, since `process_changes()` may not run until after `source()` + // popped it. Capture whenever we have no snapshot rather than only on the + // `has_changes` false->true edge, which never comes back around for a new + // page started while earlier changes are still pending (e.g. `dev.hold()`). + let needs_origin = self.pending_origin.borrow().is_none(); + if is_drawing && needs_origin { let ctx = self.capture_execution_context(); let origin = self.capture_plot_origin(&ctx); self.set_pending_origin(origin); diff --git a/crates/ark/tests/integration/plots.rs b/crates/ark/tests/integration/plots.rs index 8feb38367..f30ca73b0 100644 --- a/crates/ark/tests/integration/plots.rs +++ b/crates/ark/tests/integration/plots.rs @@ -585,6 +585,52 @@ fn test_plot_source_context_stacking() { assert!(result_a.contains(&format!("$origin_uri\n[1] \"{}\"", file_a.uri_id))); } +/// Test that a plot created inside `source()` keeps its origin when rendering +/// is held past the end of the `source()` call. +/// +/// https://github.com/posit-dev/ark/issues/1334 +/// +/// While a hold is active, `process_changes()` deliberately leaves +/// `has_changes` set so it can notify once the hold is released. That leaves +/// `has_changes` already true when the next plot starts drawing, which skips +/// the eager origin capture in `hook_mode()`. By the time the hold is released, +/// `source()` has returned and its `defer()` has popped the source context, so +/// the origin has nothing left to fall back to. +#[test] +fn test_plot_origin_survives_hold_across_source() { + let frontend = DummyArkFrontend::lock(); + + // Two plots under a hold, with no `dev.flush()`, so the notification is + // deferred past the end of `source()`. + let file = SourceFile::new("invisible(dev.hold())\nplot(1:5)\nplot(1:3)\n"); + + let code = format!("source('{}')", file.path); + frontend.send_execute_request(&code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // Release the hold in a separate request, after source() has returned. + frontend.send_execute_request("invisible(dev.flush())", ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + let display_id = frontend.recv_iopub_display_data_id(); + assert!(!display_id.is_empty()); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + let query = format!(".ps.graphics.get_metadata('{display_id}')"); + frontend.send_execute_request(&query, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + let result = frontend.recv_iopub_execute_result(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + assert!(result.contains(&format!("$origin_uri\n[1] \"{}\"", file.uri_id))); +} + /// Test that plots rendered with fig-width/fig-height metadata produce /// a PNG at the expected pixel dimensions (inches * 96 DPI). #[test]