From 8d754017fad866c25a9b749c26fc1b7901229e20 Mon Sep 17 00:00:00 2001 From: lashman Date: Sun, 8 Mar 2026 00:22:24 +0200 Subject: [PATCH] Clean up low-severity code quality issues - Share IMAGE_EXTENSIONS between discovery.rs and watcher.rs (DRY) - Extract compute_renamed_path() to deduplicate ~100 lines in executor - Extract estimate_text_dimensions() to deduplicate watermark calc (3 copies) - Fix encoder fallback defaults: WebP 85, AVIF 63 (match QualityPreset::High) - Extract watch_config_dir() and load_watches() helpers in CLI (4 copies) - Remove redundant else branches after unwrap_or_default() - Rename misleading chrono_timestamp() to unix_timestamp() --- pixstrip-cli/src/main.rs | 67 +++++------- pixstrip-core/src/discovery.rs | 4 +- pixstrip-core/src/encoder.rs | 4 +- pixstrip-core/src/executor.rs | 121 +++++++++------------- pixstrip-core/src/operations/watermark.rs | 29 ++++-- pixstrip-core/src/watcher.rs | 5 +- 6 files changed, 97 insertions(+), 133 deletions(-) diff --git a/pixstrip-cli/src/main.rs b/pixstrip-cli/src/main.rs index 89441a3..afdfa35 100644 --- a/pixstrip-cli/src/main.rs +++ b/pixstrip-cli/src/main.rs @@ -9,6 +9,20 @@ use pixstrip_core::types::*; use std::io::Write; use std::path::PathBuf; +fn watch_config_dir() -> PathBuf { + dirs::config_dir() + .or_else(|| dirs::home_dir().map(|h| h.join(".config"))) + .unwrap_or_else(|| std::env::temp_dir()) + .join("pixstrip") +} + +fn load_watches(watches_path: &std::path::Path) -> Vec { + std::fs::read_to_string(watches_path) + .ok() + .and_then(|s| serde_json::from_str(&s).ok()) + .unwrap_or_default() +} + #[derive(Parser)] #[command(name = "pixstrip")] #[command(about = "Batch image processor - resize, convert, compress, strip metadata, watermark, and rename")] @@ -440,7 +454,7 @@ fn cmd_process(args: CmdProcessArgs) { // Save to history - use actual output paths from the executor let history = HistoryStore::new(); if let Err(e) = history.add(pixstrip_core::storage::HistoryEntry { - timestamp: chrono_timestamp(), + timestamp: unix_timestamp(), input_dir: input_dir.canonicalize().unwrap_or_else(|_| input_dir.to_path_buf()).to_string_lossy().into(), output_dir: output_dir.canonicalize().unwrap_or_else(|_| output_dir.to_path_buf()).to_string_lossy().into(), preset_name: args.preset, @@ -640,19 +654,9 @@ fn cmd_watch_add(path: &str, preset_name: &str, recursive: bool) { }; // Store in config - let config_dir = dirs::config_dir() - .or_else(|| dirs::home_dir().map(|h| h.join(".config"))) - .unwrap_or_else(|| std::env::temp_dir()) - .join("pixstrip"); + let config_dir = watch_config_dir(); let watches_path = config_dir.join("watches.json"); - let mut watches: Vec = if watches_path.exists() { - std::fs::read_to_string(&watches_path) - .ok() - .and_then(|s| serde_json::from_str(&s).ok()) - .unwrap_or_default() - } else { - Vec::new() - }; + let mut watches = load_watches(&watches_path); // Don't add duplicate paths if watches.iter().any(|w| w.path == watch.path) { @@ -682,20 +686,10 @@ fn cmd_watch_add(path: &str, preset_name: &str, recursive: bool) { } fn cmd_watch_list() { - let config_dir = dirs::config_dir() - .or_else(|| dirs::home_dir().map(|h| h.join(".config"))) - .unwrap_or_else(|| std::env::temp_dir()) - .join("pixstrip"); + let config_dir = watch_config_dir(); let watches_path = config_dir.join("watches.json"); - let watches: Vec = if watches_path.exists() { - std::fs::read_to_string(&watches_path) - .ok() - .and_then(|s| serde_json::from_str(&s).ok()) - .unwrap_or_default() - } else { - Vec::new() - }; + let watches = load_watches(&watches_path); if watches.is_empty() { println!("No watch folders configured."); @@ -718,20 +712,10 @@ fn cmd_watch_list() { } fn cmd_watch_remove(path: &str) { - let config_dir = dirs::config_dir() - .or_else(|| dirs::home_dir().map(|h| h.join(".config"))) - .unwrap_or_else(|| std::env::temp_dir()) - .join("pixstrip"); + let config_dir = watch_config_dir(); let watches_path = config_dir.join("watches.json"); - let mut watches: Vec = if watches_path.exists() { - std::fs::read_to_string(&watches_path) - .ok() - .and_then(|s| serde_json::from_str(&s).ok()) - .unwrap_or_default() - } else { - Vec::new() - }; + let mut watches = load_watches(&watches_path); let original_len = watches.len(); let target = PathBuf::from(path); @@ -759,10 +743,7 @@ fn cmd_watch_remove(path: &str) { } fn cmd_watch_start() { - let config_dir = dirs::config_dir() - .or_else(|| dirs::home_dir().map(|h| h.join(".config"))) - .unwrap_or_else(|| std::env::temp_dir()) - .join("pixstrip"); + let config_dir = watch_config_dir(); let watches_path = config_dir.join("watches.json"); let watches: Vec = if watches_path.exists() { @@ -876,7 +857,7 @@ fn cmd_watch_start() { println!(" Processed: {} -> {}", format_bytes(r.total_input_bytes), format_bytes(r.total_output_bytes)); let history = HistoryStore::new(); let _ = history.add(pixstrip_core::storage::HistoryEntry { - timestamp: chrono_timestamp(), + timestamp: unix_timestamp(), input_dir: input_dir.to_string_lossy().into(), output_dir: output_dir.to_string_lossy().into(), preset_name: Some(preset_name.to_string()), @@ -1065,7 +1046,7 @@ fn format_duration(ms: u64) -> String { } } -fn chrono_timestamp() -> String { +fn unix_timestamp() -> String { // Store as Unix seconds string (must match GTK format for pruning compatibility) std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) diff --git a/pixstrip-core/src/discovery.rs b/pixstrip-core/src/discovery.rs index 91d5533..7e97404 100644 --- a/pixstrip-core/src/discovery.rs +++ b/pixstrip-core/src/discovery.rs @@ -2,11 +2,11 @@ use std::path::{Path, PathBuf}; use walkdir::WalkDir; -const IMAGE_EXTENSIONS: &[&str] = &[ +pub const IMAGE_EXTENSIONS: &[&str] = &[ "jpg", "jpeg", "png", "webp", "avif", "gif", "tiff", "tif", "bmp", ]; -fn is_image_extension(ext: &str) -> bool { +pub fn is_image_extension(ext: &str) -> bool { IMAGE_EXTENSIONS.contains(&ext.to_lowercase().as_str()) } diff --git a/pixstrip-core/src/encoder.rs b/pixstrip-core/src/encoder.rs index a6475a4..82825e5 100644 --- a/pixstrip-core/src/encoder.rs +++ b/pixstrip-core/src/encoder.rs @@ -40,8 +40,8 @@ impl OutputEncoder { match format { ImageFormat::Jpeg => self.encode_jpeg(img, quality.unwrap_or(85)), ImageFormat::Png => self.encode_png(img, quality.unwrap_or(3)), - ImageFormat::WebP => self.encode_webp(img, quality.unwrap_or(80)), - ImageFormat::Avif => self.encode_avif(img, quality.unwrap_or(80)), + ImageFormat::WebP => self.encode_webp(img, quality.unwrap_or(85)), + ImageFormat::Avif => self.encode_avif(img, quality.unwrap_or(63)), ImageFormat::Gif => self.encode_fallback(img, image::ImageFormat::Gif), ImageFormat::Tiff => self.encode_fallback(img, image::ImageFormat::Tiff), ImageFormat::Bmp => self.encode_fallback(img, image::ImageFormat::Bmp), diff --git a/pixstrip-core/src/executor.rs b/pixstrip-core/src/executor.rs index 1486316..bd06e6c 100644 --- a/pixstrip-core/src/executor.rs +++ b/pixstrip-core/src/executor.rs @@ -325,6 +325,46 @@ impl PipelineExecutor { Ok(result) } + /// Compute the renamed output path for a source file. + /// `ext` is the output extension, `dims` is (width, height) or None. + fn compute_renamed_path( + job: &ProcessingJob, + rename: &crate::operations::RenameConfig, + source: &crate::types::ImageSource, + ext: &str, + index: usize, + dims: Option<(u32, u32)>, + ) -> std::path::PathBuf { + let stem = source.path.file_stem().and_then(|s| s.to_str()).unwrap_or("output"); + if let Some(ref template) = rename.template { + let original_ext = source.path.extension().and_then(|e| e.to_str()); + let working_stem = crate::operations::rename::apply_regex_replace( + stem, &rename.regex_find, &rename.regex_replace, + ); + let working_stem = crate::operations::rename::apply_space_replacement(&working_stem, rename.replace_spaces); + let working_stem = crate::operations::rename::apply_special_chars(&working_stem, rename.special_chars); + let new_name = crate::operations::rename::apply_template_full( + template, &working_stem, ext, + rename.counter_start.saturating_add(index as u32), + dims, original_ext, Some(&source.path), None, + ); + let new_name = if rename.case_mode > 0 { + if let Some(dot_pos) = new_name.rfind('.') { + let (name_part, ext_part) = new_name.split_at(dot_pos); + format!("{}{}", crate::operations::rename::apply_case_conversion(name_part, rename.case_mode), ext_part) + } else { + crate::operations::rename::apply_case_conversion(&new_name, rename.case_mode) + } + } else { + new_name + }; + job.output_dir.join(sanitize_filename(&new_name)) + } else { + let new_name = rename.apply_simple(stem, ext, (index as u32).saturating_add(1)); + job.output_dir.join(sanitize_filename(&new_name)) + } + } + fn process_single_static( job: &ProcessingJob, source: &crate::types::ImageSource, @@ -350,40 +390,17 @@ impl PipelineExecutor { // just copy the file instead of decoding/re-encoding. if !job.needs_pixel_processing() && !metadata_needs_reencode { let output_path = if let Some(ref rename) = job.rename { - let stem = source.path.file_stem().and_then(|s| s.to_str()).unwrap_or("output"); let ext = source.path.extension().and_then(|e| e.to_str()).unwrap_or("jpg"); - if let Some(ref template) = rename.template { - // Read dimensions without full decode for {width}/{height} templates - let dims = image::ImageReader::open(&source.path) + // Read dimensions without full decode for {width}/{height} templates + let dims = if rename.template.is_some() { + image::ImageReader::open(&source.path) .ok() .and_then(|r| r.with_guessed_format().ok()) - .and_then(|r| r.into_dimensions().ok()); - // Apply rename transformations on stem (matching the pixel path) - let working_stem = crate::operations::rename::apply_regex_replace( - stem, &rename.regex_find, &rename.regex_replace, - ); - let working_stem = crate::operations::rename::apply_space_replacement(&working_stem, rename.replace_spaces); - let working_stem = crate::operations::rename::apply_special_chars(&working_stem, rename.special_chars); - let new_name = crate::operations::rename::apply_template_full( - template, &working_stem, ext, - rename.counter_start.saturating_add(index as u32), - dims, Some(ext), Some(&source.path), None, - ); - let new_name = if rename.case_mode > 0 { - if let Some(dot_pos) = new_name.rfind('.') { - let (name_part, ext_part) = new_name.split_at(dot_pos); - format!("{}{}", crate::operations::rename::apply_case_conversion(name_part, rename.case_mode), ext_part) - } else { - crate::operations::rename::apply_case_conversion(&new_name, rename.case_mode) - } - } else { - new_name - }; - job.output_dir.join(sanitize_filename(&new_name)) + .and_then(|r| r.into_dimensions().ok()) } else { - let new_name = rename.apply_simple(stem, ext, (index as u32).saturating_add(1)); - job.output_dir.join(sanitize_filename(&new_name)) - } + None + }; + Self::compute_renamed_path(job, rename, source, ext, index, dims) } else { job.output_path_for(source, None) }; @@ -503,49 +520,9 @@ impl PipelineExecutor { // Determine output path (with rename if configured) let output_path = if let Some(ref rename) = job.rename { - let stem = source - .path - .file_stem() - .and_then(|s| s.to_str()) - .unwrap_or("output"); let ext = output_format.extension(); - - if let Some(ref template) = rename.template { - let dims = Some((img.width(), img.height())); - let original_ext = source.path.extension() - .and_then(|e| e.to_str()); - // Apply rename transformations on the stem before template expansion - let working_stem = crate::operations::rename::apply_regex_replace( - stem, &rename.regex_find, &rename.regex_replace, - ); - let working_stem = crate::operations::rename::apply_space_replacement(&working_stem, rename.replace_spaces); - let working_stem = crate::operations::rename::apply_special_chars(&working_stem, rename.special_chars); - let new_name = crate::operations::rename::apply_template_full( - template, - &working_stem, - ext, - rename.counter_start.saturating_add(index as u32), - dims, - original_ext, - Some(&source.path), - None, - ); - // Apply case conversion to the final name (without extension) - let new_name = if rename.case_mode > 0 { - if let Some(dot_pos) = new_name.rfind('.') { - let (name_part, ext_part) = new_name.split_at(dot_pos); - format!("{}{}", crate::operations::rename::apply_case_conversion(name_part, rename.case_mode), ext_part) - } else { - crate::operations::rename::apply_case_conversion(&new_name, rename.case_mode) - } - } else { - new_name - }; - job.output_dir.join(sanitize_filename(&new_name)) - } else { - let new_name = rename.apply_simple(stem, ext, (index as u32).saturating_add(1)); - job.output_dir.join(sanitize_filename(&new_name)) - } + let dims = Some((img.width(), img.height())); + Self::compute_renamed_path(job, rename, source, ext, index, dims) } else { job.output_path_for(source, Some(output_format)) }; diff --git a/pixstrip-core/src/operations/watermark.rs b/pixstrip-core/src/operations/watermark.rs index 55fc2a4..4e448ff 100644 --- a/pixstrip-core/src/operations/watermark.rs +++ b/pixstrip-core/src/operations/watermark.rs @@ -189,6 +189,18 @@ fn walkdir_depth_inner(dir: &std::path::Path, max_depth: u32, results: &mut Vec< } } +/// Estimate text dimensions for a given string and font size. +/// Returns (width, height) in pixels. +fn estimate_text_dimensions(text: &str, font_size: f32) -> (u32, u32) { + let w = ((text.chars().count().min(10_000) as f32 * font_size.min(1000.0) * 0.6) as u32) + .saturating_add(4) + .min(16384); + let h = ((font_size.min(1000.0) * 1.4) as u32) + .saturating_add(4) + .min(4096); + (w, h) +} + /// Render text onto a transparent RGBA buffer and return it as a DynamicImage fn render_text_to_image( text: &str, @@ -198,8 +210,7 @@ fn render_text_to_image( opacity: f32, ) -> image::RgbaImage { let scale = ab_glyph::PxScale::from(font_size); - let text_width = ((text.chars().count().min(10_000) as f32 * font_size.min(1000.0) * 0.6) as u32).saturating_add(4).min(16384); - let text_height = ((font_size.min(1000.0) * 1.4) as u32).saturating_add(4).min(4096); + let (text_width, text_height) = estimate_text_dimensions(text, font_size); let alpha = (opacity * 255.0).clamp(0.0, 255.0) as u8; let draw_color = Rgba([color[0], color[1], color[2], alpha]); @@ -296,11 +307,10 @@ fn apply_text_watermark( } else { // No rotation - draw text directly (faster) let scale = ab_glyph::PxScale::from(font_size); - let text_width = ((text.chars().count().min(10_000) as f32 * font_size.min(1000.0) * 0.6) as u32).saturating_add(4).min(16384); - let text_height = ((font_size.min(1000.0) * 1.4) as u32).saturating_add(4).min(4096); + let (tw, th) = estimate_text_dimensions(text, font_size); let text_dims = Dimensions { - width: text_width, - height: text_height, + width: tw, + height: th, }; let image_dims = Dimensions { width: img.width(), @@ -366,17 +376,16 @@ fn apply_tiled_text_watermark( let alpha = (opacity * 255.0).clamp(0.0, 255.0) as u8; let draw_color = Rgba([color[0], color[1], color[2], alpha]); - let text_width = ((text.chars().count().min(10_000) as f32 * font_size.min(1000.0) * 0.6) as i64 + 4).min(16384); - let text_height = ((font_size.min(1000.0) * 1.4) as i64 + 4).min(4096); + let (text_width, text_height) = estimate_text_dimensions(text, font_size); let mut y: i64 = 0; while y < ih as i64 { let mut x: i64 = 0; while x < iw as i64 { draw_text_mut(&mut rgba, draw_color, x as i32, y as i32, scale, &font, text); - x += text_width + spacing as i64; + x += text_width as i64 + spacing as i64; } - y += text_height + spacing as i64; + y += text_height as i64 + spacing as i64; } } diff --git a/pixstrip-core/src/watcher.rs b/pixstrip-core/src/watcher.rs index becd23d..22871a6 100644 --- a/pixstrip-core/src/watcher.rs +++ b/pixstrip-core/src/watcher.rs @@ -112,10 +112,7 @@ impl Default for FolderWatcher { } fn is_image_file(path: &Path) -> bool { - let supported = [ - "jpg", "jpeg", "png", "webp", "avif", "gif", "tiff", "tif", "bmp", - ]; path.extension() .and_then(|e| e.to_str()) - .is_some_and(|ext| supported.contains(&ext.to_lowercase().as_str())) + .is_some_and(crate::discovery::is_image_extension) }