diff --git a/src/draw_target.rs b/src/draw_target.rs index a7aae979..ba363d89 100644 --- a/src/draw_target.rs +++ b/src/draw_target.rs @@ -117,6 +117,14 @@ impl ProgressDrawTarget { } } + /// Notifies the backing `MultiProgress` (if applicable) that the associated progress bar should + /// be marked a zombie. + pub(crate) fn mark_zombie(&self) { + if let TargetKind::Multi { idx, state } = &self.kind { + state.write().unwrap().mark_zombie(*idx); + } + } + /// Apply the given draw state (draws it). pub(crate) fn drawable(&mut self, force_draw: bool, now: Instant) -> Option> { match &mut self.kind { @@ -188,8 +196,8 @@ impl ProgressDrawTarget { } } - pub(crate) fn last_line_count(&mut self) -> Option<&mut usize> { - self.kind.last_line_count() + pub(crate) fn adjust_last_line_count(&mut self, adjust: LineAdjust) { + self.kind.adjust_last_line_count(adjust); } } @@ -214,15 +222,21 @@ enum TargetKind { } impl TargetKind { - fn last_line_count(&mut self) -> Option<&mut usize> { - match self { + /// Adjust `last_line_count` such that the next draw operation keeps/clears additional lines + fn adjust_last_line_count(&mut self, adjust: LineAdjust) { + let last_line_count: &mut usize = match self { TargetKind::Term { last_line_count, .. - } => Some(last_line_count), + } => last_line_count, TargetKind::TermLike { last_line_count, .. - } => Some(last_line_count), - _ => None, + } => last_line_count, + _ => return, + }; + + match adjust { + LineAdjust::Clear(count) => *last_line_count = last_line_count.saturating_add(count), + LineAdjust::Keep(count) => *last_line_count = last_line_count.saturating_sub(count), } } } @@ -247,6 +261,24 @@ pub(crate) enum Drawable<'a> { } impl<'a> Drawable<'a> { + /// Adjust `last_line_count` such that the next draw operation keeps/clears additional lines + pub(crate) fn adjust_last_line_count(&mut self, adjust: LineAdjust) { + let last_line_count: &mut usize = match self { + Drawable::Term { + last_line_count, .. + } => last_line_count, + Drawable::TermLike { + last_line_count, .. + } => last_line_count, + _ => return, + }; + + match adjust { + LineAdjust::Clear(count) => *last_line_count = last_line_count.saturating_add(count), + LineAdjust::Keep(count) => *last_line_count = last_line_count.saturating_sub(count), + } + } + pub(crate) fn state(&mut self) -> DrawStateWrapper<'_> { let mut state = match self { Drawable::Term { draw_state, .. } => DrawStateWrapper::for_term(draw_state), @@ -286,6 +318,13 @@ impl<'a> Drawable<'a> { } } +pub(crate) enum LineAdjust { + /// Adds to `last_line_count` so that the next draw also clears those lines + Clear(usize), + /// Subtracts from `last_line_count` so that the next draw retains those lines + Keep(usize), +} + pub(crate) struct DrawStateWrapper<'a> { state: &'a mut DrawState, orphan_lines: Option<&'a mut Vec>, diff --git a/src/multi.rs b/src/multi.rs index bb31da14..bfefcffe 100644 --- a/src/multi.rs +++ b/src/multi.rs @@ -1,11 +1,11 @@ use std::fmt::{Debug, Formatter}; use std::io; -use std::sync::{Arc, Mutex, RwLock, Weak}; +use std::sync::{Arc, RwLock}; +use std::thread::panicking; use std::time::Instant; -use crate::draw_target::{DrawState, DrawStateWrapper, ProgressDrawTarget}; +use crate::draw_target::{DrawState, DrawStateWrapper, LineAdjust, ProgressDrawTarget}; use crate::progress_bar::ProgressBar; -use crate::state::BarState; /// Manages multiple progress bars from different threads #[derive(Debug, Clone)] @@ -134,7 +134,6 @@ impl MultiProgress { let idx = state.insert(location); pb.set_draw_target(ProgressDrawTarget::new_remote(self.state.clone(), idx)); - state.members.get_mut(idx).unwrap().pb = pb.weak_bar_state(); pb } @@ -185,8 +184,11 @@ pub(crate) struct MultiState { move_cursor: bool, /// Controls how the multi progress is aligned if some of its progress bars get removed, default is `Top` alignment: MultiProgressAlignment, - /// Orphaned lines are carried over across draw operations + /// Lines to be drawn above everything else in the MultiProgress. These specifically come from + /// calling `ProgressBar::println` on a pb that is connected to a `MultiProgress`. orphan_lines: Vec, + /// The count of currently visible zombie lines. + zombie_lines_count: usize, } impl MultiState { @@ -199,34 +201,84 @@ impl MultiState { move_cursor: false, alignment: Default::default(), orphan_lines: Vec::new(), + zombie_lines_count: 0, } } + pub(crate) fn mark_zombie(&mut self, index: usize) { + let member = &mut self.members[index]; + + // If the zombie is the first visual bar then we can reap it right now instead of + // deferring it to the next draw. + if index != self.ordering.first().copied().unwrap() { + member.is_zombie = true; + return; + } + + let line_count = member + .draw_state + .as_ref() + .map(|d| d.lines.len()) + .unwrap_or_default(); + + // Track the total number of zombie lines on the screen + self.zombie_lines_count += line_count; + + // Make `DrawTarget` forget about the zombie lines so that they aren't cleared on next draw. + self.draw_target + .adjust_last_line_count(LineAdjust::Keep(line_count)); + + self.remove_idx(index); + } + pub(crate) fn draw( &mut self, mut force_draw: bool, extra_lines: Option>, now: Instant, ) -> io::Result<()> { - // Reap all consecutive 'zombie' progress bars from head of the list + if panicking() { + return Ok(()); + } + + // Assumption: if extra_lines is not None, then it has at least one line + debug_assert_eq!( + extra_lines.is_some(), + extra_lines.as_ref().map(Vec::len).unwrap_or_default() > 0 + ); + + let mut reap_indices = vec![]; + + // Reap all consecutive 'zombie' progress bars from head of the list. let mut adjust = 0; - while let Some(index) = self.ordering.first().copied() { + for &index in self.ordering.iter() { let member = &self.members[index]; if !member.is_zombie { break; } - adjust += member + let line_count = member .draw_state .as_ref() .map(|d| d.lines.len()) .unwrap_or_default(); - self.remove_idx(index); + + // Track the total number of zombie lines on the screen. + self.zombie_lines_count += line_count; + + // Track the number of zombie lines that will be drawn by this call to draw. + adjust += line_count; + + reap_indices.push(index); } - // Adjust last line count so we don't clear too many lines - if let Some(last_line_count) = self.draw_target.last_line_count() { - *last_line_count -= adjust; + // If this draw is due to a `println`, then we need to erase all the zombie lines. + // This is because `println` is supposed to appear above all other elements in the + // `MultiProgress`. + if extra_lines.is_some() { + self.draw_target + .adjust_last_line_count(LineAdjust::Clear(self.zombie_lines_count)); + self.zombie_lines_count = 0; } let orphan_lines_count = self.orphan_lines.len(); @@ -244,23 +296,31 @@ impl MultiState { draw_state.orphan_lines_count += extra_lines.len(); } - // Make orphaned lines appear at the top, so they can be properly forgotten. + // Add lines from `ProgressBar::println` call. draw_state.lines.append(&mut self.orphan_lines); for index in self.ordering.iter() { - let member = &mut self.members[*index]; + let member = &self.members[*index]; if let Some(state) = &member.draw_state { draw_state.lines.extend_from_slice(&state.lines[..]); } - - // Mark the dead progress bar as a zombie - will be reaped on next draw - if member.pb.upgrade().is_none() { - member.is_zombie = true; - } } drop(draw_state); - drawable.draw() + let drawable = drawable.draw(); + + for index in reap_indices.drain(..) { + self.remove_idx(index); + } + + // The zombie lines were drawn for the last time, so make `DrawTarget` forget about them + // so they aren't cleared on next draw. + if extra_lines.is_none() { + self.draw_target + .adjust_last_line_count(LineAdjust::Keep(adjust)); + } + + drawable } pub(crate) fn println>(&mut self, msg: I, now: Instant) -> io::Result<()> { @@ -344,7 +404,12 @@ impl MultiState { fn clear(&mut self, now: Instant) -> io::Result<()> { match self.draw_target.drawable(true, now) { - Some(drawable) => drawable.clear(), + Some(mut drawable) => { + // Make the clear operation also wipe out zombie lines + drawable.adjust_last_line_count(LineAdjust::Clear(self.zombie_lines_count)); + self.zombie_lines_count = 0; + drawable.clear() + } None => Ok(()), } } @@ -375,8 +440,7 @@ struct MultiStateMember { /// Draw state will be `None` for members that haven't been drawn before, or for entries that /// correspond to something in the free set. draw_state: Option, - /// This will be a valid reference unless the containing member is actually in the free set. - pb: Weak>, + /// Whether the corresponding progress bar (more precisely, `BarState`) has been dropped. is_zombie: bool, } diff --git a/src/progress_bar.rs b/src/progress_bar.rs index d33b071e..fb7bf83d 100644 --- a/src/progress_bar.rs +++ b/src/progress_bar.rs @@ -289,10 +289,6 @@ impl ProgressBar { } } - pub(crate) fn weak_bar_state(&self) -> Weak> { - Arc::downgrade(&self.state) - } - /// Resets the ETA calculation /// /// This can be useful if the progress bars made a large jump or was paused for a prolonged diff --git a/src/state.rs b/src/state.rs index b0e87acf..a3228c81 100644 --- a/src/state.rs +++ b/src/state.rs @@ -184,12 +184,17 @@ impl BarState { impl Drop for BarState { fn drop(&mut self) { - // Progress bar is already finished. Do not need to do anything. + // Progress bar is already finished. Do not need to do anything other than notify + // the `MultiProgress` that we're now a zombie. if self.state.is_finished() { + self.draw_target.mark_zombie(); return; } self.finish_using_style(Instant::now(), self.on_finish.clone()); + + // Notify the `MultiProgress` that we're now a zombie. + self.draw_target.mark_zombie(); } } diff --git a/tests/render.rs b/tests/render.rs index c7d56e56..6c561cba 100644 --- a/tests/render.rs +++ b/tests/render.rs @@ -130,6 +130,14 @@ fn multi_progress_two_bars() { ); drop(pb1); + assert_eq!( + in_mem.contents(), + r#" +██████████████████████████████████████████████████████████████████████████ 10/10 +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/5"# + .trim_start() + ); + drop(pb2); assert_eq!( @@ -177,7 +185,24 @@ fn multi_progress() { ); drop(pb1); + assert_eq!( + in_mem.contents(), + r#" +██████████████████████████████████████████████████████████████████████████ 10/10 +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/5 +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/100"# + .trim_start() + ); + drop(pb2); + assert_eq!( + in_mem.contents(), + r#" +██████████████████████████████████████████████████████████████████████████ 10/10 +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/100"# + .trim_start() + ); + drop(pb3); assert_eq!( @@ -397,7 +422,7 @@ fn multi_progress_prune_zombies() { drop(pb0); // Clear the screen - in_mem.reset(); + mp.clear().unwrap(); // Write a line that we expect to remain. This helps ensure the adjustment to last_line_count is // working as expected, and `MultiState` isn't erasing lines when it shouldn't. @@ -467,9 +492,11 @@ fn multi_progress_prune_zombies_2() { .trim_start() ); - in_mem.reset(); + mp.clear().unwrap(); + + assert_eq!(in_mem.contents(), ""); - // Another sacrificial line we expect shouldn't be touched + // A sacrificial line we expect shouldn't be touched in_mem.write_line("don't erase plz").unwrap(); mp.println("Test friend :)").unwrap(); @@ -492,18 +519,34 @@ Test friend :) ); drop(pb4); + assert_eq!( + in_mem.contents(), + r#" +don't erase plz +Test friend :) +████████████████████████████████████████████████████████████████████████ 500/500"# + .trim_start() + ); + + mp.clear().unwrap(); + assert_eq!(in_mem.contents(), "don't erase plz\nTest friend :)"); - in_mem.reset(); pb5.tick(); assert_eq!( in_mem.contents(), - "░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/7" + r#" +don't erase plz +Test friend :) +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/7"# + .trim_start() ); mp.println("not your friend, buddy").unwrap(); assert_eq!( in_mem.contents(), r#" +don't erase plz +Test friend :) not your friend, buddy ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/7"# .trim_start() @@ -513,18 +556,23 @@ not your friend, buddy assert_eq!( in_mem.contents(), r#" +don't erase plz +Test friend :) not your friend, buddy ██████████░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 1/7"# .trim_start() ); - in_mem.reset(); + mp.clear().unwrap(); in_mem.write_line("don't erase me either").unwrap(); pb5.inc(1); assert_eq!( in_mem.contents(), r#" +don't erase plz +Test friend :) +not your friend, buddy don't erase me either █████████████████████░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2/7"# .trim_start() @@ -532,7 +580,15 @@ don't erase me either drop(pb5); - assert_eq!(in_mem.contents(), "don't erase me either"); + assert_eq!( + in_mem.contents(), + r#" +don't erase plz +Test friend :) +not your friend, buddy +don't erase me either"# + .trim_start() + ); } #[test] @@ -594,3 +650,127 @@ fn progress_style_tab_width_unification() { spinner.tick(); assert_eq!(in_mem.contents(), "OK OK"); } + +#[test] +fn multi_progress_clear_println() { + let in_mem = InMemoryTerm::new(10, 80); + let mp = + MultiProgress::with_draw_target(ProgressDrawTarget::term_like(Box::new(in_mem.clone()))); + + mp.println("Test of println").unwrap(); + // Should have no effect + mp.clear().unwrap(); + assert_eq!(in_mem.contents(), "Test of println"); +} + +#[test] +fn multi_progress_clear_zombies_no_ticks() { + _multi_progress_clear_zombies(0); +} + +#[test] +fn multi_progress_clear_zombies_one_tick() { + _multi_progress_clear_zombies(1); +} + +#[test] +fn multi_progress_clear_zombies_two_ticks() { + _multi_progress_clear_zombies(2); +} + +// In the old (broken) implementation, zombie handling sometimes worked differently depending on +// how many draws were between certain operations. Let's make sure that doesn't happen again. +fn _multi_progress_clear_zombies(ticks: usize) { + let in_mem = InMemoryTerm::new(10, 80); + let mp = + MultiProgress::with_draw_target(ProgressDrawTarget::term_like(Box::new(in_mem.clone()))); + let style = ProgressStyle::with_template("{msg}").unwrap(); + + let pb1 = mp.add( + ProgressBar::new_spinner() + .with_style(style.clone()) + .with_message("pb1"), + ); + pb1.tick(); + + let pb2 = mp.add( + ProgressBar::new_spinner() + .with_style(style) + .with_message("pb2"), + ); + + pb2.tick(); + assert_eq!(in_mem.contents(), "pb1\npb2"); + + pb1.finish_with_message("pb1 done"); + drop(pb1); + assert_eq!(in_mem.contents(), "pb1 done\npb2"); + + for _ in 0..ticks { + pb2.tick(); + } + + mp.clear().unwrap(); + assert_eq!(in_mem.contents(), ""); +} + +// This test reproduces examples/multi.rs in a simpler form +#[test] +fn multi_zombie_handling() { + let in_mem = InMemoryTerm::new(10, 80); + let mp = + MultiProgress::with_draw_target(ProgressDrawTarget::term_like(Box::new(in_mem.clone()))); + let style = ProgressStyle::with_template("{msg}").unwrap(); + + let pb1 = mp.add( + ProgressBar::new_spinner() + .with_style(style.clone()) + .with_message("pb1"), + ); + pb1.tick(); + let pb2 = mp.add( + ProgressBar::new_spinner() + .with_style(style.clone()) + .with_message("pb2"), + ); + pb2.tick(); + let pb3 = mp.add( + ProgressBar::new_spinner() + .with_style(style) + .with_message("pb3"), + ); + pb3.tick(); + + mp.println("pb1 done!").unwrap(); + pb1.finish_with_message("done"); + assert_eq!(in_mem.contents(), "pb1 done!\ndone\npb2\npb3"); + drop(pb1); + + assert_eq!(in_mem.contents(), "pb1 done!\ndone\npb2\npb3"); + + pb2.tick(); + assert_eq!(in_mem.contents(), "pb1 done!\ndone\npb2\npb3"); + pb3.tick(); + assert_eq!(in_mem.contents(), "pb1 done!\ndone\npb2\npb3"); + + mp.println("pb3 done!").unwrap(); + assert_eq!(in_mem.contents(), "pb1 done!\npb3 done!\npb2\npb3"); + + pb3.finish_with_message("done"); + drop(pb3); + + pb2.tick(); + + mp.println("pb2 done!").unwrap(); + pb2.finish_with_message("done"); + drop(pb2); + + assert_eq!( + in_mem.contents(), + "pb1 done!\npb3 done!\npb2 done!\ndone\ndone" + ); + + mp.clear().unwrap(); + + assert_eq!(in_mem.contents(), "pb1 done!\npb3 done!\npb2 done!"); +}