fix-deviant(links): bound hover lookup and renderer regex work
Follow-up testing showed that the mailbox limit did not fix the base64 hang: moving the pointer stalled the app, and opening a tab could hang it. Mouse link lookup copied the entire wrapped line before checking modifier eligibility, while renderer link searches had no whole-search work budget. Filter eligible links before traversing scrollback. Add bounded logical line selection and skip automatic regex lookup on lines exceeding 65,536 grid cells. Keep explicit line selection and OSC8 handling unrestricted. Skip oversized lines instead of truncating URLs into different targets. Apply the existing terminal StringMap retry budget to renderer searches and handle search errors without panicking. The regression triggered a retry-limit panic through the old onig search API and now returns normally, allowing subsequent link patterns to run. Document the lookup limits and correct the earlier mailbox fix entry to record that it did not resolve the reported hang on its own. Test Plan: - 1,513 unit/integration tests and one doctest pass across core, renderer, and terminal with Mesa software OpenGL; one existing doctest is ignored. - Release application build, formatting, and all-target Clippy pass. - GTK handles 400 pointer moves, including Ctrl hover, during continuous base64 output, opens/closes a tab, handles Ctrl+C, prints a marker, and exits cleanly. Normal URL hover still works after a hard newline. Reference: docs/rewrite/DEVIANT_FIXES.md (entry 41)
This commit is contained in:
1 parent
4c31d27888
commit
9010117d08
9 files changed
+153
-24
No files matched your search
@@ -49,6 +49,10 @@ use ghostty_vt::modes::Mode;
|
||||
use super::{MOUSE_BUTTON_COUNT, Surface};
|
||||
use crate::surface_mouse;
|
||||
|
||||
/// Bound synchronous link lookup while the terminal mutex is held. Oversized
|
||||
/// logical lines are skipped, rather than truncating a URL into another target.
|
||||
const MAX_LINK_CELLS: usize = 64 * 1024;
|
||||
|
||||
/// Mouse state for the surface (Zig: `Surface.Mouse`).
|
||||
pub struct Mouse {
|
||||
/// The last tracked mouse button state by button.
|
||||
@@ -1570,35 +1574,37 @@ impl Surface {
|
||||
mouse_mods: Option<Mods>,
|
||||
) -> Option<Link> {
|
||||
let cfg = self.cfg();
|
||||
if cfg.links.is_empty() {
|
||||
// Check eligibility before walking scrollback or allocating a string
|
||||
// map. Ordinary mouse movement needs no URL lookup without Ctrl/Super.
|
||||
let mut links = cfg
|
||||
.links
|
||||
.iter()
|
||||
.filter(|link| {
|
||||
mouse_mods.is_none_or(|mods| match link.highlight {
|
||||
Highlight::Always | Highlight::Hover => true,
|
||||
Highlight::AlwaysMods(v) | Highlight::HoverMods(v) => v.equal(mods),
|
||||
})
|
||||
})
|
||||
.peekable();
|
||||
if links.peek().is_none() {
|
||||
return None;
|
||||
}
|
||||
|
||||
let line = screen.select_line(SelectLine {
|
||||
pin: mouse_pin,
|
||||
whitespace: None,
|
||||
semantic_prompt_boundary: false,
|
||||
})?;
|
||||
let line = screen.select_line_bounded(
|
||||
SelectLine {
|
||||
pin: mouse_pin,
|
||||
whitespace: None,
|
||||
semantic_prompt_boundary: false,
|
||||
},
|
||||
MAX_LINK_CELLS / usize::from(screen.pages.cols),
|
||||
)?;
|
||||
|
||||
let strmap = screen.selection_string_map(SelectionString {
|
||||
sel: line,
|
||||
trim: false,
|
||||
});
|
||||
|
||||
for link in &cfg.links {
|
||||
// Skip highlight/mods check when mouse_mods is None
|
||||
// (double-click mode)
|
||||
if let Some(mods) = mouse_mods {
|
||||
match link.highlight {
|
||||
Highlight::Always | Highlight::Hover => {}
|
||||
Highlight::AlwaysMods(v) | Highlight::HoverMods(v) => {
|
||||
if !v.equal(mods) {
|
||||
continue;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
for link in links {
|
||||
let mut it = strmap.search_iterator(&link.regex);
|
||||
while let Some(m) = it.next_match() {
|
||||
let sel = m.selection();
|
||||
|
||||
@@ -342,6 +342,43 @@ fn regex_url_ctrl_click_opens_and_plain_click_does_not() {
|
||||
});
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn long_unwrapped_output_allows_mouse_motion_and_new_tab_shortcut() {
|
||||
let h = harness!(Harness::recorder("", &[]));
|
||||
let (cols, rows) = h.grid();
|
||||
{
|
||||
let mut t = h.surface.lock_terminal();
|
||||
t.full_reset();
|
||||
// One logical line extending far beyond the viewport into history,
|
||||
// as produced by base64 -w0. Keep an otherwise valid URL at its tail.
|
||||
let mut bytes = vec![b'a'; usize::from(cols) * 4096];
|
||||
bytes.extend_from_slice(b"https://example.com");
|
||||
t.vt_stream().next_slice(&bytes).unwrap();
|
||||
}
|
||||
h.clear_actions();
|
||||
for mods in [none(), ctrl(), ctrl_shift()] {
|
||||
h.move_to(5, u32::from(rows - 1), Some(mods));
|
||||
h.move_to(6, u32::from(rows - 1), Some(mods));
|
||||
}
|
||||
h.key(Key::KeyT, ctrl_shift(), "T");
|
||||
assert!(h.has_action(|a| matches!(a, Action::NewTab)));
|
||||
assert!(!h.has_action(|a| matches!(a, Action::MouseOverLink(l) if !l.url.is_empty())));
|
||||
|
||||
// A hard newline bounds the next URL normally; skipping an oversized
|
||||
// logical line must not disable link detection on subsequent output.
|
||||
{
|
||||
let mut t = h.surface.lock_terminal();
|
||||
t.vt_stream()
|
||||
.next_slice(b"\r\nhttps://example.com")
|
||||
.unwrap();
|
||||
}
|
||||
h.move_to(7, u32::from(rows - 1), Some(ctrl()));
|
||||
h.wait_action("URL hover after long output", |a| match a {
|
||||
Action::MouseOverLink(l) if l.url.to_text_lossy() == "https://example.com" => Some(()),
|
||||
_ => None,
|
||||
});
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn middle_click_pastes_selection_clipboard() {
|
||||
let h = harness!(Harness::recorder("", &[]));
|
||||
|
||||
@@ -13,6 +13,7 @@ ghostty-os.workspace = true
|
||||
ghostty-datastruct.workspace = true
|
||||
ghostty-font.workspace = true
|
||||
ghostty-input.workspace = true
|
||||
ghostty-onig.workspace = true
|
||||
ghostty-terminal.workspace = true
|
||||
ghostty-unicode.workspace = true
|
||||
ghostty-vt.workspace = true
|
||||
|
||||
@@ -7,7 +7,12 @@ use ghostty_input::{
|
||||
link::{Highlight, Link as InputLink},
|
||||
};
|
||||
use ghostty_terminal::{point::Coordinate, render::RenderState};
|
||||
use onig::{Regex, Region, SearchOptions};
|
||||
use onig::{MatchParam, Regex, Region, SearchOptions};
|
||||
|
||||
/// Match the whole-search work budget used by terminal StringMap searches.
|
||||
/// Renderer searches also need a limit: a busy matcher holds the renderer
|
||||
/// thread's state lock and can block tab switches and renderer shutdown.
|
||||
const ONI_SEARCH_RETRY_LIMIT: u32 = 100_000;
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
pub enum Error {
|
||||
@@ -103,16 +108,20 @@ impl Set {
|
||||
while offset < str.len() {
|
||||
let haystack = &str[offset..];
|
||||
let mut region = Region::new();
|
||||
let found = link.regex.search_with_options(
|
||||
let mut param = MatchParam::default();
|
||||
ghostty_onig::set_retry_limit_in_search(&mut param, ONI_SEARCH_RETRY_LIMIT);
|
||||
let found = link.regex.search_with_param(
|
||||
haystack,
|
||||
0,
|
||||
haystack.len(),
|
||||
SearchOptions::SEARCH_OPTION_NONE,
|
||||
Some(&mut region),
|
||||
param,
|
||||
);
|
||||
// A mismatch (or any search failure such as hitting the
|
||||
// retry limit) ends the search for this link.
|
||||
let Some((offset_start, offset_end)) = found.and(region.pos(0)) else {
|
||||
let Some((offset_start, offset_end)) = found.ok().flatten().and(region.pos(0))
|
||||
else {
|
||||
break;
|
||||
};
|
||||
|
||||
@@ -284,4 +293,24 @@ mod tests {
|
||||
fn invalid_regex_is_error() {
|
||||
assert!(Set::from_config(&[link("(", Highlight::Always)]).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn render_cell_map_limits_backtracking_and_continues_with_other_links() {
|
||||
let mut t = Terminal::new(Options::new(80, 1)).unwrap();
|
||||
// A long near-match must not monopolize the renderer thread. In a
|
||||
// tab switch the main thread may need that thread's locks to proceed.
|
||||
let text = format!("{}b", "a".repeat(60));
|
||||
t.vt_stream().next_slice(text.as_bytes()).unwrap();
|
||||
let mut state = RenderState::new();
|
||||
state.update(&mut t).unwrap();
|
||||
let set = Set::from_config(&[
|
||||
link("(a+)+$", Highlight::Always),
|
||||
link("b", Highlight::Always),
|
||||
])
|
||||
.unwrap();
|
||||
let mut result = HashSet::new();
|
||||
set.render_cell_map(&mut result, &state, None, Mods::default())
|
||||
.unwrap();
|
||||
assert_eq!(result, HashSet::from([c(60, 0)]));
|
||||
}
|
||||
}
|
||||
@@ -2653,7 +2653,16 @@ impl Screen {
|
||||
/// over whitespace but the line has non-whitespace characters elsewhere, the
|
||||
/// line will be selected.
|
||||
pub fn select_line(&self, opts: SelectLine<'_>) -> Option<Selection> {
|
||||
self.select_line_bounded(opts, usize::MAX)
|
||||
}
|
||||
|
||||
/// Select a complete logical line only if it spans at most `max_rows`
|
||||
/// physical rows. This bounds interactive link lookup on unbroken output
|
||||
/// without truncating a URL into a different, clickable target. Explicit
|
||||
/// line selection uses [`Self::select_line`] and remains unrestricted.
|
||||
pub fn select_line_bounded(&self, opts: SelectLine<'_>, max_rows: usize) -> Option<Selection> {
|
||||
let list = &self.pages;
|
||||
let mut remaining = max_rows.checked_sub(1)?; // The hovered row.
|
||||
|
||||
// Get the current point semantic prompt state since that determines
|
||||
// boundary conditions too. This makes it so that line selection can
|
||||
@@ -2698,6 +2707,8 @@ impl Screen {
|
||||
break 'start_pin copy;
|
||||
}
|
||||
|
||||
remaining = remaining.checked_sub(1)?;
|
||||
|
||||
if let Some(v) = semantic_prompt_state {
|
||||
// We need to check every cell in this row in reverse
|
||||
// order since we're going up and back.
|
||||
@@ -2726,6 +2737,9 @@ impl Screen {
|
||||
// The real end of the row is the final row in the soft-wrap.
|
||||
let end_pin: Pin = 'end_pin: {
|
||||
for p in opts.pin.row_iterator(list, Direction::RightDown, None) {
|
||||
if p.node != opts.pin.node || p.y != opts.pin.y {
|
||||
remaining = remaining.checked_sub(1)?;
|
||||
}
|
||||
let row = self.pin_row(p);
|
||||
let cols = self.pin_cols(p);
|
||||
|
||||
|
||||
@@ -292,6 +292,39 @@ fn screen_select_line_across_full_soft_wrap() {
|
||||
expect_sel(&s, &sel, Tag::Screen, (0, 0), (4, 1));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn screen_select_line_bounded_counts_both_directions_and_history() {
|
||||
let mut s = init(4, 3, 100_000);
|
||||
s.test_write_string("abcdefghijklmnopqrstuvwx").unwrap();
|
||||
for y in 0..6 {
|
||||
let opts = SelectLine {
|
||||
pin: pin(&s, screen_pt(1, y)),
|
||||
whitespace: None,
|
||||
semantic_prompt_boundary: false,
|
||||
};
|
||||
assert!(s.select_line_bounded(opts, 0).is_none());
|
||||
assert!(s.select_line_bounded(opts, 5).is_none());
|
||||
assert_eq!(s.select_line_bounded(opts, 6), s.select_line(opts));
|
||||
assert!(s.select_line_bounded(opts, 6).is_some());
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn screen_select_line_bounded_stops_at_hard_line_boundary() {
|
||||
let mut s = init(4, 5, 0);
|
||||
s.test_write_string("HEAD\nabcdefghijkl\nTAIL").unwrap();
|
||||
for y in 1..4 {
|
||||
let opts = SelectLine {
|
||||
pin: pin(&s, screen_pt(1, y)),
|
||||
whitespace: None,
|
||||
semantic_prompt_boundary: false,
|
||||
};
|
||||
let sel = s.select_line_bounded(opts, 3).unwrap();
|
||||
assert_eq!(sel, s.select_line(opts).unwrap());
|
||||
assert!(s.select_line_bounded(opts, 2).is_none());
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn screen_select_line_across_soft_wrap_ignores_blank_lines() {
|
||||
let mut s = init(5, 10, 0);
|
||||
|
||||
Reference in new issue
Block a user