fix hyperlink underlines

There were two problems:

* We weren't correctly invalidating when the hover state changed
  (a recent regression caused by recent caching changes)
* We'd underline every link with the same destination on hover,
  not just the one under the mouse (longstanding wart)

Recent changes allow the application layer to reference the underlying
Lines directly, so we can restore the original and expected
only-highlight-under-the-mouse by switching to those newer APIs.

Adjust the cache values so that we know to also verify the current
highlight and invalidate.

I was a little surprised to see that this also works with mux client
panes: I was expecting to need to do some follow up on those because
they return copies of Line rather than references to them. That happens
to work because the mux client updates the hyperlinks at the time where
it inserts into its cache. The effect of that is that lines in mux
client panes won't update to new hyperlink rules if they were received
prior to a change in the config.

refs: https://github.com/wez/wezterm/issues/2496

docs/changelog.md

@@ -22,6 +22,8 @@ As features stabilize some brief notes about them will accumulate here.
 * Correctly invalidate the display when using
   `wezterm.action.ClearScrollback("ScrollbackAndViewport")`
   [#2498](https://github.com/wez/wezterm/issues/2498)
+* Hyperlinks didn't underline on hover
+  [#2496](https://github.com/wez/wezterm/issues/2496)
 
 ### 20220905-102802-7d4b8249
 

mux/src/pane.rs

@@ -170,28 +170,6 @@ impl LogicalLine {
         }
         (y - 1, x - idx + self.physical_lines.last().unwrap().len())
     }
-
-    pub fn apply_hyperlink_rules(&mut self, rules: &[Rule]) {
-        let seq = self.logical.current_seqno();
-        self.logical.invalidate_implicit_hyperlinks(seq);
-        self.logical.scan_and_create_hyperlinks(rules);
-        if !self.logical.has_hyperlink() {
-            return;
-        }
-
-        // Re-compute the physical lines
-        let mut line = self.logical.clone();
-        let num_phys = self.physical_lines.len();
-        for (idx, phys) in self.physical_lines.iter_mut().enumerate() {
-            let len = phys.len();
-            let seq = seq.max(phys.current_seqno());
-            let remainder = line.split_off(len, seq);
-            *phys = line;
-            line = remainder;
-            let wrapped = idx == num_phys - 1;
-            phys.set_last_cell_was_wrapped(wrapped, seq);
-        }
-    }
 }
 
 /// A Pane represents a view on a terminal
@@ -260,42 +238,6 @@ pub trait Pane: Downcast {
         self.for_each_logical_line_in_stable_range_mut(lines, &mut ApplyHyperLinks { rules });
     }
 
-    fn get_lines_with_hyperlinks_applied(
-        &self,
-        lines: Range<StableRowIndex>,
-        rules: &[Rule],
-    ) -> (StableRowIndex, Vec<Line>) {
-        if rules.is_empty() {
-            return self.get_lines(lines);
-        }
-        let requested_first = lines.start;
-        let num_lines = (lines.end - lines.start) as usize;
-        let logical = self.get_logical_lines(lines);
-
-        let mut first = None;
-        let mut phys_lines = vec![];
-        'outer: for mut log_line in logical {
-            log_line.apply_hyperlink_rules(rules);
-            for (idx, phys) in log_line.physical_lines.into_iter().enumerate() {
-                if log_line.first_row + idx as StableRowIndex >= requested_first {
-                    if first.is_none() {
-                        first.replace(log_line.first_row + idx as StableRowIndex);
-                    }
-                    phys_lines.push(phys);
-                    if phys_lines.len() == num_lines {
-                        break 'outer;
-                    }
-                }
-            }
-        }
-
-        if first.is_none() {
-            assert_eq!(phys_lines.len(), 0);
-        }
-
-        (first.unwrap_or(0), phys_lines)
-    }
-
     /// Returns render related dimensions
     fn get_dimensions(&self) -> RenderableDimensions;
 

termwiz/src/surface/line/line.rs

