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.

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

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

Merged
merged 5 commits into from Sep 13, 2022

Conversation

chris-laplante
Copy link
Collaborator

@chris-laplante chris-laplante commented Aug 3, 2022

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
Copy link
Collaborator Author

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

@mitsuhiko
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

@chris-laplante chris-laplante changed the title incredibly hacky fix for #451 (do not merge) Rework zombie handling to fix #451 and #464 Aug 12, 2022
@djc
Copy link
Collaborator

djc commented Aug 13, 2022

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
Copy link
Collaborator Author

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.

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

Looking pretty good! I agree that it can't be simplified much further...

src/multi.rs Outdated Show resolved Hide resolved
src/multi.rs Outdated Show resolved Hide resolved
src/multi.rs Outdated Show resolved Hide resolved
src/draw_target.rs Outdated Show resolved Hide resolved
@chris-laplante chris-laplante force-pushed the cpl/zombie-prune-clear branch 2 times, most recently from a9b01d5 to 32a0cd7 Compare August 17, 2022 15:03
src/multi.rs Outdated Show resolved Hide resolved
src/draw_target.rs Outdated Show resolved Hide resolved
@djc
Copy link
Collaborator

djc commented Aug 26, 2022

@chris-laplante ping?

@chris-laplante
Copy link
Collaborator Author

@chris-laplante ping?

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

src/multi.rs Show resolved Hide resolved
src/draw_target.rs Outdated Show resolved Hide resolved
src/draw_target.rs Outdated Show resolved Hide resolved
src/multi.rs Outdated Show resolved Hide resolved
src/draw_target.rs Outdated Show resolved Hide resolved
src/draw_target.rs Outdated Show resolved Hide resolved
src/draw_target.rs Outdated Show resolved Hide resolved
src/draw_target.rs Outdated Show resolved Hide resolved
@chris-laplante
Copy link
Collaborator Author

chris-laplante commented Sep 8, 2022 via email

@chris-laplante chris-laplante force-pushed the cpl/zombie-prune-clear branch 2 times, most recently from be2b10c to f4562b9 Compare September 8, 2022 15:02
Fixes console-rs#464 and console-rs#451

Possibly fixes console-rs#411 (if not already fixed?)
… behavior

Specifically, `clear` no longer clears lines printed by `println`. Also,
changed some render tests to not manually reset the in-memory terminal
since then it gets out of sync with MultiProgress.
@chris-laplante
Copy link
Collaborator Author

@djc should be ready

@djc djc merged commit b65eb85 into console-rs:main Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants