Deliver working directory changes to Positron #133
Conversation
crates/ark/src/kernel.rs
Outdated
| // Get the current busy status | ||
| let busy = r_task(|| { | ||
| let main = RMain::get(); | ||
| main.is_busy | ||
| }); | ||
| self.send_event(PositronEvent::Busy(BusyEvent { busy })); | ||
| } |
There was a problem hiding this comment.
Can you help me understand why you need to send this here? When the busy status changed in fn busy(), this positron event was fired immediately.
There was a problem hiding this comment.
I tried removing this and everything still seems to work fine
There was a problem hiding this comment.
Oh, I think I see.
This is establishing an initial link to the frontend, which might also happen on reconnect right? Like CMD+R where the frontend is refreshed but the Kernel is persistent and gets reconnected?
So we have to forward on the initial working directory and initial busy state of the reconnected Kernel.
There was a problem hiding this comment.
Yeah, that's it exactly. There is no way for the frontend to query for this state so we just send it on over as soon as the frontend connects, then deliver updates whenever it changes.
I just added the busy changes since I was in the neighborhood; I think we'll need them to fix posit-dev/positron#1243 (though this PR does not entirely fix that issue).
Zooming out a little more I think we will need to have some idea of a session state that encompasses all of the stuff the client needs to know, in one bundle. But one step at a time. :-)
| /// front end if the working directory has changed. | ||
| pub fn poll_working_directory(&mut self) -> Result<()> { | ||
| // Get the current working directory | ||
| let mut current_dir = std::env::current_dir()?; |
There was a problem hiding this comment.
I was curious about current_dir() so I looked at the docs, which say:
This function currently corresponds to the getcwd function on Unix and the GetCurrentDirectoryW function on Windows.
And that corresponds pretty nicely to what R's getwd() does!
The only thing I note is that in R it looks like \\ is turned into / on Windows by this helper
https://github.com/wch/r-source/blob/458cfb5da5609ac46ee7d7f38041c6576755d482/src/main/util.c#L1726-L1733
I figure we can deal with that if it gets confusing once I get a Windows computer?
There was a problem hiding this comment.
Yes, I intentionally avoided any explicit mention of path separators here and I'm hopeful that'll get us where we need to be on Windows!
crates/ark/src/kernel.rs
Outdated
|
|
||
| // Get the current busy status | ||
| let busy = r_task(|| { | ||
| let main = RMain::get(); |
There was a problem hiding this comment.
I am mildly worried about the fact that RMain might not be available at this point in time. Technically kernel.connect() (which sets up the shell thread to listen to comm open messages that might call this) is called before start_r() (which sets up RMain). It's like we need RMain::initialized() which could return false if R_MAIN is None, in which case you could return false for the Busy state here?
There was a problem hiding this comment.
That's a good idea; implemented in 8c1baab.
Co-authored-by: Davis Vaughan <davis@rstudio.com>
Co-authored-by: Davis Vaughan <davis@rstudio.com>
This change is a companion to posit-dev/positron#1740; it causes ark to emit events to Positron when the working directory changes. The events are emitted over the frontend comm, and are always fed to the comm when it is first created so that Positron has a view of the initial working directory.
This change doesn't depend on posit-dev/positron#1740, nor does posit-dev/positron#1740 depend on it; they can be merged independently.