@@ -545,6 +545,9 @@ impl Line {
     /// This function does not remember the values of the `rules` slice, so it
     /// is the responsibility of the caller to call `invalidate_implicit_hyperlinks`
     /// if it wishes to call this function with different `rules`.
+    ///
+    /// This function will call Line::clear_appdata on lines where
+    /// hyperlinks are adjusted.
     pub fn apply_hyperlink_rules(rules: &[Rule], logical_line: &mut [&mut Line]) {
         if rules.is_empty() || logical_line.is_empty() {
             return;
@@ -574,6 +577,7 @@ impl Line {
         if !logical.has_hyperlink() {
             for line in logical_line.iter_mut() {
                 line.bits.set(LineBits::SCANNED_IMPLICIT_HYPERLINKS, true);
+                line.clear_appdata();
             }
             return;
         }
@@ -587,6 +591,7 @@ impl Line {
             **phys = logical;
             logical = remainder;
             phys.set_last_cell_was_wrapped(wrapped, seq);
+            phys.clear_appdata();
             if is_cluster {
                 phys.compress_for_scrollback();
             }

wezterm-gui/src/termwindow/mouseevent.rs

@@ -7,7 +7,7 @@ use ::window::{
 };
 use config::keyassignment::{MouseEventTrigger, SpawnTabDomain};
 use config::MouseEventAltScreen;
-use mux::pane::Pane;
+use mux::pane::{Pane, WithPaneLines};
 use mux::tab::SplitDirection;
 use mux::Mux;
 use std::convert::TryInto;
@@ -15,6 +15,8 @@ use std::ops::Sub;
 use std::rc::Rc;
 use std::sync::Arc;
 use std::time::Duration;
+use termwiz::hyperlink::Hyperlink;
+use termwiz::surface::Line;
 use wezterm_dynamic::ToDynamic;
 use wezterm_term::input::{MouseButton, MouseEventKind as TMEK};
 use wezterm_term::{ClickPosition, LastMouseClick, StableRowIndex};
@@ -670,35 +672,38 @@ impl super::TermWindow {
                 stable_row,
             ));
 
-        let (top, mut lines) = pane.get_lines_with_hyperlinks_applied(
-            stable_row..stable_row + 1,
-            &self.config.hyperlink_rules,
-        );
-        let new_highlight = if top == stable_row {
-            if let Some(line) = lines.get_mut(0) {
-                if let Some(cell) = line.get_cell(column) {
-                    cell.attrs().hyperlink().cloned()
-                } else {
-                    None
+        pane.apply_hyperlinks(stable_row..stable_row + 1, &self.config.hyperlink_rules);
+
+        struct FindCurrentLink {
+            current: Option<Arc<Hyperlink>>,
+            stable_row: StableRowIndex,
+            column: usize,
+        }
+
+        impl WithPaneLines for FindCurrentLink {
+            fn with_lines_mut(&mut self, stable_top: StableRowIndex, lines: &mut [&mut Line]) {
+                if stable_top == self.stable_row {
+                    if let Some(line) = lines.get(0) {
+                        if let Some(cell) = line.get_cell(self.column) {
+                            self.current = cell.attrs().hyperlink().cloned();
+                        }
+                    }
                 }
-            } else {
-                None
             }
-        } else {
-            None
+        }
+
+        let mut find_link = FindCurrentLink {
+            current: None,
+            stable_row,
+            column,
         };
+        pane.with_lines_mut(stable_row..stable_row + 1, &mut find_link);
+        let new_highlight = find_link.current;
 
         match (self.current_highlight.as_ref(), new_highlight) {
             (Some(old_link), Some(new_link)) if Arc::ptr_eq(&old_link, &new_link) => {
                 // Unchanged
             }
-            (Some(old_link), Some(new_link)) if *old_link == new_link => {
-                // Unchanged
-                // Note: ideally this case wouldn't exist, as we *should*
-                // only be matching distinct instances of the hyperlink.
-                // However, for horrible reasons, we always end up with duplicated
-                // hyperlink instances today, so we have to do a deeper compare.
-            }
             (None, None) => {
                 // Unchanged
             }

wezterm-gui/src/termwindow/render.rs

@@ -39,6 +39,7 @@ use std::sync::Arc;
 use std::time::{Duration, Instant};
 use termwiz::cell::{unicode_column_width, Blink};
 use termwiz::cellcluster::CellCluster;
+use termwiz::hyperlink::Hyperlink;
 use termwiz::surface::{CursorShape, CursorVisibility, SequenceNo};
 use wezterm_bidi::Direction;
 use wezterm_dynamic::Value;
@@ -179,6 +180,10 @@ pub struct LineQuadCacheValue {
     pub line: Line,
     pub expires: Option<Instant>,
     pub layers: HeapQuadAllocator,
+    // Only set if the line contains any hyperlinks, so
+    // that we can invalidate when it changes
+    pub current_highlight: Option<Arc<Hyperlink>>,
+    pub invalidate_on_hover_change: bool,
 }
 
 pub struct LineToElementParams<'a> {
@@ -202,6 +207,10 @@ pub struct LineToEleShapeCacheKey {
 pub struct LineToElementShapeItem {
     pub expires: Option<Instant>,
     pub shaped: Rc<Vec<LineToElementShape>>,
+    // Only set if the line contains any hyperlinks, so
+    // that we can invalidate when it changes
+    pub current_highlight: Option<Arc<Hyperlink>>,
+    pub invalidate_on_hover_change: bool,
 }
 
 pub struct LineToElementShape {
@@ -217,6 +226,10 @@ pub struct LineToElementShape {
     pub cluster: CellCluster,
 }
 
+pub struct RenderScreenLineOpenGLResult {
+    pub invalidate_on_hover_change: bool,
+}
+
 pub struct RenderScreenLineOpenGLParams<'a> {
     /// zero-based offset from top of the window viewport to the line that
     /// needs to be rendered, measured in pixels
@@ -1741,7 +1754,15 @@ impl super::TermWindow {
                             .expires
                             .map(|i| Instant::now() >= i)
                             .unwrap_or(false);
-                        if !expired {
+                        let hover_changed = if cached_quad.invalidate_on_hover_change {
+                            !same_hyperlink(
+                                cached_quad.current_highlight.as_ref(),
+                                self.term_window.current_highlight.as_ref(),
+                            )
+                        } else {
+                            false
+                        };
+                        if !expired && !hover_changed {
                             cached_quad
                                 .layers
                                 .apply_to(&mut TripleLayerQuadAllocator::Heap(&mut self.layers))?;
@@ -1769,7 +1790,7 @@ impl super::TermWindow {
                         },
                     };
 
-                    self.term_window.render_screen_line_opengl(
+                    let render_result = self.term_window.render_screen_line_opengl(
                         RenderScreenLineOpenGLParams {
                             top_pixel_y: *quad_key.top_pixel_y,
                             left_pixel_x: self.left_pixel_x,
@@ -1817,6 +1838,12 @@ impl super::TermWindow {
                         layers: buf,
                         expires,
                         line: (*line).clone(),
+                        invalidate_on_hover_change: render_result.invalidate_on_hover_change,
+                        current_highlight: if render_result.invalidate_on_hover_change {
+                            self.term_window.current_highlight.clone()
+                        } else {
+                            None
+                        },
                     };
 
                     self.term_window
@@ -2191,7 +2218,7 @@ impl super::TermWindow {
     fn build_line_element_shape(
         &self,
         params: LineToElementParams,
-    ) -> anyhow::Result<Rc<Vec<LineToElementShape>>> {
+    ) -> anyhow::Result<(Rc<Vec<LineToElementShape>>, bool)> {
         let (bidi_enabled, bidi_direction) = params.line.bidi_info();
         let bidi_hint = if bidi_enabled {
             Some(bidi_direction)
@@ -2215,16 +2242,19 @@ impl super::TermWindow {
         let mut last_style = None;
         let mut x_pos = 0.;
         let mut expires = None;
+        let mut invalidate_on_hover_change = false;
 
         for cluster in &cell_clusters {
             if !matches!(last_style.as_ref(), Some(ClusterStyleCache{attrs,..}) if *attrs == &cluster.attrs)
             {
                 let attrs = &cluster.attrs;
                 let style = self.fonts.match_style(params.config, attrs);
-                let is_highlited_hyperlink = match (attrs.hyperlink(), &self.current_highlight) {
-                    (Some(ref this), &Some(ref highlight)) => **this == *highlight,
-                    _ => false,
-                };
+                let hyperlink = attrs.hyperlink();
+                let is_highlited_hyperlink =
+                    same_hyperlink(hyperlink, self.current_highlight.as_ref());
+                if hyperlink.is_some() {
+                    invalidate_on_hover_change = true;
+                }
                 // underline and strikethrough
                 let underline_tex_rect = gl_state
                     .glyph_cache
@@ -2360,11 +2390,17 @@ impl super::TermWindow {
                 LineToElementShapeItem {
                     expires,
                     shaped: Rc::clone(&shaped),
+                    invalidate_on_hover_change,
+                    current_highlight: if invalidate_on_hover_change {
+                        self.current_highlight.clone()
+                    } else {
+                        None
+                    },
                 },
             );
         }
 
-        Ok(shaped)
+        Ok((shaped, invalidate_on_hover_change))
     }
 
     /// "Render" a line of the terminal screen into the vertex buffer.
@@ -2375,12 +2411,14 @@ impl super::TermWindow {
         &self,
         params: RenderScreenLineOpenGLParams,
         layers: &mut TripleLayerQuadAllocator,
-    ) -> anyhow::Result<()> {
+    ) -> anyhow::Result<RenderScreenLineOpenGLResult> {
         if params.line.is_double_height_bottom() {
             // The top and bottom lines are required to have the same content.
             // For the sake of simplicity, we render both of them as part of
             // rendering the top row, so we have nothing more to do here.
-            return Ok(());
+            return Ok(RenderScreenLineOpenGLResult {
+                invalidate_on_hover_change: false,
+            });
         }
 
         let gl_state = self.render_state.as_ref().unwrap();
@@ -2460,15 +2498,27 @@ impl super::TermWindow {
             ..params.left_pixel_x + cursor_range.end as f32 * cell_width;
 
         let mut shaped = None;
+        let mut invalidate_on_hover_change = false;
+
         if let Some(shape_key) = &params.shape_key {
             let mut cache = self.line_to_ele_shape_cache.borrow_mut();
             if let Some(entry) = cache.get(shape_key) {
                 let expired = entry.expires.map(|i| Instant::now() >= i).unwrap_or(false);
+                let hover_changed = if entry.invalidate_on_hover_change {
+                    !same_hyperlink(
+                        entry.current_highlight.as_ref(),
+                        self.current_highlight.as_ref(),
+                    )
+                } else {
+                    false
+                };
 
-                if !expired {
+                if !expired && !hover_changed {
                     self.update_next_frame_time(entry.expires);
                     shaped.replace(Rc::clone(&entry.shaped));
                 }
+
+                invalidate_on_hover_change = entry.invalidate_on_hover_change;
             }
         }
 
@@ -2486,7 +2536,9 @@ impl super::TermWindow {
                 shape_key: &params.shape_key,
             };
 
-            self.build_line_element_shape(params)?
+            let (shaped, invalidate_on_hover) = self.build_line_element_shape(params)?;
+            invalidate_on_hover_change = invalidate_on_hover;
+            shaped
         };
 
         let bounding_rect = euclid::rect(
@@ -3025,7 +3077,9 @@ impl super::TermWindow {
 
         metrics::histogram!("render_screen_line_opengl", start.elapsed());
 
-        Ok(())
+        Ok(RenderScreenLineOpenGLResult {
+            invalidate_on_hover_change,
+        })
     }
 
     fn resolve_lock_glyph(
@@ -3584,3 +3638,10 @@ fn update_next_frame_time(storage: &mut Option<Instant>, next_due: Option<Instan
         }
     }
 }
+
+fn same_hyperlink(a: Option<&Arc<Hyperlink>>, b: Option<&Arc<Hyperlink>>) -> bool {
+    match (a, b) {
+        (Some(a), Some(b)) => Arc::ptr_eq(a, b),
+        _ => false,
+    }
+}