Fix #467

[rctlmk]

Possible fix for #467.

[rctlmk]

It is possible to use parking_lot::Mutex to avoid a poisoning problem. Or it doesn't really matter, considering a poisoned mutex implies that something already went wrong elsewhere.

[rctlmk]

I thought of implementing Deref and DerefMut for ArcTracker, but then decided against it. Unfortunately, I forgot to remove those before restoring some stuff (which I removed without thinking too much), hence the force-push.

[djc]

Uh, I thought we'd just add a Sync bound to the ProgressTracker trait. Why can't we do that instead?

[rctlmk]

I forgot to mention it. I'm basing this off test_stateful_tracker(). The test fails if we change TestTracker's declaration to struct TestTracker(String). I assumed that we are always supposed to share mutable references, and wrapping all ProgressTracker objects into Arc/Mutex is one way to do that.

[djc]

Well, we should wrap TestTracker in an Arc<Mutex<_>> then (or its inner String, I guess?).

[rctlmk]

I don't know. Is it supposed to work like it does in that test? Or is the test written like that because the inner String of the TestTracker is wrapped in Arc/Mutex? Cloning a ProgressBar essentially gets us the same bar, but cloning a style returns a cloned value, if it isn't wrapped in some kind of smart pointer.

[djc]

Not sure what you mean exactly. The point here is that I don't want to impose Arc (much less Arc<Mutex>) on the implementer of the trait, so instead we just require the bound for the implementer, and then we have to fix our test type to make sure it can match the constraint.

[rctlmk]

I'm sorry if I'm not making sense, I probably don't get how it should to work. Let's suppose there is this ProgressTracker implementation:

#[derive(Debug, Clone)]
struct TestTracker(String);

impl ProgressTracker for TestTracker {
    fn clone_box(&self) -> Box<dyn ProgressTracker> {
        Box::new(self.clone())
    }

    fn tick(&mut self, state: &ProgressState, _: Instant) {
        self.0.clear();
        self.0.push_str(format!("{} {}", state.len().unwrap(), state.pos()).as_str());
    }

    fn reset(&mut self, _state: &ProgressState, _: Instant) {
        self.0.clear();
    }

    fn write(&self, _state: &ProgressState, w: &mut dyn fmt::Write) {
        w.write_str(self.0.as_str()).unwrap()
    }
}

And, like you said, Arc/Mutex is not imposed on the implementor right now. If we run this:

use crate::ProgressBar;

let pb = ProgressBar::new(1);
pb.set_style(
    ProgressStyle::with_template("{{ {foo} }}")
        .unwrap()
        .with_key("foo", TestTracker(String::default()))
        .progress_chars("#>-"),
);

let mut buf = Vec::new();
let style = pb.clone().style();

style.format_state(&pb.state().state, &mut buf, 16);
assert_eq!(&buf[0], "{  }");
buf.clear();
pb.inc(1);
style.format_state(&pb.state().state, &mut buf, 16);
assert_eq!(&buf[0], "{ 1 1 }");
pb.reset();
buf.clear();
style.format_state(&pb.state().state, &mut buf, 16);
assert_eq!(&buf[0], "{  }");
pb.finish_and_clear();

The first assert_eq! is going to pass, but the second one is going to fail. Does it fail because it is expected to do so, or any ProgressTracker should be able to pass it, regardless of its implementation?

[rctlmk]

Does it fail because it is expected to do so, or any ProgressTracker should be able to pass it, regardless of its implementation?

Probably the former:

/// Get a clone of the current progress bar style.
pub fn style(self) -> ProgressStyle {
    self.state().style.clone()
}

Well, then. If the !Sync trackers are still a concern, then instead of a Sync bound for ProgressTracker we can implement Sync for ProgressStyle manually.

[chris-laplante]

We can close this then, right?