search

console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.47k stars 244 forks source link

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

Closed oli-obk closed 1 year ago

oli-obk commented 1 year ago

fixes #496

chris-laplante commented 1 year ago

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

djc commented 1 year ago

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

oli-obk commented 1 year ago

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 commented 1 year ago

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

oli-obk commented 1 year ago

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

bjorn3 commented 1 year ago

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.

oli-obk commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

chris-laplante commented 1 year ago

@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 commented 1 year ago

Sorry for the delay. Looks great, thanks again!