console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.23k stars 240 forks source link

Rework zombie handling to fix #451 and #464 #460

Closed chris-laplante closed 1 year ago

chris-laplante commented 1 year ago

This implements what I described here (except for the resetting upon MultiProgress::clear or MultiProgress::suspend). It is ugly because it is working around a double mutable borrow that occurs when you write the code in the "obvious" way:

fn clear(&mut self, now: Instant) -> io::Result<()> {
    match self.draw_target.drawable(true, now) {
        Some(drawable) => {
            if let Some(last_line_count) = self.draw_target.last_line_count() {
                *last_line_count += self.zombie_line_count;
                self.zombie_line_count = 0;
            }

            drawable.clear()
        }
        None => Ok(()),
    }
}

Ideally, only MultiState/MultiProgress would have to know anything about zombie_line_count, but I fear we may have to push it deeper into the hierarchy. I will do my best to find a way so that doesn't happen, though.

chris-laplante commented 1 year ago

Hi @mitsuhiko, could you please give this a try? It fixed it for me™.

mitsuhiko commented 1 year ago

This fixes the yarnish example, the multi example still produces unexpected output. Potentially other issue?

$ cargo run --example multi
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/examples/multi`
starting!
pb1 is done!
[00:00:02] ########################################     128/128     done
pb3 is done!
chris-laplante commented 1 year ago

This fixes the yarnish example, the multi example still produces unexpected output. Potentially other issue?

$ cargo run --example multi
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/examples/multi`
starting!
pb1 is done!
[00:00:02] ########################################     128/128     done
pb3 is done!

Hmm, at least that one is consistent for me. I'll take a look.

chris-laplante commented 1 year ago

Hmm, at least that one is consistent for me. I'll take a look.

I think this one is broken because as I predicted MultiProgress::println messes up what MultiProgress::clear does later. Of course, I didn't predict it when I implemented println :(. I think the fix is to change the example to not use clear.

djc commented 1 year ago

I think there is probably a way to fix up the original fragment, would that be good enough or do you actually need all the complexity of the current version?

chris-laplante commented 1 year ago

I think there is probably a way to fix up the original fragment, would that be good enough or do you actually need all the complexity of the current version?

I'm all ears if you have a different approach :) but I spent a good 6 hours working on this and couldn't come up with anything simpler.

djc commented 1 year ago

@chris-laplante ping?

chris-laplante commented 1 year ago

@chris-laplante ping?

pong :). I already commented on your reviews above. Was waiting to hear your thoughts.

chris-laplante commented 1 year ago

Sure thing, I will get it done today. Sorry for the delay.

chris-laplante commented 1 year ago

@djc should be ready