Add getcellpixels() #16004

mikoto2000 · 2024-11-07T03:44:20Z

What I want to do?

I want to get the pixel size of the window in the CUI version of Vim.

Motivation:

The ability to output Sixel in echoraw has opened up possibilities such as image display plugins
If the pixel size of the window can be known, the possibilities for functions such as fitting the image to the window will be expanded.

Pros:

The ability to obtain precise pixel size opens up possibilities for image output plugins.

Cons:

Even in cases where the matrix level is sufficient, processing time to obtain pixel level values is added.

Implementation:

Issue TIOCSWINSZ to the parent process's tty to get the size, and calculate the cell size from it.
Calculate the window pixel size using the cell width and height and the window row, col.
Add wpixel and hpixel entries to the getwininfo() dictionary and store the pixel size there.

Others:

The initial implementation used CSI escape sequences, but it was changed to the TIOCSWINSZ implementation because of a problem where processing would not continue unless <Enter> was pressed after getwinsize().
(See: master...mikoto2000:vim:add-pixel-size-in-getwininfo2)

src/evalwindow.c

chrisbra · 2024-11-07T20:49:25Z

Thanks, but I have a few high-level comments:

I would think this does belong into a new function and not into getwininfo(), otherwise it may make is unneccessarily slow.
can you think about a test for this
can you please fix those warnings?

mikoto2000 · 2024-11-07T21:58:03Z

OK. I try it.

Since we know the cell size, it's easy to calculate the window size by combining getwininfo(), so we'll add a function called getcellpixels() that returns the pixel width and height of the terminal cell.
getcellpixels() return dictionary has entry xpixel, ypixel.

Cell size is dependent terminal, so test is only dictionary has entries check.

I will work with this implementation.

ychin · 2024-11-07T22:34:09Z

A few top-level questions / comments:

