window pixel size

[benjajaja]

It is possible to render images in terminals with protocols such as Sixel, iTerm2's, or Kitty's. For a basic sixel or iTerm2 image printing, it is sufficient to print some escape sequence with the data, e.g. cat image just works, the image is displayed and enough lines are scrolled.

But for more sophisticated usage of images, such as TUIs, it is necessary to know exactly what area that image would cover, in terms of columns/rows of characters. Then it would be possible to e.g. resize the image to a size that fits a col/row area precisely, not overdraw the image area, accommodate layouts, etc.

Thus, provide the window size in pixel width/height, in addition to cols/rows.

(Screenshot is not up to date, I removed the call from the is_tty demo.

[TimonPost]

im gonna make a release of winapi in a bit

[TimonPost]

Just released the verision. You should be able to use it now without having to update the version, it was a path

[markus-bauer]

I have some issues/questions about this:

1. There is duplicate code for getting winsize.
   https://github.com/benjajaja/crossterm/blob/de0e6277cc0f622a3ce83cde00f448892d688ff7/src/terminal/sys/unix.rs#L24
   https://github.com/benjajaja/crossterm/blob/de0e6277cc0f622a3ce83cde00f448892d688ff7/src/terminal/sys/unix.rs#L52

2. Why is it called font_size? AFAIU, what this returns is the size of a cell in pixels.

3. There is unchecked division. Is it guaranteed, that ws_row and ws_col are non-zero?

4. ws_xpixel, ws_ypixel are not guaranteed to be set. For example see https://man7.org/linux/man-pages/man4/tty_ioctl.4.html, where these are marked as "unused":

 The struct used by these ioctls is defined as

           struct winsize {
               unsigned short ws_row;
               unsigned short ws_col;
               unsigned short ws_xpixel;   /* unused */
               unsigned short ws_ypixel;   /* unused */
           };

Meaning they can be set to zero by some terminals. I think the function should return Result<Option<(usize, usize)>>, which is None if these are zero (i.e. not set).

5. Instead of having this function and https://docs.rs/crossterm/latest/crossterm/terminal/fn.size.html, wouldn't it be better to return a TerminalSize struct, that impls these methods. Otherwise, at least on unix, you would need two syscalls to get both.
   I was thinking something like this (just a quick sketch):

impl TerminalSize {
    pub fn new() -> Result<Self> 

    /// Returns None if terminal size is zero.
    pub fn new_non_zero() -> Result<Option<Self>>
    
    pub fn size_cells(&self) -> (u32, u32)

    pub fn size_pixels(&self) -> Option<(u32, u32) >

    /// Not sure. This could also return an Option instead of zero sizes.
    pub fn cell_sizes_in_pixels(&self) -> (u32, u32) {
        if (self.x_pixels > self.columns)
            && (self.y_pixels > self.rows)
            && (self.columns != 0)
            && (self.rows != 0)
        {
            ((self.x_pixels / self.columns), (self.y_pixels / self.rows))
        } else {
            (0, 0)
        }
    }
}

[benjajaja]

@markus-bauer changing the return of size() would be a breaking change, is that ok?

[markus-bauer]

@benjajaja Why? I don't think size would have to change. Perhaps the implementation, but not the interface.
Although, if you look at the code, it already returns an Error if the dimensions are zero:

crossterm/src/terminal/sys/unix.rs

Lines 42 to 43 in 1efdce7

	&& size.ws_col != 0
	&& size.ws_row != 0

.

You can still have separate functions for size, pixel_size (of the terminal), and cell_size_in_pixels/font_size.

Point 4 was just about the new function. Whether you return zeros or Option/Result is a design decision. AFAIK, if a unix terminal is not reporting pixel size, ws_*pixel are zero. And a zero size terminal is impossible. Therefore it should/could be interpreted as None or Err.

And with the last point, I wanted to say, that If the user want's to get both size and font_size, the unix backend would be doing the exact work twice. Therefore it should be possible to get both/all with one call. And the most obvious solution is to have a public interface on top of something like winsize.

TLDR:
I'd (1) have a common function to get winsize (in the backend); (2) do the not-zero check in font_size (like it's done in size) and return Err if zero; and (3) at least rule out zero division. As for the rest, that's @TimonPost's decision.

[benjajaja]

@markus-bauer okay I changed it to window_size() -> Result<WindowSize> that returns the same as winsize. And size() just returns the columns / rows of that. Fixes code duplication and reduces syscalls. If the user needs the cell-size, they can calculate it themselves.

[markus-bauer]

I'm not sure about the windows side, but isn't this forcing additional syscalls and calculations on windows?

