From 284a57d4d10fbad660d0669e49807b8f2c496bbd Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Thu, 7 Mar 2024 13:52:50 -0700 Subject: [PATCH] Fix panic in open urls (#9032) Co-Authored-By: Nathan Release Notes: - N/A Co-authored-by: Nathan --- crates/editor/src/editor.rs | 4 +- crates/gpui/src/app.rs | 9 +- crates/journal/src/journal.rs | 2 +- crates/recent_projects/src/recent_projects.rs | 2 +- crates/welcome/src/welcome.rs | 4 +- crates/workspace/src/workspace.rs | 11 +- crates/zed/src/main.rs | 209 +++++++----------- crates/zed/src/open_listener.rs | 136 ++++++------ crates/zed/src/zed.rs | 24 +- 9 files changed, 176 insertions(+), 225 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index a789b82eae..df4836425c 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -249,7 +249,7 @@ pub fn init(cx: &mut AppContext) { cx.on_action(move |_: &workspace::NewFile, cx| { let app_state = workspace::AppState::global(cx); if let Some(app_state) = app_state.upgrade() { - workspace::open_new(&app_state, cx, |workspace, cx| { + workspace::open_new(app_state, cx, |workspace, cx| { Editor::new_file(workspace, &Default::default(), cx) }) .detach(); @@ -258,7 +258,7 @@ pub fn init(cx: &mut AppContext) { cx.on_action(move |_: &workspace::NewWindow, cx| { let app_state = workspace::AppState::global(cx); if let Some(app_state) = app_state.upgrade() { - workspace::open_new(&app_state, cx, |workspace, cx| { + workspace::open_new(app_state, cx, |workspace, cx| { Editor::new_file(workspace, &Default::default(), cx) }) .detach(); diff --git a/crates/gpui/src/app.rs b/crates/gpui/src/app.rs index cc03e9a8e6..0591a07866 100644 --- a/crates/gpui/src/app.rs +++ b/crates/gpui/src/app.rs @@ -150,14 +150,9 @@ impl App { /// to open one or more URLs. pub fn on_open_urls(&self, mut callback: F) -> &Self where - F: 'static + FnMut(Vec, &mut AppContext), + F: 'static + FnMut(Vec), { - let this = Rc::downgrade(&self.0); - self.0.borrow().platform.on_open_urls(Box::new(move |urls| { - if let Some(app) = this.upgrade() { - callback(urls, &mut app.borrow_mut()); - } - })); + self.0.borrow().platform.on_open_urls(Box::new(callback)); self } diff --git a/crates/journal/src/journal.rs b/crates/journal/src/journal.rs index 7383a0ee55..a78d74a55d 100644 --- a/crates/journal/src/journal.rs +++ b/crates/journal/src/journal.rs @@ -102,7 +102,7 @@ pub fn new_journal_entry(app_state: Arc, cx: &mut WindowContext) { cx.spawn(|mut cx| async move { let (journal_dir, entry_path) = create_entry.await?; let (workspace, _) = cx - .update(|cx| workspace::open_paths(&[journal_dir], &app_state, None, cx))? + .update(|cx| workspace::open_paths(&[journal_dir], app_state, None, cx))? .await?; let opened = workspace diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index 3655fbb6e6..a5e8c2df32 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -493,7 +493,7 @@ mod tests { }), ) .await; - cx.update(|cx| open_paths(&[PathBuf::from("/dir/main.ts")], &app_state, None, cx)) + cx.update(|cx| open_paths(&[PathBuf::from("/dir/main.ts")], app_state, None, cx)) .await .unwrap(); assert_eq!(cx.update(|cx| cx.windows().len()), 1); diff --git a/crates/welcome/src/welcome.rs b/crates/welcome/src/welcome.rs index 01ffe2a166..d36fcf53a8 100644 --- a/crates/welcome/src/welcome.rs +++ b/crates/welcome/src/welcome.rs @@ -37,8 +37,8 @@ pub fn init(cx: &mut AppContext) { base_keymap_picker::init(cx); } -pub fn show_welcome_view(app_state: &Arc, cx: &mut AppContext) { - open_new(&app_state, cx, |workspace, cx| { +pub fn show_welcome_view(app_state: Arc, cx: &mut AppContext) { + open_new(app_state, cx, |workspace, cx| { workspace.toggle_dock(DockPosition::Left, cx); let welcome_page = WelcomePage::new(workspace, cx); workspace.add_item_to_center(Box::new(welcome_page.clone()), cx); diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 71ba6127e9..a9e3443c7f 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -262,7 +262,7 @@ pub fn init(app_state: Arc, cx: &mut AppContext) { cx.spawn(move |cx| async move { if let Some(paths) = paths.await.log_err().flatten() { cx.update(|cx| { - open_paths(&paths, &app_state, None, cx).detach_and_log_err(cx) + open_paths(&paths, app_state, None, cx).detach_and_log_err(cx) }) .ok(); } @@ -1414,7 +1414,7 @@ impl Workspace { let app_state = self.app_state.clone(); cx.spawn(|_, mut cx| async move { - cx.update(|cx| open_paths(&paths, &app_state, window_to_replace, cx))? + cx.update(|cx| open_paths(&paths, app_state, window_to_replace, cx))? .await?; Ok(()) }) @@ -4381,7 +4381,7 @@ fn activate_any_workspace_window(cx: &mut AsyncAppContext) -> Option, + app_state: Arc, requesting_window: Option>, cx: &mut AppContext, ) -> Task< @@ -4390,7 +4390,6 @@ pub fn open_paths( Vec, anyhow::Error>>>, )>, > { - let app_state = app_state.clone(); let abs_paths = abs_paths.to_vec(); // Open paths in existing workspace if possible let existing = activate_workspace_for_project(cx, { @@ -4417,11 +4416,11 @@ pub fn open_paths( } pub fn open_new( - app_state: &Arc, + app_state: Arc, cx: &mut AppContext, init: impl FnOnce(&mut Workspace, &mut ViewContext) + 'static + Send, ) -> Task<()> { - let task = Workspace::new_local(Vec::new(), app_state.clone(), None, cx); + let task = Workspace::new_local(Vec::new(), app_state, None, cx); cx.spawn(|mut cx| async move { if let Some((workspace, opened_paths)) = task.await.log_err() { workspace diff --git a/crates/zed/src/main.rs b/crates/zed/src/main.rs index 277d4dda03..dfb66e7aa0 100644 --- a/crates/zed/src/main.rs +++ b/crates/zed/src/main.rs @@ -106,11 +106,11 @@ fn main() { let (listener, mut open_rx) = OpenListener::new(); let listener = Arc::new(listener); let open_listener = listener.clone(); - app.on_open_urls(move |urls, cx| open_listener.open_urls(&urls, cx)); + app.on_open_urls(move |urls| open_listener.open_urls(urls)); app.on_reopen(move |cx| { if let Some(app_state) = AppState::try_global(cx).and_then(|app_state| app_state.upgrade()) { - workspace::open_new(&app_state, cx, |workspace, cx| { + workspace::open_new(app_state, cx, |workspace, cx| { Editor::new_file(workspace, &Default::default(), cx) }) .detach(); @@ -273,7 +273,7 @@ fn main() { cx.activate(true); let urls = collect_url_args(cx); if !urls.is_empty() { - listener.open_urls(&urls, cx) + listener.open_urls(urls) } } else { upload_panics_and_crashes(http.clone(), cx); @@ -282,141 +282,38 @@ fn main() { if std::env::var(FORCE_CLI_MODE_ENV_VAR_NAME).ok().is_some() && !listener.triggered.load(Ordering::Acquire) { - listener.open_urls(&collect_url_args(cx), cx) + listener.open_urls(collect_url_args(cx)) } } let mut triggered_authentication = false; - fn open_paths_and_log_errs( - paths: &[PathBuf], - app_state: &Arc, - cx: &mut AppContext, - ) { - let task = workspace::open_paths(&paths, &app_state, None, cx); - cx.spawn(|_| async move { - if let Some((_window, results)) = task.await.log_err() { - for result in results.into_iter().flatten() { - if let Err(err) = result { - log::error!("Error opening path: {err}",); - } - } - } - }) - .detach(); - } - - match open_rx.try_next() { - Ok(Some(OpenRequest::Paths { paths })) => { - open_paths_and_log_errs(&paths, &app_state, cx) + match open_rx + .try_next() + .ok() + .flatten() + .and_then(|urls| OpenRequest::parse(urls, cx).log_err()) + { + Some(request) => { + triggered_authentication = handle_open_request(request, app_state.clone(), cx) } - Ok(Some(OpenRequest::CliConnection { connection })) => { - let app_state = app_state.clone(); - cx.spawn(move |cx| handle_cli_connection(connection, app_state, cx)) - .detach(); - } - Ok(Some(OpenRequest::JoinChannel { channel_id })) => { - triggered_authentication = true; - let app_state = app_state.clone(); - let client = client.clone(); - cx.spawn(|cx| async move { - // ignore errors here, we'll show a generic "not signed in" - let _ = authenticate(client, &cx).await; - cx.update(|cx| { - workspace::join_channel(client::ChannelId(channel_id), app_state, None, cx) - })? - .await?; - anyhow::Ok(()) - }) - .detach_and_log_err(cx); - } - Ok(Some(OpenRequest::OpenChannelNotes { - channel_id, - heading, - })) => { - triggered_authentication = true; - let app_state = app_state.clone(); - let client = client.clone(); - cx.spawn(|mut cx| async move { - // ignore errors here, we'll show a generic "not signed in" - let _ = authenticate(client, &cx).await; - let workspace_window = - workspace::get_any_active_workspace(app_state, cx.clone()).await?; - let workspace = workspace_window.root_view(&cx)?; - cx.update_window(workspace_window.into(), |_, cx| { - ChannelView::open(client::ChannelId(channel_id), heading, workspace, cx) - })? - .await?; - anyhow::Ok(()) - }) - .detach_and_log_err(cx); - } - Ok(None) | Err(_) => cx + None => cx .spawn({ let app_state = app_state.clone(); - |cx| async move { restore_or_create_workspace(&app_state, cx).await } + |cx| async move { restore_or_create_workspace(app_state, cx).await } }) .detach(), } let app_state = app_state.clone(); cx.spawn(move |cx| async move { - while let Some(request) = open_rx.next().await { - match request { - OpenRequest::Paths { paths } => { - cx.update(|cx| open_paths_and_log_errs(&paths, &app_state, cx)) - .ok(); + while let Some(urls) = open_rx.next().await { + cx.update(|cx| { + if let Some(request) = OpenRequest::parse(urls, cx).log_err() { + handle_open_request(request, app_state.clone(), cx); } - OpenRequest::CliConnection { connection } => { - let app_state = app_state.clone(); - cx.spawn(move |cx| { - handle_cli_connection(connection, app_state.clone(), cx) - }) - .detach(); - } - OpenRequest::JoinChannel { channel_id } => { - let app_state = app_state.clone(); - cx.update(|mut cx| { - cx.spawn(|cx| async move { - cx.update(|cx| { - workspace::join_channel( - client::ChannelId(channel_id), - app_state, - None, - cx, - ) - })? - .await?; - anyhow::Ok(()) - }) - .detach_and_log_err(&mut cx); - }) - .log_err(); - } - OpenRequest::OpenChannelNotes { - channel_id, - heading, - } => { - let app_state = app_state.clone(); - let open_notes_task = cx.spawn(|mut cx| async move { - let workspace_window = - workspace::get_any_active_workspace(app_state, cx.clone()).await?; - let workspace = workspace_window.root_view(&cx)?; - cx.update_window(workspace_window.into(), |_, cx| { - ChannelView::open( - client::ChannelId(channel_id), - heading, - workspace, - cx, - ) - })? - .await?; - anyhow::Ok(()) - }); - cx.update(|cx| open_notes_task.detach_and_log_err(cx)) - .log_err(); - } - } + }) + .ok(); } }) .detach(); @@ -428,6 +325,70 @@ fn main() { }); } +fn open_paths_and_log_errs(paths: &[PathBuf], app_state: Arc, cx: &mut AppContext) { + let task = workspace::open_paths(&paths, app_state, None, cx); + cx.spawn(|_| async move { + if let Some((_window, results)) = task.await.log_err() { + for result in results.into_iter().flatten() { + if let Err(err) = result { + log::error!("Error opening path: {err}",); + } + } + } + }) + .detach(); +} + +fn handle_open_request( + request: OpenRequest, + app_state: Arc, + cx: &mut AppContext, +) -> bool { + let mut triggered_authentication = false; + match request { + OpenRequest::Paths { paths } => open_paths_and_log_errs(&paths, app_state, cx), + OpenRequest::CliConnection { connection } => { + let app_state = app_state.clone(); + cx.spawn(move |cx| handle_cli_connection(connection, app_state, cx)) + .detach(); + } + OpenRequest::JoinChannel { channel_id } => { + triggered_authentication = true; + cx.spawn(|cx| async move { + // ignore errors here, we'll show a generic "not signed in" + let _ = authenticate(app_state.client.clone(), &cx).await; + cx.update(|cx| { + workspace::join_channel(client::ChannelId(channel_id), app_state, None, cx) + })? + .await?; + anyhow::Ok(()) + }) + .detach_and_log_err(cx); + } + OpenRequest::OpenChannelNotes { + channel_id, + heading, + } => { + triggered_authentication = true; + let client = app_state.client.clone(); + cx.spawn(|mut cx| async move { + // ignore errors here, we'll show a generic "not signed in" + let _ = authenticate(client, &cx).await; + let workspace_window = + workspace::get_any_active_workspace(app_state, cx.clone()).await?; + let workspace = workspace_window.root_view(&cx)?; + cx.update_window(workspace_window.into(), |_, cx| { + ChannelView::open(client::ChannelId(channel_id), heading, workspace, cx) + })? + .await?; + anyhow::Ok(()) + }) + .detach_and_log_err(cx); + } + } + triggered_authentication +} + async fn authenticate(client: Arc, cx: &AsyncAppContext) -> Result<()> { if stdout_is_a_pty() { if client::IMPERSONATE_LOGIN.is_some() { @@ -465,7 +426,7 @@ async fn installation_id() -> Result<(String, bool)> { Ok((installation_id, false)) } -async fn restore_or_create_workspace(app_state: &Arc, cx: AsyncAppContext) { +async fn restore_or_create_workspace(app_state: Arc, cx: AsyncAppContext) { async_maybe!({ if let Some(location) = workspace::last_opened_workspace_paths().await { cx.update(|cx| workspace::open_paths(location.paths().as_ref(), app_state, None, cx))? diff --git a/crates/zed/src/open_listener.rs b/crates/zed/src/open_listener.rs index 41dfe7e432..d875219da6 100644 --- a/crates/zed/src/open_listener.rs +++ b/crates/zed/src/open_listener.rs @@ -37,8 +37,66 @@ pub enum OpenRequest { }, } +impl OpenRequest { + pub fn parse(urls: Vec, cx: &AppContext) -> Result { + if let Some(server_name) = urls.first().and_then(|url| url.strip_prefix("zed-cli://")) { + Self::parse_cli_connection(server_name) + } else if let Some(request_path) = urls.first().and_then(|url| parse_zed_link(url, cx)) { + Self::parse_zed_url(request_path) + } else { + Ok(Self::parse_file_urls(urls)) + } + } + + fn parse_cli_connection(server_name: &str) -> Result { + let connection = connect_to_cli(server_name)?; + Ok(OpenRequest::CliConnection { connection }) + } + + fn parse_zed_url(request_path: &str) -> Result { + let mut parts = request_path.split('/'); + if parts.next() == Some("channel") { + if let Some(slug) = parts.next() { + if let Some(id_str) = slug.split('-').last() { + if let Ok(channel_id) = id_str.parse::() { + let Some(next) = parts.next() else { + return Ok(OpenRequest::JoinChannel { channel_id }); + }; + + if let Some(heading) = next.strip_prefix("notes#") { + return Ok(OpenRequest::OpenChannelNotes { + channel_id, + heading: Some([heading].into_iter().chain(parts).join("/")), + }); + } else if next == "notes" { + return Ok(OpenRequest::OpenChannelNotes { + channel_id, + heading: None, + }); + } + } + } + } + } + Err(anyhow!("invalid zed url: {}", request_path)) + } + + fn parse_file_urls(urls: Vec) -> OpenRequest { + let paths: Vec<_> = urls + .iter() + .flat_map(|url| url.strip_prefix("file://")) + .flat_map(|url| { + let decoded = urlencoding::decode_binary(url.as_bytes()); + PathBuf::try_from_bytes(decoded.as_ref()).log_err() + }) + .collect(); + + OpenRequest::Paths { paths } + } +} + pub struct OpenListener { - tx: UnboundedSender, + tx: UnboundedSender>, pub triggered: AtomicBool, } @@ -55,7 +113,7 @@ impl OpenListener { cx.set_global(GlobalOpenListener(listener)) } - pub fn new() -> (Self, UnboundedReceiver) { + pub fn new() -> (Self, UnboundedReceiver>) { let (tx, rx) = mpsc::unbounded(); ( OpenListener { @@ -66,74 +124,12 @@ impl OpenListener { ) } - pub fn open_urls(&self, urls: &[String], cx: &AppContext) { + pub fn open_urls(&self, urls: Vec) { self.triggered.store(true, Ordering::Release); - let request = if let Some(server_name) = - urls.first().and_then(|url| url.strip_prefix("zed-cli://")) - { - self.handle_cli_connection(server_name) - } else if let Some(request_path) = urls.first().and_then(|url| parse_zed_link(url, cx)) { - self.handle_zed_url_scheme(request_path) - } else { - self.handle_file_urls(urls) - }; - - if let Some(request) = request { - self.tx - .unbounded_send(request) - .map_err(|_| anyhow!("no listener for open requests")) - .log_err(); - } - } - - fn handle_cli_connection(&self, server_name: &str) -> Option { - if let Some(connection) = connect_to_cli(server_name).log_err() { - return Some(OpenRequest::CliConnection { connection }); - } - - None - } - - fn handle_zed_url_scheme(&self, request_path: &str) -> Option { - let mut parts = request_path.split('/'); - if parts.next() == Some("channel") { - if let Some(slug) = parts.next() { - if let Some(id_str) = slug.split('-').last() { - if let Ok(channel_id) = id_str.parse::() { - let Some(next) = parts.next() else { - return Some(OpenRequest::JoinChannel { channel_id }); - }; - - if let Some(heading) = next.strip_prefix("notes#") { - return Some(OpenRequest::OpenChannelNotes { - channel_id, - heading: Some([heading].into_iter().chain(parts).join("/")), - }); - } else if next == "notes" { - return Some(OpenRequest::OpenChannelNotes { - channel_id, - heading: None, - }); - } - } - } - } - } - log::error!("invalid zed url: {}", request_path); - None - } - - fn handle_file_urls(&self, urls: &[String]) -> Option { - let paths: Vec<_> = urls - .iter() - .flat_map(|url| url.strip_prefix("file://")) - .flat_map(|url| { - let decoded = urlencoding::decode_binary(url.as_bytes()); - PathBuf::try_from_bytes(decoded.as_ref()).log_err() - }) - .collect(); - - Some(OpenRequest::Paths { paths }) + self.tx + .unbounded_send(urls) + .map_err(|_| anyhow!("no listener for open requests")) + .log_err(); } } @@ -210,7 +206,7 @@ pub async fn handle_cli_connection( let mut errored = false; - match cx.update(|cx| workspace::open_paths(&paths, &app_state, None, cx)) { + match cx.update(|cx| workspace::open_paths(&paths, app_state, None, cx)) { Ok(task) => match task.await { Ok((workspace, items)) => { let mut item_release_futures = Vec::new(); diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index ca28267472..4d32b40a19 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -233,7 +233,7 @@ pub fn initialize_workspace(app_state: Arc, cx: &mut AppContext) { cx.toggle_full_screen(); }) .register_action(|_, action: &OpenZedUrl, cx| { - OpenListener::global(cx).open_urls(&[action.url.clone()], cx) + OpenListener::global(cx).open_urls(vec![action.url.clone()]) }) .register_action(|_, action: &OpenBrowser, cx| cx.open_url(&action.url)) .register_action(move |_, _: &IncreaseBufferFontSize, cx| { @@ -399,7 +399,7 @@ pub fn initialize_workspace(app_state: Arc, cx: &mut AppContext) { let app_state = Arc::downgrade(&app_state); move |_, _: &NewWindow, cx| { if let Some(app_state) = app_state.upgrade() { - open_new(&app_state, cx, |workspace, cx| { + open_new(app_state, cx, |workspace, cx| { Editor::new_file(workspace, &Default::default(), cx) }) .detach(); @@ -410,7 +410,7 @@ pub fn initialize_workspace(app_state: Arc, cx: &mut AppContext) { let app_state = Arc::downgrade(&app_state); move |_, _: &NewFile, cx| { if let Some(app_state) = app_state.upgrade() { - open_new(&app_state, cx, |workspace, cx| { + open_new(app_state, cx, |workspace, cx| { Editor::new_file(workspace, &Default::default(), cx) }) .detach(); @@ -911,7 +911,7 @@ mod tests { cx.update(|cx| { open_paths( &[PathBuf::from("/root/a"), PathBuf::from("/root/b")], - &app_state, + app_state.clone(), None, cx, ) @@ -920,7 +920,7 @@ mod tests { .unwrap(); assert_eq!(cx.read(|cx| cx.windows().len()), 1); - cx.update(|cx| open_paths(&[PathBuf::from("/root/a")], &app_state, None, cx)) + cx.update(|cx| open_paths(&[PathBuf::from("/root/a")], app_state.clone(), None, cx)) .await .unwrap(); assert_eq!(cx.read(|cx| cx.windows().len()), 1); @@ -942,7 +942,7 @@ mod tests { cx.update(|cx| { open_paths( &[PathBuf::from("/root/b"), PathBuf::from("/root/c")], - &app_state, + app_state.clone(), None, cx, ) @@ -958,7 +958,7 @@ mod tests { cx.update(|cx| { open_paths( &[PathBuf::from("/root/c"), PathBuf::from("/root/d")], - &app_state, + app_state, Some(window), cx, ) @@ -995,7 +995,7 @@ mod tests { .insert_tree("/root", json!({"a": "hey"})) .await; - cx.update(|cx| open_paths(&[PathBuf::from("/root/a")], &app_state, None, cx)) + cx.update(|cx| open_paths(&[PathBuf::from("/root/a")], app_state.clone(), None, cx)) .await .unwrap(); assert_eq!(cx.update(|cx| cx.windows().len()), 1); @@ -1062,7 +1062,7 @@ mod tests { assert!(!window_is_edited(window, cx)); // Opening the buffer again doesn't impact the window's edited state. - cx.update(|cx| open_paths(&[PathBuf::from("/root/a")], &app_state, None, cx)) + cx.update(|cx| open_paths(&[PathBuf::from("/root/a")], app_state, None, cx)) .await .unwrap(); let editor = window @@ -1100,7 +1100,7 @@ mod tests { async fn test_new_empty_workspace(cx: &mut TestAppContext) { let app_state = init_test(cx); cx.update(|cx| { - open_new(&app_state, cx, |workspace, cx| { + open_new(app_state.clone(), cx, |workspace, cx| { Editor::new_file(workspace, &Default::default(), cx) }) }) @@ -1291,7 +1291,7 @@ mod tests { ) .await; - cx.update(|cx| open_paths(&[PathBuf::from("/dir1/")], &app_state, None, cx)) + cx.update(|cx| open_paths(&[PathBuf::from("/dir1/")], app_state, None, cx)) .await .unwrap(); assert_eq!(cx.update(|cx| cx.windows().len()), 1); @@ -1525,7 +1525,7 @@ mod tests { Path::new("/root/excluded_dir/ignored_subdir").to_path_buf(), ]; let (opened_workspace, new_items) = cx - .update(|cx| workspace::open_paths(&paths_to_open, &app_state, None, cx)) + .update(|cx| workspace::open_paths(&paths_to_open, app_state, None, cx)) .await .unwrap();