From 34f1503391c726b8a652d5ba39cdb1f6ad0c5a32 Mon Sep 17 00:00:00 2001 From: Christian Visintin Date: Sat, 28 Feb 2026 16:41:09 +0000 Subject: [PATCH] fix: replace recursive byte-counting with entry-based transfer progress (#395) * fix: replace recursive byte-counting with entry-based transfer progress Replace the expensive recursive `get_total_transfer_size` pre-calculation with a lightweight entry-based counter (`TransferProgress`) for the overall progress bar. This avoids deep `list_dir` traversals before transfers begin, which could cause FTP idle-timeout disconnections on large directory trees. The per-file byte-level progress bar (`ProgressStates`) remains unchanged. Bytes are still tracked via `TransferStates::add_bytes` for notification threshold logic. Closes #384 --- .../activities/filetransfer/lib/transfer.rs | 110 ++++++++++--- .../filetransfer/session/navigation.rs | 37 ----- .../filetransfer/session/transfer.rs | 148 ++++++++++-------- 3 files changed, 172 insertions(+), 123 deletions(-) diff --git a/src/ui/activities/filetransfer/lib/transfer.rs b/src/ui/activities/filetransfer/lib/transfer.rs index c14283a..533d9ac 100644 --- a/src/ui/activities/filetransfer/lib/transfer.rs +++ b/src/ui/activities/filetransfer/lib/transfer.rs @@ -9,14 +9,54 @@ use bytesize::ByteSize; // -- States and progress -/// TransferStates contains the states related to the transfer process -pub struct TransferStates { - aborted: bool, // Describes whether the transfer process has been aborted - pub full: ProgressStates, // full transfer states - pub partial: ProgressStates, // Partial transfer states +/// Tracks overall transfer progress as an entry counter (e.g. "3/12"). +/// +/// Unlike the byte-based `ProgressStates`, this counts top-level entries +/// rather than bytes, avoiding the expensive recursive size pre-calculation +/// that can cause FTP idle-timeout disconnections on large directory trees. +#[derive(Default)] +pub struct TransferProgress { + completed: usize, + total: usize, } -/// Progress states describes the states for the progress of a single transfer part +impl fmt::Display for TransferProgress { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}/{}", self.completed, self.total) + } +} + +impl TransferProgress { + /// Initialize progress with the total number of entries to transfer. + pub fn init(&mut self, total: usize) { + self.completed = 0; + self.total = total; + } + + /// Mark one entry as completed. + pub fn increment(&mut self) { + self.completed += 1; + } + + /// Calculate progress in a range between 0.0 and 1.0. + pub fn calc_progress(&self) -> f64 { + if self.total == 0 { + return 0.0; + } + let prog = self.completed as f64 / self.total as f64; + prog.min(1.0) + } +} + +/// Contains the states related to the transfer process. +pub struct TransferStates { + aborted: bool, + pub full: TransferProgress, + pub partial: ProgressStates, + bytes_transferred: usize, +} + +/// Describes the states for the progress of a single file transfer. pub struct ProgressStates { started: Instant, total: usize, @@ -30,33 +70,40 @@ impl Default for TransferStates { } impl TransferStates { - /// Instantiates a new transfer states + /// Instantiates a new transfer states. pub fn new() -> TransferStates { TransferStates { aborted: false, - full: ProgressStates::default(), + full: TransferProgress::default(), partial: ProgressStates::default(), + bytes_transferred: 0, } } - /// Re-intiialize transfer states + /// Re-initialize transfer states. pub fn reset(&mut self) { self.aborted = false; + self.bytes_transferred = 0; } - /// Set aborted to true + /// Set aborted to true. pub fn abort(&mut self) { self.aborted = true; } - /// Returns whether transfer has been aborted + /// Returns whether transfer has been aborted. pub fn aborted(&self) -> bool { self.aborted } - /// Returns the size of the entire transfer + /// Returns total bytes transferred (used for notification threshold). pub fn full_size(&self) -> usize { - self.full.total + self.bytes_transferred + } + + /// Accumulate transferred bytes for notification threshold tracking. + pub fn add_bytes(&mut self, delta: usize) { + self.bytes_transferred += delta; } } @@ -226,23 +273,46 @@ mod test { assert_eq!(states.calc_progress(), 0.0); } + #[test] + fn test_ui_activities_filetransfer_lib_transfer_progress() { + let mut progress = TransferProgress::default(); + assert_eq!(progress.calc_progress(), 0.0); + assert_eq!(progress.to_string(), "0/0"); + // Init with 4 entries + progress.init(4); + assert_eq!(progress.calc_progress(), 0.0); + assert_eq!(progress.to_string(), "0/4"); + // Increment + progress.increment(); + assert_eq!(progress.calc_progress(), 0.25); + assert_eq!(progress.to_string(), "1/4"); + // Complete all + progress.increment(); + progress.increment(); + progress.increment(); + assert_eq!(progress.calc_progress(), 1.0); + assert_eq!(progress.to_string(), "4/4"); + } + #[test] fn test_ui_activities_filetransfer_lib_transfer_states() { let mut states: TransferStates = TransferStates::default(); - assert_eq!(states.aborted, false); - assert_eq!(states.full.total, 0); - assert_eq!(states.full.written, 0); - assert!(states.full.started.elapsed().as_secs() < 5); + assert!(!states.aborted()); assert_eq!(states.partial.total, 0); assert_eq!(states.partial.written, 0); assert!(states.partial.started.elapsed().as_secs() < 5); // Aborted states.abort(); - assert_eq!(states.aborted(), true); + assert!(states.aborted()); states.reset(); - assert_eq!(states.aborted(), false); - states.full.total = 1024; + assert!(!states.aborted()); + // Bytes tracking + states.add_bytes(512); + states.add_bytes(512); assert_eq!(states.full_size(), 1024); + // Reset clears bytes + states.reset(); + assert_eq!(states.full_size(), 0); } #[test] diff --git a/src/ui/activities/filetransfer/session/navigation.rs b/src/ui/activities/filetransfer/session/navigation.rs index e633d67..b5e5461 100644 --- a/src/ui/activities/filetransfer/session/navigation.rs +++ b/src/ui/activities/filetransfer/session/navigation.rs @@ -211,43 +211,6 @@ impl FileTransferActivity { } } - // -- transfer sizes -- - - /// Get total size of transfer for the specified side. - pub(super) fn get_total_transfer_size(&mut self, entry: &File, local: bool) -> usize { - self.mount_blocking_wait("Calculating transfer size…"); - - let sz = if entry.is_dir() { - let list_result = if local { - self.browser.local_pane_mut().fs.list_dir(entry.path()) - } else { - self.browser.remote_pane_mut().fs.list_dir(entry.path()) - }; - match list_result { - Ok(files) => files - .iter() - .map(|x| self.get_total_transfer_size(x, local)) - .sum(), - Err(err) => { - self.log( - LogLevel::Error, - format!( - "Could not list directory {}: {}", - entry.path().display(), - err - ), - ); - 0 - } - } - } else { - entry.metadata.size as usize - }; - - self.umount_wait(); - sz - } - // -- file changed -- /// Check whether a file has changed on the specified side, compared to the given metadata. diff --git a/src/ui/activities/filetransfer/session/transfer.rs b/src/ui/activities/filetransfer/session/transfer.rs index 83354aa..9387143 100644 --- a/src/ui/activities/filetransfer/session/transfer.rs +++ b/src/ui/activities/filetransfer/session/transfer.rs @@ -87,9 +87,8 @@ impl FileTransferActivity { ) -> Result<(), String> { // Reset states self.transfer.reset(); - // Calculate total size of transfer - let total_transfer_size: usize = file.metadata.size as usize; - self.transfer.full.init(total_transfer_size); + // Single file = 1 entry + self.transfer.full.init(1); // Mount progress bar self.mount_progress_bar(format!("Uploading {}…", file.path.display())); // Get remote path @@ -102,54 +101,56 @@ impl FileTransferActivity { remote_path.push(remote_file_name); // Send let result = self.filetransfer_send_one(file, remote_path.as_path(), file_name); + if result.is_ok() { + self.transfer.full.increment(); + } // Umount progress bar self.umount_progress_bar(); // Return result result.map_err(|x| x.to_string()) } - /// Send a `TransferPayload` of type `Any` + /// Send a `TransferPayload` of type `Any`. fn filetransfer_send_any( &mut self, entry: &File, curr_remote_path: &Path, dst_name: Option, ) -> Result<(), String> { - // Reset states self.transfer.reset(); - // Calculate total size of transfer - let total_transfer_size: usize = self.get_total_transfer_size(entry, true); - self.transfer.full.init(total_transfer_size); - // Mount progress bar + if !entry.is_dir() { + self.transfer.full.init(1); + } self.mount_progress_bar(format!("Uploading {}…", entry.path().display())); - // Send recurse - let result = self.filetransfer_send_recurse(entry, curr_remote_path, dst_name); - // Umount progress bar + let result = self.filetransfer_send_recurse(entry, curr_remote_path, dst_name, true); self.umount_progress_bar(); result } - /// Send transfer queue entries to remote + /// Send transfer queue entries to remote. fn filetransfer_send_transfer_queue( &mut self, entries: &[(File, PathBuf)], ) -> Result<(), String> { // Reset states self.transfer.reset(); - // Calculate total size of transfer - let total_transfer_size: usize = entries - .iter() - .map(|(x, _)| self.get_total_transfer_size(x, true)) - .sum(); - self.transfer.full.init(total_transfer_size); + // Total = number of queue entries + self.transfer.full.init(entries.len()); // Mount progress bar self.mount_progress_bar(format!("Uploading {} entries…", entries.len())); - // Send recurse - let result = entries - .iter() - .map(|(x, remote)| self.filetransfer_send_recurse(x, remote, None)) - .find(|x| x.is_err()) - .unwrap_or(Ok(())); + // Send each entry + let mut result = Ok(()); + for (entry, remote) in entries { + if self.transfer.aborted() { + break; + } + let r = self.filetransfer_send_recurse(entry, remote, None, false); + if r.is_err() { + result = r; + break; + } + self.transfer.full.increment(); + } // Umount progress bar self.umount_progress_bar(); result @@ -160,6 +161,7 @@ impl FileTransferActivity { entry: &File, curr_remote_path: &Path, dst_name: Option, + track_progress: bool, ) -> Result<(), String> { // Write popup let file_name = entry.name(); @@ -200,14 +202,17 @@ impl FileTransferActivity { // Get files in dir match self.browser.local_pane_mut().fs.list_dir(entry.path()) { Ok(entries) => { - // Iterate over files + if track_progress { + self.transfer.full.init(entries.len()); + } for entry in entries.iter() { - // If aborted; break if self.transfer.aborted() { break; } - // Send entry; name is always None after first call - self.filetransfer_send_recurse(entry, remote_path.as_path(), None)? + self.filetransfer_send_recurse(entry, remote_path.as_path(), None, false)?; + if track_progress { + self.transfer.full.increment(); + } } Ok(()) } @@ -262,7 +267,12 @@ impl FileTransferActivity { } Err(err.to_string()) } - Ok(_) => Ok(()), + Ok(_) => { + if track_progress { + self.transfer.full.increment(); + } + Ok(()) + } } }; // Scan dir on remote @@ -302,7 +312,7 @@ impl FileTransferActivity { host_bridge.path().display() ), ); - self.transfer.full.update_progress(metadata.size as usize); + self.transfer.add_bytes(metadata.size as usize); return Ok(()); } // Upload file @@ -393,7 +403,7 @@ impl FileTransferActivity { }; // Increase progress self.transfer.partial.update_progress(delta); - self.transfer.full.update_progress(delta); + self.transfer.add_bytes(delta); // Draw only if a significant progress has been made (performance improvement) if last_progress_val < self.transfer.partial.calc_progress() - 0.01 { // Draw @@ -467,23 +477,19 @@ impl FileTransferActivity { /// Recv fs entry from remote. /// If dst_name is Some, entry will be saved with a different name. - /// If entry is a directory, this applies to directory only + /// If entry is a directory, this applies to directory only. fn filetransfer_recv_any( &mut self, entry: &File, host_path: &Path, dst_name: Option, ) -> Result<(), String> { - // Reset states self.transfer.reset(); - // Calculate total transfer size - let total_transfer_size: usize = self.get_total_transfer_size(entry, false); - self.transfer.full.init(total_transfer_size); - // Mount progress bar + if !entry.is_dir() { + self.transfer.full.init(1); + } self.mount_progress_bar(format!("Downloading {}…", entry.path().display())); - // Receive - let result = self.filetransfer_recv_recurse(entry, host_path, dst_name); - // Umount progress bar + let result = self.filetransfer_recv_recurse(entry, host_path, dst_name, true); self.umount_progress_bar(); result } @@ -496,40 +502,45 @@ impl FileTransferActivity { ) -> Result<(), String> { // Reset states self.transfer.reset(); - // Calculate total transfer size - let total_transfer_size: usize = entry.metadata.size as usize; - self.transfer.full.init(total_transfer_size); + // Single file = 1 entry + self.transfer.full.init(1); // Mount progress bar self.mount_progress_bar(format!("Downloading {}…", entry.path.display())); // Receive let result = self.filetransfer_recv_one(host_bridge_path, entry, entry.name()); + if result.is_ok() { + self.transfer.full.increment(); + } // Umount progress bar self.umount_progress_bar(); // Return result result.map_err(|x| x.to_string()) } - /// Receive transfer queue from remote + /// Receive transfer queue from remote. fn filetransfer_recv_transfer_queue( &mut self, entries: &[(File, PathBuf)], ) -> Result<(), String> { // Reset states self.transfer.reset(); - // Calculate total size of transfer - let total_transfer_size: usize = entries - .iter() - .map(|(x, _)| self.get_total_transfer_size(x, false)) - .sum(); - self.transfer.full.init(total_transfer_size); + // Total = number of queue entries + self.transfer.full.init(entries.len()); // Mount progress bar self.mount_progress_bar(format!("Downloading {} entries…", entries.len())); - // Send recurse - let result = entries - .iter() - .map(|(x, path)| self.filetransfer_recv_recurse(x, path, None)) - .find(|x| x.is_err()) - .unwrap_or(Ok(())); + // Receive each entry + let mut result = Ok(()); + for (entry, path) in entries { + if self.transfer.aborted() { + break; + } + let r = self.filetransfer_recv_recurse(entry, path, None, false); + if r.is_err() { + result = r; + break; + } + self.transfer.full.increment(); + } // Umount progress bar self.umount_progress_bar(); result @@ -540,6 +551,7 @@ impl FileTransferActivity { entry: &File, host_bridge_path: &Path, dst_name: Option, + track_progress: bool, ) -> Result<(), String> { // Write popup let file_name = entry.name(); @@ -583,19 +595,22 @@ impl FileTransferActivity { // Get files in dir from remote match self.browser.remote_pane_mut().fs.list_dir(entry.path()) { Ok(entries) => { - // Iterate over files + if track_progress { + self.transfer.full.init(entries.len()); + } for entry in entries.iter() { - // If transfer has been aborted; break if self.transfer.aborted() { break; } - // Receive entry; name is always None after first call - // Local path becomes host_bridge_dir_path self.filetransfer_recv_recurse( entry, host_bridge_dir_path.as_path(), None, - )? + false, + )?; + if track_progress { + self.transfer.full.increment(); + } } Ok(()) } @@ -672,6 +687,9 @@ impl FileTransferActivity { } Err(err.to_string()) } else { + if track_progress { + self.transfer.full.increment(); + } Ok(()) } }; @@ -704,9 +722,7 @@ impl FileTransferActivity { remote.path().display() ), ); - self.transfer - .full - .update_progress(remote.metadata().size as usize); + self.transfer.add_bytes(remote.metadata().size as usize); return Ok(()); } @@ -786,7 +802,7 @@ impl FileTransferActivity { }; // Set progress self.transfer.partial.update_progress(delta); - self.transfer.full.update_progress(delta); + self.transfer.add_bytes(delta); // Draw only if a significant progress has been made (performance improvement) if last_progress_val < self.transfer.partial.calc_progress() - 0.01 { // Draw