search

contour-terminal / contour

Modern C++ Terminal Emulator
http://contour-terminal.org/
Apache License 2.0
2.43k stars 105 forks source link

Extra padding added to Sixel image #809

Closed imsnif closed 2 years ago

imsnif commented 2 years ago

Contour Terminal version

Contour Terminal Emulator 0.3.4-master-e5571bab

Installer source

something else (please specify below)

Operating System

Arch Linux

Architecture

x86-64

Other Software

No response

Steps to reproduce

cat the attached minimal reproduction. hi.txt

Expected Behavior

No padding is added after the image (attached image is from xterm with the --vt340 flag) img-2022-09-09-111050

Actual Behavior

Several padding lines added after image (as seen in the image, I removed sixel-scrolling (DECSDM) to match the behaviour - I also think the default Contour behaviour is reversed here compared to other terminals, but maybe that's a different issue :) ). img-2022-09-09-111204

Additional notes

Hey there! I'm one of the Zellij maintainers and was working on this issue: https://github.com/zellij-org/zellij/issues/1697

I think I managed to narrow it down to the above reproduction. For us in the context of Zellij, this causes our UI to break when we run inside Contour and display a Sixel image.

As I mentioned above, once we solve this there is also the question of whether DECSDM enables or disables Sixel scrolling (I think Contour behaves differently than other terminal emulators in this context).

Happy to provide other reproduction steps or to discuss this further as needed.

Utkarsh-khambra commented 2 years ago

@christianparpart So I think issue is whenever no aspect ratio is passed through the initial sequence to enter "sixel mode", contour just assumes the image size is whatever the max image size in config is set to be.

Here the two sample of same image, one where aspect ratio is passed through the sequence and other where it isn't passed with_size.txt without_size.txt

christianparpart commented 2 years ago

Oha, thanks guys. I remember I noticed that too recently. @Utkarsh-khambra would you want to take a look or shall I? :-)

christianparpart commented 2 years ago

@imsnif after an internal discussion on discord, it is now pretty clear how to fix this. Thanks for the report.

Wrt DECSDM, if I accidentally flipped it, I apologize, I will check. I somehow barely remember that I strived to comply to the sixel spec but TEs have been doing it the other ways around. Nertheless, I will take a look and converge of course.