Unit tests and clean up for dupe file feature

Cargo.lock

@@ -5222,10 +5222,22 @@ dependencies = [
  "pin-project-lite",
  "signal-hook-registry",
  "socket2 0.5.7",
+ "tokio-macros",
  "tracing",
  "windows-sys 0.48.0",
 ]
 
+[[package]]
+name = "tokio-macros"
+version = "2.2.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5b8a1e28f2deaa14e508979454cb3a223b10b938b45af148bc0986de36f1923b"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn 2.0.60",
+]
+
 [[package]]
 name = "toml"
 version = "0.5.11"

Cargo.toml

@@ -73,6 +73,7 @@ fork = "0.1"
 fastrand = "2"
 tempfile = "3"
 test-log = "0.2"
+tokio = { version = "1", features = ["rt", "macros"] }
 
 # [patch.'https://github.com/pop-os/libcosmic']
 # libcosmic = { path = "../libcosmic" }

i18n/en/cosmic_files.ftl

@@ -58,6 +58,7 @@ operations = Operations
 pending = Pending
 failed = Failed
 complete = Complete
+copy_noun = Copy
 
 ## Open with
 open-with = Open with

src/app.rs

@@ -2383,6 +2383,16 @@ pub(crate) mod test_utils {
         }))
     }
 
+    // Filter `path` for files
+    pub fn filter_files(path: &Path) -> io::Result<impl Iterator<Item = PathBuf>> {
+        Ok(path.read_dir()?.filter_map(|entry| {
+            entry.ok().and_then(|entry| {
+                let path = entry.path();
+                path.is_file().then_some(path)
+            })
+        }))
+    }
+
     /// Boiler plate for Tab tests
     pub fn tab_click_new(
         files: usize,

src/operation.rs

@@ -8,7 +8,7 @@ use std::{
     },
 };
 
