From 9e731bc61212d040a634735936c02a27e4975a78 Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sat, 7 Nov 2026 15:35:52 +0000 Subject: [PATCH 01/10] Keep RTL replacement cursor selection coherent across rendering paths --- .../neomacs-gui-tests/fixtures/cursor-rtl.el | 44 ++++++ .../neomacs-gui-tests/tests/commit_editing.rs | 7 +- .../src/buffer_source/render_plan.rs | 146 +++++++++--------- .../src/display_cursor.rs | 83 +++++++--- .../src/tests/engine_line_spacing_test.rs | 89 +++++++++++ 5 files changed, 270 insertions(+), 99 deletions(-) create mode 100644 crates/neomacs-gui-tests/fixtures/cursor-rtl.el diff --git a/crates/neomacs-gui-tests/fixtures/cursor-rtl.el b/crates/neomacs-gui-tests/fixtures/cursor-rtl.el new file mode 100644 index 0000000000..d675d7f381 --- /dev/null +++ b/crates/neomacs-gui-tests/fixtures/cursor-rtl.el @@ -0,0 +1,44 @@ +;;; cursor-rtl.el --- Coherent RTL replacement cursor geometry -*- lexical-binding: t; -*- + +(defun neo-rtl-fail (err) + "Report ERR and terminate the isolated GUI process." + (princ (format "COMMIT-EDITING-FAIL %S\n" err) 'external-debugging-output) + (kill-emacs 1)) + +(run-at-time 10 nil (lambda () (neo-rtl-fail "watchdog expired"))) +(switch-to-buffer (get-buffer-create "*rtl-cursor*")) +(fundamental-mode) +(blink-cursor-mode -1) +(insert "AAA\nnext\n") +;; Logical index zero is tall but appears after the small glyphs visually. +(put-text-property 1 4 'display + (concat (propertize "א" 'face '(:height 2.0)) + (propertize "בג" 'face '(:height 0.5)))) +(goto-char 1) +(run-at-time + 0.5 nil + (lambda () + (condition-case err + (progn + (redisplay t) + (let* ((cold (window-cursor-info)) + (height (- (cdr (posn-x-y (posn-at-point 5))) (aref cold 2)))) + (unless (= (aref cold 4) height) + (error "RTL cursor must use logical first glyph's full height: %S/%S" cold height)) + (goto-char 4) + (redisplay t) + (let* ((path (and (fboundp 'neomacs--frame-snapshot) + (getenv "NEOMACS_LAYOUT_STATS_FILE"))) + (before (and path (file-attribute-size (file-attributes path))))) + (goto-char 1) + (redisplay t) + (unless (equal (window-cursor-info) cold) + (error "RTL cursor changed under replay: %S/%S" cold (window-cursor-info))) + (if before + (princ (format "CURSOR-REPLAY-STATS %d %d\n" before + (file-attribute-size (file-attributes path))) + 'external-debugging-output) + (princ "CURSOR-REPLAY-REFERENCE-GEOMETRY-ONLY\n" 'external-debugging-output))) + (princ "COMMIT-EDITING-PASS\n" 'external-debugging-output) + (kill-emacs 0))) + (error (neo-rtl-fail err))))) diff --git a/crates/neomacs-gui-tests/tests/commit_editing.rs b/crates/neomacs-gui-tests/tests/commit_editing.rs index d4b0b8f7b9..3e463ce070 100644 --- a/crates/neomacs-gui-tests/tests/commit_editing.rs +++ b/crates/neomacs-gui-tests/tests/commit_editing.rs @@ -69,7 +69,7 @@ fn run_fixture_probe(probe: &str, fixture: &str) { assert!(!result.timed_out, "{result:#?}\n{stderr}"); assert_eq!(result.exit_code, Some(0), "{result:#?}\n{stderr}"); assert!(stderr.contains("COMMIT-EDITING-PASS"), "{stderr}"); - if probe.ends_with("newline-cursor") + if (probe.ends_with("newline-cursor") || probe == "rtl-replacement-cursor") && !stderr.contains("CURSOR-REPLAY-REFERENCE-GEOMETRY-ONLY") { let marker = stderr @@ -196,6 +196,11 @@ fn mixed_newline_cursor_matches_gnu_row_geometry() { } } +#[test] +fn rtl_replacement_cursor_uses_one_glyph_for_placement_and_metrics() { + run_fixture_probe("rtl-replacement-cursor", "cursor-rtl.el"); +} + #[test] fn raised_text_keeps_the_original_descent() { run_fixture_probe("raised-text", "newline-geometry.el"); diff --git a/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs b/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs index 5db0096f71..52258fa277 100644 --- a/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs +++ b/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs @@ -389,9 +389,7 @@ fn replay_cursor_paint( .builder() .current_window_row(cursor_row) .and_then(|row| { - row.glyphs[GlyphArea::Text.index()] - .iter() - .find(|glyph| row.glyph_covers_buffer_charpos(glyph, point)) + crate::display_cursor::CursorVisualColumnResolutionRequest::point_glyph(row, point) .map(|glyph| glyph.face_id) }); let glyph_face = glyph_face_id @@ -417,9 +415,8 @@ fn retained_point_cursor_metrics( if buffer_newline && !row.continued && point == row.end_charpos { return Some((row.height_px, row.ascent_px)); } - if let Some(glyph) = row.glyphs[GlyphArea::Text.index()] - .iter() - .find(|glyph| row.glyph_covers_buffer_charpos(glyph, point)) + if let Some(glyph) = + crate::display_cursor::CursorVisualColumnResolutionRequest::point_glyph(row, point) { if glyph.pixel_height > 0.0 { return Some((glyph.pixel_height, glyph.pixel_ascent)); @@ -453,46 +450,43 @@ fn try_decorate_window_cursor( default_ascent: f32, measurement: DisplayRowMeasurementPolicy, ) -> bool { - let (row_pixel_y, row_height, row_ascent, cursor_width) = - match output.builder().current_window_row(cursor_row) { - Some(row) => { - let mut width = char_w; - for glyph in &row.glyphs[GlyphArea::Text.index()] { - if row.glyph_covers_buffer_charpos(glyph, point) { - // As the walk sizes it (`resolve_cursor_geometry`, - // GNU `get_phys_cursor_geometry`): on a stretch -- - // a TAB -- the cursor is one column wide unless - // `x-stretch-cursor` is set; a bar keeps its own. - width = if matches!(glyph.glyph_type, GlyphType::Stretch { .. }) - && !params.x_stretch_cursor - && !matches!(style, CursorStyle::Bar(_)) - { - crate::display_row::width::DisplayRowCharWidthPolicy::new(char_w) - .fallback() - } else { - glyph.pixel_width - }; - break; - } - } - let Some((face_height, face_ascent)) = - retained_point_cursor_metrics(row, point, buffer_newline, measurement) - else { - return false; + let (row_pixel_y, row_height, row_ascent, cursor_width) = match output + .builder() + .current_window_row(cursor_row) + { + Some(row) => { + let mut width = char_w; + if let Some(glyph) = + crate::display_cursor::CursorVisualColumnResolutionRequest::point_glyph(row, point) + { + // Preserve the GNU stretch/bar cursor width policy. + width = if matches!(glyph.glyph_type, GlyphType::Stretch { .. }) + && !params.x_stretch_cursor + && !matches!(style, CursorStyle::Bar(_)) + { + crate::display_row::width::DisplayRowCharWidthPolicy::new(char_w).fallback() + } else { + glyph.pixel_width }; - let (y, height, ascent) = crate::display_cursor::resolve_cursor_vertical_metrics( - row.pixel_y, - face_height, - face_ascent, - row.height_px, - row.ascent_px, - default_height, - row.ends_at_zv, - ); - (y, height, ascent, width) } - None => (0.0, default_height, default_ascent, char_w), - }; + let Some((face_height, face_ascent)) = + retained_point_cursor_metrics(row, point, buffer_newline, measurement) + else { + return false; + }; + let (y, height, ascent) = crate::display_cursor::resolve_cursor_vertical_metrics( + row.pixel_y, + face_height, + face_ascent, + row.height_px, + row.ascent_px, + default_height, + row.ends_at_zv, + ); + (y, height, ascent, width) + } + None => (0.0, default_height, default_ascent, char_w), + }; let window_id_i64 = window_id as i64; let paint = replay_cursor_paint(output, cursor_row, point, params); let cursor = PhysCursor { @@ -962,37 +956,41 @@ impl BufferSourceOutputSetup { // starting at 0. The full-rebuild path is unaffected: it // captures the cursor y during emission from `row_geometry`. cursor_row_pixel_y = row.pixel_y.max(prev_row_bottom); - for glyph in &row.glyphs[GlyphArea::Text.index()] { - if row.glyph_covers_buffer_charpos(glyph, replay.new_point as usize) { - cursor_width = glyph.pixel_width; - break; - } + if let Some(glyph) = + crate::display_cursor::CursorVisualColumnResolutionRequest::point_glyph( + row, + replay.new_point as usize, + ) + { + cursor_width = glyph.pixel_width; + } + if replay.retained_cursor.is_none() { + let buffer_newline = buf_access + .byte_at(buf_access.charpos_to_bytepos(replay.new_point)) + == Some(b'\n'); + let Some((face_height, face_ascent)) = retained_point_cursor_metrics( + row, + replay.new_point as usize, + buffer_newline, + default_face.measurement_policy(), + ) else { + output.restore_retry_checkpoint(retry_checkpoint); + return BufferSourceRenderAttemptOutcome::ReplayMispredicted; + }; + let (y, height, ascent) = + crate::display_cursor::resolve_cursor_vertical_metrics( + cursor_row_pixel_y, + face_height, + face_ascent, + row.height_px, + row.ascent_px, + geometry.char_height, + row.ends_at_zv, + ); + cursor_row_pixel_y = y; + cursor_height = height; + cursor_ascent = ascent; } - let buffer_newline = buf_access - .byte_at(buf_access.charpos_to_bytepos(replay.new_point)) - == Some(b'\n'); - let Some((face_height, face_ascent)) = retained_point_cursor_metrics( - row, - replay.new_point as usize, - buffer_newline, - default_face.measurement_policy(), - ) else { - output.restore_retry_checkpoint(retry_checkpoint); - return BufferSourceRenderAttemptOutcome::ReplayMispredicted; - }; - let (y, height, ascent) = - crate::display_cursor::resolve_cursor_vertical_metrics( - cursor_row_pixel_y, - face_height, - face_ascent, - row.height_px, - row.ascent_px, - geometry.char_height, - row.ends_at_zv, - ); - cursor_row_pixel_y = y; - cursor_height = height; - cursor_ascent = ascent; } prev_row_bottom = row.pixel_y + row.height_px; } diff --git a/crates/neomacs-layout-engine/src/display_cursor.rs b/crates/neomacs-layout-engine/src/display_cursor.rs index 910090f4e1..9a90b5d838 100644 --- a/crates/neomacs-layout-engine/src/display_cursor.rs +++ b/crates/neomacs-layout-engine/src/display_cursor.rs @@ -368,8 +368,14 @@ pub(crate) enum CursorSlotResolutionState { /// measured layout geometry with an unrelated window-grid estimate. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(crate) enum ResolvedCursorSlot { - BufferGlyph(u16), - ReplacementString(u16), + BufferGlyph { + col: u16, + text_glyph: usize, + }, + ReplacementString { + col: u16, + text_glyph: usize, + }, /// Point is left of the horizontally visible source range, not merely /// hidden inside it. GNU clamps this cursor to the visible text edge. HorizontallyClipped(u16), @@ -481,9 +487,8 @@ struct CursorDisplayCol(u16); impl ResolvedCursorSlot { pub(crate) const fn col(self) -> u16 { match self { - Self::BufferGlyph(col) - | Self::ReplacementString(col) - | Self::HorizontallyClipped(col) + Self::BufferGlyph { col, .. } | Self::ReplacementString { col, .. } => col, + Self::HorizontallyClipped(col) | Self::FollowingVisibleGlyph(col) | Self::RowEnd(col) => col, } @@ -538,7 +543,12 @@ impl CursorVisualColumnResolutionRequest { } let rows = context.rows?; let row = rows.row(self.row)?; + Self::resolve_row_slot(row, self.charpos) + } + /// Select the materialized slot and source glyph together, before inspecting + /// its measurements. Bidi strings can share coverage but not cursor identity. + fn resolve_row_slot(row: &GlyphRow, point: usize) -> Option { let mut col_acc: u16 = 0; for glyph in &row.glyphs[GlyphArea::LeftMargin.index()] { if glyph.padding { @@ -592,27 +602,28 @@ impl CursorVisualColumnResolutionRequest { } let mut nearest_after: Option<(usize, u16)> = None; - let mut replacement_candidate: Option<(usize, u16)> = None; + let mut replacement_candidate: Option<(usize, u16, usize)> = None; let text_start_col = col_acc; let mut has_buffer_before_point = false; - for glyph in &text_glyphs[..text_end] { + for (text_glyph, glyph) in text_glyphs[..text_end].iter().enumerate() { if glyph.padding { continue; } match glyph.provenance { GlyphProvenance::Buffer { charpos } => { - has_buffer_before_point |= charpos < self.charpos; - if charpos == self.charpos { - return Some(ResolvedCursorSlot::BufferGlyph(col_acc)); + has_buffer_before_point |= charpos < point; + if charpos == point { + return Some(ResolvedCursorSlot::BufferGlyph { + col: col_acc, + text_glyph, + }); } - if charpos > self.charpos - && nearest_after.is_none_or(|(after, _)| charpos < after) - { + if charpos > point && nearest_after.is_none_or(|(after, _)| charpos < after) { nearest_after = Some((charpos, col_acc)); } } GlyphProvenance::Str { index, .. } - if row.glyph_covers_buffer_charpos(glyph, self.charpos) => + if row.glyph_covers_buffer_charpos(glyph, point) => { // GNU set_cursor_from_row Step 2 chooses the smallest // string index, not the first glyph in visual order (bidi @@ -620,9 +631,9 @@ impl CursorVisualColumnResolutionRequest { // carried once by the row's source occurrence instead of // recovered heuristically or duplicated on every glyph. if replacement_candidate - .is_none_or(|(candidate_index, _)| index < candidate_index) + .is_none_or(|(candidate_index, _, _)| index < candidate_index) { - replacement_candidate = Some((index, col_acc)); + replacement_candidate = Some((index, col_acc, text_glyph)); } } GlyphProvenance::Str { .. } @@ -641,8 +652,8 @@ impl CursorVisualColumnResolutionRequest { // rather than None keeps a blank/EOL cursor out of the line-number // gutter (where the captured Text-index 0 would land it), matching GNU // set_cursor_from_row placing the cursor in the empty area after a row. - Some(if let Some((_, col)) = replacement_candidate { - ResolvedCursorSlot::ReplacementString(col) + Some(if let Some((_, col, text_glyph)) = replacement_candidate { + ResolvedCursorSlot::ReplacementString { col, text_glyph } } else if let Some((_, col)) = nearest_after { if row.truncated_left && !has_buffer_before_point { ResolvedCursorSlot::HorizontallyClipped(text_start_col) @@ -654,6 +665,19 @@ impl CursorVisualColumnResolutionRequest { }) } + /// Return the exact covering point glyph selected for cursor placement. + /// Hidden-text/row-end approximations have no covering glyph to substitute. + pub(crate) fn point_glyph(row: &GlyphRow, point: usize) -> Option<&Glyph> { + let text_glyph = match Self::resolve_row_slot(row, point)? { + ResolvedCursorSlot::BufferGlyph { text_glyph, .. } + | ResolvedCursorSlot::ReplacementString { text_glyph, .. } => text_glyph, + ResolvedCursorSlot::HorizontallyClipped(_) + | ResolvedCursorSlot::FollowingVisibleGlyph(_) + | ResolvedCursorSlot::RowEnd(_) => return None, + }; + row.glyphs[GlyphArea::Text.index()].get(text_glyph) + } + #[cfg(test)] pub(crate) fn resolve(self, context: CursorVisualColumnResolutionContext<'_>) -> Option { self.resolve_slot(context).map(ResolvedCursorSlot::col) @@ -1459,14 +1483,25 @@ impl<'a> CapturedTextWindowCursorPublishContext<'a> { // Explicit image/stretch captures and synthetic newlines keep their // own contracts; unmeasured glyphs retain the fresh active-face fallback. if cursor.vertical_source == CursorVerticalSource::ActiveFace { - if let Some(row) = output.builder().current_window_row(row_metric.row()) { - if let Some(glyph) = row.glyphs[GlyphArea::Text.index()].iter().find(|glyph| { - glyph.pixel_height > 0.0 - && row - .glyph_covers_buffer_charpos(glyph, self.point_charpos.max(0) as usize) - }) { + let builder = output.builder(); + if let Some(row) = builder.current_window_row(row_metric.row()) { + if let Some(glyph) = CursorVisualColumnResolutionRequest::point_glyph( + row, + self.point_charpos.max(0) as usize, + ) + .filter(|glyph| glyph.pixel_height > 0.0) + { cursor.face_h = glyph.pixel_height; cursor.face_ascent = glyph.pixel_ascent; + if matches!(glyph.provenance, GlyphProvenance::Str { .. }) { + // A replacement's face and width can differ from the + // buffer face captured before rendering its string. + cursor.slot_width = Some(glyph.pixel_width); + if let Some(face) = builder.output_face(glyph.face_id) { + cursor.fg = face.foreground; + cursor.bg = face.background; + } + } } } } diff --git a/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs b/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs index e953eacb1a..45476f2fb3 100644 --- a/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs +++ b/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs @@ -413,6 +413,95 @@ fn mixed_height_newline_cursor_replay_matches_fresh_capture() { } } +/// Bidi replacement placement and vertical metrics must select the same glyph. +#[test] +fn rtl_replacement_cursor_uses_its_resolved_glyph_metrics() { + let (mut eval, frame, _buffer, window) = incr_editing_frame("x\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.eval_str("(progn (put-text-property 1 2 'display (concat (propertize \"א\" 'face '(:height 2.0 :foreground \"white\" :background \"red\")) (propertize \"בג\" 'face '(:height 0.5 :foreground \"black\" :background \"blue\")))) (set-window-point (selected-window) 1))").unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + let FrameLayoutAttempt::Prepared(prepared) = engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("replacement should prepare") + }; + let row = &prepared + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows[0]; + let glyphs = &row.glyphs[GlyphArea::Text.index()]; + let replacements = || { + glyphs.iter().filter(|glyph| { + !glyph.padding + && matches!( + glyph.provenance, + neomacs_display_protocol::glyph_matrix::GlyphProvenance::Str { .. } + ) + && row.glyph_covers_buffer_charpos(glyph, 0) + }) + }; + let first = replacements().next().expect("visual replacement glyph"); + let selected = replacements() + .min_by_key(|glyph| match glyph.provenance { + neomacs_display_protocol::glyph_matrix::GlyphProvenance::Str { index, .. } => index, + _ => unreachable!(), + }) + .expect("logical replacement glyph"); + assert_ne!( + first.pixel_height, selected.pixel_height, + "fixture must reorder differently sized glyphs" + ); + let expected_height = + (row.ascent_px + selected.pixel_height - selected.pixel_ascent).min(row.height_px); + let expected_ascent = row.ascent_px; + let expected_width = selected.pixel_width.max(1.0); + let output = prepared.materialize(); + let cursor = output.active_cursor().unwrap().clone(); + assert_eq!( + (cursor.height, cursor.ascent), + (expected_height, expected_ascent) + ); + assert_eq!(cursor.width, expected_width); + assert_eq!(cursor.cursor_fg, Color::from_pixel(0xff0000)); + for point in [2, 1] { + eval.eval_str(&format!("(set-window-point (selected-window) {point})")) + .unwrap(); + let FrameLayoutAttempt::Prepared(replayed) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("replacement replay should prepare") + }; + assert_eq!(engine.last_layout_stats().cursor_only_windows, 1); + assert!(engine.last_layout_stats().reused_rows > 0); + if point == 1 { + assert_eq!(replayed.materialize().active_cursor().unwrap(), &cursor); + } + } +} + +/// An unchanged authoritative cursor needs no new point-font measurement. +#[test] +fn unchanged_unmeasured_cursor_preserves_retained_replay() { + let (mut eval, frame, _buffer, _) = incr_editing_frame("text\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + let mut engine = crate::LayoutEngine::new_without_font_metrics(); + let FrameLayoutAttempt::Prepared(first) = engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("first frame should prepare") + }; + let first = first.materialize().active_cursor().unwrap().clone(); + let FrameLayoutAttempt::Prepared(second) = engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("unchanged frame should prepare") + }; + assert_eq!(engine.last_layout_stats().cursor_only_windows, 1); + assert!(engine.last_layout_stats().reused_rows > 0); + assert_eq!(second.materialize().active_cursor().unwrap(), &first); +} + /// A scroll retaining point's row must use its point glyph, not the whole row. #[test] fn mixed_height_scroll_cursor_matches_fresh_capture() { From db85cfb5537738ee3dce67ae17e6583caa0a683d Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sat, 7 Nov 2026 19:38:30 +0000 Subject: [PATCH 02/10] Fail RTL probe explicitly when Neomacs telemetry is missing --- .../neomacs-gui-tests/fixtures/cursor-rtl.el | 14 +++++-- .../neomacs-gui-tests/tests/commit_editing.rs | 42 +++++++++++++++++-- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/crates/neomacs-gui-tests/fixtures/cursor-rtl.el b/crates/neomacs-gui-tests/fixtures/cursor-rtl.el index d675d7f381..e432e4bf81 100644 --- a/crates/neomacs-gui-tests/fixtures/cursor-rtl.el +++ b/crates/neomacs-gui-tests/fixtures/cursor-rtl.el @@ -5,6 +5,14 @@ (princ (format "COMMIT-EDITING-FAIL %S\n" err) 'external-debugging-output) (kill-emacs 1)) +(defun neo-rtl-stats-size () + "Require Neomacs telemetry, returning nil only for the GNU reference." + (when (fboundp 'neomacs--frame-snapshot) + (let* ((path (getenv "NEOMACS_LAYOUT_STATS_FILE")) + (attributes (and path (file-attributes path)))) + (unless attributes (error "Missing layout telemetry: %S" path)) + (file-attribute-size attributes)))) + (run-at-time 10 nil (lambda () (neo-rtl-fail "watchdog expired"))) (switch-to-buffer (get-buffer-create "*rtl-cursor*")) (fundamental-mode) @@ -27,16 +35,14 @@ (error "RTL cursor must use logical first glyph's full height: %S/%S" cold height)) (goto-char 4) (redisplay t) - (let* ((path (and (fboundp 'neomacs--frame-snapshot) - (getenv "NEOMACS_LAYOUT_STATS_FILE"))) - (before (and path (file-attribute-size (file-attributes path))))) + (let ((before (neo-rtl-stats-size))) (goto-char 1) (redisplay t) (unless (equal (window-cursor-info) cold) (error "RTL cursor changed under replay: %S/%S" cold (window-cursor-info))) (if before (princ (format "CURSOR-REPLAY-STATS %d %d\n" before - (file-attribute-size (file-attributes path))) + (neo-rtl-stats-size)) 'external-debugging-output) (princ "CURSOR-REPLAY-REFERENCE-GEOMETRY-ONLY\n" 'external-debugging-output))) (princ "COMMIT-EDITING-PASS\n" 'external-debugging-output) diff --git a/crates/neomacs-gui-tests/tests/commit_editing.rs b/crates/neomacs-gui-tests/tests/commit_editing.rs index 3e463ce070..471e2b9dcc 100644 --- a/crates/neomacs-gui-tests/tests/commit_editing.rs +++ b/crates/neomacs-gui-tests/tests/commit_editing.rs @@ -37,6 +37,14 @@ fn run_fixture_probe(probe: &str, fixture: &str) { let program = std::env::var_os("NEOMACS_GUI_TEST_BINARY") .map(PathBuf::from) .unwrap_or_else(|| root.join("target/release/neomacs")); + let missing_telemetry = probe == "rtl-missing-telemetry"; + let stats_path = if missing_telemetry { + artifacts + .join(format!("missing-{}", std::process::id())) + .join("layout-stats.log") + } else { + artifacts.join("layout-stats.log") + }; let mut plan = GuiTestPlan::new( backend, &root, @@ -50,12 +58,24 @@ fn run_fixture_probe(probe: &str, fixture: &str) { .with_env("NEOMACS_COMMIT_PROBE", probe) .with_env( "NEOMACS_LAYOUT_STATS_FILE", - artifacts - .join("layout-stats.log") - .to_string_lossy() - .into_owned(), + stats_path.to_string_lossy().into_owned(), ) .with_env("RUST_LOG", "warn"); + if missing_telemetry { + assert!(!stats_path.exists()); + // Exercise Neomacs's mandatory-telemetry contract on either executable. + plan = plan.with_args([ + "-Q".to_string(), + "--eval".to_string(), + "(unless (fboundp 'neomacs--frame-snapshot) (fset 'neomacs--frame-snapshot #'ignore))" + .to_string(), + "-l".to_string(), + root.join("crates/neomacs-gui-tests/fixtures") + .join(fixture) + .to_string_lossy() + .into_owned(), + ]); + } for (key, value) in session.env() { plan = plan.with_env(key, value); } @@ -67,6 +87,15 @@ fn run_fixture_probe(probe: &str, fixture: &str) { .unwrap(); let stderr = fs::read_to_string(&result.artifacts.stderr).unwrap_or_default(); assert!(!result.timed_out, "{result:#?}\n{stderr}"); + if missing_telemetry { + assert_eq!(result.exit_code, Some(1), "{result:#?}\n{stderr}"); + assert!(stderr.contains("Missing layout telemetry"), "{stderr}"); + assert!( + !stderr.contains("CURSOR-REPLAY-REFERENCE-GEOMETRY-ONLY"), + "{stderr}" + ); + return; + } assert_eq!(result.exit_code, Some(0), "{result:#?}\n{stderr}"); assert!(stderr.contains("COMMIT-EDITING-PASS"), "{stderr}"); if (probe.ends_with("newline-cursor") || probe == "rtl-replacement-cursor") @@ -196,6 +225,11 @@ fn mixed_newline_cursor_matches_gnu_row_geometry() { } } +#[test] +fn rtl_missing_telemetry_is_an_explicit_failure() { + run_fixture_probe("rtl-missing-telemetry", "cursor-rtl.el"); +} + #[test] fn rtl_replacement_cursor_uses_one_glyph_for_placement_and_metrics() { run_fixture_probe("rtl-replacement-cursor", "cursor-rtl.el"); From 7cacf25bb5bbaa9e8d8914afccce88fccdee2b06 Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sat, 7 Nov 2026 20:13:45 +0000 Subject: [PATCH 03/10] Check buffer source before highlighting trailing whitespace --- .../fixtures/org-table-whitespace.el | 48 ++++++ .../neomacs-gui-tests/tests/commit_editing.rs | 5 + .../src/buffer_source/row_lifecycle.rs | 15 +- .../src/display_row/line_end.rs | 36 ++++- .../src/display_row/source_render.rs | 5 +- .../src/display_row/trailing_whitespace.rs | 55 ++++++- .../tests/trailing_whitespace_test.rs | 28 ++-- .../src/engine/tests/engine_test.rs | 5 + .../src/incremental_layout.rs | 14 ++ .../tests/engine_trailing_whitespace_test.rs | 140 ++++++++++++++++++ 10 files changed, 333 insertions(+), 18 deletions(-) create mode 100644 crates/neomacs-gui-tests/fixtures/org-table-whitespace.el create mode 100644 crates/neomacs-layout-engine/src/tests/engine_trailing_whitespace_test.rs diff --git a/crates/neomacs-gui-tests/fixtures/org-table-whitespace.el b/crates/neomacs-gui-tests/fixtures/org-table-whitespace.el new file mode 100644 index 0000000000..b62ae46914 --- /dev/null +++ b/crates/neomacs-gui-tests/fixtures/org-table-whitespace.el @@ -0,0 +1,48 @@ +;;; org-table-whitespace.el --- Synthetic table probe -*- lexical-binding: t; -*- +;;; No third-party package, SQL, or user file is loaded. +(defun neo-org-probe-fail (error-data) + (princ (format "COMMIT-EDITING-FAIL %S\n" error-data) 'external-debugging-output) + (kill-emacs 1)) +(run-at-time 10 nil (lambda () (neo-org-probe-fail "watchdog expired"))) +(condition-case error-data + (progn + (setq warning-minimum-level :emergency) + (require 'org) + (switch-to-buffer (get-buffer-create "*org-table-whitespace-probe*")) + (insert "| label |\n| alpha |\n| longer |\n\ncontrol \n") + (let ((enable-dir-local-variables nil)) (org-mode)) + (setq-local show-trailing-whitespace t) + (font-lock-ensure) + ;; org-modern--table uses exactly this display/face for vertical bars. + (goto-char (point-min)) + (while (search-forward "|" nil t) + (add-text-properties (1- (point)) (point) + '(display (space :width (3)) + face (:inherit org-table :inverse-video t)))) + (goto-char (point-min)) + (run-at-time + 1 nil + (lambda () + (condition-case error-data + (progn + (redisplay t) + (if (fboundp 'neomacs--frame-snapshot) + (let ((snapshot (neomacs--frame-snapshot nil 'text-faces)) + row highlighted) + (princ snapshot 'external-debugging-output) + (dolist (line (split-string snapshot "\n")) + (when (string-match "\\` *\\([0-9]+\\)|" line) + (setq row (string-to-number (match-string 1 line)))) + (when (string-match-p "trailing-whitespace" line) + (push row highlighted))) + (when (seq-some (lambda (row) (memq row '(0 1 2))) highlighted) + (error "Org table border/padding was classified as trailing whitespace: %S" highlighted)) + (unless (memq 4 highlighted) + (error "Genuine trailing spaces lost their highlight: %S" highlighted))) + ;; GNU lacks the snapshot API; check the same display source. + (unless (equal (get-text-property 1 'display) '(space :width (3))) + (error "Missing reference table-bar display property"))) + (princ "COMMIT-EDITING-PASS\n" 'external-debugging-output) + (kill-emacs 0)) + (error (neo-org-probe-fail error-data)))))) + (error (neo-org-probe-fail error-data))) diff --git a/crates/neomacs-gui-tests/tests/commit_editing.rs b/crates/neomacs-gui-tests/tests/commit_editing.rs index 471e2b9dcc..97be04e426 100644 --- a/crates/neomacs-gui-tests/tests/commit_editing.rs +++ b/crates/neomacs-gui-tests/tests/commit_editing.rs @@ -178,6 +178,11 @@ fn commit_probe_rejects_unknown_backend() { commit_probe_backend(Some("unknown")); } +#[test] +fn org_table_display_stretches_preserve_padding_and_border_faces() { + run_fixture_probe("org-table-whitespace", "org-table-whitespace.el"); +} + #[test] fn first_commit_character_uses_the_asynchronously_selected_buffer() { run_commit_probe("async-map"); diff --git a/crates/neomacs-layout-engine/src/buffer_source/row_lifecycle.rs b/crates/neomacs-layout-engine/src/buffer_source/row_lifecycle.rs index 1c83fce09a..b11f680937 100644 --- a/crates/neomacs-layout-engine/src/buffer_source/row_lifecycle.rs +++ b/crates/neomacs-layout-engine/src/buffer_source/row_lifecycle.rs @@ -2077,7 +2077,13 @@ impl<'a> BufferSourceLineBreakRenderRequest<'a> { fill_char_width: metrics.char_width(), indicator_char_width: context.default_char_width, }; - source_render.render_line_end(&line_end_ctx, line_end_geometry, face_ids); + source_render.render_line_end(&line_end_ctx, line_end_geometry, face_ids, &|from| { + crate::display_row::trailing_whitespace::buffer_trailing_whitespace_p( + buffer, + context.point_charpos, + from, + ) + }); } // The row ends at a real buffer newline, so it owns the columns past // its last glyph. The cell is the one GNU's `append_space_for_newline` @@ -2323,6 +2329,13 @@ impl<'a> BufferSourceLineBreakRenderRequest<'a> { indicator_char_width: context.default_char_width, }, face_ids, + &|from| { + crate::display_row::trailing_whitespace::buffer_trailing_whitespace_p( + buffer, + context.point_charpos, + from, + ) + }, ); } line_break_action.apply_before_row_transition( diff --git a/crates/neomacs-layout-engine/src/display_row/line_end.rs b/crates/neomacs-layout-engine/src/display_row/line_end.rs index 81657cf903..9f09c264cf 100644 --- a/crates/neomacs-layout-engine/src/display_row/line_end.rs +++ b/crates/neomacs-layout-engine/src/display_row/line_end.rs @@ -454,12 +454,37 @@ impl ResolvedLineEndPlan { /// Apply the resolved plan to `row` — the single line-end executor shared /// by the buffer-source lifecycle and the item-renderer finalizer. pub(crate) fn apply_to(self, row: &mut GlyphRow) { + // Bufferless producers cannot establish source trailing whitespace. + self.apply_with_source(row, &|_| false); + } + + /// Attach the buffer/point predicate only at execution, keeping the plan + /// independent of source storage and retaining the GNU effect ordering. + pub(crate) fn with_trailing_whitespace_source( + self, + source_is_trailing: &dyn Fn(neovm_core::buffer::CharPos0) -> bool, + ) -> SourceCheckedLineEndPlan<'_> { + SourceCheckedLineEndPlan { + plan: self, + source_is_trailing, + } + } + + fn apply_with_source( + self, + row: &mut GlyphRow, + source_is_trailing: &dyn Fn(neovm_core::buffer::CharPos0) -> bool, + ) { let text_index = GlyphArea::Text.index(); let before_len = row.glyphs[text_index].len(); for step in self.steps { match step { ResolvedLineEndStep::HighlightTrailingWhitespace { face_id } => { - HighlightTrailingWhitespaceMutation { face_id }.apply(row); + HighlightTrailingWhitespaceMutation { + face_id, + source_is_trailing, + } + .apply(row); } ResolvedLineEndStep::AppendGlyph { ch, @@ -490,11 +515,16 @@ impl ResolvedLineEndPlan { } } -impl DisplayCurrentRowMutation for ResolvedLineEndPlan { +pub(crate) struct SourceCheckedLineEndPlan<'a> { + plan: ResolvedLineEndPlan, + source_is_trailing: &'a dyn Fn(neovm_core::buffer::CharPos0) -> bool, +} + +impl DisplayCurrentRowMutation for SourceCheckedLineEndPlan<'_> { type Output = (); fn apply(self, row: &mut GlyphRow) -> Self::Output { - self.apply_to(row); + self.plan.apply_with_source(row, self.source_is_trailing); } } diff --git a/crates/neomacs-layout-engine/src/display_row/source_render.rs b/crates/neomacs-layout-engine/src/display_row/source_render.rs index f6da8edb85..6d9094eb31 100644 --- a/crates/neomacs-layout-engine/src/display_row/source_render.rs +++ b/crates/neomacs-layout-engine/src/display_row/source_render.rs @@ -1823,6 +1823,7 @@ impl<'a, FS: crate::neovm_bridge::FaceServices> TextRowSourceRenderState<'a, FS> ctx: &LineEndContext, geometry: LineEndFillGeometry, face_ids: &mut FrameFaceAttempt, + source_is_trailing: &dyn Fn(neovm_core::buffer::CharPos0) -> bool, ) { let plan = line_end::plan(ctx); if plan.steps().is_empty() { @@ -1837,7 +1838,9 @@ impl<'a, FS: crate::neovm_bridge::FaceServices> TextRowSourceRenderState<'a, FS> }; self.output_render .current_row_output() - .apply_current_row_mutation(resolved); + .apply_current_row_mutation( + resolved.with_trailing_whitespace_source(source_is_trailing), + ); } #[allow(clippy::too_many_arguments)] diff --git a/crates/neomacs-layout-engine/src/display_row/trailing_whitespace.rs b/crates/neomacs-layout-engine/src/display_row/trailing_whitespace.rs index 507974b065..1221862525 100644 --- a/crates/neomacs-layout-engine/src/display_row/trailing_whitespace.rs +++ b/crates/neomacs-layout-engine/src/display_row/trailing_whitespace.rs @@ -10,13 +10,43 @@ //! glyph provenance carries the same discriminator, so this mutation can //! require the `Buffer` arm directly instead of inferring it from a sentinel. //! +//! A stretch can also represent a non-whitespace character (for example an +//! org-modern table border). The last candidate must first pass GNU's source +//! predicate; string-owned glyphs block the walk instead of being skipped. +//! //! WHEN the highlight runs is not decided here: [`super::line_end::plan`] is //! the one place that orders it (first, at a true line end only) for every //! row producer. use crate::display_current_row_output::DisplayCurrentRowMutation; -use neomacs_display_protocol::glyph_matrix::{Glyph, GlyphArea, GlyphRow, GlyphType}; +use neomacs_display_protocol::glyph_matrix::{ + Glyph, GlyphArea, GlyphProvenance, GlyphRow, GlyphType, +}; use neomacs_display_protocol::types::FaceId; +use neovm_core::buffer::{CharPos0, EmacsBytePos}; + +/// GNU `trailing_whitespace_p`: inspect actual accessible buffer bytes, not +/// the shape of a display replacement, and suppress the run ending at point. +/// The predicate is asked once for the last candidate, before the row walk. +pub(crate) fn buffer_trailing_whitespace_p( + buffer: &impl crate::neovm_bridge::LayoutBufferView, + point: i64, + from: CharPos0, +) -> bool { + if from > buffer.layout_point_max_char_pos() { + return false; + } + let end = buffer.layout_point_max_emacs_byte_pos(); + let mut pos = buffer.layout_char_pos_to_emacs_byte_pos(from); + if pos < buffer.layout_point_min_emacs_byte_pos() { + return false; + } + while pos < end && matches!(buffer.layout_emacs_byte_at_pos(pos), Some(b' ' | b'\t')) { + pos = EmacsBytePos::new(pos.get() + 1); + } + (pos == end || matches!(buffer.layout_emacs_byte_at_pos(pos), Some(b'\n' | b'\r'))) + && buffer.layout_emacs_byte_pos_to_char_pos(pos).get() as i64 != point +} /// Re-face the current row's trailing whitespace glyphs with the /// `trailing-whitespace` face (GNU `highlight_trailing_whitespace`, xdisp.c). @@ -27,8 +57,9 @@ use neomacs_display_protocol::types::FaceId; /// resolved from its `face_id`, so this paints the trailing run through the /// same per-glyph background path the `region` face uses. Called only at true /// line ends (before a real newline / at ZV), never at a visual wrap. -pub(super) struct HighlightTrailingWhitespaceMutation { +pub(super) struct HighlightTrailingWhitespaceMutation<'a> { pub(super) face_id: FaceId, + pub(super) source_is_trailing: &'a dyn Fn(CharPos0) -> bool, } /// GNU also accepts only space `Char` glyphs (tabs render as `Stretch`); the @@ -46,7 +77,7 @@ fn has_buffer_position(glyph: &Glyph) -> bool { glyph.provenance.buffer_charpos().is_some() } -impl DisplayCurrentRowMutation for HighlightTrailingWhitespaceMutation { +impl DisplayCurrentRowMutation for HighlightTrailingWhitespaceMutation<'_> { type Output = (); fn apply(self, row: &mut GlyphRow) -> Self::Output { @@ -62,11 +93,27 @@ impl DisplayCurrentRowMutation for HighlightTrailingWhitespaceMutation { glyph.glyph_type, GlyphType::Char { .. } | GlyphType::Stretch { .. } ); - if !skippable || has_buffer_position(glyph) { + let nil_object = match glyph.provenance { + GlyphProvenance::Buffer { .. } | GlyphProvenance::Str { .. } => false, + GlyphProvenance::LineEnd + | GlyphProvenance::Mark + | GlyphProvenance::LeftTruncation + | GlyphProvenance::EmptyLineNewline { .. } => true, + }; + if !skippable || !nil_object { break; } end -= 1; } + let Some(last) = end.checked_sub(1).map(|index| &glyphs[index]) else { + return; + }; + let Some(charpos) = last.provenance.buffer_charpos() else { + return; + }; + if !is_whitespace_glyph(last) || !(self.source_is_trailing)(CharPos0::new(charpos)) { + return; + } // Re-face backward while the glyph is buffer whitespace (GNU's // `BUFFERP (glyph->object)` plus space/stretch condition). let mut start = end; diff --git a/crates/neomacs-layout-engine/src/display_row/trailing_whitespace/tests/trailing_whitespace_test.rs b/crates/neomacs-layout-engine/src/display_row/trailing_whitespace/tests/trailing_whitespace_test.rs index aa06362004..24b4f6b5d6 100644 --- a/crates/neomacs-layout-engine/src/display_row/trailing_whitespace/tests/trailing_whitespace_test.rs +++ b/crates/neomacs-layout-engine/src/display_row/trailing_whitespace/tests/trailing_whitespace_test.rs @@ -18,6 +18,16 @@ use neomacs_display_protocol::types::FaceId; const TEXT_FACE: FaceId = FaceId::new(1); const TWS_FACE: FaceId = FaceId::new(42); +// These mutation-only cases supply an already-verified source predicate. +// End-to-end tests exercise the actual buffer/point classification. +fn highlight(row: &mut GlyphRow) { + HighlightTrailingWhitespaceMutation { + face_id: TWS_FACE, + source_is_trailing: &|_| true, + } + .apply(row); +} + fn push_char(row: &mut GlyphRow, ch: char, charpos: usize) { row.glyphs[GlyphArea::Text.index()].push(Glyph::char(ch, TEXT_FACE, charpos)); } @@ -51,7 +61,7 @@ fn trailing_spaces_are_refaced_leading_text_kept() { push_char(&mut row, ch, i); } - HighlightTrailingWhitespaceMutation { face_id: TWS_FACE }.apply(&mut row); + highlight(&mut row); assert_eq!( face_ids(&row), @@ -66,7 +76,7 @@ fn trailing_tab_stretch_is_refaced() { push_char(&mut row, 'x', 0); push_tab(&mut row); - HighlightTrailingWhitespaceMutation { face_id: TWS_FACE }.apply(&mut row); + highlight(&mut row); assert_eq!( face_ids(&row), @@ -83,7 +93,7 @@ fn interior_whitespace_is_not_refaced() { push_char(&mut row, ch, i); } - HighlightTrailingWhitespaceMutation { face_id: TWS_FACE }.apply(&mut row); + highlight(&mut row); assert_eq!( face_ids(&row), @@ -104,7 +114,7 @@ fn line_with_no_trailing_whitespace_is_unchanged() { push_char(&mut row, ch, i); } - HighlightTrailingWhitespaceMutation { face_id: TWS_FACE }.apply(&mut row); + highlight(&mut row); assert_eq!( face_ids(&row), @@ -124,7 +134,7 @@ fn appended_positionless_newline_space_is_skipped_but_buffer_run_refaced() { } push_positionless_space(&mut row); - HighlightTrailingWhitespaceMutation { face_id: TWS_FACE }.apply(&mut row); + highlight(&mut row); assert_eq!( face_ids(&row), @@ -140,7 +150,7 @@ fn positionless_extend_stretch_at_row_end_is_skipped() { push_char(&mut row, ' ', 1); push_positionless_stretch(&mut row); - HighlightTrailingWhitespaceMutation { face_id: TWS_FACE }.apply(&mut row); + highlight(&mut row); assert_eq!( face_ids(&row), @@ -159,7 +169,7 @@ fn refacing_stops_at_glyph_without_buffer_position() { push_positionless_space(&mut row); push_char(&mut row, ' ', 2); - HighlightTrailingWhitespaceMutation { face_id: TWS_FACE }.apply(&mut row); + highlight(&mut row); assert_eq!( face_ids(&row), @@ -174,7 +184,7 @@ fn row_of_only_positionless_glyphs_is_unchanged() { push_positionless_space(&mut row); push_positionless_stretch(&mut row); - HighlightTrailingWhitespaceMutation { face_id: TWS_FACE }.apply(&mut row); + highlight(&mut row); assert_eq!( face_ids(&row), @@ -190,7 +200,7 @@ fn all_whitespace_row_is_fully_refaced() { push_tab(&mut row); push_char(&mut row, ' ', 1); - HighlightTrailingWhitespaceMutation { face_id: TWS_FACE }.apply(&mut row); + highlight(&mut row); assert_eq!( face_ids(&row), diff --git a/crates/neomacs-layout-engine/src/engine/tests/engine_test.rs b/crates/neomacs-layout-engine/src/engine/tests/engine_test.rs index 91d5c2e1d7..c1c270a0f4 100644 --- a/crates/neomacs-layout-engine/src/engine/tests/engine_test.rs +++ b/crates/neomacs-layout-engine/src/engine/tests/engine_test.rs @@ -76,6 +76,8 @@ mod font_selection; #[path = "../../tests/engine_line_spacing_test.rs"] mod line_spacing; +#[path = "../../tests/engine_trailing_whitespace_test.rs"] +mod trailing_whitespace; #[path = "../../tests/engine_display_motion_test.rs"] mod display_motion; @@ -34207,6 +34209,9 @@ fn p45_trailing_whitespace_shadow_survives_a_wrapped_row_boundary() { // fits and EVERY glyph on both rows — the trailing spaces included — is // appended through the per-char path this rung rewires. let trace = layout_trace_with_buffer_setup("aaaaaaaa \n", 88, 240, |buffer, _, _| { + // Fragmented insertion leaves point at the newline. GNU suppresses + // that run there; move away to exercise the intended highlight path. + buffer.set_point_anchor(neovm_core::buffer::TextPositionAnchor::ZERO); buffer.set_buffer_local("truncate-lines", Value::NIL); buffer.set_buffer_local("show-trailing-whitespace", Value::T); }); diff --git a/crates/neomacs-layout-engine/src/incremental_layout.rs b/crates/neomacs-layout-engine/src/incremental_layout.rs index 94afe0b935..62d3a2d77f 100644 --- a/crates/neomacs-layout-engine/src/incremental_layout.rs +++ b/crates/neomacs-layout-engine/src/incremental_layout.rs @@ -399,6 +399,9 @@ impl RetainedWindowKey { /// properties, overlays, faces, or the viewport changed, so the retained body /// rows are no longer trustworthy). pub fn cursor_only_eligible(prev: &Self, curr: &Self) -> bool { + if !Self::trailing_whitespace_unchanged_by_point_motion(prev, curr) { + return false; + } // Nothing moved except possibly point. // // Note what this now tolerates that it did not before: an overlay set @@ -411,12 +414,23 @@ impl RetainedWindowKey { WindowDelta::between(prev, curr).only_point_moved_at_most() } + /// Highlighting suppresses the whitespace run ending at point. A moved + /// point can therefore change body faces despite unchanged source text. + /// GNU likewise refuses these shortcuts with show-trailing-whitespace + /// (xdisp.c:19880,21804); unchanged-point body reuse remains safe here. + fn trailing_whitespace_unchanged_by_point_motion(prev: &Self, curr: &Self) -> bool { + !curr.show_trailing_whitespace || prev.point == curr.point + } + /// Whether a window may take the pure-scroll fast path: every layout input is /// identical to the retained (`prev`) key EXCEPT `window_start` (which moved) /// and `point` (which may follow the scroll). Any tick/geometry/face move /// escalates to a full rebuild. Whether the scroll is by WHOLE rows is decided /// separately against the retained matrix ([`RetainedWindowMatrix::scroll_replay`]). pub fn scroll_eligible(prev: &Self, curr: &Self) -> bool { + if !Self::trailing_whitespace_unchanged_by_point_motion(prev, curr) { + return false; + } let delta = WindowDelta::between(prev, curr); // The visible region moved and nothing else did. delta.window_start_moved diff --git a/crates/neomacs-layout-engine/src/tests/engine_trailing_whitespace_test.rs b/crates/neomacs-layout-engine/src/tests/engine_trailing_whitespace_test.rs new file mode 100644 index 0000000000..a29dc61645 --- /dev/null +++ b/crates/neomacs-layout-engine/src/tests/engine_trailing_whitespace_test.rs @@ -0,0 +1,140 @@ +//! GNU trailing-whitespace classification must inspect source, not stretch shape. +use super::*; + +fn highlighted(text: &str, replacement: Option<&str>, point: Option) -> Vec { + let trace = layout_trace_with_buffer_setup(text, 800, 240, |buffer, _, _| { + buffer.set_buffer_local("show-trailing-whitespace", Value::T); + if let Some(replacement) = replacement { + let display = if replacement == "stretch" { + table_bar() + } else { + Value::string(replacement) + }; + let bar = text.rfind('|').unwrap(); + assert!(buffer.put_text_property(bar, bar + 1, Value::symbol("display"), display)); + } + { + let point = point.unwrap_or(0); + buffer.set_point_anchor(neovm_core::buffer::TextPositionAnchor::new( + neovm_core::buffer::CharPos0::new(point), + neovm_core::buffer::EmacsBytePos::new(point), + )); + } + }); + trace + .matrix_rows + .iter() + .filter(|row| row.role == GlyphRowRole::Text && !row.mode_line) + .flat_map(|row| row.glyph_areas[1].iter()) + .filter(|glyph| glyph.face.contains("trailing-whitespace")) + .map(|glyph| glyph.charpos) + .collect() +} + +#[test] +fn trailing_source_predicate_respects_cr_zv_narrowing_and_character_coordinates() { + use crate::display_row::trailing_whitespace::buffer_trailing_whitespace_p; + use neovm_core::buffer::CharPos0; + for (text, narrowed, from, point, expected) in [ + ("é \rhidden\n", false, 1, 0, true), + ("é \rhidden\n", false, 1, 3, false), + ("é \rhidden\n", false, 0, 0, false), + ("x ", false, 1, 0, true), + ("x ", false, 1, 3, false), + ("x hidden", true, 1, 0, true), + ("x hidden", false, 1, 0, false), + ] { + let mut eval = Context::new(); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .insert(text); + if narrowed { + eval.eval_str("(narrow-to-region 1 4)").unwrap(); + } + let buffer = eval.buffer_manager().current_buffer().unwrap(); + assert_eq!( + buffer_trailing_whitespace_p(buffer, point, CharPos0::new(from)), + expected, + "text={text:?} narrowed={narrowed} from={from} point={point}" + ); + } +} + +#[test] +fn trailing_whitespace_point_motion_keeps_reused_rows_equal_to_fresh() { + let positions = |trace: BackendLayoutTrace| -> Vec { + trace + .matrix_rows + .iter() + .filter(|row| row.role == GlyphRowRole::Text && !row.mode_line) + .flat_map(|row| row.glyph_areas[1].iter()) + .filter(|glyph| glyph.face.contains("trailing-whitespace")) + .map(|glyph| glyph.charpos) + .collect() + }; + for scroll in [false, true] { + let (mut eval, frame, _, _) = incr_editing_frame(&"x \n".repeat(30), 800, 240); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("show-trailing-whitespace", Value::T); + eval.eval_str("(goto-char 1)").unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + engine.layout_frame_rust(&mut eval, frame); + for point in if scroll { [44, 41] } else { [4, 1] } { + eval.eval_str(&format!("(set-window-point (selected-window) {point})")) + .unwrap(); + if scroll { + eval.eval_str("(set-window-start (selected-window) 5 t)") + .unwrap(); + } + engine.layout_frame_rust(&mut eval, frame); + let actual = positions(selected_window_layout_trace(&eval, &engine, frame)); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + fresh.layout_frame_rust(&mut eval, frame); + let expected = positions(selected_window_layout_trace(&eval, &fresh, frame)); + assert_eq!(actual, expected, "scroll={scroll} point={point}"); + } + } +} + +fn table_bar() -> Value { + Value::list(vec![ + Value::symbol("space"), + Value::keyword("width"), + Value::list(vec![Value::fixnum(3)]), + ]) +} + +#[test] +fn display_stretches_do_not_turn_org_table_padding_into_trailing_whitespace() { + for text in ["| alpha |\n", "| café |\n", "x |\n"] { + assert!( + highlighted(text, Some("stretch"), None).is_empty(), + "{text:?}" + ); + } + // Unlike nil-object end padding, a displayed string must block the scan. + for display in [" ", "word"] { + assert!( + highlighted("x |\n", Some(display), None).is_empty(), + "{display:?}" + ); + } +} + +#[test] +fn actual_trailing_spaces_and_tabs_keep_gnu_highlighting() { + assert_eq!(highlighted("x \n", None, None), vec![1, 2]); + assert_eq!(highlighted("x \t\n", None, None), vec![1, 2]); + // GNU checks the LAST candidate's source, then paints the whole glyph run. + // A display stretch before genuine trailing spaces still belongs to it. + assert_eq!(highlighted("x| \n", Some("stretch"), None), vec![1, 2, 3]); +} + +#[test] +fn trailing_whitespace_is_suppressed_when_point_is_at_its_endpoint() { + assert!(highlighted("x \n", None, Some(3)).is_empty()); + assert_eq!(highlighted("x \n", None, Some(2)), vec![1, 2]); +} From bdfa8a06bfede0cef15ac76c355749b89f0699b6 Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sat, 7 Nov 2026 21:47:30 +0000 Subject: [PATCH 04/10] Invalidate point-dependent trailing whitespace on edit replay --- .../src/incremental_layout.rs | 6 ++ .../tests/engine_trailing_whitespace_test.rs | 69 +++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/crates/neomacs-layout-engine/src/incremental_layout.rs b/crates/neomacs-layout-engine/src/incremental_layout.rs index 62d3a2d77f..2eb327c753 100644 --- a/crates/neomacs-layout-engine/src/incremental_layout.rs +++ b/crates/neomacs-layout-engine/src/incremental_layout.rs @@ -451,6 +451,12 @@ impl RetainedWindowKey { /// modiff (xdisp.c GIVE_UP 200). An overlay/face move still escalates to /// a full rebuild. `point` may also move with the edit. pub fn edit_eligible(prev: &Self, curr: &Self) -> bool { + // Even an unchanged numeric point can change which source endpoint it + // suppresses when an edit above shifts a reused row. GNU try_window_id + // refuses this shortcut outright with trailing whitespace (xdisp:22653). + if curr.show_trailing_whitespace { + return false; + } let delta = WindowDelta::between(prev, curr); // Something about the TEXT changed -- characters or properties -- and // nothing else did. An unchanged buffer is the cursor-only path's diff --git a/crates/neomacs-layout-engine/src/tests/engine_trailing_whitespace_test.rs b/crates/neomacs-layout-engine/src/tests/engine_trailing_whitespace_test.rs index a29dc61645..2d8e728fde 100644 --- a/crates/neomacs-layout-engine/src/tests/engine_trailing_whitespace_test.rs +++ b/crates/neomacs-layout-engine/src/tests/engine_trailing_whitespace_test.rs @@ -1,6 +1,9 @@ //! GNU trailing-whitespace classification must inspect source, not stretch shape. use super::*; +/// Lay out real source and optional display replacements, returning the buffer +/// positions painted with the trailing-whitespace face. Default point is zero +/// so positive controls do not accidentally suppress their own endpoint. fn highlighted(text: &str, replacement: Option<&str>, point: Option) -> Vec { let trace = layout_trace_with_buffer_setup(text, 800, 240, |buffer, _, _| { buffer.set_buffer_local("show-trailing-whitespace", Value::T); @@ -31,6 +34,8 @@ fn highlighted(text: &str, replacement: Option<&str>, point: Option) -> V .collect() } +/// Source classification uses character coordinates, accessible bounds and GNU +/// CR/ZV termination, rather than treating missing bytes or display ends as EOF. #[test] fn trailing_source_predicate_respects_cr_zv_narrowing_and_character_coordinates() { use crate::display_row::trailing_whitespace::buffer_trailing_whitespace_p; @@ -61,6 +66,8 @@ fn trailing_source_predicate_respects_cr_zv_narrowing_and_character_coordinates( } } +/// Both cursor-only and scrolling reuse must reproduce fresh endpoint-dependent +/// faces after point leaves or enters a whitespace run. #[test] fn trailing_whitespace_point_motion_keeps_reused_rows_equal_to_fresh() { let positions = |trace: BackendLayoutTrace| -> Vec { @@ -99,6 +106,62 @@ fn trailing_whitespace_point_motion_keeps_reused_rows_equal_to_fresh() { } } +/// Edits can invalidate an earlier suppressed run even if point's numeric +/// position stays unchanged; ordinary non-highlighting edits still reuse rows. +#[test] +fn trailing_whitespace_edit_replay_does_not_retain_point_dependent_faces() { + let positions = |trace: BackendLayoutTrace| -> Vec { + trace + .matrix_rows + .iter() + .filter(|row| row.role == GlyphRowRole::Text && !row.mode_line) + .flat_map(|row| row.glyph_areas[1].iter()) + .filter(|glyph| glyph.face.contains("trailing-whitespace")) + .map(|glyph| glyph.charpos) + .collect() + }; + for show in [false, true] { + for moved in [false, true] { + let (mut eval, frame, _, _) = incr_editing_frame(&"x \n".repeat(30), 800, 240); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local( + "show-trailing-whitespace", + if show { Value::T } else { Value::NIL }, + ); + eval.eval_str(if moved { + "(goto-char 4)" + } else { + "(goto-char 12)" + }) + .unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + engine.layout_frame_rust(&mut eval, frame); + // Either leave the old endpoint and edit below it, or keep the same + // numeric point while an insertion above shifts its former row. + eval.eval_str(if moved { + "(progn (goto-char 21) (insert \"z\"))" + } else { + "(progn (goto-char 1) (insert \"z\") (goto-char 12))" + }) + .unwrap(); + engine.layout_frame_rust(&mut eval, frame); + let actual = positions(selected_window_layout_trace(&eval, &engine, frame)); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + fresh.layout_frame_rust(&mut eval, frame); + let expected = positions(selected_window_layout_trace(&eval, &fresh, frame)); + assert_eq!(actual, expected, "show={show} moved={moved}"); + assert_eq!( + engine.last_layout_stats().edit_windows, + usize::from(!show), + "show={show} moved={moved}" + ); + } + } +} + +/// Model org-modern's visible border without depending on third-party packages. fn table_bar() -> Value { Value::list(vec![ Value::symbol("space"), @@ -107,6 +170,8 @@ fn table_bar() -> Value { ]) } +/// Display appearance is not source whitespace; string-owned glyphs block the +/// backward scan rather than behaving like nil-object synthetic padding. #[test] fn display_stretches_do_not_turn_org_table_padding_into_trailing_whitespace() { for text in ["| alpha |\n", "| café |\n", "x |\n"] { @@ -124,6 +189,8 @@ fn display_stretches_do_not_turn_org_table_padding_into_trailing_whitespace() { } } +/// Genuine spaces/tabs still highlight, including a preceding displayed stretch +/// licensed by GNU's single last-candidate source check. #[test] fn actual_trailing_spaces_and_tabs_keep_gnu_highlighting() { assert_eq!(highlighted("x \n", None, None), vec![1, 2]); @@ -133,6 +200,8 @@ fn actual_trailing_spaces_and_tabs_keep_gnu_highlighting() { assert_eq!(highlighted("x| \n", Some("stretch"), None), vec![1, 2, 3]); } +/// Only point at the end of the complete source run suppresses highlighting; +/// point inside that run must leave its face intact. #[test] fn trailing_whitespace_is_suppressed_when_point_is_at_its_endpoint() { assert!(highlighted("x \n", None, Some(3)).is_empty()); From bcac2dccf15733109a677b3f1f5d64861b8f6917 Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sat, 7 Nov 2026 21:48:17 +0000 Subject: [PATCH 05/10] Unify cursor width policy and finalized logical anchors --- .../fixtures/cursor-stretch-policy.el | 55 ++++++++ .../fixtures/org-table-whitespace.el | 11 +- .../neomacs-gui-tests/tests/commit_editing.rs | 31 ++++- .../src/buffer_source/render_plan.rs | 26 ++-- .../src/display_cursor.rs | 81 ++++++++--- .../src/tests/engine_line_spacing_test.rs | 129 ++++++++++++++++++ .../src/window_output.rs | 25 +++- 7 files changed, 319 insertions(+), 39 deletions(-) create mode 100644 crates/neomacs-gui-tests/fixtures/cursor-stretch-policy.el diff --git a/crates/neomacs-gui-tests/fixtures/cursor-stretch-policy.el b/crates/neomacs-gui-tests/fixtures/cursor-stretch-policy.el new file mode 100644 index 0000000000..f155d45c45 --- /dev/null +++ b/crates/neomacs-gui-tests/fixtures/cursor-stretch-policy.el @@ -0,0 +1,55 @@ +;;; cursor-stretch-policy.el --- Fresh/replay width policy -*- lexical-binding: t; -*- +(defun neo-stretch-fail (error-data) + "Report probe failure and terminate, including asynchronous timer errors." + (princ (format "COMMIT-EDITING-FAIL %S\n" error-data) 'external-debugging-output) + (kill-emacs 1)) +(defun neo-stretch-stats-size () + "Require telemetry on Neomacs; GNU is explicitly geometry-only." + (when (fboundp 'neomacs--frame-snapshot) + (let* ((path (getenv "NEOMACS_LAYOUT_STATS_FILE")) + (attributes (and path (file-attributes path)))) + (unless attributes (error "Missing layout telemetry")) + (file-attribute-size attributes)))) +(run-at-time 10 nil (lambda () (neo-stretch-fail "watchdog expired"))) +(condition-case error-data + (let* ((probe (or (getenv "NEOMACS_COMMIT_PROBE") "")) + (narrow (string-match-p "narrow" probe)) + (enabled (string-match-p "enabled" probe)) + (width (if narrow 0.25 5))) + (setq warning-minimum-level :emergency x-stretch-cursor (and enabled t)) + (blink-cursor-mode -1) + (set-frame-parameter nil 'cursor-type 'box) + (switch-to-buffer (get-buffer-create "*stretch-cursor-policy*")) + (fundamental-mode) + (insert "אxב\n") + (put-text-property 2 3 'display (list 'space :width width)) + (goto-char 2) + (run-at-time + 0.5 nil + (lambda () + (condition-case error-data + (progn + (redisplay t) + (let ((cold (window-cursor-info))) + (goto-char 1) + (redisplay t) + (let ((before (neo-stretch-stats-size))) + (goto-char 2) + (redisplay t) + (let* ((after (neo-stretch-stats-size)) + (replayed (window-cursor-info)) + (actual (aref replayed 3)) + (column (frame-char-width))) + (unless (= (aref cold 3) actual) + (error "Stretch width changed on replay: cold=%S replay=%S" cold replayed)) + (unless (and (> actual 0) + (if narrow (< actual column) + (if enabled (> actual column) (<= actual column)))) + (error "Invalid stretch-width policy: enabled=%S narrow=%S width=%S column=%S" enabled narrow actual column)) + (if before + (princ (format "CURSOR-REPLAY-STATS %d %d\n" before after) 'external-debugging-output) + (princ "CURSOR-REPLAY-REFERENCE-GEOMETRY-ONLY\n" 'external-debugging-output)) + (princ "COMMIT-EDITING-PASS\n" 'external-debugging-output) + (kill-emacs 0))))) + (error (neo-stretch-fail error-data)))))) + (error (neo-stretch-fail error-data))) diff --git a/crates/neomacs-gui-tests/fixtures/org-table-whitespace.el b/crates/neomacs-gui-tests/fixtures/org-table-whitespace.el index b62ae46914..7d63065a35 100644 --- a/crates/neomacs-gui-tests/fixtures/org-table-whitespace.el +++ b/crates/neomacs-gui-tests/fixtures/org-table-whitespace.el @@ -38,10 +38,13 @@ (when (seq-some (lambda (row) (memq row '(0 1 2))) highlighted) (error "Org table border/padding was classified as trailing whitespace: %S" highlighted)) (unless (memq 4 highlighted) - (error "Genuine trailing spaces lost their highlight: %S" highlighted))) - ;; GNU lacks the snapshot API; check the same display source. - (unless (equal (get-text-property 1 'display) '(space :width (3))) - (error "Missing reference table-bar display property"))) + (error "Genuine trailing spaces lost their highlight: %S" highlighted)) + (princ "ORG-TABLE-RENDERED-FACES-PASS\n" 'external-debugging-output)) + ;; This is explicitly source-only, not a GNU face-color oracle. + (progn + (unless (equal (get-text-property 1 'display) '(space :width (3))) + (error "Missing reference table-bar display property")) + (princ "ORG-TABLE-REFERENCE-SOURCE-ONLY\n" 'external-debugging-output))) (princ "COMMIT-EDITING-PASS\n" 'external-debugging-output) (kill-emacs 0)) (error (neo-org-probe-fail error-data)))))) diff --git a/crates/neomacs-gui-tests/tests/commit_editing.rs b/crates/neomacs-gui-tests/tests/commit_editing.rs index 97be04e426..2f83ce2e04 100644 --- a/crates/neomacs-gui-tests/tests/commit_editing.rs +++ b/crates/neomacs-gui-tests/tests/commit_editing.rs @@ -98,7 +98,20 @@ fn run_fixture_probe(probe: &str, fixture: &str) { } assert_eq!(result.exit_code, Some(0), "{result:#?}\n{stderr}"); assert!(stderr.contains("COMMIT-EDITING-PASS"), "{stderr}"); - if (probe.ends_with("newline-cursor") || probe == "rtl-replacement-cursor") + if probe == "org-table-whitespace" { + let rendered = stderr.contains("ORG-TABLE-RENDERED-FACES-PASS"); + let source_only = stderr.contains("ORG-TABLE-REFERENCE-SOURCE-ONLY"); + assert_ne!( + rendered, source_only, + "missing or ambiguous Org evidence: {stderr}" + ); + if source_only { + eprintln!("Org reference control: source setup only; rendered faces were not asserted"); + } + } + if (probe.ends_with("newline-cursor") + || probe == "rtl-replacement-cursor" + || probe.starts_with("stretch-policy-")) && !stderr.contains("CURSOR-REPLAY-REFERENCE-GEOMETRY-ONLY") { let marker = stderr @@ -178,11 +191,25 @@ fn commit_probe_rejects_unknown_backend() { commit_probe_backend(Some("unknown")); } +/// Neomacs verifies rendered faces; GNU explicitly reports a source-only control. #[test] -fn org_table_display_stretches_preserve_padding_and_border_faces() { +fn org_table_probe_reports_rendered_faces_or_reference_source() { run_fixture_probe("org-table-whitespace", "org-table-whitespace.el"); } +/// Fresh and actual cursor-only replay obey narrow/wide stretch policy in RTL. +#[test] +fn stretch_cursor_replay_obeys_width_policy() { + for probe in [ + "stretch-policy-narrow", + "stretch-policy-wide", + "stretch-policy-narrow-enabled", + "stretch-policy-wide-enabled", + ] { + run_fixture_probe(probe, "cursor-stretch-policy.el"); + } +} + #[test] fn first_commit_character_uses_the_asynchronously_selected_buffer() { run_commit_probe("async-map"); diff --git a/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs b/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs index 52258fa277..fa7b32e3ca 100644 --- a/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs +++ b/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs @@ -459,15 +459,13 @@ fn try_decorate_window_cursor( if let Some(glyph) = crate::display_cursor::CursorVisualColumnResolutionRequest::point_glyph(row, point) { - // Preserve the GNU stretch/bar cursor width policy. - width = if matches!(glyph.glyph_type, GlyphType::Stretch { .. }) - && !params.x_stretch_cursor - && !matches!(style, CursorStyle::Bar(_)) - { - crate::display_row::width::DisplayRowCharWidthPolicy::new(char_w).fallback() - } else { - glyph.pixel_width - }; + width = crate::display_cursor::resolve_cursor_width( + style, + glyph.pixel_width, + matches!(glyph.glyph_type, GlyphType::Stretch { .. }), + params.x_stretch_cursor, + char_w, + ); } let Some((face_height, face_ascent)) = retained_point_cursor_metrics(row, point, buffer_newline, measurement) @@ -937,6 +935,7 @@ impl BufferSourceOutputSetup { // canonical cursor geometry rule. Width comes from the point glyph. // Fall back to row metrics at EOL or hidden text without a glyph. let mut cursor_width = geometry.char_width; + let mut cursor_stretch_like = false; let mut cursor_height = geometry.char_height; let mut cursor_ascent = window_metrics.ascent(); let mut cursor_row_pixel_y = 0.0_f32; @@ -963,6 +962,7 @@ impl BufferSourceOutputSetup { ) { cursor_width = glyph.pixel_width; + cursor_stretch_like = matches!(glyph.glyph_type, GlyphType::Stretch { .. }); } if replay.retained_cursor.is_none() { let buffer_newline = buf_access @@ -1090,7 +1090,13 @@ impl BufferSourceOutputSetup { }, x: walk_setup.text_area_left, y: walk_setup.window_top + cursor_row_pixel_y, - width: cursor_width, + width: crate::display_cursor::resolve_cursor_width( + style, + cursor_width, + cursor_stretch_like, + params.x_stretch_cursor, + geometry.char_width, + ), height: cursor_height, ascent: cursor_ascent, style, diff --git a/crates/neomacs-layout-engine/src/display_cursor.rs b/crates/neomacs-layout-engine/src/display_cursor.rs index 9a90b5d838..c139d558d4 100644 --- a/crates/neomacs-layout-engine/src/display_cursor.rs +++ b/crates/neomacs-layout-engine/src/display_cursor.rs @@ -1348,6 +1348,28 @@ pub(crate) fn resolve_cursor_vertical_metrics( (y, height, ascent.min(height)) } +/// GNU caps non-stretching box/hbar cursors at one column, without +/// enlarging a narrower glyph; vertical bars keep their explicit ink width. +pub(crate) fn resolve_cursor_width( + style: CursorStyle, + slot_width: f32, + stretch_like: bool, + x_stretch_cursor: bool, + fallback_char_width: f32, +) -> f32 { + let actual = match style { + CursorStyle::Bar(width) => width.max(1.0), + CursorStyle::Hbar(_) | CursorStyle::FilledBox | CursorStyle::Hollow => slot_width.max(1.0), + }; + if stretch_like && !x_stretch_cursor && !matches!(style, CursorStyle::Bar(_)) { + actual.min(DisplayRowCharWidthPolicy::new(fallback_char_width).fallback()) + } else { + actual + } +} + +/// Combine the selected slot's canonical width and vertical metrics with cursor +/// paint, without letting stretch policy change explicit vertical-bar geometry. pub(crate) fn resolve_cursor_geometry( style: CursorStyle, source: CursorGeometrySource, @@ -1356,18 +1378,13 @@ pub(crate) fn resolve_cursor_geometry( color: Color, frame_cursor_foreground: Color, ) -> ResolvedCursorGeometry { - let actual_slot_width = match style { - CursorStyle::Bar(width) => width.max(1.0), - CursorStyle::Hbar(_) | CursorStyle::FilledBox | CursorStyle::Hollow => { - source.slot_width.max(1.0) - } - }; - let width = if source.stretch_like && !x_stretch_cursor && !matches!(style, CursorStyle::Bar(_)) - { - DisplayRowCharWidthPolicy::new(fallback_char_width).fallback() - } else { - actual_slot_width - }; + let width = resolve_cursor_width( + style, + source.slot_width, + source.stretch_like, + x_stretch_cursor, + fallback_char_width, + ); let (y, height, ascent) = resolve_cursor_vertical_metrics( source.y, source.face_height, @@ -1464,6 +1481,8 @@ impl<'a> CapturedTextWindowCursorPublishContext<'a> { } } + /// Publish logical anchors even when painting is disabled, then materialize + /// the same selected slot's physical geometry and face when a cursor is shown. pub(crate) fn publish_captured_cursor( self, mut cursor: CapturedCursorInfo, @@ -1493,9 +1512,14 @@ impl<'a> CapturedTextWindowCursorPublishContext<'a> { { cursor.face_h = glyph.pixel_height; cursor.face_ascent = glyph.pixel_ascent; - if matches!(glyph.provenance, GlyphProvenance::Str { .. }) { - // A replacement's face and width can differ from the - // buffer face captured before rendering its string. + let stretch = matches!( + glyph.glyph_type, + neomacs_display_protocol::glyph_matrix::GlyphType::Stretch { .. } + ); + if stretch || matches!(glyph.provenance, GlyphProvenance::Str { .. }) { + // Replacement/stretch width can differ from the buffer + // face captured before its display property was rendered. + cursor.stretch_like = stretch; cursor.slot_width = Some(glyph.pixel_width); if let Some(face) = builder.output_face(glyph.face_id) { cursor.fg = face.foreground; @@ -1532,9 +1556,30 @@ impl<'a> CapturedTextWindowCursorPublishContext<'a> { self.text_area_left, self.window_top, ); - if let Some(grid_x) = grid_x_override { - logical_cursor.x = grid_x; - } + // Logical anchors and physical publication must accept the same + // integer override only when bidi did not move the selected slot. + // Explicit string-cursor candidates already have resolved placement. + let finalized_x = if cursor.slot_state == CursorSlotResolutionState::Unresolved { + output + .builder() + .resolve_finalized_cursor_placement( + CursorVisualColumnResolutionRequest::new( + self.params.window_id, + row_metric.row(), + self.point_charpos.max(0) as usize, + ), + self.char_w, + ) + .map_or(cursor.x, |placement| placement.x()) + } else { + cursor.x + }; + logical_cursor.x = crate::window_output::cursor_snapshot_x( + cursor.x, + finalized_x, + self.text_area_left, + grid_x_override, + ); output_emitter.set_logical_cursor(logical_cursor); let Some(style) = cursor_style_for_window(self.params) else { diff --git a/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs b/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs index 45476f2fb3..1d51d14eb2 100644 --- a/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs +++ b/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs @@ -482,6 +482,135 @@ fn rtl_replacement_cursor_uses_its_resolved_glyph_metrics() { } } +/// Replacement placement must identify the selected glyph independently of +/// fresh/replay equality, and logical cursor anchors must use its finalized x. +#[test] +fn replacement_logical_cursor_tracks_the_selected_visual_slot() { + for (replacement, visible) in ["אבג", "abc"] + .into_iter() + .flat_map(|replacement| [true, false].map(|visible| (replacement, visible))) + { + let (mut eval, frame, _, window) = incr_editing_frame("ax\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", if visible { Value::T } else { Value::NIL }); + eval.eval_str(&format!("(progn (put-text-property 2 3 'display \"{replacement}\") (set-window-point (selected-window) 2))")).unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + let FrameLayoutAttempt::Prepared(prepared) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("replacement frame") + }; + let row = &prepared + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows[0]; + let glyphs = &row.glyphs[GlyphArea::Text.index()]; + let selected = glyphs + .iter() + .enumerate() + .filter(|(_, glyph)| !glyph.padding && row.glyph_covers_buffer_charpos(glyph, 1)) + .filter_map(|(i, glyph)| match glyph.provenance { + neomacs_display_protocol::glyph_matrix::GlyphProvenance::Str { index, .. } => { + Some((index, i)) + } + _ => None, + }) + .min() + .expect("replacement glyph") + .1; + let expected_col: u16 = row.glyphs[GlyphArea::LeftMargin.index()] + .iter() + .chain(glyphs[..selected].iter()) + .filter(|glyph| !glyph.padding) + .map(|glyph| glyph.materialized_slot_span()) + .sum(); + let expected_x = row.pixel_x + + glyphs[..selected] + .iter() + .filter(|glyph| !glyph.padding) + .map(|glyph| glyph.pixel_width) + .sum::(); + let output = prepared.materialize(); + if visible { + assert_eq!(output.active_cursor().unwrap().slot_id.col, expected_col); + } else { + assert!(output.active_cursor().is_none()); + } + let snapshot = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + let logical = snapshot.logical_cursor.as_ref().unwrap(); + assert_eq!( + logical.x, + expected_x.round() as i64, + "{replacement}, visible={visible}" + ); + if let Some(physical) = snapshot.phys_cursor.as_ref() { + assert_eq!(logical.x, physical.x, "{replacement}"); + } + } +} + +/// Moved-point reconstruction must share fresh stretch-width policy, including +/// preserving narrow stretches and respecting x-stretch-cursor. +#[test] +fn stretch_cursor_only_replay_matches_fresh_width_policy() { + for stretch in [false, true] { + for (width, style) in [0.25, 5.0] + .into_iter() + .flat_map(|width| ["box", "bar", "hbar"].map(|style| (width, style))) + { + let (mut eval, frame, _, _) = incr_editing_frame("אxב\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + let cursor_type = match style { + "box" => Value::T, + "bar" => Value::cons(Value::symbol("bar"), Value::fixnum(2)), + _ => Value::symbol("hbar"), + }; + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", cursor_type); + eval.eval_str(&format!("(progn (setq x-stretch-cursor {}) (put-text-property 2 3 'display '(space :width {width})) (set-window-point (selected-window) 1))", if stretch { "t" } else { "nil" })).unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + engine.layout_frame_rust(&mut eval, frame); + eval.eval_str("(set-window-point (selected-window) 2)") + .unwrap(); + let FrameLayoutAttempt::Prepared(replayed) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("stretch replay") + }; + assert_eq!(engine.last_layout_stats().cursor_only_windows, 1); + assert!(engine.last_layout_stats().reused_rows > 0); + let actual = replayed.materialize().active_cursor().unwrap().width; + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut fresh); + let FrameLayoutAttempt::Prepared(rebuilt) = + fresh.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("fresh stretch") + }; + assert_eq!( + actual, + rebuilt.materialize().active_cursor().unwrap().width, + "stretch={stretch} width={width} style={style}" + ); + } + } +} + /// An unchanged authoritative cursor needs no new point-font measurement. #[test] fn unchanged_unmeasured_cursor_preserves_retained_replay() { diff --git a/crates/neomacs-layout-engine/src/window_output.rs b/crates/neomacs-layout-engine/src/window_output.rs index 59030f0391..5d59567647 100644 --- a/crates/neomacs-layout-engine/src/window_output.rs +++ b/crates/neomacs-layout-engine/src/window_output.rs @@ -875,6 +875,20 @@ impl TextWindowCursor { } } +/// Share integer-grid rounding between logical anchors and physical snapshots. +/// A replacement boundary override is valid only if final slot placement stayed +/// at the captured x; bidi-selected glyphs otherwise use their measured x. +pub(crate) fn cursor_snapshot_x( + captured_x: f32, + display_x: f32, + text_area_left: f32, + grid_x_override: Option, +) -> i64 { + grid_x_override + .filter(|_| (display_x - captured_x).abs() <= f32::EPSILON) + .unwrap_or_else(|| (display_x - text_area_left).round() as i64) +} + impl ResolvedTextWindowCursor { fn row(self) -> usize { self.coordinates.display_slot_id().row as usize @@ -887,11 +901,12 @@ impl ResolvedTextWindowCursor { fn window_snapshot(self) -> WindowCursorSnapshot { WindowCursorSnapshot { kind: window_cursor_kind(self.captured.style), - x: self - .captured - .grid_x_override - .filter(|_| (self.display_x - self.captured.x).abs() <= f32::EPSILON) - .unwrap_or_else(|| (self.display_x - self.captured.text_area_left).round() as i64), + x: cursor_snapshot_x( + self.captured.x, + self.display_x, + self.captured.text_area_left, + self.captured.grid_x_override, + ), y: (self.captured.y - self.captured.window_top).round() as i64, width: self.captured.width.round() as i64, height: self.captured.height.round() as i64, From ec475a8fb20bd152ca6ed8089741c4c2b452f0f8 Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sat, 7 Nov 2026 22:41:16 +0000 Subject: [PATCH 06/10] Preserve logical cursor extents from selected source glyphs --- .../src/display_cursor.rs | 109 ++++++++++---- .../src/display_row/builder.rs | 61 ++++++-- .../src/display_row/line_end.rs | 2 + .../src/display_row/overlay_string.rs | 15 +- .../source_append/tests/source_append_test.rs | 1 + .../src/display_row/trailing_whitespace.rs | 2 + .../src/engine/prepared_viewports.rs | 1 + .../engine/prepared_viewports/measurement.rs | 1 + .../src/engine/query_cache/slicing.rs | 1 + .../src/engine/scroll_coverage.rs | 1 + .../src/engine/tests/engine_test.rs | 1 + .../src/tests/engine_line_spacing_test.rs | 139 ++++++++++++++++++ .../src/window_output.rs | 24 ++- .../runtime/eval/tests/eval_test.rs | 10 ++ crates/neovm-core/src/window/geometry.rs | 19 ++- crates/neovm-core/src/window/mod.rs | 13 ++ .../window/tests/geometry_positions_test.rs | 75 ++++++++++ 17 files changed, 425 insertions(+), 50 deletions(-) diff --git a/crates/neomacs-layout-engine/src/display_cursor.rs b/crates/neomacs-layout-engine/src/display_cursor.rs index c139d558d4..95b5edbdbe 100644 --- a/crates/neomacs-layout-engine/src/display_cursor.rs +++ b/crates/neomacs-layout-engine/src/display_cursor.rs @@ -104,6 +104,8 @@ pub(crate) struct CapturedCursorInfo { pub(crate) col: usize, pub(crate) display_row_offset: usize, pub(crate) slot_width: Option, + /// Raw selected extent for explicit string candidates, before paint clamps. + pub(crate) logical_dimensions: Option, pub(crate) stretch_like: bool, pub(crate) slot_state: CursorSlotResolutionState, /// For a cursor sitting at a `display`-property replacement slot: the 1-based @@ -285,6 +287,20 @@ pub(crate) enum CapturedCursorSlotWidth { Explicit(f32), } +/// Preserve measured zero width; missing or invalid measurements retain the +/// legacy source-point fallback instead of inventing a logical rectangle. +pub(crate) fn measured_logical_cursor_dimensions( + width: f32, + height: f32, +) -> Option { + (width.is_finite() && width >= 0.0 && height.is_finite() && height > 0.0).then(|| { + neovm_core::window::LogicalCursorDimensions { + width: width.round() as u32, + height: height.round() as u32, + } + }) +} + impl CapturedCursorSlotWidth { fn resolve(self, face_char_width: f32) -> f32 { match self { @@ -399,13 +415,14 @@ pub(crate) struct ResolvedCursorCoordinatePair { /// Coherent cursor placement after a late row decoration changed the /// materialized glyph sequence. /// -/// The slot and measured pixel pen are deliberately one value. A decoration +/// The slot, measured pen and optional glyph extent are deliberately one value. A decoration /// cannot publish a new column while leaving `PhysCursor::x` in the old row /// geometry, which is observable on TTY frames and in cursor effects. #[derive(Clone, Copy, Debug, PartialEq)] pub(crate) struct ResolvedDecoratedCursorPlacement { coordinates: ResolvedCursorCoordinatePair, x: f32, + logical_dimensions: Option, } impl ResolvedDecoratedCursorPlacement { @@ -413,6 +430,12 @@ impl ResolvedDecoratedCursorPlacement { self.x } + pub(crate) const fn logical_dimensions( + &self, + ) -> Option { + self.logical_dimensions + } + pub(crate) const fn coordinates(&self) -> ResolvedCursorCoordinatePair { self.coordinates } @@ -485,6 +508,17 @@ struct CursorOutputCol(u16); struct CursorDisplayCol(u16); impl ResolvedCursorSlot { + /// Return only an exact selected glyph, never a row-end or visibility approximation. + fn glyph(self, row: &GlyphRow) -> Option<&Glyph> { + let index = match self { + Self::BufferGlyph { text_glyph, .. } | Self::ReplacementString { text_glyph, .. } => { + text_glyph + } + _ => return None, + }; + row.glyphs[GlyphArea::Text.index()].get(index) + } + pub(crate) const fn col(self) -> u16 { match self { Self::BufferGlyph { col, .. } | Self::ReplacementString { col, .. } => col, @@ -668,14 +702,7 @@ impl CursorVisualColumnResolutionRequest { /// Return the exact covering point glyph selected for cursor placement. /// Hidden-text/row-end approximations have no covering glyph to substitute. pub(crate) fn point_glyph(row: &GlyphRow, point: usize) -> Option<&Glyph> { - let text_glyph = match Self::resolve_row_slot(row, point)? { - ResolvedCursorSlot::BufferGlyph { text_glyph, .. } - | ResolvedCursorSlot::ReplacementString { text_glyph, .. } => text_glyph, - ResolvedCursorSlot::HorizontallyClipped(_) - | ResolvedCursorSlot::FollowingVisibleGlyph(_) - | ResolvedCursorSlot::RowEnd(_) => return None, - }; - row.glyphs[GlyphArea::Text.index()].get(text_glyph) + Self::resolve_row_slot(row, point)?.glyph(row) } #[cfg(test)] @@ -689,7 +716,15 @@ impl CursorVisualColumnResolutionRequest { self, context: CursorVisualColumnResolutionContext<'_>, ) -> Option { - let display_slot = self.resolve_slot(context)?; + self.coordinates_for_slot(context, self.resolve_slot(context)?) + } + + /// Reuse one slot identity for coordinates, pen and extent publication. + fn coordinates_for_slot( + self, + context: CursorVisualColumnResolutionContext<'_>, + display_slot: ResolvedCursorSlot, + ) -> Option { let row = context.rows?.row(self.row)?; let left_margin_cols = row.glyphs[GlyphArea::LeftMargin.index()] .iter() @@ -737,8 +772,12 @@ impl CursorVisualColumnResolutionRequest { text_bounds: Rect, char_width: f32, ) -> Option { - let coordinates = self.resolve_cursor_coordinates(context)?; + let slot = self.resolve_slot(context)?; + let coordinates = self.coordinates_for_slot(context, slot)?; let row = context.rows?.row(self.row)?; + let logical_dimensions = slot.glyph(row).and_then(|glyph| { + measured_logical_cursor_dimensions(glyph.pixel_width, glyph.pixel_height) + }); let target_col = coordinates.display_col(); let left_margin_cols = row.glyphs[GlyphArea::LeftMargin.index()] .iter() @@ -758,7 +797,11 @@ impl CursorVisualColumnResolutionRequest { let x = geometry.x_at_slot(neomacs_display_protocol::glyph_matrix::TextSlotColumn::new( target_col - left_margin_cols, ))?; - Some(ResolvedDecoratedCursorPlacement { coordinates, x }) + Some(ResolvedDecoratedCursorPlacement { + coordinates, + x, + logical_dimensions, + }) } } @@ -843,6 +886,8 @@ impl CapturedCursorInfo { ) } + /// Capture the lightweight insertion position; selected extent is published + /// separately so hidden cursors do not require a physical cursor snapshot. pub(crate) fn logical_cursor_position( &self, row_metric: RowMetricsSnapshot, @@ -879,6 +924,8 @@ impl CapturedCursorInfo { } } + /// Capture visual defaults, not a claim about the selected glyph's extent. + /// Callers attach an extent only when an emitted source slot measured it. pub(crate) fn from_visual_state( visual_state: CapturedCursorVisualState, placement: CapturedCursorPlacement, @@ -896,6 +943,9 @@ impl CapturedCursorInfo { col: placement.col, display_row_offset: placement.display_row_offset, slot_width: Some(placement.slot_width.resolve(visual_state.face_width)), + // These are visual defaults, not necessarily the selected glyph's + // measured height (notably for explicit overlay-string cursors). + logical_dimensions: None, stretch_like: placement.stretch_like, slot_state: CursorSlotResolutionState::Unresolved, display_replacement_anchor_charpos: None, @@ -1559,28 +1609,31 @@ impl<'a> CapturedTextWindowCursorPublishContext<'a> { // Logical anchors and physical publication must accept the same // integer override only when bidi did not move the selected slot. // Explicit string-cursor candidates already have resolved placement. - let finalized_x = if cursor.slot_state == CursorSlotResolutionState::Unresolved { - output - .builder() - .resolve_finalized_cursor_placement( - CursorVisualColumnResolutionRequest::new( - self.params.window_id, - row_metric.row(), - self.point_charpos.max(0) as usize, - ), - self.char_w, - ) - .map_or(cursor.x, |placement| placement.x()) - } else { - cursor.x - }; + let (finalized_x, logical_dimensions) = + if cursor.slot_state == CursorSlotResolutionState::Unresolved { + output + .builder() + .resolve_finalized_cursor_placement( + CursorVisualColumnResolutionRequest::new( + self.params.window_id, + row_metric.row(), + self.point_charpos.max(0) as usize, + ), + self.char_w, + ) + .map_or((cursor.x, None), |placement| { + (placement.x(), placement.logical_dimensions()) + }) + } else { + (cursor.x, cursor.logical_dimensions) + }; logical_cursor.x = crate::window_output::cursor_snapshot_x( cursor.x, finalized_x, self.text_area_left, grid_x_override, ); - output_emitter.set_logical_cursor(logical_cursor); + output_emitter.set_logical_cursor(logical_cursor, logical_dimensions); let Some(style) = cursor_style_for_window(self.params) else { return CapturedTextWindowCursorPublishOutcome::NoWindowCursor; diff --git a/crates/neomacs-layout-engine/src/display_row/builder.rs b/crates/neomacs-layout-engine/src/display_row/builder.rs index 039d14a081..0528ebea52 100644 --- a/crates/neomacs-layout-engine/src/display_row/builder.rs +++ b/crates/neomacs-layout-engine/src/display_row/builder.rs @@ -1352,6 +1352,7 @@ pub(crate) struct DisplayRowGlyphSlot { // Primitive boundary relative to the start of this text append. Source // characters and stored glyphs differ for padding and composition. text_glyph_offset: Option, + glyph_height_px: Option, } /// Source positions and paint primitives are not one-to-one. Composition @@ -1376,6 +1377,8 @@ impl DisplayRowGlyphSlot { Self::with_pointer_appearance(source, x_px, col, width_px, width_cols, None) } + /// Record a source slot's advance and pointer appearance. A vertical + /// measurement is attached separately only when a primitive was emitted. pub(crate) fn with_pointer_appearance( source: DisplaySourcePosition, x_px: f32, @@ -1395,6 +1398,7 @@ impl DisplayRowGlyphSlot { }, default_cell_height: false, text_glyph_offset: None, + glyph_height_px: None, } } @@ -1418,6 +1422,17 @@ impl DisplayRowGlyphSlot { self } + /// Keep the emitted glyph's measured height with its source slot; row + /// defaults and composition members are not measurements of this glyph. + fn with_glyph_height(mut self, height: Option) -> Self { + self.glyph_height_px = height.filter(|height| height.is_finite() && *height > 0.0); + self + } + + pub(crate) fn glyph_height_px(&self) -> Option { + self.glyph_height_px + } + pub(crate) fn text_glyph_offset(&self) -> Option { self.text_glyph_offset } @@ -2128,7 +2143,8 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer written.width_cols(), item.pointer_appearance.clone(), ) - .with_text_glyph_offset(char_offset.get()), + .with_text_glyph_offset(char_offset.get()) + .with_glyph_height(Some(glyph.pixel_height)), ); self.writer.row.glyphs[GlyphArea::Text.index()].push(glyph); self.advance(written); @@ -2152,6 +2168,8 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer )) } + /// Append one canonical item and report its addressable source slots with + /// the metrics of the actual emitted primitives, including clipped output. pub(crate) fn push_item(&mut self, item: DisplayItem) -> DisplayRowAppendProgress { let start = self.position; let mut metrics = DisplayRowWriteMetrics::default(); @@ -2400,14 +2418,21 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer } } if !written.is_empty() { - slots.push(DisplayRowGlyphSlot::with_pointer_appearance( - slot_source, - slot_start.x_px(), - slot_start.col(), - written.width_px(), - written.width_cols(), - pointer_appearance.clone(), - )); + slots.push( + DisplayRowGlyphSlot::with_pointer_appearance( + slot_source, + slot_start.x_px(), + slot_start.col(), + written.width_px(), + written.width_cols(), + pointer_appearance.clone(), + ) + .with_glyph_height( + self.writer.row.glyphs[self.writer.area_index()] + .get(before_len) + .map(|glyph| glyph.pixel_height), + ), + ); } self.advance(written); metrics.add(written); @@ -2477,6 +2502,8 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer apply_box_run_topology_to_glyphs(glyphs, membership, source_edges, source_item_complete); } + /// Keep each character's source slot paired with its emitted primitive + /// after measurement and clipping, rather than using the row's defaults. #[allow(clippy::too_many_arguments)] fn push_text_item( &mut self, @@ -2594,7 +2621,12 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer .with_default_cell_height( ch == '\t' && matches!(source_mapping, DisplayTextSourceMapping::NaturalText), ) - .with_text_glyph_offset(before_len - glyph_start), + .with_text_glyph_offset(before_len - glyph_start) + .with_glyph_height( + self.writer.row.glyphs[area_index] + .get(before_len) + .map(|glyph| glyph.pixel_height), + ), ); self.advance(written); metrics.add(written); @@ -2606,6 +2638,8 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer status } + /// Emit a composition once while retaining all source positions. Only its + /// head owns primitive metrics; members must not invent measured extents. #[allow(clippy::too_many_arguments)] fn push_automatic_text_item( &mut self, @@ -2671,6 +2705,13 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer }, pointer_appearance.cloned(), ); + if char_offset == 0 { + slot = slot.with_glyph_height( + self.writer.row.glyphs[area_index] + .get(before_len) + .map(|glyph| glyph.pixel_height), + ); + } slot.coverage = if char_offset == 0 { DisplayRowGlyphCoverage::Primitive { source_chars } } else { diff --git a/crates/neomacs-layout-engine/src/display_row/line_end.rs b/crates/neomacs-layout-engine/src/display_row/line_end.rs index 9f09c264cf..c39be75e1a 100644 --- a/crates/neomacs-layout-engine/src/display_row/line_end.rs +++ b/crates/neomacs-layout-engine/src/display_row/line_end.rs @@ -470,6 +470,8 @@ impl ResolvedLineEndPlan { } } + /// Execute source-dependent highlighting only with the owning buffer's + /// predicate; keep this stored row plan independent of buffer lifetimes. fn apply_with_source( self, row: &mut GlyphRow, diff --git a/crates/neomacs-layout-engine/src/display_row/overlay_string.rs b/crates/neomacs-layout-engine/src/display_row/overlay_string.rs index 6e8db299fd..12ec16cff4 100644 --- a/crates/neomacs-layout-engine/src/display_row/overlay_string.rs +++ b/crates/neomacs-layout-engine/src/display_row/overlay_string.rs @@ -980,6 +980,8 @@ fn root_lisp_position_char(source: &crate::display_item::DisplaySourcePosition) } } +/// Resolve cursor-property eligibility at the string's source slot and attach +/// its measured extent before later bidi reordering changes visual placement. fn capture_overlay_string_cursor_at_slot( text_props: Option<&neovm_core::buffer::text_props::TextPropertyTable>, slot: &DisplayRowGlyphSlot, @@ -992,7 +994,7 @@ fn capture_overlay_string_cursor_at_slot( let Some(char_idx) = root_lisp_position_char(&slot.source()) else { return; }; - capture_overlay_string_cursor( + capture_overlay_string_cursor_with_dimensions( text_props, char_idx, cursor_info, @@ -1003,11 +1005,16 @@ fn capture_overlay_string_cursor_at_slot( visual_state, CapturedCursorSlotWidth::Explicit(slot.width_px()), cursor_context, + slot.glyph_height_px().and_then(|height| { + crate::display_cursor::measured_logical_cursor_dimensions(slot.width_px(), height) + }), ); } +/// Keep explicit string-cursor identity and measured extent together; window +/// row defaults are not evidence of the selected slot's glyph height. #[allow(clippy::too_many_arguments)] -fn capture_overlay_string_cursor( +fn capture_overlay_string_cursor_with_dimensions( text_props: Option<&neovm_core::buffer::text_props::TextPropertyTable>, char_idx: usize, cursor_info: &mut CursorCaptureState, @@ -1018,6 +1025,7 @@ fn capture_overlay_string_cursor( visual_state: CapturedCursorVisualState, slot_width: CapturedCursorSlotWidth, cursor_context: DisplayStringCursorContext, + logical_dimensions: Option, ) { let Some(props) = text_props else { return; @@ -1028,7 +1036,7 @@ fn capture_overlay_string_cursor( ) else { return; }; - let info = CapturedCursorInfo::from_visual_state( + let mut info = CapturedCursorInfo::from_visual_state( visual_state, CapturedCursorPlacement { x, @@ -1040,6 +1048,7 @@ fn capture_overlay_string_cursor( stretch_like: false, }, ); + info.logical_dimensions = logical_dimensions; if let Some(candidate) = cursor_context.resolve(cursor_prop, info) { cursor_info.capture_display_string_cursor(candidate); } diff --git a/crates/neomacs-layout-engine/src/display_row/source_append/tests/source_append_test.rs b/crates/neomacs-layout-engine/src/display_row/source_append/tests/source_append_test.rs index 29f44ed5ef..4d5bb2663c 100644 --- a/crates/neomacs-layout-engine/src/display_row/source_append/tests/source_append_test.rs +++ b/crates/neomacs-layout-engine/src/display_row/source_append/tests/source_append_test.rs @@ -7976,6 +7976,7 @@ fn buffer_text_window_tail_finalize_request_publishes_cursor_and_finishes_row() let mut cursor_info = CursorCaptureState::new(); cursor_info.capture_once(crate::display_cursor::CapturedCursorInfo { vertical_source: crate::display_cursor::CursorVerticalSource::PointGlyph, + logical_dimensions: None, x: 0.0, y: 0.0, face_w: 8.0, diff --git a/crates/neomacs-layout-engine/src/display_row/trailing_whitespace.rs b/crates/neomacs-layout-engine/src/display_row/trailing_whitespace.rs index 1221862525..5c6386ec08 100644 --- a/crates/neomacs-layout-engine/src/display_row/trailing_whitespace.rs +++ b/crates/neomacs-layout-engine/src/display_row/trailing_whitespace.rs @@ -80,6 +80,8 @@ fn has_buffer_position(glyph: &Glyph) -> bool { impl DisplayCurrentRowMutation for HighlightTrailingWhitespaceMutation<'_> { type Output = (); + /// Validate the last buffer candidate once, then paint its logical run. + /// String-owned glyphs stop the run; only nil-object end markers are skipped. fn apply(self, row: &mut GlyphRow) -> Self::Output { let glyphs = &mut row.glyphs[GlyphArea::Text.index()]; // GNU: "Skip over glyphs inserted to display the cursor at the end of diff --git a/crates/neomacs-layout-engine/src/engine/prepared_viewports.rs b/crates/neomacs-layout-engine/src/engine/prepared_viewports.rs index b1d7d0ef84..2d184eb267 100644 --- a/crates/neomacs-layout-engine/src/engine/prepared_viewports.rs +++ b/crates/neomacs-layout-engine/src/engine/prepared_viewports.rs @@ -359,6 +359,7 @@ impl PreparedViewports { candidate.presented_cursor = None; let snapshot = std::sync::Arc::make_mut(&mut candidate.display_snapshot); snapshot.logical_cursor = None; + snapshot.logical_cursor_dimensions = None; snapshot.phys_cursor = None; snapshot.layout_freshness = None; snapshot.window_end_record = None; diff --git a/crates/neomacs-layout-engine/src/engine/prepared_viewports/measurement.rs b/crates/neomacs-layout-engine/src/engine/prepared_viewports/measurement.rs index af52d60bba..87d8cd57d6 100644 --- a/crates/neomacs-layout-engine/src/engine/prepared_viewports/measurement.rs +++ b/crates/neomacs-layout-engine/src/engine/prepared_viewports/measurement.rs @@ -170,6 +170,7 @@ impl PreparedViewports { }) .collect(); snapshot.logical_cursor = None; + snapshot.logical_cursor_dimensions = None; snapshot.phys_cursor = None; snapshot.chrome_strings = Default::default(); snapshot.regions_materialized = true; diff --git a/crates/neomacs-layout-engine/src/engine/query_cache/slicing.rs b/crates/neomacs-layout-engine/src/engine/query_cache/slicing.rs index 74f1e948a1..63cf2eeb60 100644 --- a/crates/neomacs-layout-engine/src/engine/query_cache/slicing.rs +++ b/crates/neomacs-layout-engine/src/engine/query_cache/slicing.rs @@ -115,6 +115,7 @@ pub(super) fn pixels_suffix( row.body_y = row.body_y.checked_sub(y_delta)?; } placed.logical_cursor = None; + placed.logical_cursor_dimensions = None; placed.phys_cursor = None; placed.window_end_record = Some( record.with_matrix_row(MatrixRow0::new( diff --git a/crates/neomacs-layout-engine/src/engine/scroll_coverage.rs b/crates/neomacs-layout-engine/src/engine/scroll_coverage.rs index 09042000cd..2d795b0d5f 100644 --- a/crates/neomacs-layout-engine/src/engine/scroll_coverage.rs +++ b/crates/neomacs-layout-engine/src/engine/scroll_coverage.rs @@ -344,6 +344,7 @@ impl ScrollCoverage { snapshot.rows = body.geometry.rows; snapshot.body_rows = body.geometry.body_rows; snapshot.logical_cursor = None; + snapshot.logical_cursor_dimensions = None; snapshot.phys_cursor = None; // Geometry remains private; keep the captured source identity separately // so bounded measurement can validate it together with the read certificate. diff --git a/crates/neomacs-layout-engine/src/engine/tests/engine_test.rs b/crates/neomacs-layout-engine/src/engine/tests/engine_test.rs index c1c270a0f4..e5e1704e79 100644 --- a/crates/neomacs-layout-engine/src/engine/tests/engine_test.rs +++ b/crates/neomacs-layout-engine/src/engine/tests/engine_test.rs @@ -692,6 +692,7 @@ fn face_scan_checkpoint_tracks_resolution_boundaries_and_invalidation() { fn cursor_info_at(x: f32, display_row_offset: usize) -> CapturedCursorInfo { CapturedCursorInfo { vertical_source: crate::display_cursor::CursorVerticalSource::PointGlyph, + logical_dimensions: None, x, y: display_row_offset as f32 * 14.0, face_w: 7.0, diff --git a/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs b/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs index 1d51d14eb2..3d07518cda 100644 --- a/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs +++ b/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs @@ -561,6 +561,142 @@ fn replacement_logical_cursor_tracks_the_selected_visual_slot() { } } +/// Hidden-cursor anchors must use the replacement's dimensions, not a preceding +/// zero-width combining point that happens to share its logical column. +#[test] +fn hidden_rtl_cursor_anchor_uses_selected_glyph_dimensions() { + use neovm_core::window::geometry::{PresentationId, VisualAnchor, VisualAnchorQuery}; + let (mut eval, frame, _, window) = incr_editing_frame("a\u{0301}x\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", Value::NIL); + eval.eval_str( + "(progn (put-text-property 3 4 'display \"אבג\") (set-window-point (selected-window) 3))", + ) + .unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + let FrameLayoutAttempt::Prepared(prepared) = engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("hidden replacement") + }; + let row = &prepared + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows[0]; + let selected = row.glyphs[GlyphArea::Text.index()] + .iter() + .filter(|glyph| !glyph.padding && row.glyph_covers_buffer_charpos(glyph, 2)) + .filter_map(|glyph| match glyph.provenance { + neomacs_display_protocol::glyph_matrix::GlyphProvenance::Str { index, .. } => { + Some((index, glyph)) + } + _ => None, + }) + .min_by_key(|(index, _)| *index) + .unwrap() + .1; + let expected_width = selected.pixel_width.round(); + assert!(expected_width > 0.0); + assert!(prepared.materialize().active_cursor().is_none()); + let snapshot = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + let owner = eval.frame_manager_mut().get_mut(frame).unwrap(); + // The layout attempt already prepared generation one. + let presentation = PresentationId::new(2); + owner + .prepare_live_window_presentation(presentation, vec![snapshot]) + .unwrap(); + owner.activate_display_presentation(presentation).unwrap(); + let anchor = owner + .resolve_active_visual_anchor(VisualAnchorQuery::new( + presentation, + VisualAnchor::CursorBottom { window }, + )) + .unwrap(); + assert_eq!(anchor.bounds_in_frame().width().get(), expected_width); +} + +/// Explicit string cursors must carry their own measured height, not the +/// window's default row height used while recording the overlay candidate. +#[test] +fn hidden_explicit_string_cursor_anchor_uses_selected_glyph_height() { + use neovm_core::window::geometry::{PresentationId, VisualAnchor, VisualAnchorQuery}; + for (source, selected_index) in [ + ("(propertize \"X\" 'cursor 1 'face '(:height 2.0))", 0), + ( + "(concat \"א\" (propertize \"X\" 'cursor 1 'face '(:height 2.0)) \"ב\")", + 1, + ), + ] { + let (mut eval, frame, _, window) = incr_editing_frame("a\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", Value::NIL); + eval.eval_str(&format!( + "(overlay-put (make-overlay 1 1) 'before-string {source})" + )) + .unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + let FrameLayoutAttempt::Prepared(prepared) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("explicit overlay") + }; + let row = &prepared + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows[0]; + let selected = row.glyphs[GlyphArea::Text.index()] + .iter() + .find(|glyph| { + matches!( + glyph.provenance, + neomacs_display_protocol::glyph_matrix::GlyphProvenance::Str { index, .. } if index == selected_index + ) + }) + .unwrap(); + let expected = selected.pixel_height.round(); + assert!(prepared.materialize().active_cursor().is_none()); + let snapshot = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + let owner = eval.frame_manager_mut().get_mut(frame).unwrap(); + let presentation = PresentationId::new(2); + owner + .prepare_live_window_presentation(presentation, vec![snapshot]) + .unwrap(); + owner.activate_display_presentation(presentation).unwrap(); + let anchor = owner + .resolve_active_visual_anchor(VisualAnchorQuery::new( + presentation, + VisualAnchor::CursorBottom { window }, + )) + .unwrap(); + assert_eq!(anchor.bounds_in_frame().height().get(), expected); + } +} + /// Moved-point reconstruction must share fresh stretch-width policy, including /// preserving narrow stretches and respecting x-stretch-cursor. #[test] @@ -595,6 +731,9 @@ fn stretch_cursor_only_replay_matches_fresh_width_policy() { assert_eq!(engine.last_layout_stats().cursor_only_windows, 1); assert!(engine.last_layout_stats().reused_rows > 0); let actual = replayed.materialize().active_cursor().unwrap().width; + if style == "bar" { + assert_eq!(actual, 2.0, "stretch={stretch} width={width}"); + } let mut fresh = crate::font::metrics::test_catalog::layout_engine(); crate::font::metrics::test_catalog::enable_metrics(&mut fresh); let FrameLayoutAttempt::Prepared(rebuilt) = diff --git a/crates/neomacs-layout-engine/src/window_output.rs b/crates/neomacs-layout-engine/src/window_output.rs index 5d59567647..193ceb4195 100644 --- a/crates/neomacs-layout-engine/src/window_output.rs +++ b/crates/neomacs-layout-engine/src/window_output.rs @@ -1442,7 +1442,11 @@ pub(crate) struct WindowOutputEmitter { eob_overlay_motion: neovm_core::window::EobOverlayMotionCoverage, current_automatic_mini_row_end: AutomaticMiniRowEnd, previous_automatic_mini_row_end: AutomaticMiniRowEnd, - logical_cursor: Option, + // Extent cannot outlive its position within the producer. + logical_cursor: Option<( + WindowCursorPos, + Option, + )>, phys_cursor: Option, chrome_strings: WindowChromeStringState, } @@ -2088,8 +2092,13 @@ impl WindowOutputEmitter { self.geometry.push_chrome_row_progress(progress) } - pub(crate) fn set_logical_cursor(&mut self, cursor: WindowCursorPos) { - self.logical_cursor = Some(cursor); + /// Publish a position and its selected extent atomically, independently of paint. + pub(crate) fn set_logical_cursor( + &mut self, + cursor: WindowCursorPos, + dimensions: Option, + ) { + self.logical_cursor = Some((cursor, dimensions)); } pub(crate) fn set_phys_cursor(&mut self, cursor: WindowCursorSnapshot) { @@ -2101,6 +2110,8 @@ impl WindowOutputEmitter { self.phys_cursor.as_ref() } + /// Freeze row geometry and cursor facts into one snapshot. Logical extent + /// travels with its position even when no physical cursor is painted. pub(crate) fn finish_snapshot_with_geometry( mut self, evaluator: &mut Context, @@ -2112,7 +2123,11 @@ impl WindowOutputEmitter { ) -> WindowDisplaySnapshot { let frame_id = self.frame_id; let window_id = self.window_id; - let logical_cursor = self.logical_cursor.take(); + let (logical_cursor, logical_cursor_dimensions) = self + .logical_cursor + .take() + .map(|(position, dimensions)| (Some(position), dimensions)) + .unwrap_or_default(); let phys_cursor = self.phys_cursor.take(); let mut eob_overlay_motion = self.eob_overlay_motion; let prepared_rows = self @@ -2170,6 +2185,7 @@ impl WindowOutputEmitter { tab_line_height, chrome_strings, logical_cursor, + logical_cursor_dimensions, phys_cursor: phys_cursor.clone(), points: prepared_rows.points, point_rows: prepared_rows.point_rows, diff --git a/crates/neovm-core/src/emacs_core/runtime/eval/tests/eval_test.rs b/crates/neovm-core/src/emacs_core/runtime/eval/tests/eval_test.rs index 5c50e3bb7a..b751320122 100644 --- a/crates/neovm-core/src/emacs_core/runtime/eval/tests/eval_test.rs +++ b/crates/neovm-core/src/emacs_core/runtime/eval/tests/eval_test.rs @@ -4614,6 +4614,7 @@ fn read_key_sequence_can_return_select_window_at_sequence_start() { assert_eq!(binding, Value::symbol("neomacs-test-handle-select-window")); } +/// Resolve a mouse press against the clicked window's published geometry. #[test] fn read_char_mouse_press_uses_clicked_window_geometry() { crate::test_utils::init_test_tracing(); @@ -4648,6 +4649,7 @@ fn read_char_mouse_press_uses_clicked_window_geometry() { .get_mut(fid) .expect("mutable frame") .commit_redisplay_cache_for_test(vec![crate::window::WindowDisplaySnapshot { + logical_cursor_dimensions: None, posn_matrix: None, point_rows: None, window_id: w2, @@ -4727,6 +4729,7 @@ fn read_char_mouse_press_uses_clicked_window_geometry() { ); } +/// Mouse-key lookup uses the clicked window's local map, not the selected one. #[test] fn read_key_sequence_uses_clicked_window_local_map_for_mouse_event() { crate::test_utils::init_test_tracing(); @@ -4823,6 +4826,7 @@ fn read_key_sequence_uses_clicked_window_local_map_for_mouse_event() { end_source: Default::default(), fringe: Default::default(), }], + logical_cursor_dimensions: None, }]); // GNU drops an up event with no matching down (src/keyboard.c:6628-6632), @@ -4854,6 +4858,7 @@ fn read_key_sequence_uses_clicked_window_local_map_for_mouse_event() { assert_eq!(position_slots[5], Value::fixnum(77)); } +/// An unbound press must not obscure the following bound release event. #[test] fn read_key_sequence_drops_unbound_down_mouse_before_bound_click() { crate::test_utils::init_test_tracing(); @@ -4950,6 +4955,7 @@ fn read_key_sequence_drops_unbound_down_mouse_before_bound_click() { end_source: Default::default(), fringe: Default::default(), }], + logical_cursor_dimensions: None, }]); ev.command_loop.keyboard.pending_input_events.push_back( @@ -5441,6 +5447,7 @@ fn read_key_sequence_reduces_unbound_triple_mouse_to_bound_click() { assert_eq!(keys, vec![Value::symbol("mouse-1")]); } +/// Route mouse keys through the clicked buffer's local minor-mode maps. #[test] fn read_key_sequence_uses_clicked_window_buffer_local_minor_mode_maps() { crate::test_utils::init_test_tracing(); @@ -5506,6 +5513,7 @@ fn read_key_sequence_uses_clicked_window_buffer_local_minor_mode_maps() { .get_mut(fid) .expect("mutable frame") .commit_redisplay_cache_for_test(vec![crate::window::WindowDisplaySnapshot { + logical_cursor_dimensions: None, posn_matrix: None, point_rows: None, window_id: w2, @@ -5581,6 +5589,7 @@ fn read_key_sequence_uses_clicked_window_buffer_local_minor_mode_maps() { assert_eq!(ev.buffers.current_buffer_id(), Some(original_buffer)); } +/// Preserve the mode-line area prefix when resolving a mouse-key binding. #[test] fn read_key_sequence_prefixes_mode_line_mouse_click_for_lookup() { crate::test_utils::init_test_tracing(); @@ -5638,6 +5647,7 @@ fn read_key_sequence_prefixes_mode_line_mouse_click_for_lookup() { regions_materialized: true, body_rows: Vec::new(), text_area_left_offset: 0, + logical_cursor_dimensions: None, mode_line_height: 18, header_line_height: 0, tab_line_height: 0, diff --git a/crates/neovm-core/src/window/geometry.rs b/crates/neovm-core/src/window/geometry.rs index 0aea1de34f..0419903704 100644 --- a/crates/neovm-core/src/window/geometry.rs +++ b/crates/neovm-core/src/window/geometry.rs @@ -244,6 +244,8 @@ impl PresentationWindow { Self::from_snapshot_with_mode(snapshot, geometry_positions_mode()) } + /// Build one presentation without forcing deferred positions. Cursor paint + /// wins over explicit logical extent, then legacy canonical lookup is used. #[inline] fn from_snapshot_with_mode( snapshot: &WindowDisplaySnapshot, @@ -272,11 +274,18 @@ impl PresentationWindow { let positions = PresentationPositions::from_snapshot(snapshot, body_rows, row_heights, mode)?; let cursor = snapshot.logical_cursor_pos().and_then(|cursor| { - let (width, height) = positions.cursor_dimensions( - cursor.row, - cursor.col, - snapshot.phys_cursor.as_ref(), - )?; + // Paint takes precedence; hidden cursors retain the selected glyph's + // extent without guessing identity from potentially colliding columns. + let (width, height) = snapshot + .phys_cursor + .as_ref() + .map(|physical| (physical.width, physical.height)) + .or_else(|| { + snapshot + .logical_cursor_dimensions + .map(|extent| (i64::from(extent.width), i64::from(extent.height))) + }) + .or_else(|| positions.cursor_dimensions(cursor.row, cursor.col, None))?; Some(PresentationCursor { x: cursor.x, body_y: cursor.y, diff --git a/crates/neovm-core/src/window/mod.rs b/crates/neovm-core/src/window/mod.rs index 5ec558aec5..80dedb8fbb 100644 --- a/crates/neovm-core/src/window/mod.rs +++ b/crates/neovm-core/src/window/mod.rs @@ -2865,6 +2865,14 @@ impl WindowCursorPos { } } +/// Measured extent of the selected logical glyph, independent of cursor paint. +/// Zero-width glyphs remain representable; absence means no explicit extent. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct LogicalCursorDimensions { + pub width: u32, + pub height: u32, +} + /// Last authoritative physical cursor geometry for a window. #[derive(Clone, Debug, PartialEq, Eq)] pub struct WindowCursorSnapshot { @@ -3067,6 +3075,9 @@ pub struct WindowDisplaySnapshot { /// Intended cursor position in the redisplay result, even when no physical /// cursor was emitted. pub logical_cursor: Option, + /// Extent from the same selected glyph as the logical position. Legacy + /// snapshots may omit it and retain canonical source-point lookup. + pub logical_cursor_dimensions: Option, /// Last redisplay physical cursor geometry for this window, if the cursor /// was shown. pub phys_cursor: Option, @@ -3946,6 +3957,7 @@ impl PartialEq for WindowDisplaySnapshot { && self.tab_line_height == other.tab_line_height && self.chrome_strings == other.chrome_strings && self.logical_cursor == other.logical_cursor + && self.logical_cursor_dimensions == other.logical_cursor_dimensions && self.phys_cursor == other.phys_cursor && self.rows == other.rows && self.eob_overlay_motion == other.eob_overlay_motion @@ -3971,6 +3983,7 @@ impl Default for WindowDisplaySnapshot { tab_line_height: 0, chrome_strings: PresentedWindowChromeStrings::default(), logical_cursor: None, + logical_cursor_dimensions: None, phys_cursor: None, points: Vec::new(), point_rows: None, diff --git a/crates/neovm-core/src/window/tests/geometry_positions_test.rs b/crates/neovm-core/src/window/tests/geometry_positions_test.rs index 67a2ace829..64b1723c3e 100644 --- a/crates/neovm-core/src/window/tests/geometry_positions_test.rs +++ b/crates/neovm-core/src/window/tests/geometry_positions_test.rs @@ -154,6 +154,81 @@ fn physical_cursor_dimensions_do_not_materialize_deferred_positions() { assert!(!deferred.positions.is_materialized()); } +/// Explicit logical extents preserve glyph identity in flat, eager and deferred +/// positions without changing canonical source-point lookup or physical priority. +#[test] +fn logical_cursor_dimensions_preserve_selected_extent_and_zero_width() { + use crate::window::LogicalCursorDimensions; + let mut combining = point(1, 0, 0, 0); + combining.width = 0; + let mut snapshot = make_snapshot( + vec![DisplayPointRow::from_points(vec![combining.clone()])], + vec![mapping(0, 0, 0)], + ); + snapshot.logical_cursor = Some(WindowCursorPos { + row: 0, + col: 0, + x: 21, + y: 0, + }); + for descriptors in [true, false] { + let mut sample = snapshot.clone(); + if !descriptors { + sample.point_rows = None; + sample.points = vec![combining.clone()]; + } + for width in [0, 7] { + sample.logical_cursor_dimensions = Some(LogicalCursorDimensions { width, height: 21 }); + let (eager, deferred) = pair(&sample); + assert_eq!(eager.cursor, deferred.cursor); + let cursor = deferred.cursor.unwrap(); + assert_eq!( + (cursor.x, cursor.width, cursor.height), + (21, i64::from(width), 21) + ); + if descriptors { + assert!(!deferred.positions.is_materialized()); + } + assert_eq!( + view(&deferred) + .point_for_buffer_pos(LispCharPos1::new(1)) + .unwrap() + .unwrap() + .width() + .get(), + 0.0 + ); + } + sample.phys_cursor = Some(WindowCursorSnapshot { + kind: WindowCursorKind::Bar, + x: 21, + y: 0, + row: 0, + col: 0, + width: 2, + height: 15, + ascent: 12, + }); + assert_eq!(pair(&sample).1.cursor.unwrap().width, 2); + sample.phys_cursor = None; + sample.logical_cursor_dimensions = None; + assert_eq!( + pair(&sample).1.cursor.unwrap().width, + 0, + "legacy source order is unchanged" + ); + sample.logical_cursor = None; + sample.logical_cursor_dimensions = Some(LogicalCursorDimensions { + width: 7, + height: 21, + }); + assert!( + pair(&sample).1.cursor.is_none(), + "an extent alone is not a cursor" + ); + } +} + #[test] fn logical_cursor_fallback_preserves_marker_and_descriptor_tie_order() { let mut marker = point(4, 9, 3, 0); From cb17b6b78282e95de87dc40ec7f72295b71d2278 Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sun, 8 Nov 2026 00:27:34 +0000 Subject: [PATCH 07/10] Fix cursor selection authority and retained source freshness --- .../buffer_source/owned_capture/strings.rs | 2 +- .../src/buffer_source/render_plan.rs | 121 +++- .../src/cursor_source_dependencies.rs | 223 ++++++ .../src/display_cursor.rs | 13 +- .../src/display_row/builder.rs | 57 +- .../src/display_row/overlay_string.rs | 6 +- .../src/display_source.rs | 3 +- crates/neomacs-layout-engine/src/engine.rs | 106 ++- .../src/frame_visual_history.rs | 29 + .../src/incremental_layout.rs | 21 + crates/neomacs-layout-engine/src/lib.rs | 1 + .../neomacs-layout-engine/src/neovm_bridge.rs | 9 + .../src/neovm_bridge/borrowed_buffer.rs | 4 + .../src/neovm_bridge/buffer_snapshot.rs | 4 + .../neovm_bridge/tests/neovm_bridge_test.rs | 23 + .../tests/cursor_source_dependencies_test.rs | 47 ++ .../src/tests/engine_line_spacing_test.rs | 673 +++++++++++++++--- .../src/window_output.rs | 5 + crates/neovm-core/src/buffer/buffer.rs | 5 + .../src/emacs_core/runtime/owned_roots/mod.rs | 43 +- .../owned_roots/tests/owned_roots_test.rs | 18 + 21 files changed, 1287 insertions(+), 126 deletions(-) create mode 100644 crates/neomacs-layout-engine/src/cursor_source_dependencies.rs create mode 100644 crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs diff --git a/crates/neomacs-layout-engine/src/buffer_source/owned_capture/strings.rs b/crates/neomacs-layout-engine/src/buffer_source/owned_capture/strings.rs index d70bfad490..ef34a11d52 100644 --- a/crates/neomacs-layout-engine/src/buffer_source/owned_capture/strings.rs +++ b/crates/neomacs-layout-engine/src/buffer_source/owned_capture/strings.rs @@ -19,7 +19,7 @@ pub(super) fn bounded_string(value: Value, max_chars: usize) -> bool { if string.schars() > max_chars || string.as_bytes().len() > max_chars.saturating_mul(4) { return false; } - let props = neovm_core::emacs_core::value::get_string_text_properties_table_for_value(value); + let props = crate::cursor_source_dependencies::string_properties(value); let Some(props) = props else { return true; }; diff --git a/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs b/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs index fa7b32e3ca..9b6aa1d65d 100644 --- a/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs +++ b/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs @@ -49,7 +49,7 @@ use crate::window_output::{ use neomacs_display_protocol::frame_glyphs::{ CursorStyle, DisplaySlotId, GlyphRowRole, PhysCursor, }; -use neomacs_display_protocol::glyph_matrix::{FaceFillItem, GlyphArea, GlyphRow, GlyphType}; +use neomacs_display_protocol::glyph_matrix::{FaceFillItem, GlyphRow, GlyphType}; use neomacs_display_protocol::types::{Color, DisplayWindowId, Rect}; use neovm_core::buffer::BufferId; use neovm_core::window::{FrameId, WindowId}; @@ -113,6 +113,7 @@ fn render_or_retain_window_chrome( ), LogicalInputsChanged, > { + let _cursor_source_pause = crate::cursor_source_dependencies::pause(); let memo = match source { WindowChromeRowSource::Recompute => None, WindowChromeRowSource::RecomputeWithMemo(memo) => Some(memo), @@ -431,6 +432,52 @@ fn retained_point_cursor_metrics( Some((row.height_without_line_spacing(), row.ascent_px)) } +/// Publish an ordinary buffer-point selection without manufacturing paint or +/// font metrics. The caller must first exclude explicit string overrides. +fn publish_canonical_logical_cursor( + output: &mut TextWindowOutputTarget<'_>, + emitter: &mut WindowOutputEmitter, + window_id: u64, + row: usize, + point: usize, + text_area_left: f32, + char_width: f32, +) -> bool { + let Some(mut pixel_y) = output + .builder() + .current_window_row(row) + .map(|row| row.pixel_y) + else { + return false; + }; + // The retained empty ZV placeholder can still have y=0. As on painted + // replay, its logical position must not precede the previous body row. + if let Some(previous_bottom) = row + .checked_sub(1) + .and_then(|previous| output.builder().current_window_row(previous)) + .filter(|row| !row.role.is_chrome()) + .map(|row| row.pixel_y + row.height_px) + { + pixel_y = pixel_y.max(previous_bottom); + } + let Some(placement) = output.builder().resolve_finalized_cursor_placement( + CursorVisualColumnResolutionRequest::new(window_id as i64, row, point), + char_width, + ) else { + return false; + }; + emitter.set_logical_cursor( + neovm_core::window::WindowCursorPos { + x: (placement.x() - text_area_left).round() as i64, + y: pixel_y.round() as i64, + row: row as i64, + col: i64::from(placement.coordinates().output_col()), + }, + placement.logical_dimensions(), + ); + true +} + /// Publish a role-directed cursor on an installed scroll row. Return false /// when retained point metrics cannot prove fresh-layout geometry. #[allow(clippy::too_many_arguments)] @@ -837,6 +884,16 @@ impl BufferSourceOutputSetup { ); let has_overlays = !buffer.layout_overlays().is_empty(); + // Live, complete source absence is stronger than glyph provenance: + // even a string emitting only synthetic/control primitives may override point. + // This is O(1), excludes category/alias properties too, and caches no + // mutable string-property claim under buffer ticks. + let canonical_buffer_cursor = buffer.layout_text_props_is_empty() + && !has_overlays + && !local_display_policy.has_prefix() + && !buffer + .layout_buffer_local_value(crate::neovm_bridge::LayoutVar::DefaultTextProperties) + .is_some_and(|value| value.is_cons()); let face_resolution = BufferSourceFaceResolutionContext::new( buffer, face_resolver.resolver(), @@ -923,10 +980,22 @@ impl BufferSourceOutputSetup { // rebuild (honest layering, spec §4.6) — the win is skipping fontify / // shaping / measurement for the body. if let Some(mut replay) = cursor_only { + // Row coverage alone cannot establish a hidden explicit selection. + // Unknown or moved semantic authority requires a fresh source walk. + if replay.retained_logical_cursor.is_none() + && !canonical_buffer_cursor + && cursor_style_for_window(params).is_none() + { + output.restore_retry_checkpoint(retry_checkpoint); + return BufferSourceRenderAttemptOutcome::ReplayMispredicted; + } // Taken before the replay's rows are moved into the grid below. let retained_chrome = replay.chrome.take(); let chrome_memo = replay.chrome_memo.take(); let mut output_emitter = output.begin_text_window_output(self.begin_request); + if let Some(logical) = replay.retained_logical_cursor.take() { + output_emitter.set_logical_cursor(logical.position, logical.dimensions); + } // Capture the point glyph's metrics from the new cursor row before the // rows are moved into the grid (fall back to the window face metrics @@ -964,7 +1033,8 @@ impl BufferSourceOutputSetup { cursor_width = glyph.pixel_width; cursor_stretch_like = matches!(glyph.glyph_type, GlyphType::Stretch { .. }); } - if replay.retained_cursor.is_none() { + if replay.retained_cursor.is_none() && cursor_style_for_window(params).is_some() + { let buffer_newline = buf_access .byte_at(buf_access.charpos_to_bytepos(replay.new_point)) == Some(b'\n'); @@ -1068,6 +1138,24 @@ impl BufferSourceOutputSetup { let display_window_id = DisplayWindowId::new(window_id_i64); // Cursor visibility/style belongs to current window policy, not // retained row decoration: absent decoration must stay absent. + if !output_emitter.has_logical_cursor() + && canonical_buffer_cursor + && !publish_canonical_logical_cursor( + &mut output, + &mut output_emitter, + output_window_id, + replay.new_cursor_row_index, + replay.new_point as usize, + walk_setup.text_area_left, + geometry.char_width, + ) + { + crate::window_output::restore_text_window_retry_checkpoint( + output.reborrow(), + retry_checkpoint, + ); + return BufferSourceRenderAttemptOutcome::ReplayMispredicted; + } if let Some(style) = cursor_style_for_window(params) { let cursor = if let Some(retained) = replay.retained_cursor.as_ref() { FastPathCursorPlacement::Retained(retained.clone()) @@ -1618,6 +1706,25 @@ impl BufferSourceOutputSetup { .builder() .find_current_window_cursor_row(scroll.new_point as usize), }; + if let Some(cursor_row) = cursor_row + && canonical_buffer_cursor + && !output_emitter.has_logical_cursor() + && !publish_canonical_logical_cursor( + &mut output, + &mut output_emitter, + output_window_id, + cursor_row, + scroll.new_point as usize, + walk_setup.text_area_left, + geometry.char_width, + ) + { + crate::window_output::restore_text_window_retry_checkpoint( + output.reborrow(), + retry_checkpoint, + ); + return BufferSourceRenderAttemptOutcome::ReplayMispredicted; + } if let Some(cursor_row) = cursor_row && let Some(style) = cursor_style_for_window(params) { @@ -1646,6 +1753,16 @@ impl BufferSourceOutputSetup { } } + // Partial walks may find semantic authority freshly. Reused rows + // alone do not certify hidden explicit string-cursor overrides. + if cursor_style_for_window(params).is_none() && !output_emitter.has_logical_cursor() { + crate::window_output::restore_text_window_retry_checkpoint( + output.reborrow(), + retry_checkpoint, + ); + return BufferSourceRenderAttemptOutcome::ReplayMispredicted; + } + // A replay may not be SEALED without the frame's physical cursor. // // GNU's reuse gives the optimization up rather than proceed without diff --git a/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs b/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs new file mode 100644 index 0000000000..1c0c4f9c9e --- /dev/null +++ b/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs @@ -0,0 +1,223 @@ +//! Owner-only observations of every string projected by a canonical window walk. +//! Numeric snapshots and worker rows deliberately carry no such certificate. + +#[cfg(test)] +#[path = "tests/cursor_source_dependencies_test.rs"] +mod tests; +use neovm_core::{ + emacs_core::{ + Context, Value, + owned_roots::{OwnedRootRetainer, OwnedRoots}, + }, + tagged::collection_reads::{self, CollectionReads}, +}; +use std::{cell::RefCell, rc::Rc}; + +#[derive(Debug)] +struct Observation { + reads: Vec, + roots: Vec, + strings: rustc_hash::FxHashSet, + retainer: OwnedRootRetainer, + owner: Option, + complete: bool, +} +thread_local! { static ACTIVE: RefCell> = const { RefCell::new(None) }; } + +/// Complete original string observations, kept alive by evaluator-owned roots. +/// Rc clones preserve the original read certificate across unchanged replay. +#[derive(Clone, Debug)] +pub(crate) struct CursorSourceDependencies(Rc); +#[derive(Debug)] +enum DependencyAuthority { + Canonical { + owner: CollectionReads, + reads: Vec, + _roots: Vec, + strings: rustc_hash::FxHashSet, + }, + // Validate a partial walk with newly introduced sources for this frame, + // then require a canonical walk. Never grow an unbounded history chain. + FrameOnly { + previous: CursorSourceDependencies, + fresh: CursorSourceDependencies, + }, +} +impl CursorSourceDependencies { + /// Validate original mutator identity and bounded projected-write history; + /// propagate reads transitively, never rebase a skipped source walk. + pub(crate) fn unchanged(&self) -> bool { + match self.0.as_ref() { + DependencyAuthority::Canonical { owner, reads, .. } => { + owner.unchanged_and_observe() + && reads.iter().all(CollectionReads::unchanged_and_observe) + } + DependencyAuthority::FrameOnly { previous, fresh } => { + previous.unchanged() && fresh.unchanged() + } + } + } + /// Expanded partial-source domains validate this frame only; the next walk + /// resets them instead of indefinitely retaining removed strings/leases. + pub(crate) fn can_replay(&self) -> bool { + matches!(self.0.as_ref(), DependencyAuthority::Canonical { .. }) + } + /// Unchanged original inputs certify retained rows, while actual new reads + /// certify walked rows. Keep the ORIGINAL certificate for a known domain; + /// new sources require a bounded frame-only union, not a rebased witness. + pub(crate) fn after_partial_walk(&self, fresh: Self) -> Option { + let DependencyAuthority::Canonical { + strings: previous, .. + } = self.0.as_ref() + else { + return None; + }; + let DependencyAuthority::Canonical { + strings: current, .. + } = fresh.0.as_ref() + else { + return None; + }; + if previous.is_empty() { + // Retained rows had no string inputs; all current source inputs + // were actually observed in the newly walked rows. + Some(fresh) + } else if current.is_subset(previous) { + Some(self.clone()) + } else { + Some(Self(Rc::new(DependencyAuthority::FrameOnly { + previous: self.clone(), + fresh, + }))) + } + } +} + +/// Chrome strings cannot override the buffer cursor. Nested canonical window +/// walks inside chrome callbacks still install their own independent capture. +pub(crate) struct Pause(Option); +/// Suspend only the enclosing window's observation while producing chrome. +pub(crate) fn pause() -> Pause { + Pause(ACTIVE.with(|active| active.borrow_mut().take())) +} +impl Drop for Pause { + fn drop(&mut self) { + ACTIVE.with(|active| { + active.replace(self.0.take()); + }); + } +} + +/// Lexical producer scope; retries and unwinding discard speculative reads. +/// Strings receive original-evaluator roots immediately, before callbacks or GC; +/// neither scratch-stack slots nor borrowed Context pointers escape the walk. +pub(crate) struct Capture { + previous: Option, +} +impl Capture { + /// Start an original owner-world witness and a weak evaluator root factory. + /// A nested walk replaces, rather than merges into, its caller's capture. + pub(crate) fn begin(context: &Context) -> Self { + let (_, owner) = collection_reads::capture(|| ()); + let previous = ACTIVE.with(|active| { + active.replace(Some(Observation { + reads: Vec::new(), + roots: Vec::new(), + strings: rustc_hash::FxHashSet::default(), + complete: owner.is_some(), + owner, + retainer: context.owned_root_retainer(), + })) + }); + Self { previous } + } + /// Detect mutations among successfully observed inputs. Completeness is + /// checked separately by finish: missing instrumentation grants no reuse. + pub(crate) fn unchanged(&self) -> bool { + ACTIVE.with(|active| { + active.borrow().as_ref().is_some_and(|observation| { + observation + .reads + .iter() + .all(CollectionReads::unchanged_and_observe) + }) + }) + } + /// Seal original rooted observations after callbacks and row admission. + /// Unknown/overflowed captures cannot become an empty absence certificate. + pub(crate) fn finish(self) -> Option { + if !self.unchanged() { + return None; + } + let observation = ACTIVE.with(|active| active.borrow_mut().take())?; + if !observation.complete { + return None; + } + Some(CursorSourceDependencies(Rc::new( + DependencyAuthority::Canonical { + owner: observation.owner?, + reads: observation.reads, + _roots: observation.roots, + strings: observation.strings, + }, + ))) + } +} +impl Drop for Capture { + fn drop(&mut self) { + ACTIVE.with(|active| { + active.replace(self.previous.take()); + }); + } +} + +/// Observe presence AND absence of intervals, before selection filters or +/// early winner selection. Property-less competing strings may gain `cursor`. +pub(crate) fn string_properties( + value: Value, +) -> Option { + let retainer = ACTIVE.with(|active| { + let mut active = active.borrow_mut(); + let observation = active.as_mut()?; + if !observation + .owner + .as_ref() + .is_some_and(CollectionReads::unchanged_and_observe) + { + observation.complete = false; + return None; + } + Some(observation.retainer.clone()) + }); + let Some(roots) = retainer.and_then(|retainer| retainer.retain([value])) else { + ACTIVE.with(|active| { + if let Some(observation) = active.borrow_mut().as_mut() { + observation.complete = false; + } + }); + return neovm_core::emacs_core::value::get_string_text_properties_table_for_value(value); + }; + let (properties, reads) = collection_reads::capture(|| { + // The cursor property is indexed in the original string. Observe its + // contents/storage too, without copying/scanning bytes, so in-place + // string edits cannot retain an old source slot or glyph extent. + if let Some(string) = value.as_lisp_string() { + let _ = string.as_bytes(); + let _ = string.is_multibyte(); + let _ = string.schars(); + } + neovm_core::emacs_core::value::get_string_text_properties_table_for_value(value) + }); + ACTIVE.with(|active| { + if let Some(observation) = active.borrow_mut().as_mut() { + observation.roots.push(roots); + observation.strings.insert(value); + if let Some(reads) = reads { + observation.reads.push(reads); + } else { + observation.complete = false; + } + } + }); + properties +} diff --git a/crates/neomacs-layout-engine/src/display_cursor.rs b/crates/neomacs-layout-engine/src/display_cursor.rs index 95b5edbdbe..fd5fb33793 100644 --- a/crates/neomacs-layout-engine/src/display_cursor.rs +++ b/crates/neomacs-layout-engine/src/display_cursor.rs @@ -1609,7 +1609,7 @@ impl<'a> CapturedTextWindowCursorPublishContext<'a> { // Logical anchors and physical publication must accept the same // integer override only when bidi did not move the selected slot. // Explicit string-cursor candidates already have resolved placement. - let (finalized_x, logical_dimensions) = + let (finalized_x, finalized_col, logical_dimensions) = if cursor.slot_state == CursorSlotResolutionState::Unresolved { output .builder() @@ -1621,12 +1621,17 @@ impl<'a> CapturedTextWindowCursorPublishContext<'a> { ), self.char_w, ) - .map_or((cursor.x, None), |placement| { - (placement.x(), placement.logical_dimensions()) + .map_or((cursor.x, logical_cursor.col, None), |placement| { + ( + placement.x(), + i64::from(placement.coordinates().output_col()), + placement.logical_dimensions(), + ) }) } else { - (cursor.x, cursor.logical_dimensions) + (cursor.x, logical_cursor.col, cursor.logical_dimensions) }; + logical_cursor.col = finalized_col; logical_cursor.x = crate::window_output::cursor_snapshot_x( cursor.x, finalized_x, diff --git a/crates/neomacs-layout-engine/src/display_row/builder.rs b/crates/neomacs-layout-engine/src/display_row/builder.rs index 0528ebea52..8e9bf3bf0a 100644 --- a/crates/neomacs-layout-engine/src/display_row/builder.rs +++ b/crates/neomacs-layout-engine/src/display_row/builder.rs @@ -1352,7 +1352,7 @@ pub(crate) struct DisplayRowGlyphSlot { // Primitive boundary relative to the start of this text append. Source // characters and stored glyphs differ for padding and composition. text_glyph_offset: Option, - glyph_height_px: Option, + selected_primitive_dimensions: Option, } /// Source positions and paint primitives are not one-to-one. Composition @@ -1398,7 +1398,7 @@ impl DisplayRowGlyphSlot { }, default_cell_height: false, text_glyph_offset: None, - glyph_height_px: None, + selected_primitive_dimensions: None, } } @@ -1422,15 +1422,22 @@ impl DisplayRowGlyphSlot { self } - /// Keep the emitted glyph's measured height with its source slot; row - /// defaults and composition members are not measurements of this glyph. - fn with_glyph_height(mut self, height: Option) -> Self { - self.glyph_height_px = height.filter(|height| height.is_finite() && *height > 0.0); + /// Preserve both dimensions from one emitted primitive, not the aggregate + /// advance of a source character that may expand into several glyphs. + fn with_selected_primitive(mut self, glyph: Option<&Glyph>) -> Self { + self.selected_primitive_dimensions = glyph.and_then(|glyph| { + crate::display_cursor::measured_logical_cursor_dimensions( + glyph.pixel_width, + glyph.pixel_height, + ) + }); self } - pub(crate) fn glyph_height_px(&self) -> Option { - self.glyph_height_px + pub(crate) fn selected_primitive_dimensions( + &self, + ) -> Option { + self.selected_primitive_dimensions } pub(crate) fn text_glyph_offset(&self) -> Option { @@ -2143,8 +2150,7 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer written.width_cols(), item.pointer_appearance.clone(), ) - .with_text_glyph_offset(char_offset.get()) - .with_glyph_height(Some(glyph.pixel_height)), + .with_text_glyph_offset(char_offset.get()), ); self.writer.row.glyphs[GlyphArea::Text.index()].push(glyph); self.advance(written); @@ -2159,6 +2165,18 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer item.box_vertical_edges, true, ); + // Retained layout can still mutate space extents. Read the paired + // measurement only after all item layout has been applied. + for (slot, glyph) in slots + .iter_mut() + .zip(&self.writer.row.glyphs[GlyphArea::Text.index()][glyph_start..]) + { + slot.selected_primitive_dimensions = + crate::display_cursor::measured_logical_cursor_dimensions( + glyph.pixel_width, + glyph.pixel_height, + ); + } Some(DisplayRowAppendProgress::new( start_position, self.position, @@ -2427,10 +2445,8 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer written.width_cols(), pointer_appearance.clone(), ) - .with_glyph_height( - self.writer.row.glyphs[self.writer.area_index()] - .get(before_len) - .map(|glyph| glyph.pixel_height), + .with_selected_primitive( + self.writer.row.glyphs[self.writer.area_index()].get(before_len), ), ); } @@ -2622,11 +2638,7 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer ch == '\t' && matches!(source_mapping, DisplayTextSourceMapping::NaturalText), ) .with_text_glyph_offset(before_len - glyph_start) - .with_glyph_height( - self.writer.row.glyphs[area_index] - .get(before_len) - .map(|glyph| glyph.pixel_height), - ), + .with_selected_primitive(self.writer.row.glyphs[area_index].get(before_len)), ); self.advance(written); metrics.add(written); @@ -2706,11 +2718,8 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer pointer_appearance.cloned(), ); if char_offset == 0 { - slot = slot.with_glyph_height( - self.writer.row.glyphs[area_index] - .get(before_len) - .map(|glyph| glyph.pixel_height), - ); + slot = slot + .with_selected_primitive(self.writer.row.glyphs[area_index].get(before_len)); } slot.coverage = if char_offset == 0 { DisplayRowGlyphCoverage::Primitive { source_chars } diff --git a/crates/neomacs-layout-engine/src/display_row/overlay_string.rs b/crates/neomacs-layout-engine/src/display_row/overlay_string.rs index 12ec16cff4..ae0269ab45 100644 --- a/crates/neomacs-layout-engine/src/display_row/overlay_string.rs +++ b/crates/neomacs-layout-engine/src/display_row/overlay_string.rs @@ -5,6 +5,7 @@ use crate::buffer_source::loop_state::BufferSourceRowCarryoverState; use crate::buffer_source::row_prelude::BufferSourceRowPreludeRequestContext; use crate::coords::layout_char_pos_from_i64; +use crate::cursor_source_dependencies::string_properties as get_string_text_properties_table_for_value; use crate::display_cursor::{ CapturedCursorInfo, CapturedCursorPlacement, CapturedCursorSlotWidth, CapturedCursorVisualState, CursorCaptureState, DisplayStringCursorContext, @@ -44,7 +45,6 @@ use crate::types::{LayoutCharPos0, WindowParams}; use neomacs_display_protocol::types::Px; use neovm_core::buffer::{CharPos0, DisplayColumn}; use neovm_core::emacs_core::Value; -use neovm_core::emacs_core::value::get_string_text_properties_table_for_value; #[derive(Clone, Copy)] pub(crate) struct OverlayStringRenderSource { @@ -1005,9 +1005,7 @@ fn capture_overlay_string_cursor_at_slot( visual_state, CapturedCursorSlotWidth::Explicit(slot.width_px()), cursor_context, - slot.glyph_height_px().and_then(|height| { - crate::display_cursor::measured_logical_cursor_dimensions(slot.width_px(), height) - }), + slot.selected_primitive_dimensions(), ); } diff --git a/crates/neomacs-layout-engine/src/display_source.rs b/crates/neomacs-layout-engine/src/display_source.rs index e58e5e6fd5..a35830d9a2 100644 --- a/crates/neomacs-layout-engine/src/display_source.rs +++ b/crates/neomacs-layout-engine/src/display_source.rs @@ -2,6 +2,7 @@ //! scalar materializer; unset or `bulk` copies validated ordinary UTF-8 runs. //! The selector is native immutable process configuration, read once. +use crate::cursor_source_dependencies::string_properties as get_string_text_properties_table_for_value; use crate::display_item::{ BufferDisplayReplacementSource, DisplayGlyphless, DisplayItem, DisplayItemKind, DisplayItemLayout, DisplayLength, DisplayLineHeightPolicy, DisplayLineSpacingPolicy, @@ -38,7 +39,7 @@ use neovm_core::emacs_core::composite::composition_display_text_for_property; use neovm_core::emacs_core::composite::{AutomaticCompositionRules, AutomaticCompositionSpan}; use neovm_core::emacs_core::emacs_char::EmacsChar; use neovm_core::emacs_core::intern::WellKnown; -use neovm_core::emacs_core::value::{get_string_text_properties_table_for_value, list_to_vec}; +use neovm_core::emacs_core::value::list_to_vec; use neovm_core::face::LispFaceId; // Each key is a fixed DEFSYM id resolved at its original query boundary. Cells diff --git a/crates/neomacs-layout-engine/src/engine.rs b/crates/neomacs-layout-engine/src/engine.rs index 2b8db873e0..1a60d8a165 100644 --- a/crates/neomacs-layout-engine/src/engine.rs +++ b/crates/neomacs-layout-engine/src/engine.rs @@ -1127,7 +1127,7 @@ impl FrameWindowEndAttempts { } } -/// GNU-visible callback acknowledgements owned by one logical frame attempt. +/// Callback acknowledgements and producer artifacts of one logical frame attempt. /// /// Chrome/minibuffer convergence may perform several physical row walks. Those /// retries must not replay Lisp that the logical redisplay already ran. @@ -1139,6 +1139,11 @@ struct RedisplayLispLedger { mini_eob_scroll: mini_eob_scroll::MiniEobScrollLedger, /// `when` answers evaluated after a walk, carried to its one replay. display_when: crate::display_when::DisplayWhenReplayLedger, + /// Original string observations commit only with the accepted matrices. + cursor_sources: rustc_hash::FxHashMap< + DisplayWindowId, + crate::cursor_source_dependencies::CursorSourceDependencies, + >, } impl RedisplayLispLedger { @@ -3566,6 +3571,28 @@ impl LayoutEngine { continue 'frame_layout; } + // A later leaf's Lisp can mutate an earlier leaf's source string + // without changing buffer/overlay ticks. Validate the complete + // accepted set after the final Lisp-capable boundary, not merely + // when each individual leaf finishes. + if let Some(window_id) = curr_window_infos.keys().copied().find(|window| { + lisp_ledger + .cursor_sources + .get(window) + .is_some_and(|sources| !sources.unchanged()) + }) { + if !Self::accept_frame_relayout_request( + &mut layout_coordinator, + evaluator, + &mut frame_window_end_attempts, + presentation_id, + FrameRelayoutRequest::LogicalInputsChanged { window_id }, + ) { + return None; + } + continue 'frame_layout; + } + let accepted_window_chrome_metrics: rustc_hash::FxHashMap< DisplayWindowId, WindowChromeMetrics, @@ -4204,12 +4231,17 @@ impl LayoutEngine { // set the retained-matrix commit writes); a window that settled but // did not reach the presentation is not evidence. accepted_display_when.retain(|window, _| curr_window_infos.contains_key(window)); + let mut cursor_sources = std::mem::take(&mut lisp_ledger.cursor_sources); + cursor_sources.retain(|window, _| curr_window_infos.contains_key(window)); + self.frame_visual_histories + .retain_live(|frame| evaluator.frame_manager().get(frame).is_some()); self.frame_visual_histories.commit( frame_id, FrameVisualHistory::from_accepted_presentation( curr_window_infos, accepted_display_when, - ), + ) + .with_cursor_sources(cursor_sources), ); { let frame_manager = evaluator.frame_manager_mut(); @@ -4803,6 +4835,16 @@ impl LayoutEngine { ); return None; }; + // The row key cannot see interval mutations on an existing string. + // A missing/expired producer certificate must never retain selection. + if !self + .retained_frame + .and_then(|frame| self.frame_visual_histories.get(frame)) + .and_then(|history| history.cursor_sources_for(window_id)) + .is_some_and(|sources| sources.can_replay() && sources.unchanged()) + { + return None; + } let curr_key = RetainedWindowKey::from_params(params, layout_box, evaluator); let mut replay = match prev.cursor_only_replay(&curr_key) { Ok(replay) => replay, @@ -5434,6 +5476,22 @@ impl LayoutEngine { topology_generation: u64, face_attempt: &FrameFaceAttempt, ) -> LeafLayoutAttempt { + let source_capture = crate::cursor_source_dependencies::Capture::begin(evaluator); + let source_window = DisplayWindowId::new(params.window_id); + lisp_ledger.cursor_sources.remove(&source_window); + let previous_sources = self + .frame_visual_histories + .get(frame_id) + .and_then(|history| history.cursor_sources_for(source_window)); + let sources_current = previous_sources + .as_ref() + .is_some_and(|sources| sources.can_replay() && sources.unchanged()); + let cursor_only_replay = if sources_current { + cursor_only_replay + } else { + None + }; + let scroll_replay = if sources_current { scroll_replay } else { None }; let face_resolver = face_services.resolver(); #[cfg(test)] let _viewport_retry_depth = viewport_retry_depth_probe::Guard::enter(); @@ -5761,6 +5819,19 @@ impl LayoutEngine { ViewportResolutionPhase::Measure(measurement) => Some(measurement.clone()), ViewportResolutionPhase::Resolve | ViewportResolutionPhase::Commit(_) => None, }); + // Conditional display/face callbacks above may have changed a source + // after Phase A admission. Validate again at consumption, not just lookup. + let sources_current = previous_sources + .as_ref() + .is_some_and(|sources| sources.can_replay() && sources.unchanged()); + let cursor_only_replay = if sources_current { + cursor_only_replay + } else { + None + }; + let scroll_replay = if sources_current { scroll_replay } else { None }; + let canonical_source_walk = cursor_only_replay.is_none() && scroll_replay.is_none(); + let unchanged_body_replay = cursor_only_replay.is_some(); let render_outcome = render_request.render_into( BufferSourceRenderAttemptContext::from_frame_output_owner( &mut self.frame_output, @@ -5836,6 +5907,11 @@ impl LayoutEngine { }; if evaluator.frame_manager().window_topology_generation() != topology_generation || !lisp_boundaries_remain_valid + || !source_capture.unchanged() + || (!canonical_source_walk + && !previous_sources + .as_ref() + .is_some_and(|sources| sources.unchanged())) { if let Some(attempt) = window_end_attempt.take() { evaluator.reject_redisplay_window_end_attempt(attempt); @@ -6256,6 +6332,32 @@ impl LayoutEngine { reused_matrix_rows, line_number_field_width, } => { + // Only the canonical producer establishes completeness. A + // cursor-only walk carries the original certificate, never an + // empty observation from skipping the strings. Partial walks + // preserve original known domains; newly expanded domains + // validate this frame but require a fresh canonical next walk. + if !source_capture.unchanged() + || (!canonical_source_walk + && !previous_sources + .as_ref() + .is_some_and(|sources| sources.unchanged())) + { + return LeafLayoutAttempt::LogicalInputsChanged; + } + let observed = source_capture.finish(); + let sources = if canonical_source_walk { + observed + } else if unchanged_body_replay { + previous_sources + } else { + previous_sources + .as_ref() + .and_then(|old| observed.and_then(|fresh| old.after_partial_walk(fresh))) + }; + if let Some(sources) = sources { + lisp_ledger.cursor_sources.insert(source_window, sources); + } if position_publication.is_synchronous_query() { self.query_row_coverage = query_row_coverage; self.query_target_prefix_policy = query_target_prefix_policy; diff --git a/crates/neomacs-layout-engine/src/frame_visual_history.rs b/crates/neomacs-layout-engine/src/frame_visual_history.rs index da949c2926..fb3c944637 100644 --- a/crates/neomacs-layout-engine/src/frame_visual_history.rs +++ b/crates/neomacs-layout-engine/src/frame_visual_history.rs @@ -23,6 +23,8 @@ pub(crate) struct FrameVisualHistory { /// type is embedded inline in the thread-local `ScrollCoverage`, so /// growing it would move every `%fs` offset in the binary. display_when: FxHashMap, + cursor_sources: + FxHashMap, } impl FrameVisualHistory { @@ -33,9 +35,31 @@ impl FrameVisualHistory { Self { window_infos, display_when, + cursor_sources: FxHashMap::default(), } } + /// Attach owner-only source certificates to this accepted presentation, + /// without growing TLS-embedded matrices or public numeric snapshots. + pub(crate) fn with_cursor_sources( + mut self, + sources: FxHashMap< + DisplayWindowId, + crate::cursor_source_dependencies::CursorSourceDependencies, + >, + ) -> Self { + self.cursor_sources = sources; + self + } + + /// Clone the original certificate for exactly this accepted window. + pub(crate) fn cursor_sources_for( + &self, + window: DisplayWindowId, + ) -> Option { + self.cursor_sources.get(&window).cloned() + } + pub(crate) fn window_infos(&self) -> &FxHashMap { &self.window_infos } @@ -73,6 +97,11 @@ impl FrameVisualHistories { self.by_frame.get(&frame_id) } + /// Closing a frame retires its original string roots with its history. + pub(crate) fn retain_live(&mut self, mut live: impl FnMut(FrameId) -> bool) { + self.by_frame.retain(|frame, _| live(*frame)); + } + /// Commit history only after the matching frame presentation has sealed. pub(crate) fn commit(&mut self, frame_id: FrameId, history: FrameVisualHistory) { self.by_frame.insert(frame_id, history); diff --git a/crates/neomacs-layout-engine/src/incremental_layout.rs b/crates/neomacs-layout-engine/src/incremental_layout.rs index 2eb327c753..169189870e 100644 --- a/crates/neomacs-layout-engine/src/incremental_layout.rs +++ b/crates/neomacs-layout-engine/src/incremental_layout.rs @@ -651,10 +651,20 @@ pub struct CursorOnlyReplay { /// This also preserves explicit display-string `cursor` placement; /// reconstructing from buffer point would lose that semantic override. pub(crate) retained_cursor: Option, + /// Semantic selection survives unchanged placement independently of paint. + pub(crate) retained_logical_cursor: Option, /// Sealed frame-face generation that owns every ID in `body_rows`. pub(crate) face_generation: FrameFaceGeneration, } +/// Accepted semantic cursor authority, including a measured extent when known. +/// A nonpainting selection is not the same as absence of a selection. +#[derive(Clone, Debug)] +pub(crate) struct RetainedLogicalCursor { + pub(crate) position: neovm_core::window::WindowCursorPos, + pub(crate) dimensions: Option, +} + /// A retained cursor's renderer placement paired with its live-window output /// coordinate. Neither half is meaningful as a cursor replay without the /// other, so the incremental boundary never exposes two independent options. @@ -1612,6 +1622,17 @@ impl RetainedWindowMatrix { chrome: None, chrome_memo: None, retained_cursor, + retained_logical_cursor: if point_moved { + None + } else { + self.display_snapshot + .logical_cursor + .clone() + .map(|position| RetainedLogicalCursor { + position, + dimensions: self.display_snapshot.logical_cursor_dimensions, + }) + }, face_generation: self.face_generation, }) } diff --git a/crates/neomacs-layout-engine/src/lib.rs b/crates/neomacs-layout-engine/src/lib.rs index 9632f155d8..bdb94845eb 100644 --- a/crates/neomacs-layout-engine/src/lib.rs +++ b/crates/neomacs-layout-engine/src/lib.rs @@ -12,6 +12,7 @@ pub mod bidi; pub(crate) mod buffer_source; pub mod composition; pub(crate) mod coords; +pub(crate) mod cursor_source_dependencies; pub(crate) mod display_current_row_output; pub(crate) mod display_cursor; pub(crate) mod display_face_layout; diff --git a/crates/neomacs-layout-engine/src/neovm_bridge.rs b/crates/neomacs-layout-engine/src/neovm_bridge.rs index d65b850ad8..fab6a35c27 100644 --- a/crates/neomacs-layout-engine/src/neovm_bridge.rs +++ b/crates/neomacs-layout-engine/src/neovm_bridge.rs @@ -194,6 +194,11 @@ pub(crate) trait LayoutBufferView { fn layout_indexed_newline_count(&self, _range: EmacsByteRange) -> Option { None } + /// Prove the complete source has no text properties, including categories + /// and aliases. Projected/unknown views must conservatively return false. + fn layout_text_props_is_empty(&self) -> bool { + false + } fn layout_text_prop_at_emacs_byte_pos(&self, pos: EmacsBytePos, name: Value) -> Option; /// Return PROPERTY from CATEGORY's symbol plist as captured for this /// immutable layout view. Live-buffer test adapters have no evaluator @@ -461,6 +466,10 @@ impl LayoutBufferView for Buffer { self.indexed_newline_count(range, neovm_core::buffer::LineIndexBuild::IfLarge) } + fn layout_text_props_is_empty(&self) -> bool { + self.text_props_is_empty() + } + fn layout_text_prop_at_emacs_byte_pos(&self, pos: EmacsBytePos, name: Value) -> Option { self.text_props_get_property_at_emacs_byte_pos(pos, name) } diff --git a/crates/neomacs-layout-engine/src/neovm_bridge/borrowed_buffer.rs b/crates/neomacs-layout-engine/src/neovm_bridge/borrowed_buffer.rs index ecb5e3d1a1..4975eba74f 100644 --- a/crates/neomacs-layout-engine/src/neovm_bridge/borrowed_buffer.rs +++ b/crates/neomacs-layout-engine/src/neovm_bridge/borrowed_buffer.rs @@ -192,6 +192,10 @@ impl LayoutBufferView for BorrowedLayoutBuffer<'_> { .previous_syntax_prop_state_run_start_at_char_pos(pos) } + fn layout_text_props_is_empty(&self) -> bool { + self.buffer.text_props_is_empty() + } + fn layout_overlays(&self) -> &OverlayList { self.buffer.overlays() } diff --git a/crates/neomacs-layout-engine/src/neovm_bridge/buffer_snapshot.rs b/crates/neomacs-layout-engine/src/neovm_bridge/buffer_snapshot.rs index 0f44e752fc..f8054e720b 100644 --- a/crates/neomacs-layout-engine/src/neovm_bridge/buffer_snapshot.rs +++ b/crates/neomacs-layout-engine/src/neovm_bridge/buffer_snapshot.rs @@ -548,6 +548,10 @@ impl LayoutBufferView for LayoutBufferSnapshot { .indexed_newline_count(range, neovm_core::buffer::LineIndexBuild::IfLarge) } + fn layout_text_props_is_empty(&self) -> bool { + self.text_snapshot.text_props_is_empty() + } + fn layout_text_prop_at_emacs_byte_pos(&self, pos: EmacsBytePos, name: Value) -> Option { self.text_snapshot.text_prop_at_emacs_byte_pos(pos, name) } diff --git a/crates/neomacs-layout-engine/src/neovm_bridge/tests/neovm_bridge_test.rs b/crates/neomacs-layout-engine/src/neovm_bridge/tests/neovm_bridge_test.rs index 243a56e2db..10e8315b27 100644 --- a/crates/neomacs-layout-engine/src/neovm_bridge/tests/neovm_bridge_test.rs +++ b/crates/neomacs-layout-engine/src/neovm_bridge/tests/neovm_bridge_test.rs @@ -1764,6 +1764,29 @@ fn layout_buffer_snapshot_is_text_backend_neutral_for_positions_bytes_and_proper } } +/// Absence is about all properties, including unrecognized aliases/categories, +/// and remains tied to the immutable source rather than later buffer mutation. +#[test] +fn layout_property_absence_proof_is_complete_and_snapshot_stable() { + let _eval = neovm_core::emacs_core::Context::new(); + for kind in BufferTextBackendKind::implemented_variants() { + let mut buffer = + test_buffer_with_backend(300 + u64::from(u8::from(kind)), "*property-proof*", kind); + set_buffer_text(&mut buffer, "abc"); + let empty = LayoutBufferSnapshot::from_buffer(&buffer); + assert!(buffer.layout_text_props_is_empty()); + assert!(empty.layout_text_props_is_empty()); + buffer.put_text_property(0, 1, Value::symbol("unrecognized-display-alias"), Value::T); + let occupied = LayoutBufferSnapshot::from_buffer(&buffer); + assert!(!buffer.layout_text_props_is_empty()); + assert!(!occupied.layout_text_props_is_empty()); + assert!( + empty.layout_text_props_is_empty(), + "{kind:?}: frozen proof changed" + ); + } +} + // ----------------------------------------------------------------------- // RustTextPropAccess tests // ----------------------------------------------------------------------- diff --git a/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs b/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs new file mode 100644 index 0000000000..78977b80fd --- /dev/null +++ b/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs @@ -0,0 +1,47 @@ +use super::*; + +/// Partial walks preserve original read origins without accumulating a chain +/// of removed source strings; new strings after an empty domain are complete. +#[test] +fn partial_source_domains_preserve_original_authority_and_bound_history() { + for target in ["neo-source-first", "neo-source-second"] { + let mut context = Context::new(); + let first = context + .eval_str("(setq neo-source-first \"first\")") + .unwrap(); + let second = context + .eval_str("(setq neo-source-second \"second\")") + .unwrap(); + let capture = |values: &[Value]| { + let scope = Capture::begin(&context); + for value in values { + let _ = string_properties(*value); + } + scope.finish().unwrap() + }; + let empty = capture(&[]); + let original = capture(&[first]); + let introduced = empty.after_partial_walk(original.clone()).unwrap(); + assert!(introduced.can_replay()); + assert!(Rc::ptr_eq(&introduced.0, &original.0)); + let same = original.after_partial_walk(capture(&[first])).unwrap(); + assert!( + Rc::ptr_eq(&same.0, &original.0), + "never rebase skipped original inputs" + ); + let expanded = original.after_partial_walk(capture(&[second])).unwrap(); + assert!(expanded.unchanged()); + assert!( + !expanded.can_replay(), + "reset expanded histories with a full walk" + ); + assert!(expanded.after_partial_walk(capture(&[first])).is_none()); + context + .eval_str(&format!("(put-text-property 0 1 'cursor 1 {target})")) + .unwrap(); + assert!( + !expanded.unchanged(), + "mutation of either union arm must invalidate it: {target}" + ); + } +} diff --git a/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs b/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs index 3d07518cda..ed9e809322 100644 --- a/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs +++ b/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs @@ -490,73 +490,103 @@ fn replacement_logical_cursor_tracks_the_selected_visual_slot() { .into_iter() .flat_map(|replacement| [true, false].map(|visible| (replacement, visible))) { - let (mut eval, frame, _, window) = incr_editing_frame("ax\n", 800, 240); - realize_test_gui_frame(&mut eval, frame); - eval.buffer_manager_mut() - .current_buffer_mut() - .unwrap() - .set_buffer_local("cursor-type", if visible { Value::T } else { Value::NIL }); - eval.eval_str(&format!("(progn (put-text-property 2 3 'display \"{replacement}\") (set-window-point (selected-window) 2))")).unwrap(); - let mut engine = crate::font::metrics::test_catalog::layout_engine(); - crate::font::metrics::test_catalog::enable_metrics(&mut engine); - let FrameLayoutAttempt::Prepared(prepared) = - engine.redisplay_frame_attempt(&mut eval, frame) - else { - panic!("replacement frame") - }; - let row = &prepared - .window_matrices - .iter() - .find(|entry| entry.window_id.get() == window.0 as i64) - .unwrap() - .matrix - .rows[0]; - let glyphs = &row.glyphs[GlyphArea::Text.index()]; - let selected = glyphs - .iter() - .enumerate() - .filter(|(_, glyph)| !glyph.padding && row.glyph_covers_buffer_charpos(glyph, 1)) - .filter_map(|(i, glyph)| match glyph.provenance { - neomacs_display_protocol::glyph_matrix::GlyphProvenance::Str { index, .. } => { - Some((index, i)) - } - _ => None, - }) - .min() - .expect("replacement glyph") - .1; - let expected_col: u16 = row.glyphs[GlyphArea::LeftMargin.index()] - .iter() - .chain(glyphs[..selected].iter()) - .filter(|glyph| !glyph.padding) - .map(|glyph| glyph.materialized_slot_span()) - .sum(); - let expected_x = row.pixel_x - + glyphs[..selected] + for line_numbers in [false, true] { + let (mut eval, frame, _, window) = incr_editing_frame("ax\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local( + "display-line-numbers", + if line_numbers { Value::T } else { Value::NIL }, + ); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", if visible { Value::T } else { Value::NIL }); + eval.eval_str(&format!("(progn (put-text-property 2 3 'display \"{replacement}\") (set-window-point (selected-window) 2))")).unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + let FrameLayoutAttempt::Prepared(prepared) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("replacement frame") + }; + let row = &prepared + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows[0]; + let glyphs = &row.glyphs[GlyphArea::Text.index()]; + let selected = glyphs .iter() + .enumerate() + .filter(|(_, glyph)| !glyph.padding && row.glyph_covers_buffer_charpos(glyph, 1)) + .filter_map(|(i, glyph)| match glyph.provenance { + neomacs_display_protocol::glyph_matrix::GlyphProvenance::Str { + index, .. + } => Some((index, i)), + _ => None, + }) + .min() + .expect("replacement glyph") + .1; + let expected_col: u16 = row.glyphs[GlyphArea::LeftMargin.index()] + .iter() + .chain(glyphs[..selected].iter()) .filter(|glyph| !glyph.padding) - .map(|glyph| glyph.pixel_width) - .sum::(); - let output = prepared.materialize(); - if visible { - assert_eq!(output.active_cursor().unwrap().slot_id.col, expected_col); - } else { - assert!(output.active_cursor().is_none()); - } - let snapshot = eval - .frame_manager() - .get(frame) - .unwrap() - .redisplay_snapshot(window) - .unwrap(); - let logical = snapshot.logical_cursor.as_ref().unwrap(); - assert_eq!( - logical.x, - expected_x.round() as i64, - "{replacement}, visible={visible}" - ); - if let Some(physical) = snapshot.phys_cursor.as_ref() { - assert_eq!(logical.x, physical.x, "{replacement}"); + .map(|glyph| glyph.materialized_slot_span()) + .sum(); + let expected_x = row.pixel_x + + glyphs[..selected] + .iter() + .filter(|glyph| !glyph.padding) + .map(|glyph| glyph.pixel_width) + .sum::(); + let output = prepared.materialize(); + if visible { + assert_eq!(output.active_cursor().unwrap().slot_id.col, expected_col); + } else { + assert!(output.active_cursor().is_none()); + } + let snapshot = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + let logical = snapshot.logical_cursor.as_ref().unwrap(); + // This fixture's line-number marks occupy display slots, not + // buffer-text output columns. Keep this oracle independent of the resolver. + let expected_output_col: u16 = glyphs[..selected] + .iter() + .filter(|glyph| { + !glyph.padding + && !matches!( + glyph.provenance, + neomacs_display_protocol::glyph_matrix::GlyphProvenance::Mark + ) + }) + .map(|glyph| glyph.materialized_slot_span()) + .sum(); + if line_numbers { + assert!(expected_col > expected_output_col); + } + assert_eq!( + logical.col, + i64::from(expected_output_col), + "{replacement}, visible={visible}, line_numbers={line_numbers}" + ); + assert_eq!( + logical.x, + expected_x.round() as i64, + "{replacement}, visible={visible}" + ); + if let Some(physical) = snapshot.phys_cursor.as_ref() { + assert_eq!(logical.x, physical.x, "{replacement}"); + } } } } @@ -627,16 +657,17 @@ fn hidden_rtl_cursor_anchor_uses_selected_glyph_dimensions() { assert_eq!(anchor.bounds_in_frame().width().get(), expected_width); } -/// Explicit string cursors must carry their own measured height, not the -/// window's default row height used while recording the overlay candidate. +/// Explicit string cursors retain one selected primitive's dimensions, not +/// row defaults or the aggregate advance of a multi-glyph source character. #[test] -fn hidden_explicit_string_cursor_anchor_uses_selected_glyph_height() { +fn hidden_explicit_string_cursor_anchor_uses_selected_glyph_dimensions() { use neovm_core::window::geometry::{PresentationId, VisualAnchor, VisualAnchorQuery}; - for (source, selected_index) in [ - ("(propertize \"X\" 'cursor 1 'face '(:height 2.0))", 0), + for (source, selected_char) in [ + ("(propertize \"X\" 'cursor 1 'face '(:height 2.0))", 'X'), + ("(propertize (string 1) 'cursor 1)", '^'), ( "(concat \"א\" (propertize \"X\" 'cursor 1 'face '(:height 2.0)) \"ב\")", - 1, + 'X', ), ] { let (mut eval, frame, _, window) = incr_editing_frame("a\n", 800, 240); @@ -646,7 +677,7 @@ fn hidden_explicit_string_cursor_anchor_uses_selected_glyph_height() { .unwrap() .set_buffer_local("cursor-type", Value::NIL); eval.eval_str(&format!( - "(overlay-put (make-overlay 1 1) 'before-string {source})" + "(progn (setq ctl-arrow t) (overlay-put (make-overlay 1 1) 'before-string {source}))" )) .unwrap(); let mut engine = crate::font::metrics::test_catalog::layout_engine(); @@ -664,15 +695,11 @@ fn hidden_explicit_string_cursor_anchor_uses_selected_glyph_height() { .matrix .rows[0]; let selected = row.glyphs[GlyphArea::Text.index()] - .iter() - .find(|glyph| { - matches!( - glyph.provenance, - neomacs_display_protocol::glyph_matrix::GlyphProvenance::Str { index, .. } if index == selected_index - ) - }) - .unwrap(); + .iter() + .find(|glyph| matches!(&glyph.glyph_type, neomacs_display_protocol::glyph_matrix::GlyphType::Char { ch } if *ch == selected_char)) + .expect("selected primitive"); let expected = selected.pixel_height.round(); + let expected_width = selected.pixel_width.round(); assert!(prepared.materialize().active_cursor().is_none()); let snapshot = eval .frame_manager() @@ -694,19 +721,453 @@ fn hidden_explicit_string_cursor_anchor_uses_selected_glyph_height() { )) .unwrap(); assert_eq!(anchor.bounds_in_frame().height().get(), expected); + assert_eq!( + anchor.bounds_in_frame().width().get(), + expected_width, + "{source}" + ); + } +} + +/// An unchanged hidden explicit cursor still owns its position and extent. +/// Visible cursors remain a positive control for ordinary retained replay. +#[test] +fn hidden_explicit_cursor_survives_unchanged_replay() { + use neovm_core::window::geometry::{PresentationId, VisualAnchor, VisualAnchorQuery}; + for visible in [true, false] { + let (mut eval, frame, _, window) = incr_editing_frame("a\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", if visible { Value::T } else { Value::NIL }); + eval.eval_str("(overlay-put (make-overlay 1 1) 'before-string (propertize \"X\" 'cursor 1 'face '(:height 2.0)))").unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + engine.layout_frame_rust(&mut eval, frame); + let before = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + assert!(before.logical_cursor.is_some()); + assert!(before.logical_cursor_dimensions.is_some()); + let FrameLayoutAttempt::Prepared(replayed) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("unchanged replay") + }; + let after = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + assert_eq!( + engine.last_layout_stats().cursor_only_windows, + 1, + "visible={visible}" + ); + if visible { + assert!(after.phys_cursor.is_some()); + } else { + assert!(replayed.materialize().active_cursor().is_none()); + let position = |snapshot: &neovm_core::window::WindowDisplaySnapshot| { + snapshot + .logical_cursor + .as_ref() + .map(|cursor| (cursor.x, cursor.y, cursor.row, cursor.col)) + }; + assert_eq!(position(&after), position(&before)); + assert_eq!( + after.logical_cursor_dimensions, + before.logical_cursor_dimensions + ); + let expected = before.logical_cursor_dimensions.unwrap(); + let owner = eval.frame_manager_mut().get_mut(frame).unwrap(); + let presentation = PresentationId::new(3); + owner + .prepare_live_window_presentation(presentation, vec![after]) + .unwrap(); + owner.activate_display_presentation(presentation).unwrap(); + let anchor = owner + .resolve_active_visual_anchor(VisualAnchorQuery::new( + presentation, + VisualAnchor::CursorBottom { window }, + )) + .unwrap(); + assert_eq!( + anchor.bounds_in_frame().width().get(), + expected.width as f32 + ); + assert_eq!( + anchor.bounds_in_frame().height().get(), + expected.height as f32 + ); + } + } +} + +/// Mutating a retained string's cursor intervals is independent of buffer and +/// overlay ticks; unchanged point must still reselect the authoritative slot. +#[test] +fn hidden_explicit_cursor_reselects_after_string_property_mutation() { + for (setup, mutation) in [ + ( + "(progn (setq neo-replay-string (propertize \"XY\" 'face '(:height 2.0))) (put-text-property 0 1 'cursor 1 neo-replay-string) (overlay-put (make-overlay 1 1) 'before-string neo-replay-string))", + "(progn (put-text-property 0 1 'cursor nil neo-replay-string) (put-text-property 1 2 'cursor 1 neo-replay-string))", + ), + ( + "(progn (setq neo-replay-string (copy-sequence \"XY\")) (overlay-put (make-overlay 1 1) 'before-string neo-replay-string))", + "(put-text-property 0 1 'cursor 1 neo-replay-string)", + ), + ] { + let (mut eval, frame, _, window) = incr_editing_frame("a\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", Value::NIL); + eval.eval_str(setup).unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + engine.layout_frame_rust(&mut eval, frame); + eval.eval_str("(put-text-property 0 1 'face 'bold (copy-sequence \"unrelated\"))") + .unwrap(); + engine.layout_frame_rust(&mut eval, frame); + assert_eq!( + engine.last_layout_stats().cursor_only_windows, + 1, + "unrelated string mutation must not invalidate observed sources" + ); + eval.eval_str(mutation).unwrap(); + engine.layout_frame_rust(&mut eval, frame); + let actual = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut fresh); + fresh.layout_frame_rust(&mut eval, frame); + let expected = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + assert!(actual.phys_cursor.is_none()); + let position = |snapshot: &neovm_core::window::WindowDisplaySnapshot| { + snapshot + .logical_cursor + .as_ref() + .map(|cursor| (cursor.x, cursor.y, cursor.row, cursor.col)) + }; + assert_eq!(position(&actual), position(expected)); + assert_eq!( + actual.logical_cursor_dimensions, + expected.logical_cursor_dimensions + ); + } +} + +/// A later leaf may mutate an earlier leaf's string without changing its +/// buffer or overlay ticks. Acceptance must revalidate the whole frame. +#[test] +fn hidden_cursor_sources_are_revalidated_after_later_window_callbacks() { + let (mut eval, frame, _, window) = incr_editing_frame("a\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", Value::NIL); + eval.eval_str("(progn (setq neo-frame-string (propertize \"XY\" 'cursor 1) neo-frame-mutated nil noninteractive nil) (overlay-put (make-overlay 1 1) 'before-string neo-frame-string))").unwrap(); + let side = eval.buffer_manager_mut().create_buffer("*neo-later-leaf*"); + eval.buffer_manager_mut() + .get_mut(side) + .unwrap() + .insert("b\n"); + eval.frame_manager_mut() + .split_window( + frame, + window, + neovm_core::window::SplitDirection::Horizontal, + side, + None, + neovm_core::window::SplitPlacement::AfterTarget, + ) + .unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + engine.layout_frame_rust(&mut eval, frame); + let hook = eval.eval_str("(list (lambda (_pos) (if neo-frame-mutated nil (progn (setq neo-frame-mutated t) (put-text-property 0 1 'cursor nil neo-frame-string) (put-text-property 1 2 'cursor 1 neo-frame-string)))))").unwrap(); + eval.buffer_manager_mut() + .get_mut(side) + .unwrap() + .set_buffer_local("fontification-functions", hook); + engine.layout_frame_rust(&mut eval, frame); + assert!( + !eval.eval_str("neo-frame-mutated").unwrap().is_nil(), + "later leaf hook must actually run" + ); + let actual = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut fresh); + fresh.layout_frame_rust(&mut eval, frame); + let expected = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + let position = |s: &neovm_core::window::WindowDisplaySnapshot| { + s.logical_cursor.as_ref().map(|c| (c.x, c.y, c.row, c.col)) + }; + assert!(actual.phys_cursor.is_none()); + assert_eq!(position(&actual), position(expected)); + assert_eq!( + actual.logical_cursor_dimensions, + expected.logical_cursor_dimensions + ); +} + +/// Global default properties can supply an explicit string cursor even when +/// the buffer has no intervals or overlays of its own. +#[test] +fn hidden_default_property_cursor_is_not_canonical() { + let (mut eval, frame, _, window) = incr_editing_frame("ab\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", Value::NIL); + eval.eval_str("(setq default-text-properties (list 'display (let ((s (copy-sequence \"XY\"))) (put-text-property 1 2 'cursor 2 s) s)))").unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + engine.layout_frame_rust(&mut eval, frame); + eval.eval_str("(set-window-point (selected-window) 2)") + .unwrap(); + engine.layout_frame_rust(&mut eval, frame); + let actual = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut fresh); + fresh.layout_frame_rust(&mut eval, frame); + let expected = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + let position = |snapshot: &neovm_core::window::WindowDisplaySnapshot| { + snapshot + .logical_cursor + .as_ref() + .map(|cursor| (cursor.x, cursor.y, cursor.row, cursor.col)) + }; + assert_eq!(position(&actual), position(expected)); + assert_eq!( + actual.logical_cursor_dimensions, + expected.logical_cursor_dimensions + ); + assert!(actual.phys_cursor.is_none()); +} + +/// Ordinary hidden selection uses finalized geometry without requiring paint +/// metrics; an empty last line must retain its real row origin too. +#[test] +fn hidden_canonical_cursor_replay_matches_fresh_positions() { + for point in [2, 5] { + let (mut eval, frame, _, window) = incr_editing_frame("abc\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", Value::NIL); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + engine.layout_frame_rust(&mut eval, frame); + eval.eval_str(&format!("(set-window-point (selected-window) {point})")) + .unwrap(); + engine.layout_frame_rust(&mut eval, frame); + assert_eq!(engine.last_layout_stats().cursor_only_windows, 1); + let actual = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + assert!(actual.phys_cursor.is_none()); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + fresh.layout_frame_rust(&mut eval, frame); + let expected = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + let position = |snapshot: &neovm_core::window::WindowDisplaySnapshot| { + snapshot + .logical_cursor + .as_ref() + .map(|cursor| (cursor.x, cursor.y, cursor.row, cursor.col)) + }; + assert_eq!(position(&actual), position(expected), "point={point}"); + assert_eq!( + actual.logical_cursor_dimensions, expected.logical_cursor_dimensions, + "point={point}" + ); } } +/// A moved point or translated retained row cannot certify an old explicit +/// selection. Hidden overrides must be reselected, and retry must discard rows. +#[test] +fn hidden_explicit_cursor_reselects_after_point_and_start_changes() { + let (mut eval, frame, _, window) = + incr_editing_frame("ab\ncd\nef\ngh\nij\nkl\nmn\nop\n", 800, 80); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", Value::NIL); + eval.eval_str("(progn (overlay-put (make-overlay 1 1) 'before-string (propertize \"X\" 'cursor 2 'face '(:height 2.0))) (overlay-put (make-overlay 4 4) 'before-string (propertize \"X\" 'cursor 2 'face '(:height 1.5))))").unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + engine.layout_frame_rust(&mut eval, frame); + let mut last = None; + for (change, height) in [ + ("(set-window-point (selected-window) 2)", 24), + ("(set-window-point (selected-window) 5)", 18), + ("(set-window-start (selected-window) 4 t)", 18), + ] { + eval.eval_str(change).unwrap(); + let FrameLayoutAttempt::Prepared(prepared) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("changed hidden selection") + }; + assert!(prepared.materialize().active_cursor().is_none()); + assert_eq!(engine.last_layout_stats().full_windows, 1, "{change}"); + let actual = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + assert_eq!(actual.logical_cursor.as_ref().unwrap().x, 0, "{change}"); + assert_eq!( + actual.logical_cursor_dimensions.unwrap().height, + height, + "{change}" + ); + let rows = prepared + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows + .iter() + .map(|row| { + ( + row.pixel_y, + row.height_px, + row.glyphs[GlyphArea::Text.index()] + .iter() + .map(|glyph| { + ( + glyph.glyph_type.clone(), + glyph.pixel_width, + glyph.pixel_height, + ) + }) + .collect::>(), + ) + }) + .collect::>(); + last = Some((actual, rows)); + } + let (actual, rows) = last.unwrap(); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut fresh); + let FrameLayoutAttempt::Prepared(expected_frame) = + fresh.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("fresh hidden selection") + }; + let expected = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + let position = |snapshot: &neovm_core::window::WindowDisplaySnapshot| { + snapshot + .logical_cursor + .as_ref() + .map(|cursor| (cursor.x, cursor.y, cursor.row, cursor.col)) + }; + assert_eq!(position(&actual), position(expected)); + assert_eq!( + actual.logical_cursor_dimensions, + expected.logical_cursor_dimensions + ); + let expected_rows = expected_frame + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows + .iter() + .map(|row| { + ( + row.pixel_y, + row.height_px, + row.glyphs[GlyphArea::Text.index()] + .iter() + .map(|glyph| { + ( + glyph.glyph_type.clone(), + glyph.pixel_width, + glyph.pixel_height, + ) + }) + .collect::>(), + ) + }) + .collect::>(); + assert_eq!(rows, expected_rows); +} + /// Moved-point reconstruction must share fresh stretch-width policy, including /// preserving narrow stretches and respecting x-stretch-cursor. #[test] fn stretch_cursor_only_replay_matches_fresh_width_policy() { for stretch in [false, true] { - for (width, style) in [0.25, 5.0] + for (width, style) in [0.01, 0.25, 5.0] .into_iter() .flat_map(|width| ["box", "bar", "hbar"].map(|style| (width, style))) { - let (mut eval, frame, _, _) = incr_editing_frame("אxב\n", 800, 240); + let (mut eval, frame, _, window) = incr_editing_frame("אxב\n", 800, 240); realize_test_gui_frame(&mut eval, frame); let cursor_type = match style { "box" => Value::T, @@ -721,6 +1182,12 @@ fn stretch_cursor_only_replay_matches_fresh_width_policy() { let mut engine = crate::font::metrics::test_catalog::layout_engine(); crate::font::metrics::test_catalog::enable_metrics(&mut engine); engine.layout_frame_rust(&mut eval, frame); + let column_width = eval + .eval_str("(frame-char-width)") + .unwrap() + .as_fixnum() + .unwrap() as f32; + assert!(column_width >= 1.0); eval.eval_str("(set-window-point (selected-window) 2)") .unwrap(); let FrameLayoutAttempt::Prepared(replayed) = @@ -731,8 +1198,42 @@ fn stretch_cursor_only_replay_matches_fresh_width_policy() { assert_eq!(engine.last_layout_stats().cursor_only_windows, 1); assert!(engine.last_layout_stats().reused_rows > 0); let actual = replayed.materialize().active_cursor().unwrap().width; - if style == "bar" { - assert_eq!(actual, 2.0, "stretch={stretch} width={width}"); + let row = &replayed + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows[0]; + let glyph_width = row.glyphs[GlyphArea::Text.index()] + .iter() + .find(|glyph| { + row.glyph_covers_buffer_charpos(glyph, 1) + && matches!( + glyph.glyph_type, + neomacs_display_protocol::glyph_matrix::GlyphType::Stretch { .. } + ) + }) + .unwrap() + .pixel_width; + let expected_width = if style == "bar" { + 2.0 + } else if stretch { + glyph_width.max(1.0) + } else { + glyph_width.max(1.0).min(column_width) + }; + assert_eq!( + actual, expected_width, + "stretch={stretch} width={width} style={style}" + ); + if width < 0.1 && style != "bar" { + assert_eq!(actual, 1.0); + } + if width > 1.0 { + assert!(glyph_width > column_width); + } else { + assert!(glyph_width < column_width); } let mut fresh = crate::font::metrics::test_catalog::layout_engine(); crate::font::metrics::test_catalog::enable_metrics(&mut fresh); @@ -742,9 +1243,9 @@ fn stretch_cursor_only_replay_matches_fresh_width_policy() { panic!("fresh stretch") }; assert_eq!( - actual, rebuilt.materialize().active_cursor().unwrap().width, - "stretch={stretch} width={width} style={style}" + expected_width, + "fresh stretch={stretch} width={width} style={style}" ); } } diff --git a/crates/neomacs-layout-engine/src/window_output.rs b/crates/neomacs-layout-engine/src/window_output.rs index 193ceb4195..78a1322d4d 100644 --- a/crates/neomacs-layout-engine/src/window_output.rs +++ b/crates/neomacs-layout-engine/src/window_output.rs @@ -2092,6 +2092,11 @@ impl WindowOutputEmitter { self.geometry.push_chrome_row_progress(progress) } + /// Whether this pass has established semantic selection, regardless of paint. + pub(crate) fn has_logical_cursor(&self) -> bool { + self.logical_cursor.is_some() + } + /// Publish a position and its selected extent atomically, independently of paint. pub(crate) fn set_logical_cursor( &mut self, diff --git a/crates/neovm-core/src/buffer/buffer.rs b/crates/neovm-core/src/buffer/buffer.rs index baef637144..58a12a61c0 100644 --- a/crates/neovm-core/src/buffer/buffer.rs +++ b/crates/neovm-core/src/buffer/buffer.rs @@ -2044,6 +2044,11 @@ impl BufferTextSnapshot { .indexed_newline_count(range.start(), range.end(), build) } + /// Constant-time absence proof over the complete, unprojected property table. + pub fn text_props_is_empty(&self) -> bool { + self.text.text_props_is_empty() + } + pub fn text_prop_at_emacs_byte_pos(&self, pos: EmacsBytePos, name: Value) -> Option { self.text .text_props_get_property_at_emacs_byte_pos(pos, name) diff --git a/crates/neovm-core/src/emacs_core/runtime/owned_roots/mod.rs b/crates/neovm-core/src/emacs_core/runtime/owned_roots/mod.rs index 7ffa54eadd..5df80ab8c3 100644 --- a/crates/neovm-core/src/emacs_core/runtime/owned_roots/mod.rs +++ b/crates/neovm-core/src/emacs_core/runtime/owned_roots/mod.rs @@ -22,19 +22,50 @@ impl std::fmt::Debug for OwnedRoots { } } +type RootLeases = RefCell>>; + +#[repr(C)] #[derive(Default)] pub(crate) struct OwnedRootRegistry { - leases: RefCell>>, + leases: Rc, + // Context's pinned repr(C) ABI reserves the former inline RefCell + // slot. Sharing the registry must not move any subsequent Context field. + _context_abi_reserved: [usize; 3], +} +const _: () = + assert!(std::mem::size_of::() == std::mem::size_of::()); +const _: () = + assert!(std::mem::align_of::() == std::mem::align_of::()); + +/// Evaluator-local root registration without borrowing the entire Context. +/// This weak factory cannot prolong its evaluator or cross worker threads. +#[derive(Clone, Debug)] +pub struct OwnedRootRetainer { + leases: Weak, +} +impl OwnedRootRetainer { + /// Retain already-live operands, or decline after the original evaluator + /// has retired. This grants lifetime only, never mutation freshness. + pub fn retain(&self, values: impl IntoIterator) -> Option { + let leases = self.leases.upgrade()?; + Some(OwnedRootRegistry::retain_in(&leases, values)) + } } impl OwnedRootRegistry { fn retain(&self, values: impl IntoIterator) -> OwnedRoots { + Self::retain_in(&self.leases, values) + } + + /// Native owners hold strong values; the original evaluator traces only + /// live weak leases. Registration never transfers evaluator ownership. + fn retain_in(leases: &RootLeases, values: impl IntoIterator) -> OwnedRoots { let values: Rc<[Value]> = values .into_iter() .filter(|value| value.is_heap_object()) .collect(); if !values.is_empty() { - let mut leases = self.leases.borrow_mut(); + let mut leases = leases.borrow_mut(); leases.retain(|lease| lease.strong_count() != 0); leases.push(Rc::downgrade(&values)); } @@ -55,6 +86,14 @@ impl OwnedRootRegistry { } impl Context { + /// Register roots while other display services borrow the Context. The + /// returned factory is weak and remains tied to this evaluator's registry. + pub fn owned_root_retainer(&self) -> OwnedRootRetainer { + OwnedRootRetainer { + leases: Rc::downgrade(&self.owned_roots.leases), + } + } + /// Retain already-live operands from this evaluator. This grants object /// lifetime only: callers must separately validate mutation and layout /// generations before using derived data. diff --git a/crates/neovm-core/src/emacs_core/runtime/owned_roots/tests/owned_roots_test.rs b/crates/neovm-core/src/emacs_core/runtime/owned_roots/tests/owned_roots_test.rs index 02890ef234..1ceaa6f929 100644 --- a/crates/neovm-core/src/emacs_core/runtime/owned_roots/tests/owned_roots_test.rs +++ b/crates/neovm-core/src/emacs_core/runtime/owned_roots/tests/owned_roots_test.rs @@ -26,6 +26,24 @@ fn cloned_lease_survives_exact_gc_and_retires_with_last_owner() { assert!(context.owned_roots.leases.borrow().is_empty()); } +#[test] +fn weak_retainer_registers_live_graphs_without_extending_context_lifetime() { + let mut context = Context::new(); + context.gc_collect_exact(); + let baseline = context.tagged_heap.allocated_count(); + let factory = context.owned_root_retainer(); + let value = Value::cons(Value::fixnum(7), Value::vector(vec![Value::fixnum(9)])); + let lease = factory.retain([value]).unwrap(); + context.gc_collect_exact(); + assert_eq!(context.tagged_heap.allocated_count(), baseline + 2); + assert_eq!(value.cons_car(), Value::fixnum(7)); + drop(lease); + context.gc_collect_exact(); + assert_eq!(context.tagged_heap.allocated_count(), baseline); + drop(context); + assert!(factory.retain([Value::NIL]).is_none()); +} + #[test] fn retired_leases_do_not_accumulate_between_collections() { let context = Context::new(); From 3659b7755d30731a048ce9ed038aacd6c8a81d8f Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sun, 8 Nov 2026 01:09:48 +0000 Subject: [PATCH 08/10] Amortize root lease registration and deduplicate source roots --- .../src/cursor_source_dependencies.rs | 25 ++++---- .../tests/cursor_source_dependencies_test.rs | 25 ++++++++ .../src/emacs_core/runtime/owned_roots/mod.rs | 31 +++++++++- .../owned_roots/tests/owned_roots_test.rs | 57 +++++++++++++++++++ 4 files changed, 126 insertions(+), 12 deletions(-) diff --git a/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs b/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs index 1c0c4f9c9e..5730105677 100644 --- a/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs +++ b/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs @@ -176,7 +176,7 @@ impl Drop for Capture { pub(crate) fn string_properties( value: Value, ) -> Option { - let retainer = ACTIVE.with(|active| { + let rooted = ACTIVE.with(|active| { let mut active = active.borrow_mut(); let observation = active.as_mut()?; if !observation @@ -187,16 +187,21 @@ pub(crate) fn string_properties( observation.complete = false; return None; } - Some(observation.retainer.clone()) - }); - let Some(roots) = retainer.and_then(|retainer| retainer.retain([value])) else { - ACTIVE.with(|active| { - if let Some(observation) = active.borrow_mut().as_mut() { + if !observation.strings.contains(&value) { + let Some(roots) = observation.retainer.retain([value]) else { observation.complete = false; - } - }); + return None; + }; + // Root each original object once, before its first projection. + // Repeated projections must still record mutation observations. + observation.roots.push(roots); + observation.strings.insert(value); + } + Some(()) + }); + if rooted.is_none() { return neovm_core::emacs_core::value::get_string_text_properties_table_for_value(value); - }; + } let (properties, reads) = collection_reads::capture(|| { // The cursor property is indexed in the original string. Observe its // contents/storage too, without copying/scanning bytes, so in-place @@ -210,8 +215,6 @@ pub(crate) fn string_properties( }); ACTIVE.with(|active| { if let Some(observation) = active.borrow_mut().as_mut() { - observation.roots.push(roots); - observation.strings.insert(value); if let Some(reads) = reads { observation.reads.push(reads); } else { diff --git a/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs b/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs index 78977b80fd..9de1d35af2 100644 --- a/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs +++ b/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs @@ -1,5 +1,30 @@ use super::*; +/// Repeated projection of one original string needs one immediate root lease, +/// while all original content/interval observations remain freshness evidence. +#[test] +fn repeated_string_projections_share_one_root_lease() { + let context = Context::new(); + let value = Value::string("repeated source"); + let capture = Capture::begin(&context); + for _ in 0..100 { + let _ = string_properties(value); + } + let sources = capture.finish().unwrap(); + let DependencyAuthority::Canonical { + _roots, strings, .. + } = sources.0.as_ref() + else { + panic!("canonical producer capture"); + }; + assert_eq!(strings.len(), 1); + assert_eq!( + _roots.len(), + 1, + "do not register a lease per repeated projection" + ); +} + /// Partial walks preserve original read origins without accumulating a chain /// of removed source strings; new strings after an empty domain are complete. #[test] diff --git a/crates/neovm-core/src/emacs_core/runtime/owned_roots/mod.rs b/crates/neovm-core/src/emacs_core/runtime/owned_roots/mod.rs index 5df80ab8c3..3b847fa01b 100644 --- a/crates/neovm-core/src/emacs_core/runtime/owned_roots/mod.rs +++ b/crates/neovm-core/src/emacs_core/runtime/owned_roots/mod.rs @@ -24,6 +24,9 @@ impl std::fmt::Debug for OwnedRoots { type RootLeases = RefCell>>; +#[cfg(test)] +thread_local! { static LEASE_PRUNING_INSPECTIONS: std::cell::Cell = const { std::cell::Cell::new(0) }; } + #[repr(C)] #[derive(Default)] pub(crate) struct OwnedRootRegistry { @@ -66,7 +69,33 @@ impl OwnedRootRegistry { .collect(); if !values.is_empty() { let mut leases = leases.borrow_mut(); - leases.retain(|lease| lease.strong_count() != 0); + // Reclaim an expired suffix cheaply (including the usual single + // transient lease), preserving prompt cleanup without rescanning + // live predecessors. Each expired entry is popped at most once. + while leases.last().is_some_and(|lease| { + #[cfg(test)] + LEASE_PRUNING_INSPECTIONS.with(|count| count.set(count.get() + 1)); + lease.strong_count() == 0 + }) { + drop(leases.pop()); + } + // Sweep at capacity, not on every registration. If most leases + // remain live, grow as well: freeing only one dead slot must not + // cause another full scan on the very next insertion. Either + // branch leaves at least half a buffer of registration headroom, + // so total pruning work is amortized linear. GC tracing also + // removes dead leases; weak entries never keep Lisp graphs live. + if leases.len() == leases.capacity() { + leases.retain(|lease| { + #[cfg(test)] + LEASE_PRUNING_INSPECTIONS.with(|count| count.set(count.get() + 1)); + lease.strong_count() != 0 + }); + if leases.len() > leases.capacity() / 2 { + let additional = leases.capacity(); + leases.reserve(additional); + } + } leases.push(Rc::downgrade(&values)); } OwnedRoots { values } diff --git a/crates/neovm-core/src/emacs_core/runtime/owned_roots/tests/owned_roots_test.rs b/crates/neovm-core/src/emacs_core/runtime/owned_roots/tests/owned_roots_test.rs index 1ceaa6f929..40f34bb026 100644 --- a/crates/neovm-core/src/emacs_core/runtime/owned_roots/tests/owned_roots_test.rs +++ b/crates/neovm-core/src/emacs_core/runtime/owned_roots/tests/owned_roots_test.rs @@ -1,5 +1,62 @@ use super::*; +/// Count actual weak-lease inspections, not timing: registrations must remain +/// linear with both a growing live set and long-lived plus transient owners. +#[test] +fn root_registration_pruning_work_is_amortized_linear() { + let mut context = Context::new(); + context.gc_collect_exact(); + let baseline = context.tagged_heap.allocated_count(); + let value = Value::string("registration workload"); + let retainer = context.owned_root_retainer(); + let count = 1024; + LEASE_PRUNING_INSPECTIONS.with(|work| work.set(0)); + let mut live: std::collections::VecDeque<_> = (0..count) + .map(|_| retainer.retain([value]).unwrap()) + .collect(); + let growing = LEASE_PRUNING_INSPECTIONS.with(|work| work.get()); + assert!( + growing <= 8 * count, + "growing live leases: {growing} inspections for {count} registrations" + ); + LEASE_PRUNING_INSPECTIONS.with(|work| work.set(0)); + for _ in 0..4 * count { + drop(retainer.retain([value]).unwrap()); + } + let transient = LEASE_PRUNING_INSPECTIONS.with(|work| work.get()); + assert!( + transient <= 8 * 4 * count, + "live plus transient leases: {transient} inspections" + ); + // Fill the registry, then expire its oldest (non-tail) lease each time. + // A capacity sweep freeing only one slot must create more headroom rather + // than immediately rescanning the whole live set on the next insertion. + let capacity = context.owned_roots.leases.borrow().capacity(); + while live.len() < capacity { + live.push_back(retainer.retain([value]).unwrap()); + } + LEASE_PRUNING_INSPECTIONS.with(|work| work.set(0)); + for _ in 0..4 * capacity { + drop(live.pop_front()); + live.push_back(retainer.retain([value]).unwrap()); + } + let rotating = LEASE_PRUNING_INSPECTIONS.with(|work| work.get()); + assert!( + rotating <= 8 * 4 * capacity, + "rotating interior leases: {rotating} inspections" + ); + eprintln!( + "root-registration inspections: growing={growing}, transient={transient}, rotating={rotating}" + ); + drop(live); + context.gc_collect_exact(); + assert_eq!( + context.tagged_heap.allocated_count(), + baseline, + "dead weak entries do not keep Lisp graphs alive" + ); +} + #[test] fn cloned_lease_survives_exact_gc_and_retires_with_last_owner() { let mut context = Context::new(); From 706b6517cbcd66befd533d8ee9447f19e7a14d4f Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sun, 8 Nov 2026 01:28:59 +0000 Subject: [PATCH 09/10] Key cursor source roots and domains by allocation identity --- .../src/cursor_source_dependencies.rs | 15 +++- .../tests/cursor_source_dependencies_test.rs | 76 +++++++++++++++++++ 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs b/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs index 5730105677..05c07e021c 100644 --- a/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs +++ b/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs @@ -13,11 +13,17 @@ use neovm_core::{ }; use std::{cell::RefCell, rc::Rc}; +/// Original allocation identity, not Lisp `equal`: equal-text strings may +/// have different intervals and lifetimes. The matching roots keep these +/// tagged addresses alive for the entire certificate/domain lifetime. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +struct SourceStringIdentity(usize); + #[derive(Debug)] struct Observation { reads: Vec, roots: Vec, - strings: rustc_hash::FxHashSet, + strings: rustc_hash::FxHashSet, retainer: OwnedRootRetainer, owner: Option, complete: bool, @@ -34,7 +40,7 @@ enum DependencyAuthority { owner: CollectionReads, reads: Vec, _roots: Vec, - strings: rustc_hash::FxHashSet, + strings: rustc_hash::FxHashSet, }, // Validate a partial walk with newly introduced sources for this frame, // then require a canonical walk. Never grow an unbounded history chain. @@ -187,7 +193,8 @@ pub(crate) fn string_properties( observation.complete = false; return None; } - if !observation.strings.contains(&value) { + let identity = SourceStringIdentity(value.bits()); + if !observation.strings.contains(&identity) { let Some(roots) = observation.retainer.retain([value]) else { observation.complete = false; return None; @@ -195,7 +202,7 @@ pub(crate) fn string_properties( // Root each original object once, before its first projection. // Repeated projections must still record mutation observations. observation.roots.push(roots); - observation.strings.insert(value); + observation.strings.insert(identity); } Some(()) }); diff --git a/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs b/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs index 9de1d35af2..7c68385c0f 100644 --- a/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs +++ b/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs @@ -1,5 +1,81 @@ use super::*; +/// Equal text is not object identity: each original string (including distinct +/// interval state) must survive exact GC for its retained read certificate. +#[test] +fn equal_content_sources_keep_distinct_roots() { + use neovm_core::emacs_core::value::{mutate_string_text_properties, positions::*}; + let mut context = Context::new(); + context.gc_collect_exact(); + let allocated = || neovm_core::tagged::gc::with_tagged_heap(|heap| heap.allocated_count()); + let baseline = allocated(); + let first = Value::string("same"); + let second = Value::string("same"); + mutate_string_text_properties(second, |props| { + props.put_property_for_object_char_len( + CharRange::new(CharPos0::new(0), CharPos0::new(1)), + CharLen::new(4), + Value::symbol("cursor"), + Value::fixnum(1), + ); + }); + assert_ne!(first.bits(), second.bits()); + assert_eq!( + first, second, + "Value equality intentionally ignores string properties" + ); + // Establish the whole live graph, including the interval metadata, without + // assuming how many managed objects the property representation needs. + let setup_roots = context.retain_gc_roots([first, second]); + context.gc_collect_exact(); + let expected = allocated(); + let capture = Capture::begin(&context); + let _ = string_properties(first); + let _ = string_properties(second); + let sources = capture.finish().unwrap(); + drop(setup_roots); + context.gc_collect_exact(); + assert_eq!( + allocated(), + expected, + "both original strings must remain rooted, not only one equal-content object" + ); + let DependencyAuthority::Canonical { + _roots, strings, .. + } = sources.0.as_ref() + else { + panic!("canonical producer capture"); + }; + assert_eq!(_roots.len(), 2); + assert_eq!(strings.len(), 2); + drop(sources); + context.gc_collect_exact(); + assert_eq!(allocated(), baseline); +} + +/// A different original object expands a partial source domain even when its +/// text compares equal; the skipped rows' original authority cannot cover it. +#[test] +fn equal_content_new_source_expands_partial_domain() { + let context = Context::new(); + let first = Value::string("same"); + let second = Value::string("same"); + assert_ne!(first.bits(), second.bits()); + assert_eq!(first, second); + let capture = |value| { + let scope = Capture::begin(&context); + let _ = string_properties(value); + scope.finish().unwrap() + }; + let original = capture(first); + let expanded = original.after_partial_walk(capture(second)).unwrap(); + assert!( + !expanded.can_replay(), + "equal text does not certify a different original object" + ); + assert!(expanded.unchanged()); +} + /// Repeated projection of one original string needs one immediate root lease, /// while all original content/interval observations remain freshness evidence. #[test] From 42b098c0e42aaffc0b35cea9b7ad5dd9bfe604fb Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sun, 8 Nov 2026 02:19:17 +0000 Subject: [PATCH 10/10] Validate original cursor sources through replay and retirement --- .../src/buffer_source/render_plan.rs | 26 +- .../src/cursor_source_dependencies.rs | 192 +++++++-- .../src/display_row/builder.rs | 19 + .../display_row/builder/tests/builder_test.rs | 105 +++++ .../src/display_source.rs | 13 +- crates/neomacs-layout-engine/src/engine.rs | 128 ++++-- .../src/engine/prepared_viewports.rs | 76 +++- .../engine/prepared_viewports/measurement.rs | 3 +- .../tests/certificate_test.rs | 309 ++++++++++++++ .../src/engine/scroll_coverage.rs | 14 + .../src/engine/tests/offscreen_row_test.rs | 10 +- .../src/incremental_layout.rs | 4 + .../tests/cursor_source_dependencies_test.rs | 95 +++++ .../src/tests/engine_line_spacing_test.rs | 397 ++++++++++++++++++ 14 files changed, 1299 insertions(+), 92 deletions(-) create mode 100644 crates/neomacs-layout-engine/src/engine/prepared_viewports/tests/certificate_test.rs diff --git a/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs b/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs index 9b6aa1d65d..a6a6113b66 100644 --- a/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs +++ b/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs @@ -888,12 +888,17 @@ impl BufferSourceOutputSetup { // even a string emitting only synthetic/control primitives may override point. // This is O(1), excludes category/alias properties too, and caches no // mutable string-property claim under buffer ticks. - let canonical_buffer_cursor = buffer.layout_text_props_is_empty() - && !has_overlays - && !local_display_policy.has_prefix() - && !buffer - .layout_buffer_local_value(crate::neovm_bridge::LayoutVar::DefaultTextProperties) - .is_some_and(|value| value.is_cons()); + let canonical_buffer_cursor = cursor_only + .as_ref() + .is_some_and(|replay| replay.ordinary_cursor_source.is_some()) + || (buffer.layout_text_props_is_empty() + && !has_overlays + && !local_display_policy.has_prefix() + && !buffer + .layout_buffer_local_value( + crate::neovm_bridge::LayoutVar::DefaultTextProperties, + ) + .is_some_and(|value| value.is_cons())); let face_resolution = BufferSourceFaceResolutionContext::new( buffer, face_resolver.resolver(), @@ -980,12 +985,9 @@ impl BufferSourceOutputSetup { // rebuild (honest layering, spec §4.6) — the win is skipping fontify / // shaping / measurement for the body. if let Some(mut replay) = cursor_only { - // Row coverage alone cannot establish a hidden explicit selection. - // Unknown or moved semantic authority requires a fresh source walk. - if replay.retained_logical_cursor.is_none() - && !canonical_buffer_cursor - && cursor_style_for_window(params).is_none() - { + // Row coverage cannot establish an explicit selection, regardless + // of paint visibility. Moved authority needs a fresh source walk. + if replay.retained_logical_cursor.is_none() && !canonical_buffer_cursor { output.restore_retry_checkpoint(retry_checkpoint); return BufferSourceRenderAttemptOutcome::ReplayMispredicted; } diff --git a/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs b/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs index 05c07e021c..f2d292ade1 100644 --- a/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs +++ b/crates/neomacs-layout-engine/src/cursor_source_dependencies.rs @@ -1,4 +1,4 @@ -//! Owner-only observations of every string projected by a canonical window walk. +//! Owner-only observations of original source objects and decoded payloads. //! Numeric snapshots and worker rows deliberately carry no such certificate. #[cfg(test)] @@ -13,20 +13,30 @@ use neovm_core::{ }; use std::{cell::RefCell, rc::Rc}; -/// Original allocation identity, not Lisp `equal`: equal-text strings may -/// have different intervals and lifetimes. The matching roots keep these -/// tagged addresses alive for the entire certificate/domain lifetime. +/// Original allocation identity, not Lisp `equal`: equal objects may have +/// different contents, intervals, or decoded payloads. Matching roots protect +/// their graphs while the original observed ancestor edges remain unchanged. #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] -struct SourceStringIdentity(usize); +struct SourceObjectIdentity(usize); + +// Replaces the former completeness bool without growing the TLS observation. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum CaptureState { + Ordinary, + SourceSelectionRequired, + Incomplete, +} +const _: () = assert!(std::mem::size_of::() == std::mem::size_of::()); #[derive(Debug)] struct Observation { reads: Vec, roots: Vec, - strings: rustc_hash::FxHashSet, + // Source strings AND directly interpreted payload operands. + strings: rustc_hash::FxHashSet, retainer: OwnedRootRetainer, owner: Option, - complete: bool, + state: CaptureState, } thread_local! { static ACTIVE: RefCell> = const { RefCell::new(None) }; } @@ -34,14 +44,26 @@ thread_local! { static ACTIVE: RefCell> = const { RefCell::n /// Rc clones preserve the original read certificate across unchanged replay. #[derive(Clone, Debug)] pub(crate) struct CursorSourceDependencies(Rc); + +/// Original producer evidence that replay needs no Lisp-string cursor override. +#[derive(Clone, Copy, Debug)] +pub(crate) struct OrdinaryCursorSourceProof(()); #[derive(Debug)] enum DependencyAuthority { Canonical { owner: CollectionReads, reads: Vec, _roots: Vec, - strings: rustc_hash::FxHashSet, + strings: rustc_hash::FxHashSet, + ordinary: Option, }, + // Original prepared-row producer evidence and its original source leases. + // The domain is opaque, so partial walks cannot claim subset inclusion. + PreparedRows { + reads: CollectionReads, + _roots: Vec, + }, + PreparedUnion(Vec), // Validate a partial walk with newly introduced sources for this frame, // then require a canonical walk. Never grow an unbounded history chain. FrameOnly { @@ -58,6 +80,8 @@ impl CursorSourceDependencies { owner.unchanged_and_observe() && reads.iter().all(CollectionReads::unchanged_and_observe) } + DependencyAuthority::PreparedRows { reads, .. } => reads.unchanged_and_observe(), + DependencyAuthority::PreparedUnion(sources) => sources.iter().all(Self::unchanged), DependencyAuthority::FrameOnly { previous, fresh } => { previous.unchanged() && fresh.unchanged() } @@ -66,12 +90,78 @@ impl CursorSourceDependencies { /// Expanded partial-source domains validate this frame only; the next walk /// resets them instead of indefinitely retaining removed strings/leases. pub(crate) fn can_replay(&self) -> bool { - matches!(self.0.as_ref(), DependencyAuthority::Canonical { .. }) + !matches!(self.0.as_ref(), DependencyAuthority::FrameOnly { .. }) + } + /// Complete canonical walks with observed absence of explicit cursors, or successful owned + /// row programs (which reject all non-nil string `cursor` properties), allow + /// moved-point reconstruction. This proof applies only to unchanged rows. + pub(crate) fn ordinary_cursor_source_proof(&self) -> Option { + let ordinary = match self.0.as_ref() { + DependencyAuthority::Canonical { ordinary, .. } => ordinary.is_some(), + DependencyAuthority::PreparedRows { .. } => true, + DependencyAuthority::PreparedUnion(parts) => parts + .iter() + .all(|part| part.ordinary_cursor_source_proof().is_some()), + DependencyAuthority::FrameOnly { .. } => false, + }; + (ordinary && self.unchanged()).then_some(OrdinaryCursorSourceProof(())) + } + /// Preserve a prepared page producer's original evidence and leases. This + /// does not recapture or rebase dependencies when a cached page is selected. + pub(crate) fn from_prepared_rows(reads: CollectionReads, roots: Vec) -> Self { + Self(Rc::new(DependencyAuthority::PreparedRows { + reads, + _roots: roots, + })) + } + /// Combine actual original page authorities, flattening and deduplicating + /// them under a fixed budget. Never build an unbounded historical chain. + pub(crate) fn combine_originals(sources: impl IntoIterator) -> Option { + fn append( + source: CursorSourceDependencies, + leaves: &mut Vec, + ) -> bool { + match source.0.as_ref() { + DependencyAuthority::PreparedUnion(parts) => { + parts.iter().all(|part| append(part.clone(), leaves)) + } + DependencyAuthority::FrameOnly { .. } => false, + _ => { + if !leaves.iter().any(|leaf| Rc::ptr_eq(&leaf.0, &source.0)) { + if leaves.len() == 32 { + return false; + } + leaves.push(source); + } + true + } + } + } + let mut leaves = Vec::new(); + for source in sources { + if !source.unchanged() || !append(source, &mut leaves) { + return None; + } + } + if leaves.is_empty() { + None + } else { + Some(Self(Rc::new(DependencyAuthority::PreparedUnion(leaves)))) + } } /// Unchanged original inputs certify retained rows, while actual new reads /// certify walked rows. Keep the ORIGINAL certificate for a known domain; /// new sources require a bounded frame-only union, not a rebased witness. pub(crate) fn after_partial_walk(&self, fresh: Self) -> Option { + if matches!( + self.0.as_ref(), + DependencyAuthority::PreparedRows { .. } | DependencyAuthority::PreparedUnion(_) + ) { + return Some(Self(Rc::new(DependencyAuthority::FrameOnly { + previous: self.clone(), + fresh, + }))); + } let DependencyAuthority::Canonical { strings: previous, .. } = self.0.as_ref() @@ -130,7 +220,11 @@ impl Capture { reads: Vec::new(), roots: Vec::new(), strings: rustc_hash::FxHashSet::default(), - complete: owner.is_some(), + state: if owner.is_some() { + CaptureState::Ordinary + } else { + CaptureState::Incomplete + }, owner, retainer: context.owned_root_retainer(), })) @@ -156,7 +250,7 @@ impl Capture { return None; } let observation = ACTIVE.with(|active| active.borrow_mut().take())?; - if !observation.complete { + if observation.state == CaptureState::Incomplete { return None; } Some(CursorSourceDependencies(Rc::new( @@ -165,6 +259,8 @@ impl Capture { reads: observation.reads, _roots: observation.roots, strings: observation.strings, + ordinary: (observation.state == CaptureState::Ordinary) + .then_some(OrdinaryCursorSourceProof(())), }, ))) } @@ -177,11 +273,13 @@ impl Drop for Capture { } } -/// Observe presence AND absence of intervals, before selection filters or -/// early winner selection. Property-less competing strings may gain `cursor`. -pub(crate) fn string_properties( - value: Value, -) -> Option { +/// Root an original operand BEFORE its callback-free native projection and +/// observe the actual decoder, including unsuccessful/absent lookups. Rooted +/// ancestor graphs protect child identities while observed edges stay valid; +/// changing an edge invalidates the original certificate before reuse. +/// Operand identities also participate in partial-domain inclusion: observing +/// a new face plist is not covered merely by having seen its string before. +pub(crate) fn observe_projection(value: Value, project: impl FnOnce() -> T) -> T { let rooted = ACTIVE.with(|active| { let mut active = active.borrow_mut(); let observation = active.as_mut()?; @@ -190,13 +288,13 @@ pub(crate) fn string_properties( .as_ref() .is_some_and(CollectionReads::unchanged_and_observe) { - observation.complete = false; + observation.state = CaptureState::Incomplete; return None; } - let identity = SourceStringIdentity(value.bits()); + let identity = SourceObjectIdentity(value.bits()); if !observation.strings.contains(&identity) { let Some(roots) = observation.retainer.retain([value]) else { - observation.complete = false; + observation.state = CaptureState::Incomplete; return None; }; // Root each original object once, before its first projection. @@ -207,27 +305,57 @@ pub(crate) fn string_properties( Some(()) }); if rooted.is_none() { - return neovm_core::emacs_core::value::get_string_text_properties_table_for_value(value); + return project(); } - let (properties, reads) = collection_reads::capture(|| { - // The cursor property is indexed in the original string. Observe its - // contents/storage too, without copying/scanning bytes, so in-place - // string edits cannot retain an old source slot or glyph extent. - if let Some(string) = value.as_lisp_string() { - let _ = string.as_bytes(); - let _ = string.is_multibyte(); - let _ = string.schars(); - } - neovm_core::emacs_core::value::get_string_text_properties_table_for_value(value) - }); + let (result, reads) = collection_reads::capture(project); ACTIVE.with(|active| { if let Some(observation) = active.borrow_mut().as_mut() { if let Some(reads) = reads { observation.reads.push(reads); } else { - observation.complete = false; + observation.state = CaptureState::Incomplete; } } }); + result +} + +/// Observe presence AND absence of intervals before candidate selection. +/// Property-less competing strings may gain `cursor` independently of ticks. +pub(crate) fn string_properties( + value: Value, +) -> Option { + let mut requires_selection = false; + let properties = observe_projection(value, || { + let mut chars = 0; + if let Some(string) = value.as_lisp_string() { + let _ = string.as_bytes(); + let _ = string.is_multibyte(); + chars = string.schars(); + } + let properties = + neovm_core::emacs_core::value::get_string_text_properties_table_for_value(value); + // Observe the ORIGINAL interval lookup while its collection scope is + // active. Category-derived behavior remains conservatively unproven. + requires_selection = properties.as_ref().is_some_and(|props| { + props.has_any_non_nil_property_in_char_range( + neovm_core::buffer::CharRange::new( + neovm_core::buffer::CharPos0::ZERO, + neovm_core::buffer::CharPos0::new(chars), + ), + &[Value::symbol("cursor"), Value::symbol("category")], + ) + }); + properties + }); + if requires_selection { + ACTIVE.with(|active| { + if let Some(observation) = active.borrow_mut().as_mut() { + if observation.state == CaptureState::Ordinary { + observation.state = CaptureState::SourceSelectionRequired; + } + } + }); + } properties } diff --git a/crates/neomacs-layout-engine/src/display_row/builder.rs b/crates/neomacs-layout-engine/src/display_row/builder.rs index 8e9bf3bf0a..bc7dd44593 100644 --- a/crates/neomacs-layout-engine/src/display_row/builder.rs +++ b/crates/neomacs-layout-engine/src/display_row/builder.rs @@ -2534,6 +2534,7 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer slots: &mut Vec, ) -> DisplayRowAppendStatus { let glyph_start = self.area_len(); + let slot_start = slots.len(); let face_id = self.writer.face_id(face); let measurement = self.text_run_measurement(text, face_id); // An explicit or supplied measured plan remains authoritative. Query @@ -2647,6 +2648,24 @@ impl<'layout, 'row, 'measurer> DisplayRowProgressWriter<'layout, 'row, 'measurer break; } } + // Later contextual members can grow the head selected by an earlier + // slot. Refresh that same primitive only after this append is complete. + // A continuation that emitted nothing must stay unselected even if its + // boundary offset is now occupied by a later character's glyph. + let glyphs = &self.writer.row.glyphs[self.writer.area_index()]; + for slot in &mut slots[slot_start..] { + if slot.selected_primitive_dimensions.is_some() + && let Some(offset) = slot.text_glyph_offset + { + slot.selected_primitive_dimensions = + glyphs.get(glyph_start + offset).and_then(|glyph| { + crate::display_cursor::measured_logical_cursor_dimensions( + glyph.pixel_width, + glyph.pixel_height, + ) + }); + } + } status } diff --git a/crates/neomacs-layout-engine/src/display_row/builder/tests/builder_test.rs b/crates/neomacs-layout-engine/src/display_row/builder/tests/builder_test.rs index bf34cb3ede..9bfee9aa8f 100644 --- a/crates/neomacs-layout-engine/src/display_row/builder/tests/builder_test.rs +++ b/crates/neomacs-layout-engine/src/display_row/builder/tests/builder_test.rs @@ -2095,6 +2095,111 @@ fn display_row_builder_groups_arabic_complex_run() { assert!(glyphs.iter().filter(|glyph| glyph.padding).count() >= 3); } +struct SelectedPrimitiveMeasurer; + +impl DisplayGlyphMeasurer for SelectedPrimitiveMeasurer { + fn glyph_advance_px( + &mut self, + _ch: char, + _face_id: FaceId, + _columns: u8, + _fallback_advance_px: f32, + ) -> Option { + None + } + + fn glyph_vertical_metrics_px( + &mut self, + _ch: char, + _face_id: FaceId, + ) -> Option { + Some(DisplayRowVerticalMetrics::new(16.0, 12.0)) + } +} + +fn assert_contextual_selected_primitive_dimensions(item: DisplayItem) { + let row_layout = layout(); + let mut row = GlyphRow::new(GlyphRowRole::Text); + let progress = { + let mut measurer = SelectedPrimitiveMeasurer; + let mut writer = DisplayRowProgressWriter::with_glyph_measurer( + &row_layout, + &mut row, + &mut measurer, + DisplayRowPosition::new(0.0, 0), + 240.0, + ); + // A separate append with a different face and padding makes the + // composition's append-local glyph offsets differ from row indices. + let mut prefix = text_item("中"); + prefix.face = RenderFaceRef::FaceId(FaceId::new(3)); + writer.push_item(prefix); + writer.push_item(item) + }; + let head = row.glyphs[GlyphArea::Text.index()] + .iter() + .find(|glyph| { + !glyph.padding + && matches!(&glyph.glyph_type, GlyphType::Composite { text } if text.as_ref() == "سلام") + }) + .expect("finalized Arabic composition"); + assert_eq!(head.pixel_width, 32.0); + assert_eq!( + progress.slots()[0].selected_primitive_dimensions(), + Some(neovm_core::window::LogicalCursorDimensions { + width: head.pixel_width.round() as u32, + height: head.pixel_height.round() as u32, + }), + ); +} + +#[test] +fn contextual_text_selected_primitive_dimensions_are_finalized() { + assert_contextual_selected_primitive_dimensions(text_item("سلام")); +} + +#[test] +fn contextual_source_mapped_selected_primitive_dimensions_are_finalized() { + assert_contextual_selected_primitive_dimensions(mapped_text_item("سلام")); +} + +#[test] +fn text_selected_primitive_dimensions_preserve_unpainted_and_control_slots() { + let row_layout = layout(); + let mut row = GlyphRow::new(GlyphRowRole::Text); + let (progress, control_progress) = { + let mut measurer = SelectedPrimitiveMeasurer; + let mut writer = DisplayRowProgressWriter::with_glyph_measurer( + &row_layout, + &mut row, + &mut measurer, + DisplayRowPosition::new(0.0, 0), + 240.0, + ); + ( + writer.push_item(text_item("e\u{301}X")), + writer.push_item(control_item('\u{1}')), + ) + }; + let slots = progress.slots(); + assert_eq!(slots[1].selected_primitive_dimensions(), None); + let control = row.glyphs[GlyphArea::Text.index()] + .iter() + .find(|glyph| matches!(glyph.glyph_type, GlyphType::Char { ch: '^' })) + .expect("first control-character primitive"); + assert_eq!( + control_progress.slots()[0].selected_primitive_dimensions(), + Some(neovm_core::window::LogicalCursorDimensions { + width: control.pixel_width.round() as u32, + height: control.pixel_height.round() as u32, + }), + ); + assert_eq!( + control_progress.slots()[0].width_px(), + 2.0 * control.pixel_width + ); +} + #[test] fn display_row_builder_produces_logical_order_rows() { // Slice 5: DisplayRowBuilder::finish normalizes displays_text but no longer diff --git a/crates/neomacs-layout-engine/src/display_source.rs b/crates/neomacs-layout-engine/src/display_source.rs index a35830d9a2..3ad3707848 100644 --- a/crates/neomacs-layout-engine/src/display_source.rs +++ b/crates/neomacs-layout-engine/src/display_source.rs @@ -125,10 +125,15 @@ impl<'a> DisplaySourceContext<'a> { base: RenderFaceRef, face_value: Value, ) -> RenderFaceRef { - self.face_resolver - .as_mut() - .map(|resolver| resolver.resolve_face_ref(base, face_value)) - .unwrap_or(base) + // Interpret the original payload under observation, not just the + // string's cloned interval table. Native face decoding is callback-free; + // observed ancestor edges protect the rooted original plist graph. + crate::cursor_source_dependencies::observe_projection(face_value, || { + self.face_resolver + .as_mut() + .map(|resolver| resolver.resolve_face_ref(base, face_value)) + .unwrap_or(base) + }) } pub(crate) fn resolve_face_sources( diff --git a/crates/neomacs-layout-engine/src/engine.rs b/crates/neomacs-layout-engine/src/engine.rs index 1a60d8a165..9fdc04b3c5 100644 --- a/crates/neomacs-layout-engine/src/engine.rs +++ b/crates/neomacs-layout-engine/src/engine.rs @@ -1139,13 +1139,32 @@ struct RedisplayLispLedger { mini_eob_scroll: mini_eob_scroll::MiniEobScrollLedger, /// `when` answers evaluated after a walk, carried to its one replay. display_when: crate::display_when::DisplayWhenReplayLedger, - /// Original string observations commit only with the accepted matrices. + /// Original authority selected with a historical/prepared row plan. + replay_sources: rustc_hash::FxHashMap< + DisplayWindowId, + crate::cursor_source_dependencies::CursorSourceDependencies, + >, + /// Original source observations commit only with the accepted matrices. cursor_sources: rustc_hash::FxHashMap< DisplayWindowId, crate::cursor_source_dependencies::CursorSourceDependencies, >, } +/// A rejected source projection must not expose its speculative window end. +fn reject_changed_cursor_sources( + evaluator: &mut neovm_core::emacs_core::Context, + window_end_attempt: &mut Option, + changed: bool, +) -> bool { + if changed { + if let Some(attempt) = window_end_attempt.take() { + evaluator.reject_redisplay_window_end_attempt(attempt); + } + } + changed +} + impl RedisplayLispLedger { fn publication_for_site( &self, @@ -1778,6 +1797,7 @@ impl LayoutEngine { purpose: LayoutPurpose, ) -> FrameLayoutAttempt { debug_assert!(purpose.query_window().is_none()); + self.retire_removed_frame_owners(evaluator); if evaluator.redisplay_hook_flow_pending() { return FrameLayoutAttempt::Aborted; } @@ -1789,6 +1809,7 @@ impl LayoutEngine { #[cfg(test)] None, ); + self.retire_removed_frame_owners(evaluator); if evaluator.has_mode_line_display_flow() || evaluator.redisplay_hook_flow_pending() { self.last_frame_display_state = None; return FrameLayoutAttempt::Aborted; @@ -2486,6 +2507,7 @@ impl LayoutEngine { // allocated for this attempt. This is both the multi-window // same-buffer ordering guarantee and the ownership boundary for the // frame-wide face namespace below. + lisp_ledger.replay_sources.clear(); let mut window_plans: Vec = window_params_list .iter() .zip(&window_layout_inputs) @@ -2538,12 +2560,17 @@ impl LayoutEngine { self.build_cursor_only_replay(params, *layout_box, evaluator); let mut prepared_faces = None; if cursor_only.is_none() { - if let Some((replay, faces)) = self.prepared_viewports.replay( + if let Some((replay, faces, sources)) = self.prepared_viewports.replay( frame_id, DisplayWindowId::new(params.window_id), key, params.force_start, ) { + if let Some(sources) = sources { + lisp_ledger + .replay_sources + .insert(DisplayWindowId::new(params.window_id), sources); + } cursor_only = Some(replay); prepared_faces = Some(faces); } @@ -2583,12 +2610,17 @@ impl LayoutEngine { if resolved_start.get() != key.window_start { let mut prepared_key = key.clone(); prepared_key.window_start = resolved_start.get(); - if let Some((replay, faces)) = self.prepared_viewports.replay( + if let Some((replay, faces, sources)) = self.prepared_viewports.replay( frame_id, DisplayWindowId::new(params.window_id), &prepared_key, params.force_start, ) { + if let Some(sources) = sources { + lisp_ledger + .replay_sources + .insert(DisplayWindowId::new(params.window_id), sources); + } cursor_only = Some(replay); prepared_faces = Some(faces); } @@ -2609,12 +2641,19 @@ impl LayoutEngine { }) { let reused = scroll.as_ref().map_or(0, |replay| replay.reused_rows.len()); - if let Some((replay, faces)) = self.prepared_viewports.scroll_replay( - frame_id, - DisplayWindowId::new(params.window_id), - key, - reused, - ) { + if let Some((replay, faces, sources)) = + self.prepared_viewports.scroll_replay( + frame_id, + DisplayWindowId::new(params.window_id), + key, + reused, + ) + { + if let Some(sources) = sources { + lisp_ledger + .replay_sources + .insert(DisplayWindowId::new(params.window_id), sources); + } scroll = Some(replay); prepared_faces = Some(faces); is_edit = false; @@ -2653,8 +2692,22 @@ impl LayoutEngine { }) }) .flatten(); + let prefix_sources = lisp_ledger + .replay_sources + .get(&DisplayWindowId::new(params.window_id)) + .cloned() + .or_else(|| { + self.frame_visual_histories + .get(frame_id) + .and_then(|history| { + history.cursor_sources_for(DisplayWindowId::new( + params.window_id, + )) + }) + }); if let Some(prefix) = scroll.as_ref().or(projected.as_ref()) - && let Some((replay, faces)) = + && let Some(prefix_sources) = prefix_sources + && let Some((replay, faces, sources)) = self.prepared_viewports.complete_forward_scroll( frame_id, DisplayWindowId::new(params.window_id), @@ -2663,8 +2716,14 @@ impl LayoutEngine { geometry, &committed_face_arena, params.force_start, + prefix_sources, ) { + if let Some(sources) = sources { + lisp_ledger + .replay_sources + .insert(DisplayWindowId::new(params.window_id), sources); + } cursor_only = Some(replay); prepared_faces = Some(faces); scroll = None; @@ -4180,6 +4239,7 @@ impl LayoutEngine { std::mem::take(&mut self.retained_window_matrices), &next_retained_window_matrices, self.frame_face_arenas.get(&frame_id), + self.frame_visual_histories.get(frame_id), ); self.retained_window_matrices = next_retained_window_matrices; self.frame_face_arenas.insert(frame_id, sealed_face_arena); @@ -5479,10 +5539,15 @@ impl LayoutEngine { let source_capture = crate::cursor_source_dependencies::Capture::begin(evaluator); let source_window = DisplayWindowId::new(params.window_id); lisp_ledger.cursor_sources.remove(&source_window); - let previous_sources = self - .frame_visual_histories - .get(frame_id) - .and_then(|history| history.cursor_sources_for(source_window)); + let previous_sources = lisp_ledger + .replay_sources + .get(&source_window) + .cloned() + .or_else(|| { + self.frame_visual_histories + .get(frame_id) + .and_then(|history| history.cursor_sources_for(source_window)) + }); let sources_current = previous_sources .as_ref() .is_some_and(|sources| sources.can_replay() && sources.unchanged()); @@ -5825,7 +5890,12 @@ impl LayoutEngine { .as_ref() .is_some_and(|sources| sources.can_replay() && sources.unchanged()); let cursor_only_replay = if sources_current { - cursor_only_replay + cursor_only_replay.map(|mut replay| { + replay.ordinary_cursor_source = previous_sources + .as_ref() + .and_then(|sources| sources.ordinary_cursor_source_proof()); + replay + }) } else { None }; @@ -6337,12 +6407,15 @@ impl LayoutEngine { // empty observation from skipping the strings. Partial walks // preserve original known domains; newly expanded domains // validate this frame but require a fresh canonical next walk. - if !source_capture.unchanged() - || (!canonical_source_walk - && !previous_sources - .as_ref() - .is_some_and(|sources| sources.unchanged())) - { + if reject_changed_cursor_sources( + evaluator, + &mut window_end_attempt, + !source_capture.unchanged() + || (!canonical_source_walk + && !previous_sources + .as_ref() + .is_some_and(|sources| sources.unchanged())), + ) { return LeafLayoutAttempt::LogicalInputsChanged; } let observed = source_capture.finish(); @@ -6355,9 +6428,16 @@ impl LayoutEngine { .as_ref() .and_then(|old| observed.and_then(|fresh| old.after_partial_walk(fresh))) }; - if let Some(sources) = sources { - lisp_ledger.cursor_sources.insert(source_window, sources); - } + let Some(sources) = sources else { + // Unknown observations cannot authorize this frame after + // later Lisp callbacks. Use the coordinator's bounded + // retry/abort policy, not a fabricated empty certificate. + if let Some(attempt) = window_end_attempt.take() { + evaluator.reject_redisplay_window_end_attempt(attempt); + } + return LeafLayoutAttempt::LogicalInputsChanged; + }; + lisp_ledger.cursor_sources.insert(source_window, sources); if position_publication.is_synchronous_query() { self.query_row_coverage = query_row_coverage; self.query_target_prefix_policy = query_target_prefix_policy; diff --git a/crates/neomacs-layout-engine/src/engine/prepared_viewports.rs b/crates/neomacs-layout-engine/src/engine/prepared_viewports.rs index 2d184eb267..87c0af3ea0 100644 --- a/crates/neomacs-layout-engine/src/engine/prepared_viewports.rs +++ b/crates/neomacs-layout-engine/src/engine/prepared_viewports.rs @@ -8,8 +8,14 @@ mod export; mod measurement; +#[cfg(test)] +#[path = "prepared_viewports/tests/certificate_test.rs"] +mod certificate_tests; + use super::*; +use crate::cursor_source_dependencies::CursorSourceDependencies; use crate::frame_face_arena::PreparedFaceSnapshot; +use crate::frame_visual_history::FrameVisualHistory; use crate::incremental_layout::WindowDelta; use neomacs_display_protocol::GlyphRowRole; use neovm_core::buffer::CharPos0; @@ -30,9 +36,9 @@ struct PreparedViewport { computed: bool, // Partial worker pages may extend coverage, but cannot replace a viewport. complete_viewport: bool, - reads: Option, + // The authority belongs to these original rows, not the latest viewport. + cursor_sources: Option, query_freshness: Option, - _source_roots: Vec, } impl PreparedViewport { @@ -40,9 +46,9 @@ impl PreparedViewport { self.retained.key.fontset_generation == neovm_core::emacs_core::fontset::fontset_generation() && self - .reads + .cursor_sources .as_ref() - .is_none_or(CollectionReads::unchanged_and_observe) + .is_some_and(|sources| sources.can_replay() && sources.unchanged()) } } @@ -67,6 +73,15 @@ pub(super) struct PreparedViewports { } impl PreparedViewports { + pub(super) fn retain_live_frames( + &mut self, + live: impl Fn(neovm_core::window::FrameId) -> bool, + ) { + self.entries.retain(|entry| live(entry.frame)); + self.exports.retain(|export| live(export.frame)); + self.invalidated.retain(|(frame, _)| live(*frame)); + } + pub(super) fn retire_invalid_dependencies(&mut self) { self.entries.retain(|entry| { if entry.dependencies_valid() { @@ -124,6 +139,10 @@ impl PreparedViewports { complete_viewport: bool, query_freshness: Option, ) -> bool { + let cursor_sources = CursorSourceDependencies::from_prepared_rows(reads, source_roots); + if !cursor_sources.can_replay() || !cursor_sources.unchanged() { + return false; + } // Retargeting can revisit a page that is already prepared. A preview // must never shorten its still-valid coverage while the rest is being // captured again. Freshness and geometry must match before retaining @@ -188,9 +207,8 @@ impl PreparedViewports { glyphs, computed: true, complete_viewport, - reads: Some(reads), + cursor_sources: Some(cursor_sources), query_freshness, - _source_roots: source_roots, }); self.trim(); // Removing only the newly appended entry leaves both payloads and @@ -293,7 +311,11 @@ impl PreparedViewports { window: DisplayWindowId, key: &RetainedWindowKey, force_start: bool, - ) -> Option<(CursorOnlyReplay, PreparedFaceSnapshot)> { + ) -> Option<( + CursorOnlyReplay, + PreparedFaceSnapshot, + Option, + )> { self.entries.iter().rev().find_map(|entry| { if !entry.complete_viewport || !entry.dependencies_valid() @@ -309,7 +331,7 @@ impl PreparedViewports { .ok()?; // The cursor builder deliberately leaves chrome empty. Even when // revisiting a page, mode-line Lisp must run against today's state. - Some((replay, entry.faces.clone())) + Some((replay, entry.faces.clone(), entry.cursor_sources.clone())) }) } @@ -325,7 +347,12 @@ impl PreparedViewports { geometry: crate::buffer_source::window_geometry::BufferWindowGeometry, arena: &FrameFaceArena, force_start: bool, - ) -> Option<(CursorOnlyReplay, PreparedFaceSnapshot)> { + prefix_sources: CursorSourceDependencies, + ) -> Option<( + CursorOnlyReplay, + PreparedFaceSnapshot, + Option, + )> { if prefix.sync.is_some() || prefix.edit || prefix.face_generation != arena.generation() { return None; } @@ -483,10 +510,18 @@ impl PreparedViewports { Some(point) }), ); + let sources = CursorSourceDependencies::combine_originals([ + prefix_sources.clone(), + entry.cursor_sources.clone()?, + ])?; let replay = candidate .cursor_only_replay_with_forced_start(key, force_start) .ok()?; - Some((replay, arena.prepared_with_retained(&entry.faces).ok()?)) + Some(( + replay, + arena.prepared_with_retained(&entry.faces).ok()?, + Some(sources), + )) }) } @@ -499,7 +534,11 @@ impl PreparedViewports { window: DisplayWindowId, key: &RetainedWindowKey, minimum_reused: usize, - ) -> Option<(ScrollReplay, PreparedFaceSnapshot)> { + ) -> Option<( + ScrollReplay, + PreparedFaceSnapshot, + Option, + )> { self.entries.iter().rev().find_map(|entry| { if !entry.dependencies_valid() || entry.frame != frame @@ -536,7 +575,8 @@ impl PreparedViewports { .len() .checked_sub(replay.exposed_row_base)?; } - (replay.reused_rows.len() > minimum_reused).then(|| (replay, entry.faces.clone())) + (replay.reused_rows.len() > minimum_reused) + .then(|| (replay, entry.faces.clone(), entry.cursor_sources.clone())) }) } @@ -546,6 +586,7 @@ impl PreparedViewports { previous: rustc_hash::FxHashMap, next: &rustc_hash::FxHashMap, faces: Option<&FrameFaceArena>, + previous_sources: Option<&FrameVisualHistory>, ) { self.retire_invalid_dependencies(); self.entries.retain(|entry| { @@ -569,6 +610,14 @@ impl PreparedViewports { return; }; for (window, retained) in previous { + let cursor_sources = + previous_sources.and_then(|history| history.cursor_sources_for(window)); + if !cursor_sources + .as_ref() + .is_some_and(|sources| sources.can_replay() && sources.unchanged()) + { + continue; + } let Some(current) = next.get(&window) else { continue; }; @@ -618,9 +667,8 @@ impl PreparedViewports { glyphs, computed: false, complete_viewport: true, - reads: None, + cursor_sources, query_freshness: None, - _source_roots: Vec::new(), }); } self.trim(); diff --git a/crates/neomacs-layout-engine/src/engine/prepared_viewports/measurement.rs b/crates/neomacs-layout-engine/src/engine/prepared_viewports/measurement.rs index 87d8cd57d6..fa40f6e972 100644 --- a/crates/neomacs-layout-engine/src/engine/prepared_viewports/measurement.rs +++ b/crates/neomacs-layout-engine/src/engine/prepared_viewports/measurement.rs @@ -53,7 +53,8 @@ impl PreparedViewports { self.entries.iter().rev().find_map(|entry| { tracing::trace!(target: "neomacs_layout_engine::motion_geometry", ?scope, computed = entry.computed, - collections_match = entry.reads.as_ref().is_none_or(|reads| reads.unchanged()), + collections_match = entry.cursor_sources.as_ref() + .is_some_and(|sources| sources.can_replay() && sources.unchanged()), fontset_matches = entry.retained.key.fontset_generation == neovm_core::emacs_core::fontset::fontset_generation(), start_present = entry.retained.display_snapshot.rows.iter() diff --git a/crates/neomacs-layout-engine/src/engine/prepared_viewports/tests/certificate_test.rs b/crates/neomacs-layout-engine/src/engine/prepared_viewports/tests/certificate_test.rs new file mode 100644 index 0000000000..c2e345683b --- /dev/null +++ b/crates/neomacs-layout-engine/src/engine/prepared_viewports/tests/certificate_test.rs @@ -0,0 +1,309 @@ +use super::*; +use neovm_core::emacs_core::{Context, Value}; + +fn viewport() -> (Context, PreparedViewport) { + let mut context = Context::new(); + let buffer = context.buffer_manager().current_buffer().unwrap().id(); + context + .buffer_manager_mut() + .get_mut(buffer) + .unwrap() + .insert(&"source row\n".repeat(200)); + let frame = context + .frame_manager_mut() + .create_frame("prepared-certificate", 800, 600, buffer); + context + .frame_manager_mut() + .get_mut(frame) + .unwrap() + .window_system = Some(Value::symbol("neomacs")); + let window = context.frame_manager().get(frame).unwrap().selected_window; + let window = DisplayWindowId::new(window.0 as i64); + context + .eval_str("(goto-char 1) (set-window-start nil 1 t)") + .unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + engine.layout_frame_rust(&mut context, frame); + let retained = engine.retained_window_matrices[&window].clone(); + let faces = engine.frame_face_arenas[&frame].prepared_snapshot(); + ( + context, + PreparedViewport { + frame, + window, + retained, + faces, + rows: 0, + glyphs: 0, + computed: false, + complete_viewport: true, + cursor_sources: None, + query_freshness: None, + }, + ) +} + +#[test] +fn missing_original_certificate_does_not_authorize_prepared_rows() { + let (_context, entry) = viewport(); + assert!( + !entry.dependencies_valid(), + "absence of observations is not an empty source certificate" + ); +} + +#[test] +fn computed_rows_keep_original_reads_instead_of_recapturing_at_admission() { + use neovm_core::tagged::collection_reads; + let (mut context, entry) = viewport(); + let a = context + .eval_str("(setq prepared-computed-a (copy-sequence \"original worker A\"))") + .unwrap(); + let roots = context.retain_gc_roots([a]); + let (_, reads) = collection_reads::capture(|| { + let _ = a.as_lisp_string().unwrap().as_bytes(); + }); + let mut prepared = PreparedViewports::default(); + assert!(prepared.insert_computed( + entry.frame, + entry.window, + entry.retained.clone(), + entry.faces.clone(), + reads.unwrap(), + vec![roots], + true, + None, + )); + let b = context + .eval_str("(copy-sequence \"newer worker B\")") + .unwrap(); + let latest = capture(&context, b); + context.eval_str("(aset prepared-computed-a 0 ?Z)").unwrap(); + assert!(latest.unchanged()); + assert!(!prepared.entries[0].dependencies_valid()); + assert!( + prepared + .replay(entry.frame, entry.window, &entry.retained.key, true) + .is_none() + ); + + let (_, reads) = collection_reads::capture(|| { + let _ = a.as_lisp_string().unwrap().as_bytes(); + }); + context.eval_str("(aset prepared-computed-a 0 ?Y)").unwrap(); + assert!( + !prepared.insert_computed( + entry.frame, + entry.window, + entry.retained, + entry.faces, + reads.unwrap(), + vec![context.retain_gc_roots([a])], + true, + None, + ), + "admission cannot rebase changed original producer reads" + ); +} + +fn capture(context: &Context, value: Value) -> CursorSourceDependencies { + let capture = crate::cursor_source_dependencies::Capture::begin(context); + let _ = crate::cursor_source_dependencies::string_properties(value); + capture.finish().unwrap() +} + +#[test] +fn historical_rows_keep_original_string_authority_after_a_newer_capture() { + let (mut context, entry) = viewport(); + let a = context + .eval_str("(setq prepared-source-a (copy-sequence \"original A\"))") + .unwrap(); + let b = context + .eval_str("(setq prepared-source-b (copy-sequence \"current B\"))") + .unwrap(); + let original = capture(&context, a); + let history = FrameVisualHistory::default() + .with_cursor_sources([(entry.window, original.clone())].into_iter().collect()); + let mut next = entry.retained.clone(); + next.key.window_start += 100; + let mut prepared = PreparedViewports::default(); + prepared.accept( + entry.frame, + [(entry.window, entry.retained.clone())] + .into_iter() + .collect(), + &[(entry.window, next)].into_iter().collect(), + Some(&FrameFaceArena::default()), + Some(&history), + ); + assert_eq!(prepared.entries.len(), 1); + let (_, _, returned_sources) = prepared + .replay(entry.frame, entry.window, &entry.retained.key, true) + .unwrap(); + let returned_sources = returned_sources.unwrap(); + let mut shifted_key = entry.retained.key.clone(); + shifted_key.window_start = entry + .retained + .matrix + .rows + .iter() + .filter(|row| row.enabled && row.role == GlyphRowRole::Text) + .nth(1) + .unwrap() + .start_charpos as i64; + let (_, _, shifted_sources) = prepared + .scroll_replay(entry.frame, entry.window, &shifted_key, 0) + .unwrap(); + let latest = capture(&context, b); + context.eval_str("(aset prepared-source-a 0 ?Z)").unwrap(); + assert!( + latest.unchanged(), + "B's certificate must not stand in for A" + ); + assert!(!original.unchanged()); + assert!( + !returned_sources.unchanged(), + "exact replay must return A's original authority" + ); + assert!( + !shifted_sources.unwrap().unchanged(), + "shifted replay must return A's original authority" + ); + assert!( + prepared + .replay(entry.frame, entry.window, &entry.retained.key, true) + .is_none() + ); + assert!( + prepared + .scroll_replay(entry.frame, entry.window, &shifted_key, 0) + .is_none() + ); + prepared.retire_invalid_dependencies(); + assert!(prepared.entries.is_empty()); + assert_eq!(prepared.invalidated, vec![(entry.frame, entry.window)]); +} + +#[test] +fn forward_completion_certificate_keeps_prefix_and_multiple_original_pages() { + for changed in ["prepared-prefix", "prepared-page-a", "prepared-page-b"] { + let mut context = Context::new(); + let originals: Vec<_> = ["prepared-prefix", "prepared-page-a", "prepared-page-b"] + .into_iter() + .map(|name| { + let value = context + .eval_str(&format!("(setq {name} (copy-sequence \"original\"))")) + .unwrap(); + capture(&context, value) + }) + .collect(); + let combined = CursorSourceDependencies::combine_originals(originals).unwrap(); + assert!(combined.can_replay()); + assert!(combined.unchanged()); + let latest = capture(&context, Value::string("latest B is not an authority")); + context.eval_str(&format!("(aset {changed} 0 ?Z)")).unwrap(); + assert!(latest.unchanged()); + assert!( + !combined.unchanged(), + "completion must retain the original authority of {changed}" + ); + } +} + +#[test] +fn history_without_an_original_certificate_is_not_cached() { + let (_context, entry) = viewport(); + let mut next = entry.retained.clone(); + next.key.window_start += 100; + let mut prepared = PreparedViewports::default(); + for history in [None, Some(FrameVisualHistory::default())] { + prepared.accept( + entry.frame, + [(entry.window, entry.retained.clone())] + .into_iter() + .collect(), + &[(entry.window, next.clone())].into_iter().collect(), + Some(&FrameFaceArena::default()), + history.as_ref(), + ); + assert!(prepared.entries.is_empty()); + } +} + +#[test] +fn frame_only_original_certificate_is_not_cached_as_history() { + let (context, entry) = viewport(); + let original = capture(&context, Value::string("original")); + let fresh = capture(&context, Value::string("expanded")); + let partial = original.after_partial_walk(fresh).unwrap(); + assert!(partial.unchanged()); + assert!(!partial.can_replay()); + let history = FrameVisualHistory::default() + .with_cursor_sources([(entry.window, partial)].into_iter().collect()); + let mut next = entry.retained.clone(); + next.key.window_start += 100; + let mut prepared = PreparedViewports::default(); + prepared.accept( + entry.frame, + [(entry.window, entry.retained)].into_iter().collect(), + &[(entry.window, next)].into_iter().collect(), + Some(&FrameFaceArena::default()), + Some(&history), + ); + assert!(prepared.entries.is_empty()); +} + +#[test] +fn dead_frame_retirement_releases_original_source_roots() { + let (mut context, mut entry) = viewport(); + context.gc_collect_exact(); + let allocated = || neovm_core::tagged::gc::with_tagged_heap(|heap| heap.allocated_count()); + let baseline = allocated(); + entry.cursor_sources = Some(capture( + &context, + Value::string("only prepared rows own this source"), + )); + let mut prepared = PreparedViewports::default(); + prepared.entries.push_back(entry); + context.gc_collect_exact(); + assert_eq!( + allocated(), + baseline + 1, + "original source must remain rooted with its rows" + ); + prepared.retain_live_frames(|_| false); + context.gc_collect_exact(); + assert_eq!( + allocated(), + baseline, + "retiring the dead frame must release its source lease" + ); +} + +#[test] +fn retiring_dead_frames_purges_entries_exports_and_invalidations() { + let (context, mut entry) = viewport(); + let live_frame = neovm_core::window::FrameId(entry.frame.0 + 1); + entry.cursor_sources = Some(capture(&context, Value::string("owned source"))); + let dead_frame = entry.frame; + let window = entry.window; + let export = |frame| ExportedCoverage { + frame, + window, + key: entry.retained.key.clone(), + epoch: 0, + backward_start: None, + visible_source: 0..10, + }; + let exports = vec![export(dead_frame), export(live_frame)]; + let mut prepared = PreparedViewports { + entries: [entry].into_iter().collect(), + exports, + invalidated: vec![(dead_frame, window), (live_frame, window)], + }; + prepared.retain_live_frames(|frame| frame == live_frame); + assert!(prepared.entries.is_empty()); + assert_eq!(prepared.exports.len(), 1); + assert_eq!(prepared.exports[0].frame, live_frame); + assert_eq!(prepared.invalidated, vec![(live_frame, window)]); +} diff --git a/crates/neomacs-layout-engine/src/engine/scroll_coverage.rs b/crates/neomacs-layout-engine/src/engine/scroll_coverage.rs index 2d795b0d5f..bf85d7ea0a 100644 --- a/crates/neomacs-layout-engine/src/engine/scroll_coverage.rs +++ b/crates/neomacs-layout-engine/src/engine/scroll_coverage.rs @@ -386,6 +386,19 @@ impl ScrollCoverage { } impl LayoutEngine { + /// Retire original evaluator leases independently of presentation acceptance. + /// Idle and aborted attempts still process deleted owners, including when + /// every remaining frame is hidden or no selected frame exists. + pub(super) fn retire_removed_frame_owners( + &mut self, + evaluator: &neovm_core::emacs_core::Context, + ) { + self.frame_visual_histories + .retain_live(|frame| evaluator.frame_manager().get(frame).is_some()); + self.prepared_viewports + .retain_live_frames(|frame| evaluator.frame_manager().get(frame).is_some()); + } + /// A completed idle page needs one fresh immutable transport publication. pub fn take_scroll_coverage_publication(&mut self) -> bool { std::mem::take(&mut self.scroll_coverage.publication_pending) @@ -397,6 +410,7 @@ impl LayoutEngine { &mut self, evaluator: &neovm_core::emacs_core::Context, ) -> Option { + self.retire_removed_frame_owners(evaluator); self.prepared_viewports.retire_invalid_dependencies(); if !self.prepared_viewports.invalidated.is_empty() { // Retire the old render-thread certificate even before a new diff --git a/crates/neomacs-layout-engine/src/engine/tests/offscreen_row_test.rs b/crates/neomacs-layout-engine/src/engine/tests/offscreen_row_test.rs index 6e651f54b4..001ebbf1a7 100644 --- a/crates/neomacs-layout-engine/src/engine/tests/offscreen_row_test.rs +++ b/crates/neomacs-layout-engine/src/engine/tests/offscreen_row_test.rs @@ -508,7 +508,7 @@ fn first_visit_with_setup_and_gc( "ordinary point motion must still run viewport resolution" ); } - let (replay, faces) = engine + let (replay, faces, _) = engine .prepared_viewports .replay(frame, display_window, &requested_key, true) .expect("ready coverage must satisfy the requested key"); @@ -764,7 +764,7 @@ fn offscreen_capture_budget_ignores_unused_retained_matrix_capacity() { let mut key = engine.retained_window_matrices[&window_id].key.clone(); key.window_start = 120 * 23; key.point = 125 * 23; - let (replay, _) = engine + let (replay, _, _) = engine .prepared_viewports .replay(frame, window_id, &key, false) .expect("unused slots must not exceed the prepared-cache capacity either"); @@ -1651,7 +1651,7 @@ fn worker_keeps_complete_prefix_before_unsupported_rows() { ); key.window_start += line.len() as i64; key.point = key.window_start; - let (prefix, _) = engine + let (prefix, _, _) = engine .prepared_viewports .scroll_replay(frame, owner, &key, 0) .unwrap(); @@ -3737,7 +3737,7 @@ fn repeated_idle_preview_preserves_a_complete_prepared_page() { let mut key = engine.retained_window_matrices[&owner].key.clone(); key.window_start = start as i64; key.point = (start + 5 * line.len()) as i64; - let (before, _) = engine + let (before, _, _) = engine .prepared_viewports .replay(frame, owner, &key, false) .expect("initial full page"); @@ -3757,7 +3757,7 @@ fn repeated_idle_preview_preserves_a_complete_prepared_page() { assert!(std::time::Instant::now() < deadline); std::thread::yield_now(); } - let (after, _) = engine + let (after, _, _) = engine .prepared_viewports .replay(frame, owner, &key, false) .expect("short preview must preserve the complete prepared page"); diff --git a/crates/neomacs-layout-engine/src/incremental_layout.rs b/crates/neomacs-layout-engine/src/incremental_layout.rs index 169189870e..8b932122a7 100644 --- a/crates/neomacs-layout-engine/src/incremental_layout.rs +++ b/crates/neomacs-layout-engine/src/incremental_layout.rs @@ -653,6 +653,9 @@ pub struct CursorOnlyReplay { pub(crate) retained_cursor: Option, /// Semantic selection survives unchanged placement independently of paint. pub(crate) retained_logical_cursor: Option, + /// Original complete-source evidence, refreshed by the engine at consumption. + pub(crate) ordinary_cursor_source: + Option, /// Sealed frame-face generation that owns every ID in `body_rows`. pub(crate) face_generation: FrameFaceGeneration, } @@ -1633,6 +1636,7 @@ impl RetainedWindowMatrix { dimensions: self.display_snapshot.logical_cursor_dimensions, }) }, + ordinary_cursor_source: None, face_generation: self.face_generation, }) } diff --git a/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs b/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs index 7c68385c0f..b3191434d2 100644 --- a/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs +++ b/crates/neomacs-layout-engine/src/tests/cursor_source_dependencies_test.rs @@ -1,5 +1,100 @@ use super::*; +/// Ordinary reconstruction needs observed cursor absence, not merely absence +/// of a currently winning candidate; relevant interval writes revoke it. +#[test] +fn ordinary_cursor_proof_observes_original_string_absence() { + let mut context = Context::new(); + for (expression, ordinary) in [ + ("(copy-sequence \"XY\")", true), + ("(propertize \"XY\" 'face 'bold)", true), + ("(propertize \"XY\" 'cursor nil)", true), + ("(propertize \"XY\" 'cursor 1)", false), + ( + "(propertize \"XY\" 'category 'neo-unproven-category)", + false, + ), + ] { + let source = context + .eval_str(&format!("(setq neo-proof-source {expression})")) + .unwrap(); + let scope = Capture::begin(&context); + string_properties(source); + let original = scope.finish().unwrap(); + assert_eq!( + original.ordinary_cursor_source_proof().is_some(), + ordinary, + "{expression}" + ); + context + .eval_str("(put-text-property 0 1 'cursor 2 neo-proof-source)") + .unwrap(); + assert!(original.ordinary_cursor_source_proof().is_none()); + } +} + +/// Flattened original authorities have a hard leaf budget, not a growing +/// historical chain, and repeating the same union does not consume more budget. +#[test] +fn original_authority_unions_are_flattened_deduplicated_and_bounded() { + let context = Context::new(); + let mut originals = Vec::new(); + for index in 0..33 { + let source = Value::string(format!("source-{index}")); + let scope = Capture::begin(&context); + string_properties(source); + originals.push(scope.finish().unwrap()); + } + let joined = + CursorSourceDependencies::combine_originals(originals[..32].iter().cloned()).unwrap(); + let repeated = CursorSourceDependencies::combine_originals([joined.clone(), joined]).unwrap(); + let DependencyAuthority::PreparedUnion(parts) = repeated.0.as_ref() else { + panic!("flat original union required") + }; + assert_eq!(parts.len(), 32); + assert!(repeated.unchanged()); + assert!(CursorSourceDependencies::combine_originals(originals).is_none()); +} + +/// A newly decoded payload is a new original operand even if its source string +/// was already observed. Detaching an observed descendant invalidates before GC. +#[test] +fn decoded_payload_domains_and_ancestor_mutations_keep_original_authority() { + let mut context = Context::new(); + let first = context + .eval_str("(setq neo-original-face (list :height 2.0))") + .unwrap(); + let string = Value::string("same-source"); + let capture = |context: &Context, face: Value| { + let scope = Capture::begin(context); + string_properties(string); + observe_projection(face, || neovm_core::emacs_core::value::list_to_vec(&face)); + scope.finish().unwrap() + }; + let original = capture(&context, first); + context + .eval_str("(setcar (cdr (list :height 1.0)) 0.5)") + .unwrap(); + assert!(original.unchanged(), "unrelated payloads do not invalidate"); + let second = context.eval_str("(list :height 0.1)").unwrap(); + let expanded = original + .after_partial_walk(capture(&context, second)) + .unwrap(); + assert!( + !expanded.can_replay(), + "the same string does not cover a new decoded operand" + ); + context + .eval_str("(setcdr neo-original-face (list 0.5))") + .unwrap(); + context.gc_collect_exact(); + assert!(!original.unchanged()); + assert!( + !expanded.unchanged(), + "original ancestor writes remain invalid after descendant reclamation" + ); +} + /// Equal text is not object identity: each original string (including distinct /// interval state) must survive exact GC for its retained read certificate. #[test] diff --git a/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs b/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs index ed9e809322..134c85c594 100644 --- a/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs +++ b/crates/neomacs-layout-engine/src/tests/engine_line_spacing_test.rs @@ -1036,6 +1036,403 @@ fn hidden_canonical_cursor_replay_matches_fresh_positions() { } } +/// Late source rejection occurs after speculative end publication; restore +/// the exact token before asking the frame coordinator to retry. +#[test] +fn late_source_rejection_restores_speculative_window_end() { + use neovm_core::buffer::{EmacsBytePos, LispCharPos1}; + use neovm_core::window::{MatrixRow0, WindowEndRecord}; + let (mut eval, frame, buffer, window) = incr_editing_frame("a\n", 800, 240); + let previous = eval + .frame_manager() + .get(frame) + .unwrap() + .find_window(window) + .unwrap() + .window_end_state(); + let mut attempt = eval.begin_redisplay_window_end_attempt(frame, window, buffer); + assert!(attempt.is_some()); + eval.publish_redisplay_window_end( + frame, + window, + WindowEndRecord::from_positions( + LispCharPos1::new(3), + EmacsBytePos::new(3), + LispCharPos1::new(3), + EmacsBytePos::new(3), + MatrixRow0::new(1), + ), + ); + assert_ne!( + eval.frame_manager() + .get(frame) + .unwrap() + .find_window(window) + .unwrap() + .window_end_state(), + previous + ); + eval.eval_str("(setq neo-end-source (propertize \"XY\" 'cursor 1))") + .unwrap(); + let source = eval.eval_str("neo-end-source").unwrap(); + let capture = crate::cursor_source_dependencies::Capture::begin(&eval); + crate::cursor_source_dependencies::string_properties(source); + eval.eval_str("(put-text-property 0 1 'cursor nil neo-end-source)") + .unwrap(); + assert!(!capture.unchanged()); + assert!(reject_changed_cursor_sources( + &mut eval, + &mut attempt, + !capture.unchanged() + )); + assert!(attempt.is_none(), "rejected local token must be consumed"); + assert_eq!( + eval.frame_manager() + .get(frame) + .unwrap() + .find_window(window) + .unwrap() + .window_end_state(), + previous + ); +} + +/// Deleted owners must release source leases at idle maintenance, even when +/// the remaining frame is hidden and no presentation is accepted afterward. +#[test] +fn deleted_frame_source_roots_retire_during_idle_without_a_presentation() { + use crate::cursor_source_dependencies::{Capture, string_properties}; + use crate::frame_visual_history::FrameVisualHistory; + let (mut eval, frame, buffer, window) = incr_editing_frame("a\n", 800, 240); + let other = eval + .frame_manager_mut() + .create_frame("hidden-survivor", 800, 240, buffer); + eval.frame_manager_mut().get_mut(other).unwrap().visibility = + neovm_core::window::FrameVisibility::Invisible; + // Drain initialization's deferred reclamation before measuring our graph. + for _ in 0..3 { + eval.gc_collect_exact(); + } + let count = || neovm_core::tagged::gc::with_tagged_heap(|heap| heap.allocated_count()); + let baseline = count(); + let source = Value::string("deleted-frame-unique-source"); + let capture = Capture::begin(&eval); + string_properties(source); + let certificate = capture.finish().unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + engine.frame_visual_histories.commit( + frame, + FrameVisualHistory::default().with_cursor_sources( + [( + neomacs_display_protocol::types::DisplayWindowId::new(window.0 as i64), + certificate, + )] + .into_iter() + .collect(), + ), + ); + eval.gc_collect_exact(); + assert_eq!(count(), baseline + 1); + // Deletion also retires ordinary frame parameters. Measure that separate + // graph transition under a temporary root before testing our owner lease. + let setup = eval.retain_gc_roots([source]); + eval.frame_manager_mut().delete_frame(frame); + for _ in 0..3 { + eval.gc_collect_exact(); + } + let with_source = count(); + drop(setup); + assert!(engine.last_frame_display_state.is_none()); + engine.maintain_scroll_coverage(&eval); + eval.gc_collect_exact(); + assert_eq!( + count(), + with_source - 1, + "deleted frame's original source lease must retire without a new accepted frame" + ); + assert!(engine.frame_visual_histories.get(frame).is_none()); +} + +/// Exhausted nested observation scopes mean unknown evidence, not an empty +/// certificate. Persistent failure must abort within the shared retry budget. +#[test] +fn incomplete_source_capture_aborts_without_publishing_speculative_state() { + fn nested(depth: usize, operation: &mut dyn FnMut() -> T) -> T { + if depth == 0 { + operation() + } else { + neovm_core::tagged::collection_reads::capture(|| nested(depth - 1, operation)).0 + } + } + let (mut eval, frame, _, window) = incr_editing_frame("a\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.eval_str("(overlay-put (make-overlay 1 1) 'before-string (propertize \"X\" 'cursor 1))") + .unwrap(); + let previous = eval + .frame_manager() + .get(frame) + .unwrap() + .find_window(window) + .unwrap() + .window_end_state(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + let result = nested(8, &mut || engine.redisplay_frame_attempt(&mut eval, frame)); + assert!( + matches!(result, FrameLayoutAttempt::Aborted), + "unknown source evidence cannot authorize an accepted frame" + ); + let frame_state = eval.frame_manager().get(frame).unwrap(); + assert!(!frame_state.has_prepared_display_presentations()); + assert!(frame_state.active_presentation().is_none()); + assert_eq!( + frame_state.find_window(window).unwrap().window_end_state(), + previous + ); + assert!(engine.last_frame_display_state.is_none()); + // Leaving the exhausted outer scopes must allow the same producer to recover. + assert!(matches!( + engine.redisplay_frame_attempt(&mut eval, frame), + FrameLayoutAttempt::Prepared(_) + )); +} + +/// Integer string coverage takes precedence over buffer point for visible as +/// well as hidden cursors; moved replay needs fresh semantic selection. +#[test] +fn moved_explicit_cursor_authority_matches_fresh_for_both_styles() { + for visible in [false, true] { + let (mut eval, frame, _, window) = incr_editing_frame("ab\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", if visible { Value::T } else { Value::NIL }); + eval.eval_str("(overlay-put (make-overlay 1 1) 'before-string (propertize \"X\" 'cursor 2 'face '(:height 2.0)))").unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + engine.layout_frame_rust(&mut eval, frame); + eval.eval_str("(set-window-point (selected-window) 2)") + .unwrap(); + engine.layout_frame_rust(&mut eval, frame); + let stats = engine.last_layout_stats().clone(); + let actual = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut fresh); + fresh.layout_frame_rust(&mut eval, frame); + let expected = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + let position = |s: &neovm_core::window::WindowDisplaySnapshot| { + s.logical_cursor.as_ref().map(|c| (c.x, c.y, c.row, c.col)) + }; + assert_eq!(position(&actual), position(expected), "visible={visible}"); + assert_eq!( + actual.logical_cursor_dimensions, expected.logical_cursor_dimensions, + "visible={visible}" + ); + assert_eq!( + actual.phys_cursor, expected.phys_cursor, + "visible={visible}" + ); + assert_eq!( + stats.cursor_only_windows, 0, + "moved override must be reselected" + ); + } +} + +/// A reused hidden point row needs its own logical capture, not a fallback +/// published by the newly walked tail. Require actual partial reuse. +#[test] +fn hidden_partial_cursor_on_reused_row_matches_fresh() { + let (mut eval, frame, buffer, window) = incr_editing_frame(&"abc\n".repeat(80), 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", Value::NIL); + scroll_window_to(&mut eval, frame, window, buffer, 1, 21); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + engine.layout_frame_rust(&mut eval, frame); + scroll_window_to(&mut eval, frame, window, buffer, 9, 21); + engine.layout_frame_rust(&mut eval, frame); + let stats = engine.last_layout_stats().clone(); + assert_eq!(stats.scroll_windows, 1); + assert!(stats.reused_rows > 0); + let actual = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + assert!(actual.phys_cursor.is_none()); + assert!(actual.logical_cursor.is_some()); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut fresh); + fresh.layout_frame_rust(&mut eval, frame); + let expected = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + let position = |s: &neovm_core::window::WindowDisplaySnapshot| { + s.logical_cursor.as_ref().map(|c| (c.x, c.y, c.row, c.col)) + }; + assert_eq!(position(&actual), position(expected)); + assert_eq!( + actual.logical_cursor_dimensions, + expected.logical_cursor_dimensions + ); +} + +/// Hidden point rows above or below a localized edit must retain canonical +/// cursor facts while the edit plan actually reuses retained rows. +#[test] +fn hidden_partial_edit_cursor_on_reused_rows_matches_fresh() { + use crate::incremental_layout::edit_sync::{ + EditSyncMode, set_edit_sync_mode_for_test, set_prove_first_for_test, + }; + struct Restore; + impl Drop for Restore { + fn drop(&mut self) { + set_edit_sync_mode_for_test(None); + set_prove_first_for_test(None); + } + } + let _restore = Restore; + set_edit_sync_mode_for_test(Some(EditSyncMode::Sync)); + set_prove_first_for_test(Some(true)); + for dirty_line in [2, 9, 14] { + let (mut eval, frame, buffer, window) = incr_editing_frame(&"abc\n".repeat(80), 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", Value::NIL); + scroll_window_to(&mut eval, frame, window, buffer, 1, 21); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + engine.layout_frame_rust(&mut eval, frame); + eval.eval_str(&format!( + "(save-excursion (goto-char {}) (insert \"X\"))", + dirty_line * 4 + 1 + )) + .unwrap(); + engine.layout_frame_rust(&mut eval, frame); + assert_eq!( + engine.last_layout_stats().edit_windows, + 1, + "dirty_line={dirty_line}" + ); + assert!(engine.last_layout_stats().reused_rows > 0); + let actual = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + assert!(actual.phys_cursor.is_none()); + assert!(actual.logical_cursor.is_some()); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut fresh); + fresh.layout_frame_rust(&mut eval, frame); + let expected = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + let position = |s: &neovm_core::window::WindowDisplaySnapshot| { + s.logical_cursor.as_ref().map(|c| (c.x, c.y, c.row, c.col)) + }; + assert_eq!( + position(&actual), + position(expected), + "dirty_line={dirty_line}" + ); + assert_eq!( + actual.logical_cursor_dimensions, expected.logical_cursor_dimensions, + "dirty_line={dirty_line}" + ); + } +} + +/// Mutating an original face plist must invalidate the source projection even +/// when neither the source string nor its interval table is replaced. +#[test] +fn retained_string_face_payload_mutation_matches_fresh_layout() { + for mutation in [ + "(setcar (cdr neo-payload-face) 0.1)", + "(setcdr neo-payload-face (list 0.1 :foreground \"blue\"))", + ] { + let (mut eval, frame, _, window) = incr_editing_frame("a\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.buffer_manager_mut() + .current_buffer_mut() + .unwrap() + .set_buffer_local("cursor-type", Value::NIL); + eval.eval_str("(progn (setq neo-payload-face (list :height 2.0 :foreground \"red\")) (setq neo-payload-string (propertize \"XY\" 'face neo-payload-face 'cursor 1)) (overlay-put (make-overlay 1 1) 'before-string neo-payload-string))").unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + engine.layout_frame_rust(&mut eval, frame); + eval.eval_str("(setcar (cdr (list :height 1.0)) 0.5)") + .unwrap(); + engine.layout_frame_rust(&mut eval, frame); + assert_eq!( + engine.last_layout_stats().cursor_only_windows, + 1, + "unrelated plist remains irrelevant" + ); + let before = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .logical_cursor_dimensions; + eval.eval_str(mutation).unwrap(); + engine.layout_frame_rust(&mut eval, frame); + let actual = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap() + .clone(); + let mut fresh = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut fresh); + fresh.layout_frame_rust(&mut eval, frame); + let expected = eval + .frame_manager() + .get(frame) + .unwrap() + .redisplay_snapshot(window) + .unwrap(); + assert_ne!( + before, expected.logical_cursor_dimensions, + "mutation changes actual geometry" + ); + assert_eq!( + actual.logical_cursor_dimensions, expected.logical_cursor_dimensions, + "{mutation}" + ); + assert_eq!(engine.last_layout_stats().cursor_only_windows, 0); + } +} + /// A moved point or translated retained row cannot certify an old explicit /// selection. Hidden overrides must be reselected, and retry must discard rows. #[test]