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

fix(terminal): report terminal window size in pixels #28621

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

Conversation

lumynou5
Copy link

@lumynou5 lumynou5 commented May 3, 2024

Window size in pixels is required by some programs, reporting it can help those programs work inside NeoVim terminals.

Closes #8259.

However, this patch doesn't make icat work. Instead, it just makes the program show another error, because the problem is not only related to window sizes in pixels but how escape sequences (specifically, APC) are handled. See #4349, #20176.

Window size in pixels is required by some programs, reporting it can
help those programs work inside NeoVim terminals.

See also: neovim#8259, neovim#12991
@github-actions github-actions bot added the terminal built-in :terminal or :shell label May 3, 2024
Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks. If no one else has comments, ping me if this isn't merged in a couple days

@justinmk
Copy link
Member

justinmk commented May 3, 2024

I wonder if this fixes any of the log messages that were showing in Windows CI

['Warning: noted but unhandled ioctl'] = 1,

Co-authored-by: Justin M. Keyes <justinkz@gmail.com>
@lumynou5
Copy link
Author

lumynou5 commented May 4, 2024

Currently the test is failed. When stdout is redirected, ioctl calls fail with errno ENOTTY, and the program crashes due to invalid division.
Adding a check to set pixel sizes to 0 in case of failed calls can help, but it seems not a desirable solution?

  int res = ioctl(1, TIOCGWINSZ, &ptyproc->winsize);
  ptyproc->winsize = (struct winsize){
    ptyproc->height,
    ptyproc->width,
    res ? 0 : (ptyproc->winsize.ws_xpixel / ptyproc->winsize.ws_col) * ptyproc->width,
    res ? 0 : (ptyproc->winsize.ws_ypixel / ptyproc->winsize.ws_row) * ptyproc->height,
  };

@lumynou5 lumynou5 requested a review from justinmk May 7, 2024 06:00
@zeertzjq
Copy link
Member

zeertzjq commented May 7, 2024

It seems this doesn't actually solve anything, and may be incorrect. For example, when running Nvim with --headless and attaching another TUI with --remote-ui, this will use the pixel size of the terminal running the --headless instance instead of the one running the --remote-ui instance.

Window sizes in pixels now have a fallback value 0 when NeoVim is not
running in a TTY.  This prevents NeoVim from crashing if it's running in
an IDE's integrated terminal, a terminal multiplexer, or a testing, etc.
@lumynou5
Copy link
Author

lumynou5 commented May 7, 2024

It seems this doesn't actually solve anything, and may be incorrect. For example, when running Nvim with --headless and attaching another TUI with --remote-ui, this will use the pixel size of the terminal running the --headless instance instead of the one running the --remote-ui instance.

Thanks. I didn't think about remote. I'll try to solve it.

@justinmk justinmk added the needs:response waiting for reply from the author label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:response waiting for reply from the author terminal built-in :terminal or :shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Terminal does not support reporting screen size s via the TIOCGWINSZ ioctl"
3 participants