feat: add row-level diff rendering #179
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 88.00% 89.03% +1.03%
==========================================
Files 35 35
Lines 5662 6340 +678
Branches 5662 6340 +678
==========================================
+ Hits 4983 5645 +662
+ Misses 569 566 -3
- Partials 110 129 +19
🚀 New features to boost your workflow:
|
85f56e3 to
322e793
Compare
This reduces TUI flicker by only re-rendering rows that changed. Also bumps crossterm to 0.29.0 to match iocraft's dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
We're releasing this part of devenv 2.0.6 and will let you know what folks say. |
|
Thanks for the PR! It looks great at a glance, but I'll do my best to review this fully in the next few days. |
|
Neat stuff! - it was pretty much exactly what I needed for a project that I'm working on. I gave it some testing and found some issues with inline mode:
I've put fixes for these at https://github.com/owtaylor/iocraft/tree/row-level-diff-fixes - feel free to add them to your PR if they look useful. |
|
Thanks @owtaylor I've pulled your fixes in! |
|
Note that I've reverted this change in devenv for now as it doesn't fully work yet. I'll test the bug fixes in following days. |
|
looking forward to this! |
5e6ca04 to
df07325
Compare
|
I've rebased on main |
|
Hi! Effect of the bug on this example is that moving mouse across the window corrupts the data of the footer view (instead of footer data, the content of the row below the mouse is displayed there) On main, everything is shown correctly, but there is a very visible flicker while moving a mouse (at least on my terminal: konsole 25.04.2). edit: |
Good work at figuring this out! I didn't try out fullscreen operation at all. (*) I'm wondering if the actual explanation is just slightly different - that calling EnterAlternateScreen does not move the cursor is at 0,0 and just leaves the cursor where it was on the main screen. I checked the implementation for vte, and also avt which is used in tests. Beyond the comment, I think the flushes are now unnecessary and should be removed (not because they harm anything, but because they don't make any sense any more.) (*) I was first wondering "if this is the explanation, then why isn't fullscreen completely broken" - but trying out examples/fullscreen it is exactly as broken as I would expect with the original patch. One note, if someone is trying to find docs for the alternate screen behavior, the exact sequence used by crossterm for EnterAlternateScreen is https://terminalguide.namepad.de/mode/p1049/ - "Alternate Screen Buffer, With Cursor Save and Clear on Enter". |
|
Thanks, you are right, explanation in comment is wrong. If I understand EnterAlternateScreen correctly, it does move the cursor to home, but it is called only once (when entering alternate screen mode), and this code is called every time canvas is cleared (may be multiple times). Not resetting here explicitly causes artifacts when changing height in fullscreen mode. Removed in my branch. I also removed flush calls, and also did some manual tests to see if it would break something for me - it didn't.
|
|
@ccbrown let me know what you'd like me to do here :) |
No need to do anything. :) I was just waiting a bit to let everyone here identify any other bugs. I'll try to find time this weekend to do a final round of review and testing, then merge it if it looks good. Thanks! |
ccbrown
left a comment
There was a problem hiding this comment.
Thanks for the PR! This is great.
A few minor comments. Let me know what you think, but I don't think it would be a premature optimization to go ahead and eliminate where possible the new resets that this adds.
| } | ||
|
|
||
| pub(crate) fn row_eq(&self, other: &Self, y: usize) -> bool { | ||
| self.width == other.width && self.row(y) == other.row(y) |
There was a problem hiding this comment.
Is the width check necessary here? I would think if the width differed, but the rows had the same number of non-empty cells, this should return true. Is there a case I'm not thinking of?
There was a problem hiding this comment.
IIRC I added this because I was seeing garbage when resizing the terminal.
Extract per-row rendering from Canvas so individual rows can be written and compared independently. The terminal now diffs against the previous canvas and only re-renders rows that changed, reducing flicker and redundant output in both fullscreen and inline modes. In fullscreen mode, absolute cursor positioning targets only changed rows. In inline mode, relative cursor movement achieves the same. The clear_canvas path in fullscreen mode now preserves output above the canvas area. Co-Authored-By: Samuel Corsi-House <sam@chouse.dev> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When the canvas grows beyond its previous height, MoveToNextLine (CSI E) was used to reach the new rows. CSI E only repositions within existing terminal content — it won't create new lines when the cursor is at the bottom of the screen. Use \r\n instead for rows beyond the previous canvas height to actually extend the scrollback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instead of unconditionally clearing when the canvas height >= terminal height, check each changed row during the diff. Only fall back to a full rewrite if a changed row is above the visible area (off-screen). When only visible rows changed, the normal row-level diff handles it without any flicker. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In fullscreen (alternate screen) mode, EnterAlternateScreen guarantees the cursor starts at (0, 0). The previous code called cursor::position() to determine prev_canvas_top_row, but inside BeginSynchronizedUpdate some terminals return a stale cursor position from the main screen. The wrong prev_canvas_top_row causes all subsequent row-level diffs to use incorrect absolute positions — every MoveTo(0, top_row + y) is offset by the stale value, so changed rows get written to wrong terminal positions, corrupting the visible display. Fix: set prev_canvas_top_row = 0 unconditionally for fullscreen mode and queue an explicit MoveTo(0, 0) as a safety measure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add three tests for the fullscreen prev_canvas_top_row behavior: - test_fullscreen_initial_write_sets_zero_top_row: Exercises the full initial-write → diff pipeline. Verifies that write_canvas(None, …) anchors prev_canvas_top_row at 0. Without the fix, cursor::position() is called and fails in non-TTY test environments (timeout), so this test reliably catches the regression. - test_fullscreen_diff_zero_top_row_renders_correctly: Verifies that with prev_canvas_top_row = 0, a single-cell diff (simulating mouse overlay) writes each changed row to its correct terminal position. - test_fullscreen_diff_nonzero_top_row_offsets_changed_rows: Demonstrates the root cause: with a non-zero prev_canvas_top_row, every changed row Y is written to terminal line (top_row + Y) instead of line Y. Unchanged rows are skipped by row_eq, so the corruption is never self-correcting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove unnecessary flushes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
write_row_impl previously emitted csi!("0m") at both the start and end
of every row. Each row's trailing reset already leaves the writer in
a clean SGR state, so the leading reset of the next row is redundant.
Document the contract on write_ansi_row_without_newline (callers must
ensure clean SGR state; function leaves clean SGR state) and seed the
state once in write_impl for the public canvas writers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
What It Does
Extract per-row rendering from Canvas so individual rows can be written and compared independently. The terminal now diffs against the previous canvas and only re-renders rows that changed, reducing flicker and redundant output in both fullscreen and inline modes.
In fullscreen mode, absolute cursor positioning targets only changed rows. In inline mode, relative cursor movement achieves the same. The clear_canvas path in fullscreen mode now preserves output above the canvas area.
I'd be happy to add more tests for regressions since it's a substantial change in the core logic of rendering.
Related Issues
#117