Skip to content

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

Merged
djc merged 5 commits intoconsole-rs:mainfrom
chris-laplante:cpl/zombie-prune-clear
Sep 13, 2022
Merged

Rework zombie handling to fix #451 and #464#460
djc merged 5 commits intoconsole-rs:mainfrom
chris-laplante:cpl/zombie-prune-clear

Conversation

@chris-laplante
Copy link
Copy Markdown
Collaborator

@chris-laplante chris-laplante commented Aug 3, 2022 •
edited
Loading

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

chris-laplante commented Aug 3, 2022

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

@mitsuhiko
Copy link
Copy Markdown
Collaborator

mitsuhiko commented Aug 4, 2022

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

chris-laplante commented Aug 4, 2022

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

chris-laplante commented Aug 4, 2022

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 force-pushed the cpl/zombie-prune-clear branch from 6666b48 to 0cd925e Compare August 12, 2022 22:57
@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
@chris-laplante chris-laplante force-pushed the cpl/zombie-prune-clear branch from 0cd925e to ebbba89 Compare August 13, 2022 03:07
@chris-laplante chris-laplante requested a review from djc August 13, 2022 03:07
@chris-laplante chris-laplante force-pushed the cpl/zombie-prune-clear branch from ebbba89 to e776623 Compare August 13, 2022 03:19
@djc
Copy link
Copy Markdown
Member

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

chris-laplante 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?

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
djc reviewed Aug 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@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...

Comment thread src/multi.rs Outdated
Comment thread src/multi.rs Outdated
Comment thread src/multi.rs Outdated
Comment thread src/draw_target.rs Outdated
@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
djc
djc reviewed Aug 18, 2022
View reviewed changes
Comment thread src/multi.rs Outdated
Comment thread src/draw_target.rs Outdated
@djc
Copy link
Copy Markdown
Member

djc commented Aug 26, 2022

@chris-laplante ping?

@chris-laplante
Copy link
Copy Markdown
Collaborator Author

chris-laplante commented Aug 26, 2022

@chris-laplante ping?

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

@chris-laplante chris-laplante requested a review from djc August 27, 2022 13:21
chris-laplante
chris-laplante commented Aug 27, 2022
View reviewed changes
Comment thread src/multi.rs
Comment thread src/draw_target.rs Outdated
Comment thread src/draw_target.rs Outdated
Comment thread src/multi.rs Outdated
Comment thread src/draw_target.rs Outdated
Comment thread src/draw_target.rs Outdated
Comment thread src/draw_target.rs Outdated
Comment thread src/draw_target.rs Outdated
@djc djc mentioned this pull request Aug 28, 2022
Open
@chris-laplante
Copy link
Copy Markdown
Collaborator Author

chris-laplante commented Sep 8, 2022 via email •
edited
Loading

@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
@chris-laplante
rewrite zombie handling
4f20824 
Fixes console-rs#464 and console-rs#451

Possibly fixes console-rs#411 (if not already fixed?)
@chris-laplante
modify render tests to account for fixed MultiProgress::println/clear…
88e288e 
… 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
add new render tests that exercise zombie handling
c2f2c75
@chris-laplante
Copy link
Copy Markdown
Collaborator Author

chris-laplante commented Sep 8, 2022

@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

Reviewers

@djc djc djc approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chris-laplante @mitsuhiko @djc