From 3732513587510a0d1b3399251f543a4ae72325f2 Mon Sep 17 00:00:00 2001 From: simota Date: Thu, 10 Sep 2026 12:42:05 +0900 Subject: [PATCH 1/3] fix: address focus, input, IPC, config, and grid static-analysis findings Ten of twelve findings from the 2026-09-10 static analysis (N01-N12); N08 and N10 are rejected as Ghostty parity (key_encode.zig escape-encodes under any non-zero kitty flag and sends IME/unidentified-key text raw). - N01 secure input: decide from app-level focus (`os_focused` after the loss), so an outgoing window's `Focused(false)` that follows the successor's `Focused(true)` no longer releases Secure Keyboard Entry. - N02 ipc: `noa.sendText` reports a dropped (queue full) or sink-less (disconnected) input as an error instead of `Ok`. - N03 mouse: the left press captures its pane; motion and release of that gesture route to it across split dividers, with cancellation on focus loss, pane close, and pane move. - N04 config writer: after a font-family save, re-parse with includes expanded and, when an include's family still heads the list, rewrite the key as reset + new primary + surviving fallbacks. - N05 favorites: share session.rs's per-process/per-call staging file via a new `atomic_write` module instead of a fixed `favorites.tmp`. - N06 grid: `clear_scrollback` shares `ED 3`'s placement collapse so live Kitty images keep their row when only history is cleared. - N07 grid: with DECOM off, CUP/CHA/HPA and the DECSLRM home are bounded by the screen, not the left/right margins (Ghostty `setCursorPos`). - N09 kitty: a composing macOS Option is not Alt for the escape decision; the composed text passes through, report-all keeps the modifier bit and the associated text. - N11 kitty: physical keypad keys use their dedicated codes (KP_Enter is 57414 even though winit's logical key is Enter). - N12 ime: the candidate window anchors to the owning modal's input row (palette / remote UI / title prompt / theme settings / search prompt / sidebar rename) instead of the terminal cursor behind the card. --- crates/noa-app/src/app/event_loop.rs | 178 ++++++++++++++++-- crates/noa-app/src/app/input_ops/ime.rs | 117 +++++++++++- crates/noa-app/src/app/input_ops/pointer.rs | 40 +++- crates/noa-app/src/app/ipc.rs | 49 ++++- crates/noa-app/src/app/lifecycle.rs | 4 + crates/noa-app/src/app/quick_terminal.rs | 1 + crates/noa-app/src/app/remote_ui.rs | 9 + crates/noa-app/src/app/scratch_terminal.rs | 1 + crates/noa-app/src/app/split_ops.rs | 3 + crates/noa-app/src/app/state.rs | 4 + crates/noa-app/src/atomic_write.rs | 110 +++++++++++ crates/noa-app/src/input/key.rs | 1 + crates/noa-app/src/input/kitty.rs | 148 ++++++++------- crates/noa-app/src/input/tests.rs | 105 +++++++++++ crates/noa-app/src/lib.rs | 1 + crates/noa-app/src/macos_overlay.rs | 5 +- .../noa-app/src/macos_overlay/imp/appkit.rs | 16 +- crates/noa-app/src/macos_overlay/model.rs | 76 ++++++++ crates/noa-app/src/macos_overlay/tests.rs | 43 ++++- crates/noa-app/src/session.rs | 59 +----- crates/noa-app/src/theme_favorites.rs | 15 +- crates/noa-config/src/writer.rs | 125 ++++++++++++ crates/noa-grid/src/screen/edit.rs | 62 +++--- crates/noa-grid/src/tests/kitty_graphics.rs | 45 +++++ crates/noa-grid/src/tests/terminal_state.rs | 43 ++++- 25 files changed, 1073 insertions(+), 187 deletions(-) create mode 100644 crates/noa-app/src/atomic_write.rs diff --git a/crates/noa-app/src/app/event_loop.rs b/crates/noa-app/src/app/event_loop.rs index 6bf4294d..a2016969 100644 --- a/crates/noa-app/src/app/event_loop.rs +++ b/crates/noa-app/src/app/event_loop.rs @@ -30,6 +30,34 @@ fn shared_modifiers_after_focus_loss( } } +/// The app-level OS focus after `window_id` reports `Focused(false)`. When +/// macOS switches between two of our own windows the incoming window's +/// `Focused(true)` can land first and already repoint `os_focused`; the +/// outgoing window's loss must then leave it alone. Every consumer that +/// needs "is this *app* still frontmost" (Secure Keyboard Entry above all) +/// derives it from this result, never from the per-window boolean. +fn os_focused_after_focus_loss( + os_focused: Option, + window_id: WindowId, +) -> Option { + if os_focused == Some(window_id) { + None + } else { + os_focused + } +} + +/// The pane a button event routes to: the pane holding the live left-button +/// capture first, then the pane under the pointer, then the focused pane +/// (for events with no pointer position at all). +fn mouse_gesture_pane( + captured: Option, + hovered: Option, + focused: PaneId, +) -> PaneId { + captured.or(hovered).unwrap_or(focused) +} + impl App { /// Pane-dnd P1-1 remediation (`docs/specs/pane-dnd.md` L2(e)): re-resolve /// a pane-scoped `UserEvent`'s window at *receive* time rather than @@ -668,9 +696,11 @@ impl ApplicationHandler for App { let was_input_target = self.os_focused == Some(window_id); self.modifiers = shared_modifiers_after_focus_loss(self.modifiers, was_input_target); - if was_input_target { - self.os_focused = None; - } + self.os_focused = os_focused_after_focus_loss(self.os_focused, window_id); + // A left-button gesture captured by one of this window's panes + // can't complete once focus is gone (the release may never + // arrive): drop it now so the pane isn't left "pressed". + self.cancel_mouse_capture(window_id); self.finish_active_split_drag(window_id); // Cancel an in-flight overview pane drag when its host window // loses focus (Cmd-Tab / native tab switch mid-drag): the @@ -712,10 +742,15 @@ impl ApplicationHandler for App { self.report_focus_event(window_id, false); // Release Secure Keyboard Entry while backgrounded so it never // blocks key input to the rest of the system; a matching - // `Focused(true)` (including switching between our own windows) - // restores it. - self.secure_input - .on_focus_change(false, &mut crate::secure_input::CarbonSecureInput); + // `Focused(true)` restores it. The decision is made from the + // *app-level* focus computed above, not this window's boolean: + // when another of our windows already took focus (its + // `Focused(true)` arrived first) the app is still frontmost + // and the protection must stay up (N01). + self.secure_input.on_focus_change( + self.os_focused.is_some(), + &mut crate::secure_input::CarbonSecureInput, + ); if let Some(state) = self.windows.get(&window_id) { state.window.request_redraw(); } @@ -1406,7 +1441,22 @@ impl App { return; } - let Some((pane_id, cell)) = self.pane_cell_at_position(window_id, position, metrics) else { + // A left press captured a pane: this motion belongs to it (the cell is + // computed against *its* rect, clamped at the edges), not to whatever + // pane the pointer is over now. That keeps a selection or SGR drag + // that crosses a divider extending in the pane that saw the press, + // and lets that pane — not the neighbour — receive the release (N03). + let captured = self + .windows + .get(&window_id) + .and_then(|state| state.mouse_capture_pane); + let resolved = match captured { + Some(pane_id) => self + .pane_cell_in(window_id, pane_id, position, metrics) + .map(|cell| (pane_id, cell)), + None => self.pane_cell_at_position(window_id, position, metrics), + }; + let Some((pane_id, cell)) = resolved else { if let Some(state) = self.windows.get_mut(&window_id) { state.last_mouse_pane = None; } @@ -1462,9 +1512,16 @@ impl App { pub(super) fn on_cursor_left(&mut self, window_id: WindowId) { if let Some(state) = self.windows.get_mut(&window_id) { state.last_mouse_point = None; - state.last_mouse_pane = None; - for surface in state.surfaces.values_mut() { - surface.last_mouse_cell = None; + // A captured gesture survives the pointer leaving the window: the + // release still arrives here (macOS delivers mouse-up to the + // window that saw mouse-down) and must find the pane and its last + // cell intact. + let captured = state.mouse_capture_pane; + state.last_mouse_pane = captured; + for (pane_id, surface) in &mut state.surfaces { + if Some(*pane_id) != captured { + surface.last_mouse_cell = None; + } } } self.update_sidebar_button_hover(window_id, None); @@ -1640,15 +1697,31 @@ impl App { } } - let pane_id = self - .windows - .get(&window_id) - .and_then(|state| state.last_mouse_pane) - .or_else(|| self.windows.get(&window_id).map(|state| state.focused_pane)); + let pane_id = self.windows.get(&window_id).map(|tab| { + mouse_gesture_pane( + tab.mouse_capture_pane, + tab.last_mouse_pane, + tab.focused_pane, + ) + }); let Some(pane_id) = pane_id else { return; }; + // The left button captures the pane it pressed in until its release + // (see `on_cursor_moved`); the release always goes to the capturing + // pane, so a press in A and a release over B still clears A's + // pressed/drag state instead of handing B a release it never saw the + // press for. + if button == MouseButton::Left + && let Some(tab) = self.windows.get_mut(&window_id) + { + tab.mouse_capture_pane = match state { + ElementState::Pressed => Some(pane_id), + ElementState::Released => None, + }; + } + if button == MouseButton::Left && state == ElementState::Pressed { self.focus_pane(window_id, pane_id); } @@ -1842,6 +1915,10 @@ impl App { } Ime::Enabled | Ime::Disabled => self.modal_preedit = None, } + // The candidate window follows the modal's caret (N12): re-anchor + // on every composition step so it tracks the typed text rather + // than the terminal cursor behind the card. + self.update_focused_ime_cursor_area(window_id); self.request_window_redraw(window_id); return; } @@ -2069,3 +2146,72 @@ mod window_modifier_tests { ); } } + +#[cfg(test)] +mod focus_loss_tests { + use super::*; + use crate::secure_input::{SecureInput, SecureInputBackend}; + + struct Recording(Vec); + impl SecureInputBackend for Recording { + fn set_enabled(&mut self, enabled: bool) { + self.0.push(enabled); + } + } + + fn window(id: u64) -> WindowId { + WindowId::from(id) + } + + // N01: `B Focused(true)` → `A Focused(false)` (macOS reorders the pair + // when switching between our own windows). A's loss must not read as + // "the app lost focus": `os_focused` still points at B. + #[test] + fn outgoing_window_loss_keeps_app_focus_when_successor_already_focused() { + let (a, b) = (window(1), window(2)); + let os_focused = Some(b); + assert_eq!(os_focused_after_focus_loss(os_focused, a), Some(b)); + assert_eq!(os_focused_after_focus_loss(Some(a), a), None); + assert_eq!(os_focused_after_focus_loss(None, a), None); + } + + // The Secure Keyboard Entry state driven from that app-level focus: + // `A true → B true → A false` leaves the protection up; only `B false` + // (the last window) releases it. + #[test] + fn secure_input_survives_intra_app_window_switch() { + let (a, b) = (window(1), window(2)); + let mut secure = SecureInput::new(); + let mut backend = Recording(Vec::new()); + let mut os_focused = Some(a); + secure.toggle(os_focused.is_some(), &mut backend); + assert_eq!(backend.0, [true]); + + // B gains focus first … + os_focused = Some(b); + secure.on_focus_change(os_focused.is_some(), &mut backend); + // … then A's loss arrives: still frontmost, nothing released. + os_focused = os_focused_after_focus_loss(os_focused, a); + secure.on_focus_change(os_focused.is_some(), &mut backend); + assert_eq!( + backend.0, + [true], + "A's stale loss must not release the switch" + ); + + // B's loss is the real backgrounding. + os_focused = os_focused_after_focus_loss(os_focused, b); + secure.on_focus_change(os_focused.is_some(), &mut backend); + assert_eq!(backend.0, [true, false]); + } + + // N03: a live left-button capture wins over the pane under the pointer, + // so the release of a press in A that ends over B still routes to A. + #[test] + fn captured_pane_outranks_hovered_pane_for_button_events() { + let (a, b, focused) = (PaneId::new(1), PaneId::new(2), PaneId::new(3)); + assert_eq!(mouse_gesture_pane(Some(a), Some(b), focused), a); + assert_eq!(mouse_gesture_pane(None, Some(b), focused), b); + assert_eq!(mouse_gesture_pane(None, None, focused), focused); + } +} diff --git a/crates/noa-app/src/app/input_ops/ime.rs b/crates/noa-app/src/app/input_ops/ime.rs index 9dc68802..1f21965b 100644 --- a/crates/noa-app/src/app/input_ops/ime.rs +++ b/crates/noa-app/src/app/input_ops/ime.rs @@ -105,10 +105,9 @@ impl App { } } - // #TODO(agent): while a modal (search prompt / palette / rename) owns the - // composition, the candidate window still anchors to the terminal cursor - // below — it should anchor to the modal's caret instead (needs the modal - // card's pixel geometry here). + /// Tell the OS where the composition caret is, so the IME candidate + /// window opens beside it. While a modal owns the composition that is + /// the modal's input row (N12); otherwise the terminal cursor. pub(in crate::app) fn update_focused_ime_cursor_area(&self, window_id: WindowId) { let Some(gpu) = self.gpu.as_ref() else { return; @@ -119,17 +118,125 @@ impl App { let Some(surface) = state.focused_surface() else { return; }; + let metrics = gpu.fonts.get(state.font_px).metrics(); + if let Some((position, size)) = self.modal_ime_caret_area(window_id, state, metrics) { + state.window.set_ime_cursor_area(position, size); + return; + } let cursor = { let terminal = surface.terminal.lock(); terminal.active().cursor }; update_ime_cursor_area( &state.window, - gpu.fonts.get(state.font_px).metrics(), + metrics, cursor.x, cursor.y, surface.rect, self.padding, ); } + + /// The caret area (window-relative physical pixels) of the modal that + /// owns `window_id`'s composition, `None` when no modal does or the + /// owner has no text field (confirm dialog). The card geometry comes + /// from the same constants the AppKit builders use; the live preedit is + /// counted into the text length so the anchor tracks the composition. + fn modal_ime_caret_area( + &self, + window_id: WindowId, + state: &WindowState, + metrics: noa_font::Metrics, + ) -> Option<(PhysicalPosition, PhysicalSize)> { + let target = self.modal_ime_target(window_id)?; + let preedit_chars = self.modal_preedit_for(window_id, target).chars().count(); + let focused = state.focused_surface()?; + let scale = state.window.scale_factor(); + let pane = crate::macos_overlay::PaneRectPt::from_px( + focused.rect.x, + focused.rect.y, + focused.rect.w, + focused.rect.h, + scale, + ); + let caret_px = |caret: crate::macos_overlay::CaretPt| { + ( + PhysicalPosition::new( + ((pane.x + caret.x) * scale).round().max(0.0) as i32, + ((pane.y + caret.y) * scale).round().max(0.0) as i32, + ), + PhysicalSize::new( + (caret.w * scale).ceil().max(1.0) as u32, + (caret.h * scale).ceil().max(1.0) as u32, + ), + ) + }; + match target { + ModalImeTarget::ConfirmDialog => None, + ModalImeTarget::SearchPrompt => { + // One row at the top-right of the searched pane (see + // `append_search_prompt_instances`): the prompt text ends at + // the last column, with a status suffix of at most + // ` no matches` / ` 999/999` after the query. + const SUFFIX_COLS: usize = 12; + let session = self.search_prompt.as_ref()?; + let surface = state.surfaces.get(&session.pane_id)?; + let cols = usize::from(surface.grid_size.cols); + let shown = session.prompt.buffer().chars().count() + preedit_chars + SUFFIX_COLS; + let col = cols.saturating_sub(shown).min(cols.saturating_sub(1)); + Some(ime_cursor_area( + metrics, + col as u16, + 0, + surface.rect, + self.padding, + )) + } + ModalImeTarget::CommandPalette => { + let chars = self + .command_palette + .as_ref() + .map_or(0, |session| session.palette.query().chars().count()); + Some(caret_px(crate::macos_overlay::palette_query_caret( + pane, + chars + preedit_chars, + ))) + } + ModalImeTarget::RemoteUi => Some(caret_px(crate::macos_overlay::palette_query_caret( + pane, + self.remote_ui_input_chars() + preedit_chars, + ))), + ModalImeTarget::TabTitlePrompt => { + let chars = self + .tab_title_prompt + .as_ref() + .map_or(0, |session| session.buffer.chars().count()); + Some(caret_px(crate::macos_overlay::title_prompt_caret( + pane, + chars + preedit_chars, + ))) + } + ModalImeTarget::ThemeSettings => { + Some(caret_px(crate::macos_overlay::theme_settings_caret(pane))) + } + ModalImeTarget::SidebarRename => { + // The renamed card's name row, from the same layout the + // sidebar draws and hit-tests with. + let session = self.sidebar_rename.as_ref()?; + let inset = self.window_sidebar_inset_px(window_id); + let bounds = self.sidebar_layout_bounds(window_id, inset); + let windows = self.session_windows_for_window(window_id); + let ids = self.session_store.ordered_ids_for_windows(&windows); + let layout = + self.sidebar_metrics(window_id) + .layout(bounds, &ids, state.sidebar_scroll); + let card = layout.cards.iter().find(|card| card.id == session.card)?; + let rect = card.name_line; + Some(( + PhysicalPosition::new(rect.x as i32, rect.y as i32), + PhysicalSize::new(1, rect.h.max(1)), + )) + } + } + } } diff --git a/crates/noa-app/src/app/input_ops/pointer.rs b/crates/noa-app/src/app/input_ops/pointer.rs index d857136d..d942c865 100644 --- a/crates/noa-app/src/app/input_ops/pointer.rs +++ b/crates/noa-app/src/app/input_ops/pointer.rs @@ -159,18 +159,50 @@ impl App { Some(HitTarget::Pane(pane_id)) => pane_id, Some(HitTarget::Divider) | None => return None, }; - let surface = state.surfaces.get(&pane_id)?; + let cell = self.pane_cell_in(window_id, pane_id, position, metrics)?; + Some((pane_id, cell)) + } + + /// The grid cell of `pane_id` under a window-relative `position`, with no + /// hit test: a position outside the pane's rect clamps to its edge cells. + /// This is how a captured drag keeps reporting cells of the pane that saw + /// the press after the pointer has crossed a divider. `None` only when + /// the pane no longer exists in `window_id`. + pub(in crate::app) fn pane_cell_in( + &self, + window_id: WindowId, + pane_id: PaneId, + position: PhysicalPosition, + metrics: noa_font::Metrics, + ) -> Option { + let surface = self.windows.get(&window_id)?.surfaces.get(&pane_id)?; let local_x = position.x - f64::from(surface.rect.x); let local_y = position.y - f64::from(surface.rect.y); - let cell = mouse::physical_position_to_grid_point( + Some(mouse::physical_position_to_grid_point( local_x, local_y, metrics.cell_w, metrics.cell_h, surface.grid_size, self.padding, - ); - Some((pane_id, cell)) + )) + } + + /// Abandon the live left-button capture in `window_id` (focus loss): the + /// capturing pane's pressed-button and selection-drag state are cleared + /// as if its release had arrived, so a later motion can't keep extending + /// a selection or report a phantom button-held drag to a tracking TUI. + pub(in crate::app) fn cancel_mouse_capture(&mut self, window_id: WindowId) { + let Some(state) = self.windows.get_mut(&window_id) else { + return; + }; + let Some(pane_id) = state.mouse_capture_pane.take() else { + return; + }; + if let Some(surface) = state.surfaces.get_mut(&pane_id) { + surface.pressed_mouse_button = None; + let _ = surface.mouse_selection.left_released(); + } } /// The Cmd+hover link under the mouse in `window_id`'s focused-under- diff --git a/crates/noa-app/src/app/ipc.rs b/crates/noa-app/src/app/ipc.rs index c7bf4871..d251e6e4 100644 --- a/crates/noa-app/src/app/ipc.rs +++ b/crates/noa-app/src/app/ipc.rs @@ -398,7 +398,12 @@ impl App { self.mark_pane_paste_input(window_id, pane_id); } self.snap_pane_viewport_to_bottom(window_id, pane_id); - self.write_pane_pty_bytes(window_id, pane_id, bytes); + // The queue outcome is the reply: a full input budget or a + // gone sink must surface to the automation client, which + // otherwise proceeds as if the text had been accepted + // (N02). `write_pane_pty_bytes` only logs those. + let result = self.queue_pane_pty_bytes(window_id, pane_id, bytes); + ipc_input_queue_result(result)?; } Ok(IpcActionReply::Ok) } @@ -578,6 +583,26 @@ fn apply_attach_grid_first_resize( dispatch_pty_resize(new_size) } +/// Map a pty input queue outcome onto the `noa.sendText` reply. `Queued` and +/// `Deferred` are both acceptance (the queue owns the bytes and will write +/// them); `Dropped` means the pane's input budget is exhausted because the +/// foreground program isn't reading its tty, and `Disconnected` that the +/// pane has no live sink (writer thread or remote transport gone). Neither +/// of the last two may be reported as success. "Accepted by the queue" is +/// still not "consumed by the child" — that guarantee doesn't exist. +fn ipc_input_queue_result( + result: crate::io_thread::QueueInputResult, +) -> Result<(), noa_ipc::IpcError> { + use crate::io_thread::QueueInputResult; + match result { + QueueInputResult::Queued | QueueInputResult::Deferred => Ok(()), + QueueInputResult::Dropped => Err(noa_ipc::IpcError::Internal( + "pty input queue is full: the foreground program is not reading its tty".to_string(), + )), + QueueInputResult::Disconnected => Err(noa_ipc::IpcError::PaneClosed), + } +} + #[cfg(test)] mod attach_tests { use super::*; @@ -618,3 +643,25 @@ mod attach_tests { ); } } + +#[cfg(test)] +mod send_text_reply_tests { + use super::*; + use crate::io_thread::QueueInputResult; + + // N02: a rejected or sink-less queue must not turn into `Ok` for the + // automation client; only actual acceptance does. + #[test] + fn send_text_reports_queue_rejection_instead_of_success() { + assert!(ipc_input_queue_result(QueueInputResult::Queued).is_ok()); + assert!(ipc_input_queue_result(QueueInputResult::Deferred).is_ok()); + assert!(matches!( + ipc_input_queue_result(QueueInputResult::Dropped), + Err(noa_ipc::IpcError::Internal(message)) if message.contains("queue is full") + )); + assert!(matches!( + ipc_input_queue_result(QueueInputResult::Disconnected), + Err(noa_ipc::IpcError::PaneClosed) + )); + } +} diff --git a/crates/noa-app/src/app/lifecycle.rs b/crates/noa-app/src/app/lifecycle.rs index 18382d63..8294c9f4 100644 --- a/crates/noa-app/src/app/lifecycle.rs +++ b/crates/noa-app/src/app/lifecycle.rs @@ -608,6 +608,7 @@ impl App { focused_pane: initial_pane, surfaces, last_mouse_pane: Some(initial_pane), + mouse_capture_pane: None, last_mouse_point: None, last_mouse_physical_position: None, active_split_drag: None, @@ -1372,6 +1373,9 @@ impl App { .surfaces .get(&pane_id) .and_then(|surface| surface.scrollback_key.clone()); + if state.mouse_capture_pane == Some(pane_id) { + state.mouse_capture_pane = None; + } if let Some(mut surface) = state.surfaces.remove(&pane_id) { surface.shutdown(); } diff --git a/crates/noa-app/src/app/quick_terminal.rs b/crates/noa-app/src/app/quick_terminal.rs index 7511bcfb..0c250244 100644 --- a/crates/noa-app/src/app/quick_terminal.rs +++ b/crates/noa-app/src/app/quick_terminal.rs @@ -821,6 +821,7 @@ impl App { focused_pane: initial_pane, surfaces, last_mouse_pane: Some(initial_pane), + mouse_capture_pane: None, last_mouse_point: None, last_mouse_physical_position: None, active_split_drag: None, diff --git a/crates/noa-app/src/app/remote_ui.rs b/crates/noa-app/src/app/remote_ui.rs index 229d8bb8..14d940b7 100644 --- a/crates/noa-app/src/app/remote_ui.rs +++ b/crates/noa-app/src/app/remote_ui.rs @@ -1023,6 +1023,15 @@ impl App { } } + /// Characters in the endpoint input field (0 outside that phase), for + /// the IME candidate-window anchor. + pub(in crate::app) fn remote_ui_input_chars(&self) -> usize { + match self.remote_ui.as_ref().map(|session| &session.phase) { + Some(RemoteUiPhase::EndpointInput { buffer, .. }) => buffer.chars().count(), + _ => 0, + } + } + pub(in crate::app) fn push_remote_ui_text(&mut self, text: &str) { let filtered = text .chars() diff --git a/crates/noa-app/src/app/scratch_terminal.rs b/crates/noa-app/src/app/scratch_terminal.rs index f745f56f..d0e0aa9e 100644 --- a/crates/noa-app/src/app/scratch_terminal.rs +++ b/crates/noa-app/src/app/scratch_terminal.rs @@ -401,6 +401,7 @@ impl App { focused_pane: initial_pane, surfaces, last_mouse_pane: Some(initial_pane), + mouse_capture_pane: None, last_mouse_point: None, last_mouse_physical_position: None, active_split_drag: None, diff --git a/crates/noa-app/src/app/split_ops.rs b/crates/noa-app/src/app/split_ops.rs index c9152084..a823bff0 100644 --- a/crates/noa-app/src/app/split_ops.rs +++ b/crates/noa-app/src/app/split_ops.rs @@ -515,6 +515,9 @@ impl App { let Some(mut surface) = source_state.surfaces.remove(&pane) else { return false; }; + if source_state.mouse_capture_pane == Some(pane) { + source_state.mouse_capture_pane = None; + } source_state.split_tree = transform.source_tree; if source_state.zoomed == Some(pane) { source_state.zoomed = None; diff --git a/crates/noa-app/src/app/state.rs b/crates/noa-app/src/app/state.rs index 06c53caf..c422a10c 100644 --- a/crates/noa-app/src/app/state.rs +++ b/crates/noa-app/src/app/state.rs @@ -270,6 +270,10 @@ pub(super) struct WindowState { pub(super) focused_pane: PaneId, pub(super) surfaces: HashMap, pub(super) last_mouse_pane: Option, + /// The pane that received the live left-button press. Motion and the + /// matching release route to it even when the pointer has crossed into a + /// sibling split (xterm/Ghostty grab semantics); `None` outside a press. + pub(super) mouse_capture_pane: Option, pub(super) last_mouse_point: Option, /// Raw physical pointer position from the most recent `CursorMoved`. /// Kept alongside `last_mouse_point`/`last_mouse_pane` for handlers that diff --git a/crates/noa-app/src/atomic_write.rs b/crates/noa-app/src/atomic_write.rs new file mode 100644 index 00000000..3b5214e2 --- /dev/null +++ b/crates/noa-app/src/atomic_write.rs @@ -0,0 +1,110 @@ +//! Crash- and race-safe whole-file replacement: write to a sibling staging +//! file, `sync_all`, then `rename` over the target, so a reader never sees a +//! truncated file and a crash mid-write leaves the previous good file. +//! +//! The staging name is unique per process *and* per call (`create_new`), so +//! two noa processes saving the same path at once can never share one +//! staging file — the shared `path.with_extension("tmp")` a store used to +//! pick let the loser's open descriptor rewrite the winner's already +//! published file and then fail its own rename, leaving "save failed" in the +//! UI with the failed content on disk (session store B03, favorites N05). +//! Which of two concurrent whole-file saves lands last is still unordered; +//! only corruption and phantom failures are ruled out here. + +use std::fs; +use std::io::{self, Write}; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +/// Replace the file at `path` with `bytes`, creating the parent directory. +pub(crate) fn write_atomic(path: &Path, bytes: &[u8]) -> io::Result<()> { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent)?; + } + let (tmp, mut file) = loop { + let tmp = staging_path(path); + match fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&tmp) + { + Ok(file) => break (tmp, file), + // A crashed process with a reused PID may have left this exact + // name behind; the counter makes the next candidate fresh. + Err(err) if err.kind() == io::ErrorKind::AlreadyExists => continue, + Err(err) => return Err(err), + } + }; + let result = (|| { + file.write_all(bytes)?; + file.sync_all()?; + fs::rename(&tmp, path) + })(); + drop(file); + if result.is_err() { + let _ = fs::remove_file(&tmp); + } + result +} + +/// `....tmp` beside `path`, unique per process and call. +fn staging_path(path: &Path) -> PathBuf { + static SEQ: AtomicU64 = AtomicU64::new(0); + let name = path + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_else(|| "file".to_string()); + path.with_file_name(format!( + ".{name}.{}.{}.tmp", + std::process::id(), + SEQ.fetch_add(1, Ordering::Relaxed) + )) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn temp_dir(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "noa-atomic-write-{tag}-{}-{}", + std::process::id(), + SEQ_TEST.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&dir).unwrap(); + dir + } + static SEQ_TEST: AtomicU64 = AtomicU64::new(0); + + #[test] + fn staging_names_are_unique_per_call() { + let path = Path::new("/some/dir/favorites"); + let a = staging_path(path); + let b = staging_path(path); + assert_ne!(a, b); + assert_eq!(a.parent(), path.parent()); + assert!( + a.file_name() + .unwrap() + .to_string_lossy() + .starts_with(".favorites.") + ); + assert!(a.extension().is_some_and(|ext| ext == "tmp")); + } + + #[test] + fn write_replaces_the_file_and_leaves_no_staging_file_behind() { + let dir = temp_dir("replace"); + let path = dir.join("nested").join("store"); + write_atomic(&path, b"one").unwrap(); + write_atomic(&path, b"two").unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"two"); + let leftovers: Vec<_> = fs::read_dir(path.parent().unwrap()) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .filter(|name| name != "store") + .collect(); + assert!(leftovers.is_empty(), "{leftovers:?}"); + fs::remove_dir_all(&dir).unwrap(); + } +} diff --git a/crates/noa-app/src/input/key.rs b/crates/noa-app/src/input/key.rs index 142259a8..ad806151 100644 --- a/crates/noa-app/src/input/key.rs +++ b/crates/noa-app/src/input/key.rs @@ -75,6 +75,7 @@ pub fn encode_key_with_modes( physical_key, text, mods, + alt_sends_esc, kitty_flags, pressed, repeat, diff --git a/crates/noa-app/src/input/kitty.rs b/crates/noa-app/src/input/kitty.rs index 32253ee6..18f76544 100644 --- a/crates/noa-app/src/input/kitty.rs +++ b/crates/noa-app/src/input/kitty.rs @@ -44,6 +44,13 @@ fn kitty_modifier_value(mods: ModifiersState) -> u32 { value } +/// `alt_sends_esc` is the caller's per-event verdict on whether a held Alt +/// is *Alt* or a macOS Option that composed `text` (see +/// `encode_key_with_modes`). A composing Option is not a modifier for the +/// "does this key escape-encode" decision (Ghostty's `effectiveMods`): the +/// character it produced is plain text under every flag set. The reported +/// modifier field still carries it (Ghostty reports `all_mods`), and it only +/// suppresses associated text when it is claimed as Alt. #[allow(clippy::too_many_arguments)] pub(super) fn encode_kitty( logical_key: &Key, @@ -51,6 +58,7 @@ pub(super) fn encode_kitty( physical_key: Option, text: Option<&str>, mods: ModifiersState, + alt_sends_esc: bool, flags: u8, pressed: bool, repeat: bool, @@ -74,81 +82,94 @@ pub(super) fn encode_kitty( }; let mods_value = kitty_modifier_value(mods); - let has_non_shift = mods.control_key() || mods.alt_key() || mods.super_key(); + let alt_is_modifier = mods.alt_key() && (alt_sends_esc || text.is_none_or(str::is_empty)); + let has_non_shift = mods.control_key() || alt_is_modifier || mods.super_key(); + // Physical keypad keys carry dedicated code points: Ghostty's kitty + // table keys on the physical key, so KP_Enter is 57414, never Enter's + // 13. Text-producing keypad keys (digits, operators) stay legacy text + // without a non-shift modifier unless report-all forces the escape + // form, exactly like their main-block twins; the non-text ones + // (KP_Enter) always escape-encode under any flag, per the disambiguate + // rule for keypad keys (N11). + let keypad = keypad_key_code(physical_key).map(|number| KittyKey { + number, + suffix: b'u', + shifted: None, + }); // Classify the key and decide whether it escape-encodes under these flags. - let key = match logical_key { - Key::Named(NamedKey::Escape) => KittyKey { - number: 27, - suffix: b'u', - shifted: None, - }, - Key::Named(NamedKey::Enter) => { - if mods_value == 1 && !report_all { - return legacy_or_ignore(event); - } - KittyKey { - number: 13, - suffix: b'u', - shifted: None, - } - } - Key::Named(NamedKey::Tab) => { - if mods_value == 1 && !report_all { + let key = match (keypad, logical_key) { + (Some(key), Key::Character(_)) => { + if !report_all && !has_non_shift { return legacy_or_ignore(event); } - KittyKey { - number: 9, - suffix: b'u', - shifted: None, - } + key } - Key::Named(NamedKey::Backspace) => { - if mods_value == 1 && !report_all { - return legacy_or_ignore(event); - } - KittyKey { - number: 127, + (Some(key), _) => key, + (None, logical_key) => match logical_key { + Key::Named(NamedKey::Escape) => KittyKey { + number: 27, suffix: b'u', shifted: None, + }, + Key::Named(NamedKey::Enter) => { + if mods_value == 1 && !report_all { + return legacy_or_ignore(event); + } + KittyKey { + number: 13, + suffix: b'u', + shifted: None, + } } - } - Key::Named(NamedKey::Space) => { - if !report_all && !has_non_shift { - return legacy_or_ignore(event); - } - KittyKey { - number: 32, - suffix: b'u', - shifted: None, + Key::Named(NamedKey::Tab) => { + if mods_value == 1 && !report_all { + return legacy_or_ignore(event); + } + KittyKey { + number: 9, + suffix: b'u', + shifted: None, + } } - } - Key::Named(named) => match functional_key(*named) { - // Functional keys (arrows, F-keys, Home/End/...) always escape-encode. - Some((number, suffix)) => KittyKey { - number, - suffix, - shifted: None, - }, - // Modifier keys alone are reported only with report-all-keys. - None => match modifier_key_code(physical_key) { - Some(number) if report_all => KittyKey { - number, + Key::Named(NamedKey::Backspace) => { + if mods_value == 1 && !report_all { + return legacy_or_ignore(event); + } + KittyKey { + number: 127, suffix: b'u', shifted: None, - }, - _ => return KittyOutcome::Ignore, - }, - }, - Key::Character(s) => { - // Numpad keys get their dedicated codes only under report-all. - if report_all && let Some(number) = keypad_key_code(physical_key) { + } + } + Key::Named(NamedKey::Space) => { + if !report_all && !has_non_shift { + return legacy_or_ignore(event); + } KittyKey { - number, + number: 32, suffix: b'u', shifted: None, } - } else { + } + Key::Named(named) => match functional_key(*named) { + // Functional keys (arrows, F-keys, Home/End/...) always escape-encode. + Some((number, suffix)) => KittyKey { + number, + suffix, + shifted: None, + }, + // Modifier keys alone are reported only with report-all-keys. + None => match modifier_key_code(physical_key) { + Some(number) if report_all => KittyKey { + number, + suffix: b'u', + shifted: None, + }, + _ => return KittyOutcome::Ignore, + }, + }, + Key::Character(s) => { let Some((base, shifted)) = character_key_codes(s, unmodified_key, mods) else { return legacy_or_ignore(event); }; @@ -163,12 +184,13 @@ pub(super) fn encode_kitty( shifted, } } - } - _ => return legacy_or_ignore(event), + _ => return legacy_or_ignore(event), + }, }; // Associated text: only for press/repeat, only when no modifier other than - // shift is active, and only for genuinely printable text. + // shift (or a composing Option) is active, and only for genuinely + // printable text. let assoc_text = if report_text && event != 3 && !has_non_shift { associated_text_codepoints(text) } else { diff --git a/crates/noa-app/src/input/tests.rs b/crates/noa-app/src/input/tests.rs index e172c319..330a30f8 100644 --- a/crates/noa-app/src/input/tests.rs +++ b/crates/noa-app/src/input/tests.rs @@ -1574,3 +1574,108 @@ fn kitty_flags_take_precedence_over_modify_other_keys() { ); assert_eq!(bytes, Some(b"\x1b[105;5u".to_vec())); } + +// N09: a macOS Option that composed the delivered character (`alt_sends_esc` +// false) is not Alt for the Kitty escape decision — Ghostty's +// `effectiveMods` strips a consumed modifier — so the composed text passes +// through as text under disambiguate, and under report-all the sequence +// keeps the modifier bit but still carries the associated text. +#[test] +fn kitty_composing_option_is_not_alt() { + let press = |flags: u8, alt_sends_esc: bool| { + encode_key_with_modes( + &Key::Character("å".into()), + Some(&Key::Character("a".into())), + None, + Some("å"), + ModifiersState::ALT, + alt_sends_esc, + false, + false, + flags, + false, + true, + false, + ) + }; + assert_eq!( + press(KITTY_DISAMBIGUATE, false), + Some("å".as_bytes().to_vec()) + ); + // Option claimed as Alt (`macos-option-as-alt`): a real Alt+a chord. + assert_eq!( + press(KITTY_DISAMBIGUATE, true), + Some(b"\x1b[97;3u".to_vec()) + ); + assert_eq!( + press(KITTY_REPORT_ALL_KEYS | KITTY_REPORT_ASSOCIATED_TEXT, false), + Some(b"\x1b[97;3;229u".to_vec()) + ); + assert_eq!( + press(KITTY_REPORT_ALL_KEYS | KITTY_REPORT_ASSOCIATED_TEXT, true), + Some(b"\x1b[97;3u".to_vec()) + ); +} + +// N11: the keypad Enter is a distinct Kitty key (57414), reachable even +// though winit reports its *logical* key as plain `Enter`. +#[test] +fn kitty_keypad_enter_uses_its_dedicated_code() { + let press = |flags: u8, mods: ModifiersState| { + encode_key_with_modes( + &Key::Named(NamedKey::Enter), + None, + Some(PhysicalKey::Code(KeyCode::NumpadEnter)), + Some("\r"), + mods, + true, + false, + false, + flags, + false, + true, + false, + ) + }; + assert_eq!( + press(KITTY_REPORT_ALL_KEYS, ModifiersState::empty()), + Some(b"\x1b[57414u".to_vec()) + ); + // Non-text keypad keys escape-encode under disambiguate too (Ghostty + // keys its table on the physical key, so KP_Enter never takes Enter's + // bare-CR exemption). + assert_eq!( + press(KITTY_DISAMBIGUATE, ModifiersState::empty()), + Some(b"\x1b[57414u".to_vec()) + ); + assert_eq!( + press(KITTY_DISAMBIGUATE, ModifiersState::CONTROL), + Some(b"\x1b[57414;5u".to_vec()) + ); + // A bare keypad digit is still text under disambiguate; a modified one + // reports its keypad code, not the main-block digit's. + let digit = |flags: u8, mods: ModifiersState| { + encode_key_with_modes( + &Key::Character("5".into()), + None, + Some(PhysicalKey::Code(KeyCode::Numpad5)), + Some("5"), + mods, + true, + false, + false, + flags, + false, + true, + false, + ) + }; + assert_eq!( + digit(KITTY_DISAMBIGUATE, ModifiersState::empty()), + Some(b"5".to_vec()) + ); + assert_eq!( + digit(KITTY_DISAMBIGUATE, ModifiersState::CONTROL), + Some(b"\x1b[57404;5u".to_vec()) + ); +} diff --git a/crates/noa-app/src/lib.rs b/crates/noa-app/src/lib.rs index 7edcea31..c194989c 100644 --- a/crates/noa-app/src/lib.rs +++ b/crates/noa-app/src/lib.rs @@ -6,6 +6,7 @@ mod anim; mod app; mod app_actions; +mod atomic_write; pub mod auto_approve; mod branch_poll; mod chrome; diff --git a/crates/noa-app/src/macos_overlay.rs b/crates/noa-app/src/macos_overlay.rs index 226c039d..09039e49 100644 --- a/crates/noa-app/src/macos_overlay.rs +++ b/crates/noa-app/src/macos_overlay.rs @@ -25,7 +25,10 @@ mod tests; #[cfg(target_os = "macos")] pub(crate) use model::cg; -pub(crate) use model::{NativeOverlayCache, OverlayColors, PaneRectPt, TITLE_PROMPT_HINT}; +pub(crate) use model::{ + CaretPt, NativeOverlayCache, OverlayColors, PaneRectPt, TITLE_PROMPT_HINT, palette_query_caret, + theme_settings_caret, title_prompt_caret, +}; pub(crate) use sync::{ sync_command_palette, sync_confirm_dialog, sync_process_monitor, sync_scratch_badge, sync_theme_settings, sync_title_prompt, sync_toast, diff --git a/crates/noa-app/src/macos_overlay/imp/appkit.rs b/crates/noa-app/src/macos_overlay/imp/appkit.rs index af3b4425..a8a6d2ac 100644 --- a/crates/noa-app/src/macos_overlay/imp/appkit.rs +++ b/crates/noa-app/src/macos_overlay/imp/appkit.rs @@ -34,13 +34,15 @@ const ID_TITLE_PROMPT: &str = "noa.native-overlay.title-prompt"; const ID_TOAST: &str = "noa.native-overlay.toast"; const ID_SCRATCH_BADGE: &str = "noa.native-overlay.scratch-badge"; -/// Palette metrics (points). -const PALETTE_WIDTH: f64 = 560.0; -const QUERY_ROW_H: f64 = 44.0; +/// Palette metrics (points). The widths/row heights the IME caret anchor +/// also needs live in `model.rs` so the two can't drift. +use crate::macos_overlay::model::{ + CARD_PAD_H, PALETTE_WIDTH, QUERY_ROW_H, THEME_SETTINGS_WIDTH, TITLE_PROMPT_H, + TITLE_PROMPT_WIDTH, +}; const ENTRY_ROW_H: f64 = 26.0; const HEADER_ROW_H: f64 = 24.0; const LIST_PAD_V: f64 = 6.0; -const CARD_PAD_H: f64 = 16.0; const CARD_RADIUS: f64 = 12.0; /// Max list rows (headers + entries) visible at once — matches the wgpu /// card's 12-row window. @@ -863,7 +865,7 @@ pub(in crate::macos_overlay) fn rebuild_theme_settings( return; }; - let card_w = 660.0_f64.min(pane.w - 32.0).max(320.0); + let card_w = THEME_SETTINGS_WIDTH.min(pane.w - 32.0).max(320.0); // The card height is content-driven: title block + exactly one // section (the theme list+sample pane in Theme mode, the settings // rows in Settings mode — a session never shows both, DEC-2) + @@ -1843,8 +1845,8 @@ pub(in crate::macos_overlay) fn rebuild_title_prompt( return; }; - let card_w = 420.0_f64.min(pane.w - 32.0).max(240.0); - let card_h = 104.0; + let card_w = TITLE_PROMPT_WIDTH.min(pane.w - 32.0).max(240.0); + let card_h = TITLE_PROMPT_H; let card_frame = NSRect::new( NSPoint::new( (pane.w - card_w) / 2.0, diff --git a/crates/noa-app/src/macos_overlay/model.rs b/crates/noa-app/src/macos_overlay/model.rs index 005cf6e5..62a497ed 100644 --- a/crates/noa-app/src/macos_overlay/model.rs +++ b/crates/noa-app/src/macos_overlay/model.rs @@ -69,6 +69,82 @@ impl NativeOverlayCache { /// card in `app.rs`. pub(crate) const TITLE_PROMPT_HINT: &str = "Enter to set \u{b7} Empty clears \u{b7} Esc to cancel"; +/// Card metrics (points) shared by the AppKit card builders (`imp/appkit.rs`) +/// and the IME caret anchors below, so the candidate window and the drawn +/// input row can't drift apart. +pub(crate) const PALETTE_WIDTH: f64 = 560.0; +pub(crate) const QUERY_ROW_H: f64 = 44.0; +pub(crate) const CARD_PAD_H: f64 = 16.0; +pub(crate) const TITLE_PROMPT_WIDTH: f64 = 420.0; +pub(crate) const TITLE_PROMPT_H: f64 = 104.0; +pub(crate) const THEME_SETTINGS_WIDTH: f64 = 660.0; +/// Mean advance of the 15pt system font the input rows use — an estimate +/// (the labels are proportional), good enough to put the candidate window +/// at the end of the typed text rather than at its start. +const INPUT_FONT_ADVANCE: f64 = 8.3; +const INPUT_ROW_H: f64 = 20.0; + +/// A caret rectangle in points, relative to the pane rect's origin (the +/// cards are laid out inside the focused pane's frame). +#[derive(Clone, Copy, Debug, PartialEq)] +pub(crate) struct CaretPt { + pub(crate) x: f64, + pub(crate) y: f64, + pub(crate) w: f64, + pub(crate) h: f64, +} + +/// The palette query row's caret after `query_chars` characters (also the +/// send-selection picker and the remote-UI endpoint field, which draw the +/// same card). Mirrors `rebuild_palette`'s frame math with the card's +/// minimum height (query row + empty-list stub) standing in for the +/// list-dependent height — the `min(pane.h - card_h)` term only binds on a +/// pane shorter than the card. +pub(crate) fn palette_query_caret(pane: PaneRectPt, query_chars: usize) -> CaretPt { + let card_w = PALETTE_WIDTH.min(pane.w - 32.0).max(280.0); + let card_h_min = QUERY_ROW_H + 1.0 + 36.0; + let card_x = (pane.w - card_w) / 2.0; + let card_top = (pane.h * 0.14).min(pane.h - card_h_min).max(8.0); + CaretPt { + x: card_x + CARD_PAD_H + 22.0 + query_chars as f64 * INPUT_FONT_ADVANCE, + y: card_top + 13.0, + w: 1.0, + h: INPUT_ROW_H, + } +} + +/// The "Set Tab Title" prompt's caret: its input row is centred, so the +/// caret sits half the text's width right of the card's centre line. +pub(crate) fn title_prompt_caret(pane: PaneRectPt, input_chars: usize) -> CaretPt { + let card_w = TITLE_PROMPT_WIDTH.min(pane.w - 32.0).max(240.0); + let card_x = (pane.w - card_w) / 2.0; + let card_top = (pane.h * 0.30).min(pane.h - TITLE_PROMPT_H); + CaretPt { + x: card_x + card_w / 2.0 + input_chars as f64 * INPUT_FONT_ADVANCE / 2.0, + y: card_top + 40.0, + w: 1.0, + h: INPUT_ROW_H, + } +} + +/// The theme-settings card's search/filter field, at the card's upper-left. +/// The card's height depends on its row lists; the vertical bound it is +/// capped at (`pane.h - 24`, at least 240) is used, which is exact whenever +/// the catalogue fills the card (the common case) and a few rows high +/// otherwise. +pub(crate) fn theme_settings_caret(pane: PaneRectPt) -> CaretPt { + let card_w = THEME_SETTINGS_WIDTH.min(pane.w - 32.0).max(320.0); + let card_h = (pane.h - 24.0).max(240.0); + let card_x = (pane.w - card_w) / 2.0; + let card_top = (pane.h - card_h) / 2.0; + CaretPt { + x: card_x + 20.0, + y: card_top + 46.0, + w: 1.0, + h: INPUT_ROW_H, + } +} + /// A pane rectangle in AppKit points, top-left origin relative to the /// window's content view (i.e. physical px / scale factor). #[derive(Clone, Copy, Debug, PartialEq)] diff --git a/crates/noa-app/src/macos_overlay/tests.rs b/crates/noa-app/src/macos_overlay/tests.rs index ab6e80ac..61598cbc 100644 --- a/crates/noa-app/src/macos_overlay/tests.rs +++ b/crates/noa-app/src/macos_overlay/tests.rs @@ -1,5 +1,7 @@ use super::model::{ - NativeOverlayCache, OverlayColors, PaneRectPt, overlay_scroll_window, theme_settings_view_model, + CARD_PAD_H, NativeOverlayCache, OverlayColors, PALETTE_WIDTH, PaneRectPt, THEME_SETTINGS_WIDTH, + overlay_scroll_window, palette_query_caret, theme_settings_caret, theme_settings_view_model, + title_prompt_caret, }; use super::sync::theme_settings_sync_decision; use crate::theme_settings::{Liveness, ThemeSettings, ThemeSettingsInit, ThemeSettingsMode}; @@ -344,3 +346,42 @@ fn pane_rect_pt_scales_from_px() { assert_eq!(rect.w, 400.0); assert_eq!(rect.h, 300.0); } + +// N12: the modal caret anchors land inside their card's input row, not at +// the terminal cursor, and advance with the typed text. +#[test] +fn modal_caret_anchors_sit_in_the_input_row_and_follow_the_text() { + let pane = PaneRectPt { + x: 0.0, + y: 0.0, + w: 1200.0, + h: 800.0, + }; + let card_x = (1200.0 - PALETTE_WIDTH) / 2.0; + let empty = palette_query_caret(pane, 0); + assert_eq!(empty.x, card_x + CARD_PAD_H + 22.0); + assert_eq!(empty.y, 800.0 * 0.14 + 13.0); + let typed = palette_query_caret(pane, 10); + assert!(typed.x > empty.x && typed.y == empty.y); + + let title = title_prompt_caret(pane, 0); + assert_eq!(title.x, 600.0); + assert_eq!(title.y, 800.0 * 0.30 + 40.0); + assert!(title_prompt_caret(pane, 4).x > title.x); + + let theme = theme_settings_caret(pane); + assert_eq!(theme.x, (1200.0 - THEME_SETTINGS_WIDTH) / 2.0 + 20.0); + assert_eq!(theme.y, 12.0 + 46.0); + + // A pane narrower than the card clamps the card to the pane's width + // minus its margins; the caret must stay inside it. + let narrow = PaneRectPt { + x: 0.0, + y: 0.0, + w: 400.0, + h: 300.0, + }; + let caret = palette_query_caret(narrow, 0); + assert!(caret.x >= 16.0 && caret.x < 400.0 - 16.0); + assert!(caret.y > 0.0 && caret.y < 300.0); +} diff --git a/crates/noa-app/src/session.rs b/crates/noa-app/src/session.rs index 57986b9d..030be018 100644 --- a/crates/noa-app/src/session.rs +++ b/crates/noa-app/src/session.rs @@ -468,60 +468,13 @@ fn parse_remote(value: &json::Value) -> Option { }) } -/// Atomically write the session to `path`, creating the parent directory. -/// Writes to a sibling temp file then renames, so a crash mid-write cannot -/// truncate an existing good session file. -/// -/// The temp name is unique per process and per call (`create_new`), so two -/// noa processes saving to the same path at once cannot share one staging -/// file and interleave their bytes into the published `session.json` -/// (B03, 2026-09 audit #2). Which of two concurrent whole-file saves lands -/// last is still unordered; only corruption is ruled out here. +/// Atomically write the session to `path`, creating the parent directory +/// (staging file + rename, unique per process and call — see +/// [`crate::atomic_write`], B03 2026-09 audit #2). Which of two concurrent +/// whole-file saves lands last is still unordered; only corruption is ruled +/// out here. pub fn save(path: &Path, state: &SessionState) -> std::io::Result<()> { - use std::io::Write; - - if let Some(parent) = path.parent() { - fs::create_dir_all(parent)?; - } - let (tmp, mut file) = loop { - let tmp = staging_path(path); - match fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(&tmp) - { - Ok(file) => break (tmp, file), - // A crashed process with a reused PID may have left this exact - // name behind; the counter makes the next candidate fresh. - Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => continue, - Err(err) => return Err(err), - } - }; - let result = (|| { - file.write_all(serialize(state).as_bytes())?; - file.sync_all()?; - fs::rename(&tmp, path) - })(); - drop(file); - if result.is_err() { - let _ = fs::remove_file(&tmp); - } - result -} - -/// `....tmp` beside `path`, unique per process and call. -fn staging_path(path: &Path) -> std::path::PathBuf { - use std::sync::atomic::{AtomicU64, Ordering}; - static SEQ: AtomicU64 = AtomicU64::new(0); - let name = path - .file_name() - .map(|n| n.to_string_lossy().into_owned()) - .unwrap_or_else(|| "session.json".to_string()); - path.with_file_name(format!( - ".{name}.{}.{}.tmp", - std::process::id(), - SEQ.fetch_add(1, Ordering::Relaxed) - )) + crate::atomic_write::write_atomic(path, serialize(state).as_bytes()) } /// Load and parse the session at `path`, or `None` if it is absent, unreadable, diff --git a/crates/noa-app/src/theme_favorites.rs b/crates/noa-app/src/theme_favorites.rs index 6b197e4f..223274ce 100644 --- a/crates/noa-app/src/theme_favorites.rs +++ b/crates/noa-app/src/theme_favorites.rs @@ -52,15 +52,14 @@ pub(crate) fn load(path: &Path) -> HashSet { } } -/// Atomically write `favorites` to `path` (temp file + rename), creating -/// the parent directory if needed — mirrors `session.rs::save`'s pattern. +/// Atomically write `favorites` to `path`, creating the parent directory if +/// needed. Uses the shared per-process/per-call staging file +/// ([`crate::atomic_write`]) rather than a fixed `favorites.tmp`: with the +/// fixed name two noa processes toggling at once shared one staging file, +/// and the loser could overwrite the already-published file through its +/// open descriptor and then report a failed save (N05). pub(crate) fn save(path: &Path, favorites: &HashSet) -> io::Result<()> { - if let Some(parent) = path.parent() { - fs::create_dir_all(parent)?; - } - let tmp = path.with_extension("tmp"); - fs::write(&tmp, serialize(favorites))?; - fs::rename(&tmp, path) + crate::atomic_write::write_atomic(path, serialize(favorites).as_bytes()) } /// `App`'s handle on the favorites store: lazily loaded (best-effort, empty diff --git a/crates/noa-config/src/writer.rs b/crates/noa-config/src/writer.rs index 4af0e1cb..0ed1db1b 100644 --- a/crates/noa-config/src/writer.rs +++ b/crates/noa-config/src/writer.rs @@ -212,6 +212,7 @@ pub fn write_config_updates(path: &Path, updates: &[(String, String)]) -> io::Re }; let updated = apply_updates(&original, updates); + let updated = repair_font_family_primaries(path, updated, updates); let target = resolve_symlinks(path)?; let parent = target.parent().ok_or_else(|| { @@ -249,6 +250,76 @@ pub fn write_config_updates(path: &Path, updates: &[(String, String)]) -> io::Re write_result } +/// [`apply_updates`] only sees the primary file, but the parser splices +/// `config-file` includes in at their directive's position and accumulates +/// every `font-family*` line into one ordered stack. A family contributed by +/// an include that precedes the rewritten slot — or by an include standing +/// in for a slot the primary file never had — therefore still heads the +/// effective list after a save, and the panel's "primary font" change is +/// silently ineffective (N04). Re-parse the would-be file with includes +/// expanded and, when the requested family did not land in front, rewrite +/// the key as a list reset followed by the new primary and every surviving +/// fallback (in effective order, deduplicated), all trailing the last +/// include so nothing can shadow them. +fn repair_font_family_primaries( + path: &Path, + mut updated: String, + updates: &[(String, String)], +) -> String { + for (key, value) in updates { + if !is_font_family_key(key) || value.is_empty() { + continue; + } + let (overrides, _) = crate::parse_overrides(path, &updated); + let families = font_families_for_key(&overrides.font, key); + if families.first() == Some(value) { + continue; + } + // The reset replaces the value line `apply_updates` just placed (the + // key's last occurrence) and, when an include follows it, is appended + // again after that include — so the primary + fallbacks appended + // below start from an empty list in every reload order. + updated = apply_updates(&updated, &[(key.clone(), String::new())]); + let mut lines = vec![(key.clone(), value.clone())]; + lines.extend( + families + .iter() + .filter(|family| *family != value) + .map(|family| (key.clone(), family.clone())), + ); + updated = append_directives(&updated, &lines); + } + updated +} + +fn font_families_for_key<'a>(font: &'a crate::FontConfig, key: &str) -> &'a [String] { + match key { + "font-family-bold" => &font.families_bold, + "font-family-italic" => &font.families_italic, + "font-family-bold-italic" => &font.families_bold_italic, + _ => &font.families, + } +} + +/// Append `lines` as `key = value` directives at the end of `text`, on the +/// file's dominant line terminator (same rule as [`apply_updates`]). +fn append_directives(text: &str, lines: &[(String, String)]) -> String { + let existing = split_lines_preserving_terminators(text); + let terminator = dominant_terminator(&existing); + let mut output = text.to_string(); + if existing + .last() + .is_some_and(|(_, terminator)| terminator.is_empty()) + { + output.push_str(terminator); + } + for (key, value) in lines { + output.push_str(&format!("{key} = {value}")); + output.push_str(terminator); + } + output +} + /// Upper bound on symlink hops before `resolve_symlinks` reports a cycle /// (Linux's `MAXSYMLINKS` — `fs::canonicalize` would fail at the same depth). const MAX_SYMLINK_HOPS: usize = 40; @@ -490,6 +561,60 @@ theme = 3024 Day\r fs::remove_dir_all(&dir).unwrap(); } + // N04: a family that only an include provides must not stay primary + // after the panel saves a different one — with the include either + // standing in for a missing slot or preceding the primary file's own. + #[test] + fn font_saves_beat_families_contributed_by_includes() { + for (label, main) in [ + ("include-only", "config-file = fonts.conf\n"), + ( + "include-before-slot", + "config-file = fonts.conf\nfont-family = Menlo\n", + ), + ] { + let dir = unique_temp_dir(&format!("font-include-{label}")); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("config"); + fs::write( + dir.join("fonts.conf"), + "font-family = Monaco\nfont-family = Courier\n", + ) + .unwrap(); + fs::write(&path, main).unwrap(); + + for family in ["Fira Code", "Hack"] { + write_config_updates(&path, &[("font-family".into(), family.into())]).unwrap(); + let source = fs::read_to_string(&path).unwrap(); + let (overrides, diagnostics) = crate::parse_overrides(&path, &source); + assert!(diagnostics.is_empty(), "{label}: {diagnostics:?}"); + assert_eq!( + overrides.font.families.first().map(String::as_str), + Some(family), + "{label}: the saved family heads the effective list" + ); + assert!( + overrides.font.families.contains(&"Monaco".to_string()) + && overrides.font.families.contains(&"Courier".to_string()), + "{label}: the include's families survive as fallbacks: {:?}", + overrides.font.families + ); + assert_eq!( + overrides.font.families.len(), + 3, + "{label}: no duplicate or stale entries: {:?}", + overrides.font.families + ); + } + // Untouched by the writer: the include file itself. + assert_eq!( + fs::read_to_string(dir.join("fonts.conf")).unwrap(), + "font-family = Monaco\nfont-family = Courier\n" + ); + fs::remove_dir_all(&dir).unwrap(); + } + } + #[test] fn font_family_reset_clears_trailing_includes_after_reload() { let dir = unique_temp_dir("font-reset-include"); diff --git a/crates/noa-grid/src/screen/edit.rs b/crates/noa-grid/src/screen/edit.rs index 64ed19b5..005e45a5 100644 --- a/crates/noa-grid/src/screen/edit.rs +++ b/crates/noa-grid/src/screen/edit.rs @@ -5,13 +5,36 @@ use super::*; impl Screen { pub(crate) fn clear_scrollback(&mut self) { - if self.scrollback.len() > 0 { + self.collapse_scrollback(); + self.clear_selection(); + self.clear_search(); + } + + /// Drop the scrollback and collapse the session-absolute coordinate + /// space by its length: Kitty placements anchored in the cleared history + /// go, survivors in the live area re-anchor into the shrunken space so + /// they keep their screen position. Shared by [`Self::clear_scrollback`] + /// (the app's "Clear Scrollback" action) and `ED 3`, which used to be two + /// diverging copies — the former left every anchor `old_len` rows too + /// high and pushed visible images off-screen (N06). Returns the number + /// of rows removed, for callers that also shift a selection. + fn collapse_scrollback(&mut self) -> usize { + let old_len = self.scrollback.len(); + let old_live_top = self.live_area_abs_top(); + if old_len > 0 { self.invalidate_coordinate_space(); } self.scrollback.clear(); self.viewport_offset = 0; - self.clear_selection(); - self.clear_search(); + self.kitty_placements.retain_mut(|p| { + if p.anchor_abs_row < old_live_top { + false + } else { + p.anchor_abs_row -= old_len; + true + } + }); + old_len } /// Ghostty parity: the non-prompt branch of `Termio.clearScreen` — erase @@ -520,10 +543,15 @@ impl Screen { self.cursor.x = self.cursor.x.saturating_sub(n).max(self.left_margin()); } + /// Absolute cursor placement (`CUP`/`HVP` with DECOM off). Ghostty's + /// `setCursorPos` bounds the target by the *screen* unless origin mode is + /// on — the left/right margins only constrain the origin-relative form + /// ([`Self::cursor_position_with_origin`]) — so a TUI that draws outside + /// a DECSLRM window lands where it asked, not snapped to the margin (N07). pub fn cursor_position(&mut self, row: u16, col: u16) { self.cursor.pending_wrap = false; self.cursor.y = row.saturating_sub(1).min(self.rows.saturating_sub(1)); - self.cursor.x = self.clamp_x_to_margins(col.saturating_sub(1)); + self.cursor.x = col.saturating_sub(1).min(self.cols.saturating_sub(1)); } pub(crate) fn cursor_position_with_origin(&mut self, row: u16, col: u16, origin: bool) { @@ -543,9 +571,10 @@ impl Screen { .min(self.right_margin()); } + /// `CHA`/`HPA` with DECOM off: screen-bounded like [`Self::cursor_position`]. pub fn cursor_col_abs(&mut self, col: u16) { self.cursor.pending_wrap = false; - self.cursor.x = self.clamp_x_to_margins(col.saturating_sub(1)); + self.cursor.x = col.saturating_sub(1).min(self.cols.saturating_sub(1)); } pub(crate) fn cursor_col_abs_with_origin(&mut self, col: u16, origin: bool) { @@ -578,10 +607,12 @@ impl Screen { .min(self.region.bottom); } + /// Home is the top-left of the scrolling region under DECOM and of the + /// screen otherwise (DECSLRM/DECSTBM/DECOM all home through here). pub(crate) fn home_cursor(&mut self, origin: bool) { self.cursor.pending_wrap = false; self.cursor.y = if origin { self.region.top } else { 0 }; - self.cursor.x = self.left_margin(); + self.cursor.x = if origin { self.left_margin() } else { 0 }; } pub(crate) fn reported_cursor_position(&self, origin: bool) -> (u16, u16) { @@ -716,24 +747,7 @@ impl Screen { self.remove_placements_intersecting_grid_rows(0, last); } EraseDisplay::Scrollback => { - // Clearing scrollback collapses the absolute coordinate space by - // its length: drop placements anchored in the cleared history and - // re-anchor the survivors (live area) into the shrunken space. - let old_len = self.scrollback.len(); - let old_live_top = self.live_area_abs_top(); - if old_len > 0 { - self.invalidate_coordinate_space(); - } - self.scrollback.clear(); - self.viewport_offset = 0; - self.kitty_placements.retain_mut(|p| { - if p.anchor_abs_row < old_live_top { - false - } else { - p.anchor_abs_row -= old_len; - true - } - }); + let old_len = self.collapse_scrollback(); // Same collapse for the selection: a live-area selection // shifts with its rows, one touching cleared history is gone. self.selection = self diff --git a/crates/noa-grid/src/tests/kitty_graphics.rs b/crates/noa-grid/src/tests/kitty_graphics.rs index 2eec4a63..b56dd0c9 100644 --- a/crates/noa-grid/src/tests/kitty_graphics.rs +++ b/crates/noa-grid/src/tests/kitty_graphics.rs @@ -732,3 +732,48 @@ fn kitty_rectangle_scroll_keeps_placements_outside_the_margins() { ); assert_eq!(t.kitty_visible_placements()[0].grid_y, 5); } + +// N06: the app's "Clear Scrollback" (`Terminal::clear_scrollback`, not +// `ED 3`) must collapse placement anchors with the history it removes, or a +// live-area image jumps `scrollback_len` rows down and off-screen. +#[test] +fn kitty_clear_scrollback_reanchors_live_placements_and_drops_history_ones() { + let mut t = kitty_terminal(); + // One image on the first row, then push it into history with a screenful + // of newlines plus a few more so the scrollback is non-empty. + feed( + &mut t, + &kitty_apc("a=T,f=32,s=10,v=20,i=1,C=1", &vec![0u8; 10 * 20 * 4]), + ); + feed(&mut t, b"\x1b[24;1H"); + feed(&mut t, &[b'\n'; 30]); + assert!( + t.scrollback_len() >= 30, + "the first image is in history now" + ); + // A second image on live row 2. + feed(&mut t, b"\x1b[3;1H"); + feed( + &mut t, + &kitty_apc("a=T,f=32,s=10,v=20,i=2,C=1", &vec![0u8; 10 * 20 * 4]), + ); + let before = t.kitty_visible_placements(); + assert_eq!(before.len(), 1); + assert_eq!((before[0].image_id, before[0].grid_y), (2, 2)); + + t.clear_scrollback(); + + assert_eq!(t.scrollback_len(), 0); + assert_eq!( + t.primary.kitty_placements.len(), + 1, + "the history-anchored image is gone with the history" + ); + let after = t.kitty_visible_placements(); + assert_eq!(after.len(), 1, "the live image is still visible"); + assert_eq!( + (after[0].image_id, after[0].grid_y), + (2, 2), + "the live image keeps its screen row" + ); +} diff --git a/crates/noa-grid/src/tests/terminal_state.rs b/crates/noa-grid/src/tests/terminal_state.rs index 8541cfff..e169f90f 100644 --- a/crates/noa-grid/src/tests/terminal_state.rs +++ b/crates/noa-grid/src/tests/terminal_state.rs @@ -170,15 +170,19 @@ fn decslrm_requires_left_right_margin_mode() { } #[test] -fn decslrm_sets_horizontal_margins_and_homes_to_left_margin() { +fn decslrm_sets_horizontal_margins_and_homes_the_cursor() { + // xterm/Ghostty: DECSLRM homes to the *screen's* top-left; only DECOM + // makes home margin-relative (N07). let t = run_size(10, 5, b"\x1b[?69h\x1b[3;7s"); assert_eq!( t.primary.horizontal_margins, Some(HorizontalMargins { left: 2, right: 6 }) ); - assert_eq!(t.primary.cursor.x, 2); - assert_eq!(t.primary.cursor.y, 0); + assert_eq!((t.primary.cursor.x, t.primary.cursor.y), (0, 0)); + + let t = run_size(10, 5, b"\x1b[?69h\x1b[?6h\x1b[3;7s"); + assert_eq!((t.primary.cursor.x, t.primary.cursor.y), (2, 0)); } #[test] @@ -195,7 +199,7 @@ fn horizontal_margins_clamp_cursor_motion_and_carriage_return() { #[test] fn horizontal_margins_wrap_printing_to_left_margin() { - let t = run_size(10, 5, b"\x1b[?69h\x1b[3;7sabcdeZ"); + let t = run_size(10, 5, b"\x1b[?69h\x1b[3;7s\x1b[3GabcdeZ"); assert_eq!(row_text(&t, 0, 8), " abcde "); assert_eq!(cell(&t, 2, 1).ch, 'Z'); @@ -1335,3 +1339,34 @@ fn ris_keeps_the_configured_default_cursor_style() { s.feed(b"\x1bc", &mut t); assert_eq!(t.primary.cursor.style, CursorStyle::SteadyBar); } + +// N07: with DECOM off, absolute cursor placement (CUP/CHA/HPA and the home +// DECSLRM performs) is bounded by the screen, not the left/right margins — +// Ghostty's `setCursorPos` only offsets/clamps to the margins in origin mode. +#[test] +fn absolute_cursor_placement_ignores_horizontal_margins_without_origin_mode() { + // DECLRMM on, margins at columns 10..20 (0-based 9..19). + let t = run(b"\x1b[?69h\x1b[10;20s\x1b[1;1H"); + assert_eq!((t.primary.cursor.x, t.primary.cursor.y), (0, 0)); + let t = run(b"\x1b[?69h\x1b[10;20s\x1b[3;5H"); + assert_eq!((t.primary.cursor.x, t.primary.cursor.y), (4, 2)); + let t = run(b"\x1b[?69h\x1b[10;20s\x1b[30G"); + assert_eq!(t.primary.cursor.x, 29); + let t = run(b"\x1b[?69h\x1b[10;20s\x1b[30`"); + assert_eq!(t.primary.cursor.x, 29); + // DECSLRM itself homes to the screen's top-left when origin mode is off. + let t = run(b"\x1b[?69h\x1b[5;5H\x1b[10;20s"); + assert_eq!((t.primary.cursor.x, t.primary.cursor.y), (0, 0)); +} + +#[test] +fn absolute_cursor_placement_is_margin_relative_in_origin_mode() { + let t = run(b"\x1b[?69h\x1b[10;20s\x1b[?6h\x1b[1;1H"); + assert_eq!((t.primary.cursor.x, t.primary.cursor.y), (9, 0)); + let t = run(b"\x1b[?69h\x1b[10;20s\x1b[?6h\x1b[1;30H"); + assert_eq!(t.primary.cursor.x, 19, "clamped to the right margin"); + let t = run(b"\x1b[?69h\x1b[10;20s\x1b[?6h\x1b[5G"); + assert_eq!(t.primary.cursor.x, 13); + let t = run(b"\x1b[?69h\x1b[?6h\x1b[5;5H\x1b[10;20s"); + assert_eq!((t.primary.cursor.x, t.primary.cursor.y), (9, 0)); +} From 17c581893462ccd6fee8f6f4f01aca0a7df510b6 Mon Sep 17 00:00:00 2001 From: simota Date: Thu, 10 Sep 2026 13:20:37 +0900 Subject: [PATCH 2/3] fix: correct margin, mouse, keyboard, and IME regressions --- crates/noa-app/src/app/event_loop.rs | 37 ++++--- crates/noa-app/src/app/input_ops/ime.rs | 6 +- crates/noa-app/src/app/input_ops/pointer.rs | 22 +++++ crates/noa-app/src/app/lifecycle.rs | 1 + crates/noa-app/src/app/quick_terminal.rs | 1 + crates/noa-app/src/app/render.rs | 17 +--- crates/noa-app/src/app/scratch_terminal.rs | 1 + crates/noa-app/src/app/state.rs | 1 + crates/noa-app/src/input.rs | 2 +- crates/noa-app/src/input/key.rs | 33 ++++++- crates/noa-app/src/input/kitty.rs | 4 +- crates/noa-app/src/input/tests.rs | 63 ++++++++++++ .../noa-app/src/macos_overlay/imp/appkit.rs | 58 +++++------ crates/noa-app/src/macos_overlay/model.rs | 96 +++++++++++++++++-- crates/noa-app/src/macos_overlay/tests.rs | 42 +++++++- crates/noa-grid/src/screen/edit.rs | 19 ++-- crates/noa-grid/src/screen/print.rs | 55 +++++++---- crates/noa-grid/src/tests/bulk_print.rs | 17 +++- crates/noa-grid/src/tests/terminal_state.rs | 59 ++++++++++++ 19 files changed, 429 insertions(+), 105 deletions(-) diff --git a/crates/noa-app/src/app/event_loop.rs b/crates/noa-app/src/app/event_loop.rs index a2016969..68f5e9eb 100644 --- a/crates/noa-app/src/app/event_loop.rs +++ b/crates/noa-app/src/app/event_loop.rs @@ -687,6 +687,7 @@ impl ApplicationHandler for App { // modifier press, so nothing is lost. if let Some(state) = self.windows.get_mut(&window_id) { state.modifiers = ModifiersState::empty(); + state.key_modifiers.clear(); } self.end_copy_mode_for_window(window_id); // Only clear if this window is the one we recorded as focused — @@ -861,6 +862,22 @@ impl ApplicationHandler for App { WindowEvent::Ime(event) => self.on_ime_event(window_id, event), WindowEvent::KeyboardInput { event, .. } => { let pressed = event.state == ElementState::Pressed; + // Pair before any modal/shortcut early return so every + // release also retires the physical key's classification. + let event_alt_sends_esc = !cfg!(target_os = "macos") + || event.text.as_deref().is_none_or(str::is_empty) + || event.text.as_deref() != event.text_with_all_modifiers(); + let alt_sends_esc = + self.windows + .get_mut(&window_id) + .map_or(event_alt_sends_esc, |state| { + state.key_modifiers.alt_sends_esc( + event.physical_key, + pressed, + event.repeat, + event_alt_sends_esc, + ) + }); if pressed { // NOA_LATENCY_TRACE t0: winit key-event receipt, before // any routing — this is the earliest app-side timestamp @@ -1114,13 +1131,6 @@ impl ApplicationHandler for App { let (app_cursor_keys, app_keypad, kitty_flags, modify_other_keys) = self.key_encode_modes(window_id); let unmodified_key = event.key_without_modifiers(); - // On macOS, Option only acts as Alt when winit stripped its - // composition per `macos-option-as-alt` — i.e. the delivered - // text differs from the text with every modifier applied. - // Otherwise the composed character must pass through with no - // ESC prefix. - let alt_sends_esc = !cfg!(target_os = "macos") - || event.text.as_deref() != event.text_with_all_modifiers(); let bytes = input::encode_key_with_modes( &event.logical_key, Some(&unmodified_key), @@ -1512,6 +1522,7 @@ impl App { pub(super) fn on_cursor_left(&mut self, window_id: WindowId) { if let Some(state) = self.windows.get_mut(&window_id) { state.last_mouse_point = None; + state.last_mouse_physical_position = None; // A captured gesture survives the pointer leaving the window: the // release still arrives here (macOS delivers mouse-up to the // window that saw mouse-down) and must find the pane and its last @@ -1714,12 +1725,10 @@ impl App { // pressed/drag state instead of handing B a release it never saw the // press for. if button == MouseButton::Left + && state == ElementState::Pressed && let Some(tab) = self.windows.get_mut(&window_id) { - tab.mouse_capture_pane = match state { - ElementState::Pressed => Some(pane_id), - ElementState::Released => None, - }; + tab.mouse_capture_pane = Some(pane_id); } if button == MouseButton::Left && state == ElementState::Pressed { @@ -1760,6 +1769,9 @@ impl App { } } } + if button == MouseButton::Left && state == ElementState::Released { + self.release_mouse_capture(window_id); + } return; } @@ -1800,6 +1812,9 @@ impl App { }) .unwrap_or(SelectionGesture::None); self.apply_selection_gesture(window_id, pane_id, gesture); + if state == ElementState::Released { + self.release_mouse_capture(window_id); + } } pub(super) fn on_mouse_wheel(&mut self, window_id: WindowId, delta: MouseScrollDelta) { diff --git a/crates/noa-app/src/app/input_ops/ime.rs b/crates/noa-app/src/app/input_ops/ime.rs index 1f21965b..696ac00f 100644 --- a/crates/noa-app/src/app/input_ops/ime.rs +++ b/crates/noa-app/src/app/input_ops/ime.rs @@ -217,7 +217,11 @@ impl App { ))) } ModalImeTarget::ThemeSettings => { - Some(caret_px(crate::macos_overlay::theme_settings_caret(pane))) + let session = self.theme_settings.as_ref()?; + Some(caret_px(crate::macos_overlay::theme_settings_caret( + pane, + &session.state, + ))) } ModalImeTarget::SidebarRename => { // The renamed card's name row, from the same layout the diff --git a/crates/noa-app/src/app/input_ops/pointer.rs b/crates/noa-app/src/app/input_ops/pointer.rs index d942c865..69a9360e 100644 --- a/crates/noa-app/src/app/input_ops/pointer.rs +++ b/crates/noa-app/src/app/input_ops/pointer.rs @@ -188,6 +188,27 @@ impl App { )) } + /// Re-hit-test the physical pointer after delivering mouse-up to the + /// captured pane. This updates routing without synthesizing mouse motion. + pub(in crate::app) fn release_mouse_capture(&mut self, window_id: WindowId) { + let hovered = (|| { + let state = self.windows.get(&window_id)?; + let position = state.last_mouse_physical_position?; + let metrics = self.gpu.as_ref()?.fonts.get(state.font_px).metrics(); + self.pane_cell_at_position(window_id, position, metrics) + })(); + if let Some(state) = self.windows.get_mut(&window_id) { + state.mouse_capture_pane = None; + state.last_mouse_pane = hovered.map(|(pane_id, _)| pane_id); + for (pane_id, surface) in &mut state.surfaces { + surface.last_mouse_cell = hovered + .filter(|(hovered_pane, _)| hovered_pane == pane_id) + .map(|(_, cell)| cell); + } + } + self.sync_hover_link(window_id); + } + /// Abandon the live left-button capture in `window_id` (focus loss): the /// capturing pane's pressed-button and selection-drag state are cleared /// as if its release had arrived, so a later motion can't keep extending @@ -203,6 +224,7 @@ impl App { surface.pressed_mouse_button = None; let _ = surface.mouse_selection.left_released(); } + self.release_mouse_capture(window_id); } /// The Cmd+hover link under the mouse in `window_id`'s focused-under- diff --git a/crates/noa-app/src/app/lifecycle.rs b/crates/noa-app/src/app/lifecycle.rs index 8294c9f4..e931a085 100644 --- a/crates/noa-app/src/app/lifecycle.rs +++ b/crates/noa-app/src/app/lifecycle.rs @@ -613,6 +613,7 @@ impl App { last_mouse_physical_position: None, active_split_drag: None, modifiers: ModifiersState::empty(), + key_modifiers: input::KeyModifierState::default(), occluded: false, title: "Noa".to_string(), proxy_icon_cwd: None, diff --git a/crates/noa-app/src/app/quick_terminal.rs b/crates/noa-app/src/app/quick_terminal.rs index 0c250244..64603617 100644 --- a/crates/noa-app/src/app/quick_terminal.rs +++ b/crates/noa-app/src/app/quick_terminal.rs @@ -826,6 +826,7 @@ impl App { last_mouse_physical_position: None, active_split_drag: None, modifiers: ModifiersState::empty(), + key_modifiers: input::KeyModifierState::default(), occluded: false, title: "Noa".to_string(), title_override: None, diff --git a/crates/noa-app/src/app/render.rs b/crates/noa-app/src/app/render.rs index 23b5ecbe..524c979c 100644 --- a/crates/noa-app/src/app/render.rs +++ b/crates/noa-app/src/app/render.rs @@ -459,6 +459,9 @@ impl App { // early-return below, so a background tab tracks its shell instead of // freezing at its last-foreground title (tab-close title-freeze fix). self.refresh_window_title(window_id); + // Redraws and IME events share the modal-aware anchor, including + // frame requests caused by preedit or a card layout change. + self.update_focused_ime_cursor_area(window_id); #[cfg(target_os = "macos")] let has_visible_background_image = self.background_image.has_visible_image(); let (Some(gpu), Some(state)) = (self.gpu.as_mut(), self.windows.get_mut(&window_id)) else { @@ -738,20 +741,6 @@ impl App { crate::macos_window::set_represented_url(&state.window, resolved.as_deref()); state.proxy_icon_cwd = new_cwd; } - if let Some((_, rect, snapshot)) = snapshots - .iter() - .find(|(pane_id, _, _)| *pane_id == state.focused_pane) - { - update_ime_cursor_area( - &state.window, - gpu.fonts.get(state.font_px).metrics(), - snapshot.cursor.x, - snapshot.cursor.y, - *rect, - self.padding, - ); - } - let panes = snapshots .iter() .map(|(pane_id, rect, snapshot)| PaneFrame { diff --git a/crates/noa-app/src/app/scratch_terminal.rs b/crates/noa-app/src/app/scratch_terminal.rs index d0e0aa9e..693c00da 100644 --- a/crates/noa-app/src/app/scratch_terminal.rs +++ b/crates/noa-app/src/app/scratch_terminal.rs @@ -406,6 +406,7 @@ impl App { last_mouse_physical_position: None, active_split_drag: None, modifiers: ModifiersState::empty(), + key_modifiers: input::KeyModifierState::default(), occluded: false, title: "Scratch Terminal".to_string(), title_override: None, diff --git a/crates/noa-app/src/app/state.rs b/crates/noa-app/src/app/state.rs index c422a10c..bb084d88 100644 --- a/crates/noa-app/src/app/state.rs +++ b/crates/noa-app/src/app/state.rs @@ -284,6 +284,7 @@ pub(super) struct WindowState { pub(super) active_split_drag: Option, /// Modifier state tracked by winit for this native window's view. pub(super) modifiers: ModifiersState, + pub(super) key_modifiers: input::KeyModifierState, pub(super) occluded: bool, /// Whether this window was *created* with `with_transparent(true)`. /// AppKit fixes a window's opacity at creation — a window built opaque diff --git a/crates/noa-app/src/input.rs b/crates/noa-app/src/input.rs index 3578f4d7..82aa348d 100644 --- a/crates/noa-app/src/input.rs +++ b/crates/noa-app/src/input.rs @@ -14,7 +14,7 @@ mod text; pub(crate) use ime::ImeState; #[cfg(test)] use key::encode_key; -pub(crate) use key::{encode_enter_key, encode_key_with_modes}; +pub(crate) use key::{KeyModifierState, encode_enter_key, encode_key_with_modes}; pub use paste::encode_paste; pub(crate) use paste::{applescript_input_bytes, paste_is_unsafe, raw_input_bytes}; #[cfg(test)] diff --git a/crates/noa-app/src/input/key.rs b/crates/noa-app/src/input/key.rs index ad806151..6cfe00f4 100644 --- a/crates/noa-app/src/input/key.rs +++ b/crates/noa-app/src/input/key.rs @@ -3,6 +3,35 @@ use winit::keyboard::{Key, KeyCode, ModifiersState, NamedKey, PhysicalKey}; use super::kitty::{KittyOutcome, encode_kitty}; use super::text::encode_text; +/// The Option/Alt classification belongs to a physical press, including its +/// repeats and release: winit's release events carry no composed text. +#[derive(Default)] +pub(crate) struct KeyModifierState { + alt_by_key: std::collections::HashMap, +} + +impl KeyModifierState { + pub(crate) fn alt_sends_esc( + &mut self, + key: PhysicalKey, + pressed: bool, + repeat: bool, + event_alt_sends_esc: bool, + ) -> bool { + if !pressed { + return self.alt_by_key.remove(&key).unwrap_or(event_alt_sends_esc); + } + if !repeat { + self.alt_by_key.insert(key, event_alt_sends_esc); + } + *self.alt_by_key.entry(key).or_insert(event_alt_sends_esc) + } + + pub(crate) fn clear(&mut self) { + self.alt_by_key.clear(); + } +} + /// Encode a pressed key into the bytes that should be written to the pty, if /// any. `app_cursor_keys` mirrors `ModeState::app_cursor_keys()` (DECCKM): /// when set, arrow keys send `SS3` (`ESC O `) instead of `CSI` @@ -47,8 +76,8 @@ pub fn encode_key( /// /// `alt_sends_esc` says whether Alt held with this press should ESC-prefix the /// produced text. On macOS the Option key composes characters unless -/// `macos-option-as-alt` claims it, so the caller decides per event; on other -/// platforms it is simply `true`. +/// `macos-option-as-alt` claims it, so the caller retains the press's verdict +/// through repeats and release; on other platforms it is simply `true`. #[allow(clippy::too_many_arguments)] pub fn encode_key_with_modes( logical_key: &Key, diff --git a/crates/noa-app/src/input/kitty.rs b/crates/noa-app/src/input/kitty.rs index 18f76544..1f2b11c9 100644 --- a/crates/noa-app/src/input/kitty.rs +++ b/crates/noa-app/src/input/kitty.rs @@ -44,7 +44,7 @@ fn kitty_modifier_value(mods: ModifiersState) -> u32 { value } -/// `alt_sends_esc` is the caller's per-event verdict on whether a held Alt +/// `alt_sends_esc` is the caller's press-paired verdict on whether a held Alt /// is *Alt* or a macOS Option that composed `text` (see /// `encode_key_with_modes`). A composing Option is not a modifier for the /// "does this key escape-encode" decision (Ghostty's `effectiveMods`): the @@ -82,7 +82,7 @@ pub(super) fn encode_kitty( }; let mods_value = kitty_modifier_value(mods); - let alt_is_modifier = mods.alt_key() && (alt_sends_esc || text.is_none_or(str::is_empty)); + let alt_is_modifier = mods.alt_key() && alt_sends_esc; let has_non_shift = mods.control_key() || alt_is_modifier || mods.super_key(); // Physical keypad keys carry dedicated code points: Ghostty's kitty diff --git a/crates/noa-app/src/input/tests.rs b/crates/noa-app/src/input/tests.rs index 330a30f8..540cd69a 100644 --- a/crates/noa-app/src/input/tests.rs +++ b/crates/noa-app/src/input/tests.rs @@ -1615,6 +1615,69 @@ fn kitty_composing_option_is_not_alt() { press(KITTY_REPORT_ALL_KEYS | KITTY_REPORT_ASSOCIATED_TEXT, true), Some(b"\x1b[97;3u".to_vec()) ); + // winit omits text on release; it must retain the press's composition + // classification so a legacy text press has no unpaired Kitty release. + assert_eq!( + encode_key_with_modes( + &Key::Character("å".into()), + Some(&Key::Character("a".into())), + Some(PhysicalKey::Code(KeyCode::KeyA)), + None, + ModifiersState::ALT, + false, + false, + false, + KITTY_DISAMBIGUATE | KITTY_REPORT_EVENT_TYPES, + false, + false, + false, + ), + None + ); +} + +#[test] +fn option_classification_survives_repeat_and_release_per_physical_key() { + let mut state = KeyModifierState::default(); + let composed = PhysicalKey::Code(KeyCode::KeyA); + let alt = PhysicalKey::Code(KeyCode::KeyB); + assert!(!state.alt_sends_esc(composed, true, false, false)); + assert!(state.alt_sends_esc(alt, true, false, true)); + // Missing/different repeat text must not reclassify the held key. + assert!(!state.alt_sends_esc(composed, true, true, true)); + let flags = KITTY_DISAMBIGUATE | KITTY_REPORT_EVENT_TYPES; + let release = |state: &mut KeyModifierState, physical, flags| { + encode_key_with_modes( + &Key::Character("a".into()), + None, + Some(physical), + None, + ModifiersState::ALT, + state.alt_sends_esc(physical, false, false, true), + false, + false, + flags, + false, + false, + false, + ) + }; + assert_eq!(release(&mut state, composed, flags), None); + assert_eq!( + release(&mut state, alt, flags), + Some(b"\x1b[97;3:3u".to_vec()) + ); + // Report-all still pairs its encoded press with an encoded release. + assert!(!state.alt_sends_esc(composed, true, false, false)); + assert_eq!( + release(&mut state, composed, flags | KITTY_REPORT_ALL_KEYS), + Some(b"\x1b[97;3:3u".to_vec()) + ); + assert!(!state.alt_sends_esc(composed, true, false, false)); + state.clear(); + assert!(state.alt_sends_esc(composed, false, false, true)); + // Reusing the same key starts a fresh classification. + assert!(state.alt_sends_esc(composed, true, false, true)); } // N11: the keypad Enter is a distinct Kitty key (57414), reachable even diff --git a/crates/noa-app/src/macos_overlay/imp/appkit.rs b/crates/noa-app/src/macos_overlay/imp/appkit.rs index a8a6d2ac..f4152340 100644 --- a/crates/noa-app/src/macos_overlay/imp/appkit.rs +++ b/crates/noa-app/src/macos_overlay/imp/appkit.rs @@ -1,6 +1,7 @@ use crate::macos_overlay::model::{ - OverlayColors, PaneRectPt, ProcessMonitorViewModel, ThemeSettingsViewModel, Tone, - overlay_scroll_window, + OverlayColors, PaneRectPt, ProcessMonitorViewModel, SETTINGS_DESCRIPTION_H, SETTINGS_FOOTER_H, + SETTINGS_ROW_H, SETTINGS_SEARCH_H, SETTINGS_TOP, THEME_FILTER_TOP, THEME_LIST_TOP, THEME_ROW_H, + ThemeSettingsLayout, ThemeSettingsViewModel, Tone, overlay_scroll_window, }; use crate::theme_settings::{Liveness, ThemeSettingsMode}; use noa_render::{CommandPaletteSnapshot, ConfirmDialogSnapshot, PaletteRow}; @@ -880,46 +881,31 @@ pub(in crate::macos_overlay) fn rebuild_theme_settings( // Question, resolved differently per path since one has a dynamic // card height and the other doesn't). Settings mode is unaffected // — it uses its own independent `settings_top`, not this constant. - let list_top = 106.0; + let list_top = THEME_LIST_TOP; let chip_row_y = 84.0; - let row_h = 24.0; - let srow_h = 23.0; - let settings_header_h = 20.0; - let footer_h = 34.0; - let avail = (pane.h - 24.0).max(240.0); + let row_h = THEME_ROW_H; + let srow_h = SETTINGS_ROW_H; + let footer_h = SETTINGS_FOOTER_H; let settings_total = vm.settings_visible.len(); // Settings mode has no filter line/theme list above it, so its // section header sits where the "Theme"/"Sample" column headers // otherwise would (y=46); rows start directly below the header. - let settings_top = 46.0 + settings_header_h; + let settings_top = SETTINGS_TOP; // R-6/R-5 fixed lines (Addendum D-3/FM-04): always reserve the // description line; reserve the search line only while active. - let description_h = 19.0; - let search_h = 16.0; - - let (list_rows, settings_rows, card_h) = match vm.mode { - ThemeSettingsMode::Theme => { - let needed = |list_rows: usize| list_top + list_rows as f64 * row_h + footer_h; - let mut list_rows = vm.themes.len().max(1); - while needed(list_rows) > avail && list_rows > 3 { - list_rows -= 1; - } - (list_rows, 0usize, needed(list_rows).min(avail)) - } - ThemeSettingsMode::Settings => { - let (settings_rows, height) = crate::macos_overlay::model::settings_rows_budget( - settings_total, - avail, - settings_top, - srow_h, - footer_h, - description_h, - search_h, - vm.search_active, - ); - (0usize, settings_rows, height) - } - }; + let description_h = SETTINGS_DESCRIPTION_H; + let search_h = SETTINGS_SEARCH_H; + let ThemeSettingsLayout { + list_rows, + settings_rows, + card_h, + } = ThemeSettingsLayout::new( + pane.h, + vm.mode, + vm.themes.len(), + settings_total, + vm.search_active, + ); let card_frame = NSRect::new( NSPoint::new( (pane.w - card_w) / 2.0, @@ -1019,7 +1005,7 @@ pub(in crate::macos_overlay) fn rebuild_theme_settings( mono_digit_font(12.0), muted, NSRect::new( - NSPoint::new(pad, from_top(card_h, 64.0, 16.0)), + NSPoint::new(pad, from_top(card_h, THEME_FILTER_TOP, SETTINGS_SEARCH_H)), NSSize::new(col_split - pad - 8.0 - count_w, 16.0), ), ); diff --git a/crates/noa-app/src/macos_overlay/model.rs b/crates/noa-app/src/macos_overlay/model.rs index 62a497ed..8faea6bd 100644 --- a/crates/noa-app/src/macos_overlay/model.rs +++ b/crates/noa-app/src/macos_overlay/model.rs @@ -127,21 +127,37 @@ pub(crate) fn title_prompt_caret(pane: PaneRectPt, input_chars: usize) -> CaretP } } -/// The theme-settings card's search/filter field, at the card's upper-left. -/// The card's height depends on its row lists; the vertical bound it is -/// capped at (`pane.h - 24`, at least 240) is used, which is exact whenever -/// the catalogue fills the card (the common case) and a few rows high -/// otherwise. -pub(crate) fn theme_settings_caret(pane: PaneRectPt) -> CaretPt { +/// The search/filter field uses the same content-driven card height as the +/// native builder, including filtered/empty lists and the Settings mode. +pub(crate) fn theme_settings_caret( + pane: PaneRectPt, + state: &crate::theme_settings::ThemeSettings, +) -> CaretPt { + use crate::theme_settings::{SettingsRowKind, ThemeSettingsMode}; let card_w = THEME_SETTINGS_WIDTH.min(pane.w - 32.0).max(320.0); - let card_h = (pane.h - 24.0).max(240.0); + let settings_total = if state.settings_search_active() { + state.settings_filtered_len() + } else { + SettingsRowKind::COUNT + }; + let layout = ThemeSettingsLayout::new( + pane.h, + state.mode(), + state.filtered_len().min(THEME_LIST_ROWS), + settings_total, + state.settings_search_active(), + ); let card_x = (pane.w - card_w) / 2.0; - let card_top = (pane.h - card_h) / 2.0; + let card_top = (pane.h - layout.card_h) / 2.0; + let input_top = match state.mode() { + ThemeSettingsMode::Theme => THEME_FILTER_TOP, + ThemeSettingsMode::Settings => SETTINGS_TOP, + }; CaretPt { x: card_x + 20.0, - y: card_top + 46.0, + y: card_top + input_top, w: 1.0, - h: INPUT_ROW_H, + h: SETTINGS_SEARCH_H, } } @@ -367,6 +383,66 @@ pub(crate) struct ThemeSettingsViewModel { /// Rows the theme list shows at once in the native card. const THEME_LIST_ROWS: usize = 8; +pub(crate) const THEME_FILTER_TOP: f64 = 64.0; +pub(crate) const THEME_LIST_TOP: f64 = 106.0; +pub(crate) const THEME_ROW_H: f64 = 24.0; +pub(crate) const SETTINGS_TOP: f64 = 66.0; +pub(crate) const SETTINGS_ROW_H: f64 = 23.0; +pub(crate) const SETTINGS_FOOTER_H: f64 = 34.0; +pub(crate) const SETTINGS_DESCRIPTION_H: f64 = 19.0; +pub(crate) const SETTINGS_SEARCH_H: f64 = 16.0; + +pub(crate) struct ThemeSettingsLayout { + pub(crate) list_rows: usize, + pub(crate) settings_rows: usize, + pub(crate) card_h: f64, +} + +impl ThemeSettingsLayout { + pub(crate) fn new( + pane_h: f64, + mode: crate::theme_settings::ThemeSettingsMode, + theme_rows: usize, + settings_total: usize, + search_active: bool, + ) -> Self { + use crate::theme_settings::ThemeSettingsMode; + let avail = (pane_h - 24.0).max(240.0); + match mode { + ThemeSettingsMode::Theme => { + let needed = + |rows: usize| THEME_LIST_TOP + rows as f64 * THEME_ROW_H + SETTINGS_FOOTER_H; + let mut list_rows = theme_rows.max(1); + while needed(list_rows) > avail && list_rows > 3 { + list_rows -= 1; + } + Self { + list_rows, + settings_rows: 0, + card_h: needed(list_rows).min(avail), + } + } + ThemeSettingsMode::Settings => { + let (settings_rows, card_h) = settings_rows_budget( + settings_total, + avail, + SETTINGS_TOP, + SETTINGS_ROW_H, + SETTINGS_FOOTER_H, + SETTINGS_DESCRIPTION_H, + SETTINGS_SEARCH_H, + search_active, + ); + Self { + list_rows: 0, + settings_rows, + card_h, + } + } + } + } +} + pub(crate) fn theme_settings_view_model( state: &crate::theme_settings::ThemeSettings, ) -> ThemeSettingsViewModel { diff --git a/crates/noa-app/src/macos_overlay/tests.rs b/crates/noa-app/src/macos_overlay/tests.rs index 61598cbc..90cc5b26 100644 --- a/crates/noa-app/src/macos_overlay/tests.rs +++ b/crates/noa-app/src/macos_overlay/tests.rs @@ -369,9 +369,13 @@ fn modal_caret_anchors_sit_in_the_input_row_and_follow_the_text() { assert_eq!(title.y, 800.0 * 0.30 + 40.0); assert!(title_prompt_caret(pane, 4).x > title.x); - let theme = theme_settings_caret(pane); + let theme_state = ThemeSettings::open(ThemeSettingsInit { + mode: ThemeSettingsMode::Theme, + ..settings_init() + }); + let theme = theme_settings_caret(pane, &theme_state); assert_eq!(theme.x, (1200.0 - THEME_SETTINGS_WIDTH) / 2.0 + 20.0); - assert_eq!(theme.y, 12.0 + 46.0); + assert_eq!(theme.y, 298.0); // A pane narrower than the card clamps the card to the pane's width // minus its margins; the caret must stay inside it. @@ -385,3 +389,37 @@ fn modal_caret_anchors_sit_in_the_input_row_and_follow_the_text() { assert!(caret.x >= 16.0 && caret.x < 400.0 - 16.0); assert!(caret.y > 0.0 && caret.y < 300.0); } + +#[test] +fn theme_caret_uses_filtered_rows_short_panes_and_settings_search_layout() { + let pane = PaneRectPt { + x: 0.0, + y: 0.0, + w: 1200.0, + h: 800.0, + }; + let mut theme = ThemeSettings::open(ThemeSettingsInit { + mode: ThemeSettingsMode::Theme, + ..settings_init() + }); + // Eight displayed rows give a 332pt card; a 300pt pane fits five rows + // in a 260pt card. Both anchors sit 64pt below the actual card top. + assert_eq!(theme_settings_caret(pane, &theme).y, 298.0); + assert_eq!( + theme_settings_caret(PaneRectPt { h: 300.0, ..pane }, &theme).y, + 84.0 + ); + theme.push_text("no-such-theme-999999999", std::time::Instant::now()); + assert_eq!(theme.filtered_len(), 0); + // Empty lists retain one row: 106 + 24 + 34 = 164pt. + assert_eq!(theme_settings_caret(pane, &theme).y, 382.0); + + let mut settings = ThemeSettings::open(settings_init()); + settings.toggle_settings_search(); + settings.push_text("no-such-setting-999999999", std::time::Instant::now()); + assert!(settings.settings_search_active()); + assert_eq!(settings.settings_filtered_len(), 0); + // Settings reserve the description and search lines, yielding 158pt; + // their search input starts at 66pt rather than the theme's 64pt. + assert_eq!(theme_settings_caret(pane, &settings).y, 387.0); +} diff --git a/crates/noa-grid/src/screen/edit.rs b/crates/noa-grid/src/screen/edit.rs index 005e45a5..a8380e8b 100644 --- a/crates/noa-grid/src/screen/edit.rs +++ b/crates/noa-grid/src/screen/edit.rs @@ -808,15 +808,12 @@ impl Screen { // ── edit ───────────────────────────────────────────────────────── pub fn insert_blank_chars(&mut self, n: u16) { - // #TODO(agent): Guard IL/DL outside the horizontal margins, and - // ICH/DCH below the left margin, when cursor addressing stops - // clamping to DECSLRM. self.cursor.pending_wrap = false; let blank = self.blank(); let x = self.cursor.x as usize; let y = self.cursor.y as usize; let right = self.right_margin() as usize; - if x > right { + if x < self.left_margin() as usize || x > right { return; } let len = right + 1 - x; @@ -843,7 +840,7 @@ impl Screen { let x = self.cursor.x as usize; let y = self.cursor.y as usize; let right = self.right_margin() as usize; - if x > right { + if x < self.left_margin() as usize || x > right { return; } let len = right + 1 - x; @@ -884,7 +881,11 @@ impl Screen { pub fn insert_lines(&mut self, n: u16) { self.cursor.pending_wrap = false; - if self.cursor.y < self.region.top || self.cursor.y > self.region.bottom { + if self.cursor.y < self.region.top + || self.cursor.y > self.region.bottom + || self.cursor.x < self.left_margin() + || self.cursor.x > self.right_margin() + { return; } let start = self.cursor.y as usize; @@ -909,7 +910,11 @@ impl Screen { pub fn delete_lines(&mut self, n: u16) { self.cursor.pending_wrap = false; - if self.cursor.y < self.region.top || self.cursor.y > self.region.bottom { + if self.cursor.y < self.region.top + || self.cursor.y > self.region.bottom + || self.cursor.x < self.left_margin() + || self.cursor.x > self.right_margin() + { return; } let start = self.cursor.y as usize; diff --git a/crates/noa-grid/src/screen/print.rs b/crates/noa-grid/src/screen/print.rs index d27814fb..20d84c4f 100644 --- a/crates/noa-grid/src/screen/print.rs +++ b/crates/noa-grid/src/screen/print.rs @@ -5,6 +5,17 @@ use super::*; impl Screen { // ── printing ─────────────────────────────────────────────────── + /// Absolute addressing can place the cursor outside DECSLRM. Text there + /// wraps at the screen edge until the cursor enters the margin interval. + fn print_margins(&self, x: u16) -> (u16, u16) { + let (left, right) = (self.left_margin(), self.right_margin()); + if (left..=right).contains(&x) { + (left, right) + } else { + (0, self.cols.saturating_sub(1)) + } + } + /// [`Screen::print_width`] behind a direct-indexed BMP table. The /// per-scalar `unicode-width` multi-level lookup shows up at ~6% of the /// bulk unicode ingest profile; one byte per BMP codepoint (64 KiB, @@ -62,7 +73,8 @@ impl Screen { return; } - if width == 2 && self.right_margin() <= self.left_margin() { + let (left, right) = self.print_margins(self.cursor.x); + if width == 2 && right <= left { let blank = self.blank(); let (x, y) = (self.cursor.x as usize, self.cursor.y as usize); let Some(row) = self.grid.get_mut(y) else { @@ -80,17 +92,17 @@ impl Screen { row.wrapped = true; } self.index(); - self.cursor.x = self.left_margin(); + self.cursor.x = left; self.cursor.pending_wrap = false; } - if width == 2 && self.cursor.x.saturating_add(1) > self.right_margin() { + if width == 2 && self.cursor.x.saturating_add(1) > right { if autowrap { if let Some(row) = self.grid.get_mut(self.cursor.y as usize) { row.wrapped = true; } self.index(); - self.cursor.x = self.left_margin(); + self.cursor.x = left; self.cursor.pending_wrap = false; } else { let blank = self.blank(); @@ -171,8 +183,8 @@ impl Screen { row.dirty = true; row.mark_occupied(x + width); - if self.cursor.x.saturating_add(width as u16) > self.right_margin() { - self.cursor.x = self.right_margin(); + if self.cursor.x.saturating_add(width as u16) > right { + self.cursor.x = right; self.cursor.pending_wrap = true; // latch; stay in the last column } else { self.cursor.x += width as u16; @@ -191,6 +203,13 @@ impl Screen { bytes.iter().all(|&b| (0x20..=0x7e).contains(&b)), "print_ascii_run only takes printable ASCII" ); + if self.cursor.x < self.left_margin() || self.cursor.x > self.right_margin() { + // Crossing into the margins can change the wrap boundary mid-run. + for &b in bytes { + self.print(b as char, autowrap, grapheme_clustering); + } + return; + } let mut bytes = bytes; if grapheme_clustering { // Only a run prefix can extend a pre-existing cluster (an ASCII @@ -238,16 +257,9 @@ impl Screen { // scalar without moving the cursor, so the whole rest drops. return; } - // Cells available on this row segment: through the right margin - // (inclusive) and within the row; a cursor already past the - // margin still writes one cell before snapping back (as the - // per-scalar path does). + // Cells available through the right margin (inclusive). let seg_end = (right as usize + 1).min(row.cells.len()); - let n = if x >= seg_end { - 1 - } else { - (seg_end - x).min(bytes.len() - i) - }; + let n = (seg_end - x).min(bytes.len() - i); // A wide pair straddling the segment can only leak a stray half // *outside* the segment through its two edge cells — an interior // hit's neighbor is also inside the segment and gets the same @@ -321,6 +333,12 @@ impl Screen { ) where I: Iterator, { + if self.cursor.x < self.left_margin() || self.cursor.x > self.right_margin() { + for c in chars { + self.print(c, autowrap, grapheme_clustering); + } + return; + } let mut chars = chars.peekable(); if grapheme_clustering { // As in `print_ascii_run`: only a run prefix can extend a @@ -552,7 +570,8 @@ impl Screen { /// glyph may overdraw its neighbor, but the grid never desyncs. fn promote_cluster_to_wide(&mut self, x: usize) { let spacer_x = x + 1; - if spacer_x > self.right_margin() as usize { + let (_, right) = self.print_margins(x as u16); + if spacer_x > right as usize { return; } let blank = self.blank(); @@ -582,8 +601,8 @@ impl Screen { }; row.dirty = true; if self.cursor.x as usize == spacer_x { - if self.cursor.x.saturating_add(1) > self.right_margin() { - self.cursor.x = self.right_margin(); + if self.cursor.x.saturating_add(1) > right { + self.cursor.x = right; self.cursor.pending_wrap = true; } else { self.cursor.x += 1; diff --git a/crates/noa-grid/src/tests/bulk_print.rs b/crates/noa-grid/src/tests/bulk_print.rs index 312a4b47..3a3cc299 100644 --- a/crates/noa-grid/src/tests/bulk_print.rs +++ b/crates/noa-grid/src/tests/bulk_print.rs @@ -52,10 +52,25 @@ fn bulk_print_matches_per_scalar_with_autowrap_off() { fn bulk_print_matches_per_scalar_inside_horizontal_margins() { // DECLRMM + DECSLRM 3..6, cursor inside the margins. assert_bulk_print_matches_per_scalar(10, 4, b"\x1b[?69h\x1b[3;6s\x1b[1;4H", "abcdefghijkl"); - // Cursor placed right of the right margin: one write, then snap + latch. + // Cursor right of the margin wraps at the screen edge. assert_bulk_print_matches_per_scalar(10, 4, b"\x1b[?69h\x1b[3;6s\x1b[1;9H", "abc"); } +#[test] +fn bulk_print_matches_per_scalar_across_horizontal_margin_boundaries() { + for col in [1, 2, 3, 7, 8, 9, 10] { + for autowrap in [true, false] { + let setup = format!( + "\x1b[?69h\x1b[3;7s\x1b[?7{}\x1b[1;{col}H", + if autowrap { 'h' } else { 'l' } + ); + for text in ["abcdefghijkl", "日本語日本語", "❤\u{fe0f}AB"] { + assert_bulk_print_matches_per_scalar(10, 6, setup.as_bytes(), text); + } + } + } +} + #[test] fn bulk_print_matches_per_scalar_overwriting_wide_cells() { let setup = "日本語\x1b[1;2H".as_bytes(); diff --git a/crates/noa-grid/src/tests/terminal_state.rs b/crates/noa-grid/src/tests/terminal_state.rs index e169f90f..79d23c89 100644 --- a/crates/noa-grid/src/tests/terminal_state.rs +++ b/crates/noa-grid/src/tests/terminal_state.rs @@ -1370,3 +1370,62 @@ fn absolute_cursor_placement_is_margin_relative_in_origin_mode() { let t = run(b"\x1b[?69h\x1b[?6h\x1b[5;5H\x1b[10;20s"); assert_eq!((t.primary.cursor.x, t.primary.cursor.y), (9, 0)); } +#[test] +fn edit_commands_ignore_cursor_outside_horizontal_margins() { + for command in [b'@', b'P', b'L', b'M'] { + for col in [1, 2, 8, 10] { + let mut t = Terminal::new(GridSize::new(10, 3)); + let mut stream = Stream::new(); + stream.feed(b"ABCDEFGHIJ\r\nKLMNOPQRST\r\nUVWXYZ1234", &mut t); + stream.feed(b"\x1b[?69h\x1b[3;7s", &mut t); + stream.feed(format!("\x1b[1;{col}H").as_bytes(), &mut t); + let before = t.active().grid.iter().map(|row| row.cells.clone()).collect::>(); + stream.feed(&[0x1b, b'[', command], &mut t); + let after = t.active().grid.iter().map(|row| row.cells.clone()).collect::>(); + assert_eq!(after, before, "command {} at column {col}", command as char); + } + } +} + +#[test] +fn print_outside_horizontal_margins_uses_screen_edge() { + use noa_vt::Handler as _; + for bulk in [false, true] { + for text in ["ABC", "日本"] { + let mut t = Terminal::new(GridSize::new(10, 3)); + let mut stream = Stream::new(); + stream.feed(b"\x1b[?69h\x1b[3;7s\x1b[1;9H", &mut t); + if bulk { + stream.feed(text.as_bytes(), &mut t); + } else { + for c in text.chars() { t.print(c); } + } + assert_eq!(cell(&t, 8, 0).ch, text.chars().next().unwrap()); + if text == "ABC" { + assert_eq!(cell(&t, 9, 0).ch, 'B'); + assert_eq!(cell(&t, 0, 1).ch, 'C'); + } else { + assert!(cell(&t, 9, 0).attrs.contains(CellAttrs::WIDE_SPACER)); + assert_eq!(cell(&t, 0, 1).ch, '本'); + } + assert!(t.active().grid[0].wrapped); + } + } +} + +#[test] +fn print_outside_horizontal_margins_without_wrap_stays_at_screen_edge() { + let t = run_size(10, 3, b"\x1b[?69h\x1b[3;7s\x1b[?7l\x1b[1;9HABC"); + assert_eq!(row_text(&t, 0, 10), " AC"); + assert_eq!((t.primary.cursor.x, t.primary.cursor.y), (9, 0)); + assert!(!t.primary.grid[0].wrapped); +} + +#[test] +fn grapheme_outside_horizontal_margins_can_expand_to_screen_edge() { + let t = run_size(10, 3, "\x1b[?69h\x1b[3;7s\x1b[?2027h\x1b[1;9H❤\u{fe0f}".as_bytes()); + assert!(cell(&t, 8, 0).attrs.contains(CellAttrs::WIDE)); + assert!(cell(&t, 9, 0).attrs.contains(CellAttrs::WIDE_SPACER)); + assert_eq!((t.primary.cursor.x, t.primary.cursor.y), (9, 0)); + assert!(t.primary.cursor.pending_wrap); +} From bafa18c484ec02d94d3241d4ae0feaa3467bfb2e Mon Sep 17 00:00:00 2001 From: simota Date: Thu, 10 Sep 2026 14:14:09 +0900 Subject: [PATCH 3/3] fix: preserve margin contents and align modal IME carets Absolute placement outside horizontal margins must not make relative motion reverse direction or scroll unrelated cells during wrapping, LF, or RI. Use the rendered search status width and the palette's visible rows to keep IME candidate anchors aligned with the active input field. Add regressions for boundary movement, clipped search text, and short palette panes with mixed row types. --- crates/noa-app/src/app/input_ops/ime.rs | 85 +++++++++---- .../noa-app/src/macos_overlay/imp/appkit.rs | 42 ++----- crates/noa-app/src/macos_overlay/model.rs | 73 +++++++++--- crates/noa-app/src/macos_overlay/tests.rs | 60 +++++++++- crates/noa-grid/src/screen/edit.rs | 51 ++++++-- crates/noa-grid/src/tests/terminal_state.rs | 112 ++++++++++++++++++ crates/noa-render/src/lib.rs | 1 + crates/noa-render/src/renderer/mod.rs | 1 + crates/noa-render/src/renderer/overlay.rs | 9 ++ .../noa-render/src/renderer/tests/overlay.rs | 45 +++++++ 10 files changed, 398 insertions(+), 81 deletions(-) diff --git a/crates/noa-app/src/app/input_ops/ime.rs b/crates/noa-app/src/app/input_ops/ime.rs index 696ac00f..ecba1f4b 100644 --- a/crates/noa-app/src/app/input_ops/ime.rs +++ b/crates/noa-app/src/app/input_ops/ime.rs @@ -1,5 +1,14 @@ use super::super::*; +fn search_prompt_ime_caret_col( + buffer: &str, + preedit: &str, + search: &noa_grid::SearchState, + cols: u16, +) -> u16 { + noa_render::search_prompt_caret_col(&format!("{buffer}{preedit}"), search, cols) +} + impl App { pub(in crate::app) fn modal_ime_target(&self, window_id: WindowId) -> Option { if self @@ -174,38 +183,36 @@ impl App { match target { ModalImeTarget::ConfirmDialog => None, ModalImeTarget::SearchPrompt => { - // One row at the top-right of the searched pane (see - // `append_search_prompt_instances`): the prompt text ends at - // the last column, with a status suffix of at most - // ` no matches` / ` 999/999` after the query. - const SUFFIX_COLS: usize = 12; let session = self.search_prompt.as_ref()?; let surface = state.surfaces.get(&session.pane_id)?; - let cols = usize::from(surface.grid_size.cols); - let shown = session.prompt.buffer().chars().count() + preedit_chars + SUFFIX_COLS; - let col = cols.saturating_sub(shown).min(cols.saturating_sub(1)); - Some(ime_cursor_area( - metrics, - col as u16, - 0, - surface.rect, - self.padding, - )) + let col = search_prompt_ime_caret_col( + session.prompt.buffer(), + self.modal_preedit_for(window_id, target), + &surface.terminal.lock().active().search, + surface.grid_size.cols, + ); + Some(ime_cursor_area(metrics, col, 0, surface.rect, self.padding)) } ModalImeTarget::CommandPalette => { - let chars = self - .command_palette - .as_ref() - .map_or(0, |session| session.palette.query().chars().count()); + let session = self.command_palette.as_ref()?; + let snapshot = + command_palette_snapshot(&self.keybinds, &session.palette, |command| { + self.command_is_enabled(window_id, command) + }); Some(caret_px(crate::macos_overlay::palette_query_caret( pane, - chars + preedit_chars, + &snapshot, + snapshot.query.chars().count() + preedit_chars, + ))) + } + ModalImeTarget::RemoteUi => { + let (snapshot, _) = self.remote_ui_snapshot(window_id)?; + Some(caret_px(crate::macos_overlay::palette_query_caret( + pane, + &snapshot, + self.remote_ui_input_chars() + preedit_chars, ))) } - ModalImeTarget::RemoteUi => Some(caret_px(crate::macos_overlay::palette_query_caret( - pane, - self.remote_ui_input_chars() + preedit_chars, - ))), ModalImeTarget::TabTitlePrompt => { let chars = self .tab_title_prompt @@ -244,3 +251,33 @@ impl App { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn search_prompt_ime_caret_follows_the_drawn_status_suffix() { + let mut search = noa_grid::SearchState::default(); + search.set_query( + "needle".to_string(), + Vec::new(), + noa_grid::SearchAnchor::Backward(noa_grid::SelectionPoint::new(0, 0)), + ); + assert_eq!( + search_prompt_ime_caret_col(&"a".repeat(30), &"b".repeat(10), &search, 80), + 68 + ); + assert_eq!(search_prompt_ime_caret_col("a", "", &search, 80), 68); + assert_eq!( + search_prompt_ime_caret_col(&"日".repeat(100), "本", &search, 80), + 68 + ); + assert_eq!(search_prompt_ime_caret_col("a", "", &search, 5), 0); + assert_eq!(search_prompt_ime_caret_col("a", "", &search, 0), 0); + assert_eq!( + search_prompt_ime_caret_col("", "日本", &noa_grid::SearchState::default(), 80), + 75 + ); + } +} diff --git a/crates/noa-app/src/macos_overlay/imp/appkit.rs b/crates/noa-app/src/macos_overlay/imp/appkit.rs index f4152340..ee36e602 100644 --- a/crates/noa-app/src/macos_overlay/imp/appkit.rs +++ b/crates/noa-app/src/macos_overlay/imp/appkit.rs @@ -38,16 +38,10 @@ const ID_SCRATCH_BADGE: &str = "noa.native-overlay.scratch-badge"; /// Palette metrics (points). The widths/row heights the IME caret anchor /// also needs live in `model.rs` so the two can't drift. use crate::macos_overlay::model::{ - CARD_PAD_H, PALETTE_WIDTH, QUERY_ROW_H, THEME_SETTINGS_WIDTH, TITLE_PROMPT_H, - TITLE_PROMPT_WIDTH, + CARD_PAD_H, ENTRY_ROW_H, HEADER_ROW_H, LIST_PAD_V, PaletteCardLayout, QUERY_ROW_H, + THEME_SETTINGS_WIDTH, TITLE_PROMPT_H, TITLE_PROMPT_WIDTH, }; -const ENTRY_ROW_H: f64 = 26.0; -const HEADER_ROW_H: f64 = 24.0; -const LIST_PAD_V: f64 = 6.0; const CARD_RADIUS: f64 = 12.0; -/// Max list rows (headers + entries) visible at once — matches the wgpu -/// card's 12-row window. -const PALETTE_CAPACITY: usize = 12; const SCRIM_ALPHA: f64 = 0.25; /// Balance a `+1` (alloc/init) object that a superview now retains: @@ -631,32 +625,18 @@ pub(in crate::macos_overlay) fn rebuild_palette( return; }; - // Window capacity bounded by the pane height so the list never - // runs past the card's bottom edge on a short pane. - let capacity = (((pane.h - 24.0 - QUERY_ROW_H - 1.0 - LIST_PAD_V * 2.0) / ENTRY_ROW_H) - as usize) - .clamp(3, PALETTE_CAPACITY); - let (offset, shown) = overlay_scroll_window(snap.rows.len(), snap.selected, capacity); + let PaletteCardLayout { + card_w, + card_h, + card_x, + card_top, + offset, + shown, + } = PaletteCardLayout::new(pane, snap); let visible = &snap.rows[offset..offset + shown]; let empty = snap.rows.is_empty(); - let list_h: f64 = if empty { - 36.0 - } else { - visible - .iter() - .map(|row| match row { - PaletteRow::Header { .. } => HEADER_ROW_H, - PaletteRow::Entry { .. } => ENTRY_ROW_H, - }) - .sum::() - + LIST_PAD_V * 2.0 - }; - let card_w = PALETTE_WIDTH.min(pane.w - 32.0).max(280.0); - let card_h = (QUERY_ROW_H + 1.0 + list_h).min(pane.h - 24.0); - let card_x = (pane.w - card_w) / 2.0; - let card_y_top = (pane.h * 0.14).min(pane.h - card_h).max(8.0); let card_frame = NSRect::new( - NSPoint::new(card_x, from_top(pane.h, card_y_top, card_h)), + NSPoint::new(card_x, from_top(pane.h, card_top, card_h)), NSSize::new(card_w, card_h), ); let (root, effect) = card_for( diff --git a/crates/noa-app/src/macos_overlay/model.rs b/crates/noa-app/src/macos_overlay/model.rs index 8faea6bd..c940c6eb 100644 --- a/crates/noa-app/src/macos_overlay/model.rs +++ b/crates/noa-app/src/macos_overlay/model.rs @@ -1,6 +1,6 @@ use std::hash::{Hash, Hasher}; -use noa_render::OverlayStyle; +use noa_render::{CommandPaletteSnapshot, OverlayStyle, PaletteRow}; /// Opaque `CGColorRef` for `msg_send!` returns/arguments. The `-CGColor` /// property returns `^{CGColor=}`, not an object (`@`) — typing it as @@ -75,6 +75,11 @@ pub(crate) const TITLE_PROMPT_HINT: &str = "Enter to set \u{b7} Empty clears \u{ pub(crate) const PALETTE_WIDTH: f64 = 560.0; pub(crate) const QUERY_ROW_H: f64 = 44.0; pub(crate) const CARD_PAD_H: f64 = 16.0; +pub(crate) const ENTRY_ROW_H: f64 = 26.0; +pub(crate) const HEADER_ROW_H: f64 = 24.0; +pub(crate) const LIST_PAD_V: f64 = 6.0; +/// Maximum visible list rows, including headers. +const PALETTE_CAPACITY: usize = 12; pub(crate) const TITLE_PROMPT_WIDTH: f64 = 420.0; pub(crate) const TITLE_PROMPT_H: f64 = 104.0; pub(crate) const THEME_SETTINGS_WIDTH: f64 = 660.0; @@ -94,20 +99,60 @@ pub(crate) struct CaretPt { pub(crate) h: f64, } -/// The palette query row's caret after `query_chars` characters (also the -/// send-selection picker and the remote-UI endpoint field, which draw the -/// same card). Mirrors `rebuild_palette`'s frame math with the card's -/// minimum height (query row + empty-list stub) standing in for the -/// list-dependent height — the `min(pane.h - card_h)` term only binds on a -/// pane shorter than the card. -pub(crate) fn palette_query_caret(pane: PaneRectPt, query_chars: usize) -> CaretPt { - let card_w = PALETTE_WIDTH.min(pane.w - 32.0).max(280.0); - let card_h_min = QUERY_ROW_H + 1.0 + 36.0; - let card_x = (pane.w - card_w) / 2.0; - let card_top = (pane.h * 0.14).min(pane.h - card_h_min).max(8.0); +/// Card placement and visible rows shared by native drawing and IME. +pub(crate) struct PaletteCardLayout { + pub(crate) card_w: f64, + pub(crate) card_h: f64, + pub(crate) card_x: f64, + pub(crate) card_top: f64, + pub(crate) offset: usize, + pub(crate) shown: usize, +} + +impl PaletteCardLayout { + pub(crate) fn new(pane: PaneRectPt, snapshot: &CommandPaletteSnapshot) -> Self { + let capacity = (((pane.h - 24.0 - QUERY_ROW_H - 1.0 - LIST_PAD_V * 2.0) / ENTRY_ROW_H) + as usize) + .clamp(3, PALETTE_CAPACITY); + let (offset, shown) = + overlay_scroll_window(snapshot.rows.len(), snapshot.selected, capacity); + let visible = &snapshot.rows[offset..offset + shown]; + let list_h = if visible.is_empty() { + 36.0 + } else { + visible + .iter() + .map(|row| match row { + PaletteRow::Header { .. } => HEADER_ROW_H, + PaletteRow::Entry { .. } => ENTRY_ROW_H, + }) + .sum::() + + LIST_PAD_V * 2.0 + }; + let card_w = PALETTE_WIDTH.min(pane.w - 32.0).max(280.0); + let card_h = (QUERY_ROW_H + 1.0 + list_h).min(pane.h - 24.0); + Self { + card_w, + card_h, + card_x: (pane.w - card_w) / 2.0, + card_top: (pane.h * 0.14).min(pane.h - card_h).max(8.0), + offset, + shown, + } + } +} + +/// The palette query row's caret after `query_chars` characters, including +/// the remote endpoint field, which uses the same card. +pub(crate) fn palette_query_caret( + pane: PaneRectPt, + snapshot: &CommandPaletteSnapshot, + query_chars: usize, +) -> CaretPt { + let layout = PaletteCardLayout::new(pane, snapshot); CaretPt { - x: card_x + CARD_PAD_H + 22.0 + query_chars as f64 * INPUT_FONT_ADVANCE, - y: card_top + 13.0, + x: layout.card_x + CARD_PAD_H + 22.0 + query_chars as f64 * INPUT_FONT_ADVANCE, + y: layout.card_top + 13.0, w: 1.0, h: INPUT_ROW_H, } diff --git a/crates/noa-app/src/macos_overlay/tests.rs b/crates/noa-app/src/macos_overlay/tests.rs index 90cc5b26..6436bfeb 100644 --- a/crates/noa-app/src/macos_overlay/tests.rs +++ b/crates/noa-app/src/macos_overlay/tests.rs @@ -5,6 +5,23 @@ use super::model::{ }; use super::sync::theme_settings_sync_decision; use crate::theme_settings::{Liveness, ThemeSettings, ThemeSettingsInit, ThemeSettingsMode}; +use noa_render::{CommandPaletteSnapshot, PaletteRow}; + +fn palette_snapshot(entries: usize) -> CommandPaletteSnapshot { + CommandPaletteSnapshot { + query: String::new(), + rows: (0..entries) + .map(|index| PaletteRow::Entry { + title: format!("Result {index}"), + hint: None, + match_positions: Vec::new(), + enabled: true, + }) + .collect(), + selected: 0, + total_entries: entries, + } +} fn settings_init() -> ThemeSettingsInit { ThemeSettingsInit { @@ -358,10 +375,11 @@ fn modal_caret_anchors_sit_in_the_input_row_and_follow_the_text() { h: 800.0, }; let card_x = (1200.0 - PALETTE_WIDTH) / 2.0; - let empty = palette_query_caret(pane, 0); + let snapshot = palette_snapshot(0); + let empty = palette_query_caret(pane, &snapshot, 0); assert_eq!(empty.x, card_x + CARD_PAD_H + 22.0); assert_eq!(empty.y, 800.0 * 0.14 + 13.0); - let typed = palette_query_caret(pane, 10); + let typed = palette_query_caret(pane, &snapshot, 10); assert!(typed.x > empty.x && typed.y == empty.y); let title = title_prompt_caret(pane, 0); @@ -385,7 +403,7 @@ fn modal_caret_anchors_sit_in_the_input_row_and_follow_the_text() { w: 400.0, h: 300.0, }; - let caret = palette_query_caret(narrow, 0); + let caret = palette_query_caret(narrow, &snapshot, 0); assert!(caret.x >= 16.0 && caret.x < 400.0 - 16.0); assert!(caret.y > 0.0 && caret.y < 300.0); } @@ -423,3 +441,39 @@ fn theme_caret_uses_filtered_rows_short_panes_and_settings_search_layout() { // their search input starts at 66pt rather than the theme's 64pt. assert_eq!(theme_settings_caret(pane, &settings).y, 387.0); } + +#[test] +fn palette_caret_uses_twelve_result_rows_in_a_short_pane() { + let pane = PaneRectPt { + h: 400.0, + ..test_rect() + }; + // Twelve 26pt entries + 12pt list padding + 45pt query/rule = 369pt. + // The card starts at 400 - 369 = 31pt, and the query at 31 + 13 = 44pt. + assert_eq!(palette_query_caret(pane, &palette_snapshot(12), 10).y, 44.0); +} + +#[test] +fn palette_caret_tracks_visible_headers_selection_and_pane_height() { + let pane = PaneRectPt { + h: 400.0, + ..test_rect() + }; + let mut snapshot = palette_snapshot(20); + snapshot.rows[0] = PaletteRow::Header { + label: "Commands".to_string(), + }; + // One 24pt header and eleven 26pt entries make a 367pt card. + assert_eq!(palette_query_caret(pane, &snapshot, 0).y, 46.0); + snapshot.selected = 19; + // Scrolling the header out replaces it with a 26pt entry. + assert_eq!(palette_query_caret(pane, &snapshot, 0).y, 44.0); + assert_eq!(palette_query_caret(pane, &palette_snapshot(0), 0).y, 69.0); + + let short = PaneRectPt { h: 300.0, ..pane }; + // Eight entries fit (265pt card), placing the query at 35 + 13pt. + assert_eq!(palette_query_caret(short, &snapshot, 0).y, 48.0); + let tiny = PaneRectPt { h: 100.0, ..pane }; + // The card height is capped at pane.h - 24 even at the three-row floor. + assert!((palette_query_caret(tiny, &snapshot, 0).y - 27.0).abs() < 1e-9); +} diff --git a/crates/noa-grid/src/screen/edit.rs b/crates/noa-grid/src/screen/edit.rs index a8380e8b..9b4f5ce9 100644 --- a/crates/noa-grid/src/screen/edit.rs +++ b/crates/noa-grid/src/screen/edit.rs @@ -226,7 +226,9 @@ impl Screen { /// Index (IND / LF without CR): down one row, scrolling at the region bottom. pub fn index(&mut self) { self.cursor.pending_wrap = false; - if self.cursor.y == self.region.bottom { + if self.cursor.y == self.region.bottom + && (self.left_margin()..=self.right_margin()).contains(&self.cursor.x) + { self.scroll_up_region(1); } else if self.cursor.y + 1 < self.rows { self.cursor.y += 1; @@ -236,7 +238,9 @@ impl Screen { /// Reverse index (RI): up one row, scrolling down at the region top. pub fn reverse_index(&mut self) { self.cursor.pending_wrap = false; - if self.cursor.y == self.region.top { + if self.cursor.y == self.region.top + && (self.left_margin()..=self.right_margin()).contains(&self.cursor.x) + { self.scroll_down_region(1); } else if self.cursor.y > 0 { self.cursor.y -= 1; @@ -499,14 +503,32 @@ impl Screen { // ── horizontal / absolute motion ──────────────────────────────── + // A relative move stops at the margin in its direction of travel, + // unless the cursor is already beyond that margin. Absolute placement + // can put it there with DECOM off; then the screen edge is the limit. + fn cursor_left_bound(&self) -> u16 { + if self.cursor.x >= self.left_margin() { + self.left_margin() + } else { + 0 + } + } + + fn cursor_right_bound(&self) -> u16 { + if self.cursor.x <= self.right_margin() { + self.right_margin() + } else { + self.cols.saturating_sub(1) + } + } + pub fn carriage_return(&mut self) { - self.cursor.x = self.left_margin(); + self.cursor.x = self.cursor_left_bound(); self.cursor.pending_wrap = false; } pub fn backspace(&mut self) { - self.cursor.pending_wrap = false; - self.cursor.x = self.cursor.x.saturating_sub(1).max(self.left_margin()); + self.cursor_backward(1); } pub fn cursor_up(&mut self, n: u16) { @@ -534,13 +556,21 @@ impl Screen { pub fn cursor_forward(&mut self, n: u16) { self.cursor.pending_wrap = false; let n = n.max(1); - self.cursor.x = self.cursor.x.saturating_add(n).min(self.right_margin()); + self.cursor.x = self + .cursor + .x + .saturating_add(n) + .min(self.cursor_right_bound()); } pub fn cursor_backward(&mut self, n: u16) { self.cursor.pending_wrap = false; let n = n.max(1); - self.cursor.x = self.cursor.x.saturating_sub(n).max(self.left_margin()); + self.cursor.x = self + .cursor + .x + .saturating_sub(n) + .max(self.cursor_left_bound()); } /// Absolute cursor placement (`CUP`/`HVP` with DECOM off). Ghostty's @@ -630,14 +660,17 @@ impl Screen { self.cursor.x = self .tabstops .next(self.cursor.x, self.cols) - .min(self.right_margin()); + .min(self.cursor_right_bound()); } } pub fn tab_back(&mut self, n: u16) { self.cursor.pending_wrap = false; for _ in 0..n.max(1) { - self.cursor.x = self.tabstops.prev(self.cursor.x).max(self.left_margin()); + self.cursor.x = self + .tabstops + .prev(self.cursor.x) + .max(self.cursor_left_bound()); } } diff --git a/crates/noa-grid/src/tests/terminal_state.rs b/crates/noa-grid/src/tests/terminal_state.rs index 79d23c89..8ea83dce 100644 --- a/crates/noa-grid/src/tests/terminal_state.rs +++ b/crates/noa-grid/src/tests/terminal_state.rs @@ -1421,6 +1421,118 @@ fn print_outside_horizontal_margins_without_wrap_stays_at_screen_edge() { assert!(!t.primary.grid[0].wrapped); } +#[test] +fn wrap_outside_horizontal_margins_preserves_scroll_rectangle() { + for text in ["ABC", "日本", "A日"] { + let t = run_size( + 10, + 3, + format!( + "ABCDEFGHIJ\r\nKLMNOPQRST\r\nUVWXYZ1234\x1b[?69h\x1b[3;7s\x1b[3;9H{text}" + ) + .as_bytes(), + ); + assert_eq!(row_text(&t, 0, 10), "ABCDEFGHIJ", "{text}"); + assert_eq!(row_text(&t, 1, 10), "KLMNOPQRST", "{text}"); + assert_eq!( + row_text(&t, 2, 7).chars().skip(2).collect::(), + "WXYZ1", + "{text}" + ); + assert_eq!(t.primary.cursor.y, 2); + assert_eq!(t.primary.scrollback_len(), 0); + } +} + +#[test] +fn vertical_motion_outside_horizontal_margins_does_not_scroll() { + for col in [1, 2, 8, 10] { + for (region, row, command, expected_y) in [ + ("1;3", 3, "\n", 2), + ("1;3", 3, "\x1bD", 2), + ("1;3", 1, "\x1bM", 0), + ("2;3", 2, "\x1bM", 0), + ("1;2", 2, "\n", 2), + ] { + let t = run_size( + 10, + 3, + format!( + "ABCDEFGHIJ\r\nKLMNOPQRST\r\nUVWXYZ1234\x1b[?69h\x1b[3;7s\x1b[{region}r\x1b[{row};{col}H{command}" + ) + .as_bytes(), + ); + assert_eq!(row_text(&t, 0, 10), "ABCDEFGHIJ", "{col}, {command:?}"); + assert_eq!(row_text(&t, 1, 10), "KLMNOPQRST", "{col}, {command:?}"); + assert_eq!(row_text(&t, 2, 10), "UVWXYZ1234", "{col}, {command:?}"); + assert_eq!( + (t.primary.cursor.x, t.primary.cursor.y), + (col - 1, expected_y) + ); + } + } +} + +#[test] +fn vertical_motion_inside_horizontal_margins_scrolls_the_rectangle() { + for col in [3, 7] { + for (row, command, expected) in [ + (3, "\n", ["ABMNOPQHIJ", "KLWXYZ1RST", "UV 234"]), + (1, "\x1bM", ["AB HIJ", "KLCDEFGRST", "UVMNOPQ234"]), + ] { + let t = run_size( + 10, + 3, + format!( + "ABCDEFGHIJ\r\nKLMNOPQRST\r\nUVWXYZ1234\x1b[?69h\x1b[3;7s\x1b[{row};{col}H{command}" + ) + .as_bytes(), + ); + for (y, expected_row) in expected.iter().enumerate() { + assert_eq!(row_text(&t, y, 10), *expected_row, "{col}, {command:?}"); + } + assert_eq!( + (t.primary.cursor.x, t.primary.cursor.y), + (col - 1, row - 1) + ); + } + } +} + +#[test] +fn horizontal_motion_outside_margins_respects_direction_and_screen_edges() { + for (col, command, expected_x) in [ + (1, "\x08", 0), + (2, "\x08", 0), + (1, "\x1b[D", 0), + (2, "\x1b[65535D", 0), + (1, "\x1b[Z", 0), + (2, "\x1b[2Z", 0), + (2, "\r", 0), + (9, "\x1b[C", 9), + (10, "\x1b[C", 9), + (9, "\x1b[65535C", 9), + (9, "\t", 9), + (10, "\x1b[2I", 9), + // Moving toward the margin interval still stops at its far edge. + (1, "\x1b[65535C", 6), + (10, "\x1b[65535D", 2), + (2, "\x1b[2C", 3), + (8, "\x1b[2D", 5), + (4, "\x1b[65535D", 2), + (6, "\x1b[65535C", 6), + ] { + let t = run_size( + 10, + 3, + format!("\x1b[?69h\x1b[3;7s\x1b[2;{col}H{command}").as_bytes(), + ); + assert_eq!(t.primary.cursor.x, expected_x, "column {col}, {command:?}"); + assert_eq!(t.primary.cursor.y, 1); + assert!(!t.primary.cursor.pending_wrap); + } +} + #[test] fn grapheme_outside_horizontal_margins_can_expand_to_screen_edge() { let t = run_size(10, 3, "\x1b[?69h\x1b[3;7s\x1b[?2027h\x1b[1;9H❤\u{fe0f}".as_bytes()); diff --git a/crates/noa-render/src/lib.rs b/crates/noa-render/src/lib.rs index 9a0a87bd..0010f53f 100644 --- a/crates/noa-render/src/lib.rs +++ b/crates/noa-render/src/lib.rs @@ -31,6 +31,7 @@ pub use instance::{BlendMode, CellInstance, PaneUniformParams, Uniforms, populat pub use renderer::{ ConfirmDialogLayout, PaletteLayout, PaneFrame, Renderer, command_palette_layout, confirm_dialog_layout, paint_startup_frame, renderer_construction_count, + search_prompt_caret_col, }; pub use shared::{GlyphAtlasCache, PipelineCache, SharedGlyphAtlases, SharedPipelines}; pub use snapshot::{ diff --git a/crates/noa-render/src/renderer/mod.rs b/crates/noa-render/src/renderer/mod.rs index 0bcd01a2..829b7e8f 100644 --- a/crates/noa-render/src/renderer/mod.rs +++ b/crates/noa-render/src/renderer/mod.rs @@ -1372,4 +1372,5 @@ use cursor::*; use overlay::*; pub use overlay::{ ConfirmDialogLayout, PaletteLayout, command_palette_layout, confirm_dialog_layout, + search_prompt_caret_col, }; diff --git a/crates/noa-render/src/renderer/overlay.rs b/crates/noa-render/src/renderer/overlay.rs index db0692dd..55d87f25 100644 --- a/crates/noa-render/src/renderer/overlay.rs +++ b/crates/noa-render/src/renderer/overlay.rs @@ -999,6 +999,15 @@ pub(super) fn search_prompt_suffix_cols(buffer: &str, search: &SearchState) -> u search_prompt_suffix(buffer, search).chars().count() } +/// The search prompt's caret column, shared with the OS IME anchor. The +/// prompt is right-aligned and both truncation passes drop from the front, +/// so only the trailing ASCII status separates the caret from the right +/// edge, even for wide or combining query text. If the pane is too narrow +/// to show the caret, anchor at its left edge. +pub fn search_prompt_caret_col(buffer: &str, search: &SearchState, cols: u16) -> u16 { + usize::from(cols).saturating_sub(search_prompt_suffix_cols(buffer, search) + 1) as u16 +} + /// Turn the prompt's display text into row-local [`SegmentCell`]s, one per /// column — a double-width character gets a lead cell plus a blank spacer /// cell (mirroring `noa_grid::Screen`'s WIDE/WIDE_SPACER print path), and a diff --git a/crates/noa-render/src/renderer/tests/overlay.rs b/crates/noa-render/src/renderer/tests/overlay.rs index a33e7097..6db8c6f9 100644 --- a/crates/noa-render/src/renderer/tests/overlay.rs +++ b/crates/noa-render/src/renderer/tests/overlay.rs @@ -31,6 +31,51 @@ fn search_prompt_display_text_reports_no_matches_for_non_empty_query() { assert_eq!(text, "Find: needle\u{258F} no matches"); } +#[test] +fn search_prompt_caret_matches_rendered_cells_after_truncation() { + let mut no_matches = SearchState::default(); + no_matches.set_query( + "needle".to_string(), + Vec::new(), + noa_grid::SearchAnchor::Backward(SelectionPoint::new(0, 0)), + ); + let mut many_matches = SearchState::default(); + many_matches.set_query( + "a".to_string(), + (0..1234) + .map(|row| noa_grid::SearchMatch { + start: SelectionPoint::new(0, row), + end: SelectionPoint::new(0, row), + }) + .collect(), + noa_grid::SearchAnchor::Backward(SelectionPoint::new(0, 1233)), + ); + for search in [SearchState::default(), no_matches, many_matches] { + for buffer in [ + "".to_string(), + "short".to_string(), + "日e\u{301}".repeat(100), + ] { + for cols in [0, 1, 5, 11, 12, 20, 80] { + let text = search_prompt_display_text(&buffer, &search, cols); + let mut cells = search_prompt_segment_cells(&text, [255; 4]); + let excess = cells.len().saturating_sub(usize::from(cols)); + cells.drain(..excess); + let drawn_caret = cells + .iter() + .rposition(|cell| cell.ch == '\u{258F}') + .map_or(0, |index| usize::from(cols) - cells.len() + index); + assert_eq!( + usize::from(search_prompt_caret_col(&buffer, &search, cols)), + drawn_caret, + "buffer {buffer:?}, cols {cols}, status {}", + search_prompt_suffix(&buffer, &search), + ); + } + } + } +} + #[test] fn search_prompt_overlay_emits_top_right_bg_and_glyph_instances_and_tracks_the_buffer() { let Some(mut font) = font_with_rasterized_m() else {