From 30bba9bf067ac9384ae1a93d28f8e3e97515f8ee Mon Sep 17 00:00:00 2001
From: Qwerasd <qwerasd205@users.noreply.github.com>
Date: Tue, 27 Aug 2024 00:54:22 -0400
Subject: [PATCH] terminal: move refcount responsibility out of setHyperlink

avoids double counting in several places
---
 src/terminal/Screen.zig |  3 ++-
 src/terminal/page.zig   | 23 ++++++++++++++++-------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig
index 2d87c6012..afb1510a5 100644
--- a/src/terminal/Screen.zig
+++ b/src/terminal/Screen.zig
@@ -1755,7 +1755,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
diff --git a/src/terminal/page.zig b/src/terminal/page.zig
index 18a5601b7..2110b531f 100644
--- a/src/terminal/page.zig
+++ b/src/terminal/page.zig
@@ -1080,8 +1080,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 +1096,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 +1115,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;