From 902913554b4f407dbb9ceb4fc28ceb9972bb1fab Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 23 Jul 2024 15:58:12 -0700 Subject: [PATCH] terminal: printing over a cell with the same hyperlink keeps flag Fixes #1990 This fixes and adds a unit test for an edge case where when printing over the same cell with the same hyperlink ID, we were unsetting the cell hyperlink state. This commit also adds a number of integrity checks to verify hyperlinks remain in a consistent state. --- src/terminal/Terminal.zig | 27 +++++++++++++ src/terminal/page.zig | 79 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 101 insertions(+), 5 deletions(-) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index a25c46838..c58377374 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -3824,6 +3824,33 @@ test "Terminal: print with hyperlink" { try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); } +test "Terminal: print over cell with same hyperlink" { + var t = try init(testing.allocator, .{ .cols = 80, .rows = 80 }); + defer t.deinit(testing.allocator); + + // Setup our hyperlink and print + try t.screen.startHyperlink("http://example.com", null); + try t.printString("123456"); + t.setCursorPos(1, 1); + try t.printString("123456"); + + // Verify all our cells have a hyperlink + for (0..6) |x| { + const list_cell = t.screen.pages.getCell(.{ .screen = .{ + .x = @intCast(x), + .y = 0, + } }).?; + const row = list_cell.row; + try testing.expect(row.hyperlink); + const cell = list_cell.cell; + try testing.expect(cell.hyperlink); + const id = list_cell.page.data.lookupHyperlink(cell).?; + try testing.expectEqual(@as(hyperlink.Id, 1), id); + } + + try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); +} + test "Terminal: print and end hyperlink" { var t = try init(testing.allocator, .{ .cols = 80, .rows = 80 }); defer t.deinit(testing.allocator); diff --git a/src/terminal/page.zig b/src/terminal/page.zig index ec8bbc370..677e3fb4a 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -291,6 +291,10 @@ pub const Page = struct { UnmarkedStyleRow, MismatchedStyleRef, InvalidStyleCount, + MissingHyperlinkData, + MismatchedHyperlinkRef, + UnmarkedHyperlinkCell, + UnmarkedHyperlinkRow, InvalidSpacerTailLocation, InvalidSpacerHeadLocation, UnwrappedSpacerHead, @@ -356,6 +360,8 @@ pub const Page = struct { var graphemes_seen: usize = 0; var styles_seen = std.AutoHashMap(style.Id, usize).init(alloc); defer styles_seen.deinit(); + var hyperlinks_seen = std.AutoHashMap(hyperlink.Id, usize).init(alloc); + defer hyperlinks_seen.deinit(); const rows = self.rows.ptr(self.memory)[0..self.size.rows]; for (rows, 0..) |*row, y| { @@ -397,6 +403,41 @@ pub const Page = struct { gop.value_ptr.* += 1; } + if (cell.hyperlink) { + const id = self.lookupHyperlink(cell) orelse { + log.warn( + "page integrity violation y={} x={} hyperlink data missing", + .{ y, x }, + ); + return IntegrityError.MissingHyperlinkData; + }; + + if (!row.hyperlink) { + log.warn( + "page integrity violation y={} x={} row not marked as hyperlink", + .{ y, x }, + ); + return IntegrityError.UnmarkedHyperlinkRow; + } + + const gop = try hyperlinks_seen.getOrPut(id); + if (!gop.found_existing) gop.value_ptr.* = 0; + gop.value_ptr.* += 1; + + // Hyperlink ID should be valid. This just straight crashes + // if this fails due to assertions. + _ = self.hyperlink_set.get(self.memory, id); + } else { + // It should not have hyperlink data if it isn't marked + if (self.lookupHyperlink(cell) != null) { + log.warn( + "page integrity violation y={} x={} cell not marked as hyperlink", + .{ y, x }, + ); + return IntegrityError.UnmarkedHyperlinkCell; + } + } + switch (cell.wide) { .narrow => {}, .wide => {}, @@ -483,6 +524,21 @@ pub const Page = struct { } } + // Verify all our hyperlinks have the correct ref count. + { + var it = hyperlinks_seen.iterator(); + while (it.next()) |entry| { + const ref_count = self.hyperlink_set.refCount(self.memory, entry.key_ptr.*); + if (ref_count < entry.value_ptr.*) { + log.warn( + "page integrity violation hyperlink ref count mismatch id={} expected={} actual={}", + .{ entry.key_ptr.*, entry.value_ptr.*, ref_count }, + ); + return IntegrityError.MismatchedHyperlinkRef; + } + } + } + // Verify there are no zombie styles, that is, styles in the // set with ref counts > 0, which are not present in the page. { @@ -1017,7 +1073,17 @@ pub const Page = struct { if (gop.found_existing) { // If the hyperlink matches then we don't need to do anything. - if (gop.value_ptr.* == id) return; + if (gop.value_ptr.* == id) { + // It is possible for cell hyperlink to be false but row + // must never be false. The cell hyperlink can be false because + // in Terminal.print we clear the hyperlink for the cursor cell + // before writing the cell again, so if someone prints over + // a cell with a matching hyperlink this state can happen. + // This is tested in Terminal.zig. + assert(row.hyperlink); + cell.hyperlink = true; + return; + } // Different hyperlink, we need to release the old one self.hyperlink_set.release(self.memory, gop.value_ptr.*); @@ -1034,10 +1100,8 @@ pub const Page = struct { /// because we avoid any allocations since we're just moving data. /// Destination must NOT have a hyperlink. fn moveHyperlink(self: *Page, src: *Cell, dst: *Cell) void { - if (comptime std.debug.runtime_safety) { - assert(src.hyperlink); - assert(!dst.hyperlink); - } + assert(src.hyperlink); + assert(!dst.hyperlink); const src_offset = getOffset(Cell, self.memory, src); const dst_offset = getOffset(Cell, self.memory, dst); @@ -1046,6 +1110,11 @@ pub const Page = struct { const value = entry.value_ptr.*; map.removeByPtr(entry.key_ptr); map.putAssumeCapacity(dst_offset, value); + + // NOTE: We must not set src/dst.hyperlink here because this + // function is used in various cases where we swap cell contents + // and its unsafe. The flip side: the caller must be careful + // to set the proper cell state to represent the move. } /// Returns the number of hyperlinks in the page. This isn't the byte