Skip to content

Fix ghostty-org#4323: Ensure Kitty images scroll with text #6979

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

henriquevmac
Copy link

@henriquevmac henriquevmac commented Apr 2, 2025

Previously, Kitty images did not scroll as shown in #4323 when the terminal was scrolled using ANSI commands (\033[xT for scrolling x down and \033[xS for scrolling x up).

This fix updates the insertLines and deleteLines methods in src/terminal/Terminal.zig to iterate over all kitty_images placements, updating their positions or deleting them if their top-left corner (the Pin position) scrolls off-screen.

Added four unit tests (two per method):

  • One verifies that image positions update correctly when scrolled up or down.

  • The other ensures images are deleted when they scroll out of view.

I ran some tests as shown in this video

The three scripts run where this:

  • test_script:
#!/bin/sh

IMG="$(realpath $1)"

printf '\033[100S' # Scroll up by 100
printf '\033[1d' # Move cursor to row 0
printf '\033[1G' # Move cursor to column 0

printf '========= Image ============\n'
printf '\033_Gi=1,t=f,q=1,f=100,a=T,r=10,c=20;%s\033\\' "$(printf '%s' "$IMG" | base64 -w0)"
printf '<- image\n'
printf '1\n'
printf '2\n'
printf '3\n'

sleep 1

printf '\033[4T' # Scroll down by 4
sleep 1
printf '\033[4S' # Scroll up by 4
printf '\033[1d' # Move cursor to row 0
printf '\033[1G' # Move cursor to column 0

sleep 5
  • test_script_out_bottom:
#!/bin/sh

IMG="$(realpath $1)"

printf '\033[100S' # Scroll up by 100
printf '\033[1d' # Move cursor to row 0
printf '\033[1G' # Move cursor to column 0

printf '========= Image ============\n'
printf '\033_Gi=1,t=f,q=1,f=100,a=T,r=10,c=20;%s\033\\' "$(printf '%s' "$IMG" | base64 -w0)"
printf '<- image\n'
printf '1\n'
printf '2\n'
printf '3\n'

sleep 1

printf '\033[100T' # Scroll down by 100
printf '\033[1d' # Move cursor to row 0
printf '\033[1G' # Move cursor to column 0

sleep 5
  • test_script_out_top:
#!/bin/sh

IMG="$(realpath $1)"

printf '\033[100S' # Scroll up by 100
printf '\033[1d' # Move cursor to row 0
printf '\033[1G' # Move cursor to column 0

printf '========= Image ============\n'
printf '\033_Gi=1,t=f,q=1,f=100,a=T,r=10,c=20;%s\033\\' "$(printf '%s' "$IMG" | base64 -w0)"
printf '<- image\n'
printf '1\n'
printf '2\n'
printf '3\n'

sleep 1

printf '\033[4S' # Scroll up by 4
printf '\033[1d' # Move cursor to row 0
printf '\033[1G' # Move cursor to column 0

sleep 5

Previously, Kitty images did not scroll when the terminal was scrolled
using ANSI commands (\033[xT for scrolling down and \033[xS for
scrolling up).

This fix updates the insertLines and deleteLines methods in
src/terminal/Terminal.zig to iterate over all kitty_images placements,
updating their positions or deleting them if their top-right corner
scrolls off-screen.

Added four unit tests (two per method):
- One verifies that image positions update correctly when scrolled up or
down.
- The other ensures images are deleted when they scroll out of view.
@henriquevmac henriquevmac requested a review from a team as a code owner April 2, 2025 15:18
@qwerasd205
Copy link
Member

Just taking a brief look at this, it seems like you're not accounting for left/right scrolling margins. I'll give this a more thorough review later today.

Copy link
Member

@qwerasd205 qwerasd205 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this should probably account for all tracked pins, not just the ones used for image placements, since we also use tracked pins for, for example, selection endpoints - anything that needs to follow content uses a tracked pin.

Additionally, your pin adjustment logic doesn't account for the left/right scroll margin, as I noted before -- it should, and tests should be added for that as well.

(Side note: You'll find that all the existing tracked pin adjustment logic lives in PageList, and operations in Screen rely on the PageList operations behaving appropriately - so doing this all the way in Terminal is unfortunate, but it needs a refactor to avoid; as my note above insertLines explains, this operation really should belong to Screen not Terminal, but it's fine for now to do it here until that gets refactored.)

@henriquevmac
Copy link
Author

So for the first point I should just have the loop iterate over all tracked pins instead of the pins related to images right?

And for the the left/right scroll margin I should implement something like page.moveCells is doing right now?

@qwerasd205
Copy link
Member

Hi, sorry for the delay in getting back to you. Yes, iterate over all tracked pins (for examples search for pin_keys in PageList.zig), and account for the left/right scroll margins in the same way as is done when deciding how to move the cell contents in the insertLines/deleteLines functions.

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.

2 participants