Skip to content

Use new UI event to send a putative path to the system for opening #877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+60 −28
Open

Use new UI event to send a putative path to the system for opening #877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fde9cb8
WIP poking around
jennybc Jul 16, 2025
79f2a25
Rough in over here in ark
jennybc Jul 18, 2025
1041ba0
Regenerate UI comm; actually implement the event
jennybc Jul 19, 2025
6747682
Normalize on this side
jennybc Jul 19, 2025
d9ec603
Align function name with event name
jennybc Jul 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions crates/amalthea/src/comm/ui_comm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,13 @@ pub struct ShowHtmlFileParams {
pub height: i64,
}

/// Parameters for the OpenWithSystem method.
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
pub struct OpenWithSystemParams {
/// The file path to open with the system default application
pub path: String,
}

/**
* Backend RPC request types for the ui comm
*/
Expand Down Expand Up @@ -502,6 +509,10 @@ pub enum UiFrontendEvent {
#[serde(rename = "show_html_file")]
ShowHtmlFile(ShowHtmlFileParams),

/// Open a file or folder with the system default application
#[serde(rename = "open_with_system")]
OpenWithSystem(OpenWithSystemParams),

/// This event is used to signal that the stored messages the front-end
/// replays when constructing multi-output plots should be reset. This
/// happens for things like a holoviews extension being changed.
Expand Down
46 changes: 23 additions & 23 deletions crates/ark/src/browser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@
//
//

use amalthea::comm::ui_comm::ShowUrlParams;
use amalthea::comm::ui_comm::UiFrontendEvent;
use harp::object::RObject;
use harp::utils::r_normalize_path;
use libr::Rf_ScalarLogical;
use libr::SEXP;

use crate::help::message::HelpEvent;
use crate::help::message::ShowHelpUrlParams;
use crate::interface::RMain;
use crate::ui::events::send_open_with_system_event;
use crate::ui::events::send_show_url_event;

#[harp::register]
pub unsafe extern "C-unwind" fn ps_browse_url(url: SEXP) -> anyhow::Result<SEXP> {
Expand All @@ -35,33 +36,32 @@ fn handle_help_url(url: String) -> anyhow::Result<()> {
}

unsafe fn ps_browse_url_impl(url: SEXP) -> anyhow::Result<SEXP> {
// Extract URL.
let url = RObject::view(url).to::<String>()?;
let _span = tracing::trace_span!("browseURL", url = %url).entered();
// Extract URL string for analysis
let url_string = RObject::view(url).to::<String>()?;
let _span = tracing::trace_span!("browseURL", url = %url_string).entered();

// Handle help server requests.
if is_help_url(&url) {
if is_help_url(&url_string) {
log::trace!("Help is handling URL");
handle_help_url(url)?;
handle_help_url(url_string)?;
return Ok(Rf_ScalarLogical(1));
} else {
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.
Comment on lines -51 to -52
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anyone want me to hang onto this comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, we should only inject this option when launched from Positron. Probably an easy fix if you want to do it here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really follow how ark would hit ps_browse_url_impl() outside of Positron. Because I assume it works like this:

In Positron, ark takes charge of the "browser" option and that eventually routes through here.

But outside of Positron, ark would presumably have the default implementation of "browser", yeah?

Is the claim that ark would error outside of Positron actually true or true anymore? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently I am wrong. Why do we set an option like "browser" in ark, but outside of Positron? This feels like it might be out of scope for this PR (a bigger problem). In which case I should just retain the comment and move on.

~/tmp % jupyter console --kernel=ark
Jupyter console 6.6.3


R version 4.4.2 (2024-10-31) -- "Pile of Leaves"
Copyright (C) 2024 The R Foundation for Statistical Computing
Platform: aarch64-apple-darwin20

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.


In [1]: browseURL("~/tmp")
  2025-07-21T21:35:02.025887Z ERROR  No help port is available to check if '~/tmp' is a help url. Is the help comm open?
    at crates/ark/src/interface.rs:2032

  2025-07-21T21:35:02.026935Z ERROR  Failed to browse url due to: UI comm not connected, can't run `open_with_system`.
    at crates/ark/src/browser.rs:22


In [2]: getOption("browser")
Out[2]:
function(url) {
    .ps.Call("ps_browse_url", as.character(url))
}
<environment: 0x11fd5bc70>

Copy link
Member Author

@jennybc jennybc Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are already tracking this problem here: #588.

So I'm going to merge this once the Positron side is approved.


// For all other URLs, create a ShowUrl event and send it to the main
// thread; Positron will handle it.
let params = ShowUrlParams { url };
let event = UiFrontendEvent::ShowUrl(params);

let main = RMain::get();
let ui_comm_tx = main
.get_ui_comm_tx()
.ok_or_else(|| anyhow::anyhow!("UI comm not connected."))?;
Comment on lines -54 to -62
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was basically duplicated in ps_ui_show_url() so now they share logic via the send_show_url_event() helper.


ui_comm_tx.send_event(event);
// Handle web URLs
if is_web_url(&url_string) {
log::trace!("Handling web URL");
send_show_url_event(&url_string)?;
return Ok(Rf_ScalarLogical(1));
}

// This is probably a file path? Send to the front end and ask for system
// default opener.
log::trace!("Treating as file path and asking system to open");
let path = r_normalize_path(url.into())?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The opener service on the Positron side expects a resolved path, so it's important to normalize on the backend.

send_open_with_system_event(&path)?;
Ok(Rf_ScalarLogical(1))
}

fn is_web_url(url: &str) -> bool {
url.starts_with("http://") || url.starts_with("https://")
}
31 changes: 26 additions & 5 deletions crates/ark/src/ui/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
//

use amalthea::comm::ui_comm::OpenEditorParams;
use amalthea::comm::ui_comm::OpenWithSystemParams;
use amalthea::comm::ui_comm::OpenWorkspaceParams;
use amalthea::comm::ui_comm::Position;
use amalthea::comm::ui_comm::Range;
Expand Down Expand Up @@ -96,23 +97,43 @@ pub unsafe extern "C-unwind" fn ps_ui_set_selection_ranges(ranges: SEXP) -> anyh
Ok(R_NilValue)
}

#[harp::register]
pub unsafe extern "C-unwind" fn ps_ui_show_url(url: SEXP) -> anyhow::Result<SEXP> {
pub fn send_show_url_event(url: &str) -> anyhow::Result<()> {
let params = ShowUrlParams {
url: RObject::view(url).try_into()?,
url: url.to_string(),
};

let event = UiFrontendEvent::ShowUrl(params);

let main = RMain::get();
let ui_comm_tx = main
.get_ui_comm_tx()
.ok_or_else(|| ui_comm_not_connected("ui_show_url"))?;
.ok_or_else(|| ui_comm_not_connected("show_url"))?;
ui_comm_tx.send_event(event);

Ok(())
}

#[harp::register]
pub unsafe extern "C-unwind" fn ps_ui_show_url(url: SEXP) -> anyhow::Result<SEXP> {
let url_string = RObject::view(url).to::<String>()?;
send_show_url_event(&url_string)?;
Ok(R_NilValue)
}

pub fn send_open_with_system_event(path: &str) -> anyhow::Result<()> {
let params = OpenWithSystemParams {
path: path.to_string(),
};
let event = UiFrontendEvent::OpenWithSystem(params);

let main = RMain::get();
let ui_comm_tx = main
.get_ui_comm_tx()
.ok_or_else(|| ui_comm_not_connected("open_with_system"))?;
ui_comm_tx.send_event(event);

Ok(())
}

pub fn ps_ui_robj_as_ranges(ranges: SEXP) -> anyhow::Result<Vec<Range>> {
let ranges_as_r_objects: Vec<RObject> = RObject::view(ranges).try_into()?;
let ranges_as_result: Result<Vec<Vec<i32>>, _> = ranges_as_r_objects
Expand Down
Loading