Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/ghostty.h
Original file line number Diff line number Diff line change
Expand Up @@ -1180,6 +1180,12 @@ GHOSTTY_API void ghostty_surface_complete_clipboard_request(ghostty_surface_t,
GHOSTTY_API bool ghostty_surface_has_selection(ghostty_surface_t);
GHOSTTY_API bool ghostty_surface_select_cursor_cell(ghostty_surface_t);
GHOSTTY_API bool ghostty_surface_select_cursor_line(ghostty_surface_t);
GHOSTTY_API bool ghostty_surface_set_selection_range(ghostty_surface_t,
uint32_t,
uint32_t,
uint32_t,
uint32_t,
bool);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 The declaration omits parameter names for all four 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.

Suggested change
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!

GHOSTTY_API bool ghostty_surface_clear_selection(ghostty_surface_t);
GHOSTTY_API bool ghostty_surface_read_selection(ghostty_surface_t, ghostty_text_s*);
GHOSTTY_API bool ghostty_surface_read_text(ghostty_surface_t,
Expand Down
24 changes: 24 additions & 0 deletions src/Surface.zig
Original file line number Diff line number Diff line change
Expand Up @@ -2118,6 +2118,30 @@ pub fn selectCursorLine(self: *Surface) !bool {
return true;
}

/// Set a selection range from buffer (screen) coordinates (cmux-specific).
pub fn setSelectionRange(
self: *Surface,
row_start: u32,
col_start: u32,
row_end: u32,
col_end: u32,
is_rectangular: bool,
) !bool {
self.renderer_state.mutex.lock();
defer self.renderer_state.mutex.unlock();

const screen: *terminal.Screen = self.io.terminal.screens.active;
const col_start_cell = std.math.cast(terminal.size.CellCountInt, col_start) orelse return false;
const col_end_cell = std.math.cast(terminal.size.CellCountInt, col_end) orelse return false;
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;
Comment on lines +2136 to +2140

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

try self.queueRender();
return true;
}

/// Clear the active selection (cmux-specific).
pub fn clearSelection(self: *Surface) !bool {
self.renderer_state.mutex.lock();
Expand Down
21 changes: 21 additions & 0 deletions src/apprt/embedded.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1684,6 +1684,27 @@ pub const CAPI = struct {
};
}

/// Set a selection range from buffer (screen) coordinates (cmux-specific).
export fn ghostty_surface_set_selection_range(
surface: *Surface,
row_start: u32,
col_start: u32,
row_end: u32,
col_end: u32,
is_rectangular: bool,
) bool {
return surface.core_surface.setSelectionRange(
row_start,
col_start,
row_end,
col_end,
is_rectangular,
) catch |err| {
log.warn("error setting selection range err={}", .{err});
return false;
};
}

/// Same as ghostty_surface_read_text but reads from the user selection,
/// if any.
export fn ghostty_surface_read_selection(
Expand Down