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

Provide cell-pixel geometry in TIOCGWINSZ #241

Closed
dankamongmen opened this issue Jun 21, 2021 · 11 comments · Fixed by #243
Closed

Provide cell-pixel geometry in TIOCGWINSZ #241

dankamongmen opened this issue Jun 21, 2021 · 11 comments · Fixed by #243
Labels
bug Something isn't working
Milestone

Comments

@dankamongmen
Copy link
Contributor

Abstract

Contour's implementation of the TIOCGWINSZ ioctl(2) doesn't fill in the following two optional fields:

               unsigned short ws_xpixel;   /* unused */
               unsigned short ws_ypixel;

Contour supports Sixel. Unfortunately, without this knowledge, it's difficult to use Sixel correctly for anything but trivial work.

Motivation

These fields ought be filled in, as they are by most terminals supporting bitmap graphics (all of which I am aware).

Specification

When TIOCGWINSZ is answered by Contour, the ws_xpixel field ought be filled in with the number of native pixels per cell width, and ws_ypixel ought be filled in with the number of native pixels per cell height.

When the cell-pixel geometry changes (for instance in response to a font change), the information ought be updated in subsequent ioctl(2)s, and ideally a SIGWINCH ought be delivered as it would upon a cell geometry change.

see dankamongmen/notcurses#1805

@christianparpart
Copy link
Member

christianparpart commented Jun 21, 2021

Oh.

Edit: should have been filled. I will be fixing that.

Edit2: also set x/ypixels at PTY construction and not just at resize events. (https://github.com/christianparpart/contour/blob/master/src/terminal/pty/UnixPty.cpp#L111)

@christianparpart christianparpart added the bug Something isn't working label Jun 21, 2021
@christianparpart christianparpart added this to the 0.2.0 milestone Jun 21, 2021
christianparpart added a commit that referenced this issue Jun 21, 2021
… only set during resize but not initially. Fixes #241.
@christianparpart
Copy link
Member

Thanks @dankamongmen, I really appreciate your reports. I owe you a beer or two ;-)

@dankamongmen
Copy link
Contributor Author

Thanks @dankamongmen, I really appreciate your reports. I owe you a beer or two ;-)

i'm strictly a krokodil / bathsalts / adrenochrome man, though i have been known to quaff some Sterno in my time

@dankamongmen
Copy link
Contributor Author

btw i gave the wrong semantics here. i said they should have the cell-pixel geometry; it ought be the window geometry in pixels.

christianparpart added a commit that referenced this issue Jun 22, 2021
…ixels

Fixes `ioctl(..., TIOCGWINSZ, ...)` pixel values that were only set during resize but not initially. Fixes #241.
@dankamongmen
Copy link
Contributor Author

confirmed, thanks much holmes!

 notcurses 2.3.4 by nick black et al on Contour 0.2.0-unreleased-master-96b8f89
  30 rows (22px) 130 cols (11px) (60.94KiB) 48B crend 256 colors+RGB
  compiled with gcc-10.2.1 20210110, 16B little-endian cells
  terminfo from ncurses 6.2.20201114
  avformat 58.45.100 avutil 56.51.100 swscale 5.7.100

 colors: 256 rgb: y ccc: y setaf: y setab: y
 sgr: y sgr0: y op: y fgop: y bgop: y
 rows: 30 cols: 130 rpx: 22 cpx: 11 (660x1430)
 sixel colorregs: 256

@dankamongmen
Copy link
Contributor Author

  Width: 1446
  Height: 660

checks out (i've got a scrollbar which could easily be 16 pixels wide)!

@christianparpart
Copy link
Member

christianparpart commented Jun 22, 2021 via email

@dankamongmen
Copy link
Contributor Author

Is this a problem? Just so i understand :)

nope, just confirming via xwininfo that the rematerialized window pixel geometry was reasonable. it was! =]

@dankamongmen
Copy link
Contributor Author

rows: 30 cols: 130 rpx: 22 cpx: 11 (660x1430) <----------- notcurses-info

Width: 1446 <--------------------- xwininfo
Height: 660

they're not an exact match, but i think the 16 extra pixels are probably due to the scrollbar.

@christianparpart
Copy link
Member

Then I shall fix this

@dankamongmen
Copy link
Contributor Author

no it's working! i must not be clear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants