From 7ce4010f7a37b2a8aa4f8f62858f741b3e3f15ae Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Wed, 28 Feb 2024 09:19:27 -0800
Subject: [PATCH] terminal/new: scrolling viewport into active area pins to
 active

---
 src/terminal/Screen.zig       |  1 +
 src/terminal/new/PageList.zig | 88 +++++++++++++++++++----------------
 src/terminal/new/Screen.zig   | 19 ++++++++
 3 files changed, 68 insertions(+), 40 deletions(-)

diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig
index 53471ac4a..0e00ec2fc 100644
--- a/src/terminal/Screen.zig
+++ b/src/terminal/Screen.zig
@@ -3839,6 +3839,7 @@ test "Screen: scrolling" {
     }
 }
 
+// X
 test "Screen: scroll down from 0" {
     const testing = std.testing;
     const alloc = testing.allocator;
diff --git a/src/terminal/new/PageList.zig b/src/terminal/new/PageList.zig
index 90a77df42..c188c1a0c 100644
--- a/src/terminal/new/PageList.zig
+++ b/src/terminal/new/PageList.zig
@@ -203,54 +203,43 @@ pub fn scroll(self: *PageList, behavior: Scroll) void {
     switch (behavior) {
         .active => self.viewport = .{ .active = {} },
         .top => self.viewport = .{ .top = {} },
-        .delta_row => |n| {
+        .delta_row => |n| delta_row: {
             if (n == 0) return;
 
             const top = self.getTopLeft(.viewport);
             const offset: RowOffset = if (n < 0) switch (top.backwardOverflow(@intCast(-n))) {
                 .offset => |v| v,
                 .overflow => |v| v.end,
-            } else forward: {
-                // Not super happy with the logic to scroll forward. I think
-                // this is pretty slow, but it is human-driven (scrolling
-                // this way) so hyper speed isn't strictly necessary. Still,
-                // it feels bad.
-
-                const forward_offset = switch (top.forwardOverflow(@intCast(n))) {
-                    .offset => |v| v,
-                    .overflow => |v| v.end,
-                };
-
-                var final_offset: ?RowOffset = forward_offset;
-
-                // Ensure we have at least rows rows in the viewport. There
-                // is probably a smarter way to do this.
-                var page = self.pages.last.?;
-                var rem = self.rows;
-                while (rem > page.data.size.rows) {
-                    rem -= page.data.size.rows;
-
-                    // If we see our forward page here then we know its
-                    // beyond the active area and we can set final null.
-                    if (page == forward_offset.page) final_offset = null;
-
-                    page = page.prev.?; // assertion: we always have enough rows for active
-                }
-                const active_offset = .{ .page = page, .row_offset = page.data.size.rows - rem };
-
-                // If we have a final still and we're on the same page
-                // but the active area is before the forward area, then
-                // we can use the active area.
-                if (final_offset != null and
-                    active_offset.page == forward_offset.page and
-                    forward_offset.row_offset > active_offset.row_offset)
-                {
-                    final_offset = active_offset;
-                }
-
-                break :forward final_offset orelse active_offset;
+            } else switch (top.forwardOverflow(@intCast(n))) {
+                .offset => |v| v,
+                .overflow => |v| v.end,
             };
 
+            // If we are still within the active area, then we pin the
+            // viewport to active. This isn't EXACTLY the same behavior as
+            // other scrolling because normally when you scroll the viewport
+            // is pinned to _that row_ even if new scrollback is created.
+            // But in a terminal when you get to the bottom and back into the
+            // active area, you usually expect that the viewport will now
+            // follow the active area.
+            const active = self.getTopLeft(.active);
+            if (offset.page == active.page) {
+                if (offset.row_offset >= active.row_offset) {
+                    self.viewport = .{ .active = {} };
+                    break :delta_row;
+                }
+            } else active: {
+                // Check forward pages too.
+                var page = active.page.next orelse break :active;
+                while (true) {
+                    if (page == offset.page) {
+                        self.viewport = .{ .active = {} };
+                        break :delta_row;
+                    }
+                    page = page.next orelse break :active;
+                }
+            }
+
             self.viewport = .{ .exact = offset };
         },
     }
@@ -1100,6 +1089,25 @@ test "PageList scroll delta row forward into active" {
     }
 }
 
+test "PageList scroll delta row back without space preserves active" {
+    const testing = std.testing;
+    const alloc = testing.allocator;
+
+    var s = try init(alloc, 80, 24, null);
+    defer s.deinit();
+    s.scroll(.{ .delta_row = -1 });
+
+    {
+        const pt = s.getCell(.{ .viewport = .{} }).?.screenPoint();
+        try testing.expectEqual(point.Point{ .screen = .{
+            .x = 0,
+            .y = 0,
+        } }, pt);
+    }
+
+    try testing.expect(s.viewport == .active);
+}
+
 test "PageList scroll clear" {
     const testing = std.testing;
     const alloc = testing.allocator;
diff --git a/src/terminal/new/Screen.zig b/src/terminal/new/Screen.zig
index 8eb202189..434971bc1 100644
--- a/src/terminal/new/Screen.zig
+++ b/src/terminal/new/Screen.zig
@@ -1113,3 +1113,22 @@ test "Screen: scrolling" {
         try testing.expectEqualStrings("2EFGH\n3IJKL", contents);
     }
 }
+
+test "Screen: scroll down from 0" {
+    const testing = std.testing;
+    const alloc = testing.allocator;
+
+    var s = try init(alloc, 10, 3, 0);
+    defer s.deinit();
+    try s.testWriteString("1ABCD\n2EFGH\n3IJKL");
+
+    // Scrolling up does nothing, but allows it
+    s.scroll(.{ .delta_row = -1 });
+    try testing.expect(s.pages.viewport == .active);
+
+    {
+        const contents = try s.dumpStringAlloc(alloc, .{ .viewport = .{} });
+        defer alloc.free(contents);
+        try testing.expectEqualStrings("1ABCD\n2EFGH\n3IJKL", contents);
+    }
+}