Merge pull request #280 from figsoda/rewrite-progress

rewrite progress module
2025-06-06 19:45:29 +00:00 · 2022-10-13 15:00:57 -03:00 · 2022-10-13 15:00:57 -03:00 · 385a630d0e
commit 385a630d0e
parent b64d0c0c55 889f1d9c35
9 changed files with 139 additions and 211 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -98,6 +98,7 @@ Categories Used:
 - Clean up the description for the `-d/--dir` argument to `decompress` [\#264](https://github.com/ouch-org/ouch/pull/264) ([hivehand](https://github.com/hivehand))
 - Show subcommand aliases on --help [\#275](https://github.com/ouch-org/ouch/pull/275) ([marcospb19](https://github.com/marcospb19))
 - Update dependencies [\#276](https://github.com/ouch-org/ouch/pull/276) ([figsoda](https://github.com/figsoda))
+- Rewrite progress module [\#280](https://github.com/ouch-org/ouch/pull/280) ([figsoda](https://github.com/figsoda))

 ### New Contributors

--- a/src/archive/tar.rs
+++ b/src/archive/tar.rs
@ -15,6 +15,7 @@ use crate::{
    error::FinalError,
    info,
    list::FileInArchive,
+    progress::OutputLine,
    utils::{self, FileVisibilityPolicy},
 };

@ -23,7 +24,7 @@ use crate::{
 pub fn unpack_archive(
    reader: Box<dyn Read>,
    output_folder: &Path,
-    mut display_handle: impl Write,
+    mut log_out: impl OutputLine,
 ) -> crate::Result<Vec<PathBuf>> {
    assert!(output_folder.read_dir().expect("dir exists").count() == 0);
    let mut archive = tar::Archive::new(reader);
@ -41,7 +42,7 @@ pub fn unpack_archive(
        // and so on

        info!(
-            @display_handle,
+            @log_out,
            inaccessible,
            "{:?} extracted. ({})",
            utils::strip_cur_dir(&output_folder.join(file.path()?)),
@ -88,11 +89,11 @@ pub fn build_archive_from_paths<W, D>(
    input_filenames: &[PathBuf],
    writer: W,
    file_visibility_policy: FileVisibilityPolicy,
-    mut display_handle: D,
+    mut log_out: D,
 ) -> crate::Result<W>
 where
    W: Write,
-    D: Write,
+    D: OutputLine,
 {
    let mut builder = tar::Builder::new(writer);

@ -110,7 +111,7 @@ where
            // little importance for most users, but would generate lots of
            // spoken text for users using screen readers, braille displays
            // and so on
-            info!(@display_handle, inaccessible, "Compressing '{}'.", utils::to_utf(path));
+            info!(@log_out, inaccessible, "Compressing '{}'.", utils::to_utf(path));

            if path.is_dir() {
                builder.append_dir(path, path)?;
--- a/src/archive/zip.rs
+++ b/src/archive/zip.rs
@ -20,6 +20,7 @@ use crate::{
    error::FinalError,
    info,
    list::FileInArchive,
+    progress::OutputLine,
    utils::{
        self, cd_into_same_dir_as, get_invalid_utf8_paths, pretty_format_list_of_paths, strip_cur_dir, to_utf,
        FileVisibilityPolicy,
@ -31,11 +32,11 @@ use crate::{
 pub fn unpack_archive<R, D>(
    mut archive: ZipArchive<R>,
    output_folder: &Path,
-    mut display_handle: D,
+    mut log_out: D,
 ) -> crate::Result<Vec<PathBuf>>
 where
    R: Read + Seek,
-    D: Write,
+    D: OutputLine,
 {
    assert!(output_folder.read_dir().expect("dir exists").count() == 0);

@ -58,7 +59,7 @@ where
                // importance for most users, but would generate lots of
                // spoken text for users using screen readers, braille displays
                // and so on
-                info!(@display_handle, inaccessible, "File {} extracted to \"{}\"", idx, file_path.display());
+                info!(@log_out, inaccessible, "File {} extracted to \"{}\"", idx, file_path.display());
                fs::create_dir_all(&file_path)?;
            }
            _is_file @ false => {
@ -71,7 +72,7 @@ where

                // same reason is in _is_dir: long, often not needed text
                info!(
-                    @display_handle,
+                    @log_out,
                    inaccessible,
                    "{:?} extracted. ({})",
                    file_path.display(),
@ -139,11 +140,11 @@ pub fn build_archive_from_paths<W, D>(
    input_filenames: &[PathBuf],
    writer: W,
    file_visibility_policy: FileVisibilityPolicy,
-    mut display_handle: D,
+    mut log_out: D,
 ) -> crate::Result<W>
 where
    W: Write + Seek,
-    D: Write,
+    D: OutputLine,
 {
    let mut writer = zip::ZipWriter::new(writer);
    let options = zip::write::FileOptions::default();
@ -179,7 +180,7 @@ where
            // little importance for most users, but would generate lots of
            // spoken text for users using screen readers, braille displays
            // and so on
-            info!(@display_handle, inaccessible, "Compressing '{}'.", to_utf(path));
+            info!(@log_out, inaccessible, "Compressing '{}'.", to_utf(path));

            let metadata = match path.metadata() {
                Ok(metadata) => metadata,
--- a/src/commands/compress.rs
+++ b/src/commands/compress.rs
@ -1,11 +1,12 @@
 use std::{
-    io::{self, BufWriter, Write},
+    io::{self, BufWriter, Cursor, Seek, Write},
    path::{Path, PathBuf},
 };

 use fs_err as fs;

 use crate::{
+    accessible::is_running_in_accessible_mode,
    archive,
    commands::warn_user_about_loading_zip_in_memory,
    extension::{
@ -42,12 +43,6 @@ pub fn compress_files(
            (total_size + size, and_precise & precise)
        });

-    // NOTE: canonicalize is here to avoid a weird bug:
-    //      > If output_file_path is a nested path and it exists and the user overwrite it
-    //      >> output_file_path.exists() will always return false (somehow)
-    //      - canonicalize seems to fix this
-    let output_file_path = output_file.path().canonicalize()?;
-
    let file_writer = BufWriter::with_capacity(BUFFER_CAPACITY, output_file);

    let mut writer: Box<dyn Write> = Box::new(file_writer);
@ -80,37 +75,28 @@ pub fn compress_files(

    match first_format {
        Gzip | Bzip | Lz4 | Lzma | Snappy | Zstd => {
-            let _progress = Progress::new_accessible_aware(
-                total_input_size,
-                precise,
-                Some(Box::new(move || {
-                    output_file_path.metadata().expect("file exists").len()
-                })),
-            );
-
            writer = chain_writer_encoder(&first_format, writer)?;
            let mut reader = fs::File::open(&files[0]).unwrap();
-            io::copy(&mut reader, &mut writer)?;
+
+            if is_running_in_accessible_mode() {
+                io::copy(&mut reader, &mut writer)?;
+            } else {
+                io::copy(
+                    &mut Progress::new(total_input_size, precise, true).wrap_read(reader),
+                    &mut writer,
+                )?;
+            }
        }
        Tar => {
-            let mut progress = Progress::new_accessible_aware(
-                total_input_size,
-                precise,
-                Some(Box::new(move || {
-                    output_file_path.metadata().expect("file exists").len()
-                })),
-            );
-
-            archive::tar::build_archive_from_paths(
-                &files,
-                &mut writer,
-                file_visibility_policy,
-                progress
-                    .as_mut()
-                    .map(Progress::display_handle)
-                    .unwrap_or(&mut io::stdout()),
-            )?;
-            writer.flush()?;
+            if is_running_in_accessible_mode() {
+                archive::tar::build_archive_from_paths(&files, &mut writer, file_visibility_policy, io::stderr())?;
+                writer.flush()?;
+            } else {
+                let mut progress = Progress::new(total_input_size, precise, true);
+                let mut writer = progress.wrap_write(writer);
+                archive::tar::build_archive_from_paths(&files, &mut writer, file_visibility_policy, &mut progress)?;
+                writer.flush()?;
+            }
        }
        Zip => {
            if !formats.is_empty() {
@ -122,34 +108,18 @@ pub fn compress_files(
                }
            }

-            let mut vec_buffer = io::Cursor::new(vec![]);
+            let mut vec_buffer = Cursor::new(vec![]);

-            let current_position_fn = {
-                let vec_buffer_ptr = {
-                    struct FlyPtr(*const io::Cursor<Vec<u8>>);
-                    unsafe impl Send for FlyPtr {}
-                    FlyPtr(&vec_buffer as *const _)
-                };
-                Box::new(move || {
-                    let vec_buffer_ptr = &vec_buffer_ptr;
-                    // Safety: ptr is valid and vec_buffer is still alive
-                    unsafe { &*vec_buffer_ptr.0 }.position()
-                })
-            };
-
-            let mut progress = Progress::new_accessible_aware(total_input_size, precise, Some(current_position_fn));
-
-            archive::zip::build_archive_from_paths(
-                &files,
-                &mut vec_buffer,
-                file_visibility_policy,
-                progress
-                    .as_mut()
-                    .map(Progress::display_handle)
-                    .unwrap_or(&mut io::stdout()),
-            )?;
-            let vec_buffer = vec_buffer.into_inner();
-            io::copy(&mut vec_buffer.as_slice(), &mut writer)?;
+            if is_running_in_accessible_mode() {
+                archive::zip::build_archive_from_paths(&files, &mut vec_buffer, file_visibility_policy, io::stderr())?;
+                vec_buffer.rewind()?;
+                io::copy(&mut vec_buffer, &mut writer)?;
+            } else {
+                let mut progress = Progress::new(total_input_size, precise, true);
+                archive::zip::build_archive_from_paths(&files, &mut vec_buffer, file_visibility_policy, &mut progress)?;
+                vec_buffer.rewind()?;
+                io::copy(&mut progress.wrap_read(vec_buffer), &mut writer)?;
+            }
        }
    }

--- a/src/commands/decompress.rs
+++ b/src/commands/decompress.rs
@ -1,5 +1,5 @@
 use std::{
-    io::{self, BufReader, Read, Write},
+    io::{self, BufReader, Read},
    ops::ControlFlow,
    path::{Path, PathBuf},
 };
@ -7,6 +7,7 @@ use std::{
 use fs_err as fs;

 use crate::{
+    accessible::is_running_in_accessible_mode,
    commands::warn_user_about_loading_zip_in_memory,
    extension::{
        split_first_compression_format,
@ -14,7 +15,7 @@ use crate::{
        Extension,
    },
    info,
-    progress::Progress,
+    progress::{OutputLine, Progress},
    utils::{self, nice_directory_display, user_wants_to_continue},
    QuestionAction, QuestionPolicy, BUFFER_CAPACITY,
 };
@ -110,13 +111,15 @@ pub fn decompress_file(
            }
            let mut writer = writer.unwrap();

-            let current_position_fn = Box::new({
-                let output_file_path = output_file_path.clone();
-                move || output_file_path.clone().metadata().expect("file exists").len()
-            });
-            let _progress = Progress::new_accessible_aware(total_input_size, true, Some(current_position_fn));
+            if is_running_in_accessible_mode() {
+                io::copy(&mut reader, &mut writer)?;
+            } else {
+                io::copy(
+                    &mut Progress::new(total_input_size, true, true).wrap_read(reader),
+                    &mut writer,
+                )?;
+            }

-            io::copy(&mut reader, &mut writer)?;
            vec![output_file_path]
        }
        Tar => {
@ -180,7 +183,7 @@ pub fn decompress_file(
 ///   output_dir named after the archive (given by `output_file_path`)
 /// Note: This functions assumes that `output_dir` exists
 fn smart_unpack(
-    unpack_fn: impl FnOnce(&Path, &mut dyn Write) -> crate::Result<Vec<PathBuf>>,
+    unpack_fn: impl FnOnce(&Path, &mut dyn OutputLine) -> crate::Result<Vec<PathBuf>>,
    total_input_size: u64,
    output_dir: &Path,
    output_file_path: &Path,
@ -196,13 +199,11 @@ fn smart_unpack(
    );

    // unpack the files
-    let files = unpack_fn(
-        temp_dir_path,
-        Progress::new_accessible_aware(total_input_size, true, None)
-            .as_mut()
-            .map(Progress::display_handle)
-            .unwrap_or(&mut io::stdout()),
-    )?;
+    let files = if is_running_in_accessible_mode() {
+        unpack_fn(temp_dir_path, &mut io::stderr())
+    } else {
+        unpack_fn(temp_dir_path, &mut Progress::new(total_input_size, true, false))
+    }?;

    let root_contains_only_one_element = fs::read_dir(temp_dir_path)?.count() == 1;
    if root_contains_only_one_element {
--- a/src/commands/mod.rs
+++ b/src/commands/mod.rs
@ -5,7 +5,6 @@ mod decompress;
 mod list;

 use std::{
-    io::Write,
    ops::ControlFlow,
    path::{Path, PathBuf},
 };
@ -19,6 +18,7 @@ use crate::{
    extension::{self, flatten_compression_formats, Extension, SUPPORTED_EXTENSIONS},
    info,
    list::ListOptions,
+    progress::OutputLine,
    utils::{
        self, dir_is_empty, pretty_format_list_of_paths, to_utf, try_infer_extension, user_wants_to_continue,
        FileVisibilityPolicy,
--- a/src/macros.rs
+++ b/src/macros.rs
@ -18,42 +18,33 @@ use crate::accessible::is_running_in_accessible_mode;
 ///   ability to skip some lines deemed not important like a seeing person would.
 ///
 /// By default `info` outputs to Stdout, if you want to specify the output you can use
-/// `@display_handle` modifier
+/// `@log_out` modifier

 #[macro_export]
 macro_rules! info {
    // Accessible (short/important) info message.
    // Show info message even in ACCESSIBLE mode
    (accessible, $($arg:tt)*) => {
-        info!(@::std::io::stdout(), accessible, $($arg)*);
+        info!(@::std::io::stderr(), accessible, $($arg)*);
    };
-    (@$display_handle: expr, accessible, $($arg:tt)*) => {
-        let display_handle = &mut $display_handle;
+    (@$log_out: expr, accessible, $($arg:tt)*) => {{
        // if in ACCESSIBLE mode, suppress the "[INFO]" and just print the message
        if !$crate::accessible::is_running_in_accessible_mode() {
-            $crate::macros::_info_helper(display_handle);
+            $log_out.output_line_info(format_args!($($arg)*));
+        } else {
+            $log_out.output_line(format_args!($($arg)*));
        }
-        writeln!(display_handle, $($arg)*).unwrap();
-    };
+    }};
    // Inccessible (long/no important) info message.
    // Print info message if ACCESSIBLE is not turned on
    (inaccessible, $($arg:tt)*) => {
-        info!(@::std::io::stdout(), inaccessible, $($arg)*);
+        info!(@::std::io::stderr(), inaccessible, $($arg)*);
    };
-    (@$display_handle: expr, inaccessible, $($arg:tt)*) => {
+    (@$log_out: expr, inaccessible, $($arg:tt)*) => {{
        if !$crate::accessible::is_running_in_accessible_mode() {
-            let display_handle = &mut $display_handle;
-            $crate::macros::_info_helper(display_handle);
-            writeln!(display_handle, $($arg)*).unwrap();
+            $log_out.output_line_info(format_args!($($arg)*));
        }
-    };
-}
-
-/// Helper to display "\[INFO\]", colored yellow
-pub fn _info_helper(handle: &mut impl std::io::Write) {
-    use crate::utils::colors::{RESET, YELLOW};
-
-    write!(handle, "{}[INFO]{} ", *YELLOW, *RESET).unwrap();
+    }};
 }

 /// Macro that prints \[WARNING\] messages, wraps [`eprintln`].
--- a/src/progress.rs
+++ b/src/progress.rs
@ -1,122 +1,85 @@
 //! Module that provides functions to display progress bars for compressing and decompressing files.
 use std::{
-    io,
-    sync::mpsc::{self, Receiver, Sender},
-    thread,
-    time::Duration,
+    fmt::Arguments,
+    io::{Read, Stderr, Write},
 };

-use indicatif::{ProgressBar, ProgressStyle};
+use indicatif::{ProgressBar, ProgressBarIter, ProgressStyle};

-use crate::accessible::is_running_in_accessible_mode;
+use crate::utils::colors::{RESET, YELLOW};

 /// Draw a ProgressBar using a function that checks periodically for the progress
 pub struct Progress {
-    draw_stop: Sender<()>,
-    clean_done: Receiver<()>,
-    display_handle: DisplayHandle,
+    bar: ProgressBar,
 }

-/// Writes to this struct will be displayed on the progress bar or stdout depending on the
-/// ProgressBarPolicy
-struct DisplayHandle {
-    buf: Vec<u8>,
-    sender: Sender<String>,
+pub trait OutputLine {
+    fn output_line(&mut self, args: Arguments);
+    fn output_line_info(&mut self, args: Arguments);
 }
-impl io::Write for DisplayHandle {
-    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
-        self.buf.extend(buf);
-        // Newline is the signal to flush
-        if matches!(buf.last(), Some(&b'\n')) {
-            self.buf.pop();
-            self.flush()?;
-        }
-        Ok(buf.len())
+
+impl OutputLine for Progress {
+    fn output_line(&mut self, args: Arguments) {
+        self.bar.set_message(args.to_string());
    }

-    fn flush(&mut self) -> io::Result<()> {
-        fn io_error<X>(_: X) -> io::Error {
-            io::Error::new(io::ErrorKind::Other, "failed to flush buffer")
-        }
-        self.sender
-            .send(String::from_utf8(self.buf.drain(..).collect()).map_err(io_error)?)
-            .map_err(io_error)
+    fn output_line_info(&mut self, args: Arguments) {
+        self.bar.set_message(format!("{}[INFO]{} {args}", *YELLOW, *RESET));
+    }
+}
+
+impl OutputLine for Stderr {
+    fn output_line(&mut self, args: Arguments) {
+        self.write_fmt(args).unwrap();
+    }
+
+    fn output_line_info(&mut self, args: Arguments) {
+        write!(self, "{}[INFO]{} {args}", *YELLOW, *RESET).unwrap();
+        self.write_fmt(args).unwrap();
+        self.write_all(b"\n").unwrap();
+    }
+}
+
+impl<T: OutputLine + ?Sized> OutputLine for &mut T {
+    fn output_line(&mut self, args: Arguments) {
+        (*self).output_line(args)
+    }
+
+    fn output_line_info(&mut self, args: Arguments) {
+        (*self).output_line_info(args);
    }
 }

 impl Progress {
-    /// Create a ProgressBar using a function that checks periodically for the progress
-    /// If precise is true, the total_input_size will be displayed as the total_bytes size
-    /// If ACCESSIBLE is set, this function returns None
-    pub fn new_accessible_aware(
-        total_input_size: u64,
-        precise: bool,
-        current_position_fn: Option<Box<dyn Fn() -> u64 + Send>>,
-    ) -> Option<Self> {
-        if is_running_in_accessible_mode() {
-            return None;
-        }
-        Some(Self::new(total_input_size, precise, current_position_fn))
-    }
-
-    fn new(total_input_size: u64, precise: bool, current_position_fn: Option<Box<dyn Fn() -> u64 + Send>>) -> Self {
-        let (draw_tx, draw_rx) = mpsc::channel();
-        let (clean_tx, clean_rx) = mpsc::channel();
-        let (msg_tx, msg_rx) = mpsc::channel();
-
-        thread::spawn(move || {
-            let template = {
-                let mut t = String::new();
-                t += "{wide_msg} [{elapsed_precise}] ";
-                if precise && current_position_fn.is_some() {
-                    t += "[{bar:.cyan/blue}] ";
-                } else {
-                    t += "{spinner:.green} ";
-                }
-                if current_position_fn.is_some() {
-                    t += "{bytes}/ ";
-                }
-                if precise {
-                    t += "{total_bytes} ";
-                }
-                t += "({bytes_per_sec}, {eta}) {path}";
-                t
-            };
-            let bar = ProgressBar::new(total_input_size)
-                .with_style(ProgressStyle::with_template(&template).unwrap().progress_chars("#>-"));
-
-            while draw_rx.try_recv().is_err() {
-                if let Some(ref pos_fn) = current_position_fn {
-                    bar.set_position(pos_fn());
-                } else {
-                    bar.tick();
-                }
-                if let Ok(msg) = msg_rx.try_recv() {
-                    bar.set_message(msg);
-                }
-                thread::sleep(Duration::from_millis(100));
+    pub(crate) fn new(total_input_size: u64, precise: bool, position_updates: bool) -> Self {
+        let template = {
+            let mut t = String::new();
+            t += "{wide_msg} [{elapsed_precise}] ";
+            if precise && position_updates {
+                t += "[{bar:.cyan/blue}] ";
+            } else {
+                t += "{spinner:.green} ";
            }
-            bar.finish();
-            let _ = clean_tx.send(());
-        });
+            if position_updates {
+                t += "{bytes}/ ";
+            }
+            if precise {
+                t += "{total_bytes} ";
+            }
+            t += "({bytes_per_sec}, {eta}) {path}";
+            t
+        };
+        let bar = ProgressBar::new(total_input_size)
+            .with_style(ProgressStyle::with_template(&template).unwrap().progress_chars("#>-"));

-        Progress {
-            draw_stop: draw_tx,
-            clean_done: clean_rx,
-            display_handle: DisplayHandle {
-                buf: Vec::new(),
-                sender: msg_tx,
-            },
-        }
+        Progress { bar }
    }

-    pub(crate) fn display_handle(&mut self) -> &mut dyn io::Write {
-        &mut self.display_handle
-    }
-}
-impl Drop for Progress {
-    fn drop(&mut self) {
-        let _ = self.draw_stop.send(());
-        let _ = self.clean_done.recv();
+    pub(crate) fn wrap_read<R: Read>(&self, read: R) -> ProgressBarIter<R> {
+        self.bar.wrap_read(read)
+    }
+
+    pub(crate) fn wrap_write<W: Write>(&self, write: W) -> ProgressBarIter<W> {
+        self.bar.wrap_write(write)
    }
 }
--- a/src/utils/fs.rs
+++ b/src/utils/fs.rs
@ -3,14 +3,14 @@
 use std::{
    env,
    fs::ReadDir,
-    io::{Read, Write},
+    io::Read,
    path::{Path, PathBuf},
 };

 use fs_err as fs;

 use super::{to_utf, user_wants_to_overwrite};
-use crate::{extension::Extension, info, QuestionPolicy};
+use crate::{extension::Extension, info, progress::OutputLine, QuestionPolicy};

 /// Checks if given path points to an empty directory.
 pub fn dir_is_empty(dir_path: &Path) -> bool {