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

Workaround the cursor placement bug #51

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Workaround the cursor placement bug #51

wants to merge 2 commits into from

Conversation

hackerb9
Copy link
Owner

@hackerb9 hackerb9 commented Sep 18, 2021

It has been pointed out by @j4james that, in correct sixel implementations, the text cursor after showing a sixel image should overlap with the bottom of the image. It seems likely that terminal emulators will gradually change to reflect that because that is how a full screen sixel image can be shown without scrolling. There are also subtle problems that occur with different heights of sixel images. See:

This patch checks $TERM and adds a text newline if the terminal is known to have a correct sixel implementation. Otherwise, it is presumed the terminal will be adding its own newline.

Relatedly, this patch also removes the graphics newline that is appended to the end of sixel images generated by ImageMagick. Again, @j4james has helped by explaining that the final graphics newline is actually harmful as it can cause the terminal to scroll on a fullscreen image. It can also be difficult for a program like lsix to tell whether a text newline is necessary or not. I believe ImageMagick and libsixel will eventually be updated to remove the final graphics newline. By doing it ourselves now, lsix is prepared for that transition and also avoids the question of extra space between rows of tiles when we send a text newline.

Most sixel compatible terminal emulators do not yet correctly place
the text cursor on the final line of sixel image data (the upper row
if straddling two lines of text) after a sixel image has been
transmitted.
montage always generates a '-' at the end of a sixel image. As
pointed out by @j4james, this should not happen by default.

Until ImageMagick fixes the bug (which may require an update to
libsixel), we can easily remove the offending character using bash.

Lsix will continue to work either way.
@PerBothner
Copy link

The latest DomTerm also implements the "correct" behavior. Hence, please add xterm-domterm to the These terminals are correct pattern.

Also, plain xterm now works, as of version 374.

@j4james
Copy link

j4james commented Apr 2, 2023

Also, plain xterm now works, as of version 374.

Weird that there's no mention of that change in the log. But I've just tested now, and the cursor positioning is certainly improved. It's still not entirely correct, though, if you're expecting it to emulate a VT340. If the final sixel line spans two rows, the cursor ends up on the bottom row when it should be the top row.

It also has some weird bugs, like it can sometimes scroll early, even though the sixels haven't reached the bottom of the screen, and if you output a sixel image with no content, the text cursor actually moves up!

@PerBothner
Copy link

"Weird that there's no mention of that change in the log" It is mentioned:
"change default for sixelScrolling resource to better match VT330/VT340 DECSDM setting (patch by Ben Wong)."

"It's still not entirely correct, though, if you're expecting it to emulate a VT340. If the final sixel line spans two rows, the cursor ends up on the bottom row when it should be the top row."

That seems broken. That means you would sometimes have emit two newlines to avoid following text overlapping.

@j4james
Copy link

j4james commented Apr 2, 2023

change default for sixelScrolling resource to better match VT330/VT340 DECSDM setting

That's just a correction to the sixelScrolling resource, which had the opposite meaning of what it was intended to be. It has nothing to do with the cursor positioning.

That seems broken. That means you would sometimes have emit two newlines to avoid following text overlapping.

If you are trying to align images with text, you typically want your images to be an exact multiple of the text line height. To achieve that with sixel, the last sixel line will often extend beyond the final text row, with the extraneous pixels being transparent. In that scenario, you want the text to overlap those transparent pixels, so DEC's algorithm positions the cursor exactly where you need it to be. XTerm positions it too far down.

You can argue that there are situations where a different algorithm may be better, but the doesn't make DEC's algorithm broken. And the one massive advantage that the DEC algorithm has over anything else is that it hasn't changed in something like 36 years. XTerm is already on its fourth iteration (at least), and I doubt it will be the last.

@dnkl
Copy link

dnkl commented Feb 29, 2024

Foot also supports correct positioning, since 1.15.0.

The changelog mentions the behavior matching xterm's, but that was before I realized xterm doesn't handle last sixel spanning two rows correctly - foot does handle it correctly (as in, it implements the DEC algorithm).

@dnkl
Copy link

dnkl commented Feb 29, 2024

diff --git a/lsix b/lsix
index a621c57..f8150a0 100755
--- a/lsix
+++ b/lsix
@@ -122,7 +122,7 @@ autodetect() {
 
     # SIXEL CURSOR PLACEMENT BUG WORKAROUND
     case "$TERM" in
-	vt340*|contour*)
+	vt340*|contour*|foot*)
 	    # These terminals are correct. 
 	    sixelcursorbug=""	
 	    ;;

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

Successfully merging this pull request may close these issues.

None yet

4 participants