mirror of
https://github.com/veeso/termscp.git
synced 2026-09-27 06:21:22 -07:00
refactor: FileTransferActivity pane-agnostic dispatch (#386)
Comprehensive design for incremental refactoring of the 13k-line FileTransferActivity god-struct using a unified Pane abstraction. Detailed step-by-step plan covering 6 phases: split monoliths, error handling, Pane struct, action dedup, session split, view reorg. Extract 26 popup components from the monolithic 1,868-line popups.rs into 20 individual files under popups/. Each file contains one or two related components with their own imports. The popups.rs module file now contains only module declarations and re-exports. Replace 8 panic!() calls with error!() logging and early returns/fallthrough. These panics documented invariants (e.g. "this tab can't do X") but would crash the app if somehow triggered. Error logging is safer and more resilient. Replace raw FileExplorer fields in Browser with Pane structs that bundle the explorer and connected state. Move host_bridge_connected and remote_connected from FileTransferActivity into the panes. Add navigation API (fs_pane, opposite_pane, is_find_tab) for future unification tasks. Rename private get_selected_file to get_selected_file_by_id and add three new unified methods (get_selected_entries, get_selected_file, is_selected_one) that dispatch based on self.browser.tab(). Old per-tab methods are kept for now until their callers are migrated in subsequent tasks. Collapse _local_/_remote_ action method pairs (mkdir, delete, symlink, chmod, rename, copy) into unified methods that branch internally on is_local_tab(). This halves the number of action methods and simplifies the update.rs dispatch logic. Also unifies ShowFileInfoPopup and ShowChmodPopup dispatching to use get_selected_entries(). Move `host_bridge` and `client` filesystem fields from FileTransferActivity into the Pane struct, enabling tab-agnostic dispatch via `fs_pane()`/ `fs_pane_mut()`. This eliminates most `is_local_tab()` branching across 15+ action files. Key changes: - Add `fs: Box<dyn HostBridge>` to Pane, remove from FileTransferActivity - Replace per-side method pairs with unified pane-dispatched methods - Unify navigation (changedir, reload, scan, file_exists, has_file_changed) - Replace 147-line popup if/else chain with data-driven priority table - Replace assert!/panic!/unreachable! with proper error handling - Fix typo "filetransfer_activiy" across ~29 files - Add unit tests for Pane Net result: -473 lines, single code path for most file operations.
This commit is contained in:
Christian Visintin
@@ -1,11 +1,10 @@
|
||||
//! ## FileTransferActivity
|
||||
//!
|
||||
//! `filetransfer_activiy` is the module which implements the Filetransfer activity, which is the main activity afterall
|
||||
//! `filetransfer_activity` is the module which implements the Filetransfer activity, which is the main activity afterall
|
||||
|
||||
// locals
|
||||
use std::path::PathBuf;
|
||||
|
||||
use super::super::browser::FileExplorerTab;
|
||||
use super::{File, FileTransferActivity, LogLevel, SelectedFile, TransferOpts, TransferPayload};
|
||||
|
||||
impl FileTransferActivity {
|
||||
@@ -22,33 +21,19 @@ impl FileTransferActivity {
|
||||
Some(p) => p.to_path_buf(),
|
||||
}
|
||||
};
|
||||
// Change directory
|
||||
match self.browser.tab() {
|
||||
FileExplorerTab::FindHostBridge | FileExplorerTab::HostBridge => {
|
||||
self.host_bridge_changedir(path.as_path(), true)
|
||||
}
|
||||
FileExplorerTab::FindRemote | FileExplorerTab::Remote => {
|
||||
self.remote_changedir(path.as_path(), true)
|
||||
}
|
||||
}
|
||||
// Change directory on the active tab's pane
|
||||
self.pane_changedir(path.as_path(), true);
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn action_find_transfer(&mut self, opts: TransferOpts) {
|
||||
let wrkdir: PathBuf = match self.browser.tab() {
|
||||
FileExplorerTab::FindHostBridge | FileExplorerTab::HostBridge => {
|
||||
self.remote().wrkdir.clone()
|
||||
}
|
||||
FileExplorerTab::FindRemote | FileExplorerTab::Remote => {
|
||||
self.host_bridge().wrkdir.clone()
|
||||
}
|
||||
};
|
||||
let wrkdir: PathBuf = self.browser.opposite_pane().explorer.wrkdir.clone();
|
||||
match self.get_found_selected_entries() {
|
||||
SelectedFile::One(entry) => match self.browser.tab() {
|
||||
FileExplorerTab::FindHostBridge | FileExplorerTab::HostBridge => {
|
||||
SelectedFile::One(entry) => {
|
||||
if self.is_local_tab() {
|
||||
let file_to_check = Self::file_to_check(&entry, opts.save_as.as_ref());
|
||||
if self.config().get_prompt_on_file_replace()
|
||||
&& self.remote_file_exists(file_to_check.as_path())
|
||||
&& self.file_exists(file_to_check.as_path(), false)
|
||||
&& !self.should_replace_file(
|
||||
opts.save_as.clone().unwrap_or_else(|| entry.name()),
|
||||
)
|
||||
@@ -66,11 +51,10 @@ impl FileTransferActivity {
|
||||
format!("Could not upload file: {err}"),
|
||||
);
|
||||
}
|
||||
}
|
||||
FileExplorerTab::FindRemote | FileExplorerTab::Remote => {
|
||||
} else {
|
||||
let file_to_check = Self::file_to_check(&entry, opts.save_as.as_ref());
|
||||
if self.config().get_prompt_on_file_replace()
|
||||
&& self.host_bridge_file_exists(file_to_check.as_path())
|
||||
&& self.file_exists(file_to_check.as_path(), true)
|
||||
&& !self.should_replace_file(
|
||||
opts.save_as.clone().unwrap_or_else(|| entry.name()),
|
||||
)
|
||||
@@ -89,7 +73,7 @@ impl FileTransferActivity {
|
||||
);
|
||||
}
|
||||
}
|
||||
},
|
||||
}
|
||||
SelectedFile::Many(entries) => {
|
||||
// In case of selection: save multiple files in wrkdir/input
|
||||
let mut dest_path: PathBuf = wrkdir;
|
||||
@@ -97,58 +81,51 @@ impl FileTransferActivity {
|
||||
dest_path.push(save_as);
|
||||
}
|
||||
// Iter files
|
||||
match self.browser.tab() {
|
||||
FileExplorerTab::FindHostBridge | FileExplorerTab::HostBridge => {
|
||||
let super::save::TransferFilesWithOverwritesResult::FilesToTransfer(
|
||||
entries,
|
||||
) = self.get_files_to_transfer_with_overwrites(
|
||||
if self.is_local_tab() {
|
||||
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(),
|
||||
None,
|
||||
) {
|
||||
{
|
||||
self.log_and_alert(
|
||||
LogLevel::Error,
|
||||
format!("Could not upload file: {err}"),
|
||||
);
|
||||
}
|
||||
}
|
||||
else {
|
||||
debug!("User cancelled file transfer due to overwrites");
|
||||
return;
|
||||
};
|
||||
if let Err(err) = self.filetransfer_send(
|
||||
TransferPayload::TransferQueue(entries),
|
||||
dest_path.as_path(),
|
||||
None,
|
||||
) {
|
||||
self.log_and_alert(
|
||||
LogLevel::Error,
|
||||
format!("Could not upload file: {err}"),
|
||||
);
|
||||
}
|
||||
FileExplorerTab::FindRemote | FileExplorerTab::Remote => {
|
||||
let super::save::TransferFilesWithOverwritesResult::FilesToTransfer(
|
||||
entries,
|
||||
) = self.get_files_to_transfer_with_overwrites(
|
||||
} else {
|
||||
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(),
|
||||
None,
|
||||
) {
|
||||
self.log_and_alert(
|
||||
LogLevel::Error,
|
||||
format!("Could not download file: {err}"),
|
||||
);
|
||||
}
|
||||
else {
|
||||
debug!("User cancelled file transfer due to overwrites");
|
||||
return;
|
||||
};
|
||||
if let Err(err) = self.filetransfer_recv(
|
||||
TransferPayload::TransferQueue(entries),
|
||||
dest_path.as_path(),
|
||||
None,
|
||||
) {
|
||||
self.log_and_alert(
|
||||
LogLevel::Error,
|
||||
format!("Could not download file: {err}"),
|
||||
);
|
||||
}
|
||||
|
||||
// clear selection
|
||||
if let Some(f) = self.found_mut() {
|
||||
f.clear_queue();
|
||||
self.update_find_list();
|
||||
}
|
||||
// clear selection
|
||||
if let Some(f) = self.found_mut() {
|
||||
f.clear_queue();
|
||||
self.update_find_list();
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -159,14 +136,11 @@ impl FileTransferActivity {
|
||||
pub(crate) fn action_find_delete(&mut self) {
|
||||
match self.get_found_selected_entries() {
|
||||
SelectedFile::One(entry) => {
|
||||
// Delete file
|
||||
self.remove_found_file(&entry);
|
||||
self.remove_file(&entry);
|
||||
}
|
||||
SelectedFile::Many(entries) => {
|
||||
// Iter files
|
||||
for (entry, _) in entries.iter() {
|
||||
// Delete file
|
||||
self.remove_found_file(entry);
|
||||
self.remove_file(entry);
|
||||
}
|
||||
|
||||
// clear selection
|
||||
@@ -179,17 +153,6 @@ impl FileTransferActivity {
|
||||
}
|
||||
}
|
||||
|
||||
fn remove_found_file(&mut self, entry: &File) {
|
||||
match self.browser.tab() {
|
||||
FileExplorerTab::FindHostBridge | FileExplorerTab::HostBridge => {
|
||||
self.local_remove_file(entry);
|
||||
}
|
||||
FileExplorerTab::FindRemote | FileExplorerTab::Remote => {
|
||||
self.remote_remove_file(entry);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn action_find_open(&mut self) {
|
||||
match self.get_found_selected_entries() {
|
||||
SelectedFile::One(entry) => {
|
||||
@@ -235,13 +198,6 @@ impl FileTransferActivity {
|
||||
}
|
||||
|
||||
fn open_found_file(&mut self, entry: &File, with: Option<&str>) {
|
||||
match self.browser.tab() {
|
||||
FileExplorerTab::FindHostBridge | FileExplorerTab::HostBridge => {
|
||||
self.action_open_local_file(entry, with);
|
||||
}
|
||||
FileExplorerTab::FindRemote | FileExplorerTab::Remote => {
|
||||
self.action_open_remote_file(entry, with);
|
||||
}
|
||||
}
|
||||
self.open_file(entry, with);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user