From 314906d1b9723c04033e944876726b8cf56938c3 Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sat, 7 Nov 2026 11:45:55 +0000 Subject: [PATCH 1/3] Fix popup reader buffer selection after asynchronous display --- .../src/emacs_core/display/display/mod.rs | 11 +++++- .../display/display/tests/display_test.rs | 35 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/crates/neovm-core/src/emacs_core/display/display/mod.rs b/crates/neovm-core/src/emacs_core/display/display/mod.rs index 28a88a7071..b1a1e41e73 100644 --- a/crates/neovm-core/src/emacs_core/display/display/mod.rs +++ b/crates/neovm-core/src/emacs_core/display/display/mod.rs @@ -2021,6 +2021,7 @@ fn x_popup_menu_interactive(ctx: &mut Context, position: Value, menu: Value) -> /// throws out of the modal read BEFORE the C-g key is dequeued; the popup then /// exits with the key still queued, the command loop reads it next as /// `keyboard-quit`, and its `cmd_error` echoes "Quit". +/// Read navigation in the selected buffer and root its values across unwinding. fn read_popup_menu_key_sequence(ctx: &mut Context) -> Result<(Vec, Value), Flow> { let count = ctx.specpdl.len(); ctx.try_specbind_or_unwind_to( @@ -2028,7 +2029,15 @@ fn read_popup_menu_key_sequence(ctx: &mut Context) -> Result<(Vec, Value) crate::emacs_core::intern::intern("inhibit-quit"), Value::T, )?; - let read = ctx.read_key_sequence(); + // GNU read_menu_command, like the command loop, follows a buffer + // displayed by an asynchronous callback before resolving its first key. + // Keep the other popup-reader options (including switch-frame handling). + let read = ctx + .read_key_sequence_with_options(crate::keyboard::ReadKeySequenceOptions { + buffer_selection: crate::keyboard::KeySequenceBufferSelection::SelectedWindow, + ..Default::default() + }) + .map(|(keys, binding)| (keys, binding.executed())); // The read result carries Lisp values; keep them in the VM root window // while `unbind_to` can run an unlet/debug-on-exit watcher, exactly as the // command loop's own read does (runtime/eval/command_loop.rs:774-790). diff --git a/crates/neovm-core/src/emacs_core/display/display/tests/display_test.rs b/crates/neovm-core/src/emacs_core/display/display/tests/display_test.rs index 4a31a3c472..800a065b49 100644 --- a/crates/neovm-core/src/emacs_core/display/display/tests/display_test.rs +++ b/crates/neovm-core/src/emacs_core/display/display/tests/display_test.rs @@ -32,6 +32,41 @@ use neomacs_display_protocol::SelectionOwner; use std::fs; use std::sync::{Arc, Mutex}; +/// Popup navigation follows the selected window after callbacks display another buffer. +#[test] +fn popup_key_reader_follows_selected_buffer_after_async_display() { + use crate::emacs_core::eval::RedisplayHookPolicyGuard; + use crate::test_utils::input_wait::InputGateOwner; + let _policy = RedisplayHookPolicyGuard::legacy(); + let mut ctx = crate::test_utils::runtime_startup_context(); + let old = ctx.buffers.create_buffer("*popup-old*"); + ctx.buffers.set_current(old); + let frame = ctx.frames.create_frame("F1", 80, 24, old); + assert!(ctx.frames.select_frame(frame)); + let _input_owner = InputGateOwner::install(&mut ctx); + ctx.eval_str(r#"(progn + (setq timer-list nil timer-idle-list nil unread-command-events nil popup-read-timer-fired nil) + (use-local-map (make-sparse-keymap)) + (local-set-key "a" 'popup-old-command) + (with-current-buffer (get-buffer-create "*popup-new*") + (fundamental-mode) + (use-local-map (make-sparse-keymap)) + (local-set-key "a" 'popup-new-command)) + (run-at-time 0.01 nil + (lambda () + (save-current-buffer (switch-to-buffer "*popup-new*")) + (setq popup-read-timer-fired t unread-command-events (list ?a)))))"#).unwrap(); + let (keys, binding) = super::read_popup_menu_key_sequence(&mut ctx).unwrap(); + assert_eq!(ctx.eval_symbol("popup-read-timer-fired").unwrap(), Value::T); + assert_eq!(keys, vec![Value::fixnum('a' as i64)]); + assert_eq!(binding, Value::symbol("popup-new-command")); + assert_eq!( + ctx.eval_str("(eq (current-buffer) (window-buffer))") + .unwrap(), + Value::T + ); +} + fn builtin_x_popup_menu(eval: &mut Context, args: Vec) -> EvalResult { super::builtin_x_popup_menu_batch(eval, args) } From 788e66428ff0b171ca2a56541cec83047b699383 Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sat, 7 Nov 2026 11:53:54 +0000 Subject: [PATCH 2/3] Match GNU newline font and retained cursor geometry --- .../fixtures/newline-geometry.el | 80 +++++++ .../neomacs-gui-tests/tests/commit_editing.rs | 32 ++- .../src/buffer_source/render_plan.rs | 38 ++-- .../src/display_row/finalizer.rs | 6 +- .../finalizer/tests/finalizer_test.rs | 21 ++ .../src/display_row/metrics.rs | 10 +- .../src/display_row/mod.rs | 53 +++-- .../src/display_row/render_policy.rs | 4 + .../src/display_row/replacement.rs | 29 ++- .../src/row_layout/tests/program_test.rs | 4 + .../src/tests/engine_line_spacing_test.rs | 205 ++++++++++++++++++ 11 files changed, 435 insertions(+), 47 deletions(-) create mode 100644 crates/neomacs-gui-tests/fixtures/newline-geometry.el diff --git a/crates/neomacs-gui-tests/fixtures/newline-geometry.el b/crates/neomacs-gui-tests/fixtures/newline-geometry.el new file mode 100644 index 0000000000..e835a181f4 --- /dev/null +++ b/crates/neomacs-gui-tests/fixtures/newline-geometry.el @@ -0,0 +1,80 @@ +;;; newline-geometry.el --- Newline font and replay regressions -*- lexical-binding: t; -*- + +(defun neo-newline-probe-fail (err) + "Report ERR and terminate the isolated GUI process." + (princ (format "COMMIT-EDITING-FAIL %S\n" err) 'external-debugging-output) + (kill-emacs 1)) + +;; Install before setup, so an unexpected input wait cannot hang the suite. +(run-at-time 10 nil (lambda () (neo-newline-probe-fail "watchdog expired"))) + +(defun neo-newline-probe-y (position) + "Return the redisplayed buffer POSITION's vertical pixel coordinate." + (cdr (posn-x-y (posn-at-point position)))) + +(defun neo-newline-probe-finish () + "Check physical row or cursor geometry after the queued redisplay." + (condition-case err + (progn + (redisplay t) + (let* ((probe (getenv "NEOMACS_COMMIT_PROBE")) + (body-y (neo-newline-probe-y 6)) + (normal (- (neo-newline-probe-y 10) body-y))) + (cond + ((equal probe "replacement-newline") + ;; The replaced position need not have a buffer glyph. Measure + ;; the short row from the following body row instead. + (let ((height (- body-y (neo-newline-probe-y 1) normal))) + (unless (and (> height 0) (< height (/ normal 2.0))) + (error "Replacement newline is not short: %S/%S" height normal)))) + ((equal probe "tall-overlay-newline") + (let ((height (- body-y (neo-newline-probe-y 1) (* 2 normal)))) + (unless (> height (* normal 1.5)) + (error "Overlay newline did not grow: %S/%S" height normal)))) + ((member probe '("raised-text" "lowered-text")) + (let ((height (- (neo-newline-probe-y 5) (neo-newline-probe-y 1))) + (expected (* normal 1.5))) + (unless (<= (abs (- height expected)) 1) + (error "Shifted text lost an opposite extent: %S/%S" height expected)))) + ((equal probe "short-newline-cursor") + (let ((row-height (- body-y (neo-newline-probe-y 5))) + (cursor-height (aref (window-cursor-info) 4))) + (unless (and (> row-height 0) (= cursor-height row-height)) + (error "Replayed cursor differs from short spaced row: %S/%S" + cursor-height row-height)))) + (t (error "Unknown newline probe: %S" probe)))) + (princ "COMMIT-EDITING-PASS\n" 'external-debugging-output) + (kill-emacs 0)) + (error (neo-newline-probe-fail err)))) + +(switch-to-buffer (get-buffer-create "*newline-geometry*")) +(fundamental-mode) +(blink-cursor-mode -1) +(insert "AAA\n\nBBB\nnext\n") +(pcase (getenv "NEOMACS_COMMIT_PROBE") + ((or "raised-text" "lowered-text") + (put-text-property 1 4 'display + (list 'raise (if (equal (getenv "NEOMACS_COMMIT_PROBE") "raised-text") 0.5 -0.5))) + (put-text-property 4 5 'line-height t)) + ("replacement-newline" + (put-text-property 5 6 'display + (propertize "\n" 'face '(:height 0.1 :inverse-video t :extend t)))) + ("tall-overlay-newline" + (overlay-put (make-overlay 5 5) 'before-string + (propertize "\n" 'face '(:height 2.0 :inverse-video t :extend t)))) + ("short-newline-cursor" + (put-text-property 5 6 'face '(:height 0.1)) + (setq-local line-spacing 5))) +(goto-char 1) +(run-at-time + 0.5 nil + (lambda () + (condition-case err + (progn + (redisplay t) + (when (equal (getenv "NEOMACS_COMMIT_PROBE") "short-newline-cursor") + ;; Retain rows first, then move only point onto the short newline. + (goto-char 5) + (redisplay t)) + (run-at-time 0.5 nil #'neo-newline-probe-finish)) + (error (neo-newline-probe-fail err))))) diff --git a/crates/neomacs-gui-tests/tests/commit_editing.rs b/crates/neomacs-gui-tests/tests/commit_editing.rs index 53cd3b6802..dc8c76f13a 100644 --- a/crates/neomacs-gui-tests/tests/commit_editing.rs +++ b/crates/neomacs-gui-tests/tests/commit_editing.rs @@ -20,6 +20,11 @@ fn commit_probe_backend(configured: Option<&str>) -> GuiBackend { /// Run a native commit-editing fixture and require its explicit success marker. fn run_commit_probe(probe: &str) { + run_fixture_probe(probe, "commit-editing.el"); +} + +/// Run an isolated native fixture, retaining diagnostics and checking its marker. +fn run_fixture_probe(probe: &str, fixture: &str) { let root = neomacs_infra::crate_root!().join("../.."); let configured = std::env::var("NEOMACS_GUI_TEST_BACKEND").ok(); let backend = commit_probe_backend(configured.as_deref()); @@ -38,7 +43,7 @@ fn run_commit_probe(probe: &str) { &artifacts, GuiScenario::new( probe, - root.join("crates/neomacs-gui-tests/fixtures/commit-editing.el"), + root.join("crates/neomacs-gui-tests/fixtures").join(fixture), ), ) .with_program(program) @@ -104,3 +109,28 @@ fn overlay_separator_uses_its_small_font_height() { fn log_edit_separator_uses_its_small_font_height() { run_commit_probe("separator"); } + +#[test] +fn replacement_newline_uses_its_own_font_height() { + run_fixture_probe("replacement-newline", "newline-geometry.el"); +} + +#[test] +fn tall_overlay_newline_grows_the_empty_row() { + run_fixture_probe("tall-overlay-newline", "newline-geometry.el"); +} + +#[test] +fn short_newline_cursor_replay_uses_full_row_bounds() { + run_fixture_probe("short-newline-cursor", "newline-geometry.el"); +} + +#[test] +fn raised_text_keeps_the_original_descent() { + run_fixture_probe("raised-text", "newline-geometry.el"); +} + +#[test] +fn lowered_text_keeps_the_original_ascent() { + run_fixture_probe("lowered-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 a3af0820c8..27b17fe442 100644 --- a/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs +++ b/crates/neomacs-layout-engine/src/buffer_source/render_plan.rs @@ -928,8 +928,7 @@ 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); - cursor_height = row.height_without_line_spacing(); - cursor_ascent = row.ascent_px; + let mut point_face_metrics = None; 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; @@ -942,26 +941,31 @@ impl BufferSourceOutputSetup { let face_ascent = face.font_ascent as f32; let face_height = face_ascent + face.font_descent as f32; if face_height > 0.0 { - let (y, height, ascent) = - crate::display_cursor::resolve_cursor_vertical_metrics( - cursor_row_pixel_y, - face_height, - face_ascent, - // Fresh capture clamps against the - // whole row, including spacing. - row.height_px, - cursor_ascent, - geometry.char_height, - row.ends_at_zv, - ); - cursor_row_pixel_y = y; - cursor_height = height; - cursor_ascent = ascent; + point_face_metrics = Some((face_height, face_ascent)); } } break; } } + // A newline/hidden position has no covering text glyph, + // but follows the same full-row cursor bounds as fresh + // rendering. Spacing bounds the cursor; it is not added + // unconditionally to the fallback font's height. + let (face_height, face_ascent) = point_face_metrics + .unwrap_or((row.height_without_line_spacing(), row.ascent_px)); + 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_row/finalizer.rs b/crates/neomacs-layout-engine/src/display_row/finalizer.rs index cfdca8d763..5919f1081a 100644 --- a/crates/neomacs-layout-engine/src/display_row/finalizer.rs +++ b/crates/neomacs-layout-engine/src/display_row/finalizer.rs @@ -229,8 +229,10 @@ pub(crate) fn display_row_visible_content_metrics( ); let ascent = ascent.max(0.0).min(height); let descent = (height - ascent).max(0.0); - max_ascent = max_ascent.max((ascent - glyph.vertical_offset_px).max(0.0)); - max_descent = max_descent.max((descent + glyph.vertical_offset_px).max(0.0)); + // GNU take_vertical_position_into_account enlarges only the side + // displaced by raise/lower; the opposite logical extent is retained. + max_ascent = max_ascent.max(ascent - glyph.vertical_offset_px.min(0.0)); + max_descent = max_descent.max(descent + glyph.vertical_offset_px.max(0.0)); } if saw_glyph { let height = (max_ascent + max_descent).max(1.0); diff --git a/crates/neomacs-layout-engine/src/display_row/finalizer/tests/finalizer_test.rs b/crates/neomacs-layout-engine/src/display_row/finalizer/tests/finalizer_test.rs index f016a59e7f..c081ea2faf 100644 --- a/crates/neomacs-layout-engine/src/display_row/finalizer/tests/finalizer_test.rs +++ b/crates/neomacs-layout-engine/src/display_row/finalizer/tests/finalizer_test.rs @@ -32,6 +32,27 @@ fn phys_cursor(window_id: i64, row: usize, col: u16) -> PhysCursor { } } +/// GNU enlarges only the displaced side; shifting a glyph does not shrink its opposite extent. +#[test] +fn logical_content_metrics_preserve_opposite_extents_when_shifted() { + for (offset, expected) in [ + (-3.0, (16.0, 13.0)), + (3.0, (16.0, 10.0)), + (0.0, (13.0, 10.0)), + ] { + let mut row = GlyphRow::new(GlyphRowRole::Text); + push_text(&mut row, "x"); + let glyph = &mut row.glyphs[GlyphArea::Text as usize][0]; + glyph.pixel_height = 13.0; + glyph.pixel_ascent = 10.0; + glyph.vertical_offset_px = offset; + assert_eq!( + display_row_visible_content_metrics(&row, 13.0, 10.0, |_| None), + expected + ); + } +} + #[test] fn finalizes_matrix_row_with_bidi_reorder() { let mut row = GlyphRow::new(GlyphRowRole::Text); diff --git a/crates/neomacs-layout-engine/src/display_row/metrics.rs b/crates/neomacs-layout-engine/src/display_row/metrics.rs index da507daf39..2a74900906 100644 --- a/crates/neomacs-layout-engine/src/display_row/metrics.rs +++ b/crates/neomacs-layout-engine/src/display_row/metrics.rs @@ -105,17 +105,17 @@ pub(crate) struct DisplayLineHeightMetrics { pub newline_ascent: f32, } -/// The seeded default is redundant only if the newline covers both extents. -/// Ordinary default-font rows can keep their accumulated geometry without -/// scanning glyphs; smaller newline fonts must resolve from actual content. +/// Only an unchanged default-font newline can keep seeded row extents. +/// Other newline fonts resolve from actual content, including taller empty +/// lines; ordinary default-font rows avoid an unnecessary glyph scan. pub(crate) fn newline_requires_height_resolution( policy: crate::display_item::DisplayLineHeightPolicy, face: (f32, f32), default: (f32, f32), ) -> bool { policy != crate::display_item::DisplayLineHeightPolicy::Default - || face.1 < default.1 - || face.0 - face.1 < default.0 - default.1 + || face.1 != default.1 + || face.0 - face.1 != default.0 - default.1 } pub(crate) fn resolve_line_height( diff --git a/crates/neomacs-layout-engine/src/display_row/mod.rs b/crates/neomacs-layout-engine/src/display_row/mod.rs index e44b9d7abd..485ea78da5 100644 --- a/crates/neomacs-layout-engine/src/display_row/mod.rs +++ b/crates/neomacs-layout-engine/src/display_row/mod.rs @@ -1270,9 +1270,6 @@ impl<'metrics> DisplayRowRenderer<'metrics> { }; let item_face_id = render_face_ref_id(item.face, default_row_face_id); let item_resolved_face = state.resolved_face(item_face_id).unwrap_or(base_face); - if policy.stop_before_item(&item, item_face_id, item_resolved_face) { - break DisplayRowRenderStop::SourceExhausted; - } if let RenderFaceRef::FaceId(face_id) = item.face && face_id != default_row_face_id && !row_faces.iter().any(|face| face.face_id == face_id) @@ -1292,6 +1289,36 @@ impl<'metrics> DisplayRowRenderer<'metrics> { } row_faces.push(realized); } + // Intercepted display-property newlines need the same primary + // font geometry as ordinary string newlines, before dispatch. + let newline_font_metrics = if matches!(item.kind, DisplayItemKind::RowBreak(_)) + && self.measurement_mode.uses_concrete_font_geometry() + && let Some(face) = row_faces.iter().find(|face| face.face_id == item_face_id) + && let Some(service) = face_realizer.font_metrics_service_mut() + { + let metrics = service.font_metrics( + &face.font_family, + face.font_weight, + face.italic, + face.font_size, + ); + Some(metrics::DisplayRowMeasuredFaceMetrics::new( + metrics.char_width, + metrics.line_height, + metrics.ascent, + metrics.space_width, + )) + } else { + None + }; + if policy.stop_before_item( + &item, + item_face_id, + item_resolved_face, + newline_font_metrics, + ) { + break DisplayRowRenderStop::SourceExhausted; + } let item_edges = item.box_vertical_edges; let item = item.with_box_run_topology(item_resolved_face.box_type > 0, item_edges); let render_item = DisplayRowRenderItem::from_source_item(item); @@ -1380,6 +1407,7 @@ impl<'metrics> DisplayRowRenderer<'metrics> { item_face_id, render_item.source_item().box_vertical_edges, render_item.source_item().box_run_membership, + newline_font_metrics, )); break DisplayRowRenderStop::RowBreak(row_break); } @@ -1387,7 +1415,7 @@ impl<'metrics> DisplayRowRenderer<'metrics> { }; let resolved_row_metrics = if let ( DisplayRowRenderStop::RowBreak(row_break), - Some((row_break_face_id, box_edges, box_membership)), + Some((row_break_face_id, box_edges, box_membership, newline_font_metrics)), ) = (stop, row_break_face) { let mut finalizer = DisplayRowLineEndFinalizer::new( @@ -1400,22 +1428,9 @@ impl<'metrics> DisplayRowRenderer<'metrics> { box_edges, box_membership, ); - // Measure the primary newline font, not a newline glyph or the - // row's seeded default extents. Logical-cell rows stay untouched. - if self.measurement_mode.uses_concrete_font_geometry() - && let Some(face) = row_faces - .iter() - .find(|face| face.face_id == row_break_face_id) - && let Some(service) = face_realizer.font_metrics_service_mut() - { - let metrics = service.font_metrics( - &face.font_family, - face.font_weight, - face.italic, - face.font_size, - ); + if let Some(metrics) = newline_font_metrics { finalizer = finalizer.with_newline_font_metrics( - builder::DisplayRowVerticalMetrics::new(metrics.line_height, metrics.ascent), + builder::DisplayRowVerticalMetrics::new(metrics.row_height(), metrics.ascent()), ); } finalizer.finalize(row, &row_faces).map(|metrics| { diff --git a/crates/neomacs-layout-engine/src/display_row/render_policy.rs b/crates/neomacs-layout-engine/src/display_row/render_policy.rs index eae424a24b..9e1ab23271 100644 --- a/crates/neomacs-layout-engine/src/display_row/render_policy.rs +++ b/crates/neomacs-layout-engine/src/display_row/render_policy.rs @@ -11,11 +11,15 @@ pub(crate) enum DisplayRowRenderClipBehavior { } pub(crate) trait DisplayRowRenderPolicy { + /// Inspect an admitted face before consuming an item. Concrete-font + /// newline measurements are available here even when a policy intercepts + /// the break; logical-cell rendering never supplies font measurements. fn stop_before_item( &mut self, _item: &DisplayItem, _face_id: FaceId, _face: &ResolvedFace, + _newline_font_metrics: Option, ) -> bool { false } diff --git a/crates/neomacs-layout-engine/src/display_row/replacement.rs b/crates/neomacs-layout-engine/src/display_row/replacement.rs index 06213bade6..4a5ac3ce6f 100644 --- a/crates/neomacs-layout-engine/src/display_row/replacement.rs +++ b/crates/neomacs-layout-engine/src/display_row/replacement.rs @@ -80,21 +80,31 @@ struct DisplayReplacementStringRenderPolicy<'a, M> { impl DisplayRowRenderPolicy for DisplayReplacementStringRenderPolicy<'_, M> { + /// Capture measured newline extents without consuming the buffer row + /// transition, which remains the replacement-string walker's responsibility. fn stop_before_item( &mut self, item: &DisplayItem, face_id: FaceId, face: &ResolvedFace, + newline_font_metrics: Option, ) -> bool { if let DisplayItemKind::RowBreak(row_break) = item.kind { - self.produced_row_break = Some(DisplayReplacementStringLineBreak::from_resolved_face( + let mut line_break = DisplayReplacementStringLineBreak::from_resolved_face( face_id, face, self.fallback_metrics, row_break.line_height, row_break.line_spacing, item.box_vertical_edges, - )); + ); + if let Some(metrics) = newline_font_metrics { + line_break.metrics = metrics; + line_break.extend_face = line_break + .extend_face + .map(|face| DisplayRowExtendFace::new(face.background(), face_id, metrics)); + } + self.produced_row_break = Some(line_break); true } else { false @@ -311,7 +321,20 @@ impl DisplayReplacementStringRowSession { )?; let stop = if let Some(mut line_break) = render_policy.produced_row_break { match line_break.line_height() { - DisplayLineHeightPolicy::Default => { + DisplayLineHeightPolicy::Default + if !state.measurement_mode().uses_concrete_font_geometry() + || !super::metrics::newline_requires_height_resolution( + line_break.line_height(), + ( + line_break.metrics().row_height(), + line_break.metrics().ascent(), + ), + ( + replacement_append_context.fallback_metrics.row_height(), + replacement_append_context.fallback_metrics.ascent(), + ), + ) => + { outcome.include_vertical_metrics(row_geometry); let metrics = line_break.metrics(); row_geometry diff --git a/crates/neomacs-layout-engine/src/row_layout/tests/program_test.rs b/crates/neomacs-layout-engine/src/row_layout/tests/program_test.rs index 2c29547f02..1a2a2557ea 100644 --- a/crates/neomacs-layout-engine/src/row_layout/tests/program_test.rs +++ b/crates/neomacs-layout-engine/src/row_layout/tests/program_test.rs @@ -300,6 +300,9 @@ fn complete_row_program_uses_exact_captured_fonts_on_worker() { position = progress.end(); slots.extend(progress.slots().iter().cloned()); if let Some(newline) = newline { + // Supply this independent measurer's primary newline extents, + // just as the buffer reader and captured worker do. Without + // them, the finalizer keeps seeded rather than actual maxima. let _ = DisplayRowLineEndFinalizer::new( newline, FaceId::new(1), @@ -310,6 +313,7 @@ fn complete_row_program_uses_exact_captured_fonts_on_worker() { Default::default(), Default::default(), ) + .with_newline_font_metrics(minimum) .finalize(&mut expected, &faces); } } 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 8ab4ed7f75..0f5c0bc6c5 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 @@ -94,6 +94,28 @@ fn small_newline_face_does_not_shrink_terminal_rows() { assert_eq!(rows[2].height_px, rows[0].height_px); } +/// Terminal display ignores graphical raise/lower even with content-only line height. +#[test] +fn shifted_text_does_not_resize_terminal_cells() { + for raise in [-0.5, 0.5] { + let (mut eval, frame, _buffer, window) = incr_editing_frame("AAA\n\nBBB\n", 800, 240); + eval.eval_str(&format!("(progn (put-text-property 1 4 'display '(raise {raise})) (put-text-property 4 5 'line-height t))")).unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + let FrameLayoutAttempt::Prepared(output) = engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("terminal frame should prepare") + }; + let rows = &output + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows; + assert_eq!(rows[0].height_px, rows[1].height_px, "raise {raise}"); + } +} + /// Overlay newlines use their own GUI font, but never shrink terminal cells. #[test] fn small_overlay_newline_uses_its_own_font_geometry() { @@ -159,6 +181,189 @@ fn small_overlay_newline_uses_its_own_font_geometry() { } } +/// Both smaller and larger empty newline fonts determine GUI row extents. +#[test] +fn tall_newline_grows_empty_buffer_and_string_rows() { + for source in ["buffer", "overlay", "replacement"] { + let (mut eval, frame, _, window) = incr_editing_frame("AAA\n\nBBB\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + let face = "'(:height 2.0 :inverse-video t :extend t)"; + let setup = match source { + "buffer" => format!("(put-text-property 5 6 'face {face})"), + "overlay" => format!( + "(overlay-put (make-overlay 5 5) 'before-string (propertize \"\\n\" 'face {face}))" + ), + "replacement" => { + format!("(put-text-property 5 6 'display (propertize \"\\n\" 'face {face}))") + } + _ => unreachable!(), + }; + eval.eval_str(&setup).unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + let FrameLayoutAttempt::Prepared(output) = engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("tall newline frame should prepare"); + }; + let rows = &output + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows; + assert!( + rows[1].height_px > rows[0].height_px * 1.5, + "{source} newline should grow an empty row: normal={}, newline={}", + rows[0].height_px, + rows[1].height_px + ); + assert_eq!(rows[2].pixel_y - rows[1].pixel_y, rows[1].height_px); + } +} + +/// Display-property newlines resolve their own font without shrinking mixed text or TTY cells. +#[test] +fn small_replacement_newline_uses_its_own_font_geometry() { + for graphical in [false, true] { + for extend in ["nil", "t"] { + for prefix in ["", "x"] { + let (mut eval, frame, _, window) = incr_editing_frame("AAA\n\nBBB\n", 800, 240); + if graphical { + realize_test_gui_frame(&mut eval, frame); + } + eval.eval_str(&format!( + "(put-text-property 5 6 'display (concat \"{prefix}\" (propertize \"\\n\" 'face '(:height 0.1 :inverse-video t :extend {extend}))))" + )).unwrap(); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + if graphical { + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + } + let FrameLayoutAttempt::Prepared(output) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("replacement frame should prepare"); + }; + let rows = &output + .window_matrices + .iter() + .find(|entry| entry.window_id.get() == window.0 as i64) + .unwrap() + .matrix + .rows; + if graphical && prefix.is_empty() { + assert!( + rows[1].height_px < rows[0].height_px / 2.0, + "replacement newline should shrink: normal={}, newline={}", + rows[0].height_px, + rows[1].height_px + ); + } else { + assert_eq!(rows[1].height_px, rows[0].height_px); + } + assert_eq!(rows[2].height_px, rows[0].height_px); + assert_eq!(rows[2].pixel_y - rows[1].pixel_y, rows[1].height_px); + } + } + } +} + +/// Point-only replay must use the same full-row cursor bounds as fresh rendering. +#[test] +fn newline_cursor_replay_preserves_font_and_spacing_metrics() { + for (height, box_spec, hide_text) in [ + ("0.1", "", false), + ("1.0", "", false), + ("2.0", "", false), + ("0.1", ":box 1", false), + ("0.1", "", true), + ] { + for spacing in [0, 5] { + for extend in ["nil", "t"] { + let (mut eval, frame, buffer, window) = + incr_editing_frame("alpha\n\nbeta\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.eval_str(&format!( + "(put-text-property 7 8 'face '(:height {height} :extend {extend} {box_spec}))" + )) + .unwrap(); + if hide_text { + eval.eval_str("(put-text-property 8 10 'invisible t)") + .unwrap(); + } + eval.buffer_manager_mut() + .get_mut(buffer) + .unwrap() + .set_buffer_local("line-spacing", Value::fixnum(spacing)); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + let FrameLayoutAttempt::Prepared(initial) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("initial frame should prepare"); + }; + let initial_rows: Vec<_> = initial + .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.ascent_px)) + .collect(); + for point in [7, 1, 8, 11, 13, 7] { + eval.eval_str(&format!("(set-window-point (selected-window) {point})")) + .unwrap(); + let FrameLayoutAttempt::Prepared(replayed) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("replay should prepare"); + }; + assert_eq!(engine.last_layout_stats().cursor_only_windows, 1); + assert!(engine.last_layout_stats().reused_rows > 0); + let rows: Vec<_> = replayed + .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.ascent_px)) + .collect(); + assert_eq!( + rows, initial_rows, + "point motion must not change row geometry" + ); + 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 should prepare"); + }; + let replayed = replayed.materialize(); + let rebuilt = rebuilt.materialize(); + let actual = replayed + .active_cursor() + .map(|cursor| (cursor.y, cursor.height, cursor.ascent)); + let expected = rebuilt + .active_cursor() + .map(|cursor| (cursor.y, cursor.height, cursor.ascent)); + if !hide_text || !(8..10).contains(&point) { + assert!(expected.is_some(), "visible point must have a cursor"); + } + assert_eq!( + actual, expected, + "cursor replay: height={height}, spacing={spacing}, extend={extend}, box={box_spec}, hidden={hide_text}, point={point}" + ); + } + } + } + } +} + #[test] fn text_cursor_keeps_its_metrics_when_line_spacing_enlarges_the_row() { let (mut eval, frame, buffer, _window) = incr_editing_frame("alpha\nbeta\n", 800, 240); From a48137e1f7e0abd33258b90fc032096ff21b025c Mon Sep 17 00:00:00 2001 From: Alfonso Alvarez Date: Sat, 7 Nov 2026 15:03:47 +0000 Subject: [PATCH 3/3] Unify fresh and retained cursor geometry with GNU glyph metrics --- .../src/glyph_matrix.rs | 3 +- .../fixtures/newline-geometry.el | 45 ++++- .../neomacs-gui-tests/tests/commit_editing.rs | 71 ++++++++ .../src/buffer_source/render_plan.rs | 108 ++++++++---- .../src/buffer_source/row_lifecycle.rs | 21 ++- .../src/display_cursor.rs | 58 ++++++- .../source_append/tests/source_append_test.rs | 1 + .../src/engine/tests/engine_test.rs | 1 + .../src/tests/engine_line_spacing_test.rs | 161 ++++++++++++++++++ 9 files changed, 411 insertions(+), 58 deletions(-) diff --git a/crates/neomacs-display-protocol/src/glyph_matrix.rs b/crates/neomacs-display-protocol/src/glyph_matrix.rs index 97477740c8..9889d878df 100644 --- a/crates/neomacs-display-protocol/src/glyph_matrix.rs +++ b/crates/neomacs-display-protocol/src/glyph_matrix.rs @@ -1090,7 +1090,8 @@ pub struct GlyphRow { /// /// Mirrors GNU `struct glyph_row::height`. `0.0` means unset. pub height_px: f32, - /// Additional logical descent from line spacing. Cursor height excludes it. + /// Additional logical descent from line spacing. Ordinary point glyphs do + /// not inherit it; GNU's synthetic newline cursor uses the full row. #[serde(default)] pub line_spacing_px: f32, /// Authoritative baseline ascent from row top in pixels. diff --git a/crates/neomacs-gui-tests/fixtures/newline-geometry.el b/crates/neomacs-gui-tests/fixtures/newline-geometry.el index e835a181f4..d4b7c3e0ec 100644 --- a/crates/neomacs-gui-tests/fixtures/newline-geometry.el +++ b/crates/neomacs-gui-tests/fixtures/newline-geometry.el @@ -12,6 +12,17 @@ "Return the redisplayed buffer POSITION's vertical pixel coordinate." (cdr (posn-x-y (posn-at-point position)))) +(defun neo-newline-probe-stats-size () + "Return the layout telemetry size in Neomacs, or nil 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)))) + +(defvar neo-newline-probe-cold-cursor nil + "Physical cursor captured by the first mixed-height full layout.") + (defun neo-newline-probe-finish () "Check physical row or cursor geometry after the queued redisplay." (condition-case err @@ -36,6 +47,14 @@ (expected (* normal 1.5))) (unless (<= (abs (- height expected)) 1) (error "Shifted text lost an opposite extent: %S/%S" height expected)))) + ((string-prefix-p "mixed-" probe) + (let ((height (- (neo-newline-probe-y 5) (neo-newline-probe-y 1))) + (cursor (window-cursor-info))) + (unless (and (> height 0) (= (aref cursor 4) height) + (= (aref neo-newline-probe-cold-cursor 4) height) + (= (aref cursor 5) (aref neo-newline-probe-cold-cursor 5))) + (error "Mixed newline cursor must fill the row, cold and replayed: %S/%S/%S" + neo-newline-probe-cold-cursor cursor height)))) ((equal probe "short-newline-cursor") (let ((row-height (- body-y (neo-newline-probe-y 5))) (cursor-height (aref (window-cursor-info) 4))) @@ -65,16 +84,32 @@ ("short-newline-cursor" (put-text-property 5 6 'face '(:height 0.1)) (setq-local line-spacing 5))) -(goto-char 1) +(when (string-prefix-p "mixed-" (getenv "NEOMACS_COMMIT_PROBE")) + (put-text-property 1 4 'face '(:height 2.0)) + (put-text-property 4 5 'face + (list :height (if (string-match-p "small" (getenv "NEOMACS_COMMIT_PROBE")) 0.1 1.0))) + (setq-local line-spacing (if (string-match-p "spaced" (getenv "NEOMACS_COMMIT_PROBE")) 5 0))) +(goto-char (if (string-prefix-p "mixed-" (getenv "NEOMACS_COMMIT_PROBE")) 4 1)) (run-at-time 0.5 nil (lambda () (condition-case err (progn (redisplay t) - (when (equal (getenv "NEOMACS_COMMIT_PROBE") "short-newline-cursor") - ;; Retain rows first, then move only point onto the short newline. - (goto-char 5) - (redisplay t)) + (when (string-suffix-p "newline-cursor" (getenv "NEOMACS_COMMIT_PROBE")) + (when (string-prefix-p "mixed-" (getenv "NEOMACS_COMMIT_PROBE")) + (setq neo-newline-probe-cold-cursor (window-cursor-info)) + (goto-char 1) + (redisplay t)) + ;; Retain rows first, then move only point onto the newline. + (let ((before (neo-newline-probe-stats-size))) + (goto-char (if neo-newline-probe-cold-cursor 4 5)) + (redisplay t) + (if before + (princ (format "CURSOR-REPLAY-STATS %d %d\n" + before (neo-newline-probe-stats-size)) + 'external-debugging-output) + (princ "CURSOR-REPLAY-REFERENCE-GEOMETRY-ONLY\n" + 'external-debugging-output)))) (run-at-time 0.5 nil #'neo-newline-probe-finish)) (error (neo-newline-probe-fail err))))) diff --git a/crates/neomacs-gui-tests/tests/commit_editing.rs b/crates/neomacs-gui-tests/tests/commit_editing.rs index dc8c76f13a..d4b0b8f7b9 100644 --- a/crates/neomacs-gui-tests/tests/commit_editing.rs +++ b/crates/neomacs-gui-tests/tests/commit_editing.rs @@ -48,6 +48,13 @@ fn run_fixture_probe(probe: &str, fixture: &str) { ) .with_program(program) .with_env("NEOMACS_COMMIT_PROBE", probe) + .with_env( + "NEOMACS_LAYOUT_STATS_FILE", + artifacts + .join("layout-stats.log") + .to_string_lossy() + .into_owned(), + ) .with_env("RUST_LOG", "warn"); for (key, value) in session.env() { plan = plan.with_env(key, value); @@ -62,6 +69,58 @@ 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") + && !stderr.contains("CURSOR-REPLAY-REFERENCE-GEOMETRY-ONLY") + { + let marker = stderr + .lines() + .find_map(|line| line.strip_prefix("CURSOR-REPLAY-STATS ")) + .expect("missing replay telemetry interval"); + let bounds: Vec = marker + .split_whitespace() + .map(|value| value.parse().unwrap()) + .collect(); + assert_eq!(bounds.len(), 2, "{marker}"); + let stats = fs::read_to_string(artifacts.join("layout-stats.log")).unwrap(); + let interval = stats + .get(bounds[0]..bounds[1]) + .expect("invalid replay telemetry interval"); + assert!( + cursor_replay_reused_rows(interval), + "point move did not reuse rows through cursor-only replay: {interval}" + ); + } +} + +/// Require the marked redisplay to reuse body rows rather than rebuilding them. +fn cursor_replay_reused_rows(stats: &str) -> bool { + stats.lines().any(|line| { + let counter = |name: &str| { + line.split_whitespace().find_map(|entry| { + entry + .strip_prefix(name) + .and_then(|value| value.parse::().ok()) + }) + }; + counter("cursor_only=").is_some_and(|value| value > 0) + && counter("reused=").is_some_and(|value| value > 0) + && counter("relaid_body=") == Some(0) + }) +} + +#[test] +fn replay_telemetry_rejects_full_layout_and_missing_reuse() { + assert!(cursor_replay_reused_rows( + "cursor_only=1 reused=3 relaid_body=0" + )); + for stats in [ + "", + "full=1 reused=3 relaid_body=0", + "cursor_only=1 reused=0 relaid_body=0", + "cursor_only=1 reused=3 relaid_body=1", + ] { + assert!(!cursor_replay_reused_rows(stats), "{stats}"); + } } /// Backend aliases and platform defaults must not require a live compositor. @@ -125,6 +184,18 @@ fn short_newline_cursor_replay_uses_full_row_bounds() { run_fixture_probe("short-newline-cursor", "newline-geometry.el"); } +#[test] +fn mixed_newline_cursor_matches_gnu_row_geometry() { + for probe in [ + "mixed-newline-cursor", + "mixed-small-newline-cursor", + "mixed-spaced-newline-cursor", + "mixed-spaced-small-newline-cursor", + ] { + run_fixture_probe(probe, "newline-geometry.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 27b17fe442..5db0096f71 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, GlyphType}; +use neomacs_display_protocol::glyph_matrix::{FaceFillItem, GlyphArea, GlyphRow, GlyphType}; use neomacs_display_protocol::types::{Color, DisplayWindowId, Rect}; use neovm_core::buffer::BufferId; use neovm_core::window::{FrameId, WindowId}; @@ -406,21 +406,44 @@ fn replay_cursor_paint( ) } -/// Re-decorate a window's cursor for the current point on an already-installed -/// grid row (Phase 2 scroll fast path). Reads the row geometry from the grid, -/// resolves the visual column, and writes the cursor onto the matrix row + the -/// window snapshot, then publishes the cursor via the shared role-directed -/// machinery (see [`publish_fast_path_cursor`]) so an inactive window's cursor -/// never clobbers the active window's frame phys-cursor. Mirrors the Phase 1 -/// cursor-only branch but sources the row from the grid (the cursor may land in -/// a reused or a newly-exposed row). +/// Retained point glyphs keep their measured descent; a real newline is the +/// synthetic full-row glyph from GNU append_space_for_newline, including spacing. +fn retained_point_cursor_metrics( + row: &GlyphRow, + point: usize, + buffer_newline: bool, + measurement: DisplayRowMeasurementPolicy, +) -> Option<(f32, f32)> { + 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 glyph.pixel_height > 0.0 { + return Some((glyph.pixel_height, glyph.pixel_ascent)); + } + if measurement.uses_concrete_font_geometry() { + // Zero means unmeasured, not a zero-descent point font. Fresh + // capture distinguishes active-face and special-glyph fallbacks; + // without a measurement replay cannot prove either geometry. + return None; + } + } + Some((row.height_without_line_spacing(), row.ascent_px)) +} + +/// 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)] -fn decorate_window_cursor( +fn try_decorate_window_cursor( output: &mut TextWindowOutputTarget<'_>, output_emitter: &mut WindowOutputEmitter, window_id: u64, cursor_row: usize, point: usize, + buffer_newline: bool, style: CursorStyle, params: &WindowParams, text_area_left: f32, @@ -428,7 +451,8 @@ fn decorate_window_cursor( char_w: f32, default_height: f32, 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) => { @@ -451,12 +475,21 @@ fn decorate_window_cursor( break; } } - ( + 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, - row.height_without_line_spacing(), + face_height, + face_ascent, + row.height_px, row.ascent_px, - width, - ) + default_height, + row.ends_at_zv, + ); + (y, height, ascent, width) } None => (0.0, default_height, default_ascent, char_w), }; @@ -492,6 +525,7 @@ fn decorate_window_cursor( text_area_left, window_top, ); + true } impl BufferSourceOutputSetup { @@ -928,31 +962,24 @@ 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); - let mut point_face_metrics = None; 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; - // Use the point face's descent, not the tallest - // neighbouring face's full cell, just as the - // canonical cursor capture path does. - if let Some(face) = - output.output_target().builder().output_face(glyph.face_id) - { - let face_ascent = face.font_ascent as f32; - let face_height = face_ascent + face.font_descent as f32; - if face_height > 0.0 { - point_face_metrics = Some((face_height, face_ascent)); - } - } break; } } - // A newline/hidden position has no covering text glyph, - // but follows the same full-row cursor bounds as fresh - // rendering. Spacing bounds the cursor; it is not added - // unconditionally to the fallback font's height. - let (face_height, face_ascent) = point_face_metrics - .unwrap_or((row.height_without_line_spacing(), row.ascent_px)); + 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, @@ -1590,12 +1617,14 @@ impl BufferSourceOutputSetup { if let Some(cursor_row) = cursor_row && let Some(style) = cursor_style_for_window(params) { - decorate_window_cursor( + if !try_decorate_window_cursor( &mut output, &mut output_emitter, output_window_id, cursor_row, scroll.new_point as usize, + buf_access.byte_at(buf_access.charpos_to_bytepos(scroll.new_point)) + == Some(b'\n'), style, params, walk_setup.text_area_left, @@ -1603,7 +1632,14 @@ impl BufferSourceOutputSetup { geometry.char_width, geometry.char_height, window_metrics.ascent(), - ); + default_face.measurement_policy(), + ) { + 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. 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 dce0d02610..1c83fce09a 100644 --- a/crates/neomacs-layout-engine/src/buffer_source/row_lifecycle.rs +++ b/crates/neomacs-layout-engine/src/buffer_source/row_lifecycle.rs @@ -2512,14 +2512,19 @@ impl BufferSourceLineBreakSourceAction { x: f32, col: usize, ) -> CapturedCursorInfo { - CapturedCursorInfo::from_active_face_state( - active_face_state, - CapturedCursorPlacement::from_row_text_position( - row_geometry.text_position(x, self.ch_start_byte_idx, col), - CapturedCursorSlotWidth::FaceChar, - false, - ), - ) + // append_space_for_newline gives the synthetic glyph the accumulated + // row extents. Resolve after finalization, when spacing is also known. + CapturedCursorInfo { + vertical_source: crate::display_cursor::CursorVerticalSource::SyntheticNewline, + ..CapturedCursorInfo::from_active_face_state( + active_face_state, + CapturedCursorPlacement::from_row_text_position( + row_geometry.text_position(x, self.ch_start_byte_idx, col), + CapturedCursorSlotWidth::FaceChar, + false, + ), + ) + } } pub(crate) fn capture_cursor_if_point( diff --git a/crates/neomacs-layout-engine/src/display_cursor.rs b/crates/neomacs-layout-engine/src/display_cursor.rs index f06788ef2e..910090f4e1 100644 --- a/crates/neomacs-layout-engine/src/display_cursor.rs +++ b/crates/neomacs-layout-engine/src/display_cursor.rs @@ -80,8 +80,19 @@ impl ResolvedBoxCursorPaint { } } +/// GNU's synthetic newline glyph uses finalized row extents, not its face alone. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum CursorVerticalSource { + PointGlyph, + /// Prefer the finalized point glyph (including fallback fonts); retain the + /// active face only when no measured glyph represents the capture. + ActiveFace, + SyntheticNewline, +} + #[derive(Clone, Copy, Debug)] pub(crate) struct CapturedCursorInfo { + pub(crate) vertical_source: CursorVerticalSource, pub(crate) x: f32, pub(crate) y: f32, pub(crate) face_w: f32, @@ -851,6 +862,7 @@ impl CapturedCursorInfo { Self { x: placement.x, y: placement.y, + vertical_source: CursorVerticalSource::PointGlyph, face_w: visual_state.face_width, face_h: visual_state.face_height, face_ascent: visual_state.face_ascent, @@ -870,10 +882,13 @@ impl CapturedCursorInfo { active_face_state: &DisplayRowActiveFaceState, placement: CapturedCursorPlacement, ) -> Self { - Self::from_visual_state( - CapturedCursorVisualState::from_active_face_state(active_face_state), - placement, - ) + Self { + vertical_source: CursorVerticalSource::ActiveFace, + ..Self::from_visual_state( + CapturedCursorVisualState::from_active_face_state(active_face_state), + placement, + ) + } } pub(crate) fn display_box_from_active_face_state( @@ -967,8 +982,18 @@ impl CursorGeometrySource { x: cursor.x, y: cursor.y, slot_width: context.slot_width.max(1.0), - face_height: cursor.face_h, - face_ascent: cursor.face_ascent, + face_height: match cursor.vertical_source { + CursorVerticalSource::PointGlyph | CursorVerticalSource::ActiveFace => { + cursor.face_h + } + CursorVerticalSource::SyntheticNewline => row_metric.height(), + }, + face_ascent: match cursor.vertical_source { + CursorVerticalSource::PointGlyph | CursorVerticalSource::ActiveFace => { + cursor.face_ascent + } + CursorVerticalSource::SyntheticNewline => row_metric.ascent(), + }, row_height: row_metric.height(), row_ascent: row_metric.ascent(), default_line_height: context.default_line_height, @@ -1417,10 +1442,10 @@ impl<'a> CapturedTextWindowCursorPublishContext<'a> { pub(crate) fn publish_captured_cursor( self, - cursor: CapturedCursorInfo, + mut cursor: CapturedCursorInfo, row_metrics: &[RowMetricsSnapshot], fallback_row_metric: RowMetricsSnapshot, - output: TextWindowOutputTarget<'_>, + mut output: TextWindowOutputTarget<'_>, output_emitter: &mut WindowOutputEmitter, ) -> CapturedTextWindowCursorPublishOutcome { let row_metric = row_metrics_for_cursor( @@ -1429,6 +1454,23 @@ impl<'a> CapturedTextWindowCursorPublishContext<'a> { fallback_row_metric, ); + // GNU uses the point glyph's actual descent, including a fallback + // font's metrics, rather than the primary face captured before shaping. + // 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) + }) { + cursor.face_h = glyph.pixel_height; + cursor.face_ascent = glyph.pixel_ascent; + } + } + } + // For a cursor at a `display`-property replacement slot, derive its // integer grid x from the already-rounded display point of the glyph // immediately before the slot (`x + width`, both text-area-relative) 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 b1522c97c5..29f44ed5ef 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 @@ -7975,6 +7975,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, x: 0.0, y: 0.0, face_w: 8.0, 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 17b45adef6..91d5c2e1d7 100644 --- a/crates/neomacs-layout-engine/src/engine/tests/engine_test.rs +++ b/crates/neomacs-layout-engine/src/engine/tests/engine_test.rs @@ -689,6 +689,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, 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 0f5c0bc6c5..e953eacb1a 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 @@ -364,6 +364,167 @@ fn newline_cursor_replay_preserves_font_and_spacing_metrics() { } } +/// Mixed-height rows must not change cursor geometry when only point moves. +#[test] +fn mixed_height_newline_cursor_replay_matches_fresh_capture() { + for spacing in [0, 5] { + for newline_height in ["0.1", "1.0"] { + let (mut eval, frame, buffer, _) = incr_editing_frame("AAAx\nnext\n", 800, 240); + realize_test_gui_frame(&mut eval, frame); + eval.eval_str(&format!("(progn (put-text-property 1 4 'face '(:height 2.0)) (put-text-property 5 6 'face '(:height {newline_height})))")).unwrap(); + eval.buffer_manager_mut() + .get_mut(buffer) + .unwrap() + .set_buffer_local("line-spacing", Value::fixnum(spacing)); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + assert!(matches!( + engine.redisplay_frame_attempt(&mut eval, frame), + FrameLayoutAttempt::Prepared(_) + )); + for point in [4, 5, 1, 5] { + eval.eval_str(&format!("(set-window-point (selected-window) {point})")) + .unwrap(); + let FrameLayoutAttempt::Prepared(replay) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("cursor replay should prepare") + }; + assert_eq!(engine.last_layout_stats().cursor_only_windows, 1); + assert!(engine.last_layout_stats().reused_rows > 0); + 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 frame should prepare") + }; + let replay = replay.materialize(); + let rebuilt = rebuilt.materialize(); + let actual = replay.active_cursor().unwrap(); + let expected = rebuilt.active_cursor().unwrap(); + assert_eq!( + (actual.y, actual.height, actual.ascent), + (expected.y, expected.height, expected.ascent), + "spacing={spacing}, newline={newline_height}, point={point}" + ); + } + } + } +} + +/// A scroll retaining point's row must use its point glyph, not the whole row. +#[test] +fn mixed_height_scroll_cursor_matches_fresh_capture() { + for spacing in [0, 5] { + for point_offset in [4, 5] { + let text = "AAAx\n".repeat(80); + let point = 11 * 5 + point_offset; + let (mut eval, frame, buffer, window) = incr_editing_frame(&text, 800, 600); + realize_test_gui_frame(&mut eval, frame); + eval.eval_str("(put-text-property 56 59 'face '(:height 2.0))") + .unwrap(); + eval.buffer_manager_mut() + .get_mut(buffer) + .unwrap() + .set_buffer_local("line-spacing", Value::fixnum(spacing)); + let mut engine = crate::font::metrics::test_catalog::layout_engine(); + crate::font::metrics::test_catalog::enable_metrics(&mut engine); + scroll_window_to(&mut eval, frame, window, buffer, 1, point - 1); + assert!(matches!( + engine.redisplay_frame_attempt(&mut eval, frame), + FrameLayoutAttempt::Prepared(_) + )); + scroll_window_to(&mut eval, frame, window, buffer, 21, point - 1); + let FrameLayoutAttempt::Prepared(replay) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("scroll should prepare") + }; + assert_eq!(engine.last_layout_stats().scroll_windows, 1); + assert!(engine.last_layout_stats().reused_rows > 0); + 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 frame should prepare") + }; + let replay = replay.materialize(); + let rebuilt = rebuilt.materialize(); + let actual = replay.active_cursor().unwrap(); + let expected = rebuilt.active_cursor().unwrap(); + assert_eq!( + (actual.y, actual.height, actual.ascent), + (expected.y, expected.height, expected.ascent), + "spacing={spacing}, point offset={point_offset}" + ); + } + } +} + +/// An unmeasured point glyph must decline replay rather than invent its descent. +#[test] +fn unmeasured_point_glyphs_decline_cursor_replay() { + for stretch in [false, true] { + for scroll in [false, true] { + let (mut eval, frame, buffer, window) = + incr_editing_frame(&"AAAx\n".repeat(80), 800, 600); + realize_test_gui_frame(&mut eval, frame); + eval.eval_str("(put-text-property 56 59 'face '(:height 2.0))") + .unwrap(); + if stretch { + eval.eval_str( + "(put-text-property 56 57 'display '(space :width 1 :height (40) :ascent 75))", + ) + .unwrap(); + } + let mut engine = crate::LayoutEngine::new_without_font_metrics(); + scroll_window_to( + &mut eval, + frame, + window, + buffer, + 1, + if scroll { 58 } else { 0 }, + ); + assert!(matches!( + engine.redisplay_frame_attempt(&mut eval, frame), + FrameLayoutAttempt::Prepared(_) + )); + if scroll { + scroll_window_to(&mut eval, frame, window, buffer, 21, 58); + } else { + eval.eval_str("(set-window-point (selected-window) 59)") + .unwrap(); + } + let FrameLayoutAttempt::Prepared(replayed) = + engine.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("replay should prepare") + }; + assert_eq!(engine.last_layout_stats().full_windows, 1); + assert_eq!(engine.last_layout_stats().scroll_windows, 0); + assert_eq!(engine.last_layout_stats().cursor_only_windows, 0); + let mut fresh = crate::LayoutEngine::new_without_font_metrics(); + let FrameLayoutAttempt::Prepared(rebuilt) = + fresh.redisplay_frame_attempt(&mut eval, frame) + else { + panic!("fresh frame should prepare") + }; + let replayed = replayed.materialize(); + let rebuilt = rebuilt.materialize(); + let actual = replayed.active_cursor().unwrap(); + let expected = rebuilt.active_cursor().unwrap(); + assert_eq!( + (actual.y, actual.height, actual.ascent), + (expected.y, expected.height, expected.ascent), + "scroll={scroll}, stretch={stretch}" + ); + } + } +} + #[test] fn text_cursor_keeps_its_metrics_when_line_spacing_enlarges_the_row() { let (mut eval, frame, buffer, _window) = incr_editing_frame("alpha\nbeta\n", 800, 240);