Fix Kitty Unicode placeholder rendering for scrollable images

[grimwm]

Summary

This PR fixes multiple issues with Kitty graphics protocol support, particularly for Unicode placeholder mode which enables images to scroll with terminal content.

Key changes

1. Two-step rendering for Unicode placeholders

   • Transmit image data as PNG without display (f=100,t=d)
   • Create virtual placement with U=1 and explicit cols/rows (a=p,U=1)
   • Generate placeholder characters with image ID encoded in foreground color
   • Skip pre-resizing when using Unicode placeholders (terminal handles scaling)

2. Fix image ID encoding

   • Add globalKittyImageNum counter for 24-bit compatible image numbers
   • Use small sequential numbers (1-0xFFFFFF) that fit in RGB encoding

3. Fix ClearAll functions

   • Use d=A (delete ALL) instead of just a=d
   • Add missing escape sequence terminator (\x1b\\)

4. Skip terminal queries when protocol detected from environment

   • If Kitty/iTerm2 detected via KITTY_WINDOW_ID, TERM, etc., skip all terminal queries to avoid leaving garbage in stdin buffer
   • Fixes first-keypress issues with TUI frameworks like bubbletea

5. Fix TOCTOU race condition in ResizeCache

   • Use atomic file operations to prevent race conditions

Test plan

• Verified images display at correct size (not thumbnails)
• Verified single 'q' press exits cleanly (no leftover images)
• Verified first keypress registers immediately in TUI apps
• Verified images scroll with terminal content
• All existing tests pass

[grimwm]

@blacktop may I ask if there is somebody that can take a look at this? I have tested it on my end and can see that images scroll, but I would love to have a more complete review in case I need to make some changes.

[blacktop]

Apologies for being slow to respond; looking this it looks good, just trying to find the time to give it a thorough review.

[grimwm]

Apologies for being slow to respond; looking this it looks good, just trying to find the time to give it a thorough review.

Oh, no worries. I understand the issues with finding time. I was just making sure the project wasn't stale. I know people lose focus (like me).

[blacktop]

Code Review: PR #27 - Fix Kitty Unicode placeholder rendering for scrollable images

This is a well-structured PR that addresses multiple important issues with the Kitty graphics protocol implementation. Here's my detailed review:

---

✅ What's Good

1. Two-step Unicode Placeholder Rendering (kitty.go:153-216)
The new implementation correctly follows the Kitty protocol spec for Unicode placeholders:

• Transmit image data first with f=100,t=d (PNG format, direct transmission, no display)
• Create placement with a=p,U=1 (placement action with Unicode mode)
• Generate placeholder characters

This matches the reference implementation in ratatui-image and the Kitty spec.

2. Image ID Encoding Fix (kitty.go:88-89, 159-165)
The globalKittyImageNum counter ensures IDs fit in 24 bits for RGB foreground color encoding. The wraparound logic at 0xFFFFFF is correct.

3. ClearAll Fix (termimg.go:343-345, 357-359)
Adding d=A (uppercase A = delete ALL images and placements) and the missing \x1b\\ terminator is a critical fix.

4. Early Return for Environment-based Detection (detect.go:315-319)
Skipping terminal queries when protocol is already detected from environment variables avoids leaving garbage in stdin - this fixes real issues with TUI frameworks like bubbletea.

5. TOCTOU Race Fix (resize.go:45-67)
The new get() method uses a single write lock for the entire check-and-update operation, eliminating the race condition.

6. Skip Resize for Unicode Placeholders (renderers.go:42-46)
Correct behavior - when using Unicode placeholders, the terminal handles scaling, so we should send full-resolution images.

---

⚠️ Suggestions & Minor Issues

1. Atomic Operation Pattern (kitty.go:160-164)

imageNum := atomic.AddUint32(&globalKittyImageNum, 1)
if imageNum > 0xFFFFFF {
    atomic.StoreUint32(&globalKittyImageNum, 1)
    imageNum = 1
}

This has a race window - two goroutines could both detect imageNum > 0xFFFFFF and both reset to 1. Consider using atomic.CompareAndSwapUint32 in a loop for proper wraparound. However, for practical purposes this is unlikely to cause issues in real usage.

2. Color Encoding Strategy (kitty.go:670-678)

if imageID <= 255 {
    colorStart = fmt.Sprintf("\x1b[38;5;%dm", imageID)
} else {
    // RGB encoding
}

The 256-color mode optimization for small IDs is nice, but verify this is compatible with all Kitty-protocol supporting terminals (Ghostty, WezTerm, etc.).

3. Newline Color Handling (kitty.go:690-693)

builder.WriteString("\x1b[39m\n")
builder.WriteString(colorStart)

Resetting and re-applying color across newlines is defensive but adds escape sequence overhead. Worth keeping for compatibility.

