From 968e37afcd1c05857f65a1834df0bceb98e5f5bb Mon Sep 17 00:00:00 2001 From: jake <77554505+brxken128@users.noreply.github.com> Date: Sat, 7 Oct 2023 12:59:58 +0100 Subject: [PATCH] [ENG-1176] Append `(x)` to the end of files instead of overwriting (#1425) * append ` (x)` to files when duplicating, renaming or pasting instead of overwriting * cleanup commented code * fix renames * rustfmt * remove unused item * Small tech debts and some nitpicks * Bug with appending number on duplicates * A bug on my new impl --------- Co-authored-by: Ericson Fogo Soares --- core/src/api/files.rs | 25 +++--- core/src/object/fs/copy.rs | 77 +++++++++++++++---- core/src/object/fs/cut.rs | 2 +- core/src/object/fs/mod.rs | 58 ++++++++------ .../ContextMenu/FilePath/CutCopyItems.tsx | 3 +- .../$libraryId/Explorer/ParentContextMenu.tsx | 3 +- packages/client/src/core.ts | 2 +- 7 files changed, 111 insertions(+), 59 deletions(-) diff --git a/core/src/api/files.rs b/core/src/api/files.rs index d4de23a92..60ecfadb3 100644 --- a/core/src/api/files.rs +++ b/core/src/api/files.rs @@ -428,9 +428,10 @@ pub(crate) fn mount() -> AlphaRouter { Ok(_) => { return Err(rspc::Error::new( ErrorCode::Conflict, - "File already exists".to_string(), - )) + "Renaming would overwrite a file".to_string(), + )); } + Err(e) => { if e.kind() != std::io::ErrorKind::NotFound { return Err(rspc::Error::with_cause( @@ -439,19 +440,19 @@ pub(crate) fn mount() -> AlphaRouter { e, )); } + + fs::rename(location_path.join(&iso_file_path), new_file_full_path) + .await + .map_err(|e| { + rspc::Error::with_cause( + ErrorCode::InternalServerError, + "Failed to rename file".to_string(), + e, + ) + })?; } } - fs::rename(location_path.join(&iso_file_path), new_file_full_path) - .await - .map_err(|e| { - rspc::Error::with_cause( - ErrorCode::Conflict, - "Failed to rename file".to_string(), - e, - ) - })?; - Ok(()) } diff --git a/core/src/object/fs/copy.rs b/core/src/object/fs/copy.rs index 1fd5aa6ef..ba6851b75 100644 --- a/core/src/object/fs/copy.rs +++ b/core/src/object/fs/copy.rs @@ -13,7 +13,7 @@ use crate::{ }, }; -use std::{hash::Hash, path::PathBuf}; +use std::{ffi::OsStr, hash::Hash, path::PathBuf}; use serde::{Deserialize, Serialize}; use serde_json::json; @@ -22,8 +22,9 @@ use tokio::{fs, io}; use tracing::{trace, warn}; use super::{ - construct_target_filename, error::FileSystemJobsError, fetch_source_and_target_location_paths, - get_file_data_from_isolated_file_path, get_many_files_datas, FileData, + append_digit_to_filename, construct_target_filename, error::FileSystemJobsError, + fetch_source_and_target_location_paths, get_file_data_from_isolated_file_path, + get_many_files_datas, FileData, }; #[derive(Serialize, Deserialize, Debug, Clone)] @@ -37,7 +38,6 @@ pub struct FileCopierJobInit { pub target_location_id: location::id::Type, pub sources_file_path_ids: Vec, pub target_location_relative_directory_path: PathBuf, - pub target_file_name_suffix: Option, } #[derive(Serialize, Deserialize, Debug)] @@ -80,10 +80,7 @@ impl StatefulJob for FileCopierJobInit { &init.target_location_relative_directory_path, ); - full_target_path.push(construct_target_filename( - &file_data, - &init.target_file_name_suffix, - )?); + full_target_path.push(construct_target_filename(&file_data)?); Ok::<_, MissingFieldError>(FileCopierJobStep { source_file_data: file_data, @@ -173,17 +170,65 @@ impl StatefulJob for FileCopierJobInit { } Ok(more_steps.into()) - } else if &source_file_data.full_path == target_full_path { - // File is already here, do nothing - Ok(().into()) } else { match fs::metadata(target_full_path).await { Ok(_) => { - // only skip as it could be half way through a huge directory copy and run into an issue - warn!( - "Skipping {} as it would be overwritten", - target_full_path.display() - ); + let new_file_name = + target_full_path + .file_stem() + .ok_or(JobError::JobDataNotFound( + "No stem on file path, but it's supposed to be a file".to_string(), + ))?; + + let new_file_full_path_without_suffix = target_full_path.parent().map_or_else( + || { + Err(JobError::JobDataNotFound( + "No parent for file path, which is supposed to be directory" + .to_string(), + )) + }, + |x| Ok(x.to_path_buf()), + )?; + + for i in 1..u32::MAX { + let mut new_file_full_path_candidate = + new_file_full_path_without_suffix.clone(); + + append_digit_to_filename( + &mut new_file_full_path_candidate, + new_file_name.to_str().ok_or(JobError::JobDataNotFound( + "Unable to convert file name to &str".to_string(), + ))?, + target_full_path.extension().and_then(OsStr::to_str), + i, + ); + + match fs::metadata(&new_file_full_path_candidate).await { + Ok(_) => { + // This candidate already exists, so we try the next one + continue; + } + Err(e) if e.kind() == io::ErrorKind::NotFound => { + fs::copy( + &source_file_data.full_path, + &new_file_full_path_candidate, + ) + .await + // Using the ? here because we don't want to increase the completed task + // count in case of file system errors + .map_err(|e| { + FileIOError::from((new_file_full_path_candidate, e)) + })?; + + break; + } + Err(e) => { + return Err( + FileIOError::from((new_file_full_path_candidate, e)).into() + ) + } + } + } Ok(JobRunErrors(vec![FileSystemJobsError::WouldOverwrite( target_full_path.clone().into_boxed_path(), diff --git a/core/src/object/fs/cut.rs b/core/src/object/fs/cut.rs index 33809f15a..88660e91a 100644 --- a/core/src/object/fs/cut.rs +++ b/core/src/object/fs/cut.rs @@ -84,7 +84,7 @@ impl StatefulJob for FileCutterJobInit { ) -> Result, JobError> { let full_output = data .full_target_directory_path - .join(construct_target_filename(file_data, &None)?); + .join(construct_target_filename(file_data)?); if file_data.full_path == full_output { // File is already here, do nothing diff --git a/core/src/object/fs/mod.rs b/core/src/object/fs/mod.rs index 1236e36a4..8d55d447e 100644 --- a/core/src/object/fs/mod.rs +++ b/core/src/object/fs/mod.rs @@ -9,6 +9,8 @@ use crate::{ use std::path::{Path, PathBuf}; +use once_cell::sync::Lazy; +use regex::Regex; use serde::{Deserialize, Serialize}; pub mod create; @@ -25,6 +27,9 @@ pub mod error; use error::FileSystemJobsError; +static DUPLICATE_PATTERN: Lazy = + Lazy::new(|| Regex::new(r" \(\d+\)").expect("Failed to compile hardcoded regex")); + // pub const BYTES_EXT: &str = ".bytes"; #[derive(Serialize, Deserialize, Debug, Clone, Eq, PartialEq)] @@ -137,41 +142,44 @@ pub async fn fetch_source_and_target_location_paths( } } -fn construct_target_filename( - source_file_data: &FileData, - target_file_name_suffix: &Option, -) -> Result { +fn construct_target_filename(source_file_data: &FileData) -> Result { // extension wizardry for cloning and such // if no suffix has been selected, just use the file name // if a suffix is provided and it's a directory, use the directory name + suffix // if a suffix is provided and it's a file, use the (file name + suffix).extension - Ok(if let Some(ref suffix) = target_file_name_suffix { - if maybe_missing(source_file_data.file_path.is_dir, "file_path.is_dir")? + Ok( + if *maybe_missing(&source_file_data.file_path.is_dir, "file_path.is_dir")? || source_file_data.file_path.extension.is_none() || source_file_data.file_path.extension == Some(String::new()) { - format!( - "{}{suffix}", - maybe_missing(&source_file_data.file_path.name, "file_path.name")? - ) + maybe_missing(&source_file_data.file_path.name, "file_path.name")?.clone() } else { format!( - "{}{suffix}.{}", + "{}.{}", maybe_missing(&source_file_data.file_path.name, "file_path.name")?, - maybe_missing(&source_file_data.file_path.extension, "file_path.extension")?, + maybe_missing(&source_file_data.file_path.extension, "file_path.extension")? ) - } - } else if *maybe_missing(&source_file_data.file_path.is_dir, "file_path.is_dir")? - || source_file_data.file_path.extension.is_none() - || source_file_data.file_path.extension == Some(String::new()) - { - maybe_missing(&source_file_data.file_path.name, "file_path.name")?.clone() - } else { - format!( - "{}.{}", - maybe_missing(&source_file_data.file_path.name, "file_path.name")?, - maybe_missing(&source_file_data.file_path.extension, "file_path.extension")? - ) - }) + }, + ) +} + +pub fn append_digit_to_filename( + final_path: &mut PathBuf, + file_name: &str, + ext: Option<&str>, + current_int: u32, +) { + let new_file_name = if let Some(found) = DUPLICATE_PATTERN.find_iter(file_name).last() { + &file_name[..found.start()] + } else { + file_name + } + .to_string(); + + if let Some(ext) = ext { + final_path.push(format!("{} ({current_int}).{}", new_file_name, ext)); + } else { + final_path.push(format!("{new_file_name} ({current_int})")); + } } diff --git a/interface/app/$libraryId/Explorer/ContextMenu/FilePath/CutCopyItems.tsx b/interface/app/$libraryId/Explorer/ContextMenu/FilePath/CutCopyItems.tsx index a32137e71..52d5eb9eb 100644 --- a/interface/app/$libraryId/Explorer/ContextMenu/FilePath/CutCopyItems.tsx +++ b/interface/app/$libraryId/Explorer/ContextMenu/FilePath/CutCopyItems.tsx @@ -64,8 +64,7 @@ export const CutCopyItems = new ConditionalItem({ source_location_id: locationId, sources_file_path_ids: selectedFilePaths.map((p) => p.id), target_location_id: locationId, - target_location_relative_directory_path: path ?? '/', - target_file_name_suffix: ' copy' + target_location_relative_directory_path: path ?? '/' }); } catch (error) { toast.error({ diff --git a/interface/app/$libraryId/Explorer/ParentContextMenu.tsx b/interface/app/$libraryId/Explorer/ParentContextMenu.tsx index e2efbeda7..1379a269f 100644 --- a/interface/app/$libraryId/Explorer/ParentContextMenu.tsx +++ b/interface/app/$libraryId/Explorer/ParentContextMenu.tsx @@ -47,8 +47,7 @@ export default (props: PropsWithChildren) => { source_location_id: sourceLocationId, sources_file_path_ids: [...sourcePathIds], target_location_id: parent.location.id, - target_location_relative_directory_path: path, - target_file_name_suffix: sameLocation ? ' copy' : null + target_location_relative_directory_path: path }); } else if (sameLocation) { toast.error('File already exists in this location'); diff --git a/packages/client/src/core.ts b/packages/client/src/core.ts index 150963ab2..c3f1df172 100644 --- a/packages/client/src/core.ts +++ b/packages/client/src/core.ts @@ -159,7 +159,7 @@ export type ExplorerLayout = "grid" | "list" | "media" export type ExplorerSettings = { layoutMode: ExplorerLayout | null; gridItemSize: number | null; mediaColumns: number | null; mediaAspectSquare: boolean | null; mediaViewWithDescendants: boolean | null; openOnDoubleClick: DoubleClickAction | null; showBytesInGridView: boolean | null; colVisibility: { [key: string]: boolean } | null; colSizes: { [key: string]: number } | null; order?: TOrder | null; showHiddenFiles?: boolean } -export type FileCopierJobInit = { source_location_id: number; target_location_id: number; sources_file_path_ids: number[]; target_location_relative_directory_path: string; target_file_name_suffix: string | null } +export type FileCopierJobInit = { source_location_id: number; target_location_id: number; sources_file_path_ids: number[]; target_location_relative_directory_path: string } export type FileCutterJobInit = { source_location_id: number; target_location_id: number; sources_file_path_ids: number[]; target_location_relative_directory_path: string }