diff --git a/crates/tty7-core/src/daemon/ssh/forward.rs b/crates/tty7-core/src/daemon/ssh/forward.rs index 4996fd5e..c1e28ead 100644 --- a/crates/tty7-core/src/daemon/ssh/forward.rs +++ b/crates/tty7-core/src/daemon/ssh/forward.rs @@ -199,6 +199,53 @@ enum ForwardCancel { None, } +/// A forward's status, shared with the task that serves it. +/// +/// The status used to be a plain field written once when the forward was set +/// up and never again, so a forward whose accept loop had already exited went +/// on reporting `Listening` for as long as the pane stayed open — which is +/// forever, because a pane is deliberately not closed when its SSH connection +/// dies. The task that discovers the truth is the one that has to be able to +/// record it. +type SharedStatus = Arc>; + +/// Why a forward's accept loop stopped. +enum LoopExit { + /// `accept()` failed over and over: the listening socket is no longer + /// usable, so nothing can even reach the forward any more. + ListenerLost, + /// The SSH transport went away under it. The port may still be bound — + /// a connection to it is accepted and then dropped — but there is nothing + /// on the far side of it. + ConnectionLost, +} + +/// The status a forward is left in once its loop has exited. +/// +/// `ForwardStatus::Error` rather than a `Stopped` variant of its own, on +/// purpose: `ForwardStatus` is serialised across the protocol to remote +/// `tty7-server` builds, and a variant an older remote has never heard of is a +/// deserialisation failure rather than an unknown status. `Error` already +/// draws as a danger badge with its message beside it. +fn loop_exit_status(exit: LoopExit) -> ForwardStatus { + ForwardStatus::Error( + match exit { + LoopExit::ListenerLost => "stopped: the listening socket closed", + LoopExit::ConnectionLost => "stopped: the SSH connection went away", + } + .to_string(), + ) +} + +/// A forward that never got as far as a listening socket. It has no task, so +/// nothing will ever move it off this status. +fn bind_failed(rule: &SshForwardRule, e: io::Error) -> SharedStatus { + Arc::new(Mutex::new(ForwardStatus::Error(format!( + "bind {}:{} failed: {e}", + rule.bind_host, rule.bind_port + )))) +} + struct ForwardEntry { id: u64, kind: SshForwardKind, @@ -207,7 +254,7 @@ struct ForwardEntry { target_host: String, target_port: u16, description: Option, - status: ForwardStatus, + status: SharedStatus, cancel: ForwardCancel, auto_local: bool, } @@ -229,7 +276,7 @@ impl ForwardEntry { target_host: self.target_host.clone(), target_port: self.target_port, description: self.description.clone(), - status: self.status.clone(), + status: self.status.lock().unwrap().clone(), } } } @@ -390,18 +437,11 @@ impl SshForwardRegistry { &self, conn: &Arc, rule: &SshForwardRule, - ) -> (u16, ForwardStatus, ForwardCancel) { + ) -> (u16, SharedStatus, ForwardCancel) { let listener = match TcpListener::bind((rule.bind_host.as_str(), rule.bind_port)).await { Ok(l) => l, Err(e) => { - return ( - rule.bind_port, - ForwardStatus::Error(format!( - "bind {}:{} failed: {e}", - rule.bind_host, rule.bind_port - )), - ForwardCancel::None, - ); + return (rule.bind_port, bind_failed(rule, e), ForwardCancel::None); } }; let bound = listener @@ -411,14 +451,16 @@ impl SshForwardRegistry { let conn = conn.clone(); let target_host = rule.target_host.clone(); let target_port = rule.target_port; + let status: SharedStatus = Arc::new(Mutex::new(ForwardStatus::Listening)); + let task_status = status.clone(); let handle = tokio::spawn(async move { - loop { + let exit = loop { let sock = match accept_retrying(&listener).await { Some((sock, _peer)) => sock, - None => break, + None => break LoopExit::ListenerLost, }; if !conn.is_alive() { - break; + break LoopExit::ConnectionLost; } let conn = conn.clone(); let target_host = target_host.clone(); @@ -432,27 +474,21 @@ impl SshForwardRegistry { } } }); - } + }; + *task_status.lock().unwrap() = loop_exit_status(exit); }); - (bound, ForwardStatus::Listening, ForwardCancel::Task(handle)) + (bound, status, ForwardCancel::Task(handle)) } async fn start_dynamic( &self, conn: &Arc, rule: &SshForwardRule, - ) -> (u16, ForwardStatus, ForwardCancel) { + ) -> (u16, SharedStatus, ForwardCancel) { let listener = match TcpListener::bind((rule.bind_host.as_str(), rule.bind_port)).await { Ok(l) => l, Err(e) => { - return ( - rule.bind_port, - ForwardStatus::Error(format!( - "bind {}:{} failed: {e}", - rule.bind_host, rule.bind_port - )), - ForwardCancel::None, - ); + return (rule.bind_port, bind_failed(rule, e), ForwardCancel::None); } }; let bound = listener @@ -460,14 +496,16 @@ impl SshForwardRegistry { .map(|a| a.port()) .unwrap_or(rule.bind_port); let conn = conn.clone(); + let status: SharedStatus = Arc::new(Mutex::new(ForwardStatus::Listening)); + let task_status = status.clone(); let handle = tokio::spawn(async move { - loop { + let exit = loop { let sock = match accept_retrying(&listener).await { Some((sock, _peer)) => sock, - None => break, + None => break LoopExit::ListenerLost, }; if !conn.is_alive() { - break; + break LoopExit::ConnectionLost; } let conn = conn.clone(); tokio::spawn(async move { @@ -492,16 +530,21 @@ impl SshForwardRegistry { } } }); - } + }; + *task_status.lock().unwrap() = loop_exit_status(exit); }); - (bound, ForwardStatus::Listening, ForwardCancel::Task(handle)) + (bound, status, ForwardCancel::Task(handle)) } + /// Unlike the two above, a remote forward has no accept loop of its own to + /// notice a dead transport: the far end opens the channels and russh hands + /// them to the connection's handler. Its status stays whatever the + /// `tcpip-forward` request answered. async fn start_remote( &self, conn: &Arc, rule: &SshForwardRule, - ) -> (u16, ForwardStatus, ForwardCancel) { + ) -> (u16, SharedStatus, ForwardCancel) { match conn .add_remote_forward( &rule.bind_host, @@ -513,7 +556,7 @@ impl SshForwardRegistry { { Ok(bound) => ( bound, - ForwardStatus::Listening, + Arc::new(Mutex::new(ForwardStatus::Listening)), ForwardCancel::Remote { conn: Arc::downgrade(conn), bind_host: rule.bind_host.clone(), @@ -522,7 +565,9 @@ impl SshForwardRegistry { ), Err(e) => ( rule.bind_port, - ForwardStatus::Error(format!("remote forward request denied: {e}")), + Arc::new(Mutex::new(ForwardStatus::Error(format!( + "remote forward request denied: {e}" + )))), ForwardCancel::None, ), } @@ -576,7 +621,7 @@ impl SshForwardRegistry { }; let id = self.next_id.fetch_add(1, Ordering::Relaxed); let (bind_port, status, cancel) = self.start_local(&conn, &rule).await; - if let ForwardStatus::Error(e) = &status { + if let ForwardStatus::Error(e) = &*status.lock().unwrap() { return Err(io::Error::other(e.clone())); } let entry = ForwardEntry { @@ -617,7 +662,9 @@ impl SshForwardRegistry { && e.kind == SshForwardKind::Local && e.target_host == remote_host && e.target_port == remote_port - && matches!(e.status, ForwardStatus::Listening) + // Now that a dead loop says so, this stops handing out the + // port of a forward that no longer serves anything. + && matches!(*e.status.lock().unwrap(), ForwardStatus::Listening) }) .map(|e| e.bind_port) } @@ -814,6 +861,47 @@ mod tests { assert_eq!(table.lookup("localhost", 9000), None); } + #[test] + fn a_stopped_forward_says_why_it_stopped() { + let listener = loop_exit_status(LoopExit::ListenerLost); + let connection = loop_exit_status(LoopExit::ConnectionLost); + assert_ne!( + listener, connection, + "a socket that closed and a transport that died are different problems" + ); + for status in [&listener, &connection] { + assert!( + matches!(status, ForwardStatus::Error(_)), + "an older remote has to be able to deserialise this, so no new variant" + ); + } + } + + /// A forward's status used to be copied into `ManagedForward` from a field + /// written once when the forward was set up, so a forward whose loop had + /// long since exited answered `Listening` to every poll of the panel. + #[tokio::test] + async fn a_forward_whose_loop_exited_stops_reporting_listening() { + let reg = SshForwardRegistry::default(); + push_listener(®, ForwardOwner::Pane(7), 0).await; + assert!(matches!(reg.list(7)[0].status, ForwardStatus::Listening)); + + // What the accept loop does on its way out. The entry is already in the + // registry by then, which is the whole reason the status is shared. + let status = reg.owners.lock().unwrap()[&ForwardOwner::Pane(7)][0] + .status + .clone(); + *status.lock().unwrap() = loop_exit_status(LoopExit::ConnectionLost); + + match ®.list(7)[0].status { + ForwardStatus::Error(msg) => assert!( + msg.contains("SSH connection"), + "the panel needs a reason, not just a red badge: {msg}" + ), + other => panic!("a dead forward still reports {other:?}"), + } + } + #[tokio::test] async fn registry_add_list_remove_teardown_bookkeeping() { let reg = SshForwardRegistry::default(); @@ -827,7 +915,7 @@ mod tests { target_host: "h".into(), target_port: 80, description: None, - status: ForwardStatus::Listening, + status: Arc::new(Mutex::new(ForwardStatus::Listening)), cancel: ForwardCancel::Task(task), auto_local: false, } @@ -872,7 +960,7 @@ mod tests { target_host: "h".into(), target_port: 80, description: None, - status: ForwardStatus::Listening, + status: Arc::new(Mutex::new(ForwardStatus::Listening)), cancel: ForwardCancel::Task(handle), auto_local: false, } @@ -933,7 +1021,7 @@ mod tests { target_host: "127.0.0.1".into(), target_port: 3000, description: None, - status: ForwardStatus::Listening, + status: Arc::new(Mutex::new(ForwardStatus::Listening)), cancel: ForwardCancel::Task(handle), auto_local: true, }; diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index 7b660143..a9ab0a70 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -1478,18 +1478,20 @@ impl RemoteTerminal { query(job_id).unwrap_or_default() } - pub fn sftp_transfer_list(pane_id: u64) -> Vec { - fn query(pane_id: u64) -> anyhow::Result> { + /// A failed poll is not an empty transfer list: the caller has to be able + /// to keep the jobs it already knows about, so this reports the failure + /// the way `sftp_list` does rather than answering with an empty `Vec`. + pub fn sftp_transfer_list(pane_id: u64) -> Result, String> { + fn query(pane_id: u64) -> anyhow::Result, String>> { let mut stream = connect()?; ClientMsg::SftpTransferList { pane_id }.encode(&mut stream)?; - match DaemonMsg::read(&mut stream)? { + Ok(match DaemonMsg::read(&mut stream)? { DaemonMsg::SftpTransferProgress(jobs) => Ok(jobs), - other => Err(anyhow::anyhow!( - "unexpected reply to SftpTransferList: {other:?}" - )), - } + DaemonMsg::Error(msg) => Err(msg), + other => Err(format!("unexpected reply to SftpTransferList: {other:?}")), + }) } - query(pane_id).unwrap_or_default() + query(pane_id).unwrap_or_else(|e| Err(e.to_string())) } pub fn add_forward(pane_id: u64, rule: SshForwardRule) -> Vec { diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 401124ee..9f741f42 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -2654,6 +2654,9 @@ impl TerminalView { cx.background_spawn(async move { route.transfer_list() }) .await }; + // A poll that failed says nothing about the job — keep asking + // until it answers or the budget above runs out. + let Ok(listed) = listed else { continue }; let Some(progress) = listed.into_iter().find(|j| j.job_id == job) else { // Pruned after the retention window, or the daemon restarted: // there is nothing left to report either way. diff --git a/src/ui/file_tree.rs b/src/ui/file_tree.rs index 08a14a1f..2b0057db 100644 --- a/src/ui/file_tree.rs +++ b/src/ui/file_tree.rs @@ -66,6 +66,9 @@ pub(crate) enum TreeNote { /// The search stopped at `SEARCH_LIMIT`; the list is a prefix, not the /// whole answer, and has to say so. SearchCapped, + /// The search never ran to an answer — the host refused it or the link to + /// it went away. An empty list here means nothing at all. + SearchFailed, } /// `landed` is how many entries the listing returned, or `None` when nothing @@ -122,6 +125,10 @@ struct SearchState { pending: String, hidden: bool, hits: Vec, + /// Whether the last search came back as a failure rather than as no hits. + /// The two used to print the same "Nothing matches …", which is the same + /// lie `unreadable` was added to stop a directory listing from telling. + failed: bool, } impl SearchState { @@ -134,22 +141,25 @@ impl SearchState { self.hidden = show_hidden; if query.is_empty() { self.hits.clear(); + self.failed = false; return None; } Some(self.generation) } - fn accept(&mut self, generation: u64, hits: Vec) -> bool { + fn accept(&mut self, generation: u64, ok: bool, hits: Vec) -> bool { if self.generation != generation { return false; } self.hits = hits; + self.failed = !ok; true } fn restart(&mut self) { self.generation += 1; self.pending.clear(); + self.failed = false; } } @@ -438,7 +448,14 @@ impl FileTreeState { host, cx, move |h| { - h.search(&roots, &query, SEARCH_LIMIT, SEARCH_MAX_DIRS, show_hidden) + // `(ok, hits)` the way `spawn_load` reports a listing: + // a search the host refused is not a search with no + // hits, and the column has to be able to tell them + // apart before it says "Nothing matches". + let found = + h.search(&roots, &query, SEARCH_LIMIT, SEARCH_MAX_DIRS, show_hidden); + let ok = found.is_ok(); + let hits = found .unwrap_or_default() .into_iter() .map(|hit| TreeEntry { @@ -447,10 +464,11 @@ impl FileTreeState { is_dir: hit.is_dir, ignored: hit.ignored, }) - .collect::>() + .collect::>(); + (ok, hits) }, - move |app, hits, cx| { - if app.file_tree.search.accept(generation, hits) { + move |app, (ok, hits), cx| { + if app.file_tree.search.accept(generation, ok, hits) { cx.notify(); } }, @@ -461,35 +479,51 @@ impl FileTreeState { } fn search_rows(&self) -> Vec { - let mut rows: Vec = self - .search - .hits - .iter() - .map(|e| TreeRow { - entry: e.clone(), - depth: 0, - is_root: false, - expanded: false, - note: None, - }) - .collect(); - if rows.len() >= SEARCH_LIMIT { - rows.push(TreeRow { - entry: TreeEntry { - name: String::new(), - path: PathBuf::new(), - is_dir: false, - ignored: false, - }, - depth: 0, - is_root: false, - expanded: false, - note: Some(TreeNote::SearchCapped), - }); - } - rows + search_rows(&self.search) } +} +/// The rows a search puts in the column, and the note that stands for whatever +/// they do not say by themselves. +fn search_rows(search: &SearchState) -> Vec { + let mut rows: Vec = search + .hits + .iter() + .map(|e| TreeRow { + entry: e.clone(), + depth: 0, + is_root: false, + expanded: false, + note: None, + }) + .collect(); + let note = if search.failed { + // Ahead of the cap: a failed search has no hits to have capped, and + // this is the one thing worth saying about it. + Some(TreeNote::SearchFailed) + } else if rows.len() >= SEARCH_LIMIT { + Some(TreeNote::SearchCapped) + } else { + None + }; + if let Some(note) = note { + rows.push(TreeRow { + entry: TreeEntry { + name: String::new(), + path: PathBuf::new(), + is_dir: false, + ignored: false, + }, + depth: 0, + is_root: false, + expanded: false, + note: Some(note), + }); + } + rows +} + +impl FileTreeState { pub(crate) fn visible_rows( &self, host: HostId, @@ -1116,6 +1150,20 @@ impl Tty7App { } let target = new_path.clone(); + // The same gap `file_tree_delete` closed: on its own, "Permission + // denied (os error 13)" says neither which file nor what was being + // done to it, and those are the only two things worth knowing here. + // A rename is named by the name it is leaving, which is the one still + // on screen to find. + let (context, failed_name) = match &edit { + TreeEdit::Rename { path, .. } => ( + L10nKey::FileTreeRenameFailed, + path.file_name() + .map(|n| n.to_string_lossy().to_string()) + .unwrap_or_else(|| name.clone()), + ), + _ => (L10nKey::FileTreeCreateFailed, name.clone()), + }; HostOps::run_in( host, window, @@ -1141,8 +1189,12 @@ impl Tty7App { { code.selected = None; } - use gpui_component::WindowExt as _; - window.push_notification(format!("{e}"), cx); + HostOps::notify_err( + window, + cx, + &t_fmt(context, &[("name", &failed_name)]), + &e, + ); } } cx.notify(); @@ -1503,6 +1555,7 @@ impl Tty7App { TreeNote::HiddenOnly => (L10nKey::TreeDirHiddenOnly, muted), TreeNote::Unreadable => (L10nKey::TreeDirUnreadable, cx.theme().danger), TreeNote::SearchCapped => (L10nKey::TreeSearchCapped, muted), + TreeNote::SearchFailed => (L10nKey::TreeSearchFailed, cx.theme().danger), }; return vec![ h_flex() @@ -1519,9 +1572,10 @@ impl Tty7App { TreeNote::SearchCapped => t_fmt(key, &[("n", &SEARCH_LIMIT.to_string())]), _ => t(key).to_string(), }) - // Every note but the capped-search one stands for a real - // directory, and carries its path; that one stands for the - // rest of a search and has nowhere to put anything. + // Every note but the two search ones stands for a real + // directory, and carries its path; those stand for the rest + // of a search, or for one that never ran, and have nowhere + // to put anything. .when(!path.as_os_str().is_empty(), |d| { d.drag_over::(|s, _, _, cx| { s.bg(cx.theme().drag_border.opacity(0.14)) @@ -2058,6 +2112,28 @@ mod tests { ); } + #[test] + fn a_failed_search_says_so_instead_of_drawing_no_rows() { + let mut search = SearchState::default(); + let walk = search.retarget("foo", false).expect("a new query walks"); + search.accept(walk, false, Vec::new()); + assert_eq!( + search_rows(&search) + .iter() + .filter_map(|r| r.note) + .collect::>(), + vec![TreeNote::SearchFailed], + "without a note the column falls through to \"Nothing matches\"" + ); + + // A search that ran and found nothing still draws nothing. + let walk = search + .retarget("bar", false) + .expect("a changed query walks"); + search.accept(walk, true, Vec::new()); + assert!(search_rows(&search).is_empty()); + } + #[test] fn a_listing_superseded_in_flight_is_still_shown() { let mut loads: InFlight = InFlight::default(); @@ -2426,10 +2502,10 @@ mod tests { assert_ne!(first, second); assert!( - !search.accept(first, vec![entry("stale.rs", false)]), + !search.accept(first, true, vec![entry("stale.rs", false)]), "the overtaken walk's hits are dropped" ); - assert!(search.accept(second, vec![entry("foo.rs", false)])); + assert!(search.accept(second, true, vec![entry("foo.rs", false)])); assert_eq!(search.hits.len(), 1); let third = search @@ -2445,6 +2521,30 @@ mod tests { assert!(search.retarget("foo", true).is_some(), "restart re-walks"); } + #[test] + fn a_search_that_failed_is_not_a_search_with_no_hits() { + let mut search = SearchState::default(); + let walk = search.retarget("foo", false).expect("a new query walks"); + assert!(search.accept(walk, false, Vec::new())); + assert!( + search.failed, + "an empty list from a host that refused the walk is not an answer" + ); + + // And it is not carried past the query it belongs to. + let next = search + .retarget("food", false) + .expect("a changed query walks"); + assert!(search.accept(next, true, vec![entry("food.rs", false)])); + assert!(!search.failed); + + let last = search.retarget("foodie", false).expect("and again"); + assert!(search.accept(last, false, Vec::new())); + assert!(search.failed); + search.retarget("", false); + assert!(!search.failed, "an emptied box has nothing to report"); + } + #[test] fn the_tree_reads_the_same_listing_out_of_the_host() { let host = tty7_core::host::local::LocalHost::new(); diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 6914debd..65229f14 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -39,6 +39,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::TreeDirHiddenOnly => "Only hidden files", L10nKey::TreeDirUnreadable => "Could not be read", L10nKey::TreeSearchCapped => "First {n} matches", + L10nKey::TreeSearchFailed => "Search failed", L10nKey::FileChangedOnDisk => "File changed on disk", L10nKey::Reload => "Reload", L10nKey::KeepMine => "Keep mine", @@ -816,6 +817,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::SftpTransferDone => "done", L10nKey::SftpTransferCancelled => "cancelled", L10nKey::SftpTransferError => "error", + L10nKey::SftpTransferListFailed => "Could not check transfers: {error}", L10nKey::SftpImagePasteUploadFailed => { "Could not upload the pasted image to {host}: {error}" } @@ -845,6 +847,8 @@ pub fn translate_en(key: L10nKey) -> &'static str { "The file will be deleted on {host}. There is no trash on the far side." } L10nKey::FileTreeDeleteFailed => "Could not delete {name}", + L10nKey::FileTreeCreateFailed => "Could not create {name}", + L10nKey::FileTreeRenameFailed => "Could not rename {name}", L10nKey::FileTreeContextOpen => "Open", L10nKey::FileTreeContextCdHere => "cd Here", L10nKey::FileTreeContextInsertPath => "Insert Path in Terminal", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index a8b2cccb..f544bd57 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -39,6 +39,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::TreeDirHiddenOnly => "隠しファイルのみ", L10nKey::TreeDirUnreadable => "読み取れません", L10nKey::TreeSearchCapped => "最初の {n} 件のみ", + L10nKey::TreeSearchFailed => "検索に失敗しました", L10nKey::FileChangedOnDisk => "ディスク上でファイルが変更されました", L10nKey::Reload => "再読み込み", L10nKey::KeepMine => "自分の変更を保持", @@ -858,6 +859,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::SftpTransferDone => "完了", L10nKey::SftpTransferCancelled => "キャンセル済み", L10nKey::SftpTransferError => "エラー", + L10nKey::SftpTransferListFailed => "転送状況を取得できませんでした: {error}", L10nKey::SftpImagePasteUploadFailed => { "貼り付けた画像を {host} にアップロードできませんでした: {error}" } @@ -887,6 +889,8 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { "{host} 上でファイルが削除されます。リモート側にゴミ箱はありません。" } L10nKey::FileTreeDeleteFailed => "{name} を削除できませんでした", + L10nKey::FileTreeCreateFailed => "{name} を作成できませんでした", + L10nKey::FileTreeRenameFailed => "{name} の名前を変更できませんでした", L10nKey::FileTreeContextOpen => "開く", L10nKey::FileTreeContextCdHere => "ここで cd", L10nKey::FileTreeContextInsertPath => "ターミナルにパスを挿入", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 489d29f7..7bd0f62c 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -107,6 +107,7 @@ l10n_keys! { TreeDirHiddenOnly, TreeDirUnreadable, TreeSearchCapped, + TreeSearchFailed, FileChangedOnDisk, Reload, KeepMine, @@ -624,6 +625,7 @@ l10n_keys! { SftpTransferDone, SftpTransferCancelled, SftpTransferError, + SftpTransferListFailed, SftpImagePasteUploadFailed, ForwardPanelTitle, ForwardDisconnected, @@ -647,6 +649,8 @@ l10n_keys! { SftpDeleteFolderBody, SftpDeleteFileBody, FileTreeDeleteFailed, + FileTreeCreateFailed, + FileTreeRenameFailed, FileTreeContextOpen, FileTreeContextCdHere, FileTreeContextInsertPath, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index ec546971..45aba1f7 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -39,6 +39,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::TreeDirHiddenOnly => "只有隐藏文件", L10nKey::TreeDirUnreadable => "无法读取", L10nKey::TreeSearchCapped => "只显示前 {n} 个匹配", + L10nKey::TreeSearchFailed => "搜索失败", L10nKey::FileChangedOnDisk => "文件在磁盘上已被修改", L10nKey::Reload => "重新加载", L10nKey::KeepMine => "保留我的版本", @@ -785,6 +786,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::SftpTransferDone => "完成", L10nKey::SftpTransferCancelled => "已取消", L10nKey::SftpTransferError => "错误", + L10nKey::SftpTransferListFailed => "无法获取传输状态：{error}", L10nKey::SftpImagePasteUploadFailed => "无法将粘贴的图片上传到 {host}：{error}", L10nKey::ForwardPanelTitle => "端口转发", L10nKey::ForwardDisconnected => "已断开", @@ -810,6 +812,8 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { } L10nKey::SftpDeleteFileBody => "该文件将在 {host} 上被删除。远端没有回收站。", L10nKey::FileTreeDeleteFailed => "无法删除 {name}", + L10nKey::FileTreeCreateFailed => "无法创建 {name}", + L10nKey::FileTreeRenameFailed => "无法重命名 {name}", L10nKey::FileTreeContextOpen => "打开", L10nKey::FileTreeContextCdHere => "cd 到此处", L10nKey::FileTreeContextInsertPath => "在终端中插入路径", diff --git a/src/ui/sftp.rs b/src/ui/sftp.rs index 18a2d781..e723fd15 100644 --- a/src/ui/sftp.rs +++ b/src/ui/sftp.rs @@ -115,21 +115,18 @@ impl SftpRoute { } } - pub(crate) fn transfer_list(&self) -> Vec { + pub(crate) fn transfer_list(&self) -> Result, String> { let Some(req) = self.workspace_op(crate::daemon::protocol::WorkspaceOp::SftpTransferList) else { return RemoteTerminal::sftp_transfer_list(self.pane_id); }; match RemoteTerminal::on_workspace(req) { - Ok(crate::daemon::protocol::DaemonMsg::SftpTransferProgress(jobs)) => jobs, - Ok(other) => { - log::warn!("unexpected reply to a workspace transfer list: {other:?}"); - Vec::new() - } - Err(e) => { - log::warn!("workspace transfer list failed: {e}"); - Vec::new() - } + Ok(crate::daemon::protocol::DaemonMsg::SftpTransferProgress(jobs)) => Ok(jobs), + Ok(other) => Err(t_fmt( + L10nKey::SftpErrorUnexpectedReply, + &[("reply", &format!("{other:?}"))], + )), + Err(e) => Err(e.to_string()), } } } @@ -143,6 +140,12 @@ pub(crate) struct SftpPanelState { pub(crate) filter_input: gpui::Entity, pub(crate) error: Option, pub(crate) jobs: Vec, + /// Why the last transfer poll came back empty-handed, if it did. + /// + /// Kept apart from `error`, which blanks the directory listing: a poll + /// that could not reach the daemon says nothing about the listing already + /// on screen, and the transfer tray is the only place it belongs. + jobs_error: Option, /// Uploads this panel started whose landing it has not listed yet. /// /// An upload is written to `.tty7-upload-` and renamed into @@ -188,6 +191,7 @@ impl SftpPanelState { filter_input, error: None, jobs: Vec::new(), + jobs_error: None, uploads_awaiting_listing: HashSet::new(), claimed_downloads: HashSet::new(), dismissed_jobs: HashSet::new(), @@ -223,6 +227,24 @@ fn uploads_still_running(owed: &HashSet, jobs: &[SftpJobProgress]) -> HashS .collect() } +/// What the tray shows after a poll: the jobs to draw, and the failure to say +/// out loud beside them. +/// +/// A poll that failed used to come back as an empty `Vec`, which reads as "the +/// transfers are all gone" — the tray disappeared and every upload the panel +/// was waiting on counted as landed. Over a link that is down that is not a +/// blink but the permanent answer, so a failure keeps the previous list and is +/// reported instead of replacing it. +fn apply_poll( + previous: Vec, + reply: Result, String>, +) -> (Vec, Option) { + match reply { + Ok(jobs) => (jobs, None), + Err(e) => (previous, Some(e)), + } +} + fn is_dir_like(e: &SftpEntry) -> bool { matches!(e.kind, SftpEntryKind::Dir) || (matches!(e.kind, SftpEntryKind::Symlink) && e.target_is_dir) @@ -362,6 +384,7 @@ impl Tty7App { self.sftp_panel.editing_path = None; self.sftp_panel.editing_path_sub.clear(); self.sftp_panel.jobs.clear(); + self.sftp_panel.jobs_error = None; self.sftp_panel.open_workspace = None; self.sftp_panel.poll_gen = self.sftp_panel.poll_gen.wrapping_add(1); cx.notify(); @@ -1007,12 +1030,28 @@ impl Tty7App { /// listing on screen was the one taken while the temporary name existed — /// and it stayed, so a finished upload read as a file with a hash glued to /// its name. - fn sftp_apply_jobs(&mut self, jobs: Vec, cx: &mut Context) { + /// + /// A poll that failed is not a job list, so it settles nothing: the uploads + /// still owe their listing, and asking for one now would only refresh from + /// the same unreachable daemon. + fn sftp_apply_jobs( + &mut self, + reply: Result, String>, + cx: &mut Context, + ) { + let previous = std::mem::take(&mut self.sftp_panel.jobs); + let (jobs, failure) = apply_poll(previous, reply); + let failed = failure.is_some(); + self.sftp_panel.jobs = jobs; + self.sftp_panel.jobs_error = failure; + if failed { + cx.notify(); + return; + } let owed = &self.sftp_panel.uploads_awaiting_listing; - let still_running = uploads_still_running(owed, &jobs); + let still_running = uploads_still_running(owed, &self.sftp_panel.jobs); let settled = still_running.len() != owed.len(); self.sftp_panel.uploads_awaiting_listing = still_running; - self.sftp_panel.jobs = jobs; cx.notify(); if settled { self.sftp_refresh(cx); @@ -1568,7 +1607,11 @@ impl Tty7App { .iter() .filter(|j| history || !self.sftp_panel.dismissed_jobs.contains(&j.job_id)) .collect(); - if jobs.is_empty() && !history { + // A poll that failed is worth a tray of its own. Without one the whole + // footer vanishes at the moment the panel stops being able to say + // anything about the transfers, which reads as "they are all finished". + let jobs_error = self.sftp_panel.jobs_error.as_ref(); + if jobs.is_empty() && !history && jobs_error.is_none() { return None; } @@ -1596,7 +1639,12 @@ impl Tty7App { } else { 0.0 }; - let summary = if running > 0 { + // The failed poll outranks the counts, because the counts are only as + // fresh as the last poll that got through and the summary is the one + // line a collapsed tray gets to say. + let summary = if let Some(e) = jobs_error { + t_fmt(L10nKey::SftpTransferListFailed, &[("error", e)]) + } else if running > 0 { t_fmt( L10nKey::SftpTransferSummaryRunning, &[ @@ -1612,7 +1660,7 @@ impl Tty7App { } else { t(L10nKey::SftpTransferSummaryIdle).to_string() }; - let summary_color = if running == 0 && failed > 0 { + let summary_color = if jobs_error.is_some() || (running == 0 && failed > 0) { danger } else { muted @@ -1671,13 +1719,23 @@ impl Tty7App { let body = expanded.then(|| { let inner: Div = if jobs.is_empty() { + // The summary above says the same thing when a poll failed, but + // it is a single truncated line; this one wraps, so it is where + // the reason is actually readable. + let (text, color): (gpui::SharedString, _) = match jobs_error { + Some(e) => ( + t_fmt(L10nKey::SftpTransferListFailed, &[("error", e)]).into(), + danger, + ), + None => (t(L10nKey::SftpNoTransfers).into(), muted), + }; v_flex().child( div() .px(px(CONTENT_INSET)) .py(px(3.)) .text_size(rems(META)) - .text_color(muted) - .child(t(L10nKey::SftpNoTransfers)), + .text_color(color) + .child(text), ) } else { let mut list = v_flex().px(px(CONTENT_INSET)).pb(px(6.)).gap(px(6.)); @@ -1883,6 +1941,36 @@ mod tests { assert_eq!(running, HashSet::from([8])); } + #[test] + fn a_failed_poll_keeps_the_transfers_it_cannot_see() { + let previous = vec![upload(7, SftpJobState::Running)]; + let (jobs, failure) = apply_poll(previous.clone(), Err("broken pipe".into())); + assert_eq!(jobs.len(), 1, "the last list anyone saw is still the truth"); + assert_eq!(jobs[0].job_id, 7); + assert_eq!(failure.as_deref(), Some("broken pipe")); + + // And the upload is still owed its listing, so nothing settles behind + // a link that has gone quiet. + assert_eq!( + uploads_still_running(&HashSet::from([7]), &jobs), + HashSet::from([7]) + ); + } + + #[test] + fn a_poll_that_got_through_replaces_the_list_and_clears_the_failure() { + let previous = vec![upload(7, SftpJobState::Running)]; + let (jobs, failure) = apply_poll(previous, Ok(vec![upload(8, SftpJobState::Done)])); + assert_eq!(jobs.len(), 1); + assert_eq!(jobs[0].job_id, 8); + assert!(failure.is_none()); + + // An empty reply from a daemon that answered really is an empty list. + let (jobs, failure) = apply_poll(jobs, Ok(Vec::new())); + assert!(jobs.is_empty()); + assert!(failure.is_none()); + } + #[test] fn a_second_download_is_numbered_rather_than_written_over_the_first() { let dir = tempfile::tempdir().expect("tempdir");