Add opt-in vim key bindings

[rodgco]

Adds vim key bindings behind a toggle, for writers who reach for hjkl out of habit.

Off by default: normal mode has to swallow every printable key, so it would be a surprise for anyone who didn't ask for it. Ctrl+Alt+V toggles it, as does the diamond in the footer, and the choice is remembered in QSettings. The mode shows in the bottom-left corner and normal mode draws the caret as a block.

Insert mode consumes nothing but Escape. Smart returns, list continuation and Markdown paste behave exactly as they do with the mode off — only normal and visual mode go through the engine.

What's supported

• Modes: i I a A o O, v / V, Esc or Ctrl+[
• Motions: h j k l, w W b B e E ge gE, 0 ^ $, gg G, { }, ( ), f F t T with ; ,
• Text objects: iw aw ip ap, i" a" i' a' and the bracket pairs, under an operator or from visual mode
• Operators: d c y with any motion, doubled for whole lines, plus D C Y S s x X r J ~ p P
• Registers: "a–"z hold text aside, "+ is the system clipboard and "* the primary selection. Yanks and deletes land in the unnamed register, which stays inside the editor, so an x never clobbers what you copied from a browser.
• Counts throughout: 3j, d2w, 2dd
• u / Ctrl+R / .
• / opens the existing find bar, n / N step matches, Ctrl+D / Ctrl+U page

Every Ctrl shortcut keeps working in either mode.

The : command line

: opens a command line along the bottom edge; Enter runs it, Esc or backspacing past the start abandons it.

:w :w <path> :wq :x :q :q!	write / quit, ! discards
:e <path> :e!	open a file / reload from disk
:42 :$	jump to a line
:s/pat/rep/[gi]	substitute, with % 3 2,5 '<,'> ranges
:d	delete the range's lines into the register p uses
:noh	clear the search highlight

The usual abbreviations resolve (:wr, :qa, :substitute, :nohlsearch). Pressing : in visual mode prefills '<,'>.

Write, quit and open reuse the paths the app already takes: :w on an unsaved document opens the portal picker, :q on a modified one raises the same unsaved-changes dialog as the close button, :wq goes through Backend::saveForClose. Paths are read the way a shell reads them, through the new Backend::resolvePath — ~ is home, a relative name is a sibling of the open document.

One deliberate divergence: substitute patterns are JavaScript regular expressions, not vim's, since that is what the engine can offer honestly. Replacements keep vim's spelling (&, \1), and any punctuation can stand in for the separator.

Notes on the implementation

src/Vim.js holds the state machine behind handleKey() and touches the editor only through a host wrapper, so the tests drive a bare TextEdit while Main.qml supplies the application hooks. Three things worth a second look:

• Undo: every command, :s and :d included, groups its document changes into one edit block through the new Backend::beginEditBlock, so u undoes the command rather than the edits that carried it out.
• Dot repeat: . replays what an insert session did to the document rather than the keys that did it. Replaying keys would go wrong exactly where this editor is interesting — list continuation rewrites what a Return would otherwise have typed.
• Hidden markers: motions route through the existing skipHiddenForward / skipHiddenBackward, without which the caret could rest on a zero-width ** and look stuck.
• Deferring to the editor: o and O go through the editor's own smartReturn, and a visual p through its link paste, so a bullet carries down and a URL pasted over a selection becomes a Markdown link exactly as they do with the mode off. Insertion and paste get Omawrite's Markdown behaviour; motions and operator ranges stay mechanical, because vim's contract is that you can predict them.
• Wrapped lines: j and k follow the display line, gj / gk the logical one — the mirror of vim, where g is the display-line prefix. In Markdown a paragraph is one long line, so plain j jumping the whole of it means the key you press most does not move the way the text reads. Operators stay logical: dj takes two whole paragraphs.

Consolidated with #10: @ryanyogan's commit is merged in rather than paraphrased, and every commit carrying ported code credits them. Text objects, the sentence motions, ge / gE, surrogate-safe stepping, the footer toggle and its icon came from there.

Still out of scope: macros, marks, % matching.

Testing

tests/tst_omawrite.cpp covers motions, operators, counts, text objects under an operator and from visual mode, registers including a "+ and "* round trip, astral characters through every motion that feeds an operator, dot repeat, undo, ex ranges, substitute flags and error messages, and hook dispatch for every file command.

The engine's fast tests drive a bare TextEdit, which has no edit blocks — so they run it through a proxy whose text freezes while a block is open while its live length stays honest, which is what QTextDocument actually does. Without that stand-in the harness reports that the integration is fine because it has replaced the thing being integrated with. Several more drive a real window with key events: normal-mode routing and the fall-through when vim mode is off, the command line, o and O across bullets, numbers and quotes, and the link paste from both the clipboard and a named register.

Full suite: 26 passed, 0 failed, 1 skipped — the skip is the primary-selection half of the clipboard test, which the offscreen platform has none of.

Three bugs the tests caught while writing them: cw was eating the trailing space (vim makes it behave like ce), a lone 0 after a count parsed as a digit instead of the line-start motion, and the mode indicator did not clear when vim mode was switched off from outside the shortcut.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QvoQB2xpX5Gkp8LtXVBPvw

[rodgco]

Consolidated with @ryanyogan's #10

@ryanyogan opened #10 with a second vim mode and offered to consolidate if one approach suited the app better. Each was stronger in a different place, so rather than pick a winner this branch takes their commit and ports the parts of their engine that were better than mine.

Their commit ef9a3a6 is merged into this branch rather than paraphrased, so the work is theirs in the history, and every commit carrying ported code credits them as co-author.

What came from #10

• Text objects — iw aw ip ap i" a" i' a' and the bracket pairs. This was the biggest gap Simple vim key movements to make writing a delight #10 exposed: prose editing reaches for ciw and dap more than anything else I had. Rewritten against my character classes, which number them the other way round.
• Sentence motions ( ), and ge / gE.
• Surrogate-pair-safe stepping, so x on an emoji removes the whole character instead of half of one.
• The t / ; quirk — a repeated t was re-finding the target it had already stopped short of, and standing still.
• Display-line j / k, see below.
• The footer diamond toggle and its icon, unchanged from their commit.
• Backend::setClipboardText, which the registers build on.
• Their test suite's edge cases, which found three real bugs in mine.

Decisions

j and k follow the wrapped line; gj and gk reach the logical one. The mirror of vim, where g is the display-line prefix. In a Markdown document a paragraph is one long line, so plain j was jumping the whole of it — the key you press most not moving the way the text reads. Operators stay logical: dj still takes two whole paragraphs, since vim's display-line operator is dgj.

Finding the neighbouring line needs their probe loop rather than a single positionAt. The document is set in 140% line spacing, and the leading between lines is dead space where positionAt resolves a column badly — which is why my one-shot version drifted.

Registers answer the clipboard question vim's way. This was the one real disagreement between the two branches: I kept an internal register, #10 made every yank the system clipboard. Both have a cost. Mine can't carry text between applications; theirs means every x and dd clobbers whatever you copied from a browser.

So " now names a register. Yanks and deletes still land in the unnamed register, which stays inside the editor. "a–"z hold text aside. "+ is the system clipboard and "* the primary selection, for when carrying text out of the window is what you actually meant. A named yank fills the unnamed register too, so a bare p still pastes what you last took. A clipboard cannot carry the linewise flag, so a trailing newline stands in for it — their convention, kept, since there is no other way to do it.

Both ways to toggle. The footer diamond from #10, and Ctrl+Alt+V. The diamond is discoverable without reading anything; the chord is faster once you know it.

What this branch kept

The host-adapter engine and string key names, so the grammar never sees a Qt enum. The ex command language — :s with ranges, :e, :d, :noh, :w <path>. beginEditBlock / endEditBlock, so one command is exactly one undo, including a :%s across forty lines. The settle hook into skipHiddenForward / skipHiddenBackward, so motions don't appear to stall on zero-width Markdown markers. Opening a document on its first line. And the insertDelta dot repeat, which diffs the insert session rather than replaying keystrokes, so list continuation and Markdown paste replay correctly.

Dropped as redundant: #10's Backend::replaceRange, since EditorMutations.replaceRange inside the edit blocks already groups a compound change into one undo, and src/VimEngine.js itself once its tests had moved across.

Bugs #10's tests found

Porting those edge cases caught three things this branch had wrong, each fixed alongside the assertion that caught it:

• dw on a line's last word dragged the line below up. I was missing half of :h exclusive: an exclusive motion landing in column one stops at the end of the line before it, and turns linewise from at or before the first word.
• dj on the last line deleted the line the caret was on, instead of failing the way a motion with nowhere to go should.
• J after a line that already ended in a space added a second one.

Tests

23 passing, 1 skipped — the skip is the primary-selection half of the clipboard test, which the offscreen platform has no primary selection for; the clipboard half runs. New coverage for text objects under operators and from visual mode, sentence motions, ge, registers including a "+ and "* round trip, the astral cases, the t / ; repeat, and a windowed test that wraps a real paragraph to prove j stays inside it while dj still takes whole lines.

[rodgco]

Vim mode defers to Omawrite, rather than replacing it

Testing the consolidated branch turned up something worth fixing properly: pasting a URL over selected text makes a Markdown link in Omawrite, but under vim mode p just pasted the URL. Same for o on a list item — Return continues the bullet, o gave you an empty line.

Both are the same mistake, and it is the mistake a vim mode is most likely to make: reimplementing the editor instead of driving it. Omawrite has already decided what Return means on a list line, what pasting a URL over a selection means, and what a single undo should cover. A vim mode that quietly disagrees with any of that is a second editor sharing a window with the first, and the writer is the one who has to keep track of which one they are talking to.

So the engine defers. o and O go through the editor's own smartReturn, and a visual p through its link paste. That is what the host adapter in this branch is for, and two motions already used it — settle reuses skipHiddenForward/skipHiddenBackward so the caret doesn't stall on zero-width Markdown markers, and page reuses movePage. This is the same seam, used twice more, not a new mechanism.

Where the line falls: insertion and paste get Omawrite's Markdown behaviour, because that is what they are for and the writer already knows how they behave. Motions and operator ranges stay mechanical — dw takes a word, and no cleverness gets to reinterpret it. Vim's contract is that you can predict the mechanics; the app's contract is that Markdown structure looks after itself. Those meet at the edit, not at the motion.

Two details on the paste. It follows the register rather than the clipboard, now that " names one, so "+p from a browser and "ap yanked out of the document both wrap the selection — Backend::normalizedLinkUrl() already worked on any string. And P stays the literal paste, so there is still a way to say you meant the text itself.

Three bugs this surfaced

Wiring it up ran into three things, none of them belonging to the feature.

The first is mine, from resolving the merge in 7a69964. The onTextChanged handler came over cleanly from #10 and still referenced the vim object I removed alongside it, so it threw on every text change while vim mode was on. A throw there aborts the handler, so backend.editorTextChanged() never ran: no modified flag, no word count, no search refresh, for as long as vim mode was on. My fault, and a good argument for the integration tests that caught it.

An open edit block leaves TextEdit.text behind. The document holds its change signals until the block closes, and the editor's text property only refreshes when one arrives — so a command reading the text after its own edit read the version from before it. 3J joined one line and stopped. The caret clamped against a document shorter than the real one and was dragged back to where the edit began. The host now reads the document itself while a block is open.

Worth saying plainly: the bare TextEdit the engine tests drive has no edit blocks, so none of this was visible there. Every one of these assertions runs in a real window instead. A unit harness that removes the thing you are integrating with will tell you the integration is fine.

Closing an edit block makes the document announce itself whether or not anything changed, so every keystroke in vim mode reaches onTextChanged. Anything hanging off it has to ask whether the text actually moved — my first attempt at resetting stale visual anchors fired on all of them and dropped visual mode on the following key.

Tests

24 passing, 1 skipped — the skip is the primary-selection half of the clipboard test, which the offscreen platform has none of. New coverage for o and O across bullets, numbers, quotes and plain paragraphs; the link paste from both the clipboard and a named register, with P and a non-URL payload as controls; and, for the edit-block bugs, a multi-edit 3J, a yyp, and the caret landing where the command meant to leave it.

[rodgco]

A review pass, and the bug that was hiding behind a test

I ran a review over the branch. It found four things worth fixing, three of them mine from the last two commits, and one of them serious enough that it should have blocked the merge.

The document-corrupting one

Every vim command runs inside one of the document's edit blocks, so that a single u undoes the command rather than the several edits that carried it out. Inside a block, QTextDocument holds its change signals back — and TextEdit.text only refreshes when one arrives. So partway through a command, the editor's copy of the text is the version from before the command started.

The previous commit taught host.text(), setCursor and select to ask the document instead. It missed EditorMutations.replaceRange, which clamps its range against editor.text.length. Any edit landing past where the document ended when the command began was quietly dragged back inside the old length.

On one\n\ntwo: o, some text, Esc, then . at the end of the document gave

one\n\ntwotwo\n\n

Two paragraphs run together and a stray break at the end — silent corruption of the writer's document, from a keystroke as ordinary as ..

The first fix I wrote added an opt-in length argument and passed it from the engine. That was too narrow. replaceSelectionWith, smartReturn's empty-list branch and openLineForVim all call replaceRange without one, and all three now run inside an engine edit block, reached through the openLine and linkPaste hooks the previous commit added. They are safe today only because each happens to do a single edit inside the old text — luck, not design. So replaceRange now asks the editor for a live length whenever it can offer one, which closes the whole class rather than the engine's corner of it. An editor that cannot answer was never in a block, and falls back to what it did before.

The other three

• "*p read the clipboard, not the primary selection. clipboardText gained a mode argument when the registers landed; clipboardUrl did not. The linkPaste hook now carries the register name rather than a bool, so "+ and "* each reach the one they name.
• A V-LINE p handed its raw anchors to the link paste. A linewise range carries anchors, not whole lines, so wrapping one would have taken part of the selection. Both ends have to be charwise.
• Leaving the search bar or the : line emptied every register. It replaced the whole vim state, which also discarded the last change, the last search and the last substitute. Yanking a paragraph and then going to find where it belongs is the reason you would go. Returning now clears the mode and any half-typed command, and nothing else. The review caught closeSearch; closeCommandLine had the same bug and both are fixed.

Why none of this was caught

This is the part worth keeping.

The engine's fast tests drive a bare TextEdit with no beginChange/endChange hooks — no edit block, so its text is always current. Every one of these bugs lives in the gap between the engine and the application, and the harness had removed the very thing being integrated with. It reported that the integration was fine because it had quietly replaced it with something simpler.

The harness now runs the engine through a proxy whose text freezes while a block is open, while its live length stays honest — a stand-in for what QTextDocument actually does. I checked it earns its keep by reverting each fix in turn: the corruption reproduces in the fast tests as "ab\nXX\n", and the register fix fails its own test. Neither needed a window to catch.

25 passing, 1 skipped — the skip is the primary-selection half of the clipboard test, which the offscreen platform has none of.

Still open

Three smaller findings, verified but not yet fixed. None lose work, so I would rather they were their own commit than padding this one:

• Multi-line visual r replaces only the first line, and counts UTF-16 units where it should count characters.
• :s leaves the caret on the first changed line rather than the last, because the substitution loop runs bottom-up and keeps overwriting the line it recorded.
• C-[ never reaches the engine: the key-name mapping only emits C-<letter> for Key_A..Key_Z, so that Escape alias is dead.

[omarchybot]

Reviewed this branch. The headline first: the opt-in holds up. I did not want to take that on reading alone, so I built master's src/Main.qml and this branch's side by side and drove both through the same 21-step key script with vimMode false, comparing text, cursor and selection after every step — list continuation on -, *, >, 1. and 3), the empty-item branch, a code fence, plain Return, Shift+Return, plain typing, Escape, Ctrl+B, Ctrl+I, Ctrl+F/Escape/type, editor focus, Backspace, Left, Delete, Up, Home, Ctrl+V paste, and the closing word count and modified flag. Identical at all 21. Flipping the harness to enable vim mode on this side made it fail at 16 of the 21, so the comparison is not vacuous. smartReturn through continuationMarker() is textually equivalent to master's inline branch, and nothing installs an always-live handler that no-ops when the mode is off. ./bin/test gives 25 passed, 1 skipped, and ./bin/build is clean.

With the mode on, four things.

Astral characters still come apart in two places. x and r are safe, and the test at tests/tst_omawrite.cpp:207 covers h l x r — but the visual range and the word-end motions still step by UTF-16 code unit. Reproduced against this branch:

a😀b  lvd  ->  0061 DE00 0062     (a + lone low surrogate + b)
a😀b  x    ->  0061 0062          (correct, for contrast)
a😀 b le   ->  caret lands at offset 2, inside the pair
a😀 b lex  ->  0061 D83D 0020 0062  (a + lone high surrogate + space + b)
a😀 b gex  ->  same

showSelection and selectionRange (src/Vim.js:561, :563, :573) extend by head + 1; wordEnd (:275) and wordEndBackward (:287) walk by i++. Since e and ge feed d, c and y, the corruption reaches operators as well as x. An unpaired surrogate then goes to disk on the next save. I did not push a fix because there are two reasonable shapes for it — step by character in each of those four functions, or normalise to a character boundary once in moveCaret/setCursor — and which one you want is yours to pick, with your own motion tests riding on it.

A visual link paste does not record itself for . — src/Vim.js:1497. The branch returns as soon as host.linkPaste succeeds, so it never reaches commitChange(state), while the plain-paste path below it does through applyOperator/paste. state.lastChange keeps whatever was there before, so after a dd, selecting a word, "+p to wrap it as a link, and then . deletes a line.

The edit block has no try/finally — src/Vim.js:724-734. If anything in dispatch() throws, Backend::m_editBlockDepth and editor.vimEditDepth are both stranded above zero for the rest of the session: the document stops emitting its change signal, TextEdit.text freezes, onTextChanged never fires again, and the modified flag, word count, search refresh and recovery draft all stop silently while the writer keeps typing. I could not find a reachable throw, so this is about blast radius rather than a bug today — but it is the same handler that threw on you in 7a69964, and effectiveCount() is unbounded, so 999999999p sits in repeatString() (src/Vim.js:395) with the block open. substitute() and deleteRange() are open the same way.

:s still lands one short when the replacement adds lines — src/Vim.js:1795. landing is a line number recorded on the last matching line, and the loop then keeps editing the lines above it; since expandReplacement supports \n, :%s/x/a\nb/ shifts that line down and the caret ends up above the real end of the change. A residual of adbc5e2, caret only.

Two smaller notes. The PR body is stale — it still lists text objects as out of scope and reports "Nine new cases … 20 passed", which undersells a branch that now has iw aw ip ap and the bracket pairs, registers, and 25 passing; the README is current. And #5 conflicts with this branch, not just textually. It puts a StandardKey.Undo/Redo interception at the top of the same Keys.onPressed where this puts the vim branch, and it replaces editor.undo() with a Backend::replayHistory that deliberately consumes several document undo entries per user action. u here calls editor.undo() directly (src/Vim.js:78), so with both in, u and Ctrl+Z would undo different amounts. Worth agreeing on before either lands. #12 merges clean, and its blank-line branch actually improves o, which now routes through smartReturn.

Whether Omawrite wants a vim layer at all is the maintainer's call and I have not made it.

[rodgco]

The four findings, fixed

Thanks for the differential run on the opt-in — building both sides and driving them through the same script is a better answer than anything reading the diff could have given, and the 16-of-21 failure with the mode on is the part that makes the other 21 mean something.

All four are fixed, one commit each. Every one of them was checked by reverting the fix and watching its own test fail, so none of these assertions are decoration.

Astral characters (75d21f5… see 3e62c07)

You were right that there were two shapes available. I took the first — step by character in the four places — rather than normalising in moveCaret/setCursor, because the corruption does not go through the caret. lvd reaches the document through selectionRange → applyOperator → host.replace, and never passes setCursor at all, so normalising there would have fixed where the caret is drawn and left the surrogate on disk. Snapping ranges at the end would also have hidden the motion bug rather than fixed it: a de whose end was half a character out would round to the right answer, and no test could then tell a correct motion from a rounded one.

So the rule is the one the file already had and had only half-applied. stepForward/stepBackward existed, clampNormal already used them, and what was left were the four places that meant "past this character" and wrote + 1: both ends of a visual selection, an inclusive motion's range, and ge's backwards range. wordEnd and wordEndBackward now walk by character too, and return the start of the character they land on rather than its last unit — that is where the caret belongs, and it is what made e land at offset 2 in your repro.

To be sure this was the astral case and nothing else, I ran the new word-end functions against the old ones over every position of fourteen BMP samples, both word sizes: identical answers throughout.

Your repro is the test, and without the fix it fails with exactly the string you quoted:

Actual   (runVim(editor.data(), emoji, 0, "lvd").text): "a\uDE00b"
Expected ("ab")                                       : "ab"

The visual link paste and . (e46f74e)

Fixed, and it was worse than the comment says. The early return skipped two steps the plain path takes on its way out, not one: commitChange, and writing what was pasted to the unnamed register. So . replayed the previous change and would have pasted a stale register even once recorded. Both now run on that path.

Your scenario is the test, in a real window against the accepting hook rather than the fast harness's declining one. Without the fix, . after a dd and a "+p does not merely delete a line — on the one-line document left behind it empties the document:

Actual   (text()): ""

The edit block (f6234c3)

Agreed on the reasoning, including that you could not find a reachable throw — nor could I. The commit fixes the blast radius rather than a bug: withEditBlock(host, body) closes in a finally and carries the exception on out, and the three places that open a block go through it. Stating the invariant once, next to why a stranded block costs the session rather than the command, seemed better than three trys.

The count is still unbounded, so 999999999p will still sit there — but it now sits with the block closing behind it whichever way it ends.

Testable without a window: the fast harness grew a hook that can be made to throw, and asserts the depth is back to zero and that the next command edits the live document. Reverted, it reports depth 1.

:s landing (75d21f5)

Fixed, and thank you for spotting that adbc5e2 left a residue rather than finishing the job. Both effects turn out to be the same count: edits above the recorded line push it down, and new lines on the landing line itself extend the change past it, so adding the lines the replacements introduced covers both.

I checked the intended answer against vim rather than reasoning it out. On one x / two x / three, :%s/x/a\rb/ leaves vim on line 4, the second b; this branch left the caret on line 2 and now lands on 4.

The stale body

Updated — text objects, registers, sentence motions and ge/gE are in the list now, the consolidation with #10 is credited in the body rather than only in a comment, and the count reads 26 passed, 1 skipped.

On #5

Worth adding to what you found, since it moves the conflict rather than resolves it: this branch now merges fix/undo-word-granularity, so u calls editor.undoEdit() rather than editor.undo() — the editor's wrapper, which steps past the formatting-only edits that end an undo run. The disagreement you identified is unchanged and still wants agreeing before either lands: #5's replayHistory deliberately consumes several document entries per user action, and vim's u is one step per command. Two things now sit in that same Keys.onPressed from this side, not one, since Ctrl+Z and Ctrl+Y had to be taken there — TextEdit claims both as built-in editing shortcuts, so the Shortcut items for them never ran.

./bin/test gives 26 passed, 1 skipped; ./bin/build is clean.

[greptile-apps]

Greptile Summary

Adds opt-in Vim key bindings, persisted settings, mode indicators, and an editor-hosted Vim state machine while preserving the existing insert-mode editing behavior.

• Adds normal, insert, and visual modes with motions, operators, registers, counts, undo, and repeat support.
• Adds an Ex command line for file operations, navigation, substitution, deletion, and search highlighting.
• Extends the backend boundary for edit blocks, clipboard registers, path resolution, document access, and persisted Vim settings.
• Adds extensive unit and window-level coverage for Vim commands and application integration.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/Vim.js	Introduces the Vim state machine, motions, operators, registers, repeat support, and Ex command execution; no eligible follow-up defect remains.
src/Main.qml	Integrates Vim routing, mode and command-line UI, host callbacks, grouped history navigation, and the footer toggle.
src/backend.cpp	Adds native support for persisted Vim mode, edit blocks, history navigation, clipboard registers, document text, and path resolution.
src/backend.h	Exposes the new QML-facing backend properties, signals, and editing and filesystem APIs.
src/EditorMutations.js	Uses the live document length while grouped edits are open so replacement ranges remain valid.
tests/tst_omawrite.cpp	Adds broad engine and integration coverage, including a real-QML-runtime regression test that bounds pathological substitute evaluation.
src/resources.qrc	Packages the new Vim JavaScript module with the runtime QML resources.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Input[Keyboard or command-line input] --> Main[Main.qml routing]
    Main -->|Insert mode| Editor[Existing TextEdit behavior]
    Main -->|Normal or visual mode| Vim[Vim.js state machine]
    Vim --> Host[Editor host wrapper]
    Host --> Editor
    Host --> Backend[Backend services]
    Backend --> Files[Local files and settings]
    Backend --> Clipboard[Clipboard and primary selection]

Loading

_{Reviews (4): Last reviewed commit: "Keep a text object, a dot repeat and a ~..." | Re-trigger Greptile}

[rodgco]

On the substitute regex finding: correct for V8, and this does not run on V8

Thank you for the review. The finding is a real property of backtracking regular expressions and the reasoning is sound — it just does not hold in the engine this code runs in, and I would rather show that than assert it.

d0b5170 adds the coverage you noted was missing, since this is the runtime's property rather than the engine's and nothing in the branch would notice if it changed.

The measurements

(a+)+$ against 'a'.repeat(n) + 'b', in Node:

n=18    33 ms
n=22    86 ms
n=24   393 ms
n=26  1739 ms
n=28  6636 ms      doubling with each further character

The same patterns in Qt's V4 engine, which is what QML runs:

(a+)+$     n=50   52 ms    n=1000  35 ms    n=5000   9 ms
(x+x+)+y   n=50   32 ms    n=1000  34 ms    n=5000  10 ms
(a|a)+$    n=50   42 ms    n=1000  41 ms    n=5000  41 ms
(a*)*b     n=50   21 ms    n=1000  20 ms    n=5000  21 ms

/(a+)+$/.test('aaaa')                    -> true,  0 ms
'one x two x'.replace(/x/g, 'y')         -> "one y two y", 0 ms

Flat, not exponential, and legitimate matches and replaces are unaffected. V4 compiles RegExp through PCRE2, whose match limit stops the backtracking rather than letting it run.

Through the real :s path rather than a bare engine — the branch's own harness, a 2001-character line, :s/(a+)+$/z/ — 10 ms, reporting Pattern not found. The test asserts it finishes; an unbounded engine would fail it by never completing.

What does hold, in a smaller way

The limit is per match attempt, and :%s runs one per line, so the cost is linear in the document:

50 lines     557 ms
200 lines   1872 ms
1000 lines  9409 ms

So a pathological pattern over a large document still blocks the UI for seconds — bounded rather than unbounded, and one u undoes whatever it did, since the command is a single edit block. I have not put a deadline across the loop: vim answers this with Ctrl-C rather than a time limit, and inventing a cutoff that stops a substitute partway through seemed worse than the linear cost it would avoid. Happy to be argued out of that.

The trade PCRE2 makes

The bound is not free: a pattern that would match, but costs more than the limit allows, reports no match rather than hanging. For a writer's substitute that is the right way round, and it is the same trade every QRegularExpression caller in backend.cpp already makes.

One caveat on my own claim, since it is the load-bearing one: I verified PCRE2's bound behaviourally, across four patterns and four input sizes, rather than by citing Qt's source. The flat timings are strong evidence, not a citation.

[omarchybot]

Re-reviewed at d0b5170, by me (Claude Opus 5) and by Codex at xhigh reasoning as a second reviewer. All four findings from the last pass are fixed, each with coverage that fails when its own fix is reverted — I checked 3e62c07, e46f74e, f6234c3 and 75d21f5 against the source rather than taking the summary, and the body is current. Two things are new: a correction to the regex answer, and five more places the astral fix does not reach, pushed as d109883 and b171d94.

The substitute regex: right conclusion, wrong engine

The bound is real. I could not make :s hang with anything I tried, and my numbers match yours, so the P1 stays withdrawn. But QML's RegExp does not go through PCRE2, and anyone who later checks that the bound is still there will check the wrong thing.

Qt 6.11.2's libQt6Qml.so embeds JavaScriptCore's YARR — JSC::Yarr::ByteCompiler, JSC::Yarr::ByteTerm, JSC::Yarr::PatternDisjunction and the string YarrJIT are all in it — and it contains no PCRE2 symbols at all: nm -D finds zero matches for pcre2_, against 15 in libQt6Core.so. The libpcre2-16 in its ldd output is QtCore's, for QRegularExpression. So the closing line of your comment is half right — it is the same trade backend.cpp makes, but not the same engine, and the two live in the same process without sharing a limit.

Codex found the mechanism I could only measure, and it is better news than a JIT artefact would have been: YARR carries a private matchLimit of 1,000,000 steps, defined in Yarr.h, decremented by the bytecode interpreter, and guarded again on the JIT's nested-parenthesis path — so a pattern that cannot be JIT-compiled falls back to a path with the same bound. That matches what I measured: the bound does not move when the stack is raised (QV4_JS_MAX_STACK_SIZE=134217728, QV4_STACK_SOFT_LIMIT=134217728, QV4_MAX_CALL_DEPTH=100000 all leave it exactly where it was) and it survives QV4_FORCE_INTERPRETER=1. Through String.replace, the call :s makes at src/Vim.js:1802:

(a+)+$        n=50  14 ms   n=500  14 ms   n=2000   5 ms   n=8000   5 ms
(a+)+$   /g   n=50  15 ms   n=500  14 ms   n=2000   5 ms   n=8000   5 ms
(a*)*\1b      n=50  10 ms   n=500  10 ms   n=2000  10 ms   n=8000  11 ms
(a+)+\1$      n=50  16 ms   n=500  16 ms   n=2000  17 ms   n=8000  18 ms
^(a|a)*\1$    n=50  22 ms   n=500  28 ms   n=2000  26 ms   n=8000  22 ms

The backreference patterns are there because YARR's JIT cannot compile all of them, so those run through the interpreter. Flat there too.

Two things follow that are worth having. The limit is private to Qt's vendored copy of masm, not a documented contract like PCRE2's MATCH_LIMIT, and this repository takes whatever Qt 6 the system has — so the durability the test is meant to pin is thinner than the comment claims, though nothing about 6.11.2 is at risk today.

And the trade bites earlier than "a match that costs more than the limit allows" suggests. (\w+\s?)*$ is a pattern a person might actually write, and it matches the empty string at the end of any input — Node agrees at every length. This engine stops agreeing at twenty characters:

n=5   MATCH at 6       n=20  NO MATCH
n=10  MATCH at 11      n=30  NO MATCH
n=15  MATCH at 16      n=50  NO MATCH

So :s can report Pattern not found for a pattern that does match, on a line of ordinary length. Not an argument against the design — a wrong answer in milliseconds beats a hung window — but a smaller threshold than the discussion has been assuming.

On the test at tests/tst_omawrite.cpp:648: its comment names PCRE2, and if the bound went away it would not fail, it would hang. spent is only read after runEx returns, so the regression arrives as a timed-out CI job rather than a red assertion.

Five more astral sites — d109883 and b171d94

3e62c07 counted characters in the four places a motion feeds an operator, which is where a split pair reaches the document in one step. What is left are the places that set a caret or record a count, and those reach the document in two steps: the position lands between the halves of a character, and the next x, X or r takes one code unit of it. Your note that the corruption "does not go through the caret" was right about lvd and not right in general.

Rather than argue from reading, I swept 106 key sequences over 16 documents holding astral and length-changing characters, from every caret a motion can actually reach — 10600 combinations, each result checked for an unpaired surrogate. At d0b5170: 114 corrupted. After both commits: 0. Each fix was then reverted on its own and fails its own assertion.

d109883, found here:

• findInLine returns i - 1 for t, the last code unit of whatever precedes the target. On a😀X from column 0, tX leaves the caret at offset 2 and tXx gives a\uD83DX. The ; repeat had to move with the return, since the skip that stops ; standing still was comparing the unit after the caret.
• verticalMove measures gj/gk's column in code units on the line it left and applies it to the line it lands on. On ab\n😀z from offset 1, gjx gives ab\n\uD83Dz.

b171d94, all three found by codex and reproduced here before being touched:

• applyObject steps back one code unit from a text object's exclusive end to seat the visual head. On a document holding one emoji, viw<Esc>x leaves \uD83D.
• insertDelta records what . should delete behind the caret as a code-unit count, measured during one insert session and spent wherever the repeat lands. On a 😀x from the space, i<BS><Esc> then wl. gives \uD83Dx.
• ~ restores the offset the run ended at before the toggle, which is stale whenever toggling changes a length. ß becomes SS, so on ß😀x, 2~x gives SS\uD83Dx. Reading the end from what was actually written also fixes the caret sitting a character short after any ~ over a ß, astral or not.

Two consequences worth your eye, since they are choices rather than pure fixes. gj/gk's column is now the nth character rather than the nth code unit, so on a line containing an emoji it can land one unit further right than before — the only way it can name a position that is not inside a character, but round it the other way if you prefer. And ~ now leaves the caret after the run as written, which moves it by one for every ß even with no emoji in sight.

Three related things I did not touch:

• The fast harness's own Backspace stand-in removes one code unit (tokenize/feed in tst_omawrite.cpp), where a real TextEdit removes a character. That is scaffolding rather than the engine — i<Backspace> corrupts in the harness with no . involved at all — but it means the harness cannot currently be used to test astral behaviour around Backspace, and a future test could pass on behaviour the app does not have.
• insertDelta's prefix/suffix scan can still split a pair if an insert session replaces one emoji with another sharing a high surrogate. I could not reproduce it, because the harness cannot type an astral character at all, so I left it rather than push something untestable.
• toggleCase's /./g walks code units, so ~ on an astral cased letter such as 𐐀 leaves it unchanged instead of producing 𐐨. Wrong, but it does not corrupt.

Separately, and not introduced by this PR: src/Main.qml's moveCursorVisibly does cursorPosition + direction, one code unit per arrow key, and it is already on master (line 703 there). Vim insert mode returns unhandled keys to that path, so it is reachable from this feature, but it is a pre-existing bug and belongs in its own change. Also, vimKeyName returns "" for any event.text longer than one unit, so an astral character can never be an r or f argument — r😀 cannot work today.

If you check any of this yourself: QString::toUtf8() silently drops an unpaired surrogate, so a qDebug() of a corrupted document prints something that looks perfectly fine. That cost me a wrong conclusion before I switched the sweep to comparing UTF-16 units.

Not a defect, but not this pull request's to settle

a650dfc, merged in as 3a4dce3, is not on master and reaches everyone. src/Main.qml:1052 takes Ctrl+Z and Ctrl+Y for every user, vim mode or not, and routes them through undoEdit()/redoEdit(), while backend.cpp:427 ends an undo run at each word — gated on !m_vimMode, so it is precisely the people who never turn vim mode on who get the new granularity. It may well be the right change; it is a change to undo for everyone arriving inside a pull request whose title says opt-in, and worth naming so it gets decided on its own terms.

What was checked

./bin/test and ./bin/build on a disposable Arch worker running Qt 6.11.2, before and after each push: 26 passed, 0 failed, 1 skipped, clean build. The regex timings and the astral sweep are from that same worker. Codex reviewed d0b5170 independently at xhigh and agreed on the four earlier fixes and on the :s landing arithmetic; the Yarr matchLimit citation and three of the five astral sites are its contribution rather than mine, and I reproduced each before changing anything. Nothing here was exercised against a real compositor.