Skip to content

feat(term): report terminal size in pixels #32408

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

Closed
wants to merge 1 commit into from

Conversation

tbung
Copy link
Contributor

@tbung tbung commented Feb 11, 2025

Terminal pixel size is currently reported as 0, which prevents some programs from working in the built-in terminal, notably programs displaying images (see #32189).

To be able to report terminal pixel size correctly we need to track it alongside the terminal size in cells. In order to do that UIs need to keep track and report their pixel size, which means changes to the API of nvim_ui_attach and nvim_ui_try_resize. That means this PR depends on #31903 to be resolved.

Currently, this PR implements a proof of concept. Terminal pixel size reporting works as is and reports the correct total size of the TUI (or the smallest values of multiple TUIs in the --headless and more general --remote-ui case).

The following things need to be done to consider the pixel size reporting useable:

  • Make nvim_list_uis report pixel sizes
  • Properly update API based on API: relax api contract #31903
  • Use floats for pixel sizes
  • Maybe handle different terminals in TUI (could be delegated to follow up PR)

The following needs to be done for builtin term ioctl reporting:

  • Correct pixel sizes to only include the inner terminal size

The following things need to be done before this PR is in a mergeable state:

  • Check proper default values are set and handled
  • Update docs
  • Implement new tests
  • Fix old tests

@github-actions github-actions bot added tui terminal built-in :terminal or :shell labels Feb 11, 2025
@tbung tbung force-pushed the screensize-pixels branch from 6cee275 to b28f1a6 Compare February 11, 2025 23:05
@@ -37,6 +37,8 @@ typedef struct {
bool ui_ext[kUIExtCount]; ///< Externalized UI capabilities.
int width;
int height;
int pixel_width;
Copy link
Contributor

Choose a reason for hiding this comment

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

If these are changed to floating points, they would be more generally useable for other purposes. For example, to calculate the pixel coordinate of a given cell. Neovim, could internally round this to integers where needed.

Neovide, for example use a fractional grid cell size, so rounding dividing the pixel_width / width would give a pretty inaccurate cell width in some cases,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be willing to change this, but can you maybe briefly more clearly explain why? Am I getting this right, that neovide uses non-pixel aligned cells in order to fill out the entire window if for example pixel_width % width != 0? Couldn't UIs just as well cast the values to float before doing any divisions if needed? Currently, in my mind, the total amount of pixels should always be an integer amount and it should be up to the users of these values to handle division inaccuracies, but I am willing to be convinced otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's say my grid width is 10.1 pixels, and the width is 81 columns, so the real with is 818.1. But the UI passes 819.

Now, we want to display something at column 80. Neovim has to calculate the grid width as 819 / 81 = 10.11111, with the information given. Column 80 is 80 * 10.11111 = 808.88889, so you round and display it at pixel 809. While the correct position should be 80 * 10.1 = 808, so it's displayed one pixel wrong.

It might not matter for the intended use case, but the information could potentially be used for the image protocol for example, and things could become unaligned.

BTW, I'm quite sure that many terminals use fractional grid sizes as well, not just Neovide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see, thank you for the clarification. I'll change it, but I am waiting on some clarification in #31903 wrt to what actual types to use.

I'm guessing in terminal buffers we should then always report the floor of the value to prevent applications from overdrawing. Unfortunately ioctl only takes shorts for TIOCSWINSZ.

@tbung tbung force-pushed the screensize-pixels branch 3 times, most recently from cef5249 to ebe9846 Compare May 12, 2025 19:21
@tbung
Copy link
Contributor Author

tbung commented May 12, 2025

I have now updated this to only require one API change: nvim_ui_try_resize now has an optionals dict parameter. This should then be compatible with #33392 and #31903. Is this how you imagined it @bfredl? Is there a path to get this merged without waiting on #33392?

Also, nvim_list_uis now reports pixel sizes aswell.

I still need to switch everything over to float, if I just do this nvim_list_uis reports these values as small tables. While I think this is a bug in the conversion, I did not yet have the time to get to the bottom of this.

Also, I pulled out the terminal code for now, there's enough changes to be reviewed as is, and I think that stuff might be better off in its own PR, or maybe as a last commit once the internals are working.

@fredizzimo
Copy link
Contributor

Let's put this on hold for a while.

The new image protocol design #34093 does not require the exact size to be exposed. Some approximation like a 2:1 ratio is probably good enough when determining how many rows should be allocated for inline markdown images for example. The actual provider will always render with the correct aspect ratio anyway.

There are also some unanswered questions and problems that I found when taking a closer look.

  1. nvim_ui_try_resize, is not the correct hook since the font for example can change without the grid size changing. It could also cause infinite loops, for example Neovide calls nvim_ui_try_resize, when the window is resized by the user, but the actual resize happens when we get the response back that the columns or rows has changed, and then it's no longer safe to call it again. So, I think another function is needed
  2. For it to be fully usable, it needs to be stored per connection, so that you can get the values for individual UIs, where the plugin selects the primary UI to target by some logic.
  3. In some cases, it might even have to be window specific, when the UI displays a window with a different font for example.

I think the above is something that needs to be resolved, but we also need to figure out the actual practical use cases. It might be that it's only needed when you know that you are targeting a specific GUI, and in that case, the GUI could provide some API by itself.

Or maybe the plugin that needs this information could some hooks for the UI to hook into, providing more targeted information.

In both cases we need generic information from the TUI though, that's an easier case since only one font size is supported. Until the Kitty variable font size protocol is implemented...

@justinmk justinmk added the needs:response waiting for reply from the author label May 21, 2025
@github-actions github-actions bot closed this Jun 21, 2025
Copy link
Contributor

This has been closed since a request for information has not been answered for 30 days. It can be reopened when the requested information is provided.

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 tui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants