Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Certify point-geometry reuse crates/neovm-core/src/window/*, crates/neovm-core/src/window/tests/window_test.rs, crates/neovm-core/src/emacs_core/display/xdisp/{mod.rs,tests/xdisp_test.rs} |
Freshness uses the selected window’s live point and records whether point-dependent geometry can be reused. Retained-row reuse now relies on that certification. Tests cover line-number settings, selected-window point changes, dynamic header height, and current-line face geometry. |
Measure live chrome in position queries crates/neomacs-layout-engine/src/buffer_source/*, crates/neomacs-layout-engine/src/engine.rs, crates/neomacs-layout-engine/src/engine/query_cache.rs, crates/neomacs-layout-engine/src/engine/tests/* |
Position queries use a distinct publication mode and measure live chrome. If measurement requires relayout, the query retries with chrome marked measured. Viewport observations cover position queries only when chrome is absent. Tests check measurement, retry behavior, and query results. |
Isolate query-local records crates/neomacs-layout-engine/src/display_status_line.rs, crates/neomacs-layout-engine/src/incremental_layout/chrome_memo.rs, crates/neomacs-layout-engine/src/engine.rs, crates/neomacs-layout-engine/src/engine/tests/*, crates/neovm-core/src/emacs_core/display/xdisp/mod.rs |
Query scopes isolate and restore chrome-generation, fingerprint, column-consumption, and line-evaluation records. Tests check that position queries preserve the enclosing redisplay’s records. |
Verify geometry across engine and GUI crates/neomacs-gui-tests/fixtures/point-query-geometry/init.el, crates/neomacs-gui-tests/tests/{gui.rs,point_query_geometry.rs}, docs/research/point-dependent-position-query-geometry.md |
The GUI fixture records position visibility and coordinates before and after point changes. The integration test compares GNU Emacs and native-engine results. The research document records observations, query rules, and verification notes. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant posn_at_point
participant LayoutEngine
participant QueryCache
participant FrameLayout
posn_at_point->>LayoutEngine: Request a Position query
LayoutEngine->>QueryCache: Check viewport observation coverage
QueryCache-->>LayoutEngine: Do not cover the query when chrome is present
LayoutEngine->>FrameLayout: Measure live chrome
FrameLayout-->>LayoutEngine: Report whether relayout is required
LayoutEngine->>FrameLayout: Retry with chrome marked Measured
Possibly related issues
eval-exec/neomacs#612: The issue reports navigation stalls involvingpos-visible-in-windowandquery_window_layout. This PR changes reuse and measurement in that position-query path, but does not establish the full cause of the stalls.eval-exec/neomacs#607: The issue reports high CPU and memory use during line navigation. This PR changes position-query reuse and measurement for point-dependent geometry, but does not establish the reported memory cause.
Possibly related PRs
- eval-exec/neomacs#616: This PR extends the retained-row reuse and position-query freshness work with point-dependent chrome and line-number geometry.
Suggested reviewers: vritser
Merge Risk: 
145bb
-
No merge-blocking issue was identified; the reviewed point-geometry behavior is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly identifies the main change: remeasuring point-dependent position geometry. Description check The description directly explains the geometry issue, implementation, regression coverage, and validation results. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 20 functions across 16 files. (3 skipped: 2 unsupported, 1 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
🔵 Needs a closer look
A subtle redisplay/geometry parity change on a performance-sensitive hot path tied to an unverified macOS spike, with CI not serving as a completion gate, calls for hands-on human validation of the GUI and performance behavior.
No open issues 🎉
What changed in this PR
This PR corrects point-dependent display geometry for posn-at-point / pos-visible-in-window-p queries issued after point moves but before redisplay. PR #616's point-only shortcut could reuse rows whose geometry actually depends on point: window chrome (mode/header/tab line) whose :eval/propertized/%-substituted content changes height, and active line numbering where the line-number-current-line face shifts gutter width and row height. The fix introduces a canonical PointGeometryReuse certification carried by the freshness token, captures the selected window's live buffer point before its marker syncs (matching GNU window_point), and routes dynamic/structured chrome and any active numbering through canonical live-chrome measurement while retaining the fast path only for literal, property-free chrome.
Changes:
- Add a closed
PointGeometryReuseenum andWindowChromePresence::{is_empty, point_geometry_reuse}; gate point-only row reuse on it and on the selected-window live point, replacing the removedline_numbers_are_point_independenthelper. - Distinguish
window-endfrom position queries in the layout engine: position queries measure live chrome once via a newPositionQueryChrome::{Measure, Measured}one-use retry, isolate thread-local chrome bookkeeping with RAII guards, and never publish start/end markers or replace the accepted snapshot. - Restrict viewport→position cache reuse to chrome-absent windows; add core, layout-engine, and live GUI regression coverage plus a research write-up.
| File | Description |
|---|---|
crates/neovm-core/src/window/chrome.rs |
New PointGeometryReuse enum; is_empty and point_geometry_reuse classifying literal vs. dynamic chrome. |
crates/neovm-core/src/window/display.rs |
Projects selected-window live point; classifies point-geometry reuse (numbering + chrome). |
crates/neovm-core/src/window/mod.rs |
Freshness carries point_geometry_reuse; comparison blocks reuse unless both tokens certify independence. |
crates/neovm-core/src/emacs_core/display/xdisp/mod.rs |
Adds ColumnSpecConsumptionScope; removes line_numbers_are_point_independent, deferring to the canonical token. |
crates/neomacs-layout-engine/src/engine.rs |
Adds SynchronousPositionQuery/one-use chrome retry plumbing and chrome-record isolation for position queries. |
crates/neomacs-layout-engine/src/buffer_source/render_attempt.rs |
New publication variant + preserves_reserved_chrome split from is_synchronous_query. |
crates/neomacs-layout-engine/src/buffer_source/render_plan.rs |
Chooses recompute vs. preserve chrome via preserves_reserved_chrome. |
crates/neomacs-layout-engine/src/display_status_line.rs |
ChromeQueryRecordScope RAII isolating generation/eval-count/fingerprint/column records. |
crates/neomacs-layout-engine/src/incremental_layout/chrome_memo.rs |
ChromeFingerprintRecordScope RAII for fingerprint records. |
crates/neomacs-layout-engine/src/engine/query_cache.rs |
Gates viewport→position cache reuse on chrome absence. |
crates/neomacs-layout-engine/src/engine/tests/* |
New engine tests for remeasurement, non-publication, and record preservation; MemoGuard exposed. |
crates/neovm-core/src/.../tests/*_test.rs |
Core tests for live selected-point and default/local numbering reuse. |
crates/neomacs-gui-tests/{tests/point_query_geometry.rs,tests/gui.rs,fixtures/.../init.el} |
Live GNU vs. Neomacs GUI regression for point-dependent chrome/gutter. |
docs/research/point-dependent-position-query-geometry.md |
Records GNU references, root cause, and reuse policy. |
No blocking code defects were found within the changed regions. The implementation is internally consistent, exported symbols resolve across crates, removed/renamed symbols have no stale references, and the new behavior is well covered by core, layout-engine, and GUI tests. Given the subtlety of the redisplay-geometry parity change, its performance sensitivity (relation to the unverified #607 macOS spike), and that CI was explicitly not used as a completion gate, final sign-off is best confirmed by a human.
Review effort: Balanced
github-actions
Bot
added
the
merge-conflict
Moving point before redisplay can change mode/header/tab-line heights and which row receives the taller
line-number-current-lineface. PR #616's point-only shortcut could return a hidden target or coordinates measured with the previous current-line face.Make the canonical freshness token carry a closed
PointGeometryReuseenum, and capture the selected window's live buffer point before its saved marker is synchronized. Literal, property-free chrome without dynamic substitutions retains the shortcut; dynamic/structured chrome and active line numbering use canonical targeted measurement.The real GUI regression also exposed a producer gap: position queries shared
window-end's accepted-chrome policy. Distinct enum variants now make position queries measure live chrome with the existing renderer. One-use native retry state avoids duplicate observable:evalcalls during height convergence. Queries preserve accepted redisplay snapshots and isolate native chrome bookkeeping with thread-confined RAII guards. Cache reuse across viewport/position scopes remains available only when live chrome is absent; active-chrome position results cannot become scalar-Lisp-blind cache entries.This follows the two explicitly deferred review comments in #616:
GNU source references, live probes, root causes and reuse policy are recorded in
docs/research/point-dependent-position-query-geometry.md. Related to #607; its native macOS memory spike remains unverified.Validation (all final test runs used
cargo nextest):window-endcache reuse and enclosing chrome bookkeeping.[true,false]for all three chrome cases; current-line Y decreased from 38 to 20 in GNU and from 35 to 18 in Neomacs.quality_mode=Full, reached line 261 and recorded 317 scene commits, 494 root glyph passes and 1,099 composite-only frames in the final periodic sample. Startup readback was visually checked.The first release failed the new live GUI regression and led to the selected-point and producer fixes above; the rebuilt final source passes it. All GUI processes ran on private displays. CI was not used as a completion gate, per the requested workflow.