Fix #262 - #273
Conversation
vyncint
left a comment
There was a problem hiding this comment.
Welcome, and thank you for taking this one on — it is one of the harder issues in the queue, and the part you have written is genuinely good.
Before the change requests, so you know where you stand: your work is correct. I completed the one missing piece locally and ran it. All ten of your tests pass, the full suite is green, clippy is clean in all four feature configurations, MSRV 1.85 builds, and the docs build with warnings denied. The TabStops design, the TabOp split between tracker and emulator, the strictly-left back-tab, the resize rule, the clamping — all of it is right, and the comments explain why in the way this repository asks for. Your CHANGELOG entry is in [Unreleased], which four other open PRs got wrong today.
So this is not "start over". It is "one file did not make it, and the way you uploaded it needs to change".
1. The blocker: crates/termlens/src/emu/vt100.rs is not in the PR
Your PR changes five files. The sixth one — the emulator half your docs/DESIGN.md paragraph describes in detail — is missing, so the crate does not compile:
error[E0061]: this function takes 2 arguments but 1 argument was supplied
--> crates/termlens/src/emu/vt100.rs:72:22
|
72 | tracker: SeqTracker::new(capture),
| ^^^^^^^^^^^^^^^--------- argument #2 of type `u16` is missing
error[E0004]: non-exhaustive patterns: `SeqEvent::Tabs(_)` not covered
--> crates/termlens/src/emu/vt100.rs:230:30
That single failure is why clippy, test, features, msrv and skill are all red. They are not five problems; they are one. (skill compiles the agent guide's examples against the crate, so it fails whenever the crate does.)
If you still have your local vt100.rs, include it and skip to §2 — but please check the ordering note at the end of this section first, because it is the one place this is easy to get subtly wrong. If you need to rebuild it, it is three edits.
Edit 1 — the constructor, around line 72. The tab-stop set is one flag per column, so it needs the width:
tracker: SeqTracker::new(capture, cols),Edit 2 — set_size, around line 413. The set has to follow a resize:
fn set_size(&mut self, rows: u16, cols: u16) {
self.parser.screen_mut().set_size(rows, cols);
self.shadow.set_size(rows, cols);
// The tab-stop set is one flag per column, so it follows the width.
self.tracker.set_cols(cols);
// A resize can push rows into history on its own.
self.capture_scrolled_rows();
}Edit 3 — a new arm in the match self.tracker.step(byte) inside process, next to the SeqEvent::SoftReset arm:
SeqEvent::Tabs(op) => {
// Everything *before* this byte, so the column read is the one the
// operation is relative to. It matters for plain HT alone: vt100
// acts on it and would have moved the cursor to its own fixed
// eight before we looked.
self.feed_staged(&bytes[fed..i]);
let (_, col) = self.parser.screen().cursor_position();
let target = self.tracker.tab_op(op, col);
self.feed_staged(&bytes[i..=i]);
fed = i + 1;
if let Some(target) = target {
self.feed(format!("\x1b[{}G", target.saturating_add(1)).as_bytes());
}
None
}The ordering note, which is worth the paragraph. My first attempt fed the operation's own byte before reading the cursor column — the obvious reading of "feed what you owe the parser first" — and seven of your ten tests failed. The reason is the case your own DESIGN paragraph singles out: vt100 ignores HTS, TBC, CHT and CBT, but it does act on a plain HT, so by the time that byte has been fed the cursor has already jumped to vt100's fixed eight and the column you read is the wrong base. Reading before feeding is correct for all five operations, because no escape prefix moves the cursor either. Splitting the feed at i rather than i + 1 is the whole fix, and it is invisible until you run the tests.
That is also the best evidence that your tests are doing their job: they caught a real ordering bug in an implementation that compiled fine.
2. cargo fmt — one spot
crates/termlens/src/emu/seq.rs:1151. rustfmt wants this broken across lines:
let ps = if params_empty { 0 } else { self.csi_first_param };Do not hand-edit it — run cargo fmt --all and it is done. That is the only formatting complaint in the whole PR.
3. Sign-off, commit messages, and the upload workflow
Your four commits are all titled Add files via upload and none carries a Signed-off-by line, so commit-policy fails. Both come from the same cause: GitHub's web upload button cannot sign off a commit and names them for you. It is also almost certainly why vt100.rs was left behind — five files is one too many to shepherd through that dialog.
The fix is to work from a real clone, which §4 walks through. What CI needs at the end:
- One commit, signed off with
git commit -s. That appendsSigned-off-by: Your Name <your@email>and is how you certify the DCO — see CONTRIBUTING §5. The sign-off email must match the commit author email. - A Conventional Commit subject, imperative and under 72 characters. For this change:
fix(emu): honour the tab stops an application sets. - A PR title in the same form. This repository squash-merges, so the PR title becomes the commit subject on
main.Fix #262would land there as-is.
4. Step by step, from where you are
Everything you have already written is recoverable — it is sitting on main in your fork. This pulls it onto a proper branch, adds the missing file, and updates this same PR.
# 1. A real clone (the web UI cannot sign a commit).
git clone https://github.com/bernalalexis-try/termlens
cd termlens
git remote add upstream https://github.com/vyncint/termlens
git fetch upstream
# 2. A branch off current upstream main. Working on your fork's `main`
# is what left you with no way to keep this PR separate from the next.
git switch -c fix/262-tab-stops upstream/main
# 3. Recover the five files you already uploaded.
git checkout origin/main -- CHANGELOG.md README.md docs/DESIGN.md \
crates/termlens/src/emu/seq.rs crates/termlens/tests/tabs.rs
# 4. Make the three edits from §1 to crates/termlens/src/emu/vt100.rs.
# 5. Format, then run what CI runs.
cargo fmt --all
cargo test --workspace --all-features
cargo clippy --workspace --all-targets --all-features -- -D warnings
# 6. One signed-off commit.
git add -A
git commit -s -m "fix(emu): honour the tab stops an application sets"
# 7. Check the two policy gates before pushing — they take a range.
.github/scripts/check-dco.sh upstream/main..HEAD
.github/scripts/check-no-ai-attribution.sh upstream/main..HEAD
# 8. Update this PR in place.
git push --force-with-lease origin fix/262-tab-stops:mainStep 8 pushes your branch onto your fork's main, which is what this PR tracks, so #273 updates rather than a new one opening. If you would rather start a clean PR, push to fix/262-tab-stops instead and open one from that branch; close this one and say so, and nothing is lost.
Then edit the PR title to fix(emu): honour the tab stops an application sets, and rewrite the description to say what the PR does rather than what you would do — the current text is in the conditional ("I'd add a TabStops struct…"), which reads as a plan rather than a change. Closes #262 in the body links them so the issue closes on merge.
The full gate list is in CONTRIBUTING §1. Everything there is reproducible locally, which is the point of writing it out.
5. Two small things for when the rest is green
docs/DESIGN.md. Your paragraph says the emulator resolves aTabOp"feeding the bytes it still owes the parser first, so the position it reads is current". After §1 you know that is precise only if "owes" excludes the operation's own byte. One clause naming plainHTas the reason would save the next reader the hour it cost me.tests/tabs.rs. Ten cases, and the one gap is the resize rule you documented in three places. I checked it by hand and your implementation is right — a stop set at column 3, then a widen from 24 to 40 columns, keeps the custom stop and gives the new columns the every-eighth pattern. It deserves a test, because it is the rule most likely to be broken by a later change and the only one nothing currently pins.
None of this is a reflection on the work. The tracker, the tests and the docs are the hard three-quarters and you got them right; what is left is one file and the mechanics of getting it here. Push when you are ready and I will re-review.
I'd add a TabStops struct in emu/seq.rs holding one flag per column plus a TabOp, handling HT, HTS, TBC, CHT and CBT, with RIS/DECSTR restoring the every-eight default and set_cols on resize.
In vt100.rs, apply_tabs would read the cursor column and rewrite movement as CSI n G, covered by a new tests/tabs.rs.