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

Only render as many lines as the terminal can actually display #563

Merged
merged 5 commits into from Jul 31, 2023

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jul 19, 2023

fixes #496

@djc djc requested a review from chris-laplante July 19, 2023 16:50
src/record.rs Outdated Show resolved Hide resolved
@chris-laplante
Copy link
Collaborator

@oli-obk thank you for working on this, I appreciate it greatly! I will review today or tomorrow :)

@djc
Copy link
Collaborator

djc commented Jul 20, 2023

@oli-obk I'm curious about your use case for indicatif?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 20, 2023

I am rewriting compiletest-rs (https://github.com/oli-obk/ui_test). Similar to regular Rust ui tests, it currently has a --quiet mode where it prints one . per test that was run. I have a WIP PR that switches to indicatif displaying a progress bar. But when I added displaying a spinner per currently running test below that, this became an issue, as the test runner uses all cores to run one test per core, and I tested this on a 96 core cloud machine 😆

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 20, 2023

Note that this PR appears to not fully fix the issue (I got the progress bar moved upwards and then everything fixed itself for about 15 times while running >200 tests). I have tried to debug that with the dummy terminal I added, but I think that nothing is actually wrong, and this may just be an issue because I'm running everything across ssh and there are some delays sometimes

Copy link
Collaborator

@chris-laplante chris-laplante left a comment

Choose a reason for hiding this comment

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

Looks really good! I can tell you've put a ton of work into this :), and we (@djc and I) really appreciate it. I think just a couple small things to clean up.

I tested the multi-many example on my own laptop and it worked fine, even as I was resizing the terminal.

Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/record.rs Outdated Show resolved Hide resolved
src/record.rs Outdated Show resolved Hide resolved
src/record.rs Outdated Show resolved Hide resolved
src/record.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 24, 2023

I made the tests simpler (they are just strings now that you can mostly copy out of the assertion failure message).

All other reviews should also have been addressed

examples/multi-many.rs Outdated Show resolved Hide resolved
src/draw_target.rs Show resolved Hide resolved
@bjorn3
Copy link

bjorn3 commented Jul 25, 2023

Does this handle shrinking the vertical size of the terminal? When doing so the terminal is temporarily 1 line smaller than indicatif expected when rendering until the next time the size is queried and everything is rendered again. Leaving a line as buffer may help with this.

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.

Thanks!

Minor nit if you want to clean it up: some of the commit messages are now slightly wrong respective to the commit contents: "Add a test terminal" is more like "Add history to the in-memory terminal" and "Edit an existing test" is more like "Edit an existing example".

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 25, 2023

Does this handle shrinking the vertical size of the terminal? When doing so the terminal is temporarily 1 line smaller than indicatif expected when rendering until the next time the size is queried and everything is rendered again. Leaving a line as buffer may help with this.

I tried it out and resizing is indeed an issue. I don't think leaving an extra line as buffer is going to significantly help us here, and I'd rather look into this in general

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 25, 2023

Thanks!

Minor nit if you want to clean it up: some of the commit messages are now slightly wrong respective to the commit contents: "Add a test terminal" is more like "Add history to the in-memory terminal" and "Edit an existing test" is more like "Edit an existing example".

done

@djc djc requested a review from chris-laplante July 27, 2023 10:00
@djc
Copy link
Collaborator

djc commented Jul 27, 2023

@chris-laplante do you want to take another look? Would be good to release this along with the other recent fixes.

@chris-laplante
Copy link
Collaborator

@chris-laplante do you want to take another look? Would be good to release this along with the other recent fixes.

Yes, I'll take a quick look this morning.

@chris-laplante
Copy link
Collaborator

Sorry for the delay. Looks great, thanks again!

@chris-laplante chris-laplante merged commit 505d282 into console-rs:main Jul 31, 2023
9 checks passed
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.

If the number of progress bar exceed terminal height, newlines appears instead of clearing itself.
5 participants