This has a fallback default of 5x10 pixels if for some reason the info could not be obtained. I think the desired behavior should instead be to indicate clearly to the caller that the info is unavailable instead of silently failing and using a wrong value. This could either be a error return value, or something like [-1,-1].
Using /proc is non-portable (e.g. it doesn't work on non-Linux Unix systems like macOS) and seems like an unnecessary way to obtain the fd. Given that Vim is constantly writing to the terminal I would imagine the fd should already be available to us? Why would we need to query for the tty manually? We should be able to just use it and do the ioctl call.
Windows support?
GUI support?
Should probably use Vim-standardized typedef's and functions, e.g. UINT for unsigned ints, and vim_snprintf.

ychin · 2024-11-07T22:35:17Z

Since we know the cell size, it's easy to calculate the window size by combining getwininfo(), so we'll add a function called getcellpixels() that returns the pixel width and height of the terminal cell.
getcellpixels() return dictionary has entry xpixel, ypixel.

Should this just return an array, something like [7,11] instead of a dictionary? I'm not strongly opinionated on this one but for simple x/y coords that seems easiest to me, but would make it less extensible in the future.

ychin · 2024-11-07T22:57:20Z

Windows support?

This (https://stackoverflow.com/a/50395589/8916547) seems to provide a way to query this in Windows.

mikoto2000 · 2024-11-08T00:12:22Z

@ychin Thank you for your comments!

A few top-level questions / comments:

You're right. I use list([x, y]), and return [-1, -1] if error.
You're right. I get current process's tty by ttyname().
It's not included in this PR.
- thank you Add getcellpixels() #16004 (comment) I check it.
It's not included in this PR.
Thank you, I check Vim-standardized typedef's and functions.

…tty using `ttyname`.

ychin · 2024-11-08T01:26:24Z

You're right. I get current process's tty by ttyname().

No, what I mean is just use the stdin / stdout fd. I don't think there is any need to do any of the other methods to get the tty fd.

In fact, did you look at the mch_get_shellsize() implementation? It's already calling TIOCGWINSZ but it's only retrieving the number of rows/columns. I think the implementation of your function could just do something similar to that function (similar to the part I pasted below), and prefixed with mch_ for the function name (which we do for platform-specific functionality):

// os_unix.c

    int
mch_get_shellsize(void)
{
// …

    /*
     * 1. try using an ioctl. It is the most accurate method.
     *
     * Try using TIOCGWINSZ first, some systems that have it also define
     * TIOCGSIZE but don't have a struct ttysize.
     */
# ifdef TIOCGWINSZ
    {
	struct winsize	ws;
	int fd = 1;

	// When stdout is not a tty, use stdin for the ioctl().
	if (!isatty(fd) && isatty(read_cmd_fd))
	    fd = read_cmd_fd;
	if (ioctl(fd, TIOCGWINSZ, &ws) == 0)
	{
	    columns = ws.ws_col;
	    rows = ws.ws_row;
#  ifdef FEAT_EVAL
	    ch_log(NULL, "Got size with TIOCGWINSZ: %ld x %ld", columns, rows);
#  endif
	}
// …
}

For reference, this function is used for determining the values of 'lines' and 'columns'.

Another comment (just to save back-and-forth time) is we usually put function declaration in the misc .pro header files inside src/proto folder. For example there is a proto/os_unix.pro file which contains the header declarations. (I don't really know the history behind them)

mikoto2000 · 2024-11-08T01:51:54Z

Sorry, I did not study enough.
We will look at mch_get_shellsize() and the surrounding functions and change the implementation.

src/os_unix.c

src/proto/os_unix.pro

mikoto2000 · 2024-11-08T06:25:40Z

@zeertzjq
Sorry, I mixed up x and y in one place, so I made an additional commit: 177734a.

mattn · 2024-11-08T06:38:39Z

runtime/doc/builtin.txt

+		List format is [xpixels, ypixels].
+		Only on UNIX.
+
+		Return type: list<any>


The document should explain that some environments may return unreliable values.

On what environments would it be unreliable? I think it just needs to clearly explain that it would return an error value, e.g. -1,-1 when it's unavailable. That's not "unreliable" per se.

chrisbra · 2024-11-09T10:39:22Z

runtime/doc/builtin.txt


+getcellpixels()						*getcellpixels()*
+		Returns a |List| of cell pixel size.
+		List format is [xpixels, ypixels].


Only works on Unix. For gVim, returns [5, 10], on other systems, returns [-1, -1].

Shouldn't gVIm return [-1,-1] until gVim gets a proper implementation?

ychin · 2024-11-12T17:48:26Z

Seems from the implementation that this is going to return [] on unsupported platforms (e.g. Windows), since it's just provided a NULL implementation (in evalfunc.c)? On Unix, gVim does return [-1,-1]. This doesn't match the documented behavior that it always returns [-1,-1].

Should we just standardize on this and fix the doc? Perhaps just returning [] for all unsupported platforms is better than [-1,-1] and it's actually easier for a caller to check as well. This way we could just use a NULL implementation and have it just work. That means we just need to fix the GUI implementation and also the documentation.

I think the tests are also in the wrong file as well. I can submit a PR to fix it. It's put into the UTF-8 tests next to getcellwidths() but getcellwidths() is related to setting specialized double-width cells depending on which UTF-8 char which is completely different from the purpose of getcellpixels().

chrisbra · 2024-11-12T19:36:18Z

Yes makes sense to just return an empty list. and the documentation should then be changed back to List<any>. @mikoto2000 can you make those changes on a followup PR please?

mikoto2000 · 2024-11-12T22:26:23Z

Thank you for pointing out the improvement.
I will create a followup PR.

mikoto2000 · 2024-11-12T23:11:03Z

Can not call getcellpixels() when NULL implementation in evalfunc.c.
(:call getcellpixels() occures E117 unknown function)

I will fix doc and gui implementation, test position.

mikoto2000 · 2024-11-12T23:16:23Z

I will create test file test_getcellpixels.vim.

mikoto2000 · 2024-11-14T22:43:23Z

@ychin

Windows support?

This (https://stackoverflow.com/a/50395589/8916547) seems to provide a way to query this in Windows.

I tried the stack overflow post you suggested, but the font size was returned as a fixed value of 0x16.
I'll try to find a different approach.

prototype branch: 2fb4877#diff-6e09d275741ed4a17372f6e64995645e818d605e41a0591b2c20741f4878d3c1R9074

Don't use `gui.char_width` / `char_height` unlike the other GVim implementations. Those are used for deriving screen pixel sizes and MacVim has been hard-coding them to 1 for simplicity since the actual GUI functionality is handled out of the Vim process anyway. Changing those values would require some refactoring. Instead, just use a new variable to store them. Related: vim/vim#16004

Don't use `gui.char_width` / `char_height` unlike the other GVim implementations. Those are used for deriving screen pixel sizes and MacVim has been hard-coding them to 1 for simplicity since the actual GUI functionality is handled out of the Vim process anyway. Changing those values would require some refactoring. Instead, just use a new variable to store them. Note that there is a delay with this method, as we only update Vim's knowledge of cell size after MacVim has received the font change message. This means if a user wants to immediately query getcellpixels() in vimrc or after changing guifont this will not work. This is a deliberate design choice to avoid having to add synchronous state query APIs to MacVim, but it's possible to change it in the future. The handling of this message did get placed in `processInput:` which means it will be picked up whenever we sleep or process messages in Vim, instead of only when waiting for keys (where most of other MacVim messages are handled in `processInputQueue`). Related: vim/vim#16004

Added pixel size in getwininfo()

511470f

github-advanced-security bot found potential problems Nov 7, 2024

View reviewed changes

src/evalwindow.c Fixed Show fixed Hide fixed

src/evalwindow.c Fixed Show fixed Hide fixed

Fix invalid source format.

db51514

mikoto2000 force-pushed the add-pixel-size-in-getwininfo branch from 8718e22 to db51514 Compare November 7, 2024 04:27

mikoto2000 added 5 commits November 8, 2024 01:05

Stopped getting the parent process's tty and modified to get its own …

f8937f3

…tty using `ttyname`.

Added evalfunc of f_getcellpixels.

a6eec0e

Deleted pixel info in get_win_info().

2c5d783

Fix document.

e680808

Added test.

030cb73

Remove unnecessary tty fd acquisition processing

9ec7038

github-advanced-security bot found potential problems Nov 8, 2024

View reviewed changes

src/os_unix.c Fixed Show fixed Hide fixed

src/os_unix.c Fixed Show fixed Hide fixed

mikoto2000 added 5 commits November 8, 2024 02:05

Fix format type.

c35296a

Added function in usr_41.

ebc16c9

Added compile switch.

d4197c1

Updated tags file.

690fc79

Added Windows check and skip.

fc822c7

zeertzjq changed the title ~~Added pixel size in getwininfo()~~ Add getcellpixels() Nov 8, 2024

zeertzjq reviewed Nov 8, 2024

View reviewed changes

src/proto/os_unix.pro Outdated Show resolved Hide resolved

mikoto2000 added 2 commits November 8, 2024 04:54

Fixed miss operations comment break.

a2adff2

Fixed the mistake where x and y were mixed up.

177734a

mattn reviewed Nov 8, 2024

View reviewed changes

Added notes on use to the documentation.

3703fb1

chrisbra reviewed Nov 9, 2024

View reviewed changes

mikoto2000 added 2 commits November 9, 2024 11:21

Clarify operation conditions and return values.

3dd15e8

Fixed return value on gVim.

ab6443a

chrisbra closed this in 1083cae Nov 11, 2024

mikoto2000 mentioned this pull request Nov 13, 2024

Improve getcellpixels(). #16047

Closed

mikoto2000 mentioned this pull request Nov 15, 2024

Add gui support to getcellpixels #16059

Closed

ychin mentioned this pull request Feb 13, 2025

Implement getcellpixels() for MacVim macvim-dev/macvim#1554

Merged

Uh oh!

Add getcellpixels() #16004

Add getcellpixels() #16004

Uh oh!

Conversation

mikoto2000 commented Nov 7, 2024

What I want to do?

Motivation:

Pros:

Cons:

Implementation:

Others:

Uh oh!

Uh oh!

Uh oh!

chrisbra commented Nov 7, 2024

Uh oh!

mikoto2000 commented Nov 7, 2024

Uh oh!

ychin commented Nov 7, 2024

Uh oh!

ychin commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ychin commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikoto2000 commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ychin commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikoto2000 commented Nov 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikoto2000 commented Nov 8, 2024

Uh oh!

mattn Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

ychin Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbra Nov 9, 2024

Choose a reason for hiding this comment

Uh oh!

ychin Nov 9, 2024

Choose a reason for hiding this comment

Uh oh!

ychin commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbra commented Nov 12, 2024

Uh oh!

mikoto2000 commented Nov 12, 2024

Uh oh!

mikoto2000 commented Nov 12, 2024

Uh oh!

mikoto2000 commented Nov 12, 2024

Uh oh!

mikoto2000 commented Nov 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ychin commented Nov 7, 2024 •

edited

Loading

ychin commented Nov 7, 2024 •

edited

Loading

mikoto2000 commented Nov 8, 2024 •

edited

Loading

ychin commented Nov 8, 2024 •

edited

Loading

ychin Nov 8, 2024 •

edited

Loading

ychin commented Nov 12, 2024 •

edited

Loading