ghostty: add ghostty_surface_set_selection_range C API#64
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a public C API and Zig implementation to set a surface selection by start/end row and column with an optional rectangular flag; header declaration, Zig method converting coordinates and updating selection under lock, and C export wrapper are included. ChangesSelection Range API Implementation
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryAdds
Confidence Score: 4/5The change introduces a new, additive C API function; existing behavior is untouched and the new code follows the locking, pin-resolution, and render-queuing patterns established by the sibling selection functions. The implementation is structurally sound and consistent with the rest of the selection API. The only issue is the missing parameter names in the C header, where four consecutive uint32_t arguments give no hint of their order to callers reading the header alone. include/ghostty.h — the declaration would benefit from named parameters to prevent row/column transposition at call sites. Important Files Changed
Sequence DiagramsequenceDiagram
participant C as C Caller (cmux)
participant E as embedded.zig (CAPI)
participant S as Surface.zig
participant T as Terminal / PageList
participant R as Renderer Thread
C->>E: ghostty_surface_set_selection_range(surface, row_start, col_start, row_end, col_end, is_rectangular)
E->>S: core_surface.setSelectionRange(...)
S->>S: renderer_state.mutex.lock()
S->>T: screens.active (get active screen)
S->>T: "pages.pin(.screen {x=col_start, y=row_start})"
T-->>S: start_pin (or null → return false)
S->>T: "pages.pin(.screen {x=col_end, y=row_end})"
T-->>S: end_pin (or null → return false)
S->>S: setSelection(Selection.init(start_pin, end_pin, is_rectangular))
S->>T: screen.select(sel)
S->>T: "screen.dirty.selection = true"
S->>R: queueRender() (wakeup notify)
S->>S: renderer_state.mutex.unlock()
S-->>E: true
E-->>C: true
Reviews (1): Last reviewed commit: "ghostty: add ghostty_surface_set_selecti..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| GHOSTTY_API bool ghostty_surface_set_selection_range(ghostty_surface_t, | ||
| uint32_t, | ||
| uint32_t, | ||
| uint32_t, | ||
| uint32_t, | ||
| bool); |
There was a problem hiding this comment.
uint32_t arguments. With four consecutive same-typed params the argument order (row_start, col_start, row_end, col_end) is invisible at the call site, and callers must read the implementation to determine it. Other single-uint32_t functions in this header are unambiguous, but this one is not. Adding names matches how Zig defines it and prevents accidental row/column transposition.
| GHOSTTY_API bool ghostty_surface_set_selection_range(ghostty_surface_t, | |
| uint32_t, | |
| uint32_t, | |
| uint32_t, | |
| uint32_t, | |
| bool); | |
| GHOSTTY_API bool ghostty_surface_set_selection_range(ghostty_surface_t, | |
| uint32_t row_start, | |
| uint32_t col_start, | |
| uint32_t row_end, | |
| uint32_t col_end, | |
| bool is_rectangular); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…stream/main) Includes resolve of conflict: both selectCursorLine (upstream) and setSelectionRange (Slice 0) are kept as independent cmux additions to the fork.
tayiorbeii
force-pushed
the
copy-mode-cursor-ux-slice-0
branch
from
e173506 to
c1d033e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Surface.zig`:
- Around line 2136-2140: The selection endpoints must be normalized before
constructing the terminal.Selection to guarantee start ≤ end; modify the block
that obtains start_pin and end_pin (from screen.pages.pin) to compare their
screen coordinates (e.g., pin.screen.x and pin.screen.y) and swap them when
necessary (or construct normalized start_pin/ end_pin variables) so that the
values passed to self.setSelection(terminal.Selection.init(start_pin, end_pin,
is_rectangular)) always satisfy the start ≤ end contract; ensure
screen.dirty.selection is still set after calling setSelection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76273134-c968-47c7-bf21-be18a57a01ab
📒 Files selected for processing (3)
include/ghostty.hsrc/Surface.zigsrc/apprt/embedded.zig
| const start_pin = screen.pages.pin(.{ .screen = .{ .x = col_start_cell, .y = row_start } }) orelse return false; | ||
| const end_pin = screen.pages.pin(.{ .screen = .{ .x = col_end_cell, .y = row_end } }) orelse return false; | ||
|
|
||
| try self.setSelection(terminal.Selection.init(start_pin, end_pin, is_rectangular)); | ||
| screen.dirty.selection = true; |
There was a problem hiding this comment.
Normalize selection endpoints before constructing the selection.
Line 2139 uses caller-provided start/end as-is. Reversed coordinates can violate the start≤end selection contract and produce incorrect selection behavior. Normalize pins first.
Proposed fix
const start_pin = screen.pages.pin(.{ .screen = .{ .x = col_start_cell, .y = row_start } }) orelse return false;
const end_pin = screen.pages.pin(.{ .screen = .{ .x = col_end_cell, .y = row_end } }) orelse return false;
- try self.setSelection(terminal.Selection.init(start_pin, end_pin, is_rectangular));
+ var ordered_start = start_pin;
+ var ordered_end = end_pin;
+ if (ordered_end.before(ordered_start)) {
+ std.mem.swap(terminal.Pin, &ordered_start, &ordered_end);
+ }
+
+ try self.setSelection(terminal.Selection.init(ordered_start, ordered_end, is_rectangular));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const start_pin = screen.pages.pin(.{ .screen = .{ .x = col_start_cell, .y = row_start } }) orelse return false; | |
| const end_pin = screen.pages.pin(.{ .screen = .{ .x = col_end_cell, .y = row_end } }) orelse return false; | |
| try self.setSelection(terminal.Selection.init(start_pin, end_pin, is_rectangular)); | |
| screen.dirty.selection = true; | |
| const start_pin = screen.pages.pin(.{ .screen = .{ .x = col_start_cell, .y = row_start } }) orelse return false; | |
| const end_pin = screen.pages.pin(.{ .screen = .{ .x = col_end_cell, .y = row_end } }) orelse return false; | |
| var ordered_start = start_pin; | |
| var ordered_end = end_pin; | |
| if (ordered_end.before(ordered_start)) { | |
| std.mem.swap(terminal.Pin, &ordered_start, &ordered_end); | |
| } | |
| try self.setSelection(terminal.Selection.init(ordered_start, ordered_end, is_rectangular)); | |
| screen.dirty.selection = true; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Surface.zig` around lines 2136 - 2140, The selection endpoints must be
normalized before constructing the terminal.Selection to guarantee start ≤ end;
modify the block that obtains start_pin and end_pin (from screen.pages.pin) to
compare their screen coordinates (e.g., pin.screen.x and pin.screen.y) and swap
them when necessary (or construct normalized start_pin/ end_pin variables) so
that the values passed to self.setSelection(terminal.Selection.init(start_pin,
end_pin, is_rectangular)) always satisfy the start ≤ end contract; ensure
screen.dirty.selection is still set after calling setSelection.
1 participant
Adds the
ghostty_surface_set_selection_rangeC API function for programmatic selection control in embedded mode.This is needed by cmux for the copy-mode cursor UX slices (see plans/2026-05-22-copy-mode-cursor-ux-slices.md Slice 0).
Changes:
include/ghostty.h: Declareghostty_surface_set_selection_rangesrc/Surface.zig: ImplementsetSelectionRangepublic methodsrc/apprt/embedded.zig: ImplementsetSelectionRangeon embedded surfaceAcceptance:
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds
ghostty_surface_set_selection_rangeto the embedded C API to set text selections using screen coordinates (supports rectangular). Enables cmux copy-mode cursor UX Slice 0 while keeping existing selection APIs unchanged.ghostty_surface_set_selection_range(ghostty_surface_t, row_start, col_start, row_end, col_end, is_rectangular).Written for commit 0a650e9. Summary will update on new commits. Review in cubic
Summary by CodeRabbit