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

Jump to bottom

Shrink ProgressDrawTarget size #277

Merged
merged 2 commits into from Apr 29, 2021
Merged

Shrink ProgressDrawTarget size #277

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f29026
Move last_draw and rate out of `Option` to reduce size and simplify code
mibac138 Apr 16, 2021
001200c
Replace DrawTarget::Term::last_state with last_line_count
mibac138 Apr 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 10 additions & 8 deletions src/state.rs
View file
Open in desktop
Expand Up @@ -412,13 +412,16 @@ impl ProgressDrawTarget {
///
/// Will panic if refresh_rate is `Some(0)`. To disable rate limiting use `None` instead.
pub fn term(term: Term, refresh_rate: impl Into<Option<u64>>) -> ProgressDrawTarget {
let rate = refresh_rate.into().map(|x| Duration::from_millis(1000 / x));
let rate = refresh_rate
.into()
.map(|x| Duration::from_millis(1000 / x))
.unwrap_or_else(|| Duration::from_secs(0));
ProgressDrawTarget {
kind: ProgressDrawTargetKind::Term {
term,
last_state: None,
rate,
last_draw: None,
last_draw: Instant::now() - rate,
},
}
}
Expand Down Expand Up @@ -468,9 +471,8 @@ impl ProgressDrawTarget {
} => {
if draw_state.finished
|| draw_state.force_draw
|| rate.is_none()
|| last_draw.is_none()
|| last_draw.unwrap().elapsed() > rate.unwrap()
|| rate == Duration::from_secs(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I honestly don't like this use of a sentinel value. The benefit of saving 8 bytes doesn't really seem worth it to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

|| rate == Duration::from_secs(0) could be removed without affecting behavior (leaving just forced || finished || elapsed > rate), and there is some precedent for this kind of usage of duration, see rust-lang/rust#73544, and rust-lang/rust#84084 which stabilizes Duration::is_zero. However, I'm fine with reverting this part of the PR if that's not convincing

|| last_draw.elapsed() > rate
{
if let Some(ref last_state) = *last_state {
if !draw_state.lines.is_empty() && draw_state.move_cursor {
Expand All @@ -482,7 +484,7 @@ impl ProgressDrawTarget {
draw_state.draw_to_term(term)?;
term.flush()?;
*last_state = Some(draw_state);
*last_draw = Some(Instant::now());
*last_draw = Instant::now();
}
}
ProgressDrawTargetKind::Remote { idx, ref chan, .. } => {
Expand Down Expand Up @@ -524,8 +526,8 @@ pub(crate) enum ProgressDrawTargetKind {
Term {
term: Term,
last_state: Option<ProgressDrawState>,
rate: Option<Duration>,
last_draw: Option<Instant>,
rate: Duration,
last_draw: Instant,
},
Remote {
state: Arc<RwLock<MultiProgressState>>,
Expand Down