-use crate::app::Message;
+use crate::{app::Message, fl};
 
 fn err_str<T: ToString>(err: T) -> String {
     err.to_string()
@@ -71,31 +71,22 @@ impl Operation {
                         .into_iter()
                         .zip(std::iter::repeat(to.as_path()))
                         .map(|(from, to)| {
-                            log::info!("{:?}", from.parent());
                             if matches!(from.parent(), Some(parent) if parent == to) {
                                 // `from`'s parent is equal to `to` which means we're copying to the same
                                 // directory (duplicating files)
                                 let mut to = to.to_owned();
-                                let to = if let Some(full_name) =
+                                // Separate the full file name into its file name plus extension.
-                                    from.file_name().and_then(|name| name.to_str())
+                                // `[Path::file_stem]` returns the full name for dotfiles (e.g.
-                                {
+                                // .someconf is the file name)
-                                    // Separate the full file name into its file name plus extension.
+                                let to = if let (Some(stem), ext) = (
-                                    let (base_name, ext, needs_dot) = if full_name.starts_with('.')
+                                    // FIXME: Replace `[Path::file_stem]` with `[Path::file_prefix]` when stablized to handle .tar.gz et al. better
-                                    {
+                                    from.file_stem().and_then(|name| name.to_str()),
-                                        // `[Path::file_name]` returns the full name for dotfiles (e.g.
+                                    from.extension()
-                                        // .someconf is the file_name)
+                                        .and_then(|ext| ext.to_str())
-                                        (full_name, "", false)
+                                        .unwrap_or_default(),
-                                    } else {
+                                ) {
-                                        // Consider everything beyond the first '.' to be a file
+                                    // '.' needs to be re-added for paths with extensions.
-                                        // extension.
+                                    let dot = ext.is_empty().then_some("").unwrap_or(".");
-                                        full_name
-                                            .split_once('.')
-                                            .map(|(full_name, extension)| {
-                                                (full_name, extension, !extension.is_empty())
-                                            })
-                                            // File without an extension
-                                            .unwrap_or((full_name, "", false))
-                                    };
                                     let mut n = 0u32;
                                     // Loop until a valid `copy n` variant is found
                                     loop {
@@ -107,8 +98,8 @@ impl Operation {
                                         };
 
                                         // Rebuild file name
-                                        let dot = if needs_dot { "." } else { "" };
+                                        let new_name =
-                                        let new_name = format!("{base_name} (Copy {n}){dot}{ext}");
+                                            format!("{stem} ({} {n}){dot}{ext}", fl!("copy_noun"));
                                         to = to.join(new_name);
 
                                         if !matches!(to.try_exists(), Ok(true)) {
@@ -134,6 +125,7 @@ impl Operation {
                 let msg_tx = msg_tx.clone();
                 tokio::task::spawn_blocking(move || -> fs_extra::error::Result<()> {
                     log::info!("Copy {:?} to {:?}", paths, to);
+                    //TODO: set options as desired
                     let dir_options = fs_extra::dir::CopyOptions::default().copy_inside(true);
                     let file_options = fs_extra::file::CopyOptions::default();
                     let copied_bytes = AtomicU64::default();
@@ -154,6 +146,7 @@ impl Operation {
                                 .await;
                         })
                     };
+                    // Files and directory progress are handled separately
                     let file_handler = |progress: fs_extra::file::TransitProcess| {
                         copied_bytes.fetch_add(progress.copied_bytes, atomic::Ordering::Relaxed);
                         handler();
@@ -164,11 +157,7 @@ impl Operation {
                         //TODO: handle exceptions
                         fs_extra::dir::TransitProcessResult::ContinueOrAbort
                     };
-                    //TODO: set options as desired
                     for (from, to) in paths.into_iter().zip(to.into_iter()) {
-                        // This is essentially what `[fs_extra::copy_items_with_progress]` does
-                        // except without handling options (e.g. overwrite). We're currently using
-                        // the defaults anyway.
                         if from.is_dir() {
                             fs_extra::copy_items_with_progress(
                                 &[from],
@@ -324,3 +313,200 @@ impl Operation {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::{fs::File, io, path::PathBuf};
+
+    use cosmic::iced::futures::channel::mpsc;
+    use log::{debug, trace};
+    use test_log::test;
+    use tokio::sync;
+
+    use super::Operation;
+    use crate::{
+        app::{
+            test_utils::{
+                empty_fs, filter_dirs, filter_files, read_dir_sorted, simple_fs, NAME_LEN,
+                NUM_DIRS, NUM_FILES, NUM_HIDDEN, NUM_NESTED,
+            },
+            Message,
+        },
+        fl,
+    };
+
+    // Tests hang with lower values
+    const BUF_SIZE: usize = 8;
+
+    /// Simple wrapper around `[Operation::Copy]`
+    pub async fn operation_copy(paths: Vec<PathBuf>, to: PathBuf) -> Result<(), String> {
+        let id = fastrand::u64(0..u64::MAX);
+        let (tx, mut rx) = mpsc::channel(BUF_SIZE);
+        Operation::Copy {
+            paths: paths.clone(),
+            to: to.clone(),
+        }
+        .perform(id, &sync::Mutex::new(tx).into())
+        .await?;
+
+        loop {
+            match rx.try_next() {
+                Ok(Some(Message::PendingProgress(id, progress))) => {
+                    trace!("({id}) [ {paths:?} => {to:?} ] {progress}% complete)")
+                }
+                Ok(None) => break,
+                Err(e) => panic!("Receiving message from operation should succeed: {e:?}"),
+                _ => unreachable!("Only `Message::PendingProgress` is sent from operation"),
+            }
+        }
+
+        Ok(())
+    }
+
+    #[test(tokio::test)]
+    async fn copy_file_to_same_location() -> io::Result<()> {
+        let fs = simple_fs(NUM_FILES, 0, 1, 0, NAME_LEN)?;
+        let path = fs.path();
+
+        // Get the first file from the first directory
+        let first_dir = filter_dirs(path)?
+            .next()
+            .expect("Should have at least one directory");
+        let first_file = filter_files(&first_dir)?
+            .next()
+            .expect("Should have at least one file");
+
+        // Duplicate that file
+        let base_name = first_file
+            .file_name()
+            .and_then(|name| name.to_str())
+            .expect("File name exists and is valid");
+        debug!(
+            "Duplicating {} in {}",
+            first_file.display(),
+            first_dir.display()
+        );
+        operation_copy(vec![first_file.clone()], first_dir.clone())
+            .await
+            .expect("Copy operation should have succeeded");
+
+        assert!(first_file.exists(), "Original file should still exist");
+        let expected = first_dir.join(format!("{base_name} ({} 1)", fl!("copy_noun")));
+        assert!(expected.exists(), "File should have been duplicated");
+
+        Ok(())
+    }
+
+    #[test(tokio::test)]
+    async fn copy_file_with_extension_to_same_loc() -> io::Result<()> {
+        let fs = empty_fs()?;
+        let path = fs.path();
+
+        let base_name = "foo.txt";
+        let base_path = path.join(base_name);
+        File::create(&base_path)?;
+        debug!("Duplicating {}", base_path.display());
+        operation_copy(vec![base_path.clone()], path.to_owned())
+            .await
+            .expect("Copy operation should have succeeded");
+
+        assert!(base_path.exists(), "Original file should still exist");
+        let expected = path.join(format!("foo ({} 1).txt", fl!("copy_noun")));
+        assert!(expected.exists(), "File should have been duplicated");
+
+        Ok(())
+    }
+
+    #[test(tokio::test)]
+    async fn copy_dir_to_same_location() -> io::Result<()> {
+        let fs = simple_fs(NUM_FILES, 0, NUM_DIRS, NUM_NESTED, NAME_LEN)?;
+        let path = fs.path();
+
+        // First directory path
+        let first_dir = filter_dirs(path)?
+            .next()
+            .expect("Should have at least one directory");
+        let base_name = first_dir
+            .file_name()
+            .and_then(|name| name.to_str())
+            .expect("First directory exists and has a valid name");
+        debug!("Duplicating directory {}", first_dir.display());
+        operation_copy(vec![first_dir.clone()], path.to_owned())
+            .await
+            .expect("Copy operation should have succeeded");
+
+        assert!(first_dir.exists(), "Original directory should still exist");
+        let expected = path.join(format!("{base_name} ({} 1)", fl!("copy_noun")));
+        assert!(expected.exists(), "Directory should have been duplicated");
+
+        Ok(())
+    }
+
+    #[test(tokio::test)]
+    async fn copying_file_multiple_times_to_same_location() -> io::Result<()> {
+        let fs = empty_fs()?;
+        let path = fs.path();
+
+        let base_name = "cosmic";
+        let base_path = path.join(base_name);
+        File::create(&base_path)?;
+
+        for i in 1..5 {
+            debug!("Duplicating {}", base_path.display());
+            operation_copy(vec![base_path.clone()], path.to_owned())
+                .await
+                .expect("Copy operation should have succeeded");
+            assert!(base_path.exists(), "Original file should still exist");
+            assert!(
+                path.join(format!("{base_name} ({} {i})", fl!("copy_noun")))
+                    .exists(),
+                "File should have been duplicated (copy #{i})"
+            );
+        }
+
+        Ok(())
+    }
+
+    #[test(tokio::test)]
+    async fn copy_to_diff_dir_doesnt_dupe_files() -> io::Result<()> {
+        let fs = simple_fs(NUM_FILES, NUM_HIDDEN, NUM_DIRS, NUM_NESTED, NAME_LEN)?;
+        let path = fs.path();
+
+        let (first_dir, second_dir) = {
+            let mut dirs = filter_dirs(path)?;
+            (
+                dirs.next().expect("Should have at least two dirs"),
+                dirs.next().expect("Should have at least two dirs"),
+            )
+        };
+        let first_file = filter_files(&first_dir)?
+            .next()
+            .expect("Should have at least one file");
+        // Both directories have a file with the same name.
+        let base_name = first_file
+            .file_name()
+            .and_then(|name| name.to_str())
+            .expect("File name exists and is valid");
+
+        debug!(
+            "Copying {} to {}",
+            first_file.display(),
+            second_dir.display()
+        );
+        operation_copy(vec![first_file.clone()], second_dir.clone())
+            .await
+            .expect_err(
+                "Copy operation should have failed because we're copying to different directories",
+            );
+        assert!(
+            first_dir.join(base_name).exists(),
+            "First file should still exist"
+        );
+        assert!(
+            second_dir.join(base_name).exists(),
+            "Second file should still exist"
+        );
+
+        Ok(())
+    }
+}