diff --git a/freminal/src/gui/app_impl.rs b/freminal/src/gui/app_impl.rs index 437ad48f..68234421 100644 --- a/freminal/src/gui/app_impl.rs +++ b/freminal/src/gui/app_impl.rs @@ -100,6 +100,18 @@ const fn cursor_blink_wants_repaint( is_blink_style && show_cursor && is_active && !is_echo_off } +/// #459 item 9: whether pointer motion this frame must force a full present. +/// Motion only changes plain-egui-painter chrome pixels when over a +/// chrome-interactive region, or while a pane-border drag is latched (the +/// pointer may be off the sensor mid-drag). Pure so it is unit-testable. +const fn pointer_forces_full_present( + pointer_moving: bool, + pointer_over_chrome: bool, + border_drag_active: bool, +) -> bool { + pointer_moving && (pointer_over_chrome || border_drag_active) +} + impl freminal_windowing::App for FreminalGui { /// Called when a window is created. /// @@ -2576,8 +2588,32 @@ impl freminal_windowing::App for FreminalGui { // pointer moves (hover). Presenting only the cursor rect on such a // frame would leave that chrome stale, so both force `Full`. let pointer_moving = ctx.input(|i| i.pointer.is_moving()); - let force_full = - ui_overlay_open || shader_recomposites || active_pane_changed || pointer_moving; + // #459 item 9: pointer motion only changes plain-egui-painter pixels + // when it is over a chrome-interactive region (menu/tab-bar hover + // tints, pane-border highlight) or an active pane-border drag is in + // progress (the drag may have moved the pointer off the ±3px sensor + // mid-drag). Motion purely over terminal content changes no + // egui-painted chrome — the terminal band tracks its own hover + // effects (gutter tint, URL cursor, scrollbar thumb) through explicit + // per-pane damage signals. NB: `self.is_chrome_interactive_at` is + // UNUSABLE here — `win` was removed from `self.windows` for this frame + // (see the `self.windows.remove` at the top of `update`), so it would + // always return `true`; hit-test the local `win` rects directly. + let pointer_over_chrome = ctx.input(|i| i.pointer.latest_pos()).is_none_or(|pos| { + chrome_damage::point_in_chrome_rects( + pos, + win.chrome_head_rects.as_deref(), + &win.chrome_border_rects, + ) + }); + let force_full = ui_overlay_open + || shader_recomposites + || active_pane_changed + || pointer_forces_full_present( + pointer_moving, + pointer_over_chrome, + border_drag_active, + ); // A toast being visible animates its own region each frame. The // resize overlay (issue #433) animates the same way — it fades out // over its linger window on the plain painter — so it must force a @@ -3804,7 +3840,8 @@ impl FreminalGui { #[cfg(test)] mod tests { use super::{ - SettingsOwnerCloseDecision, cursor_blink_wants_repaint, settings_owner_close_decision, + SettingsOwnerCloseDecision, cursor_blink_wants_repaint, pointer_forces_full_present, + settings_owner_close_decision, }; use freminal_common::cursor::CursorVisualStyle; @@ -3872,6 +3909,26 @@ mod tests { } } + #[test] + fn pointer_forces_full_present_truth_table() { + // Moving + over chrome -> force Full (hover tint changed). + assert!(pointer_forces_full_present(true, true, false)); + // Moving + border drag latched -> force Full (drag may have moved the + // pointer off the ±3px sensor mid-drag). + assert!(pointer_forces_full_present(true, false, true)); + // Moving over plain terminal content, no drag -> no forced Full; the + // terminal band tracks its own hover damage separately. + assert!(!pointer_forces_full_present(true, false, false)); + // Not moving, over chrome -> no forced Full (nothing changed). + assert!(!pointer_forces_full_present(false, true, false)); + // Not moving, border drag latched -> no forced Full (drag state alone + // is not motion; `border_drag_active` only matters combined with + // actual pointer movement). + assert!(!pointer_forces_full_present(false, false, true)); + // Not moving, neither chrome nor drag -> no forced Full. + assert!(!pointer_forces_full_present(false, false, false)); + } + #[test] fn not_owner_ignores_other_state() { assert_eq!( diff --git a/freminal/src/gui/pty.rs b/freminal/src/gui/pty.rs index b6c80f9c..d693f6dc 100644 --- a/freminal/src/gui/pty.rs +++ b/freminal/src/gui/pty.rs @@ -119,6 +119,75 @@ pub(crate) fn forward_command_events( /// while bounding worst-case input-handling latency to one batch of parses. const MAX_PTY_READ_BATCH: usize = 64; +/// Outcome of processing one [`InputEvent`] on the PTY consumer thread (issue +/// #459). Distinguishes "this input changed something the GUI must render" +/// ([`InputOutcome::Repaint`]) from "this input changed nothing visible" +/// ([`InputOutcome::NoRepaint`] — a pure child-fd write, or a read-only request +/// whose response the GUI consumes synchronously on a side channel), so the +/// trailing `post_event` can skip the wasteful GUI wake-up for the latter. +/// [`InputOutcome::Closed`] means the input channel dropped (pane teardown). +/// +/// Classification (verified in the #459 design review): +/// +/// - `NoRepaint`: `Key`, `FocusChange` (child-fd writes only, no emulator state +/// change — the echo arrives later via `pty_read_rx`, which requests its own +/// repaint); `ExtractSelection` (read-only; the GUI blocks on `clipboard_rx` +/// in the SAME frame, so no future wake is needed). +/// - `Repaint`: `Resize`, `ScrollOffset`, `ThemeChange`, `CursorConfigChange`, +/// `AutoDetectUrls`, `ThemeModeUpdate`, `ClearScrollback` (all mutate +/// snapshot-visible state), and `RequestSearchBuffer` (read-only, but the GUI +/// POLLS `search_buffer_rx` on a LATER frame, so it needs a guaranteed wake or +/// the result can stall while the terminal is idle and the cursor-blink wake +/// is suppressed). +enum InputOutcome { + Repaint, + NoRepaint, + Closed, +} + +/// Whether processing this [`InputEvent`] changes something the GUI must +/// render, and therefore whether the PTY consumer thread should request a GUI +/// wake-up after handling it (issue #459). +/// +/// Pure and total (no wildcard arm) so a future `InputEvent` variant forces an +/// explicit classification decision at compile time, and so the classification +/// is unit-testable without spinning up a real PTY. Kept in lock-step with the +/// side-effecting arms of `handle_input` — those arms return +/// [`InputOutcome::Repaint`]/[`InputOutcome::NoRepaint`] according to exactly +/// this predicate. +/// +/// `false` (no repaint needed): +/// - `Key` / `FocusChange`: pure child-fd writes; they mutate no emulator +/// state. The visible change (the echo) arrives later via `pty_read_rx`, +/// which requests its own repaint. +/// - `ExtractSelection`: read-only; the result is delivered on `clipboard_tx` +/// and the GUI consumes it with a BLOCKING `clipboard_rx.recv_timeout` in the +/// SAME frame that requested it, so no future wake is needed. +/// +/// `true` (repaint needed): +/// - `Resize`, `ScrollOffset`, `ThemeChange`, `CursorConfigChange`, +/// `AutoDetectUrls`, `ThemeModeUpdate`, `ClearScrollback`: all mutate +/// snapshot-visible state. +/// - `RequestSearchBuffer`: read-only, BUT the GUI POLLS `search_buffer_rx` on +/// a LATER frame (not a blocking recv), so it needs a guaranteed wake or the +/// result can stall while the terminal is idle and the cursor-blink wake is +/// suppressed. +const fn input_event_needs_repaint(event: &InputEvent) -> bool { + match event { + InputEvent::Key(_) | InputEvent::FocusChange(_) | InputEvent::ExtractSelection { .. } => { + false + } + InputEvent::Resize(..) + | InputEvent::ScrollOffset { .. } + | InputEvent::ThemeChange(_) + | InputEvent::ThemeModeUpdate(..) + | InputEvent::RequestSearchBuffer + | InputEvent::AutoDetectUrls(_) + | InputEvent::ClearScrollback + | InputEvent::CursorConfigChange(_) => true, + } +} + /// Feed a just-received `PtyRead` and every `PtyRead` already queued behind it /// (up to [`MAX_PTY_READ_BATCH`]) into `sink`, in arrival order, in a single /// batch (issue #439). @@ -548,14 +617,25 @@ fn spawn_pty_consumer_thread( // Helper closure: drain window commands and command-finished // events, publish snapshot, request repaint. - let post_event = - |emulator: &mut TerminalEmulator, - window_cmd_tx: &crossbeam_channel::Sender, - arc_swap: &ArcSwap, - repaint_handle: &OnceLock<(RepaintProxy, WindowId)>| { - let cmds: Vec<_> = emulator.internal.window_commands.drain(..).collect(); - for cmd in cmds { - let wc = match &cmd { + // `request_repaint` gates ONLY the GUI wake-up request (issue #459): + // the window-command / command-event drains and the snapshot store + // ALWAYS run (they are cheap and must never be stranded), but the + // `request_repaint_after` call is skipped when the caller knows the + // just-processed event changed nothing the GUI would render. This + // removes the wasteful per-keystroke / per-mouse-report GUI wake that + // fired even though `InputEvent::Key`/`FocusChange` only write to the + // child PTY fd (the echo arrives later via `pty_read_rx`, which + // requests its own, necessary, repaint). Mirrors the existing + // `idle_deadline` arm's "byte-identical snapshot -> no spurious wake" + // discipline, applied per-`InputEvent`-variant. + let post_event = |emulator: &mut TerminalEmulator, + window_cmd_tx: &crossbeam_channel::Sender, + arc_swap: &ArcSwap, + repaint_handle: &OnceLock<(RepaintProxy, WindowId)>, + request_repaint: bool| { + let cmds: Vec<_> = emulator.internal.window_commands.drain(..).collect(); + for cmd in cmds { + let wc = match &cmd { WindowManipulation::ReportWindowState | WindowManipulation::ReportWindowPositionWholeWindow | WindowManipulation::ReportWindowPositionTextArea @@ -575,30 +655,30 @@ fn spawn_pty_consumer_thread( } _ => WindowCommand::Viewport(cmd), }; - send_or_log!(window_cmd_tx, wc, "Failed to send window command to GUI"); - } + send_or_log!(window_cmd_tx, wc, "Failed to send window command to GUI"); + } - // Drain finished-command events queued by the FTCS OSC 133 D - // handler (Task 72.3) and forward them to the GUI tagged with - // this pane's recording_pane_id (Task 72.9). - let events = emulator.internal.handler.drain_command_events(); - forward_command_events(events, recording_pane_id, &command_event_tx); - - let snap = emulator.build_snapshot(); - arc_swap.store(Arc::new(snap)); - - if let Some((proxy, wid)) = repaint_handle.get() { - // 16ms == the 60fps frame budget and the same floor - // enforced everywhere else (issue #439). The previous - // 8ms value bypassed that floor via the unclamped - // `RequestRepaintAfter` proxy path, letting a bursty - // PTY stream drive the GUI toward ~120fps. The event - // loop now also clamps this path to 16ms, so this is - // belt-and-braces: request the correct delay AND rely - // on the floor as a backstop. - proxy.request_repaint_after(*wid, std::time::Duration::from_millis(16)); - } - }; + // Drain finished-command events queued by the FTCS OSC 133 D + // handler (Task 72.3) and forward them to the GUI tagged with + // this pane's recording_pane_id (Task 72.9). + let events = emulator.internal.handler.drain_command_events(); + forward_command_events(events, recording_pane_id, &command_event_tx); + + let snap = emulator.build_snapshot(); + arc_swap.store(Arc::new(snap)); + + if request_repaint && let Some((proxy, wid)) = repaint_handle.get() { + // 16ms == the 60fps frame budget and the same floor + // enforced everywhere else (issue #439). The previous + // 8ms value bypassed that floor via the unclamped + // `RequestRepaintAfter` proxy path, letting a bursty + // PTY stream drive the GUI toward ~120fps. The event + // loop now also clamps this path to 16ms, so this is + // belt-and-braces: request the correct delay AND rely + // on the floor as a backstop. + proxy.request_repaint_after(*wid, std::time::Duration::from_millis(16)); + } + }; // Helper closure: process a single InputEvent. let handle_input = @@ -606,9 +686,23 @@ fn spawn_pty_consumer_thread( msg: std::result::Result, clipboard_tx: &crossbeam_channel::Sender, search_buffer_tx: &crossbeam_channel::Sender<(usize, Vec)>| - -> bool { - match msg { - Ok(InputEvent::Resize(w, h, pw, ph)) => { + -> InputOutcome { + let Ok(event) = msg else { + info!("Input channel closed; consumer thread exiting"); + return InputOutcome::Closed; + }; + + // Single source of truth for the repaint decision: the pure, + // unit-tested classifier. The side-effecting arms below never + // re-decide it, so they cannot drift from the tested spec. + let outcome = if input_event_needs_repaint(&event) { + InputOutcome::Repaint + } else { + InputOutcome::NoRepaint + }; + + match event { + InputEvent::Resize(w, h, pw, ph) => { if let Some(rec) = recording_swap.load_full() { rec.emit(EventPayload::PaneResize { pane_id: recording_pane_id, @@ -618,7 +712,12 @@ fn spawn_pty_consumer_thread( } emulator.handle_resize_event(w, h, pw, ph); } - Ok(InputEvent::Key(bytes)) => { + InputEvent::Key(bytes) => { + // Pure child-fd write: `write_raw_bytes` only enqueues + // a `PtyWrite::Write` to the writer thread and mutates + // no emulator state. The visible change (the echo) + // arrives later via `pty_read_rx`, which requests its + // own repaint (classified NoRepaint here, #459). if let Err(e) = emulator.write_raw_bytes(&bytes) { error!("Failed to forward key bytes to PTY: {e}"); } @@ -629,26 +728,29 @@ fn spawn_pty_consumer_thread( }); } } - Ok(InputEvent::FocusChange(focused)) => { + InputEvent::FocusChange(focused) => { + // Conditionally writes a focus escape to the child fd + // (gated on focus-reporting mode); no emulator state + // change, nothing for the GUI to render (NoRepaint). emulator.internal.send_focus_event(focused); } - Ok(InputEvent::ScrollOffset { offset, extra_rows }) => { + InputEvent::ScrollOffset { offset, extra_rows } => { emulator.set_gui_scroll_window(offset, extra_rows); } - Ok(InputEvent::ThemeChange(theme)) => { + InputEvent::ThemeChange(theme) => { emulator.internal.handler.set_theme(theme); } - Ok(InputEvent::CursorConfigChange(style)) => { + InputEvent::CursorConfigChange(style) => { emulator.internal.handler.set_cursor_visual_style(style); } - Ok(InputEvent::AutoDetectUrls(enabled)) => { + InputEvent::AutoDetectUrls(enabled) => { emulator .internal .handler .buffer_mut() .set_auto_detect_urls(enabled); } - Ok(InputEvent::ThemeModeUpdate(theme_mode, os_is_dark)) => { + InputEvent::ThemeModeUpdate(theme_mode, os_is_dark) => { emulator.internal.modes.theme_mode = theme_mode; // Sync the live theming state to match the OS preference // so that ?2031 queries reflect reality immediately. @@ -658,19 +760,29 @@ fn spawn_pty_consumer_thread( emulator.internal.modes.theming = Theming::Light; } } - Ok(InputEvent::ExtractSelection { + InputEvent::ExtractSelection { start_row, start_col, end_row, end_col, is_block, - }) => { + } => { + // Read-only extraction; the result is sent on + // `clipboard_tx` here and the GUI consumes it with a + // BLOCKING `clipboard_rx.recv_timeout` in the SAME + // frame that requested it, so no future GUI wake is + // needed (NoRepaint). let text = emulator.extract_selection_text( start_row, start_col, end_row, end_col, is_block, ); let _ = clipboard_tx.send(text); } - Ok(InputEvent::RequestSearchBuffer) => { + InputEvent::RequestSearchBuffer => { + // Read-only, BUT the GUI POLLS `search_buffer_rx` on a + // LATER frame (not a blocking recv), so a repaint MUST + // be requested or the search result can stall while the + // terminal is otherwise idle and the cursor-blink wake + // is suppressed (classified Repaint, #459 review finding). let (chars, _tags) = emulator.internal.handler.data_and_format_data_for_gui(0); let mut combined = chars.scrollback; @@ -678,7 +790,7 @@ fn spawn_pty_consumer_thread( let total_rows = emulator.internal.handler.buffer().rows().len(); let _ = search_buffer_tx.send((total_rows, combined)); } - Ok(InputEvent::ClearScrollback) => { + InputEvent::ClearScrollback => { // Drop every scrollback row; the visible display // is unaffected. Also reset the PTY-side // gui_scroll_offset so snapshots immediately render @@ -687,12 +799,9 @@ fn spawn_pty_consumer_thread( emulator.internal.handler.buffer_mut().erase_scrollback(); emulator.set_gui_scroll_offset(0); } - Err(_) => { - info!("Input channel closed; consumer thread exiting"); - return false; - } } - true + + outcome }; let mut idle_deadline: crossbeam_channel::Receiver = @@ -756,7 +865,7 @@ fn spawn_pty_consumer_thread( idle_deadline = crossbeam_channel::after(IDLE_COMPACTION_INTERVAL); } else { info!("PTY read channel closed; signaling tab death"); - post_event(&mut emulator, &window_cmd_tx, &arc_swap, &repaint_handle); + post_event(&mut emulator, &window_cmd_tx, &arc_swap, &repaint_handle, true); let _ = pty_dead_tx.send(()); if let Some((proxy, wid)) = repaint_handle.get() { proxy.request_repaint(*wid); @@ -765,17 +874,42 @@ fn spawn_pty_consumer_thread( } } recv(input_rx) -> msg => { - if !handle_input(&mut emulator, msg, &clipboard_tx, &search_buffer_tx) { - // The GUI dropped the pane's input channel — the - // pane/tab/window is being torn down while the - // child shell may still be alive. Signal the PTY - // reader thread so a subsequent failed `send` (the - // receiver we own is about to drop) is treated as - // an expected teardown, not an error. - reader_shutdown.store(true, Ordering::Release); - return; + match handle_input(&mut emulator, msg, &clipboard_tx, &search_buffer_tx) { + InputOutcome::Closed => { + // The GUI dropped the pane's input channel — the + // pane/tab/window is being torn down while the + // child shell may still be alive. Signal the PTY + // reader thread so a subsequent failed `send` (the + // receiver we own is about to drop) is treated as + // an expected teardown, not an error. + reader_shutdown.store(true, Ordering::Release); + return; + } + InputOutcome::NoRepaint => { + // Nothing visible changed (e.g. a key/mouse-report + // byte written to the child fd). Still fall through + // to `post_event` so any window-command/command-event + // drains and the snapshot store run — but tell it NOT + // to request a GUI wake (#459). `request_repaint = + // false` below. + idle_deadline = + crossbeam_channel::after(IDLE_COMPACTION_INTERVAL); + post_event( + &mut emulator, + &window_cmd_tx, + &arc_swap, + &repaint_handle, + false, + ); + continue; + } + InputOutcome::Repaint => { + idle_deadline = + crossbeam_channel::after(IDLE_COMPACTION_INTERVAL); + // Fall through to the trailing `post_event` (with + // `request_repaint = true`). + } } - idle_deadline = crossbeam_channel::after(IDLE_COMPACTION_INTERVAL); } recv(child_exit) -> _ => { info!("Child process exited; draining remaining PTY output"); @@ -787,7 +921,7 @@ fn spawn_pty_consumer_thread( } info!("PTY drain complete; signaling tab death"); - post_event(&mut emulator, &window_cmd_tx, &arc_swap, &repaint_handle); + post_event(&mut emulator, &window_cmd_tx, &arc_swap, &repaint_handle, true); let _ = pty_dead_tx.send(()); if let Some((proxy, wid)) = repaint_handle.get() { proxy.request_repaint(*wid); @@ -840,7 +974,15 @@ fn spawn_pty_consumer_thread( } } - post_event(&mut emulator, &window_cmd_tx, &arc_swap, &repaint_handle); + // Reached by the `pty_read_rx` arm (real screen change) and the + // `input_rx` `Repaint` arm — both genuinely need a GUI wake. + post_event( + &mut emulator, + &window_cmd_tx, + &arc_swap, + &repaint_handle, + true, + ); } }) { @@ -1065,4 +1207,57 @@ mod tests { CursorVisualStyle::VerticalLineCursorBlink ); } + + /// Table test locking the #459 repaint classification for EVERY + /// `InputEvent` variant. `input_event_needs_repaint` is the single source + /// of truth the PTY consumer thread uses to decide whether to wake the GUI + /// after handling an input; a regression here (e.g. reclassifying + /// `RequestSearchBuffer` as no-repaint, which the design review caught as a + /// stall bug, or `ClearScrollback` as no-repaint, which would leave a stale + /// scrollbar) would silently drop or add GUI wakes. The classifier is + /// exhaustive (no wildcard), so adding an `InputEvent` variant forces a + /// compile error until it is classified AND added here. + #[test] + fn input_event_repaint_classification() { + use freminal_common::config::ThemeMode; + use freminal_common::cursor::CursorVisualStyle; + use freminal_common::themes::DRACULA; + + // NoRepaint: pure child-fd writes (Key/FocusChange) and the + // synchronously-consumed ExtractSelection. + assert!(!input_event_needs_repaint(&InputEvent::Key(vec![b'a']))); + assert!(!input_event_needs_repaint(&InputEvent::FocusChange(true))); + assert!(!input_event_needs_repaint(&InputEvent::FocusChange(false))); + assert!(!input_event_needs_repaint(&InputEvent::ExtractSelection { + start_row: 0, + start_col: 0, + end_row: 1, + end_col: 1, + is_block: false, + })); + + // Repaint: everything that mutates snapshot-visible state, plus + // RequestSearchBuffer (polled on a later frame -> needs a guaranteed + // wake). + assert!(input_event_needs_repaint(&InputEvent::Resize( + 80, 24, 8, 16 + ))); + assert!(input_event_needs_repaint(&InputEvent::ScrollOffset { + offset: 5, + extra_rows: 0, + })); + assert!(input_event_needs_repaint(&InputEvent::ThemeChange( + &DRACULA + ))); + assert!(input_event_needs_repaint(&InputEvent::ThemeModeUpdate( + ThemeMode::Auto, + true, + ))); + assert!(input_event_needs_repaint(&InputEvent::RequestSearchBuffer)); + assert!(input_event_needs_repaint(&InputEvent::AutoDetectUrls(true))); + assert!(input_event_needs_repaint(&InputEvent::ClearScrollback)); + assert!(input_event_needs_repaint(&InputEvent::CursorConfigChange( + CursorVisualStyle::VerticalLineCursorBlink, + ))); + } } diff --git a/freminal/src/gui/terminal/widget.rs b/freminal/src/gui/terminal/widget.rs index d3add4a7..26303bf3 100644 --- a/freminal/src/gui/terminal/widget.rs +++ b/freminal/src/gui/terminal/widget.rs @@ -394,19 +394,44 @@ fn compute_command_block_hover_rows( Some((s_screen.min(e_screen), s_screen.max(e_screen))) } +/// Outcome of a scrollbar render+interaction pass. `new_offset` is the +/// scroll offset the user dragged to (if any). `rendered` is whether the +/// thumb was actually painted this frame. `hovered` is the +/// window-exit-corrected hover state (`latest_pos().is_some() && +/// interact_pos() over the hit rect`) — the SAME signal the thumb's painted +/// alpha uses, so the paint and the damage decision can never drift a frame +/// apart on window-exit. +pub(super) struct ScrollbarOutcome { + pub(super) new_offset: Option, + pub(super) rendered: bool, + pub(super) hovered: bool, +} + +impl ScrollbarOutcome { + /// The outcome for a frame where the thumb was not rendered at all + /// (scrolled to the live bottom, or a degenerate zero-height viewport). + const fn not_rendered() -> Self { + Self { + new_offset: None, + rendered: false, + hovered: false, + } + } +} /// /// The scrollbar is shown when the user is actively scrolled back /// (`scroll_offset > 0`). It disappears at the live bottom. /// -/// Supports click-to-position and drag-to-scroll. Returns the new -/// `scroll_offset` if the user interacted with the scrollbar, or `None` -/// if no scrollbar interaction occurred. +/// Supports click-to-position and drag-to-scroll. Returns a +/// [`ScrollbarOutcome`] describing the new `scroll_offset` (if the user +/// interacted with the scrollbar), whether the thumb was rendered this +/// frame, and whether it was hovered. pub(super) fn handle_scrollbar( scroll_offset: usize, max_scroll_offset: usize, ui: &Ui, dragging: &mut bool, -) -> Option { +) -> ScrollbarOutcome { const SCROLLBAR_WIDTH: f32 = 6.0; const SCROLLBAR_MARGIN: f32 = 2.0; const MIN_THUMB_HEIGHT: f32 = 12.0; @@ -417,11 +442,11 @@ pub(super) fn handle_scrollbar( // while the user is mid-drag so the scrollbar doesn't vanish when // they drag to the bottom. if !*dragging && (scroll_offset == 0 || max_scroll_offset == 0) { - return None; + return ScrollbarOutcome::not_rendered(); } if max_scroll_offset == 0 { *dragging = false; - return None; + return ScrollbarOutcome::not_rendered(); } let painter = ui.painter(); @@ -432,7 +457,7 @@ pub(super) fn handle_scrollbar( let track_bottom = viewport.bottom(); let track_height = track_bottom - track_top; if track_height <= 0.0 { - return None; + return ScrollbarOutcome::not_rendered(); } let track_right = viewport.right() - SCROLLBAR_MARGIN; @@ -504,14 +529,25 @@ pub(super) fn handle_scrollbar( }); // ── Appearance ─────────────────────────────────────────────────────── - let is_hovered = ui.input(|i| { - i.pointer - .interact_pos() - .is_some_and(|pos| hit_rect.contains(pos)) + // `interact_pos()` lags `latest_pos()` by one frame on window-exit + // (egui's documented `Event::PointerGone` behavior — `latest_pos` clears + // immediately, `interact_pos` not until the next frame). Fold in + // `pointer_in_window` (`latest_pos().is_some()`) so the PAINTED alpha and + // the returned hover state both use the SAME same-frame-corrected signal. + // Sourcing the paint and the damage decision (see the call site's + // `scrollbar_damage_decision`) from one consistent signal is what keeps + // the thumb's hover alpha from getting stuck one frame on window-exit — + // mirroring how the command-block gutter (#461) drives both its tint and + // its damage from the single non-lagged `view_state.mouse_position`. + let effectively_hovered = ui.input(|i| { + i.pointer.latest_pos().is_some() + && i.pointer + .interact_pos() + .is_some_and(|pos| hit_rect.contains(pos)) }); let alpha = if *dragging { 220 - } else if is_hovered { + } else if effectively_hovered { 200 } else { 150 @@ -521,7 +557,11 @@ pub(super) fn handle_scrollbar( painter.rect_filled(thumb_rect, rounding, color); - new_offset + ScrollbarOutcome { + new_offset, + rendered: true, + hovered: effectively_hovered, + } } /// Decide whether the command-block gutter's hover-tint state changed @@ -562,6 +602,45 @@ const fn gutter_hover_repaint_decision( (effectively_hovered != was_hovering, effectively_hovered) } +/// Snapshot of the scrollbar's render-visibility + effective-hover state for +/// a single frame, used by [`scrollbar_damage_decision`]. +/// +/// Bundled into a named struct (rather than passed as loose bool +/// parameters) both reads more clearly at the two call sites (this frame's +/// observed state vs. the previous frame's cached state) and keeps +/// `scrollbar_damage_decision` under clippy's bool-parameter-count limit. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) struct ScrollbarDamageState { + /// Whether the thumb was rendered (shown) this frame. + pub(super) rendered: bool, + /// Whether the thumb was hovered this frame. MUST already fold in + /// `pointer_in_window` (`hovered && latest_pos().is_some()`) to avoid + /// the `interact_pos()` one-frame window-exit lag that PR #461 + /// documents for the gutter. + pub(super) effectively_hovered: bool, +} + +/// Decide whether the scrollbar's damage state changed enough to need a +/// repaint + a forced Full present this frame, given this frame's observed +/// [`ScrollbarDamageState`] and the previous frame's cached state. +/// +/// A rendered->not-rendered transition erases the previously-painted thumb +/// (force Full to clear it). A hover-alpha change while rendered repaints the +/// thumb at a new alpha (force Full — the thumb is on the plain painter, +/// outside per-pane VBO damage). Either also needs a repaint scheduled since +/// nothing else is guaranteed to wake a frame. Returns whether a repaint + +/// forced Full present is needed — the single bool drives both, matching +/// the gutter's `request_repaint` + Full pattern. +const fn scrollbar_damage_decision( + current: ScrollbarDamageState, + previous: ScrollbarDamageState, +) -> bool { + let visibility_changed = current.rendered != previous.rendered; + let hover_changed = + current.rendered && (current.effectively_hovered != previous.effectively_hovered); + visibility_changed || hover_changed +} + /// Duration of the visual bell flash overlay. const BELL_FLASH_DURATION: Duration = Duration::from_millis(150); @@ -1266,6 +1345,17 @@ pub struct PaneRenderCache { /// previous frame. Used to request one extra repaint on the frame the /// pointer leaves the gutter so the hover-tint clearing frame is drawn. pub(super) pointer_in_gutter_last_frame: bool, + /// Whether the scrollbar thumb was hovered on the previous frame (using + /// the #461-proven `hovered && pointer_in_window` shape to avoid the + /// `interact_pos()` one-frame window-exit lag). Drives a one-frame + /// hover-alpha clearing repaint + Full damage, since the thumb is painted + /// on the plain egui painter outside the per-pane VBO damage tracking. + pub(super) scrollbar_was_hovered_last_frame: bool, + /// Whether the scrollbar thumb was rendered at all on the previous frame. + /// A rendered->not-rendered transition (e.g. scrolled to bottom) must + /// force one Full clear to erase the previously-painted thumb pixels; a + /// hover-only latch misses the common visible-but-unhovered vanish case. + pub(super) scrollbar_was_rendered_last_frame: bool, /// Terminal width (columns) from the last full vertex rebuild. When this /// changes (window resize), the cell-instance VBOs still contain vertices /// for the old column count; drawing them into a smaller viewport leaves @@ -1354,6 +1444,8 @@ impl PaneRenderCache { shaping_cache: crate::gui::shaping::ShapingCache::new(), scrollbar_dragging: false, pointer_in_gutter_last_frame: false, + scrollbar_was_hovered_last_frame: false, + scrollbar_was_rendered_last_frame: false, previous_term_width: 0, previous_term_height: 0, previous_fold_epoch: 0, @@ -3102,12 +3194,13 @@ impl FreminalTerminalWidget { }); // ── Scrollbar (visual + interactive) ───────────────────────── - if let Some(new_offset) = handle_scrollbar( + let scrollbar_outcome = handle_scrollbar( snap.scroll_offset, snap.max_scroll_offset, ui, &mut cache.scrollbar_dragging, - ) { + ); + if let Some(new_offset) = scrollbar_outcome.new_offset { view_state.scroll_offset = new_offset; let _ = input_tx.try_send(super::input::scroll_event( snap, @@ -3115,6 +3208,28 @@ impl FreminalTerminalWidget { new_offset, )); } + // #459 item 9 / mirrors #461 gutter: the scrollbar thumb is painted on + // the plain egui painter (alpha varies with hover), outside per-pane VBO + // damage. A hover-alpha change or a rendered->not-rendered vanish must + // force one Full clear or a Partial present (driven by an unrelated + // pane's cursor blink) would leave a stale/ghosted thumb. + // `scrollbar_outcome.hovered` is already the window-exit-corrected + // signal (the same one the thumb's alpha was painted with), so the + // paint and this damage decision can never drift a frame apart. + let current_scrollbar_state = ScrollbarDamageState { + rendered: scrollbar_outcome.rendered, + effectively_hovered: scrollbar_outcome.hovered, + }; + let previous_scrollbar_state = ScrollbarDamageState { + rendered: cache.scrollbar_was_rendered_last_frame, + effectively_hovered: cache.scrollbar_was_hovered_last_frame, + }; + if scrollbar_damage_decision(current_scrollbar_state, previous_scrollbar_state) { + ui.ctx().request_repaint(); + cache.last_frame_cursor_damage = crate::gui::renderer::PaneFrameDamage::Full; + } + cache.scrollbar_was_rendered_last_frame = scrollbar_outcome.rendered; + cache.scrollbar_was_hovered_last_frame = scrollbar_outcome.hovered; // ── Visual bell flash overlay ──────────────────────────────── paint_bell_flash(ui, rect, view_state); @@ -3789,6 +3904,99 @@ mod gutter_hover_repaint_decision_tests { } } +#[cfg(test)] +mod scrollbar_damage_decision_tests { + //! Tests for [`scrollbar_damage_decision`], the pure decision function + //! behind the scrollbar thumb's hover/visibility repaint + Full-present + //! forcing (#459 item 9, mirroring #461's gutter fix). Covers the + //! rendered<->not-rendered vanish/appear transitions, the hover-alpha + //! change while rendered, and the steady-state no-op cases — including + //! the case where hover *would* differ but the thumb isn't rendered at + //! all, which must be ignored (governed only by visibility). + + use super::{ScrollbarDamageState, scrollbar_damage_decision}; + + /// Test-only shorthand for building a [`ScrollbarDamageState`]. + const fn state(rendered: bool, effectively_hovered: bool) -> ScrollbarDamageState { + ScrollbarDamageState { + rendered, + effectively_hovered, + } + } + + #[test] + fn rendered_to_not_rendered_forces_damage() { + // Scrolled to bottom: thumb was visible last frame, gone this frame. + assert!(scrollbar_damage_decision( + state(false, false), + state(true, false) + )); + } + + #[test] + fn not_rendered_to_rendered_forces_damage() { + // Scrolled back into history: thumb appears this frame. + assert!(scrollbar_damage_decision( + state(true, false), + state(false, false) + )); + } + + #[test] + fn hover_enter_while_rendered_forces_damage() { + assert!(scrollbar_damage_decision( + state(true, true), + state(true, false) + )); + } + + #[test] + fn hover_leave_while_rendered_forces_damage() { + assert!(scrollbar_damage_decision( + state(true, false), + state(true, true) + )); + } + + #[test] + fn steady_visible_unhovered_no_damage() { + assert!(!scrollbar_damage_decision( + state(true, false), + state(true, false) + )); + } + + #[test] + fn steady_visible_hovered_no_damage() { + assert!(!scrollbar_damage_decision( + state(true, true), + state(true, true) + )); + } + + #[test] + fn steady_hidden_no_damage() { + assert!(!scrollbar_damage_decision( + state(false, false), + state(false, false) + )); + } + + #[test] + fn hover_change_while_not_rendered_is_ignored() { + // Not rendered both frames, "hover" bit differs -- irrelevant since + // visibility governs when the thumb isn't rendered at all. + assert!(!scrollbar_damage_decision( + state(false, true), + state(false, false) + )); + assert!(!scrollbar_damage_decision( + state(false, false), + state(false, true) + )); + } +} + #[cfg(test)] mod bell_flash_tests { //! Tests for [`bell_flash_outcome`], the pure decision function behind