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

The size() is broken on MacOs. #406

Closed
extrawurst opened this issue Mar 30, 2020 · 6 comments
Closed

The size() is broken on MacOs. #406

extrawurst opened this issue Mar 30, 2020 · 6 comments
Labels
bug in_progress is being worked on

Comments

@extrawurst
Copy link
Contributor

extrawurst commented Mar 30, 2020

Describe the bug
commit 52b9d47 broke querying the correct size of the terminal on MacOs aswell as the resize event. The size returned will always be 80x24 and not the actual size.
I am pretty sure this is related to the revert that was necessary to fix the input on Mac aswell in 0.17.1 because it also tries to read "/dev/tty".

see the gif as an illustration (note the resize event always returning 80x24 instead of the correct size values):
demo

note: reverting that particular commit fixes the issue

To Reproduce
run the example on Mac and resize the window:

cargo run --example event-poll-read

OS
MacOs

Terminal/Shell
Zsh/iTerm

update: clarified that by broken I mean the values for size are not correct.

@TimonPost
Copy link
Member

I think the problem with this issue is not TTY in specific. The reading terminal size was implemented in version 0.15.1. You said in discord that 0.16 did not have this issue, that would mean that the city is not the problem here.

I think it is a combo of falling back on stdout and using tput/libc. Here is a quick update, I have to go now, will come back at this tomorrow.

First:

 if let Ok(true) =
        wrap_with_result(unsafe { ioctl(STDOUT_FILENO, TIOCGWINSZ.into(), &mut size) })
    {
        Ok((size.ws_col, size.ws_row))
    } else {
        tput_size().ok_or_else(|| std::io::Error::last_os_error().into())
    }

version 1 0.15:

let file = File::open("/dev/tty").unwrap();

let file = File::open("/dev/tty").unwrap();

    if let Ok(true) =
        wrap_with_result(unsafe { ioctl(file.into_raw_fd(), TIOCGWINSZ.into(), &mut size) })
    {
        Ok((size.ws_col, size.ws_row))
    } else {
        tput_size().ok_or_else(|| std::io::Error::last_os_error().into())
    }

version 2:

let file = File::open("/dev/tty").unwrap();

let file = File::open("/dev/tty").unwrap();

    if let Ok(true) =
        wrap_with_result(unsafe { ioctl(file.into_raw_fd(), TIOCGWINSZ.into(), &mut size) })
    {
        Ok((size.ws_col, size.ws_row))
    } else {
        tput_size().ok_or_else(|| std::io::Error::last_os_error().into())
    }

version 3:

let file = File::open("/dev/tty")?;

   let file = File::open("/dev/tty")?;

    let fd = FileDesc::new(file.into_raw_fd(), true);
    if let Ok(true) = wrap_with_result(unsafe { ioctl(fd.raw_fd(), TIOCGWINSZ.into(), &mut size) })
    {
        Ok((size.ws_col, size.ws_row))
    } else {
        tput_size().ok_or_else(|| std::io::Error::last_os_error().into())
    }

version 4 (0.17):

let fd = if let Ok(file) = File::open("/dev/tty") {

let fd = if let Ok(file) = File::open("/dev/tty") {
        FileDesc::new(file.into_raw_fd(), true).raw_fd()
    } else {
        // Fallback to libc::STDOUT_FILENO if /dev/tty is missing
        STDOUT_FILENO
    };

    if let Ok(true) = wrap_with_result(unsafe { ioctl(fd, TIOCGWINSZ.into(), &mut size) }) {
        Ok((size.ws_col, size.ws_row))
    } else {
        tput_size().ok_or_else(|| std::io::Error::last_os_error().into())
    }

@extrawurst
Copy link
Contributor Author

After our discord discussion I want to emphasise that I bisected the issue down to one commit that introduced the problem: 52b9d47

the parent of that commit does not show this behaviour. Also when reverting only that commit on current master (or 0.17.1) the problem goes away - the plot thickens.

I am no expert on tty but there was already another regression regarding input that broke crossterm on macOS and my suggestion would be to revert 52b9d47 and find a solution that is tested on macos before introducing it back in.

@TimonPost
Copy link
Member

TimonPost commented Mar 31, 2020

Just a quick finding: in 0.16 we went from File::into_raw_fd fd to FileDesc::raw_fd, in our previous findings discussed in discord, the input problem of #396, was about 'wrong' argument. There we use FileDesc::raw_fd as well. Maybe the way we pass RawFd to libc in the wrong way.

@m-lima
Copy link
Contributor

m-lima commented Apr 8, 2020

I was having the same problem when I found this issue.

I believe FileDesc is being dropped at line 36.
When creating a new FileDesc, a true is passed into close_on_drop, and the descriptor gets dropped before reaching the ioctl since it goes out of scope.

I validated this by running a brute-force version that keeps FileDesc alive:

#[allow(clippy::identity_conversion)]
pub(crate) fn size() -> Result<(u16, u16)> {
    // http://rosettacode.org/wiki/Terminal_control/Dimensions#Library:_BSD_libc
    let mut size = winsize {
        ws_row: 0,
        ws_col: 0,
        ws_xpixel: 0,
        ws_ypixel: 0,
    };

    let mut file_desc = None;

    let fd = if let Ok(file) = File::open("/dev/tty") {
        file_desc = Some(FileDesc::new(file.into_raw_fd(), true));
        file_desc.as_ref().unwrap().raw_fd()
    } else {
        // Fallback to libc::STDOUT_FILENO if /dev/tty is missing
        STDOUT_FILENO
    };

    if let Ok(true) = wrap_with_result(unsafe { ioctl(fd, TIOCGWINSZ.into(), &mut size) }) {
        Ok((size.ws_col, size.ws_row))
    } else {
        tput_size().ok_or_else(|| std::io::Error::last_os_error().into())
    }
}

I'll create a more elegant PR for this fix for you guys to review

@TimonPost TimonPost changed the title (re)size broken on MacOs The size is broken on MacOs. Apr 9, 2020
@TimonPost TimonPost changed the title The size is broken on MacOs. The size() is broken on MacOs. Apr 9, 2020
@TimonPost TimonPost added enhancement bug in_progress is being worked on and removed enhancement labels Apr 9, 2020
@japert
Copy link

japert commented Apr 9, 2020

I can confirm #413 fixed this issue.
Terminal resize works as expected on macOS on master.

@TimonPost
Copy link
Member

Should be fixed in 17.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug in_progress is being worked on
Projects
None yet
Development

No branches or pull requests

4 participants