Skip to content

Outdent heading markers - #5

Open
mateuszkowalczyk wants to merge 11 commits into
omacom:masterfrom
mateuszkowalczyk:outdent_heading_markers
Open

Outdent heading markers#5
mateuszkowalczyk wants to merge 11 commits into
omacom:masterfrom
mateuszkowalczyk:outdent_heading_markers

Conversation

@mateuszkowalczyk

Copy link
Copy Markdown

Reason for Changes

I personally find it more visually pleasing to have Markdown heading markers (like ###) outdented, while keeping the actual heading text aligned with regular paragraphs.

Feel free to use my implementation if it aligns with your vision for the app. Here's a screenshot for comparison:

screenshot-2026-08-08_14-35-05

Tests

Here's the list of tests I performed manually:

  • verified that various real-world notes render correctly (copied from my Obsidian vault)
  • resized the window from fullscreen to very small and tested all states in between
  • repeatedly changed the Omarchy monitor scaling
  • verified that Markdown headings above level 6 are not formatted
  • verified undo/redo behavior

Implementation approach

  • I asked GPT-5.6 Sol to implement the initial version
  • Then went through multiple iterations of manual review (done by me) and fixes (done by Sol) until the implementation felt reasonably simple IMO
  • Performed manual testing and made some additional fixes, mostly around undo/redo behavior

@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed this by driving the real editor on a disposable VM (Qt 6.11.1, offscreen) rather than reading the diff alone. ./bin/test is green: 15 passed, 0 failed.

What holds up. The gutter measures right: with the bundled font at 20px the cell is exactly 12px, and # , ## and ###### all put their heading text at x=84, the same column as body text, with the level-6 marker landing exactly on the block's left edge — level + 1 <= 7 is what keeps a marker on screen. Geometry holds too: across window widths 720/500/394/300/200 at 1x and 2x text scale, x never went negative and the TextEdit never overflowed the Flickable. And Ctrl+Z does not double-fire — the window-scoped Shortcut consumes the key before Keys.onPressed sees it, so one real Ctrl+Z is exactly one step, measured through the window's event pipeline rather than sendEvent.

Three defects, one root cause: setHeadingCellWidth()updateAllBlocksTypography() writes block formats into the document with its undo stack enabled, so a change of desktop text size becomes a user-visible undo command. applyDocumentTypography() deliberately turns undo off first; this path does not.

1. Redo is silently discarded, and the text is unrecoverable. Type hello, press Ctrl+Z, then change the desktop text size. Qt drops the redo branch the moment a new edit is recorded, so isRedoAvailable() flips true → false and Ctrl+Shift+Z gives back nothing.

2. One undo strips the document's typography. Open a document, change the desktop text size, press Ctrl+Z once. replayHistory consumes the format command looking for a text change, finds none, and stops — text untouched, every block back at the old margin (84 where the gutter is now 168). It never repairs itself: edit the body block afterwards and that block goes to 168 while the heading stays at 84, so the document ends up with mixed margins block by block.

3. Once any block is stale, one redo replays several edits. From the state in 2, make two cursor-separated edits and undo both — a single redo replays both, because documentHasExpectedTypography() scans the whole document and an untouched block keeps failing the check. A control run without the size change replays them one at a time, correctly.

I have not pushed a fix for these. Every option I can see is a design decision rather than a bug fix: disabling undo around the width change clears the entire stack instead of just the redo branch, and the alternative is keeping the gutter out of block formats altogether. That is your call and the maintainer's, not mine to make in your branch.

Two smaller things, both verified:

blockFormatWithTypography() indents by heading.level + 1 rather than the prefix width headingMarkup() already found, so a marker followed by more than one space drifts right. With the bundled font: ## Heading puts its text at 84 (aligned), ## Heading at 95.9, ## Heading at 107.9, ##\tHeading at 128. Worth saying that prefixLength is not a drop-in replacement — it can exceed 7 cells, which is exactly what would push a marker off the left edge.

At 720px with 3x text scale the body column is no longer centred: x clamps to 0, leaving 252px of space on the left against 60px on the right. laysOutAndEditsHeadings asserts only x >= 0 and containment, so it passes on that layout.

Finally, a note rather than a defect: the new StandardKey.Undo/Redo branches in Keys.onPressed never run in a real window, because the Shortcut gets there first. They are the only path undoingHeadingDoesNotExposeTypographyStep exercises, since QCoreApplication::sendEvent(editor, ...) bypasses the shortcut map — so the route users actually take is the one the test does not cover. It does work; it just is not what is being asserted.

Heads-up on overlap you did not cause: #12 also edits smartReturn immediately above your refreshCursorAfterHeading() call, and #18 fixes narrow-window clipping in the same editorWidth/x bindings you rewrote. Whichever lands second will need a rebase.

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown

Greptile Summary

The PR outdents Markdown heading markers while keeping heading text aligned with body paragraphs and adds history-aware typography updates.

  • Adds heading-specific block indentation and responsive gutter sizing.
  • Routes undo and redo through the backend to skip internal typography operations.
  • Centralizes heading recognition between highlighting and block layout.
  • Adds integration coverage for heading layout, text scaling, cursor geometry, and history behavior.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains within the eligible follow-up review scope.

No blocking failure remains.

Important Files Changed

Filename Overview
src/Main.qml Adds the heading gutter layout, cursor-geometry refreshes, and backend-routed undo and redo actions.
src/backend.cpp Applies heading-aware block typography and coordinates typography changes with document history.
src/backend.h Exposes heading sizing and history actions to QML and declares the new typography helpers.
src/markdownhighlighter.cpp Centralizes ATX heading recognition and uses it for heading marker and content formatting.
src/markdownhighlighter.h Defines the shared heading-markup representation used by highlighting and block layout.
tests/tst_omawrite.cpp Adds coverage for heading recognition, responsive layout, cursor alignment, and undo/redo behavior.

Sequence Diagram

sequenceDiagram
    participant User
    participant Editor as QML TextEdit
    participant Backend
    participant Document as QTextDocument
    participant Highlighter

    User->>Editor: Edit Markdown text
    Editor->>Backend: editorTextChanged()
    Backend->>Document: Apply heading/body block typography
    Highlighter->>Document: Style heading markers and text
    User->>Editor: Undo or redo
    Editor->>Backend: undo(editor) / redo(editor)
    Backend->>Document: Replay text and typography history
Loading

Reviews (3): Last reviewed commit: "Preserve editor history with scalable he..." | Re-trigger Greptile

@mateuszkowalczyk

Copy link
Copy Markdown
Author

I addressed the review findings around heading layout and editor history, so it now:

  • tests undo/redo through the real window-level shortcut path
  • keeps the editor body centered at high text scales
  • prevents typography updates from corrupting undo/redo behavior
  • has regression coverage for history resets, typography consistency, and subsequent one-step undo/redo

Two design choices

1. Keep the leading spaces in headings

One of the reported issues isn’t actually a bug in my opinion, but the desired behavior. The first space after the heading markers should be reformatted, but any following spaces are regular characters that are part of the heading itself, so we shouldn’t modify them.

2. Undo/redo tradeoff

Desktop text-size changes recalculate block margins and indentation. Qt previously recorded these layout-only changes as user edits, which could discard redo history, restore stale margins, or combine several edits into a single redo step.

I considered two solutions:

  1. Clear the undo/redo history while reapplying typography.
  2. Move the gutter outside Qt block formats, which would require a substantially larger custom editor-layout implementation.

I chose the first option because desktop text-size changes are rare, making an occasional history reset an acceptable tradeoff. The alternative would introduce considerable complexity and regression risk around wrapping, cursor positioning, selection, scrolling, and marker rendering.

@omarchybot

Copy link
Copy Markdown
Collaborator

Re-reviewed at ba40a1d on a disposable VM (Qt 6.11.2, offscreen). ./bin/test is green: 16 passed, 0 failed. Everything below was measured by driving the editor, not read off the diff.

Correction to my last comment. I told you the StandardKey.Undo/Redo branches in Keys.onPressed never run in a real window. That was wrong. I put a log line in both handlers and pressed Ctrl+Z through the window: with the editor focused, an editable TextEdit accepts the ShortcutOverride event Qt sends before the shortcut map runs, so the window Shortcut never fires and your Keys.onPressed branch is what handles the key — same result with the window hidden and with it shown, exposed and active. The Shortcut fires only when focus is elsewhere; with focus on the save button, Ctrl+Z still undoes, and that is the path it takes. So the branches are the primary path and the shortcuts are the fallback, and QTest::keyClick(quickWindow, ...) goes through the same delivery a user's keypress does. The new test is doing real work. Ctrl+Y also matches StandardKey.Redo here and reaches backend.redo, not TextEdit's built-in redo.

Findings 2 and 3 are fixed, verified by exercising them rather than reading the change. After setTextScale(2.0) on a ### Heading / body document, one Ctrl+Z leaves the text intact with every block still at leftMargin 168 and the heading at textIndent -96, and a later edit keeps every block at 168 — the mixed-margin state is gone. Three cursor-separated edits made after the scale change undo one at a time and redo one at a time. I also went looking for another way to leave a block stale, since the redo loop's whole-document check is what made finding 3 bite, and could not find one: a multi-block paste, a select-all replacement, a multi-block deletion and opening a file from disk all leave every block fresh, and one redo after two edits replays exactly one of them. A text-size change on a clean document also leaves backend.modified false, so it does not turn into a spurious save prompt.

Finding 5 is fixed. Across scales 0.75/1/2/3 and window widths 1400 down to 720, the body column is centred within a pixel and nothing overflows the Flickable. The 720px/3x case that was 252px of space on the left against 60px on the right is now 252 against 252. Below 720px the max(1, ...) fallback stops being centred, but minimumWidth: 720 and the 0.5–3.0 clamp in sanitizedTextScale() put that out of reach.

Finding 1 is not fixed — it is now a deliberate tradeoff, and that is the maintainer's to accept. Type text, undo it, change the desktop text size: isRedoAvailable() goes 1 → 0 and Ctrl+Shift+Z gives back nothing. The text that was undone is still unrecoverable, and now the undo branch goes with it. What changed is that this is deliberate and documented instead of a side effect, and the corruption it used to cause is gone. Your reasoning rests on text-size changes being rare, so it is worth knowing that #21 would make them not rare: it composes m_systemTextScale * m_zoom into the same textScale and drives it from Ctrl+=, Ctrl+- and the mouse wheel, so with both merged every zoom step and every wheel notch would clear the document's history. Whichever lands second, that interaction needs a decision.

Finding 4 — extra spaces after the markers pushing heading text right — I measured again (## H starts at 95.9 against a body column at 84) and your reading is defensible; it is the maintainer's call, not mine. Worth noting the formula does one more thing: level + 1 is what guarantees the marker never leaves the 7-cell gutter, since ###### puts the marker's left edge exactly at 0. The prefix width would not.

Second opinion: codex at xhigh reasoning agreed with the conclusions above, including that the Keys branches are live and my earlier finding 6 was wrong. Its independence is not currently guaranteed, so read agreement as agreement rather than confirmation. What it added on its own: the 3.0 cap in sanitizedTextScale() (src/systemtheme.cpp:46), which is what makes the uncentred fallback unreachable, and the observation that replayHistory() would loop forever if it were ever handed an editor not backed by the attached document — not reachable today, since Main.qml always passes the one TextEdit.

Nothing pushed to your branch. Waiting on the maintainer for the history-reset tradeoff; nothing outstanding on you.

@mateuszkowalczyk

Copy link
Copy Markdown
Author

Reworked this to use integer block indents with QTextDocument::indentWidth instead of storing pixel margins in every block. A text-size or future zoom change now updates only the document’s indent width, so existing undo/redo history is preserved and no full-document reformat is needed. So this eliminates potential issue after #21 is merged

The tradeoff is that wrapped heading continuation lines begin at the marker column rather than the body-text column. This is a small layout compromise in exchange for preserving history without a larger editor architecture change. Extra spaces after heading markers remain visible and continue to push the heading text right intentionally.

@omarchybot

Copy link
Copy Markdown
Collaborator

Re-reviewed at d23f21a on a disposable Omarchy VM (Qt 6.11.2). ./bin/test is green: 16 passed, 0 failed. Everything below was measured by running the code, not read off the diff.

Finding 1 — the one that was blocking — is fixed. I built this head and the previous one (ba40a1d) side by side and ran the same probe against both. The probe types a heading, a paragraph break and a word, undoes the word so it exists only in the redo branch, changes the desktop text size, and then redoes. At ba40a1d it fails at isRedoAvailable(), which is 0 after the size change, with availableUndoSteps() also 0 — the undone word is gone for good. At d23f21a the same run passes: redo is still available, Ctrl+Shift+Z gives the word back, and three further Ctrl+Z walk back through ## Heading\n\n, ## Heading and empty, one user edit per keystroke.

No block is touched by a text-size change. A second probe snapshots every QTextBlockFormat in a seven-block document, changes the text size, and compares. At ba40a1d block 0's format is rewritten and the document's revision() goes 5 → 6. At d23f21a every format compares equal and revision() stays at 5 — the size change reaches QTextDocument::setIndentWidth() and nothing else. That matches the code: setHeadingCellWidth() no longer calls the per-block pass, and the only setLeftMargin/setTextIndent calls left in the tree are the two constant zeroes in blockFormatWithTypography() (src/backend.cpp:205-206), which cannot vary with the text size.

Undo and redo still walk one user edit at a time after the rework. Four edits, one of which prefixes ## onto the first line and so changes that block's indent; undo four times back to empty and redo four times forward, checking the text after each keystroke; then an undo, a text-size change, and a redo. Every step lands exactly where it should. The new availableUndoSteps()/availableRedoSteps() guard in replayHistory() also closes the "loops forever if handed an editor not backed by the attached document" note from the last pass — an invocation that moves neither counter now returns instead of spinning.

The #21 interaction: your claim holds, and I checked it on the merged code rather than by reading both diffs. git merge-tree of this branch and #21 merges src/Main.qml, src/backend.cpp and src/backend.h cleanly. I built that merged tree and ran the redo-across-a-size-change probe with three backend.zoomIn() steps standing in for the desktop size change: redo survives, and no block format changes. So the thing that would have made every Ctrl+= wipe the document's history is genuinely gone.

The merge-order consequence is small and mechanical, and it lands on whoever is second. Only tests/tst_omawrite.cpp conflicts, and #21 renames Backend::setTextScale to setSystemTextScale, so the three backend.setTextScale(...) calls in your new tests need renaming for the merged tree to compile. Your Main.qml never writes that property, so nothing outside the tests is affected. Separately, #12 still conflicts with you in src/Main.qml — it edits smartReturn and the blank-line Return behaviour in the same region as your refreshCursorGeometry() calls and your deleteParagraphBreakBehindCursor rewrite — and #18, #24 and #25 each conflict in src/Main.qml around the editor geometry you rewrote.

The wrapped-continuation tradeoff, seen rather than reasoned about. I ran both builds on a real compositor with a heading long enough to wrap. At ba40a1d the second line of a wrapped heading began at the body column, level with the heading's own first line. At d23f21a it begins at the marker column instead, so a wrapped ## … heading has its continuation sitting under the ## and hanging one marker-width left of where the heading text starts. It is exactly the compromise you describe, it is visible, and it is the maintainer's to accept — I am reporting it, not arguing it. Body paragraphs are unaffected: they still wrap to the body column.

Extra spaces after the markers. Unchanged from your position and unchanged from mine: ### Heading puts its text two cells right of the body column, your reading that only the first space is a separator is defensible, and it is a product decision for the maintainer rather than a defect.

One new low finding. A line beginning with # inside a fenced code block is now outdented into the heading gutter (src/backend.cpp:201headingMarkup() sees only block.text() and has no fence context). The bold heading styling of that line is pre-existing, not yours: master's highlightMarkers() had the same fence-blind check. What is new is that the line also moves left. Fixing it properly means tracking fence state across blocks, which is a bigger change than this PR, so I would leave it — but it is worth knowing it exists. In the same family, # on its own and a heading indented by one to three spaces are not recognised, which is also pre-existing behaviour that the indentation now makes visible as a small jump when you type the space after the markers.

Second opinion: codex at xhigh reasoning reviewed the branch independently and found no defect in the typography/history claim, in replayHistory()'s termination, in the exact-format comparison, in the single-newline Backspace path, in the undo/redo dispatch, or in the editor geometry — agreeing with the conclusions above. Its independence is not currently guaranteed, so read that as agreement rather than confirmation. It reached the fenced-code finding on its own from the source while I reached it from probe output, which is the one place the two reviews met from different directions. It also flagged the wrapped-continuation change and the extra-space indentation as defects; both are the deliberate choices you documented, so I am carrying them to the maintainer as decisions rather than treating them as bugs.

Nothing pushed to your branch — there is no defect here I could fix without making a product decision that is not mine. Waiting on the maintainer for the two layout calls; nothing outstanding on you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants