From 1e56bb8f1ff7d037f9024e213fe89675a5318a34 Mon Sep 17 00:00:00 2001 From: Antonios Barotsis Date: Tue, 12 Mar 2024 03:33:31 +0100 Subject: [PATCH] Improve logging interface --- src/archive/rar.rs | 26 ++----- src/archive/sevenz.rs | 61 +++++----------- src/archive/tar.rs | 53 +++++--------- src/archive/zip.rs | 66 +++++------------ src/check.rs | 50 +++++-------- src/commands/compress.rs | 15 ++-- src/commands/decompress.rs | 143 +++++++++++++++---------------------- src/commands/list.rs | 9 ++- src/commands/mod.rs | 95 ++++++------------------ src/extension.rs | 29 +++----- src/utils/fs.rs | 16 +---- src/utils/logger.rs | 75 +++++++++++++++++++ src/utils/message.rs | 29 -------- src/utils/mod.rs | 2 +- 14 files changed, 258 insertions(+), 411 deletions(-) create mode 100644 src/utils/logger.rs delete mode 100644 src/utils/message.rs diff --git a/src/archive/rar.rs b/src/archive/rar.rs index 16c5737..9e59357 100644 --- a/src/archive/rar.rs +++ b/src/archive/rar.rs @@ -1,23 +1,14 @@ //! Contains RAR-specific building and unpacking functions -use std::{path::Path, sync::mpsc::Sender}; +use std::path::Path; use unrar::Archive; -use crate::{ - error::Error, - list::FileInArchive, - utils::message::{MessageLevel, PrintMessage}, -}; +use crate::{error::Error, list::FileInArchive, utils::logger::Logger}; /// Unpacks the archive given by `archive_path` into the folder given by `output_folder`. /// Assumes that output_folder is empty -pub fn unpack_archive( - archive_path: &Path, - output_folder: &Path, - quiet: bool, - log_sender: Sender, -) -> crate::Result { +pub fn unpack_archive(archive_path: &Path, output_folder: &Path, quiet: bool, logger: Logger) -> crate::Result { assert!(output_folder.read_dir().expect("dir exists").count() == 0); let mut archive = Archive::new(archive_path).open_for_processing()?; @@ -27,13 +18,10 @@ pub fn unpack_archive( let entry = header.entry(); archive = if entry.is_file() { if !quiet { - log_sender - .send(PrintMessage { - contents: format!("{} extracted. ({})", entry.filename.display(), entry.unpacked_size), - accessible: false, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!("{} extracted. ({})", entry.filename.display(), entry.unpacked_size), + false, + ); } unpacked += 1; header.extract_with_base(output_folder)? diff --git a/src/archive/sevenz.rs b/src/archive/sevenz.rs index 0a00226..fda31ec 100644 --- a/src/archive/sevenz.rs +++ b/src/archive/sevenz.rs @@ -4,7 +4,6 @@ use std::{ env, io::{self, Read, Seek, Write}, path::{Path, PathBuf}, - sync::mpsc::Sender, }; use fs_err as fs; @@ -12,11 +11,7 @@ use same_file::Handle; use crate::{ error::FinalError, - utils::{ - self, cd_into_same_dir_as, - message::{MessageLevel, PrintMessage}, - Bytes, EscapedPathDisplay, FileVisibilityPolicy, - }, + utils::{self, cd_into_same_dir_as, logger::Logger, Bytes, EscapedPathDisplay, FileVisibilityPolicy}, }; pub fn compress_sevenz( @@ -25,7 +20,7 @@ pub fn compress_sevenz( writer: W, file_visibility_policy: FileVisibilityPolicy, quiet: bool, - log_sender: Sender, + logger: Logger, ) -> crate::Result where W: Write + Seek, @@ -47,16 +42,11 @@ where // If the output_path is the same as the input file, warn the user and skip the input (in order to avoid compression recursion) if let Ok(handle) = &output_handle { if matches!(Handle::from_path(path), Ok(x) if &x == handle) { - log_sender - .send(PrintMessage { - contents: format!( - "The output file and the input file are the same: `{}`, skipping...", - output_path.display() - ), - accessible: true, - level: MessageLevel::Warning, - }) - .unwrap(); + logger.warning(format!( + "The output file and the input file are the same: `{}`, skipping...", + output_path.display() + )); + continue; } } @@ -66,13 +56,7 @@ where // spoken text for users using screen readers, braille displays // and so on if !quiet { - log_sender - .send(PrintMessage { - contents: format!("Compressing '{}'.", EscapedPathDisplay::new(path)), - accessible: false, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info(format!("Compressing '{}'.", EscapedPathDisplay::new(path)), false); } let metadata = match path.metadata() { @@ -109,12 +93,7 @@ where Ok(bytes) } -pub fn decompress_sevenz( - reader: R, - output_path: &Path, - quiet: bool, - log_sender: Sender, -) -> crate::Result +pub fn decompress_sevenz(reader: R, output_path: &Path, quiet: bool, logger: Logger) -> crate::Result where R: Read + Seek, { @@ -130,26 +109,20 @@ where if entry.is_directory() { if !quiet { - log_sender - .send(PrintMessage { - contents: format!("File {} extracted to \"{}\"", entry.name(), file_path.display()), - accessible: false, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!("File {} extracted to \"{}\"", entry.name(), file_path.display()), + false, + ); } if !path.exists() { fs::create_dir_all(path)?; } } else { if !quiet { - log_sender - .send(PrintMessage { - contents: format!("{:?} extracted. ({})", file_path.display(), Bytes::new(entry.size()),), - accessible: false, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!("{:?} extracted. ({})", file_path.display(), Bytes::new(entry.size())), + false, + ); } if let Some(parent) = path.parent() { diff --git a/src/archive/tar.rs b/src/archive/tar.rs index 5e884c4..77e7e61 100644 --- a/src/archive/tar.rs +++ b/src/archive/tar.rs @@ -4,7 +4,7 @@ use std::{ env, io::prelude::*, path::{Path, PathBuf}, - sync::mpsc::{self, Receiver, Sender}, + sync::mpsc::{self, Receiver}, thread, }; @@ -14,11 +14,7 @@ use same_file::Handle; use crate::{ error::FinalError, list::FileInArchive, - utils::{ - self, - message::{MessageLevel, PrintMessage}, - Bytes, EscapedPathDisplay, FileVisibilityPolicy, - }, + utils::{self, logger::Logger, Bytes, EscapedPathDisplay, FileVisibilityPolicy}, }; /// Unpacks the archive given by `archive` into the folder given by `into`. @@ -27,7 +23,7 @@ pub fn unpack_archive( reader: Box, output_folder: &Path, quiet: bool, - log_sender: Sender, + logger: Logger, ) -> crate::Result { assert!(output_folder.read_dir().expect("dir exists").count() == 0); let mut archive = tar::Archive::new(reader); @@ -43,17 +39,14 @@ pub fn unpack_archive( // spoken text for users using screen readers, braille displays // and so on if !quiet { - log_sender - .send(PrintMessage { - contents: format!( - "{:?} extracted. ({})", - utils::strip_cur_dir(&output_folder.join(file.path()?)), - Bytes::new(file.size()), - ), - accessible: false, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!( + "{:?} extracted. ({})", + utils::strip_cur_dir(&output_folder.join(file.path()?)), + Bytes::new(file.size()), + ), + false, + ); files_unpacked += 1; } @@ -98,7 +91,7 @@ pub fn build_archive_from_paths( writer: W, file_visibility_policy: FileVisibilityPolicy, quiet: bool, - log_sender: Sender, + logger: Logger, ) -> crate::Result where W: Write, @@ -120,16 +113,10 @@ where // If the output_path is the same as the input file, warn the user and skip the input (in order to avoid compression recursion) if let Ok(handle) = &output_handle { if matches!(Handle::from_path(path), Ok(x) if &x == handle) { - log_sender - .send(PrintMessage { - contents: format!( - "The output file and the input file are the same: `{}`, skipping...", - output_path.display() - ), - accessible: true, - level: MessageLevel::Warning, - }) - .unwrap(); + logger.warning(format!( + "The output file and the input file are the same: `{}`, skipping...", + output_path.display() + )); continue; } @@ -140,13 +127,7 @@ where // spoken text for users using screen readers, braille displays // and so on if !quiet { - log_sender - .send(PrintMessage { - contents: format!("Compressing '{}'.", EscapedPathDisplay::new(path)), - accessible: false, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info(format!("Compressing '{}'.", EscapedPathDisplay::new(path)), false); } if path.is_dir() { diff --git a/src/archive/zip.rs b/src/archive/zip.rs index 84c45b6..e1dbf5c 100644 --- a/src/archive/zip.rs +++ b/src/archive/zip.rs @@ -6,7 +6,7 @@ use std::{ env, io::{self, prelude::*}, path::{Path, PathBuf}, - sync::mpsc::{self, Sender}, + sync::mpsc, thread, }; @@ -20,9 +20,8 @@ use crate::{ error::FinalError, list::FileInArchive, utils::{ - self, cd_into_same_dir_as, get_invalid_utf8_paths, - message::{MessageLevel, PrintMessage}, - pretty_format_list_of_paths, strip_cur_dir, Bytes, EscapedPathDisplay, FileVisibilityPolicy, + self, cd_into_same_dir_as, get_invalid_utf8_paths, logger::Logger, pretty_format_list_of_paths, strip_cur_dir, + Bytes, EscapedPathDisplay, FileVisibilityPolicy, }, }; @@ -32,7 +31,7 @@ pub fn unpack_archive( mut archive: ZipArchive, output_folder: &Path, quiet: bool, - log_sender: Sender, + logger: Logger, ) -> crate::Result where R: Read + Seek, @@ -50,7 +49,7 @@ where let file_path = output_folder.join(file_path); - display_zip_comment_if_exists(&file, log_sender.clone()); + display_zip_comment_if_exists(&file, logger.clone()); match file.name().ends_with('/') { _is_dir @ true => { @@ -59,13 +58,7 @@ where // spoken text for users using screen readers, braille displays // and so on if !quiet { - log_sender - .send(PrintMessage { - contents: format!("File {} extracted to \"{}\"", idx, file_path.display()), - accessible: false, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info(format!("File {} extracted to \"{}\"", idx, file_path.display()), false); } fs::create_dir_all(&file_path)?; } @@ -79,13 +72,10 @@ where // same reason is in _is_dir: long, often not needed text if !quiet { - log_sender - .send(PrintMessage { - contents: format!("{:?} extracted. ({})", file_path.display(), Bytes::new(file.size()),), - accessible: false, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!("{:?} extracted. ({})", file_path.display(), Bytes::new(file.size())), + false, + ); } let mut output_file = fs::File::create(file_path)?; @@ -148,7 +138,7 @@ pub fn build_archive_from_paths( writer: W, file_visibility_policy: FileVisibilityPolicy, quiet: bool, - log_sender: Sender, + logger: Logger, ) -> crate::Result where W: Write + Seek, @@ -190,18 +180,10 @@ where // If the output_path is the same as the input file, warn the user and skip the input (in order to avoid compression recursion) if let Ok(handle) = &output_handle { if matches!(Handle::from_path(path), Ok(x) if &x == handle) { - log_sender - .send(PrintMessage { - contents: format!( - "The output file and the input file are the same: `{}`, skipping...", - output_path.display() - ), - accessible: true, - level: MessageLevel::Warning, - }) - .unwrap(); - - continue; + logger.warning(format!( + "The output file and the input file are the same: `{}`, skipping...", + output_path.display() + )); } } @@ -210,13 +192,7 @@ where // spoken text for users using screen readers, braille displays // and so on if !quiet { - log_sender - .send(PrintMessage { - contents: format!("Compressing '{}'.", EscapedPathDisplay::new(path)), - accessible: false, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info(format!("Compressing '{}'.", EscapedPathDisplay::new(path)), false); } let metadata = match path.metadata() { @@ -266,7 +242,7 @@ where Ok(bytes) } -fn display_zip_comment_if_exists(file: &ZipFile, log_sender: Sender) { +fn display_zip_comment_if_exists(file: &ZipFile, logger: Logger) { let comment = file.comment(); if !comment.is_empty() { // Zip file comments seem to be pretty rare, but if they are used, @@ -279,13 +255,7 @@ fn display_zip_comment_if_exists(file: &ZipFile, log_sender: Sender, question_policy: QuestionPolicy, - log_sender: Sender, + logger: Logger, ) -> Result> { if formats.is_empty() { // File with no extension @@ -37,13 +35,11 @@ pub fn check_mime_type( if let Some(detected_format) = try_infer_extension(path) { // Inferring the file extension can have unpredicted consequences (e.g. the user just // mistyped, ...) which we should always inform the user about. - log_sender - .send(PrintMessage { - contents: format!("Detected file: `{}` extension as `{}`", path.display(), detected_format), - accessible: true, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!("Detected file: `{}` extension as `{}`", path.display(), detected_format), + true, + ); + if user_wants_to_continue(path, question_policy, QuestionAction::Decompression)? { formats.push(detected_format); } else { @@ -59,16 +55,11 @@ pub fn check_mime_type( .compression_formats .ends_with(detected_format.compression_formats) { - log_sender - .send(PrintMessage { - contents: format!( - "The file extension: `{}` differ from the detected extension: `{}`", - outer_ext, detected_format - ), - accessible: true, - level: MessageLevel::Warning, - }) - .unwrap(); + logger.warning(format!( + "The file extension: `{}` differ from the detected extension: `{}`", + outer_ext, detected_format + )); + if !user_wants_to_continue(path, question_policy, QuestionAction::Decompression)? { return Ok(ControlFlow::Break(())); } @@ -76,16 +67,13 @@ pub fn check_mime_type( } else { // NOTE: If this actually produces no false positives, we can upgrade it in the future // to a warning and ask the user if he wants to continue decompressing. - log_sender - .send(PrintMessage { - contents: format!( - "Failed to confirm the format of `{}` by sniffing the contents, file might be misnamed", - path.display() - ), - accessible: true, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!( + "Failed to confirm the format of `{}` by sniffing the contents, file might be misnamed", + path.display() + ), + true, + ); } Ok(ControlFlow::Continue(())) } diff --git a/src/commands/compress.rs b/src/commands/compress.rs index bc2f259..273d0d4 100644 --- a/src/commands/compress.rs +++ b/src/commands/compress.rs @@ -1,7 +1,6 @@ use std::{ io::{self, BufWriter, Cursor, Seek, Write}, path::{Path, PathBuf}, - sync::mpsc::Sender, }; use fs_err as fs; @@ -11,7 +10,7 @@ use crate::{ archive, commands::warn_user_about_loading_zip_in_memory, extension::{split_first_compression_format, CompressionFormat::*, Extension}, - utils::{message::PrintMessage, user_wants_to_continue, FileVisibilityPolicy}, + utils::{logger::Logger, user_wants_to_continue, FileVisibilityPolicy}, QuestionAction, QuestionPolicy, BUFFER_CAPACITY, }; @@ -35,7 +34,7 @@ pub fn compress_files( question_policy: QuestionPolicy, file_visibility_policy: FileVisibilityPolicy, level: Option, - log_sender: Sender, + logger: Logger, ) -> crate::Result { // If the input files contain a directory, then the total size will be underestimated let file_writer = BufWriter::with_capacity(BUFFER_CAPACITY, output_file); @@ -107,13 +106,13 @@ pub fn compress_files( &mut writer, file_visibility_policy, quiet, - log_sender.clone(), + logger.clone(), )?; writer.flush()?; } Zip => { if !formats.is_empty() { - warn_user_about_loading_zip_in_memory(log_sender.clone()); + warn_user_about_loading_zip_in_memory(logger.clone()); if !user_wants_to_continue(output_path, question_policy, QuestionAction::Compression)? { return Ok(false); @@ -128,7 +127,7 @@ pub fn compress_files( &mut vec_buffer, file_visibility_policy, quiet, - log_sender.clone(), + logger.clone(), )?; vec_buffer.rewind()?; io::copy(&mut vec_buffer, &mut writer)?; @@ -142,7 +141,7 @@ pub fn compress_files( } SevenZip => { if !formats.is_empty() { - warn_user_about_loading_sevenz_in_memory(log_sender.clone()); + warn_user_about_loading_sevenz_in_memory(logger.clone()); if !user_wants_to_continue(output_path, question_policy, QuestionAction::Compression)? { return Ok(false); @@ -156,7 +155,7 @@ pub fn compress_files( &mut vec_buffer, file_visibility_policy, quiet, - log_sender.clone(), + logger.clone(), )?; vec_buffer.rewind()?; io::copy(&mut vec_buffer, &mut writer)?; diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 97b938b..5c4dec9 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -2,7 +2,6 @@ use std::{ io::{self, BufReader, Read}, ops::ControlFlow, path::{Path, PathBuf}, - sync::mpsc::Sender, }; use fs_err as fs; @@ -14,11 +13,7 @@ use crate::{ CompressionFormat::{self, *}, Extension, }, - utils::{ - self, - message::{MessageLevel, PrintMessage}, - nice_directory_display, user_wants_to_continue, - }, + utils::{self, logger::Logger, nice_directory_display, user_wants_to_continue}, QuestionAction, QuestionPolicy, BUFFER_CAPACITY, }; @@ -35,7 +30,7 @@ pub fn decompress_file( output_file_path: PathBuf, question_policy: QuestionPolicy, quiet: bool, - log_sender: Sender, + logger: Logger, ) -> crate::Result<()> { assert!(output_dir.exists()); let reader = fs::File::open(input_file_path)?; @@ -54,11 +49,11 @@ pub fn decompress_file( { let zip_archive = zip::ZipArchive::new(reader)?; let files_unpacked = if let ControlFlow::Continue(files) = smart_unpack( - |output_dir| crate::archive::zip::unpack_archive(zip_archive, output_dir, quiet, log_sender.clone()), + |output_dir| crate::archive::zip::unpack_archive(zip_archive, output_dir, quiet, logger.clone()), output_dir, &output_file_path, question_policy, - log_sender.clone(), + logger.clone(), )? { files } else { @@ -69,17 +64,14 @@ pub fn decompress_file( // having a final status message is important especially in an accessibility context // as screen readers may not read a commands exit code, making it hard to reason // about whether the command succeeded without such a message - log_sender - .send(PrintMessage { - contents: format!( - "Successfully decompressed archive in {} ({} files).", - nice_directory_display(output_dir), - files_unpacked - ), - accessible: true, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!( + "Successfully decompressed archive in {} ({} files).", + nice_directory_display(output_dir), + files_unpacked + ), + true, + ); return Ok(()); } @@ -123,11 +115,11 @@ pub fn decompress_file( } Tar => { if let ControlFlow::Continue(files) = smart_unpack( - |output_dir| crate::archive::tar::unpack_archive(reader, output_dir, quiet, log_sender.clone()), + |output_dir| crate::archive::tar::unpack_archive(reader, output_dir, quiet, logger.clone()), output_dir, &output_file_path, question_policy, - log_sender.clone(), + logger.clone(), )? { files } else { @@ -136,7 +128,7 @@ pub fn decompress_file( } Zip => { if formats.len() > 1 { - warn_user_about_loading_zip_in_memory(log_sender.clone()); + warn_user_about_loading_zip_in_memory(logger.clone()); if !user_wants_to_continue(input_file_path, question_policy, QuestionAction::Decompression)? { return Ok(()); @@ -148,11 +140,11 @@ pub fn decompress_file( let zip_archive = zip::ZipArchive::new(io::Cursor::new(vec))?; if let ControlFlow::Continue(files) = smart_unpack( - |output_dir| crate::archive::zip::unpack_archive(zip_archive, output_dir, quiet, log_sender.clone()), + |output_dir| crate::archive::zip::unpack_archive(zip_archive, output_dir, quiet, logger.clone()), output_dir, &output_file_path, question_policy, - log_sender.clone(), + logger.clone(), )? { files } else { @@ -165,13 +157,13 @@ pub fn decompress_file( let unpack_fn: Box UnpackResult> = if formats.len() > 1 { let mut temp_file = tempfile::NamedTempFile::new()?; io::copy(&mut reader, &mut temp_file)?; - let log_sender_clone = log_sender.clone(); + let logger_clone = logger.clone(); Box::new(move |output_dir| { - crate::archive::rar::unpack_archive(temp_file.path(), output_dir, quiet, log_sender_clone) + crate::archive::rar::unpack_archive(temp_file.path(), output_dir, quiet, logger_clone) }) } else { Box::new(|output_dir| { - crate::archive::rar::unpack_archive(input_file_path, output_dir, quiet, log_sender.clone()) + crate::archive::rar::unpack_archive(input_file_path, output_dir, quiet, logger.clone()) }) }; @@ -180,7 +172,7 @@ pub fn decompress_file( output_dir, &output_file_path, question_policy, - log_sender.clone(), + logger.clone(), )? { files } else { @@ -193,7 +185,7 @@ pub fn decompress_file( } SevenZip => { if formats.len() > 1 { - warn_user_about_loading_sevenz_in_memory(log_sender.clone()); + warn_user_about_loading_sevenz_in_memory(logger.clone()); if !user_wants_to_continue(input_file_path, question_policy, QuestionAction::Decompression)? { return Ok(()); @@ -205,17 +197,12 @@ pub fn decompress_file( if let ControlFlow::Continue(files) = smart_unpack( |output_dir| { - crate::archive::sevenz::decompress_sevenz( - io::Cursor::new(vec), - output_dir, - quiet, - log_sender.clone(), - ) + crate::archive::sevenz::decompress_sevenz(io::Cursor::new(vec), output_dir, quiet, logger.clone()) }, output_dir, &output_file_path, question_policy, - log_sender.clone(), + logger.clone(), )? { files } else { @@ -228,23 +215,14 @@ pub fn decompress_file( // having a final status message is important especially in an accessibility context // as screen readers may not read a commands exit code, making it hard to reason // about whether the command succeeded without such a message - log_sender - .send(PrintMessage { - contents: format!( - "Successfully decompressed archive in {}.", - nice_directory_display(output_dir) - ), - accessible: true, - level: MessageLevel::Info, - }) - .unwrap(); - log_sender - .send(PrintMessage { - contents: format!("Files unpacked: {}", files_unpacked), - accessible: true, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!( + "Successfully decompressed archive in {}.", + nice_directory_display(output_dir) + ), + true, + ); + logger.info(format!("Files unpacked: {}", files_unpacked), true); Ok(()) } @@ -259,22 +237,19 @@ fn smart_unpack( output_dir: &Path, output_file_path: &Path, question_policy: QuestionPolicy, - log_sender: Sender, + logger: Logger, ) -> crate::Result> { assert!(output_dir.exists()); let temp_dir = tempfile::tempdir_in(output_dir)?; let temp_dir_path = temp_dir.path(); - log_sender - .send(PrintMessage { - contents: format!( - "Created temporary directory {} to hold decompressed elements.", - nice_directory_display(temp_dir_path) - ), - accessible: true, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!( + "Created temporary directory {} to hold decompressed elements.", + nice_directory_display(temp_dir_path) + ), + true, + ); let files = unpack_fn(temp_dir_path)?; @@ -293,17 +268,14 @@ fn smart_unpack( } fs::rename(&file_path, &correct_path)?; - log_sender - .send(PrintMessage { - contents: format!( - "Successfully moved {} to {}.", - nice_directory_display(&file_path), - nice_directory_display(&correct_path) - ), - accessible: true, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!( + "Successfully moved {} to {}.", + nice_directory_display(&file_path), + nice_directory_display(&correct_path) + ), + true, + ); } else { // Multiple files in the root directory, so: // Rename the temporary directory to the archive name, which is output_file_path @@ -312,17 +284,14 @@ fn smart_unpack( return Ok(ControlFlow::Break(())); } fs::rename(temp_dir_path, output_file_path)?; - log_sender - .send(PrintMessage { - contents: format!( - "Successfully moved {} to {}.", - nice_directory_display(temp_dir_path), - nice_directory_display(output_file_path) - ), - accessible: true, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info( + format!( + "Successfully moved {} to {}.", + nice_directory_display(temp_dir_path), + nice_directory_display(output_file_path) + ), + true, + ); } Ok(ControlFlow::Continue(files)) diff --git a/src/commands/list.rs b/src/commands/list.rs index c394179..07b7029 100644 --- a/src/commands/list.rs +++ b/src/commands/list.rs @@ -1,7 +1,6 @@ use std::{ io::{self, BufReader, Read}, path::Path, - sync::mpsc::Sender, }; use fs_err as fs; @@ -10,7 +9,7 @@ use crate::{ commands::warn_user_about_loading_zip_in_memory, extension::CompressionFormat::{self, *}, list::{self, FileInArchive, ListOptions}, - utils::{message::PrintMessage, user_wants_to_continue}, + utils::{logger::Logger, user_wants_to_continue}, QuestionAction, QuestionPolicy, BUFFER_CAPACITY, }; @@ -21,7 +20,7 @@ pub fn list_archive_contents( formats: Vec, list_options: ListOptions, question_policy: QuestionPolicy, - log_sender: Sender, + logger: Logger, ) -> crate::Result<()> { let reader = fs::File::open(archive_path)?; @@ -67,7 +66,7 @@ pub fn list_archive_contents( Tar => Box::new(crate::archive::tar::list_archive(tar::Archive::new(reader))), Zip => { if formats.len() > 1 { - warn_user_about_loading_zip_in_memory(log_sender.clone()); + warn_user_about_loading_zip_in_memory(logger.clone()); if !user_wants_to_continue(archive_path, question_policy, QuestionAction::Decompression)? { return Ok(()); @@ -96,7 +95,7 @@ pub fn list_archive_contents( } SevenZip => { if formats.len() > 1 { - warn_user_about_loading_zip_in_memory(log_sender.clone()); + warn_user_about_loading_zip_in_memory(logger.clone()); if !user_wants_to_continue(archive_path, question_policy, QuestionAction::Decompression)? { return Ok(()); } diff --git a/src/commands/mod.rs b/src/commands/mod.rs index e1d90fc..2bb6f3f 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -7,17 +7,13 @@ mod list; use std::{ ops::ControlFlow, path::PathBuf, - sync::{ - mpsc::{channel, Sender}, - Arc, Condvar, Mutex, - }, + sync::{mpsc::channel, Arc, Condvar, Mutex}, }; use rayon::prelude::{IndexedParallelIterator, IntoParallelRefIterator, ParallelIterator}; use utils::colors; use crate::{ - accessible::is_running_in_accessible_mode, check, cli::Subcommand, commands::{compress::compress_files, decompress::decompress_file, list::list_archive_contents}, @@ -26,42 +22,30 @@ use crate::{ list::ListOptions, utils::{ self, - message::{MessageLevel, PrintMessage}, + logger::{map_message, Logger, PrintMessage}, to_utf, EscapedPathDisplay, FileVisibilityPolicy, }, CliArgs, QuestionPolicy, }; /// Warn the user that (de)compressing this .zip archive might freeze their system. -fn warn_user_about_loading_zip_in_memory(log_sender: Sender) { +fn warn_user_about_loading_zip_in_memory(logger: Logger) { const ZIP_IN_MEMORY_LIMITATION_WARNING: &str = "\n\ \tThe format '.zip' is limited and cannot be (de)compressed using encoding streams.\n\ \tWhen using '.zip' with other formats, (de)compression must be done in-memory\n\ \tCareful, you might run out of RAM if the archive is too large!"; - log_sender - .send(PrintMessage { - contents: ZIP_IN_MEMORY_LIMITATION_WARNING.to_string(), - accessible: true, - level: MessageLevel::Warning, - }) - .unwrap(); + logger.warning(ZIP_IN_MEMORY_LIMITATION_WARNING.to_string()); } /// Warn the user that (de)compressing this .7z archive might freeze their system. -fn warn_user_about_loading_sevenz_in_memory(log_sender: Sender) { +fn warn_user_about_loading_sevenz_in_memory(logger: Logger) { const SEVENZ_IN_MEMORY_LIMITATION_WARNING: &str = "\n\ \tThe format '.7z' is limited and cannot be (de)compressed using encoding streams.\n\ \tWhen using '.7z' with other formats, (de)compression must be done in-memory\n\ \tCareful, you might run out of RAM if the archive is too large!"; - log_sender - .send(PrintMessage { - contents: SEVENZ_IN_MEMORY_LIMITATION_WARNING.to_string(), - accessible: true, - level: MessageLevel::Warning, - }) - .unwrap(); + logger.warning(SEVENZ_IN_MEMORY_LIMITATION_WARNING.to_string()); } /// This function checks what command needs to be run and performs A LOT of ahead-of-time checks @@ -74,43 +58,16 @@ pub fn run( file_visibility_policy: FileVisibilityPolicy, ) -> crate::Result<()> { let (log_sender, log_receiver) = channel::(); + let logger = Logger::new(log_sender); let pair = Arc::new((Mutex::new(false), Condvar::new())); let pair2 = Arc::clone(&pair); // Log received messages until all senders are dropped rayon::spawn(move || { - use utils::colors::{ORANGE, RESET, YELLOW}; - const BUFFER_SIZE: usize = 10; let mut buffer = Vec::::with_capacity(BUFFER_SIZE); - // TODO: Move this out to utils - fn map_message(msg: &PrintMessage) -> Option { - match msg.level { - MessageLevel::Info => { - if msg.accessible { - if is_running_in_accessible_mode() { - Some(format!("{}Info:{} {}", *YELLOW, *RESET, msg.contents)) - } else { - Some(format!("{}[INFO]{} {}", *YELLOW, *RESET, msg.contents)) - } - } else if !is_running_in_accessible_mode() { - Some(format!("{}[INFO]{} {}", *YELLOW, *RESET, msg.contents)) - } else { - None - } - } - MessageLevel::Warning => { - if is_running_in_accessible_mode() { - Some(format!("{}Warning:{} ", *ORANGE, *RESET)) - } else { - Some(format!("{}[WARNING]{} ", *ORANGE, *RESET)) - } - } - } - } - loop { let msg = log_receiver.recv(); @@ -124,16 +81,14 @@ pub fn run( tmp.push_str(&msg); } - // TODO: Send this to stderr - println!("{}", tmp); + eprintln!("{}", tmp); buffer.clear(); } else if let Some(msg) = map_message(&msg) { buffer.push(msg); } } else { // All senders have been dropped - // TODO: Send this to stderr - println!("{}", buffer.join("\n")); + eprintln!("{}", buffer.join("\n")); // Wake up the main thread let (lock, cvar) = &*pair2; @@ -164,7 +119,7 @@ pub fn run( let parsed_formats = parse_format(&formats)?; (Some(formats), parsed_formats) } - None => (None, extension::extensions_from_path(&output_path, log_sender.clone())), + None => (None, extension::extensions_from_path(&output_path, logger.clone())), }; check::check_invalid_compression_with_non_archive_format( @@ -197,7 +152,7 @@ pub fn run( question_policy, file_visibility_policy, level, - log_sender.clone(), + logger.clone(), ); if let Ok(true) = compress_result { @@ -205,13 +160,7 @@ pub fn run( // having a final status message is important especially in an accessibility context // as screen readers may not read a commands exit code, making it hard to reason // about whether the command succeeded without such a message - log_sender - .send(PrintMessage { - contents: format!("Successfully compressed '{}'.", to_utf(&output_path)), - accessible: true, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info(format!("Successfully compressed '{}'.", to_utf(&output_path)), true); } else { // If Ok(false) or Err() occurred, delete incomplete file at `output_path` // @@ -250,10 +199,10 @@ pub fn run( } else { for path in files.iter() { let (pathbase, mut file_formats) = - extension::separate_known_extensions_from_name(path, log_sender.clone()); + extension::separate_known_extensions_from_name(path, logger.clone()); if let ControlFlow::Break(_) = - check::check_mime_type(path, &mut file_formats, question_policy, log_sender.clone())? + check::check_mime_type(path, &mut file_formats, question_policy, logger.clone())? { return Ok(()); } @@ -268,7 +217,7 @@ pub fn run( // The directory that will contain the output files // We default to the current directory if the user didn't specify an output directory with --dir let output_dir = if let Some(dir) = output_dir { - utils::create_dir_if_non_existent(&dir, log_sender.clone())?; + utils::create_dir_if_non_existent(&dir, logger.clone())?; dir } else { PathBuf::from(".") @@ -287,7 +236,7 @@ pub fn run( output_file_path, question_policy, args.quiet, - log_sender.clone(), + logger.clone(), ) })?; } @@ -301,10 +250,10 @@ pub fn run( } } else { for path in files.iter() { - let mut file_formats = extension::extensions_from_path(path, log_sender.clone()); + let mut file_formats = extension::extensions_from_path(path, logger.clone()); if let ControlFlow::Break(_) = - check::check_mime_type(path, &mut file_formats, question_policy, log_sender.clone())? + check::check_mime_type(path, &mut file_formats, question_policy, logger.clone())? { return Ok(()); } @@ -323,13 +272,15 @@ pub fn run( println!(); } let formats = extension::flatten_compression_formats(&formats); - list_archive_contents(archive_path, formats, list_options, question_policy, log_sender.clone())?; + list_archive_contents(archive_path, formats, list_options, question_policy, logger.clone())?; } } } - // Drop our sender so when all threads are done, no clones are left - drop(log_sender); + // Drop our sender so when all threads are done, no clones are left. + // This is needed, otherwise the logging thread will never exit since we would be keeping a + // sender alive here. + drop(logger); // Prevent the main thread from exiting until the background thread handling the // logging has set `flushed` to true. diff --git a/src/extension.rs b/src/extension.rs index 612afcc..5ca9cbe 100644 --- a/src/extension.rs +++ b/src/extension.rs @@ -1,14 +1,11 @@ //! Our representation of all the supported compression formats. -use std::{ffi::OsStr, fmt, path::Path, sync::mpsc::Sender}; +use std::{ffi::OsStr, fmt, path::Path}; use bstr::ByteSlice; use self::CompressionFormat::*; -use crate::{ - error::Error, - utils::message::{MessageLevel, PrintMessage}, -}; +use crate::{error::Error, utils::logger::Logger}; pub const SUPPORTED_EXTENSIONS: &[&str] = &[ "tar", @@ -172,7 +169,7 @@ pub fn parse_format(fmt: &OsStr) -> crate::Result> { /// Extracts extensions from a path. /// /// Returns both the remaining path and the list of extension objects -pub fn separate_known_extensions_from_name(path: &Path, log_sender: Sender) -> (&Path, Vec) { +pub fn separate_known_extensions_from_name(path: &Path, logger: Logger) -> (&Path, Vec) { let mut extensions = vec![]; let Some(mut name) = path.file_name().and_then(<[u8] as ByteSlice>::from_os_str) else { @@ -187,15 +184,9 @@ pub fn separate_known_extensions_from_name(path: &Path, log_sender: Sender) -> Vec { - let (_, extensions) = separate_known_extensions_from_name(path, log_sender); +pub fn extensions_from_path(path: &Path, logger: Logger) -> Vec { + let (_, extensions) = separate_known_extensions_from_name(path, logger); extensions } @@ -260,14 +251,16 @@ pub fn build_archive_file_suggestion(path: &Path, suggested_extension: &str) -> #[cfg(test)] mod tests { use super::*; + use crate::utils::logger::PrintMessage; #[test] fn test_extensions_from_path() { let (log_sender, _log_receiver) = std::sync::mpsc::channel::(); + let logger = Logger::new(log_sender); let path = Path::new("bolovo.tar.gz"); - let extensions: Vec = extensions_from_path(path, log_sender); + let extensions: Vec = extensions_from_path(path, logger); let formats: Vec = flatten_compression_formats(&extensions); assert_eq!(formats, vec![Tar, Gzip]); diff --git a/src/utils/fs.rs b/src/utils/fs.rs index 6704cf7..b710305 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -4,15 +4,11 @@ use std::{ env, io::Read, path::{Path, PathBuf}, - sync::mpsc::Sender, }; use fs_err as fs; -use super::{ - message::{MessageLevel, PrintMessage}, - user_wants_to_overwrite, -}; +use super::{logger::Logger, user_wants_to_overwrite}; use crate::{extension::Extension, utils::EscapedPathDisplay, QuestionPolicy}; /// Remove `path` asking the user to overwrite if necessary. @@ -40,18 +36,12 @@ pub fn remove_file_or_dir(path: &Path) -> crate::Result<()> { } /// Creates a directory at the path, if there is nothing there. -pub fn create_dir_if_non_existent(path: &Path, log_sender: Sender) -> crate::Result<()> { +pub fn create_dir_if_non_existent(path: &Path, logger: Logger) -> crate::Result<()> { if !path.exists() { fs::create_dir_all(path)?; // creating a directory is an important change to the file system we // should always inform the user about - log_sender - .send(PrintMessage { - contents: format!("Directory {} created.", EscapedPathDisplay::new(path)), - accessible: true, - level: MessageLevel::Info, - }) - .unwrap(); + logger.info(format!("Directory {} created.", EscapedPathDisplay::new(path)), true); } Ok(()) } diff --git a/src/utils/logger.rs b/src/utils/logger.rs new file mode 100644 index 0000000..bf02c1b --- /dev/null +++ b/src/utils/logger.rs @@ -0,0 +1,75 @@ +use std::sync::mpsc::Sender; + +use super::colors::{ORANGE, RESET, YELLOW}; +use crate::accessible::is_running_in_accessible_mode; + +/// Message object used for sending logs from worker threads to a logging thread via channels. +/// See +#[derive(Debug)] +pub struct PrintMessage { + contents: String, + accessible: bool, + level: MessageLevel, +} + +pub fn map_message(msg: &PrintMessage) -> Option { + match msg.level { + MessageLevel::Info => { + if msg.accessible { + if is_running_in_accessible_mode() { + Some(format!("{}Info:{} {}", *YELLOW, *RESET, msg.contents)) + } else { + Some(format!("{}[INFO]{} {}", *YELLOW, *RESET, msg.contents)) + } + } else if !is_running_in_accessible_mode() { + Some(format!("{}[INFO]{} {}", *YELLOW, *RESET, msg.contents)) + } else { + None + } + } + MessageLevel::Warning => { + if is_running_in_accessible_mode() { + Some(format!("{}Warning:{} ", *ORANGE, *RESET)) + } else { + Some(format!("{}[WARNING]{} ", *ORANGE, *RESET)) + } + } + } +} + +#[derive(Clone)] +pub struct Logger { + log_sender: Sender, +} + +impl Logger { + pub fn new(log_sender: Sender) -> Self { + Self { log_sender } + } + + pub fn info(&self, contents: String, accessible: bool) { + self.log_sender + .send(PrintMessage { + contents, + accessible, + level: MessageLevel::Info, + }) + .unwrap(); + } + + pub fn warning(&self, contents: String) { + self.log_sender + .send(PrintMessage { + contents, + accessible: true, // does not matter + level: MessageLevel::Warning, + }) + .unwrap(); + } +} + +#[derive(Debug, PartialEq)] +pub enum MessageLevel { + Info, + Warning, +} diff --git a/src/utils/message.rs b/src/utils/message.rs deleted file mode 100644 index f5dfac0..0000000 --- a/src/utils/message.rs +++ /dev/null @@ -1,29 +0,0 @@ -/// Message object used for sending logs from worker threads to a logging thread via channels. -/// See -/// -/// ## Example -/// -/// ```rs -/// // This is already done in the main thread in src/commands/mod.rs -/// // Functions that want to log anything just need to have -/// // `log_sender: Sender` as an argument. -/// let (log_sender, log_receiver) = channel::(); -/// -/// log_sender -/// .send(PrintMessage { -/// contents: "Hello, world!".to_string(), -/// accessible: true, -/// }).unwrap(); -/// ``` -#[derive(Debug)] -pub struct PrintMessage { - pub contents: String, - pub accessible: bool, - pub level: MessageLevel, -} - -#[derive(Debug, PartialEq)] -pub enum MessageLevel { - Info, - Warning, -} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index a57180b..f8fd350 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -7,7 +7,7 @@ pub mod colors; mod file_visibility; mod formatting; mod fs; -pub mod message; +pub mod logger; mod question; pub use file_visibility::FileVisibilityPolicy;