From 5f31d236ecbd89d565ce1b984a9bc979ea9229d2 Mon Sep 17 00:00:00 2001 From: Chris Laplante Date: Thu, 26 May 2022 18:22:01 -0400 Subject: [PATCH 1/3] add InMemoryTerm::reset() --- src/in_memory.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/in_memory.rs b/src/in_memory.rs index a026da69..1ea16555 100644 --- a/src/in_memory.rs +++ b/src/in_memory.rs @@ -24,6 +24,11 @@ impl InMemoryTerm { } } + pub fn reset(&self) { + let mut state = self.state.lock().unwrap(); + *state = InMemoryTermState::new(state.parser.screen().size().0, state.width); + } + pub fn contents(&self) -> String { let state = self.state.lock().unwrap(); From 7c44e00ac47f3fc9ca6c35d28096bcf2b7a092be Mon Sep 17 00:00:00 2001 From: Chris Laplante Date: Fri, 27 May 2022 15:31:22 -0400 Subject: [PATCH 2/3] MultiProgress: prune 'zombie' progress bars; fixes #426 --- src/draw_target.rs | 18 ++++++++ src/multi.rs | 110 ++++++++++++++++++++++++++++++++------------ src/progress_bar.rs | 4 ++ 3 files changed, 102 insertions(+), 30 deletions(-) diff --git a/src/draw_target.rs b/src/draw_target.rs index dc83e59b..2f650694 100644 --- a/src/draw_target.rs +++ b/src/draw_target.rs @@ -187,6 +187,10 @@ impl ProgressDrawTarget { _ => None, } } + + pub(crate) fn last_line_count(&mut self) -> Option<&mut usize> { + self.kind.last_line_count() + } } #[derive(Debug)] @@ -209,6 +213,20 @@ enum TargetKind { }, } +impl TargetKind { + fn last_line_count(&mut self) -> Option<&mut usize> { + match self { + TargetKind::Term { + last_line_count, .. + } => Some(last_line_count), + TargetKind::TermLike { + last_line_count, .. + } => Some(last_line_count), + _ => None, + } + } +} + pub(crate) enum Drawable<'a> { Term { term: &'a Term, diff --git a/src/multi.rs b/src/multi.rs index cf00c89d..346beb31 100644 --- a/src/multi.rs +++ b/src/multi.rs @@ -1,9 +1,11 @@ +use std::fmt::{Debug, Formatter}; use std::io; -use std::sync::{Arc, RwLock}; +use std::sync::{Arc, Mutex, RwLock, Weak}; use std::time::Instant; use crate::draw_target::{DrawState, DrawStateWrapper, ProgressDrawTarget}; use crate::progress_bar::ProgressBar; +use crate::state::BarState; /// Manages multiple progress bars from different threads #[derive(Debug, Clone)] @@ -128,8 +130,11 @@ impl MultiProgress { } fn internalize(&self, location: InsertLocation, pb: ProgressBar) -> ProgressBar { - let idx = self.state.write().unwrap().insert(location); + let mut state = self.state.write().unwrap(); + + 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 } @@ -168,9 +173,9 @@ impl MultiProgress { #[derive(Debug)] pub(crate) struct MultiState { /// The collection of states corresponding to progress bars - /// the state is None for bars that have not yet been drawn or have been removed - draw_states: Vec>, - /// Set of removed bars, should have corresponding `None` elements in the `draw_states` vector + members: Vec, + /// Set of removed bars, should have corresponding members in the `members` vector with a + /// `draw_state` of `None`. free_set: Vec, /// Indices to the `draw_states` to maintain correct visual order ordering: Vec, @@ -187,7 +192,7 @@ pub(crate) struct MultiState { impl MultiState { fn new(draw_target: ProgressDrawTarget) -> Self { Self { - draw_states: vec![], + members: vec![], free_set: vec![], ordering: vec![], draw_target, @@ -203,6 +208,32 @@ impl MultiState { extra_lines: Option>, now: Instant, ) -> io::Result<()> { + // Reap all consecutive 'zombie' progress bars from head of the list + let mut zombies = vec![]; + let mut adjust = 0; + for index in self.ordering.iter() { + let member = &self.members[*index]; + if !member.is_zombie { + break; + } + + zombies.push(*index); + adjust += member + .draw_state + .as_ref() + .map(|d| d.lines.len()) + .unwrap_or_default(); + } + + for index in zombies { + self.remove_idx(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; + } + let orphan_lines_count = self.orphan_lines.len(); force_draw |= orphan_lines_count > 0; let mut drawable = match self.draw_target.drawable(force_draw, now) { @@ -222,9 +253,15 @@ impl MultiState { draw_state.lines.append(&mut self.orphan_lines); for index in self.ordering.iter() { - if let Some(state) = &self.draw_states[*index] { + let member = &mut 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); @@ -244,20 +281,14 @@ impl MultiState { } pub(crate) fn draw_state(&mut self, idx: usize) -> DrawStateWrapper<'_> { - let (states, orphans) = (&mut self.draw_states, &mut self.orphan_lines); - let state = match states.get_mut(idx) { - Some(Some(draw_state)) => draw_state, - Some(inner) => { - *inner = Some(DrawState::default()); - let state = inner.as_mut().unwrap(); - state.move_cursor = self.move_cursor; - state.alignment = self.alignment; - state - } - _ => unreachable!(), - }; + let member = self.members.get_mut(idx).unwrap(); + let state = member.draw_state.get_or_insert(DrawState { + move_cursor: self.move_cursor, + alignment: self.alignment, + ..Default::default() + }); - DrawStateWrapper::for_multi(state, orphans) + DrawStateWrapper::for_multi(state, &mut self.orphan_lines) } pub(crate) fn is_hidden(&self) -> bool { @@ -278,12 +309,12 @@ impl MultiState { fn insert(&mut self, location: InsertLocation) -> usize { let idx = match self.free_set.pop() { Some(idx) => { - self.draw_states[idx] = None; + self.members[idx] = MultiStateMember::default(); idx } None => { - self.draw_states.push(None); - self.draw_states.len() - 1 + self.members.push(MultiStateMember::default()); + self.members.len() - 1 } }; @@ -328,7 +359,7 @@ impl MultiState { return; } - self.draw_states[idx].take(); + self.members[idx] = MultiStateMember::default(); self.free_set.push(idx); self.ordering.retain(|&x| x != idx); @@ -340,7 +371,26 @@ impl MultiState { } fn len(&self) -> usize { - self.draw_states.len() - self.free_set.len() + self.members.len() - self.free_set.len() + } +} + +#[derive(Default)] +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>, + is_zombie: bool, +} + +impl Debug for MultiStateMember { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.debug_struct("MultiStateElement") + .field("draw_state", &self.draw_state) + .field("is_zombie", &self.is_zombie) + .finish_non_exhaustive() } } @@ -422,19 +472,19 @@ mod tests { let state = mp.state.read().unwrap(); // the removed place for p1 is reused - assert_eq!(state.draw_states.len(), 4); + assert_eq!(state.members.len(), 4); assert_eq!(state.len(), 3); // free_set may contain 1 or 2 match state.free_set.last() { Some(1) => { assert_eq!(state.ordering, vec![0, 2, 3]); - assert!(state.draw_states[1].is_none()); + assert!(state.members[1].draw_state.is_none()); assert_eq!(p4.index().unwrap(), 2); } Some(2) => { assert_eq!(state.ordering, vec![0, 1, 3]); - assert!(state.draw_states[2].is_none()); + assert!(state.members[2].draw_state.is_none()); assert_eq!(p4.index().unwrap(), 1); } _ => unreachable!(), @@ -534,10 +584,10 @@ mod tests { let state = mp.state.read().unwrap(); // the removed place for p1 is reused - assert_eq!(state.draw_states.len(), 2); + assert_eq!(state.members.len(), 2); assert_eq!(state.free_set.len(), 1); assert_eq!(state.len(), 1); - assert!(state.draw_states[0].is_none()); + assert!(state.members[0].draw_state.is_none()); assert_eq!(state.free_set.last(), Some(&0)); assert_eq!(state.ordering, vec![1]); diff --git a/src/progress_bar.rs b/src/progress_bar.rs index c3556574..b5a3915c 100644 --- a/src/progress_bar.rs +++ b/src/progress_bar.rs @@ -266,6 +266,10 @@ 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 From d87f3cc055048ebea713c257d75e4d9fd2d857a4 Mon Sep 17 00:00:00 2001 From: Chris Laplante Date: Fri, 27 May 2022 15:31:53 -0400 Subject: [PATCH 3/3] add render tests for #426 --- tests/render.rs | 241 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 241 insertions(+) diff --git a/tests/render.rs b/tests/render.rs index f6ca2e6d..45416a5b 100644 --- a/tests/render.rs +++ b/tests/render.rs @@ -60,6 +60,84 @@ fn progress_bar_builder_method_order() { ); } +#[test] +fn multi_progress_single_bar_and_leave() { + let in_mem = InMemoryTerm::new(10, 80); + let mp = + MultiProgress::with_draw_target(ProgressDrawTarget::term_like(Box::new(in_mem.clone()))); + + let pb1 = mp.add(ProgressBar::new(10).with_finish(ProgressFinish::AndLeave)); + + assert_eq!(in_mem.contents(), String::new()); + + pb1.tick(); + assert_eq!( + in_mem.contents(), + r#"░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/10"# + ); + + drop(pb1); + assert_eq!( + in_mem.contents(), + r#"██████████████████████████████████████████████████████████████████████████ 10/10"# + ); +} + +#[test] +fn multi_progress_single_bar_and_clear() { + let in_mem = InMemoryTerm::new(10, 80); + let mp = + MultiProgress::with_draw_target(ProgressDrawTarget::term_like(Box::new(in_mem.clone()))); + + let pb1 = mp.add(ProgressBar::new(10)); + + assert_eq!(in_mem.contents(), String::new()); + + pb1.tick(); + assert_eq!( + in_mem.contents(), + r#"░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/10"# + ); + + drop(pb1); + assert_eq!(in_mem.contents(), ""); +} +#[test] +fn multi_progress_two_bars() { + let in_mem = InMemoryTerm::new(10, 80); + let mp = + MultiProgress::with_draw_target(ProgressDrawTarget::term_like(Box::new(in_mem.clone()))); + + let pb1 = mp.add(ProgressBar::new(10).with_finish(ProgressFinish::AndLeave)); + let pb2 = mp.add(ProgressBar::new(5)); + + assert_eq!(in_mem.contents(), String::new()); + + pb1.tick(); + assert_eq!( + in_mem.contents(), + r#"░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/10"# + ); + + pb2.tick(); + + assert_eq!( + in_mem.contents(), + r#" +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/10 +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/5"# + .trim_start() + ); + + drop(pb1); + drop(pb2); + + assert_eq!( + in_mem.contents(), + r#"██████████████████████████████████████████████████████████████████████████ 10/10"# + ); +} + #[test] fn multi_progress() { let in_mem = InMemoryTerm::new(10, 80); @@ -293,3 +371,166 @@ fn manually_inc_ticker() { spinner.set_prefix("prefix"); assert_eq!(in_mem.contents(), "⠉ new message"); } + +#[test] +fn multi_progress_prune_zombies() { + let in_mem = InMemoryTerm::new(10, 80); + let mp = + MultiProgress::with_draw_target(ProgressDrawTarget::term_like(Box::new(in_mem.clone()))); + + let pb0 = mp + .add(ProgressBar::new(10)) + .with_finish(ProgressFinish::AndLeave); + let pb1 = mp.add(ProgressBar::new(15)); + pb0.tick(); + assert_eq!( + in_mem.contents(), + "░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/10" + ); + + pb0.inc(1); + assert_eq!( + in_mem.contents(), + "███████░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 1/10" + ); + + drop(pb0); + + // Clear the screen + in_mem.reset(); + + // 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. + in_mem.write_line("don't erase me plz").unwrap(); + + // pb0 is dead, so only pb1 should be drawn from now on + pb1.tick(); + assert_eq!( + in_mem.contents(), + "don't erase me plz\n░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/15" + ); +} + +#[test] +fn multi_progress_prune_zombies_2() { + let in_mem = InMemoryTerm::new(10, 80); + let mp = + MultiProgress::with_draw_target(ProgressDrawTarget::term_like(Box::new(in_mem.clone()))); + + let pb1 = mp.add(ProgressBar::new(10).with_finish(ProgressFinish::AndLeave)); + let pb2 = mp.add(ProgressBar::new(5)); + let pb3 = mp + .add(ProgressBar::new(100)) + .with_finish(ProgressFinish::Abandon); + let pb4 = mp + .add(ProgressBar::new(500)) + .with_finish(ProgressFinish::AndLeave); + let pb5 = mp.add(ProgressBar::new(7)); + + assert_eq!(in_mem.contents(), String::new()); + + pb1.tick(); + assert_eq!( + in_mem.contents(), + r#"░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/10"# + ); + + pb2.tick(); + + assert_eq!( + in_mem.contents(), + r#" +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/10 +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/5"# + .trim_start() + ); + + pb3.tick(); + assert_eq!( + in_mem.contents(), + r#" +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/10 +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/5 +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/100"# + .trim_start() + ); + + drop(pb1); + drop(pb2); + drop(pb3); + + assert_eq!( + in_mem.contents(), + r#" +██████████████████████████████████████████████████████████████████████████ 10/10 +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/100"# + .trim_start() + ); + + in_mem.reset(); + + // Another sacrificial line we expect shouldn't be touched + in_mem.write_line("don't erase plz").unwrap(); + + mp.println("Test friend :)").unwrap(); + assert_eq!( + in_mem.contents(), + r#" +don't erase plz +Test friend :)"# + .trim_start() + ); + + pb4.tick(); + assert_eq!( + in_mem.contents(), + r#" +don't erase plz +Test friend :) +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/500"# + .trim_start() + ); + + drop(pb4); + + in_mem.reset(); + pb5.tick(); + assert_eq!( + in_mem.contents(), + "░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/7" + ); + + mp.println("not your friend, buddy").unwrap(); + assert_eq!( + in_mem.contents(), + r#" +not your friend, buddy +░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 0/7"# + .trim_start() + ); + + pb5.inc(1); + assert_eq!( + in_mem.contents(), + r#" +not your friend, buddy +██████████░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 1/7"# + .trim_start() + ); + + in_mem.reset(); + in_mem.write_line("don't erase me either").unwrap(); + + pb5.inc(1); + assert_eq!( + in_mem.contents(), + r#" +don't erase me either +█████████████████████░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 2/7"# + .trim_start() + ); + + drop(pb5); + + assert_eq!(in_mem.contents(), "don't erase me either"); +}