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

If ioctl TIOCGWINSZ returns 0,0 try to use env vars instead. #893

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

swilde
Copy link

@swilde swilde commented May 17, 2024

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

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
@swilde swilde requested a review from TimonPost as a code owner May 17, 2024 15:04
Copy link
Collaborator

@joshka joshka left a 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).

src/terminal/sys/unix.rs Show resolved Hide resolved
src/terminal/sys/unix.rs Outdated Show resolved Hide resolved
- Added comment with rational for window size fallback mechanism.
- Only unwrap once during parsing of env vars.
@swilde
Copy link
Author

swilde commented May 22, 2024

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).

I don't expect the operation to be expensive. In addition it only happens when ioctl
fails.

@joshka
Copy link
Collaborator

joshka commented May 22, 2024

I don't expect the operation to be expensive. In addition it only happens when ioctl
fails.

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.

@joshka
Copy link
Collaborator

joshka commented May 22, 2024

@TimonPost this would probably be easy to merge

@swilde
Copy link
Author

swilde commented May 31, 2024

Don't mean to be a PITA but is there anything I could do to increase chances of getting the PR merged?

@joshka
Copy link
Collaborator

joshka commented Jun 1, 2024

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?

@TimonPost
Copy link
Member

Personally do not have a presence if ppl writing cli applications find a very usefull use to this im fine with getting it in.

Copy link
Member

@TimonPost TimonPost left a 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?

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.

crossterm::terminal::size() returns Ok((0, 0)) in eshell
3 participants