-
Notifications
You must be signed in to change notification settings - Fork 284
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
If ioctl TIOCGWINSZ returns 0,0 try to use env vars instead. #893
base: master
Are you sure you want to change the base?
Conversation
For some terminals (for example Emacs eshell/eterm) the ioctl with TIOCGWINSZ might falsely return 0 columns and 0 rows. If this happens we now try to use LINES and COLUMNS environment variables. Fixes crossterm-rs#891
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea if there are perf concerns to checking the environment variables compare with ioctl call. (Context: Ratatui checks window_size on every frame, I'd expect many apps might do something similar).
- Added comment with rational for window size fallback mechanism. - Only unwrap once during parsing of env vars.
I don't expect the operation to be expensive. In addition it only happens when ioctl |
Did a quick benchmark. On a mac, this is ~100ns, so fast enough generally to not matter. If this turned out to be a hotspot (actually measured by a profiler), it would be easy to replace with a lazy static, but there's no need to do this upfront for now. |
@TimonPost this would probably be easy to merge |
Don't mean to be a PITA but is there anything I could do to increase chances of getting the PR merged? |
I've messaged @TimonPost about this on discord. For context I'm just an interested party - not a maintainer of crossterm. Perhaps if you have some time, it might be good to get some extra eyes on the other PRs and help out from the perspective of testing / reviewing code. Does that sound like something that would work for you? |
Personally do not have a presence if ppl writing cli applications find a very usefull use to this im fine with getting it in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like some comments in the window_size
function to make this behavior explit. Do you think we should consider adding this to windows as well?
For some terminals (for example Emacs eshell/eterm) the ioctl with TIOCGWINSZ might falsely return 0 columns and 0 rows. If this happens we now try to use LINES and COLUMNS environment variables.
Fixes #891