From 75943f2b93acb76fb2e3f1c40a5b53f8bb405564 Mon Sep 17 00:00:00 2001 From: Christian Visintin Date: Sun, 9 Nov 2025 19:00:17 +0100 Subject: [PATCH] feat: Changed file overwrite behaviour (#366) Now the user can choose for each file whether to overwrite, skip or overwrite all/skip all. closes #335 --- CHANGELOG.md | 8 +- .../activities/filetransfer/actions/find.rs | 56 ++--- .../activities/filetransfer/actions/save.rs | 217 +++++++++++++----- .../activities/filetransfer/components/mod.rs | 5 +- .../filetransfer/components/popups.rs | 111 ++------- src/ui/activities/filetransfer/mod.rs | 11 +- src/ui/activities/filetransfer/update.rs | 10 - src/ui/activities/filetransfer/view.rs | 56 +---- 8 files changed, 218 insertions(+), 256 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8c7f87..17af7c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,13 +47,15 @@ Released on 20/09/2025 - [Issue 331](https://github.com/veeso/termscp/issues/331): Added new `import-ssh-hosts` CLI subcommand to import all the hosts from the ssh config as bookmarks. +- [Issue 335](https://github.com/veeso/termscp/issues/335): Changed file overwrite behaviour + - Now the user can choose for each file whether to overwrite, skip or overwrite all/skip all. +- [Issue 354](https://github.com/veeso/termscp/issues/354): + - Removed error popup message if failed to check for updates. + - Prevent long timeouts when checking for updates if the network is down or the DNS is not working. - [Issue 356](https://github.com/veeso/termscp/issues/356): Fixed SSH auth issue not trying with the password if any RSA key was found. - [Issue 334](https://github.com/veeso/termscp/issues/334): SMB support for MacOS with vendored build of libsmbclient. - [Issue 337](https://github.com/veeso/termscp/issues/337): Migrated to libssh.org on Linux and MacOS for better ssh agent support. - [Issue 361](https://github.com/veeso/termscp/issues/361): Report a message while calculating total size of files to transfer. -- [Issue 354](https://github.com/veeso/termscp/issues/354): - - Removed error popup message if failed to check for updates. - - Prevent long timeouts when checking for updates if the network is down or the DNS is not working. ## 0.18.0 diff --git a/src/ui/activities/filetransfer/actions/find.rs b/src/ui/activities/filetransfer/actions/find.rs index a8f3c9f..9ccff01 100644 --- a/src/ui/activities/filetransfer/actions/find.rs +++ b/src/ui/activities/filetransfer/actions/find.rs @@ -99,24 +99,16 @@ impl FileTransferActivity { // Iter files match self.browser.tab() { FileExplorerTab::FindHostBridge | FileExplorerTab::HostBridge => { - if self.config().get_prompt_on_file_replace() { - // Check which file would be replaced - let existing_files: Vec<&File> = entries - .iter() - .filter(|(x, dest_path)| { - self.remote_file_exists( - Self::file_to_check_many(x, dest_path.as_path()).as_path(), - ) - }) - .map(|(x, _)| x) - .collect(); - // Check whether to replace files - if !existing_files.is_empty() - && !self.should_replace_files(existing_files) - { - return; - } - } + let super::save::TransferFilesWithOverwritesResult::FilesToTransfer( + entries, + ) = self.get_files_to_transfer_with_overwrites( + entries, + super::save::CheckFileExists::Remote, + ) + else { + debug!("User cancelled file transfer due to overwrites"); + return; + }; if let Err(err) = self.filetransfer_send( TransferPayload::TransferQueue(entries), dest_path.as_path(), @@ -131,24 +123,16 @@ impl FileTransferActivity { } } FileExplorerTab::FindRemote | FileExplorerTab::Remote => { - if self.config().get_prompt_on_file_replace() { - // Check which file would be replaced - let existing_files: Vec<&File> = entries - .iter() - .filter(|(x, dest_path)| { - self.host_bridge_file_exists( - Self::file_to_check_many(x, dest_path.as_path()).as_path(), - ) - }) - .map(|(x, _)| x) - .collect(); - // Check whether to replace files - if !existing_files.is_empty() - && !self.should_replace_files(existing_files) - { - return; - } - } + let super::save::TransferFilesWithOverwritesResult::FilesToTransfer( + entries, + ) = self.get_files_to_transfer_with_overwrites( + entries, + super::save::CheckFileExists::HostBridge, + ) + else { + debug!("User cancelled file transfer due to overwrites"); + return; + }; if let Err(err) = self.filetransfer_recv( TransferPayload::TransferQueue(entries), dest_path.as_path(), diff --git a/src/ui/activities/filetransfer/actions/save.rs b/src/ui/activities/filetransfer/actions/save.rs index fe2cf65..d6c477e 100644 --- a/src/ui/activities/filetransfer/actions/save.rs +++ b/src/ui/activities/filetransfer/actions/save.rs @@ -10,6 +10,37 @@ use super::{ TransferPayload, }; +enum GetFileToReplaceResult { + Replace(Vec<(File, PathBuf)>), + Cancel, +} + +/// Result of getting files to transfer with overwrites. +/// +/// - FilesToTransfer: files to transfer. +/// - Cancel: user cancelled the operation. +pub(crate) enum TransferFilesWithOverwritesResult { + FilesToTransfer(Vec<(File, PathBuf)>), + Cancel, +} + +/// Decides whether to check file existence on host bridge or remote side. +pub(crate) enum CheckFileExists { + HostBridge, + Remote, +} + +/// Options for all files replacement. +/// +/// - ReplaceAll: user wants to replace all files. +/// - SkipAll: user wants to skip all files. +/// - Unset: no option set yet. +enum AllOpts { + ReplaceAll, + SkipAll, + Unset, +} + impl FileTransferActivity { pub(crate) fn action_local_saveas(&mut self, input: String) { self.local_send_file(TransferOpts::default().save_as(Some(input))); @@ -60,22 +91,12 @@ impl FileTransferActivity { dest_path.push(save_as); } // Iter files - if self.config().get_prompt_on_file_replace() { - // Check which file would be replaced - let existing_files: Vec<&File> = entries - .iter() - .filter(|(x, dest_path)| { - self.remote_file_exists( - Self::file_to_check_many(x, dest_path.as_path()).as_path(), - ) - }) - .map(|(x, _)| x) - .collect(); - // Check whether to replace files - if !existing_files.is_empty() && !self.should_replace_files(existing_files) { - return; - } - } + let TransferFilesWithOverwritesResult::FilesToTransfer(entries) = + self.get_files_to_transfer_with_overwrites(entries, CheckFileExists::Remote) + else { + debug!("User cancelled file transfer due to overwrites"); + return; + }; if let Err(err) = self.filetransfer_send( TransferPayload::TransferQueue(entries), dest_path.as_path(), @@ -128,23 +149,13 @@ impl FileTransferActivity { if let Some(save_as) = opts.save_as { dest_path.push(save_as); } - // Iter files - if self.config().get_prompt_on_file_replace() { - // Check which file would be replaced - let existing_files: Vec<&File> = entries - .iter() - .filter(|(x, dest_path)| { - self.host_bridge_file_exists( - Self::file_to_check_many(x, dest_path.as_path()).as_path(), - ) - }) - .map(|(x, _)| x) - .collect(); - // Check whether to replace files - if !existing_files.is_empty() && !self.should_replace_files(existing_files) { - return; - } - } + let TransferFilesWithOverwritesResult::FilesToTransfer(entries) = self + .get_files_to_transfer_with_overwrites(entries, CheckFileExists::HostBridge) + else { + debug!("User cancelled file transfer due to overwrites"); + return; + }; + if let Err(err) = self.filetransfer_recv( TransferPayload::TransferQueue(entries), dest_path.as_path(), @@ -172,11 +183,17 @@ impl FileTransferActivity { self.mount_radio_replace(&file_name); // Wait for answer trace!("Asking user whether he wants to replace file {}", file_name); - if self.wait_for_pending_msg(&[ - Msg::PendingAction(PendingActionMsg::CloseReplacePopups), - Msg::PendingAction(PendingActionMsg::TransferPendingFile), - ]) == Msg::PendingAction(PendingActionMsg::TransferPendingFile) - { + if matches!( + self.wait_for_pending_msg(&[ + Msg::PendingAction(PendingActionMsg::ReplaceCancel), + Msg::PendingAction(PendingActionMsg::ReplaceOverwrite), + Msg::PendingAction(PendingActionMsg::ReplaceSkip), + Msg::PendingAction(PendingActionMsg::ReplaceSkipAll), + Msg::PendingAction(PendingActionMsg::ReplaceOverwriteAll), + ]), + Msg::PendingAction(PendingActionMsg::ReplaceOverwrite) + | Msg::PendingAction(PendingActionMsg::ReplaceOverwriteAll) + ) { trace!("User wants to replace file"); self.umount_radio_replace(); true @@ -187,28 +204,76 @@ impl FileTransferActivity { } } - /// Set pending transfer for many files into storage and mount radio - pub(crate) fn should_replace_files(&mut self, files: Vec<&File>) -> bool { - let file_names: Vec = files.iter().map(|x| x.name()).collect(); - self.mount_radio_replace_many(file_names.as_slice()); - // Wait for answer - trace!( - "Asking user whether he wants to replace files {:?}", - file_names - ); - if self.wait_for_pending_msg(&[ - Msg::PendingAction(PendingActionMsg::CloseReplacePopups), - Msg::PendingAction(PendingActionMsg::TransferPendingFile), - ]) == Msg::PendingAction(PendingActionMsg::TransferPendingFile) - { - trace!("User wants to replace files"); + /// Get files to replace + fn get_files_to_replace(&mut self, files: Vec<(File, PathBuf)>) -> GetFileToReplaceResult { + // keep only files the user want to replace + let mut files_to_replace = vec![]; + let mut all_opts = AllOpts::Unset; + for (file, p) in files { + // Check for all opts + match all_opts { + AllOpts::ReplaceAll => { + trace!( + "User wants to replace all files, including file {}", + file.name() + ); + files_to_replace.push((file, p)); + continue; + } + AllOpts::SkipAll => { + trace!( + "User wants to skip all files, including file {}", + file.name() + ); + continue; + } + AllOpts::Unset => {} + } + + let file_name = file.name(); + self.mount_radio_replace(&file_name); + + // Wait for answer + match self.wait_for_pending_msg(&[ + Msg::PendingAction(PendingActionMsg::ReplaceCancel), + Msg::PendingAction(PendingActionMsg::ReplaceOverwrite), + Msg::PendingAction(PendingActionMsg::ReplaceSkip), + Msg::PendingAction(PendingActionMsg::ReplaceSkipAll), + Msg::PendingAction(PendingActionMsg::ReplaceOverwriteAll), + ]) { + Msg::PendingAction(PendingActionMsg::ReplaceCancel) => { + trace!("The user cancelled the replace operation"); + self.umount_radio_replace(); + return GetFileToReplaceResult::Cancel; + } + Msg::PendingAction(PendingActionMsg::ReplaceOverwrite) => { + trace!("User wants to replace file {}", file_name); + files_to_replace.push((file, p)); + } + Msg::PendingAction(PendingActionMsg::ReplaceOverwriteAll) => { + trace!( + "User wants to replace all files from now on, including file {}", + file_name + ); + files_to_replace.push((file, p)); + all_opts = AllOpts::ReplaceAll; + } + Msg::PendingAction(PendingActionMsg::ReplaceSkip) => { + trace!("The user skipped file {}", file_name); + } + Msg::PendingAction(PendingActionMsg::ReplaceSkipAll) => { + trace!( + "The user skipped all files from now on, including file {}", + file_name + ); + all_opts = AllOpts::SkipAll; + } + _ => {} + } self.umount_radio_replace(); - true - } else { - trace!("The user doesn't want replace file"); - self.umount_radio_replace(); - false } + + GetFileToReplaceResult::Replace(files_to_replace) } /// Get file to check for path @@ -224,4 +289,40 @@ impl FileTransferActivity { p.push(e.name()); p } + + /// Get the files to transfer with overwrites. + /// + /// Existing and unexisting files are splitted, and only existing files are prompted for replacement. + pub(crate) fn get_files_to_transfer_with_overwrites( + &mut self, + files: Vec<(File, PathBuf)>, + file_exists: CheckFileExists, + ) -> TransferFilesWithOverwritesResult { + if !self.config().get_prompt_on_file_replace() { + return TransferFilesWithOverwritesResult::FilesToTransfer(files); + } + + // unzip between existing and non-existing files + let (existing_files, new_files): (Vec<_>, Vec<_>) = + files.into_iter().partition(|(x, dest_path)| { + let p = Self::file_to_check_many(x, dest_path); + match file_exists { + CheckFileExists::Remote => self.remote_file_exists(p.as_path()), + CheckFileExists::HostBridge => self.host_bridge_file_exists(p.as_path()), + } + }); + + // filter only files to replace + let existing_files = match self.get_files_to_replace(existing_files) { + GetFileToReplaceResult::Replace(files) => files, + GetFileToReplaceResult::Cancel => { + return TransferFilesWithOverwritesResult::Cancel; + } + }; + + // merge back + TransferFilesWithOverwritesResult::FilesToTransfer( + existing_files.into_iter().chain(new_files).collect(), + ) + } } diff --git a/src/ui/activities/filetransfer/components/mod.rs b/src/ui/activities/filetransfer/components/mod.rs index d47a477..4f20a1e 100644 --- a/src/ui/activities/filetransfer/components/mod.rs +++ b/src/ui/activities/filetransfer/components/mod.rs @@ -21,9 +21,8 @@ pub use popups::{ ATTR_FILES, ChmodPopup, CopyPopup, DeletePopup, DisconnectPopup, ErrorPopup, FatalPopup, FileInfoPopup, FilterPopup, GotoPopup, KeybindingsPopup, MkdirPopup, NewfilePopup, OpenWithPopup, ProgressBarFull, ProgressBarPartial, QuitPopup, RenamePopup, ReplacePopup, - ReplacingFilesListPopup, SaveAsPopup, SortingPopup, StatusBarLocal, StatusBarRemote, - SymlinkPopup, SyncBrowsingMkdirPopup, WaitPopup, WalkdirWaitPopup, WatchedPathsList, - WatcherPopup, + SaveAsPopup, SortingPopup, StatusBarLocal, StatusBarRemote, SymlinkPopup, + SyncBrowsingMkdirPopup, WaitPopup, WalkdirWaitPopup, WatchedPathsList, WatcherPopup, }; pub use transfer::{ExplorerFind, ExplorerFuzzy, ExplorerLocal, ExplorerRemote}; diff --git a/src/ui/activities/filetransfer/components/popups.rs b/src/ui/activities/filetransfer/components/popups.rs index 3b6b656..efd6a61 100644 --- a/src/ui/activities/filetransfer/components/popups.rs +++ b/src/ui/activities/filetransfer/components/popups.rs @@ -1196,7 +1196,7 @@ impl ReplacePopup { .modifiers(BorderType::Rounded), ) .foreground(color) - .choices(["Yes", "No"]) + .choices(["Replace", "Skip", "Replace All", "Skip All", "Cancel"]) .title(text, Alignment::Center), } } @@ -1205,9 +1205,6 @@ impl ReplacePopup { impl Component for ReplacePopup { fn on(&mut self, ev: Event) -> Option { match ev { - Event::Keyboard(KeyEvent { code: Key::Tab, .. }) => { - Some(Msg::Ui(UiMsg::ReplacePopupTabbed)) - } Event::Keyboard(KeyEvent { code: Key::Left, .. }) => { @@ -1221,102 +1218,36 @@ impl Component for ReplacePopup { Some(Msg::None) } Event::Keyboard(KeyEvent { code: Key::Esc, .. }) => { - Some(Msg::PendingAction(PendingActionMsg::CloseReplacePopups)) + Some(Msg::PendingAction(PendingActionMsg::ReplaceCancel)) } Event::Keyboard(KeyEvent { code: Key::Char('y'), modifiers: KeyModifiers::NONE, - }) => Some(Msg::PendingAction(PendingActionMsg::TransferPendingFile)), + }) => Some(Msg::PendingAction(PendingActionMsg::ReplaceOverwrite)), Event::Keyboard(KeyEvent { code: Key::Char('n'), modifiers: KeyModifiers::NONE, - }) => Some(Msg::PendingAction(PendingActionMsg::CloseReplacePopups)), + }) => Some(Msg::PendingAction(PendingActionMsg::ReplaceSkip)), Event::Keyboard(KeyEvent { code: Key::Enter, .. - }) => { - if matches!( - self.perform(Cmd::Submit), - CmdResult::Submit(State::One(StateValue::Usize(0))) - ) { - Some(Msg::PendingAction(PendingActionMsg::TransferPendingFile)) - } else { - Some(Msg::PendingAction(PendingActionMsg::CloseReplacePopups)) + }) => match self.perform(Cmd::Submit) { + CmdResult::Submit(State::One(StateValue::Usize(0))) => { + Some(Msg::PendingAction(PendingActionMsg::ReplaceOverwrite)) } - } - _ => None, - } - } -} - -#[derive(MockComponent)] -pub struct ReplacingFilesListPopup { - component: List, -} - -impl ReplacingFilesListPopup { - pub fn new(files: &[String], color: Color) -> Self { - Self { - component: List::default() - .borders( - Borders::default() - .color(color) - .modifiers(BorderType::Rounded), - ) - .scroll(true) - .step(4) - .highlighted_color(color) - .highlighted_str("➤ ") - .title( - "The following files are going to be replaced", - Alignment::Center, - ) - .rows(files.iter().map(|x| vec![TextSpan::from(x)]).collect()), - } - } -} - -impl Component for ReplacingFilesListPopup { - fn on(&mut self, ev: Event) -> Option { - match ev { - Event::Keyboard(KeyEvent { code: Key::Esc, .. }) => { - Some(Msg::PendingAction(PendingActionMsg::CloseReplacePopups)) - } - Event::Keyboard(KeyEvent { code: Key::Tab, .. }) => { - Some(Msg::Ui(UiMsg::ReplacePopupTabbed)) - } - Event::Keyboard(KeyEvent { - code: Key::Down, .. - }) => { - self.perform(Cmd::Move(Direction::Down)); - Some(Msg::None) - } - Event::Keyboard(KeyEvent { code: Key::Up, .. }) => { - self.perform(Cmd::Move(Direction::Up)); - Some(Msg::None) - } - Event::Keyboard(KeyEvent { - code: Key::PageDown, - .. - }) => { - self.perform(Cmd::Scroll(Direction::Down)); - Some(Msg::None) - } - Event::Keyboard(KeyEvent { - code: Key::PageUp, .. - }) => { - self.perform(Cmd::Scroll(Direction::Up)); - Some(Msg::None) - } - Event::Keyboard(KeyEvent { - code: Key::Home, .. - }) => { - self.perform(Cmd::GoTo(Position::Begin)); - Some(Msg::None) - } - Event::Keyboard(KeyEvent { code: Key::End, .. }) => { - self.perform(Cmd::GoTo(Position::End)); - Some(Msg::None) - } + CmdResult::Submit(State::One(StateValue::Usize(1))) => { + Some(Msg::PendingAction(PendingActionMsg::ReplaceSkip)) + } + CmdResult::Submit(State::One(StateValue::Usize(2))) => { + Some(Msg::PendingAction(PendingActionMsg::ReplaceOverwriteAll)) + } + CmdResult::Submit(State::One(StateValue::Usize(3))) => { + Some(Msg::PendingAction(PendingActionMsg::ReplaceSkipAll)) + } + CmdResult::Submit(State::One(StateValue::Usize(4))) => { + Some(Msg::PendingAction(PendingActionMsg::ReplaceCancel)) + } + _ => Some(Msg::None), + }, _ => None, } } diff --git a/src/ui/activities/filetransfer/mod.rs b/src/ui/activities/filetransfer/mod.rs index 5416411..a3ba76d 100644 --- a/src/ui/activities/filetransfer/mod.rs +++ b/src/ui/activities/filetransfer/mod.rs @@ -72,7 +72,6 @@ enum Id { QuitPopup, RenamePopup, ReplacePopup, - ReplacingFilesListPopup, SaveAsPopup, SortingPopup, StatusBarHostBridge, @@ -98,10 +97,14 @@ enum Msg { #[derive(Debug, PartialEq)] enum PendingActionMsg { - CloseReplacePopups, CloseSyncBrowsingMkdirPopup, MakePendingDirectory, - TransferPendingFile, + /// Replace file popup + ReplaceCancel, + ReplaceOverwrite, + ReplaceOverwriteAll, + ReplaceSkip, + ReplaceSkipAll, } #[derive(Debug, PartialEq)] @@ -171,8 +174,8 @@ enum UiMsg { MarkAll, /// Clear all marks MarkClear, + Quit, - ReplacePopupTabbed, ShowChmodPopup, ShowCopyPopup, ShowDeletePopup, diff --git a/src/ui/activities/filetransfer/update.rs b/src/ui/activities/filetransfer/update.rs index 418e780..99f897f 100644 --- a/src/ui/activities/filetransfer/update.rs +++ b/src/ui/activities/filetransfer/update.rs @@ -5,7 +5,6 @@ // locals // externals use remotefs::fs::File; -use tuirealm::props::{AttrValue, Attribute}; use tuirealm::{State, StateValue, Update}; use super::actions::SelectedFile; @@ -504,15 +503,6 @@ impl FileTransferActivity { self.disconnect_and_quit(); self.umount_quit(); } - UiMsg::ReplacePopupTabbed => { - if let Ok(Some(AttrValue::Flag(true))) = - self.app.query(&Id::ReplacePopup, Attribute::Focus) - { - assert!(self.app.active(&Id::ReplacingFilesListPopup).is_ok()); - } else { - assert!(self.app.active(&Id::ReplacePopup).is_ok()); - } - } UiMsg::ShowChmodPopup => { let selected_file = match self.browser.tab() { #[cfg(posix)] diff --git a/src/ui/activities/filetransfer/view.rs b/src/ui/activities/filetransfer/view.rs index 721ee00..0842285 100644 --- a/src/ui/activities/filetransfer/view.rs +++ b/src/ui/activities/filetransfer/view.rs @@ -269,29 +269,10 @@ impl FileTransferActivity { // make popup self.app.view(&Id::DeletePopup, f, popup); } else if self.app.mounted(&Id::ReplacePopup) { - // NOTE: handle extended / normal modes - if self.is_radio_replace_extended() { - let popup = Popup(Size::Percentage(50), Size::Percentage(50)).draw_in(f.area()); - f.render_widget(Clear, popup); - let popup_chunks = Layout::default() - .direction(Direction::Vertical) - .constraints( - [ - Constraint::Percentage(85), // List - Constraint::Percentage(15), // Radio - ] - .as_ref(), - ) - .split(popup); - self.app - .view(&Id::ReplacingFilesListPopup, f, popup_chunks[0]); - self.app.view(&Id::ReplacePopup, f, popup_chunks[1]); - } else { - let popup = Popup(Size::Percentage(50), Size::Unit(3)).draw_in(f.area()); - f.render_widget(Clear, popup); - // make popup - self.app.view(&Id::ReplacePopup, f, popup); - } + let popup = Popup(Size::Percentage(50), Size::Unit(3)).draw_in(f.area()); + f.render_widget(Clear, popup); + // make popup + self.app.view(&Id::ReplacePopup, f, popup); } else if self.app.mounted(&Id::DisconnectPopup) { let popup = Popup(Size::Percentage(30), Size::Unit(3)).draw_in(f.area()); f.render_widget(Clear, popup); @@ -944,37 +925,8 @@ impl FileTransferActivity { assert!(self.app.active(&Id::ReplacePopup).is_ok()); } - pub(super) fn mount_radio_replace_many(&mut self, files: &[String]) { - let warn_color = self.theme().misc_warn_dialog; - assert!( - self.app - .remount( - Id::ReplacingFilesListPopup, - Box::new(components::ReplacingFilesListPopup::new(files, warn_color)), - vec![], - ) - .is_ok() - ); - assert!( - self.app - .remount( - Id::ReplacePopup, - Box::new(components::ReplacePopup::new(None, warn_color)), - vec![], - ) - .is_ok() - ); - assert!(self.app.active(&Id::ReplacePopup).is_ok()); - } - - /// Returns whether radio replace is in "extended" mode (for many files) - pub(super) fn is_radio_replace_extended(&self) -> bool { - self.app.mounted(&Id::ReplacingFilesListPopup) - } - pub(super) fn umount_radio_replace(&mut self) { let _ = self.app.umount(&Id::ReplacePopup); - let _ = self.app.umount(&Id::ReplacingFilesListPopup); // NOTE: replace anyway } pub(super) fn mount_file_info(&mut self, file: &File) {