mirror of
https://github.com/ghostty-org/ghostty.git
synced 2026-08-24 16:11:43 +00:00
terminal: transfer selection text into string maps (#13923)
Selection strings previously duplicated formatted text when callers requested a `StringMap`, even though the regex-link caller immediately freed the returned copy. Add a dedicated `selectionStringMap` path that transfers the formatter output and pin map directly into the returned map. This removes the extra allocation and copy while making ownership explicit. **AI Usage:** GTP 5.6 Sol identified and implemented this opportunity. I reviewed and understand it all.
This commit is contained in:
@@ -4369,12 +4369,10 @@ fn linkAtPin(
|
||||
.semantic_prompt_boundary = true,
|
||||
}) orelse return null;
|
||||
|
||||
var strmap: terminal.StringMap = undefined;
|
||||
self.alloc.free(try screen.selectionString(self.alloc, .{
|
||||
const strmap = try screen.selectionStringMap(self.alloc, .{
|
||||
.sel = line,
|
||||
.trim = false,
|
||||
.map = &strmap,
|
||||
}));
|
||||
});
|
||||
defer strmap.deinit(self.alloc);
|
||||
|
||||
for (self.config.links) |link| {
|
||||
|
||||
@@ -2872,17 +2872,8 @@ pub const SelectionString = struct {
|
||||
|
||||
/// If true, trim whitespace around the selection.
|
||||
trim: bool = true,
|
||||
|
||||
/// If non-null, a stringmap will be written here. This will use
|
||||
/// the same allocator as the call to selectionString. The string will
|
||||
/// be duplicated here and in the return value so both must be freed.
|
||||
map: ?*StringMap = null,
|
||||
};
|
||||
|
||||
const selectionString_tw = tripwire.module(enum {
|
||||
copy_map,
|
||||
}, selectionString);
|
||||
|
||||
/// Returns the raw text associated with a selection. This will unwrap
|
||||
/// soft-wrapped edges. The returned slice is owned by the caller and allocated
|
||||
/// using alloc, not the allocator associated with the screen (unless they match).
|
||||
@@ -2892,6 +2883,32 @@ pub fn selectionString(
|
||||
self: *Screen,
|
||||
alloc: Allocator,
|
||||
opts: SelectionString,
|
||||
) Allocator.Error![:0]const u8 {
|
||||
return self.selectionStringImpl(alloc, opts, null);
|
||||
}
|
||||
|
||||
/// Returns a StringMap associated with a selection. The map contains the
|
||||
/// selection's raw text and a mapping from each byte to its screen location.
|
||||
///
|
||||
/// The returned map is owned by the caller.
|
||||
pub fn selectionStringMap(
|
||||
self: *Screen,
|
||||
alloc: Allocator,
|
||||
opts: SelectionString,
|
||||
) Allocator.Error!StringMap {
|
||||
var pins: PinMap.Map = .empty;
|
||||
errdefer pins.deinit(alloc);
|
||||
return .{
|
||||
.string = try self.selectionStringImpl(alloc, opts, &pins),
|
||||
.map = pins,
|
||||
};
|
||||
}
|
||||
|
||||
fn selectionStringImpl(
|
||||
self: *Screen,
|
||||
alloc: Allocator,
|
||||
opts: SelectionString,
|
||||
pins: ?*PinMap.Map,
|
||||
) Allocator.Error![:0]const u8 {
|
||||
// We'll use this as our buffer to build our string.
|
||||
var aw: std.Io.Writer.Allocating = .init(alloc);
|
||||
@@ -2908,35 +2925,15 @@ pub fn selectionString(
|
||||
);
|
||||
formatter.content = .{ .selection = opts.sel };
|
||||
|
||||
// If we have a string map, we need to set that up.
|
||||
var pins: PinMap.Map = .empty;
|
||||
defer pins.deinit(alloc);
|
||||
if (opts.map != null) formatter.pin_map = .{
|
||||
if (pins) |map| formatter.pin_map = .{
|
||||
.alloc = alloc,
|
||||
.map = &pins,
|
||||
.map = map,
|
||||
};
|
||||
|
||||
// Emit. Since this is an allocating writer, a failed write
|
||||
// just becomes an OOM.
|
||||
formatter.format(&aw.writer) catch return error.OutOfMemory;
|
||||
|
||||
// Build our final text and if we have a string map set that up.
|
||||
const text = try aw.toOwnedSliceSentinel(0);
|
||||
errdefer alloc.free(text);
|
||||
if (opts.map) |map| {
|
||||
const map_string = try alloc.dupeZ(u8, text);
|
||||
errdefer alloc.free(map_string);
|
||||
try selectionString_tw.check(.copy_map);
|
||||
map.* = .{
|
||||
.string = map_string,
|
||||
.map = pins,
|
||||
};
|
||||
|
||||
// Ownership of the pin map moved to the string map.
|
||||
pins = .empty;
|
||||
}
|
||||
|
||||
return text;
|
||||
return try aw.toOwnedSliceSentinel(0);
|
||||
}
|
||||
|
||||
pub const SelectLine = struct {
|
||||
@@ -11440,38 +11437,6 @@ test "Screen setAttribute splits page on OutOfSpace at max styles" {
|
||||
try testing.expect(page_was_split);
|
||||
}
|
||||
|
||||
test "selectionString map allocation failure cleanup" {
|
||||
// This test verifies that if toOwnedSlice fails when building
|
||||
// the StringMap, we don't leak the already-allocated map.string.
|
||||
const testing = std.testing;
|
||||
const alloc = testing.allocator;
|
||||
const io = testing.io;
|
||||
var s = try Screen.init(io, alloc, .{ .cols = 10, .rows = 5, .max_scrollback_bytes = 0 });
|
||||
defer s.deinit();
|
||||
|
||||
try s.testWriteString("hello");
|
||||
|
||||
// Get a selection
|
||||
const sel = Selection.init(
|
||||
s.pages.pin(.{ .active = .{ .x = 0, .y = 0 } }).?,
|
||||
s.pages.pin(.{ .active = .{ .x = 4, .y = 0 } }).?,
|
||||
false,
|
||||
);
|
||||
|
||||
// Trigger allocation failure on toOwnedSlice
|
||||
var map: StringMap = undefined;
|
||||
selectionString_tw.errorAlways(.copy_map, error.OutOfMemory);
|
||||
const result = s.selectionString(alloc, .{
|
||||
.sel = sel,
|
||||
.map = &map,
|
||||
});
|
||||
try testing.expectError(error.OutOfMemory, result);
|
||||
try selectionString_tw.end(.reset);
|
||||
|
||||
// If this test passes without memory leaks (when run with testing.allocator),
|
||||
// it means the errdefer properly cleaned up map.string when toOwnedSlice failed.
|
||||
}
|
||||
|
||||
test "Screen: promptClickMove line right basic" {
|
||||
const testing = std.testing;
|
||||
const alloc = testing.allocator;
|
||||
|
||||
@@ -145,13 +145,10 @@ test "StringMap searchIterator" {
|
||||
.y = 1,
|
||||
} }).?,
|
||||
}).?;
|
||||
var map: StringMap = undefined;
|
||||
const sel_str = try s.selectionString(alloc, .{
|
||||
const map = try s.selectionStringMap(alloc, .{
|
||||
.sel = line,
|
||||
.trim = false,
|
||||
.map = &map,
|
||||
});
|
||||
alloc.free(sel_str);
|
||||
defer map.deinit(alloc);
|
||||
|
||||
// Get our iterator
|
||||
@@ -205,13 +202,10 @@ test "StringMap searchIterator URL detection" {
|
||||
.y = 0,
|
||||
} }).?,
|
||||
}).?;
|
||||
var map: StringMap = undefined;
|
||||
const sel_str = try s.selectionString(alloc, .{
|
||||
const map = try s.selectionStringMap(alloc, .{
|
||||
.sel = line,
|
||||
.trim = false,
|
||||
.map = &map,
|
||||
});
|
||||
alloc.free(sel_str);
|
||||
defer map.deinit(alloc);
|
||||
|
||||
// Search for URL match
|
||||
@@ -270,13 +264,10 @@ test "StringMap searchIterator URL with click position" {
|
||||
const line = s.selectLine(.{
|
||||
.pin = click_pin,
|
||||
}).?;
|
||||
var map: StringMap = undefined;
|
||||
const sel_str = try s.selectionString(alloc, .{
|
||||
const map = try s.selectionStringMap(alloc, .{
|
||||
.sel = line,
|
||||
.trim = false,
|
||||
.map = &map,
|
||||
});
|
||||
alloc.free(sel_str);
|
||||
defer map.deinit(alloc);
|
||||
|
||||
// Search for URL match and verify click position is within URL
|
||||
|
||||
Reference in New Issue
Block a user