diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index 89666e7bb..1629678f6 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -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] \t" + // std.log.err("[updateFrame critical time] {}\t{}", .{start_micro, end.since(start) / std.time.ns_per_us}); + // } + state.mutex.lock(); defer state.mutex.unlock(); diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 523b0e195..c0ff5a548 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -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; + 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) { - self.page_cell.style_id = stylepkg.default_id; - } + // 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( diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 2d87c6012..c6118627c 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -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; diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 667e28650..1b32eac31 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -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); } diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 18a5601b7..b1acfc2ba 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -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, diff --git a/src/terminal/size.zig b/src/terminal/size.zig index fead2b469..ffa61e6be 100644 --- a/src/terminal/size.zig +++ b/src/terminal/size.zig @@ -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.