From af6cc663696bbbe7f540746b5c95935acebf1573 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 11 Nov 2023 22:10:00 -0800 Subject: [PATCH] core: Fix various double-click word selection bugs Fixes #741 This completely reimplements double-click-and-drag logic for selecting by word. The previous implementation was horribly broken. See #741 for all the details. The implemented logic now is: * A double-click initiates a select-by-word selection mechanism. - A double-click may start on a word or whitespace - If the initial double-click is on a word, that word is immediately selected. - If the initial double-click is on whitespace, the whitespace is not selected. * A "word" is determined by a non-boundary character meeting a boundary character. - A boundary character is `NUL` ` ` (space) `\t` `'` `"` - This list is somewhat arbitrary to make the terminal "feel" good. - Cell SGR states (fg/bg, bold, italic, etc.) have no effect on boundary determination or selection logic. * As the user drags _on the same line_: - No selection change occurs until the cursor is over a new word. Whitespace change does nothing. - When selection is over a new word, that entire word added to the selection. * When the user drags _up_ one or more lines: - If the cursor is over whitespace, all lines from the selection point up to but not including the cursor line are selected. * This selection is done in accordance to the previous rules. - If the cursor is over a word, the word becomes the beginning of the selection. - The end of the selection in all cases is the first word at or before the initial double-click point. * When the user drags _down_ one or more lines: - The same logic as _up_ but swap the "beginning" and "end" of selection terminology. * With this logic, the behavior of Ghostty has the following invariants: - Whitespace is never selected unless it is between two selected words - Selection implies at least one word is highlighted - The initial double-click point marks the beginning or end of a selection, never the middle. --- src/Surface.zig | 37 +++++++++----- src/terminal/Screen.zig | 24 +++++++++ src/terminal/point.zig | 108 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 12 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index 38d1c6bcb..9454f24a8 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -1990,24 +1990,37 @@ fn dragLeftClickDouble( self: *Surface, screen_point: terminal.point.ScreenPoint, ) void { - // Get the word under our current point. If there isn't a word, do nothing. - const word = self.io.terminal.screen.selectWord(screen_point) orelse return; - - // Get our selection to grow it. If we don't have a selection, start it now. - // We may not have a selection if we started our dbl-click in an area - // that had no data, then we dragged our mouse into an area with data. - var sel = self.io.terminal.screen.selectWord(self.mouse.left_click_point) orelse { - self.setSelection(word); + // Get the word closest to our starting click. + const word_start = self.io.terminal.screen.selectWordBetween( + self.mouse.left_click_point, + screen_point, + ) orelse { + self.setSelection(null); return; }; - // Grow our selection + // Get the word closest to our current point. + const word_current = self.io.terminal.screen.selectWordBetween( + screen_point, + self.mouse.left_click_point, + ) orelse { + self.setSelection(null); + return; + }; + + // If our current mouse position is before the starting position, + // then the seletion start is the word nearest our current position. if (screen_point.before(self.mouse.left_click_point)) { - sel.start = word.start; + self.setSelection(.{ + .start = word_current.start, + .end = word_start.end, + }); } else { - sel.end = word.end; + self.setSelection(.{ + .start = word_start.start, + .end = word_current.end, + }); } - self.setSelection(sel); } /// Triple-click dragging moves the selection one "line" at a time. diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 353cfa8a8..9e27873bf 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1494,6 +1494,30 @@ pub fn selectLine(self: *Screen, pt: point.ScreenPoint) ?Selection { }; } +/// Select the nearest word to start point that is between start_pt and +/// end_pt (inclusive). Because it selects "nearest" to start point, start +/// point can be before or after end point. +pub fn selectWordBetween( + self: *Screen, + start_pt: point.ScreenPoint, + end_pt: point.ScreenPoint, +) ?Selection { + const dir: point.Direction = if (start_pt.before(end_pt)) .right_down else .left_up; + var it = start_pt.iterator(self, dir); + while (it.next()) |pt| { + // Boundary conditions + switch (dir) { + .right_down => if (end_pt.before(pt)) return null, + .left_up => if (pt.before(end_pt)) return null, + } + + // If we found a word, then return it + if (self.selectWord(pt)) |sel| return sel; + } + + return null; +} + /// Select the word under the given point. A word is any consecutive series /// of characters that are exclusively whitespace or exclusively non-whitespace. /// A selection can span multiple physical lines if they are soft-wrapped. diff --git a/src/terminal/point.zig b/src/terminal/point.zig index b0f851acd..fc248cbda 100644 --- a/src/terminal/point.zig +++ b/src/terminal/point.zig @@ -80,6 +80,11 @@ pub const ScreenPoint = struct { (self.y == other.y and self.x < other.x); } + /// Returns if two points are equal. + pub fn eql(self: ScreenPoint, other: ScreenPoint) bool { + return self.x == other.x and self.y == other.y; + } + /// Returns true if this screen point is currently in the active viewport. pub fn inViewport(self: ScreenPoint, screen: *const Screen) bool { return self.y >= screen.viewport and @@ -104,6 +109,62 @@ pub const ScreenPoint = struct { return .{ .x = self.x, .y = self.y - screen.viewport }; } + /// Returns a screen point iterator. This will iterate over all of + /// of the points in a screen in a given direction one by one. + /// + /// The iterator is only valid as long as the screen is not resized. + pub fn iterator( + self: ScreenPoint, + screen: *const Screen, + dir: Direction, + ) Iterator { + return .{ .screen = screen, .current = self, .direction = dir }; + } + + pub const Iterator = struct { + screen: *const Screen, + current: ?ScreenPoint, + direction: Direction, + + pub fn next(self: *Iterator) ?ScreenPoint { + const current = self.current orelse return null; + self.current = switch (self.direction) { + .left_up => left_up: { + if (current.x == 0) { + if (current.y == 0) break :left_up null; + break :left_up .{ + .x = self.screen.cols - 1, + .y = current.y - 1, + }; + } + + break :left_up .{ + .x = current.x - 1, + .y = current.y, + }; + }, + + .right_down => right_down: { + if (current.x == self.screen.cols - 1) { + const max = self.screen.rows + self.screen.max_scrollback; + if (current.y == max - 1) break :right_down null; + break :right_down .{ + .x = 0, + .y = current.y + 1, + }; + } + + break :right_down .{ + .x = current.x + 1, + .y = current.y, + }; + }, + }; + + return current; + } + }; + test "before" { const testing = std.testing; @@ -111,8 +172,55 @@ pub const ScreenPoint = struct { try testing.expect(p.before(.{ .x = 6, .y = 2 })); try testing.expect(p.before(.{ .x = 3, .y = 3 })); } + + test "iterator" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try Screen.init(alloc, 5, 5, 0); + defer s.deinit(); + + // Back from the first line + { + var pt: ScreenPoint = .{ .x = 1, .y = 0 }; + var it = pt.iterator(&s, .left_up); + try testing.expectEqual(ScreenPoint{ .x = 1, .y = 0 }, it.next().?); + try testing.expectEqual(ScreenPoint{ .x = 0, .y = 0 }, it.next().?); + try testing.expect(it.next() == null); + } + + // Back from second line + { + var pt: ScreenPoint = .{ .x = 1, .y = 1 }; + var it = pt.iterator(&s, .left_up); + try testing.expectEqual(ScreenPoint{ .x = 1, .y = 1 }, it.next().?); + try testing.expectEqual(ScreenPoint{ .x = 0, .y = 1 }, it.next().?); + try testing.expectEqual(ScreenPoint{ .x = 4, .y = 0 }, it.next().?); + } + + // Forward last line + { + var pt: ScreenPoint = .{ .x = 3, .y = 4 }; + var it = pt.iterator(&s, .right_down); + try testing.expectEqual(ScreenPoint{ .x = 3, .y = 4 }, it.next().?); + try testing.expectEqual(ScreenPoint{ .x = 4, .y = 4 }, it.next().?); + try testing.expect(it.next() == null); + } + + // Forward not last line + { + var pt: ScreenPoint = .{ .x = 3, .y = 3 }; + var it = pt.iterator(&s, .right_down); + try testing.expectEqual(ScreenPoint{ .x = 3, .y = 3 }, it.next().?); + try testing.expectEqual(ScreenPoint{ .x = 4, .y = 3 }, it.next().?); + try testing.expectEqual(ScreenPoint{ .x = 0, .y = 4 }, it.next().?); + } + } }; +/// Direction that points can go. +pub const Direction = enum { left_up, right_down }; + test { std.testing.refAllDecls(@This()); }