Applied-Software/ghostty
5
0
Fork 0
mirror of https://github.com/ghostty-org/ghostty.git synced 2025-07-14 15:56:13 +03:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #2156 from qwerasd205/hyperlink-fixes

Browse Source 
Various fixes for hyperlink instability
This commit is contained in:
Mitchell Hashimoto
2024-08-28 10:01:19 -07:00
committed by GitHub
parent 7540a015a3 df09a37597
commit 42a23f2d3d
6 changed files with 348 additions and 116 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/renderer/Metal.zig
View File

@ -891,6 +891,14 @@ pub fn updateFrame(
// Update all our data as tightly as possible within the mutex.
var critical: Critical = critical: {
// const start = try std.time.Instant.now();
// const start_micro = std.time.microTimestamp();
// defer {
// const end = std.time.Instant.now() catch unreachable;
// // "[updateFrame critical time] <START us>\t<TIME_TAKEN us>"
// std.log.err("[updateFrame critical time] {}\t{}", .{start_micro, end.since(start) / std.time.ns_per_us});
// }
state.mutex.lock();
defer state.mutex.unlock();

98
src/terminal/PageList.zig
View File

@ -416,93 +416,25 @@ pub fn clone(
chunk.page.data.capacity,
&page_size,
);
assert(page.data.capacity.rows >= chunk.page.data.capacity.rows);
assert(page.data.capacity.rows >= chunk.end - chunk.start);
defer page.data.assertIntegrity();
page.data.size.rows = chunk.page.data.size.rows;
page.data.size.rows = chunk.end - chunk.start;
try page.data.cloneFrom(
&chunk.page.data,
0,
chunk.page.data.size.rows,
chunk.start,
chunk.end,
);
page_list.append(page);
// If this is a full page then we're done.
if (chunk.fullPage()) {
total_rows += page.data.size.rows;
// Updating tracked pins is easy, we just change the page
// pointer but all offsets remain the same.
if (opts.tracked_pins) |remap| {
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != chunk.page) continue;
const new_p = try pool.pins.create();
new_p.* = p.*;
new_p.page = page;
try remap.putNoClobber(p, new_p);
try tracked_pins.putNoClobber(pool.alloc, new_p, {});
}
}
continue;
}
// If this is just a shortened chunk off the end we can just
// shorten the size. We don't worry about clearing memory here because
// as the page grows the memory will be reclaimable because the data
// is still valid.
if (chunk.start == 0) {
page.data.size.rows = @intCast(chunk.end);
total_rows += chunk.end;
// Updating tracked pins for the pins that are in the shortened chunk.
if (opts.tracked_pins) |remap| {
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != chunk.page or
p.y >= chunk.end) continue;
const new_p = try pool.pins.create();
new_p.* = p.*;
new_p.page = page;
try remap.putNoClobber(p, new_p);
try tracked_pins.putNoClobber(pool.alloc, new_p, {});
}
}
continue;
}
// We want to maintain the dirty bits from the original page so
// instead of setting a range we grab the dirty bit and then
// set it on the new page in the new location.
var dirty = page.data.dirtyBitSet();
// Kind of slow, we want to shift the rows up in the page up to
// end and then resize down.
const rows = page.data.rows.ptr(page.data.memory);
const len = chunk.end - chunk.start;
for (0..len) |i| {
const src: *Row = &rows[i + chunk.start];
const dst: *Row = &rows[i];
const old_dst = dst.*;
dst.* = src.*;
src.* = old_dst;
dirty.setValue(i, dirty.isSet(i + chunk.start));
}
// We need to clear the rows we're about to truncate.
for (len..page.data.size.rows) |i| {
page.data.clearCells(&rows[i], 0, page.data.size.cols);
}
page.data.size.rows = @intCast(len);
total_rows += len;
// Updating tracked pins
// Remap our tracked pins by changing the page and
// offsetting the Y position based on the chunk start.
if (opts.tracked_pins) |remap| {
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
// We're only interested in pins that were within the chunk.
if (p.page != chunk.page or
p.y < chunk.start or
p.y >= chunk.end) continue;
@ -948,17 +880,15 @@ const ReflowCursor = struct {
},
}
// Prevent integrity checks from tripping
// while copying graphemes and hyperlinks.
if (comptime std.debug.runtime_safety) {
// These will create issues by trying to clone managed memory that
// isn't set if the current dst row needs to be moved to a new page.
// They'll be fixed once we do properly copy the relevant memory.
self.page_cell.content_tag = .codepoint;
self.page_cell.hyperlink = false;
self.page_cell.style_id = stylepkg.default_id;
}
// Copy grapheme data.
if (cell.content_tag == .codepoint_grapheme) {
// The tag is asserted to be .codepoint in setGraphemes.
self.page_cell.content_tag = .codepoint;
// Copy the graphemes
const cps = src_page.lookupGrapheme(cell).?;
@ -981,8 +911,6 @@ const ReflowCursor = struct {
}
}
self.page_row.grapheme = true;
// This shouldn't fail since we made sure we have space above.
try self.page.setGraphemes(self.page_row, self.page_cell, cps);
}
@ -1015,8 +943,6 @@ const ReflowCursor = struct {
);
} orelse src_id;
self.page_row.hyperlink = true;
// We expect this to succeed due to the
// hyperlinkCapacity check we did before.
try self.page.setHyperlink(

188
src/terminal/Screen.zig
View File

@ -902,10 +902,17 @@ fn cursorDownOrScroll(self: *Screen) !void {
/// Copy another cursor. The cursor can be on any screen but the x/y
/// must be within our screen bounds.
pub fn cursorCopy(self: *Screen, other: Cursor) !void {
pub fn cursorCopy(self: *Screen, other: Cursor, opts: struct {
/// Copy the hyperlink from the other cursor. If not set, this will
/// clear our current hyperlink.
hyperlink: bool = true,
}) !void {
assert(other.x < self.pages.cols);
assert(other.y < self.pages.rows);
// End any currently active hyperlink on our cursor.
self.endHyperlink();
const old = self.cursor;
self.cursor = other;
errdefer self.cursor = old;
@ -913,6 +920,10 @@ pub fn cursorCopy(self: *Screen, other: Cursor) !void {
// Keep our old style ID so it can be properly cleaned up below.
self.cursor.style_id = old.style_id;
// Hyperlinks will be managed separately below.
self.cursor.hyperlink_id = 0;
self.cursor.hyperlink = null;
// Keep our old page pin and X/Y because:
// 1. The old style will need to be cleaned up from the page it's from.
// 2. The new position navigated to by `cursorAbsolute` needs to be in our
@ -927,6 +938,27 @@ pub fn cursorCopy(self: *Screen, other: Cursor) !void {
// Move to the correct location to match the other cursor.
self.cursorAbsolute(other.x, other.y);
// If the other cursor had a hyperlink, add it to ours.
if (opts.hyperlink and other.hyperlink_id != 0) {
// Get the hyperlink from the other cursor's page.
const other_page = &other.page_pin.page.data;
const other_link = other_page.hyperlink_set.get(other_page.memory, other.hyperlink_id);
const uri = other_link.uri.offset.ptr(other_page.memory)[0..other_link.uri.len];
const id_ = switch (other_link.id) {
.explicit => |id| id.offset.ptr(other_page.memory)[0..id.len],
.implicit => null,
};
// And it to our cursor.
self.startHyperlink(uri, id_) catch |err| {
// This shouldn't happen because startHyperlink should handle
// resizing. This only happens if we're truly out of RAM. Degrade
// to forgetting the hyperlink.
log.err("failed to update hyperlink on cursor change err={}", .{err});
};
}
}
/// Always use this to write to cursor.page_pin.*.
@ -1755,7 +1787,8 @@ pub fn cursorSetHyperlink(self: *Screen) !void {
self.cursor.page_cell,
self.cursor.hyperlink_id,
)) {
// Success!
// Success, increase the refcount for the hyperlink.
page.hyperlink_set.use(page.memory, self.cursor.hyperlink_id);
return;
} else |err| switch (err) {
// hyperlink_map is out of space, realloc the page to be larger
@ -2876,7 +2909,7 @@ test "Screen cursorCopy x/y" {
var s2 = try Screen.init(alloc, 10, 10, 0);
defer s2.deinit();
try s2.cursorCopy(s.cursor);
try s2.cursorCopy(s.cursor, .{});
try testing.expect(s2.cursor.x == 2);
try testing.expect(s2.cursor.y == 3);
try s2.testWriteString("Hello");
@ -2905,7 +2938,7 @@ test "Screen cursorCopy style deref" {
try testing.expect(s2.cursor.style.flags.bold);
// Copy default style, should release our style
try s2.cursorCopy(s.cursor);
try s2.cursorCopy(s.cursor, .{});
try testing.expect(!s2.cursor.style.flags.bold);
try testing.expectEqual(@as(usize, 0), page.styles.count());
}
@ -2971,7 +3004,7 @@ test "Screen cursorCopy style deref new page" {
// Copy the cursor for the first screen. This should release
// the style from page 1 and move the cursor back to page 0.
try s2.cursorCopy(s.cursor);
try s2.cursorCopy(s.cursor, .{});
try testing.expect(!s2.cursor.style.flags.bold);
try testing.expectEqual(@as(usize, 0), page.styles.count());
// The page after the page the cursor is now in should be page 1.
@ -2992,11 +3025,154 @@ test "Screen cursorCopy style copy" {
var s2 = try Screen.init(alloc, 10, 10, 0);
defer s2.deinit();
const page = &s2.cursor.page_pin.page.data;
try s2.cursorCopy(s.cursor);
try s2.cursorCopy(s.cursor, .{});
try testing.expect(s2.cursor.style.flags.bold);
try testing.expectEqual(@as(usize, 1), page.styles.count());
}
test "Screen cursorCopy hyperlink deref" {
const testing = std.testing;
const alloc = testing.allocator;
var s = try Screen.init(alloc, 10, 10, 0);
defer s.deinit();
var s2 = try Screen.init(alloc, 10, 10, 0);
defer s2.deinit();
const page = &s2.cursor.page_pin.page.data;
// Create a hyperlink for the cursor.
try s2.startHyperlink("https://example.com/", null);
try testing.expectEqual(@as(usize, 1), page.hyperlink_set.count());
try testing.expect(s2.cursor.hyperlink_id != 0);
// Copy a cursor with no hyperlink, should release our hyperlink.
try s2.cursorCopy(s.cursor, .{});
try testing.expectEqual(@as(usize, 0), page.hyperlink_set.count());
try testing.expect(s2.cursor.hyperlink_id == 0);
}
test "Screen cursorCopy hyperlink deref new page" {
const testing = std.testing;
const alloc = testing.allocator;
var s = try init(alloc, 10, 10, 0);
defer s.deinit();
var s2 = try Screen.init(alloc, 10, 10, 2048);
defer s2.deinit();
// We need to get the cursor on a new page.
const first_page_size = s2.pages.pages.first.?.data.capacity.rows;
// Fill the scrollback with blank lines until
// there are only 5 rows left on the first page.
for (0..first_page_size - 5) |_| {
try s2.testWriteString("\n");
}
try s2.testWriteString("1\n2\n3\n4\n5\n6\n7\n8\n9\n10");
// s2.pages.diagram(...):
//
// +----------+ = PAGE 0
// ... : :
// +-------------+ ACTIVE
// 4300 |1 | | 0
// 4301 |2 | | 1
// 4302 |3 | | 2
// 4303 |4 | | 3
// 4304 |5 | | 4
// +----------+ :
// +----------+ : = PAGE 1
// 0 |6 | | 5
// 1 |7 | | 6
// 2 |8 | | 7
// 3 |9 | | 8
// 4 |10 | | 9
// : ^ : : = PIN 0
// +----------+ :
// +-------------+
// This should be PAGE 1
const page = &s2.cursor.page_pin.page.data;
// It should be the last page in the list.
try testing.expectEqual(&s2.pages.pages.last.?.data, page);
// It should have a previous page.
try testing.expect(s2.cursor.page_pin.page.prev != null);
// The cursor should be at 2, 9
try testing.expect(s2.cursor.x == 2);
try testing.expect(s2.cursor.y == 9);
// Create a hyperlink for the cursor, should be in page 1.
try s2.startHyperlink("https://example.com/", null);
try testing.expectEqual(@as(usize, 1), page.hyperlink_set.count());
try testing.expect(s2.cursor.hyperlink_id != 0);
// Copy the cursor for the first screen. This should release
// the hyperlink from page 1 and move the cursor back to page 0.
try s2.cursorCopy(s.cursor, .{});
try testing.expectEqual(@as(usize, 0), page.hyperlink_set.count());
try testing.expect(s2.cursor.hyperlink_id == 0);
// The page after the page the cursor is now in should be page 1.
try testing.expectEqual(page, &s2.cursor.page_pin.page.next.?.data);
// The cursor should be at 0, 0
try testing.expect(s2.cursor.x == 0);
try testing.expect(s2.cursor.y == 0);
}
test "Screen cursorCopy hyperlink copy" {
const testing = std.testing;
const alloc = testing.allocator;
var s = try Screen.init(alloc, 10, 10, 0);
defer s.deinit();
// Create a hyperlink for the cursor.
try s.startHyperlink("https://example.com/", null);
try testing.expectEqual(@as(usize, 1), s.cursor.page_pin.page.data.hyperlink_set.count());
try testing.expect(s.cursor.hyperlink_id != 0);
var s2 = try Screen.init(alloc, 10, 10, 0);
defer s2.deinit();
const page = &s2.cursor.page_pin.page.data;
try testing.expectEqual(@as(usize, 0), page.hyperlink_set.count());
try testing.expect(s2.cursor.hyperlink_id == 0);
// Copy the cursor with the hyperlink.
try s2.cursorCopy(s.cursor, .{});
try testing.expectEqual(@as(usize, 1), page.hyperlink_set.count());
try testing.expect(s2.cursor.hyperlink_id != 0);
}
test "Screen cursorCopy hyperlink copy disabled" {
const testing = std.testing;
const alloc = testing.allocator;
var s = try Screen.init(alloc, 10, 10, 0);
defer s.deinit();
// Create a hyperlink for the cursor.
try s.startHyperlink("https://example.com/", null);
try testing.expectEqual(@as(usize, 1), s.cursor.page_pin.page.data.hyperlink_set.count());
try testing.expect(s.cursor.hyperlink_id != 0);
var s2 = try Screen.init(alloc, 10, 10, 0);
defer s2.deinit();
const page = &s2.cursor.page_pin.page.data;
try testing.expectEqual(@as(usize, 0), page.hyperlink_set.count());
try testing.expect(s2.cursor.hyperlink_id == 0);
// Copy the cursor with the hyperlink.
try s2.cursorCopy(s.cursor, .{ .hyperlink = false });
try testing.expectEqual(@as(usize, 0), page.hyperlink_set.count());
try testing.expect(s2.cursor.hyperlink_id == 0);
}
test "Screen style basics" {
const testing = std.testing;
const alloc = testing.allocator;

19
src/terminal/Terminal.zig
View File

@ -1198,12 +1198,12 @@ pub fn reverseIndex(self: *Terminal) void {
self.scrollDown(1);
}
// Set Cursor Position. Move cursor to the position indicated
// by row and column (1-indexed). If column is 0, it is adjusted to 1.
// If column is greater than the right-most column it is adjusted to
// the right-most column. If row is 0, it is adjusted to 1. If row is
// greater than the bottom-most row it is adjusted to the bottom-most
// row.
/// Set Cursor Position. Move cursor to the position indicated
/// by row and column (1-indexed). If column is 0, it is adjusted to 1.
/// If column is greater than the right-most column it is adjusted to
/// the right-most column. If row is 0, it is adjusted to 1. If row is
/// greater than the bottom-most row it is adjusted to the bottom-most
/// row.
pub fn setCursorPos(self: *Terminal, row_req: usize, col_req: usize) void {
// If cursor origin mode is set the cursor row will be moved relative to
// the top margin row and adjusted to be above or at bottom-most row in
@ -2473,13 +2473,12 @@ pub fn alternateScreen(
self.flags.dirty.clear = true;
// Bring our pen with us
self.screen.cursorCopy(old.cursor) catch |err| {
self.screen.cursorCopy(old.cursor, .{
.hyperlink = false,
}) catch |err| {
log.warn("cursor copy failed entering alt screen err={}", .{err});
};
// We always end hyperlink state
self.screen.endHyperlink();
if (options.clear_on_enter) {
self.eraseDisplay(.complete, false);
}

143
src/terminal/page.zig
View File

@ -745,15 +745,17 @@ pub const Page = struct {
if (src_cell.hyperlink) hyperlink: {
dst_row.hyperlink = true;
// Fast-path: same page we can move it directly
const id = other.lookupHyperlink(src_cell).?;
// Fast-path: same page we can add with the same id.
if (other == self) {
self.moveHyperlink(src_cell, dst_cell);
self.hyperlink_set.use(self.memory, id);
try self.setHyperlink(dst_row, dst_cell, id);
break :hyperlink;
}
// Slow-path: get the hyperlink from the other page,
// add it, and migrate.
const id = other.lookupHyperlink(src_cell).?;
const dst_link = dst_link: {
// Fast path is we just dupe the hyperlink because
@ -1080,8 +1082,14 @@ pub const Page = struct {
}
/// Set the hyperlink for the given cell. If the cell already has a
/// hyperlink, then this will handle memory management for the prior
/// hyperlink.
/// hyperlink, then this will handle memory management and refcount
/// update for the prior hyperlink.
///
/// DOES NOT increment the reference count for the new hyperlink!
///
/// Caller is responsible for updating the refcount in the hyperlink
/// set as necessary by calling `use` if the id was not acquired with
/// `add`.
pub fn setHyperlink(self: *Page, row: *Row, cell: *Cell, id: hyperlink.Id) !void {
defer self.assertIntegrity();
@ -1090,6 +1098,13 @@ pub const Page = struct {
const gop = try map.getOrPut(cell_offset);
if (gop.found_existing) {
// Always release the old hyperlink, because even if it's actually
// the same as the one we're setting, we'd end up double-counting
// if we left the reference count be, because the caller does not
// know whether it's the same and will have increased the count
// outside of this function.
self.hyperlink_set.release(self.memory, gop.value_ptr.*);
// If the hyperlink matches then we don't need to do anything.
if (gop.value_ptr.* == id) {
// It is possible for cell hyperlink to be false but row
@ -1102,13 +1117,9 @@ pub const Page = struct {
cell.hyperlink = true;
return;
}
// Different hyperlink, we need to release the old one
self.hyperlink_set.release(self.memory, gop.value_ptr.*);
}
// Increase ref count for our new hyperlink and set it
self.hyperlink_set.use(self.memory, id);
// Set the hyperlink on the cell and in the map.
gop.value_ptr.* = id;
cell.hyperlink = true;
row.hyperlink = true;
@ -2436,6 +2447,118 @@ test "Page cloneRowFrom partial grapheme in non-copied dest region" {
try testing.expectEqual(@as(usize, 2), page2.graphemeCount());
}
test "Page cloneRowFrom partial hyperlink in same page copy" {
var page = try Page.init(.{ .cols = 10, .rows = 10 });
defer page.deinit();
// We need to create a hyperlink.
const hyperlink_id = try page.hyperlink_set.addContext(
page.memory,
.{ .id = .{ .implicit = 0 }, .uri = .{} },
.{ .page = &page },
);
// Write
{
const y = 0;
for (0..page.size.cols) |x| {
const rac = page.getRowAndCell(x, y);
rac.cell.* = .{
.content_tag = .codepoint,
.content = .{ .codepoint = @intCast(x + 1) },
};
}
// Hyperlink in a single cell
{
const rac = page.getRowAndCell(7, y);
try page.setHyperlink(rac.row, rac.cell, hyperlink_id);
}
}
try testing.expectEqual(@as(usize, 1), page.hyperlinkCount());
// Clone into the same page
try page.clonePartialRowFrom(
&page,
page.getRow(1),
page.getRow(0),
2,
8,
);
// Read it again
{
const y = 1;
for (0..page.size.cols) |x| {
const expected: u21 = if (x >= 2 and x < 8) @intCast(x + 1) else 0;
const rac = page.getRowAndCell(x, y);
try testing.expectEqual(expected, rac.cell.content.codepoint);
}
{
const rac = page.getRowAndCell(7, y);
try testing.expect(rac.row.hyperlink);
try testing.expect(rac.cell.hyperlink);
}
}
try testing.expectEqual(@as(usize, 2), page.hyperlinkCount());
}
test "Page cloneRowFrom partial hyperlink in same page omit" {
var page = try Page.init(.{ .cols = 10, .rows = 10 });
defer page.deinit();
// We need to create a hyperlink.
const hyperlink_id = try page.hyperlink_set.addContext(
page.memory,
.{ .id = .{ .implicit = 0 }, .uri = .{} },
.{ .page = &page },
);
// Write
{
const y = 0;
for (0..page.size.cols) |x| {
const rac = page.getRowAndCell(x, y);
rac.cell.* = .{
.content_tag = .codepoint,
.content = .{ .codepoint = @intCast(x + 1) },
};
}
// Hyperlink in a single cell
{
const rac = page.getRowAndCell(7, y);
try page.setHyperlink(rac.row, rac.cell, hyperlink_id);
}
}
try testing.expectEqual(@as(usize, 1), page.hyperlinkCount());
// Clone into the same page
try page.clonePartialRowFrom(
&page,
page.getRow(1),
page.getRow(0),
2,
6,
);
// Read it again
{
const y = 1;
for (0..page.size.cols) |x| {
const expected: u21 = if (x >= 2 and x < 6) @intCast(x + 1) else 0;
const rac = page.getRowAndCell(x, y);
try testing.expectEqual(expected, rac.cell.content.codepoint);
}
{
const rac = page.getRowAndCell(7, y);
try testing.expect(!rac.row.hyperlink);
try testing.expect(!rac.cell.hyperlink);
}
}
try testing.expectEqual(@as(usize, 1), page.hyperlinkCount());
}
test "Page moveCells text-only" {
var page = try Page.init(.{
.cols = 10,

4
src/terminal/size.zig
View File

@ -26,8 +26,8 @@ pub fn Offset(comptime T: type) type {
/// A slice of type T that stores via a base offset and len.
pub const Slice = struct {
offset: Self,
len: usize,
offset: Self = .{},
len: usize = 0,
};
/// Returns a pointer to the start of the data, properly typed.