Move UI Comm sender ownership to `RMain` by DavisVaughan · Pull Request #583 · posit-dev/ark

DavisVaughan · 2024-10-12T12:27:50Z

No description provided.

DavisVaughan · 2024-10-13T18:07:30Z

crates/ark/src/interface.rs

-            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,
+                )
+            };


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

DavisVaughan · 2024-10-13T18:11:20Z

crates/ark/src/interface.rs

+    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.");
+            },
+        }
+    }


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

DavisVaughan · 2024-10-13T18:12:42Z

crates/ark/src/interface.rs

    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:?}")
+        })?;


Should only be called when we are in Positron mode, so this feels like an error

DavisVaughan · 2024-10-13T18:13:05Z

crates/ark/src/interface.rs

-                        anyhow::bail!("Can't deserialize RPC response for {request:?}:\n{err:?}");
+                        return Err(anyhow::anyhow!(
+                            "Can't deserialize RPC response for {request:?}:\n{err:?}"
+                        ));


Fixing up a few bail!s

DavisVaughan · 2024-10-13T18:14:08Z

crates/ark/src/shell.rs

            ExecuteResponse::ReplyException(err) => Err(err),
        };

-        let mut kernel = self.kernel.lock().unwrap();


I really really like that we now just:

send request

get response

return response

With no extra funny business

DavisVaughan · 2024-10-13T18:16:17Z

crates/ark/src/ui/events.rs

-    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);


Previously would eventually just do

log::info!("Discarding message {msg:?}; no frontend UI comm connected");

but i think we should actually error here

DavisVaughan · 2024-10-13T18:18:35Z

crates/ark/src/ui/sender.rs

+    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:?}");
+        }
+    }


All UI related refreshes should be here now - like an LSP refresh

lionel-

Nice progress! With a bit more effort I think we could get in an even better place with all UI-related stuff living in the UI comm module.

lionel- · 2024-10-15T07:27:07Z

crates/ark/src/browser.rs

    }

+    // TODO: What is the right thing to do outside of Positron when
+    // `options(browser =)` is called? Right now we error.


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.

crates/ark/src/interface.rs

lionel- · 2024-10-15T07:55:58Z

crates/ark/src/interface.rs

    pub fn call_frontend_method(&self, request: UiFrontendRequest) -> anyhow::Result<RObject> {
-        log::trace!("Calling frontend method '{request:?}'");
+        log::trace!("Calling frontend method {request:?}");
+


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.

crates/ark/src/ui/events.rs

crates/ark/src/interface.rs

crates/ark/src/shell.rs

DavisVaughan commented Oct 13, 2024

View reviewed changes

DavisVaughan changed the title ~~Move UI Comm ownership to RMain~~ Move UI Comm sender ownership to RMain Oct 13, 2024

DavisVaughan marked this pull request as ready for review October 13, 2024 18:19

DavisVaughan requested a review from lionel- October 13, 2024 18:19

DavisVaughan added 2 commits October 13, 2024 15:31

Move UI Comm ownership to RMain

b04d8d8

Lots of reworking

4f0fc93

DavisVaughan force-pushed the feature/rmain-owns-kernel branch from 3c3b08c to 4f0fc93 Compare October 13, 2024 19:34

lionel- reviewed Oct 15, 2024

View reviewed changes

lionel- approved these changes Oct 15, 2024

View reviewed changes

DavisVaughan added 3 commits October 15, 2024 11:28

Tweak message a little

e55eff6

Add name to error message

cdec890

Use consistent naming everywhere

c1119cb

DavisVaughan merged commit 67ff702 into main Oct 15, 2024

DavisVaughan deleted the feature/rmain-owns-kernel branch October 15, 2024 17:35

github-actions bot locked and limited conversation to collaborators Oct 15, 2024

Conversation

DavisVaughan commented Oct 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DavisVaughan commented Oct 12, 2024 •

edited

Loading