Repository navigation
Add Printer::try_on_window to avoid panicking on a stale window size - #886
akashchamp wants to merge 1 commit into
Conversation
|
Hi, and thanks for the PR!
This is not currently true: a terminal resize can never cause a panic in this case. The terminal resize event does not directly cause a buffer resize, it's only done at the start of the next cycle. Layout and draw always get the same size. The problem comes from a printer with an output window that exceeds what the buffer provides, and that's what should be fixed, rather than a fallible API that should never fail in practice. I thought we were already enforcing it, but it looks like some parts were missed (no check in |
5525092 to
6cdce00
Compare
Printer::on_window unconditionally .expect()s that the output window fits the backend buffer, even though PrintBuffer::window() already documents and returns None for exactly that mismatch (e.g. a terminal resize between the layout pass and the draw pass). Hitting that documented None case aborts the whole application instead of degrading gracefully, unlike print_at/print_with_width, which already clip to the printable area instead of panicking. Add Printer::try_on_window<F, R>(&self, f: F) -> Option<R>, following the try_* convention already used elsewhere in this crate (e.g. ListView::try_remove, SelectView::try_iter_mut). It skips the closure and returns None when the window doesn't fit, instead of panicking. on_window is now implemented in terms of try_on_window and keeps its existing panicking behavior/documentation for callers who want it. Switch GradientView's internal use of on_window to try_on_window, so the one built-in caller of this API degrades gracefully (skips the gradient overlay for one frame) instead of crashing on a stale-size race, as described in the issue. Fixes gyscos#885
6cdce00 to
5be1d56
Compare
|
Thanks for the explanation, and for fixing the root cause in 9250e09. I've corrected the PR description: it no longer claims a resize race, and it now lists the test I ran after rebasing on current main (cargo test -p cursive_core --lib, 93 passed). try_on_window is now only an optional non-panicking alternative to on_window. If you'd rather not add a fallible API, I'm happy to close this. |
This branch has not been deployed
2 participants
Fixes #885.
Printer::on_window.expect()s that the output window fits the backend buffer, so a printer whose output window exceeds the buffer panics with "printer size exceeds backend size".This adds
Printer::try_on_window<F, R>(&self, f: F) -> Option<R>, which skips the closure and returnsNonewhen the window doesn't fit.on_windowis now built on top of it and keeps its panicking behavior (now documented with a# Panicssection).GradientView, the only in-tree caller ofon_window, usestry_on_windowso the cosmetic gradient overlay is skipped for a frame instead of crashing. A CHANGELOG entry is included.As the maintainer pointed out, a terminal resize cannot cause this mismatch, since layout and draw always use the same size. The root cause (printer output windows exceeding the buffer) is addressed separately upstream in 9250e09; this PR only adds a non-panicking alternative for callers that prefer one.
Testing, after rebasing onto current main:
cargo test -p cursive_core --lib: 93 passed, 0 failed. This includes new unit tests foron_windowpanicking on an undersized buffer and fortry_on_windowreturningNoneorSome(result).