Move UI Comm sender ownership to RMain
#583
Conversation
| let event = UiFrontendEvent::PromptState(PromptStateParams { | ||
| input_prompt: info.input_prompt.clone(), | ||
| continuation_prompt: info.continuation_prompt.clone(), | ||
| // Perform a refresh of the frontend state | ||
| // (Prompts, working directory, etc) | ||
| self.with_mut_ui_comm_tx(|ui_comm_tx| { | ||
| let input_prompt = info.input_prompt.clone(); | ||
| let continuation_prompt = info.continuation_prompt.clone(); | ||
|
|
||
| ui_comm_tx.send_refresh(input_prompt, continuation_prompt); | ||
| }); | ||
| { | ||
| let kernel = self.kernel.lock().unwrap(); | ||
| kernel.send_ui_event(event); | ||
| } | ||
|
|
||
| // Check for pending graphics updates | ||
| // (Important that this occurs while in the "busy" state of this ExecuteRequest | ||
| // so that the `parent` message is set correctly in any Jupyter messages) | ||
| unsafe { | ||
| graphics_device::on_did_execute_request( | ||
| self.comm_manager_tx.clone(), | ||
| self.iopub_tx.clone(), | ||
| self.is_ui_comm_connected() && self.session_mode == SessionMode::Console, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
Previously we did a "prompt" refresh here right before unblocking shell, but over on shell when we got unblocked we then did a working directory refresh and a graphics device refresh.
We now do all 3 of those here, which seems way more appropriate
crates/ark/src/interface.rs
Outdated
| pub fn get_ui_comm_tx(&self) -> Option<&UiCommSender> { | ||
| self.ui_comm_tx.as_ref() | ||
| } | ||
|
|
||
| fn get_mut_ui_comm_tx(&mut self) -> Option<&mut UiCommSender> { | ||
| self.ui_comm_tx.as_mut() | ||
| } | ||
|
|
||
| fn with_ui_comm_tx<F>(&self, f: F) | ||
| where | ||
| F: FnOnce(&UiCommSender), | ||
| { | ||
| match self.get_ui_comm_tx() { | ||
| Some(ui_comm_tx) => f(ui_comm_tx), | ||
| None => { | ||
| // Trace level logging, its typically not a bug if the frontend | ||
| // isn't connected. Happens in all Jupyter use cases. | ||
| log::trace!("UI comm isn't connected."); | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
The idea is that in interface.rs we use with_ui_comm_tx() quite a bit because it doesn't log an error if the UI comm isn't connected - which is correct, because this code runs when in Jupyter mode too.
But in specific UI methods in other files (which only run in Positron mode), we use get_ui_comm_tx() which allows us to promote a None sender into an error, because that would be very unexpected
| pub fn call_frontend_method(&self, request: UiFrontendRequest) -> anyhow::Result<RObject> { | ||
| log::trace!("Calling frontend method '{request:?}'"); | ||
| log::trace!("Calling frontend method {request:?}"); | ||
|
|
||
| let ui_comm_tx = self.get_ui_comm_tx().ok_or_else(|| { | ||
| anyhow::anyhow!("UI comm is not connected. Can't execute request {request:?}") | ||
| })?; |
There was a problem hiding this comment.
Should only be called when we are in Positron mode, so this feels like an error
crates/ark/src/interface.rs
Outdated
| anyhow::bail!("Can't deserialize RPC response for {request:?}:\n{err:?}"); | ||
| return Err(anyhow::anyhow!( | ||
| "Can't deserialize RPC response for {request:?}:\n{err:?}" | ||
| )); |
There was a problem hiding this comment.
Fixing up a few bail!s
crates/ark/src/shell.rs
Outdated
| ExecuteResponse::ReplyException(err) => Err(err), | ||
| }; | ||
|
|
||
| let mut kernel = self.kernel.lock().unwrap(); |
There was a problem hiding this comment.
I really really like that we now just:
- send request
- get response
- return response
With no extra funny business
crates/ark/src/ui/events.rs
Outdated
| let main = RMain::get(); | ||
| let event = UiFrontendEvent::ShowMessage(params); | ||
| main.send_frontend_event(event); | ||
|
|
||
| let main = RMain::get(); | ||
| let ui_comm_tx = main.get_ui_comm_tx().ok_or_else(ui_comm_not_connected)?; | ||
| ui_comm_tx.send_event(event); |
There was a problem hiding this comment.
Previously would eventually just do
log::info!("Discarding message {msg:?}; no frontend UI comm connected");
but i think we should actually error here
| pub fn send_refresh(&mut self, input_prompt: String, continuation_prompt: String) { | ||
| self.refresh_prompt_info(input_prompt, continuation_prompt); | ||
|
|
||
| if let Err(err) = self.refresh_working_directory() { | ||
| log::error!("Can't refresh working directory: {err:?}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
All UI related refreshes should be here now - like an LSP refresh
DavisVaughan
changed the title
RMainRMain
DavisVaughan
force-pushed
the
feature/rmain-owns-kernel
branch
from
3c3b08c to
4f0fc93
Compare
| @@ -48,12 +48,20 @@ unsafe fn ps_browse_url_impl(url: SEXP) -> anyhow::Result<SEXP> { | |||
| log::trace!("Help is not handling URL"); | |||
| } | |||
|
|
|||
| // TODO: What is the right thing to do outside of Positron when | |||
| // `options(browser =)` is called? Right now we error. | |||
There was a problem hiding this comment.
The right thing to do outside Positron is to use the default action, i.e. open in a web browser. So we should not set up that option unless Positron is connected.
Ideally Positron would inject the option via its init file. Or the UI comm could launch an idle task to do it, but there would a short time where the option is not set with this approach.
| @@ -1683,26 +1756,26 @@ impl RMain { | |||
| } | |||
|
|
|||
| pub fn call_frontend_method(&self, request: UiFrontendRequest) -> anyhow::Result<RObject> { | |||
| log::trace!("Calling frontend method '{request:?}'"); | |||
| log::trace!("Calling frontend method {request:?}"); | |||
|
|
|||
There was a problem hiding this comment.
I guess we'd move that method to another global singleton created by the UI comm.
Which would be nice because it's a bit untidy for the R-level methods to have full access to RMain.
No description provided.