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

Terminal: iTerm2 inline images protocol does not explicity specify when to scroll a page. #3

Closed
ismail-yilmaz opened this issue May 20, 2020 · 8 comments

Comments

@ismail-yilmaz
Copy link
Owner

ismail-yilmaz commented May 20, 2020

Ultimate++ terminal widget (v0.3/2020.1) added support for iTerm2's inline images protocol.
However, iTerm2's original protocol does not seem to explicitly specify the following:

  1. When to scroll the page, if at all? (Currently, we are relying on DECSDM)
  2. What happens when the preserveAspectRatio parameter is set to 0 with only, say, width parameter is specified. (E.g. this is the case with ranger file manager.) Should the terminal use the original height of the image (treat it as auto)? If so, should it scroll the page, or crop the image, if the original height of the image exceeds the height of terminal's page height?

@gnachman, If you have time , I would be grateful if you could clarify these two points about iTerm2's.inline images protocol.

@gnachman
Copy link

As implemented it's pretty buggy when used with scroll regions. I think the right thing to do is to respect left/right and top/bottom margins if the cursor was inside the scroll region when the image code was received, and otherwise to ignore them.

@ismail-yilmaz
Copy link
Owner Author

I think the right thing to do is to respect left/right and top/bottom margins if the cursor was inside the scroll region when the image code was received, and otherwise to ignore them.

I see. This is more or less the current behavior of our vte, but I needed to be sure.

What about the second point? I take that the unspecified image dimensions should be treated as auto and use the image's original value when preserveAspectRatio=0?.

Thank you very much for taking the time to reply.
(Also, thanks for the esctest automatic unit tests suite.)

@gnachman
Copy link

For question #2 if width or height is unspecified, it defaults to auto.

When an image is taller than the page, it scrolls automatically. iTerm2 limits images to 255 rows in height, which is a consequence of an implementation decision, but if that's not a problem for you feel free to go over it.

I added a test suite in c89a7e6eaaf749905080b4424a1cf29cf8b957be which you might find helpful. It passes if every image is outlined on the bottom and right by - and | and there's exactly one empty row between the bottom outline and the subsequent description. When the aspect ratio is preserved, the image should be centered horizontally or vertically in the designated area.

@gnachman
Copy link

These are the defaults as implemented:

    //   name=<base64-encoded filename>    Default: Unnamed file
    //   size=<integer file size>          Default: 0
    //   width=auto|<integer>px|<integer>  Default: auto
    //   height=auto|<integer>px|<integer> Default: auto
    //   preserveAspectRatio=<bool>        Default: yes
    //   inline=<bool>                     Default: no

@gnachman
Copy link

And commit 142eedeb297415c102f3c3441954d229284945c5 fixes scroll region support to work as I described above.

@ismail-yilmaz
Copy link
Owner Author

For question #2 if width or height is unspecified, it defaults to auto.

When an image is taller than the page, it scrolls automatically. iTerm2 limits images to 255 rows in height, which is a consequence of an implementation decision, but if that's not a problem for you feel free to go over it.

I added a test suite in c89a7e6eaaf749905080b4424a1cf29cf8b957be which you might find helpful. It passes if every image is outlined on the bottom and right by - and | and there's exactly one empty row between the bottom outline and the subsequent description. When the aspect ratio is preserved, the image should be centered horizontally or vertically in the designated area.

These are the defaults as implemented:

    //   name=<base64-encoded filename>    Default: Unnamed file
    //   size=<integer file size>          Default: 0
    //   width=auto|<integer>px|<integer>  Default: auto
    //   height=auto|<integer>px|<integer> Default: auto
    //   preserveAspectRatio=<bool>        Default: yes
    //   inline=<bool>                     Default: no

This little conversation was really helpful. Thank you for clarifying everything. .I got my answers now.
That test suite will definitely be helpful for further testing and fortifying the support for the protocol in our vte widget. I will look into it ASAP.

Keep up the good work, and have a nice week!

@gnachman
Copy link

Sure thing! One other thing to keep in mind, which is a really fiddly detail but one that people will complain about, is that when centering an image it needs to be centered at the pixel level, not the cell level. So if your cells are 10 pixels wide and you're showing a 22px wide image, the leftmost and rightmost column should have a 4 pixel inset.

@ismail-yilmaz
Copy link
Owner Author

Sure thing! One other thing to keep in mind, which is a really fiddly detail but one that people will complain about, is that when centering an image it needs to be centered at the pixel level, not the cell level. So if your cells are 10 pixels wide and you're showing a 22px wide image, the leftmost and rightmost column should have a 4 pixel inset.

Yeah, that might pose a problem. But Ultimate++'s terminal widget has "a sort of solution" for this problem: It draws inline images onto screen by delegating the actual drawing and the "post-processing" of the image to client code -without modifying the original image- via the Upp::Display interface. In this way the final image can be freely and dynamically manipulated by the client code. Moreover, these display objects can be changed on-the-fly by the client code or by the user. For example, ATM, we provide NormalImageCellDisplay and ScaledImageDisplay as the default and alternative display objects. They simply do what their name suggest. It is easy to provide more alternatives that can inset or offset the final image, as you point out.

Long story short: Since this is an embeddable terminal widget, not a fully-fledged app like iTerm2, I think it is legitimate to say that it is up to client code to come up with more displaying options, as we only provide the fundamental framework and interface for inline image display and manipulation. After all, I don't intend to turn this humble vt emulator into the first cell-based gimp or photoshop clone. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants