Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document when with_key callbacks get invoked, and in which order #578

Closed
wants to merge 1 commit into from
Closed

Document when with_key callbacks get invoked, and in which order #578

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/style.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,12 @@ impl ProgressStyle {
}

/// Adds a custom key that owns a [`ProgressTracker`] to the template
///
/// The `ProgressTracker::write` method is called each time the key is referenced in the
/// template, in order with other formatting and computation in the template. The caller can
/// rely on this ordering.
///
/// It's safe to call back into the `ProgressBar` from within this callback.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for? I'm not all that comfortable with making this last sentence explicit. Changing the ProgressBar during ProgressTracker::write() is definitely not something I had in mind and really want to spend time considering as something that I should keep working.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think this might get into locking issues? Like, the ProgressTracker impl gets immutable access to the &ProgressState, which lives inside the BarState which is inside an Arc<Mutex<_>> inside the ProgressBar.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this works right now seems to be that (fortunately) the first thing the implementation does is to update its last-updated time with the current one before continuing with the update. Future calls to tick() will now immediately bounce off as the update seems to have just been done.
With that said, we also use the ticker-thread which may change the codepath, but the above is what I could gather by looking at the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProgressBar::set_position() is rate-limited, yes, like ProgressBar::inc(), but ProgressBar::tick() isn't. So this is pretty nuanced, and not something that I want to guarantee to this level of detail going forward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, it would be interesting to know why you want to do this. Presumably you can pass a ProgressBar clone into whatever thing that is making progress and call that outside of the ProgressTracker implementation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great question: what problem is calling set_position() on a clone of the progress-bar from within the with_key() callback actually solving?

We have an ongoing operation which keeps updating an atomic counter which should be mapped to the pos of the progress bar. In order to do it with the current API it's necessary to use a callback which typically is more involved and noisy than passing a reference to an atomic down to where it needs to be. That noisiness was avoided by calling set_position() from within the with_key() callback.

An alternative solution could be to make the atomic counter that's used within the progress bar directly accessible - we use an auto-ticker so communicating via shared state would (seemingly) work.

I hope this context helps.

pub fn with_key<S: ProgressTracker + 'static>(mut self, key: &'static str, f: S) -> Self {
self.format_map.insert(key, Box::new(f));
self
Expand Down