4. Removed Cursor Save/Restore (kitty.go:463-474)
The removal of \x1b[s (save cursor) and \x1b[u (restore cursor) in PlaceImageWithSize is intentional for scrolling behavior. The updated doc comment explains this well.

---

🔍 Questions

1. Line 331-333 (kitty.go): The condition opts.Virtual && !opts.KittyOpts.UseUnicode generates placeholders for non-unicode virtual placement. Is this the intended fallback behavior? The comment says "Non-unicode virtual placement" but it still calls generateUnicodePlaceholders which might be confusing.

2. Test Coverage: The PR description mentions manual testing. Would it be valuable to add automated tests for the new Unicode placeholder rendering path?

---

📋 Summary

Category	Assessment
Correctness	✅ Fixes real bugs, follows spec
Thread Safety	✅ Improved (minor atomic issue noted)
API Design	✅ UseUnicode() is clean
Backward Compat	✅ Non-breaking changes
Documentation	✅ Good comments explaining behavior

Overall: LGTM - This PR makes significant improvements to the Kitty protocol support and fixes real usability issues with TUI frameworks. The changes are well-thought-out and follow the Kitty graphics protocol specification correctly.

[blacktop]

Code Review: PR #27 - Fix Kitty Unicode placeholder rendering for scrollable images

This is a well-structured PR that addresses multiple important issues with the Kitty graphics protocol implementation. Here's my detailed review:

---

✅ What's Good

1. Two-step Unicode Placeholder Rendering (kitty.go:153-216)
The new implementation correctly follows the Kitty protocol spec for Unicode placeholders:

• Transmit image data first with f=100,t=d (PNG format, direct transmission, no display)
• Create placement with a=p,U=1 (placement action with Unicode mode)
• Generate placeholder characters

This matches the reference implementation in ratatui-image and the Kitty spec.

2. Image ID Encoding Fix (kitty.go:88-89, 159-165)
The globalKittyImageNum counter ensures IDs fit in 24 bits for RGB foreground color encoding. The wraparound logic at 0xFFFFFF is correct.

3. ClearAll Fix (termimg.go:343-345, 357-359)
Adding d=A (uppercase A = delete ALL images and placements) and the missing escape terminator is a critical fix.

4. Early Return for Environment-based Detection (detect.go:315-319)
Skipping terminal queries when protocol is already detected from environment variables avoids leaving garbage in stdin - this fixes real issues with TUI frameworks like bubbletea.

5. TOCTOU Race Fix (resize.go:45-67)
The new get() method uses a single write lock for the entire check-and-update operation, eliminating the race condition.

6. Skip Resize for Unicode Placeholders (renderers.go:42-46)
Correct behavior - when using Unicode placeholders, the terminal handles scaling, so we should send full-resolution images.

---

⚠️ Suggestions & Minor Issues

1. Atomic Operation Pattern (kitty.go:160-164)
The wraparound logic has a race window - two goroutines could both detect imageNum > 0xFFFFFF and both reset to 1. Consider using atomic.CompareAndSwapUint32 in a loop for proper wraparound. However, for practical purposes this is unlikely to cause issues in real usage.

2. Color Encoding Strategy (kitty.go:670-678)
The 256-color mode optimization for small IDs is nice, but verify this is compatible with all Kitty-protocol supporting terminals (Ghostty, WezTerm, etc.).

3. Newline Color Handling (kitty.go:690-693)
Resetting and re-applying color across newlines is defensive but adds escape sequence overhead. Worth keeping for compatibility.

4. Removed Cursor Save/Restore (kitty.go:463-474)
The removal of cursor save/restore in PlaceImageWithSize is intentional for scrolling behavior. The updated doc comment explains this well.

---

🔍 Questions

1. Line 331-333 (kitty.go): The condition opts.Virtual && !opts.KittyOpts.UseUnicode generates placeholders for non-unicode virtual placement. Is this the intended fallback behavior?

2. Test Coverage: Would it be valuable to add automated tests for the new Unicode placeholder rendering path?

---

📋 Summary

Category	Assessment
Correctness	✅ Fixes real bugs, follows spec
Thread Safety	✅ Improved (minor atomic issue noted)
API Design	✅ UseUnicode() is clean
Backward Compat	✅ Non-breaking changes
Documentation	✅ Good comments explaining behavior

Overall: LGTM - This PR makes significant improvements to the Kitty protocol support and fixes real usability issues with TUI frameworks. The changes are well-thought-out and follow the Kitty graphics protocol specification correctly.

[grimwm]

@blacktop thank you for your review. I do not mind getting the "Suggestions & Minor Issues" taken care of. I like high-quality work, and it's NBD for me to give it some attention. Just let me know what you'd like me to do.