From 4896083a3376ae28d7b695a197e7a9c15dcf8967 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 10 Mar 2026 10:29:01 +0100 Subject: [PATCH 1/6] Run plot comms synchronously on the R thread --- crates/ark/src/console/console_repl.rs | 13 +- crates/ark/src/plots/graphics_device.rs | 261 +++++++----------------- crates/ark/src/shell.rs | 5 +- 3 files changed, 76 insertions(+), 203 deletions(-) diff --git a/crates/ark/src/console/console_repl.rs b/crates/ark/src/console/console_repl.rs index 0e249fd78..00d7d7a33 100644 --- a/crates/ark/src/console/console_repl.rs +++ b/crates/ark/src/console/console_repl.rs @@ -487,11 +487,7 @@ impl Console { // by https://github.com/posit-dev/ark/blob/bd827e735970ca17102aeddfbe2c3ccf26950a36/crates/ark/src/r_task.rs#L261. // We should be able to remove this escape hatch in `r_task()` by // instantiating an `Console` in unit tests as well. - graphics_device::init_graphics_device( - console.comm_event_tx.clone(), - console.iopub_tx().clone(), - graphics_device_rx, - ); + graphics_device::init_graphics_device(console.iopub_tx().clone(), graphics_device_rx); // Now that R has started and libr and ark have fully initialized, run site and user // level R profiles, in that order @@ -2235,13 +2231,6 @@ impl Console { // might end up being executed on the LSP thread. // https://github.com/rstudio/positron/issues/431 unsafe { R_RunPendingFinalizers() }; - - // Check for Positron render requests. - // - // TODO: This should move to a spawned task that'd be woken up by - // incoming messages on plot comms. This way we'll prevent the delays - // introduced by timeout-based event polling. - graphics_device::on_process_idle_events(); } pub(super) fn eval_env(&self) -> RObject { diff --git a/crates/ark/src/plots/graphics_device.rs b/crates/ark/src/plots/graphics_device.rs index f2fe7afa6..5e34c6cb2 100644 --- a/crates/ark/src/plots/graphics_device.rs +++ b/crates/ark/src/plots/graphics_device.rs @@ -15,7 +15,6 @@ use std::io::BufReader; use std::io::Read; use amalthea::comm::comm_channel::CommMsg; -use amalthea::comm::event::CommEvent; use amalthea::comm::plot_comm::PlotBackendReply; use amalthea::comm::plot_comm::PlotBackendRequest; use amalthea::comm::plot_comm::PlotFrontendEvent; @@ -27,8 +26,7 @@ use amalthea::comm::plot_comm::PlotRenderSettings; use amalthea::comm::plot_comm::PlotResult; use amalthea::comm::plot_comm::PlotSize; use amalthea::comm::plot_comm::UpdateParams; -use amalthea::socket::comm::CommInitiator; -use amalthea::socket::comm::CommSocket; +use amalthea::socket::comm::CommOutgoingTx; use amalthea::socket::iopub::IOPubMessage; use amalthea::wire::display_data::DisplayData; use amalthea::wire::execute_request::CodeLocation; @@ -38,7 +36,6 @@ use anyhow::anyhow; use anyhow::Context; use base64::engine::general_purpose; use base64::Engine; -use crossbeam::channel::Select; use crossbeam::channel::Sender; use harp::exec::RFunction; use harp::exec::RFunctionExt; @@ -53,6 +50,9 @@ use stdext::unwrap; use tokio::sync::mpsc::UnboundedReceiver as AsyncUnboundedReceiver; use uuid::Uuid; +use crate::comm_handler::handle_rpc_request; +use crate::comm_handler::CommHandler; +use crate::comm_handler::CommHandlerContext; use crate::console::Console; use crate::console::SessionMode; use crate::modules::ARK_ENVS; @@ -68,16 +68,15 @@ thread_local! { static DEVICE_CONTEXT: RefCell = panic!("Must access `DEVICE_CONTEXT` from the R thread"); } -const POSITRON_PLOT_CHANNEL_ID: &str = "positron.plot"; +pub const PLOT_COMM_NAME: &str = "positron.plot"; // Expose thread initialization via function so we can keep the structs private. // Must be called from the main R thread. pub(crate) fn init_graphics_device( - comm_event_tx: Sender, iopub_tx: Sender, graphics_device_rx: AsyncUnboundedReceiver, ) { - DEVICE_CONTEXT.set(DeviceContext::new(comm_event_tx, iopub_tx)); + DEVICE_CONTEXT.set(DeviceContext::new(iopub_tx)); // Declare our graphics device as interactive if let Err(err) = RFunction::from(".ps.graphics.register_as_interactive").call() { @@ -134,9 +133,6 @@ struct ExecutionContext { } struct DeviceContext { - /// Channel for sending [CommEvent]s to Positron when plot events occur - comm_event_tx: Sender, - /// Channel for sending [IOPubMessage::DisplayData] and /// [IOPubMessage::UpdateDisplayData] to Jupyter frontends when plot events occur iopub_tx: Sender, @@ -179,9 +175,9 @@ struct DeviceContext { /// device specifications (i.e. for Positron's Plots pane). id: RefCell, - /// Mapping of plot ID to the communication socket used for communicating its - /// rendered results to the frontend. - sockets: RefCell>, + /// Mapping of `PlotId` to comm ID, used for sending update events to + /// existing plot comms via `CommOutgoingTx`. + comm_ids: RefCell>, /// Mapping of plot ID to its metadata (captured at creation time) metadata: RefCell>, @@ -213,16 +209,15 @@ struct DeviceContext { } impl DeviceContext { - fn new(comm_event_tx: Sender, iopub_tx: Sender) -> Self { + fn new(iopub_tx: Sender) -> Self { Self { - comm_event_tx, iopub_tx, has_changes: Cell::new(false), is_new_page: Cell::new(true), is_drawing: Cell::new(false), should_render: Cell::new(true), id: RefCell::new(Self::new_id()), - sockets: RefCell::new(HashMap::new()), + comm_ids: RefCell::new(HashMap::new()), metadata: RefCell::new(HashMap::new()), kind_counters: RefCell::new(HashMap::new()), wrapped_callbacks: WrappedDeviceCallbacks::default(), @@ -474,111 +469,6 @@ impl DeviceContext { format!("{} {}", kind, counter) } - /// Process outstanding RPC requests received from Positron - /// - /// At idle time we loop through our set of plot channels and check if Positron has - /// responded on any of them stating that it is ready for us to replay and render - /// the actual plot, and then send back the bytes that represent that plot. - /// - /// Note that we only send back rendered plots at idle time. This means that if you - /// do something like: - /// - /// ```r - /// for (i in 1:5) { - /// plot(i) - /// Sys.sleep(1) - /// } - /// ``` - /// - /// Then it goes something like this: - /// - At each new page event we tell Positron there we have a new plot for it - /// - Positron sets up 5 blank plot windows and sends back an RPC requesting the plot - /// data - /// - AFTER the entire for loop has finished and we hit idle time, we drop into - /// `process_rpc_requests()` and render all 5 plots at once - /// - /// Practically this seems okay, it is just something to keep in mind. - #[tracing::instrument(level = "trace", skip_all)] - fn process_rpc_requests(&self) { - // Don't try to render a plot if we're currently drawing. - if self.is_drawing.get() { - log::trace!("Refusing to render due to `is_drawing`"); - return; - } - - // Don't try to render a plot if someone is asking us not to, i.e. `dev.hold()` - if !self.should_render.get() { - log::trace!("Refusing to render due to `should_render`"); - return; - } - - // Collect existing sockets into a vector of tuples. - // Necessary for handling Select in a clean way. - let sockets = { - // Refcell Safety: Clone the hashmap so we don't hold a reference for too long - let sockets = self.sockets.borrow().clone(); - sockets.into_iter().collect::>() - }; - - // Dynamically load all incoming channels within the sockets into a single `Select` - let mut select = Select::new(); - for (_id, sockets) in sockets.iter() { - select.recv(&sockets.incoming_rx); - } - - // Check for incoming plot render requests. - // Totally possible to have >1 requests pending, especially if we've plotted - // multiple things in a single chunk of R code. The `Err` case is likely just - // that no channels have any messages, so we don't log in that case. - while let Ok(selection) = select.try_select() { - let socket = sockets - .get(selection.index()) - .expect("Socket should exist for the selection index"); - let id = &socket.0; - let socket = &socket.1; - - // Receive on the "selected" channel - let message = match selection.recv(&socket.incoming_rx) { - Ok(message) => message, - Err(error) => { - // If the channel is disconnected, log and remove it so we don't try - // and `recv()` on it ever again - log::error!("{error:?}"); - // Refcell Safety: Short borrows in the file. - self.sockets.borrow_mut().remove(id); - - // Process remaining messages. Safe to do because we have - // removed the `DeviceContext`'s copy off the sockets but we - // are working through our own copy of them. - continue; - }, - }; - - match message { - CommMsg::Rpc { .. } => { - log::trace!("Handling `RPC` for plot `id` {id}"); - socket.handle_request(message, |req| self.handle_rpc(req, id)); - }, - - // Note that ideally this handler should be invoked before we - // check for `should_render`. I.e. we should acknowledge a plot - // has been closed on the frontend side even when `dev.hold()` - // is active. Doing so would require some more careful - // bookkeeping of the state though, and since this is a very - // unlikely sequence of action nothing really bad happens with - // the current approach, we decided to keep handling here. - CommMsg::Close => { - log::trace!("Handling `Close` for plot `id` {id}"); - self.close_plot(id) - }, - - message => { - log::error!("Received unexpected comm message for plot `id` {id}: {message:?}") - }, - } - } - } - #[tracing::instrument(level = "trace", skip_all, fields(id = %id))] fn handle_rpc( &self, @@ -643,16 +533,10 @@ impl DeviceContext { } #[tracing::instrument(level = "trace", skip(self))] - fn close_plot(&self, id: &PlotId) { - // RefCell safety: Short borrows in the file - self.sockets.borrow_mut().remove(id); - - // Remove metadata for this plot + fn on_plot_closed(&self, id: &PlotId) { + self.comm_ids.borrow_mut().remove(id); self.metadata.borrow_mut().remove(id); - // The plot data is stored at R level. Assumes we're called on the R - // thread at idle time so there's no race issues (see - // `on_process_idle_events()`). if let Err(err) = RFunction::from("remove_recording") .param("id", id) .call_in(ARK_ENVS.positron_ns) @@ -749,18 +633,9 @@ impl DeviceContext { origin, }); - // Let Positron know that we just created a new plot. - let socket = CommSocket::new( - CommInitiator::BackEnd, - id.to_string(), - POSITRON_PLOT_CHANNEL_ID.to_string(), - self.iopub_tx.clone(), - ); - let settings = self.prerender_settings.get(); - // Prepare a pre-rendering of the plot so Positron has something to display immediately - let data = match self.render_plot(id, &settings) { + let open_data = match self.render_plot(id, &settings) { Ok(pre_render) => { let mime_type = Self::get_mime_type(&PlotRenderFormat::Png); @@ -778,14 +653,19 @@ impl DeviceContext { }, }; - let event = CommEvent::Opened(socket.clone(), data); - if let Err(error) = self.comm_event_tx.send(event) { - log::error!("{error:?}"); - } + let plot_comm = PlotComm { + id: id.clone(), + open_data, + }; - // Save our new socket. - // Refcell Safety: Short borrows in the file. - self.sockets.borrow_mut().insert(id.clone(), socket); + match Console::get_mut().comm_register(PLOT_COMM_NAME, Box::new(plot_comm)) { + Ok(comm_id) => { + self.comm_ids.borrow_mut().insert(id.clone(), comm_id); + }, + Err(err) => { + log::error!("Failed to register plot comm: {err:?}"); + }, + } } #[tracing::instrument(level = "trace", skip_all, fields(id = %id))] @@ -845,17 +725,14 @@ impl DeviceContext { fn process_update_plot_positron(&self, id: &PlotId) { log::trace!("Notifying Positron of plot update"); - // Refcell Safety: Make sure not to call other methods from this whole block. - let sockets = self.sockets.borrow(); - - // Find our socket - let socket = unwrap!(sockets.get(id), None => { - // If socket doesn't exist, bail, nothing to update (should be rare, likely a bug?) - log::error!("Can't find socket to update with id: {id}."); - return; - }); + let comm_id = match self.comm_ids.borrow().get(id).cloned() { + Some(id) => id, + None => { + log::error!("Can't find comm to update with id: {id}."); + return; + }, + }; - // Create a pre-rendering of the updated plot let settings = self.prerender_settings.get(); let update_params = match self.render_plot(id, &settings) { Ok(pre_render) => { @@ -879,9 +756,8 @@ impl DeviceContext { let value = serde_json::to_value(PlotFrontendEvent::Update(update_params)).unwrap(); - // Tell Positron we have an updated plot with optional pre-rendering - socket - .outgoing_tx + let outgoing_tx = CommOutgoingTx::new(comm_id, self.iopub_tx.clone()); + outgoing_tx .send(CommMsg::Data(value)) .context("Failed to send update message for id {id}.") .log_err(); @@ -939,36 +815,24 @@ impl DeviceContext { fn render_plot(&self, id: &PlotId, settings: &PlotRenderSettings) -> anyhow::Result { log::trace!("Rendering plot"); - let image_path = r_task(|| unsafe { - RFunction::from(".ps.graphics.render_plot_from_recording") - .param("id", id) - .param("width", RObject::try_from(settings.size.width)?) - .param("height", RObject::try_from(settings.size.height)?) - .param("pixel_ratio", settings.pixel_ratio) - .param("format", settings.format.to_string()) - .call()? - .to::() - }); - - let image_path = match image_path { - Ok(image_path) => image_path, - Err(error) => { - return Err(anyhow::anyhow!( - "Failed to render plot with `id` {id} due to: {error}." - )) - }, - }; + let image_path: String = RFunction::from(".ps.graphics.render_plot_from_recording") + .param("id", id) + .param("width", RObject::try_from(settings.size.width)?) + .param("height", RObject::try_from(settings.size.height)?) + .param("pixel_ratio", settings.pixel_ratio) + .param("format", settings.format.to_string()) + .call()? + .try_into() + .map_err(|err: harp::Error| anyhow!("Failed to render plot with `id` {id}: {err:?}"))?; log::trace!("Rendered plot to {image_path}"); - // Read contents into bytes. let conn = File::open(image_path)?; let mut reader = BufReader::new(conn); let mut buffer = vec![]; reader.read_to_end(&mut buffer)?; - // what an odd interface let data = general_purpose::STANDARD_NO_PAD.encode(buffer); Ok(data) @@ -995,6 +859,34 @@ impl DeviceContext { } } +/// Per-plot comm handler registered in Console's comm table. +/// Delegates RPC handling and lifecycle events to the shared `DeviceContext`. +#[derive(Debug)] +struct PlotComm { + id: PlotId, + open_data: serde_json::Value, +} + +impl CommHandler for PlotComm { + fn open_metadata(&self) -> serde_json::Value { + self.open_data.clone() + } + + fn handle_msg(&mut self, msg: CommMsg, ctx: &CommHandlerContext) { + DEVICE_CONTEXT.with_borrow(|dc| { + handle_rpc_request(&ctx.outgoing_tx, PLOT_COMM_NAME, msg, |req| { + dc.handle_rpc(req, &self.id) + }); + }); + } + + fn handle_close(&mut self, _ctx: &CommHandlerContext) { + DEVICE_CONTEXT.with_borrow(|dc| { + dc.on_plot_closed(&self.id); + }); + } +} + // TODO: This macro needs to be updated every time we introduce support // for a new graphics device. Is there a better way? macro_rules! with_device { @@ -1047,15 +939,6 @@ impl From<&PlotId> for RObject { } } -/// Hook applied at idle time (`R_ProcessEvents()` time) to process any outstanding -/// RPC requests from Positron -/// -/// This is called a lot, so we don't trace log each entry -#[tracing::instrument(level = "trace", skip_all)] -pub(crate) fn on_process_idle_events() { - DEVICE_CONTEXT.with_borrow(|cell| cell.process_rpc_requests()); -} - /// Hook applied when an execute request starts /// /// Pushes the execution context (execution_id, code, code_location) to the graphics device diff --git a/crates/ark/src/shell.rs b/crates/ark/src/shell.rs index f55ff8430..d43145c51 100644 --- a/crates/ark/src/shell.rs +++ b/crates/ark/src/shell.rs @@ -49,6 +49,7 @@ use crate::data_explorer::r_data_explorer::DATA_EXPLORER_COMM_NAME; use crate::help::r_help::RHelp; use crate::help_proxy; use crate::plots::graphics_device::GraphicsDeviceNotification; +use crate::plots::graphics_device::PLOT_COMM_NAME; use crate::r_task; use crate::request::KernelRequest; use crate::request::RRequest; @@ -258,7 +259,7 @@ impl ShellHandler for Shell { msg: CommMsg, ) -> amalthea::Result { match comm_name { - DATA_EXPLORER_COMM_NAME | UI_COMM_NAME => { + DATA_EXPLORER_COMM_NAME | PLOT_COMM_NAME | UI_COMM_NAME => { self.dispatch_kernel_request(|done_tx| KernelRequest::CommMsg { comm_id: comm_id.to_string(), msg, @@ -276,7 +277,7 @@ impl ShellHandler for Shell { comm_name: &str, ) -> amalthea::Result { match comm_name { - DATA_EXPLORER_COMM_NAME | UI_COMM_NAME => { + DATA_EXPLORER_COMM_NAME | PLOT_COMM_NAME | UI_COMM_NAME => { self.dispatch_kernel_request(|done_tx| KernelRequest::CommClose { comm_id: comm_id.to_string(), done_tx, From a537177a05c040b70fae50593442d88eaf5ec3ea Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 10 Mar 2026 11:22:37 +0100 Subject: [PATCH 2/6] Fix `dev.hold()` issue introduced by pre-renderings --- crates/ark/src/plots/graphics_device.rs | 15 +++++- crates/ark/tests/plots.rs | 68 +++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/crates/ark/src/plots/graphics_device.rs b/crates/ark/src/plots/graphics_device.rs index 5e34c6cb2..541fe8a73 100644 --- a/crates/ark/src/plots/graphics_device.rs +++ b/crates/ark/src/plots/graphics_device.rs @@ -330,9 +330,15 @@ impl DeviceContext { #[tracing::instrument(level = "trace", skip_all, fields(level = %level))] fn hook_holdflush(&self, level: i32) { + let was_held = !self.should_render.get(); // Be extra safe and check `level <= 0` rather than just `level == 0` in case // our shadowed device returns a negative `level` self.should_render.replace(level <= 0); + + // Flush deferred changes on hold→release transition + if was_held && self.should_render.get() { + self.process_changes(); + } } #[tracing::instrument(level = "trace", skip_all, fields(mode = %mode))] @@ -569,11 +575,18 @@ impl DeviceContext { fn process_changes(&self) { let id = self.id(); - if !self.has_changes.replace(false) { + if !self.has_changes.get() { log::trace!("No changes to process for plot `id` {id}"); return; } + if !self.should_render.get() { + log::trace!("Deferring changes for plot `id` {id} (rendering held)"); + return; + } + + self.has_changes.replace(false); + log::trace!("Processing changes for plot `id` {id}"); // Record the changes so we can replay them when Positron asks us for them. diff --git a/crates/ark/tests/plots.rs b/crates/ark/tests/plots.rs index 9b2f204cb..89a79713b 100644 --- a/crates/ark/tests/plots.rs +++ b/crates/ark/tests/plots.rs @@ -583,3 +583,71 @@ fn test_plot_source_context_stacking() { file_a.uri_id, ); } + +/// Test that `dev.hold()` suppresses intermediate plot output. +/// +/// Without hold, each `plot()` call emits a separate `display_data`. +/// With hold active, intermediate plots are suppressed and only the +/// final state after `dev.flush()` is emitted. +#[test] +fn test_dev_hold_suppresses_intermediate_plots() { + let frontend = DummyArkFrontend::lock(); + + // Activate the graphics device + frontend.send_execute_request("plot(1:10)", ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_display_data(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // Hold, draw two intermediate plots, then flush. + // Only the final plot should produce output. + let code = r#" +invisible(dev.hold()) +plot(1:5) +plot(1:3) +invisible(dev.flush()) +"#; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_display_data(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); +} + +/// Test that `dev.hold()` persists across execute requests. +/// +/// A hold started in one request should suppress output until +/// `dev.flush()` is called in a subsequent request. +#[test] +fn test_dev_hold_across_execute_requests() { + let frontend = DummyArkFrontend::lock(); + + // Activate the graphics device + frontend.send_execute_request("plot(1:10)", ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_display_data(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // Hold and plot without flushing. No display_data should appear. + frontend.send_execute_request( + "invisible(dev.hold())\nplot(1:5)", + ExecuteRequestOptions::default(), + ); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // Flush in a separate request. The held plot should now appear. + frontend.send_execute_request("invisible(dev.flush())", ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_display_data(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); +} From e3623b67a9fb7b8a1619197db50a0a93c1d4a5a7 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 10 Mar 2026 12:08:15 +0100 Subject: [PATCH 3/6] Move device context to Console --- crates/ark/src/comm_handler.rs | 12 +- crates/ark/src/console.rs | 4 + crates/ark/src/console/console_repl.rs | 14 +- crates/ark/src/plots/graphics_device.rs | 246 +++++++++++------------- 4 files changed, 139 insertions(+), 137 deletions(-) diff --git a/crates/ark/src/comm_handler.rs b/crates/ark/src/comm_handler.rs index 5b7f1ccaa..564276df2 100644 --- a/crates/ark/src/comm_handler.rs +++ b/crates/ark/src/comm_handler.rs @@ -18,9 +18,12 @@ use serde::de::DeserializeOwned; use serde::Serialize; use stdext::result::ResultExt; +use crate::console::Console; + /// Context provided to `CommHandler` methods, giving access to the outgoing -/// channel and close-request mechanism. In the future, we'll provide access to -/// more of the Console state, such as the currently active environment. +/// channel, close-request mechanism, and the Console singleton (via +/// `console()`). Through the Console, handlers can reach runtime state such +/// as the graphics device context. #[derive(Debug)] pub struct CommHandlerContext { pub outgoing_tx: CommOutgoingTx, @@ -56,6 +59,11 @@ impl CommHandlerContext { }; self.outgoing_tx.send(CommMsg::Data(json)).log_err(); } + + /// Access the Console singleton (the R runtime). + pub(crate) fn console(&self) -> &Console { + Console::get() + } } /// Trait for comm handlers that run synchronously on the R thread. diff --git a/crates/ark/src/console.rs b/crates/ark/src/console.rs index 2b291ae4c..63ef1a19d 100644 --- a/crates/ark/src/console.rs +++ b/crates/ark/src/console.rs @@ -147,6 +147,7 @@ use crate::lsp::state_handlers::ConsoleInputs; use crate::modules; use crate::modules::ARK_ENVS; use crate::plots::graphics_device; +use crate::plots::graphics_device::DeviceContext; use crate::plots::graphics_device::GraphicsDeviceNotification; use crate::r_task; use crate::r_task::BoxFuture; @@ -334,4 +335,7 @@ pub(crate) struct Console { /// Comm handlers registered on the R thread (keyed by comm ID). comms: HashMap, + + /// Graphics device state (plot recording, rendering, comm management). + device_context: DeviceContext, } diff --git a/crates/ark/src/console/console_repl.rs b/crates/ark/src/console/console_repl.rs index 00d7d7a33..0eff110c7 100644 --- a/crates/ark/src/console/console_repl.rs +++ b/crates/ark/src/console/console_repl.rs @@ -481,13 +481,16 @@ impl Console { } }); - // Initialize the GD context on this thread. + // Perform R-side graphics device initialization (register as + // interactive, spawn notification listener). The `DeviceContext` + // itself is already created as part of `Console::new()`. + // // Note that we do it after init is complete to avoid deadlocking // integration tests by spawning an async task. The deadlock is caused // by https://github.com/posit-dev/ark/blob/bd827e735970ca17102aeddfbe2c3ccf26950a36/crates/ark/src/r_task.rs#L261. // We should be able to remove this escape hatch in `r_task()` by // instantiating an `Console` in unit tests as well. - graphics_device::init_graphics_device(console.iopub_tx().clone(), graphics_device_rx); + graphics_device::init_graphics_device(graphics_device_rx); // Now that R has started and libr and ark have fully initialized, run site and user // level R profiles, in that order @@ -585,6 +588,8 @@ impl Console { dap: Arc>, session_mode: SessionMode, ) -> Self { + let device_context = DeviceContext::new(iopub_tx.clone()); + Self { r_request_rx, comm_event_tx, @@ -630,6 +635,7 @@ impl Console { read_console_shutdown: Cell::new(false), debug_filter: ConsoleFilter::new(), comms: HashMap::new(), + device_context, } } @@ -681,6 +687,10 @@ impl Console { &self.comm_event_tx } + pub(crate) fn device_context(&self) -> &DeviceContext { + &self.device_context + } + /// Run a closure while capturing console output. /// Returns the closure's result paired with any captured output. pub(crate) fn with_capture(f: impl FnOnce() -> T) -> (T, String) { diff --git a/crates/ark/src/plots/graphics_device.rs b/crates/ark/src/plots/graphics_device.rs index 541fe8a73..5020db64c 100644 --- a/crates/ark/src/plots/graphics_device.rs +++ b/crates/ark/src/plots/graphics_device.rs @@ -63,21 +63,13 @@ pub(crate) enum GraphicsDeviceNotification { DidChangePlotRenderSettings(PlotRenderSettings), } -thread_local! { - // Safety: Set once by `Console` on initialization - static DEVICE_CONTEXT: RefCell = panic!("Must access `DEVICE_CONTEXT` from the R thread"); -} - pub const PLOT_COMM_NAME: &str = "positron.plot"; -// Expose thread initialization via function so we can keep the structs private. -// Must be called from the main R thread. +/// Perform R-side initialization of the graphics device. +/// Must be called from the main R thread after Console is initialized. pub(crate) fn init_graphics_device( - iopub_tx: Sender, graphics_device_rx: AsyncUnboundedReceiver, ) { - DEVICE_CONTEXT.set(DeviceContext::new(iopub_tx)); - // Declare our graphics device as interactive if let Err(err) = RFunction::from(".ps.graphics.register_as_interactive").call() { log::error!("Failed to register Ark graphics device as interactive: {err:?}"); @@ -98,12 +90,14 @@ async fn process_notifications( match notification { GraphicsDeviceNotification::DidChangePlotRenderSettings(plot_render_settings) => { - // Safety: Note that `DEVICE_CONTEXT` is accessed at - // interrupt time. Other methods in this file should be - // written in accordance and avoid causing R interrupt - // checks while they themselves access the device. - DEVICE_CONTEXT - .with_borrow(|ctx| ctx.prerender_settings.replace(plot_render_settings)); + // Safety: The device context is accessed at interrupt + // time. Other methods in this file should be written in + // accordance and avoid causing R interrupt checks while + // they themselves access the device. + Console::get() + .device_context() + .prerender_settings + .replace(plot_render_settings); }, } } @@ -132,7 +126,7 @@ struct ExecutionContext { code_location: Option, } -struct DeviceContext { +pub(crate) struct DeviceContext { /// Channel for sending [IOPubMessage::DisplayData] and /// [IOPubMessage::UpdateDisplayData] to Jupyter frontends when plot events occur iopub_tx: Sender, @@ -209,7 +203,7 @@ struct DeviceContext { } impl DeviceContext { - fn new(iopub_tx: Sender) -> Self { + pub(crate) fn new(iopub_tx: Sender) -> Self { Self { iopub_tx, has_changes: Cell::new(false), @@ -886,17 +880,14 @@ impl CommHandler for PlotComm { } fn handle_msg(&mut self, msg: CommMsg, ctx: &CommHandlerContext) { - DEVICE_CONTEXT.with_borrow(|dc| { - handle_rpc_request(&ctx.outgoing_tx, PLOT_COMM_NAME, msg, |req| { - dc.handle_rpc(req, &self.id) - }); + let dc = ctx.console().device_context(); + handle_rpc_request(&ctx.outgoing_tx, PLOT_COMM_NAME, msg, |req| { + dc.handle_rpc(req, &self.id) }); } - fn handle_close(&mut self, _ctx: &CommHandlerContext) { - DEVICE_CONTEXT.with_borrow(|dc| { - dc.on_plot_closed(&self.id); - }); + fn handle_close(&mut self, ctx: &CommHandlerContext) { + ctx.console().device_context().on_plot_closed(&self.id); } } @@ -966,8 +957,9 @@ pub(crate) fn on_execute_request( code_location: Option, ) { log::trace!("Entering on_execute_request"); - DEVICE_CONTEXT - .with_borrow(|cell| cell.set_execution_context(execution_id, code, code_location)); + Console::get() + .device_context() + .set_execution_context(execution_id, code, code_location); } /// Hook applied after a code chunk has finished executing @@ -990,11 +982,10 @@ pub(crate) fn on_execute_request( #[tracing::instrument(level = "trace", skip_all)] pub(crate) fn on_did_execute_request() { log::trace!("Entering on_did_execute_request"); - DEVICE_CONTEXT.with_borrow(|cell| { - cell.process_changes(); - cell.clear_execution_context(); - cell.clear_pending_origin(); - }); + let dc = Console::get().device_context(); + dc.process_changes(); + dc.clear_execution_context(); + dc.clear_pending_origin(); } /// Activation callback @@ -1007,11 +998,10 @@ pub(crate) fn on_did_execute_request() { unsafe extern "C-unwind" fn callback_activate(dev: pDevDesc) { log::trace!("Entering callback_activate"); - DEVICE_CONTEXT.with_borrow(|cell| { - if let Some(callback) = cell.wrapped_callbacks.activate.get() { - callback(dev); - } - }); + let dc = Console::get().device_context(); + if let Some(callback) = dc.wrapped_callbacks.activate.get() { + callback(dev); + } } /// Deactivation callback @@ -1022,42 +1012,40 @@ unsafe extern "C-unwind" fn callback_activate(dev: pDevDesc) { unsafe extern "C-unwind" fn callback_deactivate(dev: pDevDesc) { log::trace!("Entering callback_deactivate"); - DEVICE_CONTEXT.with_borrow(|cell| { - // We run our hook first to record before we deactivate the underlying device, - // in case device deactivation messes with the display list - cell.hook_deactivate(); - if let Some(callback) = cell.wrapped_callbacks.deactivate.get() { - callback(dev); - } - }); + let dc = Console::get().device_context(); + // We run our hook first to record before we deactivate the underlying device, + // in case device deactivation messes with the display list + dc.hook_deactivate(); + if let Some(callback) = dc.wrapped_callbacks.deactivate.get() { + callback(dev); + } } #[tracing::instrument(level = "trace", skip_all, fields(level_delta = %level_delta))] unsafe extern "C-unwind" fn callback_holdflush(dev: pDevDesc, level_delta: i32) -> i32 { log::trace!("Entering callback_holdflush"); - DEVICE_CONTEXT.with_borrow(|cell| { - // If our wrapped device has a `holdflush()` method, we rely on it to apply - // the `level_delta` (typically `+1` or `-1`) and return the new level. Otherwise - // we follow the lead of `devholdflush()` in R and use a resolved `level` of `0`. - // Notably, `grDevices::png()` with a Cairo backend does not have a holdflush - // hook. - // https://github.com/wch/r-source/blob/8cebcc0a5d99890839e5171f398da643d858dcca/src/library/grDevices/src/devices.c#L129-L138 - let level = match cell.wrapped_callbacks.holdflush.get() { - Some(callback) => { - let level = callback(dev, level_delta); - log::trace!("Using resolved holdflush level from wrapped callback: {level}"); - level - }, - None => { - let level = 0; - log::trace!("Using default holdflush level: {level}"); - level - }, - }; - cell.hook_holdflush(level); - level - }) + let dc = Console::get().device_context(); + // If our wrapped device has a `holdflush()` method, we rely on it to apply + // the `level_delta` (typically `+1` or `-1`) and return the new level. Otherwise + // we follow the lead of `devholdflush()` in R and use a resolved `level` of `0`. + // Notably, `grDevices::png()` with a Cairo backend does not have a holdflush + // hook. + // https://github.com/wch/r-source/blob/8cebcc0a5d99890839e5171f398da643d858dcca/src/library/grDevices/src/devices.c#L129-L138 + let level = match dc.wrapped_callbacks.holdflush.get() { + Some(callback) => { + let level = callback(dev, level_delta); + log::trace!("Using resolved holdflush level from wrapped callback: {level}"); + level + }, + None => { + let level = 0; + log::trace!("Using default holdflush level: {level}"); + level + }, + }; + dc.hook_holdflush(level); + level } // mode = 0, graphics off @@ -1067,24 +1055,22 @@ unsafe extern "C-unwind" fn callback_holdflush(dev: pDevDesc, level_delta: i32) unsafe extern "C-unwind" fn callback_mode(mode: i32, dev: pDevDesc) { log::trace!("Entering callback_mode"); - DEVICE_CONTEXT.with_borrow(|cell| { - if let Some(callback) = cell.wrapped_callbacks.mode.get() { - callback(mode, dev); - } - cell.hook_mode(mode); - }); + let dc = Console::get().device_context(); + if let Some(callback) = dc.wrapped_callbacks.mode.get() { + callback(mode, dev); + } + dc.hook_mode(mode); } #[tracing::instrument(level = "trace", skip_all)] unsafe extern "C-unwind" fn callback_new_page(dd: pGEcontext, dev: pDevDesc) { log::trace!("Entering callback_new_page"); - DEVICE_CONTEXT.with_borrow(|cell| { - if let Some(callback) = cell.wrapped_callbacks.newPage.get() { - callback(dd, dev); - } - cell.hook_new_page(); - }); + let dc = Console::get().device_context(); + if let Some(callback) = dc.wrapped_callbacks.newPage.get() { + callback(dd, dev); + } + dc.hook_new_page(); } unsafe fn ps_graphics_device_impl() -> anyhow::Result { @@ -1107,26 +1093,24 @@ unsafe fn ps_graphics_device_impl() -> anyhow::Result { with_device!(ge_device, |ge_device, device| { (*ge_device).displayListOn = 1; - DEVICE_CONTEXT.with_borrow(|cell| { - let wrapped_callbacks = &cell.wrapped_callbacks; + let wrapped_callbacks = &Console::get().device_context().wrapped_callbacks; - // Safety: The callbacks are stored in simple cells. + // Safety: The callbacks are stored in simple cells. - wrapped_callbacks.activate.replace((*device).activate); - (*device).activate = Some(callback_activate); + wrapped_callbacks.activate.replace((*device).activate); + (*device).activate = Some(callback_activate); - wrapped_callbacks.deactivate.replace((*device).deactivate); - (*device).deactivate = Some(callback_deactivate); + wrapped_callbacks.deactivate.replace((*device).deactivate); + (*device).deactivate = Some(callback_deactivate); - wrapped_callbacks.holdflush.replace((*device).holdflush); - (*device).holdflush = Some(callback_holdflush); + wrapped_callbacks.holdflush.replace((*device).holdflush); + (*device).holdflush = Some(callback_holdflush); - wrapped_callbacks.mode.replace((*device).mode); - (*device).mode = Some(callback_mode); + wrapped_callbacks.mode.replace((*device).mode); + (*device).mode = Some(callback_mode); - wrapped_callbacks.newPage.replace((*device).newPage); - (*device).newPage = Some(callback_new_page); - }); + wrapped_callbacks.newPage.replace((*device).newPage); + (*device).newPage = Some(callback_new_page); }); Ok(R_NilValue) @@ -1169,11 +1153,9 @@ unsafe extern "C-unwind" fn ps_graphics_device() -> anyhow::Result { unsafe extern "C-unwind" fn ps_graphics_before_plot_new(_name: SEXP) -> anyhow::Result { log::trace!("Entering ps_graphics_before_plot_new"); - DEVICE_CONTEXT.with_borrow(|cell| { - // Process changes related to the last plot before opening a new page. - // Particularly important if we make multiple plots in a single chunk. - cell.process_changes(); - }); + // Process changes related to the last plot before opening a new page. + // Particularly important if we make multiple plots in a single chunk. + Console::get().device_context().process_changes(); Ok(harp::r_null()) } @@ -1188,38 +1170,36 @@ unsafe extern "C-unwind" fn ps_graphics_get_metadata(id: SEXP) -> anyhow::Result let id_str: String = RObject::view(id).try_into()?; let plot_id = PlotId(id_str); - DEVICE_CONTEXT.with_borrow(|cell| { - let metadata = cell.metadata.borrow(); - match metadata.get(&plot_id) { - Some(info) => { - let origin_uri = info.origin.as_ref().map(|o| o.uri.as_str()).unwrap_or(""); - - // Create a list with the metadata values - let values: Vec = vec![ - RObject::from(info.name.as_str()), - RObject::from(info.kind.as_str()), - RObject::from(info.execution_id.as_str()), - RObject::from(info.code.as_str()), - RObject::from(origin_uri), - ]; - let list = RObject::try_from(values)?; - - // Set the names attribute - let names: Vec = vec![ - "name".to_string(), - "kind".to_string(), - "execution_id".to_string(), - "code".to_string(), - "origin_uri".to_string(), - ]; - let names = RObject::from(names); - libr::Rf_setAttrib(list.sexp, libr::R_NamesSymbol, names.sexp); - - Ok(list.sexp) - }, - None => Ok(harp::r_null()), - } - }) + let metadata = Console::get().device_context().metadata.borrow(); + match metadata.get(&plot_id) { + Some(info) => { + let origin_uri = info.origin.as_ref().map(|o| o.uri.as_str()).unwrap_or(""); + + // Create a list with the metadata values + let values: Vec = vec![ + RObject::from(info.name.as_str()), + RObject::from(info.kind.as_str()), + RObject::from(info.execution_id.as_str()), + RObject::from(info.code.as_str()), + RObject::from(origin_uri), + ]; + let list = RObject::try_from(values)?; + + // Set the names attribute + let names: Vec = vec![ + "name".to_string(), + "kind".to_string(), + "execution_id".to_string(), + "code".to_string(), + "origin_uri".to_string(), + ]; + let names = RObject::from(names); + libr::Rf_setAttrib(list.sexp, libr::R_NamesSymbol, names.sexp); + + Ok(list.sexp) + }, + None => Ok(harp::r_null()), + } } /// Push a source file URI onto the source context stack. @@ -1227,7 +1207,7 @@ unsafe extern "C-unwind" fn ps_graphics_get_metadata(id: SEXP) -> anyhow::Result #[harp::register] unsafe extern "C-unwind" fn ps_graphics_push_source_context(uri: SEXP) -> anyhow::Result { let uri_str: String = RObject::view(uri).try_into()?; - DEVICE_CONTEXT.with_borrow(|cell| cell.push_source_context(uri_str)); + Console::get().device_context().push_source_context(uri_str); Ok(harp::r_null()) } @@ -1235,6 +1215,6 @@ unsafe extern "C-unwind" fn ps_graphics_push_source_context(uri: SEXP) -> anyhow /// Called from the `source()` hook when leaving a sourced file. #[harp::register] unsafe extern "C-unwind" fn ps_graphics_pop_source_context() -> anyhow::Result { - DEVICE_CONTEXT.with_borrow(|cell| cell.pop_source_context()); + Console::get().device_context().pop_source_context(); Ok(harp::r_null()) } From 93d3afacaaebf7826cb6bdf3aeab947221fe4456 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 10 Mar 2026 18:11:01 +0100 Subject: [PATCH 4/6] Set Prerender settings synchronously from the UI handler --- crates/ark/src/console.rs | 1 - crates/ark/src/console/console_repl.rs | 3 +- crates/ark/src/plots/graphics_device.rs | 47 ++++--------------------- crates/ark/src/shell.rs | 14 ++------ crates/ark/src/start.rs | 8 ----- crates/ark/src/ui/ui_comm.rs | 20 +++-------- 6 files changed, 15 insertions(+), 78 deletions(-) diff --git a/crates/ark/src/console.rs b/crates/ark/src/console.rs index 63ef1a19d..a92ff9215 100644 --- a/crates/ark/src/console.rs +++ b/crates/ark/src/console.rs @@ -148,7 +148,6 @@ use crate::modules; use crate::modules::ARK_ENVS; use crate::plots::graphics_device; use crate::plots::graphics_device::DeviceContext; -use crate::plots::graphics_device::GraphicsDeviceNotification; use crate::r_task; use crate::r_task::BoxFuture; use crate::r_task::RTask; diff --git a/crates/ark/src/console/console_repl.rs b/crates/ark/src/console/console_repl.rs index 0eff110c7..f500c7d7f 100644 --- a/crates/ark/src/console/console_repl.rs +++ b/crates/ark/src/console/console_repl.rs @@ -326,7 +326,6 @@ impl Console { dap: Arc>, session_mode: SessionMode, default_repos: DefaultRepos, - graphics_device_rx: AsyncUnboundedReceiver, console_notification_rx: AsyncUnboundedReceiver, ) { // Set the main thread ID. @@ -490,7 +489,7 @@ impl Console { // by https://github.com/posit-dev/ark/blob/bd827e735970ca17102aeddfbe2c3ccf26950a36/crates/ark/src/r_task.rs#L261. // We should be able to remove this escape hatch in `r_task()` by // instantiating an `Console` in unit tests as well. - graphics_device::init_graphics_device(graphics_device_rx); + graphics_device::init_graphics_device(); // Now that R has started and libr and ark have fully initialized, run site and user // level R profiles, in that order diff --git a/crates/ark/src/plots/graphics_device.rs b/crates/ark/src/plots/graphics_device.rs index 5020db64c..1d9bdade3 100644 --- a/crates/ark/src/plots/graphics_device.rs +++ b/crates/ark/src/plots/graphics_device.rs @@ -47,7 +47,6 @@ use libr::SEXP; use serde_json::json; use stdext::result::ResultExt; use stdext::unwrap; -use tokio::sync::mpsc::UnboundedReceiver as AsyncUnboundedReceiver; use uuid::Uuid; use crate::comm_handler::handle_rpc_request; @@ -56,52 +55,16 @@ use crate::comm_handler::CommHandlerContext; use crate::console::Console; use crate::console::SessionMode; use crate::modules::ARK_ENVS; -use crate::r_task; - -#[derive(Debug)] -pub(crate) enum GraphicsDeviceNotification { - DidChangePlotRenderSettings(PlotRenderSettings), -} pub const PLOT_COMM_NAME: &str = "positron.plot"; /// Perform R-side initialization of the graphics device. /// Must be called from the main R thread after Console is initialized. -pub(crate) fn init_graphics_device( - graphics_device_rx: AsyncUnboundedReceiver, -) { +pub(crate) fn init_graphics_device() { // Declare our graphics device as interactive if let Err(err) = RFunction::from(".ps.graphics.register_as_interactive").call() { log::error!("Failed to register Ark graphics device as interactive: {err:?}"); }; - - // Launch an R thread task to process messages from the frontend - r_task::spawn_interrupt(async move || process_notifications(graphics_device_rx).await); -} - -async fn process_notifications( - mut graphics_device_rx: AsyncUnboundedReceiver, -) { - log::trace!("Now listening for graphics device notifications"); - - loop { - while let Some(notification) = graphics_device_rx.recv().await { - log::trace!("Got graphics device notification: {notification:#?}"); - - match notification { - GraphicsDeviceNotification::DidChangePlotRenderSettings(plot_render_settings) => { - // Safety: The device context is accessed at interrupt - // time. Other methods in this file should be written in - // accordance and avoid causing R interrupt checks while - // they themselves access the device. - Console::get() - .device_context() - .prerender_settings - .replace(plot_render_settings); - }, - } - } - } } /// Wrapped callbacks of the original graphics device we shadow @@ -203,7 +166,7 @@ pub(crate) struct DeviceContext { } impl DeviceContext { - pub(crate) fn new(iopub_tx: Sender) -> Self { + pub fn new(iopub_tx: Sender) -> Self { Self { iopub_tx, has_changes: Cell::new(false), @@ -229,6 +192,10 @@ impl DeviceContext { } } + pub fn set_prerender_settings(&self, settings: PlotRenderSettings) { + self.prerender_settings.replace(settings); + } + /// Set the current execution context (called when an execute request starts) fn set_execution_context( &self, @@ -665,7 +632,7 @@ impl DeviceContext { open_data, }; - match Console::get_mut().comm_register(PLOT_COMM_NAME, Box::new(plot_comm)) { + match Console::get_mut().comm_open_backend(PLOT_COMM_NAME, Box::new(plot_comm)) { Ok(comm_id) => { self.comm_ids.borrow_mut().insert(id.clone(), comm_id); }, diff --git a/crates/ark/src/shell.rs b/crates/ark/src/shell.rs index d43145c51..24ad342ef 100644 --- a/crates/ark/src/shell.rs +++ b/crates/ark/src/shell.rs @@ -40,7 +40,6 @@ use harp::ParseResult; use log::*; use serde_json::json; use stdext::unwrap; -use tokio::sync::mpsc::UnboundedSender as AsyncUnboundedSender; use crate::ark_comm::ArkComm; use crate::console::Console; @@ -48,7 +47,6 @@ use crate::console::KernelInfo; use crate::data_explorer::r_data_explorer::DATA_EXPLORER_COMM_NAME; use crate::help::r_help::RHelp; use crate::help_proxy; -use crate::plots::graphics_device::GraphicsDeviceNotification; use crate::plots::graphics_device::PLOT_COMM_NAME; use crate::r_task; use crate::request::KernelRequest; @@ -62,7 +60,6 @@ pub struct Shell { kernel_request_tx: Sender, kernel_init_rx: BusReader, kernel_info: Option, - graphics_device_tx: AsyncUnboundedSender, } #[derive(Debug)] @@ -76,14 +73,12 @@ impl Shell { r_request_tx: Sender, kernel_init_rx: BusReader, kernel_request_tx: Sender, - graphics_device_tx: AsyncUnboundedSender, ) -> Self { Self { r_request_tx, kernel_request_tx, kernel_init_rx, kernel_info: None, - graphics_device_tx, } } @@ -241,11 +236,7 @@ impl ShellHandler for Shell { async fn handle_comm_open(&self, target: Comm, comm: CommSocket) -> amalthea::Result { match target { Comm::Variables => handle_comm_open_variables(comm), - Comm::Ui => handle_comm_open_ui( - comm, - self.kernel_request_tx.clone(), - self.graphics_device_tx.clone(), - ), + Comm::Ui => handle_comm_open_ui(comm, self.kernel_request_tx.clone()), Comm::Help => handle_comm_open_help(comm), Comm::Other(target_name) if target_name == "ark" => ArkComm::handle_comm_open(comm), _ => Ok(false), @@ -317,9 +308,8 @@ fn handle_comm_open_variables(comm: CommSocket) -> amalthea::Result { fn handle_comm_open_ui( comm: CommSocket, kernel_request_tx: Sender, - graphics_device_tx: AsyncUnboundedSender, ) -> amalthea::Result { - let handler = UiComm::new(graphics_device_tx); + let handler = UiComm::new(); let (done_tx, done_rx) = bounded(0); kernel_request_tx diff --git a/crates/ark/src/start.rs b/crates/ark/src/start.rs index 8bce3a448..d900f8855 100644 --- a/crates/ark/src/start.rs +++ b/crates/ark/src/start.rs @@ -25,7 +25,6 @@ use crate::console::SessionMode; use crate::control::Control; use crate::dap; use crate::lsp; -use crate::plots::graphics_device::GraphicsDeviceNotification; use crate::repos::DefaultRepos; use crate::request::KernelRequest; use crate::request::RRequest; @@ -79,18 +78,12 @@ pub fn start_kernel( // StdIn socket thread let (stdin_request_tx, stdin_request_rx) = bounded::(1); - // Communication channel between the graphics device (running on the R - // thread) and the shell thread - let (graphics_device_tx, graphics_device_rx) = - tokio::sync::mpsc::unbounded_channel::(); - // Create the shell. let kernel_init_rx = kernel_init_tx.add_rx(); let shell = Box::new(Shell::new( r_request_tx.clone(), kernel_init_rx, kernel_request_tx, - graphics_device_tx, )); // Create the control handler; this is used to handle shutdown/interrupt and @@ -150,7 +143,6 @@ pub fn start_kernel( dap, session_mode, default_repos, - graphics_device_rx, console_notification_rx, ) } diff --git a/crates/ark/src/ui/ui_comm.rs b/crates/ark/src/ui/ui_comm.rs index 78689d200..cfcd9f02d 100644 --- a/crates/ark/src/ui/ui_comm.rs +++ b/crates/ark/src/ui/ui_comm.rs @@ -24,7 +24,6 @@ use harp::exec::RFunctionExt; use harp::object::RObject; use serde_json::Value; use stdext::result::ResultExt; -use tokio::sync::mpsc::UnboundedSender as AsyncUnboundedSender; use crate::comm_handler::handle_rpc_request; use crate::comm_handler::CommHandler; @@ -32,14 +31,12 @@ use crate::comm_handler::CommHandlerContext; use crate::comm_handler::EnvironmentChanged; use crate::console::Console; use crate::console::ConsoleOutputCapture; -use crate::plots::graphics_device::GraphicsDeviceNotification; pub const UI_COMM_NAME: &str = "positron.ui"; /// Comm handler for the Positron UI comm. #[derive(Debug)] pub struct UiComm { - graphics_device_tx: AsyncUnboundedSender, working_directory: PathBuf, } @@ -63,11 +60,8 @@ impl CommHandler for UiComm { } impl UiComm { - pub(crate) fn new( - graphics_device_tx: AsyncUnboundedSender, - ) -> Self { + pub(crate) fn new() -> Self { Self { - graphics_device_tx, working_directory: PathBuf::new(), } } @@ -131,11 +125,9 @@ impl UiComm { )); } - self.graphics_device_tx - .send(GraphicsDeviceNotification::DidChangePlotRenderSettings( - params.settings, - )) - .map_err(|err| anyhow::anyhow!("Failed to send plot render settings: {err}"))?; + Console::get() + .device_context() + .set_prerender_settings(params.settings); Ok(UiBackendReply::DidChangePlotsRenderSettingsReply()) } @@ -261,9 +253,7 @@ mod tests { let (comm_event_tx, _) = bounded::(10); let ctx = CommHandlerContext::new(outgoing_tx, comm_event_tx); - let (graphics_device_tx, _) = tokio::sync::mpsc::unbounded_channel(); - let handler = UiComm::new(graphics_device_tx); - + let handler = UiComm::new(); (handler, ctx) } From 307076338185355c6443515dd9bb919a1c88747b Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 10 Mar 2026 18:45:07 +0100 Subject: [PATCH 5/6] Fix message interpolation --- crates/ark/src/plots/graphics_device.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ark/src/plots/graphics_device.rs b/crates/ark/src/plots/graphics_device.rs index 1d9bdade3..ec952fa2a 100644 --- a/crates/ark/src/plots/graphics_device.rs +++ b/crates/ark/src/plots/graphics_device.rs @@ -733,7 +733,7 @@ impl DeviceContext { let outgoing_tx = CommOutgoingTx::new(comm_id, self.iopub_tx.clone()); outgoing_tx .send(CommMsg::Data(value)) - .context("Failed to send update message for id {id}.") + .context(format!("Failed to send update message for id {id}.")) .log_err(); } From 845b4a74636acbc23c5ef2afcfc589a253f0e456 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 11 Mar 2026 08:06:37 +0100 Subject: [PATCH 6/6] Fix clippy issue --- crates/ark/src/plots/graphics_device.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ark/src/plots/graphics_device.rs b/crates/ark/src/plots/graphics_device.rs index ec952fa2a..85fc1bdd9 100644 --- a/crates/ark/src/plots/graphics_device.rs +++ b/crates/ark/src/plots/graphics_device.rs @@ -487,7 +487,7 @@ impl DeviceContext { format: plot_meta.format, }; - let data = self.render_plot(&id, &settings)?; + let data = self.render_plot(id, &settings)?; let mime_type = Self::get_mime_type(&plot_meta.format); Ok(PlotBackendReply::RenderReply(PlotResult {