Honestly, I don't know what the correct solution would be that satisfies both unix and windows. That's probably the reason why this wasn't added earlier. Perhaps @TimonPost can help you.

As to your new code. There's an additional issue:
Currently size returns Err when ws_(row/col) are zero. That's not in your code anymore.

[benjajaja]

@markus-bauer thanks for spotting that missing zero-check.

Windows will always have to call the font-size syscall, as the terminal-size syscall doesn't return pixel sizes. It's not a problem since its size() call is unaffected.

[benjajaja]

@TimonPost BTW this is good to review now :)

[TimonPost]

Thanks for working on this!

I am not sure If I understand if 'winsize.ws_xpixelandwinsize.ws_ypixel` are in physical pixels of the screen. The unix docs do state pixel height/width, whereas the windows docs state they are in 'in logical units':

Windows docs say:

A [COORD](https://learn.microsoft.com/en-us/windows/console/coord-str) structure that contains the width and height of each character in the font, in logical units. The X member contains the width, while the Y member contains the height.

The question I am having is wether we need to make this clear in the documentation if there is a difference here.

[TimonPost]

As this is a public function please specify this applies for unix systems here.

[markus-bauer]

I was also confused about that. The PR just assumed that these are the same. How could you verify that?

@TimonPost Are you generally against target specific functions? Like font_size() on windows and winsize() on unix.

In any case, at least on unix it's trivial to do this yourself, without crossterm, using libc or rustix (https://docs.rs/rustix/latest/rustix/termios/fn.tcgetwinsize.html).

[benjajaja]

Good catch. I edited the comment so that it now mentions both uncertainties about unix & windows.
For windows' "logical units", well, I tried it out and it seems to be pixels. I tried with windows 10 powershell and cmd.exe, took a screenshot and measured the pixels. I can't say that I'm 100% certain that this holds up all the time or for other windows versions/terminals.

But I don't think the windows implementation really matters, as the motivation here is to have control over drawing images with e.g. sixels and that is not implemented for any window terminal AFAIK.

To be specific, I want this to work on https://github.com/tui-rs-revival/ratatui/, where the other "backends" are termion and termwiz, which both have a method to get the window pixel sizes.

In any case, at least on unix it's trivial to do this yourself, without crossterm, using libc or rustix (https://docs.rs/rustix/latest/rustix/termios/fn.tcgetwinsize.html).

Yes, but it would be somewhat sad having to do that, when there is already a syscall available for crossterm.

[markus-bauer]

Wouldn't this mean that the units of WindowSize are different depending on the os? Pixels on unix, logical units on windows. That's confusing.

[benjajaja]

@markus-bauer yea you are right, I was ignorant about what "logical unit" means.

[benjajaja]

@markus-bauer @TimonPost I am leaving window_size unimplemented for all windows versions. This would allow ratatui to add window_size to its "backend" trait, with all three backends.

[TimonPost]

Potential could do:

physical_window_size
logical_window_size

Thats how winit defines their api's as well. Tho im fine with first doing it this way and not reusing the same api for different units. I can imagine that could result in confusion.

[benjajaja]

I would maybe expose something like font_size() -> FontSize where FontSize is an enum of Physical/Logical. But I don't know if the logical font size is even useful, at least for my use case of rendering images, only pixels are relevant.

[benjajaja]

@TimonPost I think this could be merged, if I understood correctly.

[TimonPost]

Generally, I like to refrain from adding target-specific functions but we already have a few cases, and I do not really mind it that much as long it's clear from the docs that it doesn't work on windows. Current solution is good and allows us to eventually do something for windows of similar nature. Perhaps it should be hidden behind cfg(unix).

[markus-bauer]

This was merged into main already?

I wrote too much already, but there are some potential issues with the documentation: https://docs.rs/crossterm/latest/crossterm/terminal/fn.window_size.html.

1. External and internal links are not formatted as links.
2. The width and height in pixels may not be reliably implemented or default to zero! Not implemented by whom? You mean that the pixel dimensions reported by the terminal emulator might not be reliable. And, AFAIK, all values default to zero if unknown, not just the *_pixel. The user has to check for that.
3. You talk about unix but link to a linux page. I don't know if TIOCGWINSZ is posix. (It might be, I just don't know.)
   https://austingroupbugs.net/view.php?id=1151
4. For windows it is not implemented. What does that mean? It is available, because it's not gated by a cfg. The current implementation returns an Error on windows, which I wouldn't know from reading the doc.
5. Shouldn't the WindowSize struct have documentation? State that width and height are supposed to be pixels dimensions of the terminal. The fact that all could be zero, and that pixel dimensions are perhaps not reliable could be moved to WindowSize.