Skip to content

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

Merged
chris-laplante merged 5 commits intoconsole-rs:mainfrom
oli-obk:multi_term_height
Jul 31, 2023
Merged

Only render as many lines as the terminal can actually display#563
chris-laplante merged 5 commits intoconsole-rs:mainfrom
oli-obk:multi_term_height

Conversation

@oli-obk
Copy link
Copy Markdown
Contributor

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

fixes #496

@djc djc requested a review from chris-laplante July 19, 2023 16:50
oli-obk
oli-obk commented Jul 19, 2023
View reviewed changes
Comment thread src/record.rs Outdated
@chris-laplante
Copy link
Copy Markdown
Collaborator

chris-laplante commented Jul 20, 2023

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

@djc
Copy link
Copy Markdown
Member

djc commented Jul 20, 2023

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

@oli-obk
Copy link
Copy Markdown
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
Copy Markdown
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

chris-laplante
chris-laplante reviewed Jul 21, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread Cargo.toml
Comment thread Cargo.toml Outdated
Comment thread src/record.rs Outdated
Comment thread src/record.rs Outdated
Comment thread src/record.rs Outdated
Comment thread src/record.rs Outdated
@oli-obk oli-obk force-pushed the multi_term_height branch from c773d02 to 571572a Compare July 24, 2023 10:32
@oli-obk
Copy link
Copy Markdown
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

@oli-obk oli-obk force-pushed the multi_term_height branch from 571572a to 256948f Compare July 25, 2023 07:28
@oli-obk oli-obk mentioned this pull request Jul 25, 2023
Closed
djc
djc reviewed Jul 25, 2023
View reviewed changes
Comment thread examples/multi-many.rs Outdated
Comment thread src/draw_target.rs
@oli-obk oli-obk force-pushed the multi_term_height branch from 256948f to 2a5b033 Compare July 25, 2023 08:48
@bjorn3
Copy link
Copy Markdown

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.

djc
djc approved these changes Jul 25, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@djc djc left a comment •
edited
Loading

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
Copy Markdown
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 oli-obk force-pushed the multi_term_height branch from 2a5b033 to 8347ecb Compare July 25, 2023 11:50
@oli-obk
Copy link
Copy Markdown
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
Copy Markdown
Member

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

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

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

@oli-obk oli-obk mentioned this pull request Jul 27, 2023
Merged
@chris-laplante
Copy link
Copy Markdown
Collaborator

chris-laplante commented Jul 31, 2023

Sorry for the delay. Looks great, thanks again!

@chris-laplante chris-laplante merged commit 505d282 into console-rs:main Jul 31, 2023
@oli-obk oli-obk deleted the multi_term_height branch July 31, 2023 14:54
@chris-laplante chris-laplante mentioned this pull request Aug 3, 2023
Closed
@djc djc mentioned this pull request Sep 7, 2023
Closed
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

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

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

5 participants

@oli-obk @chris-laplante @djc @bjorn3 @smoelius