Outdent heading markers - #5
Conversation
|
Reviewed this by driving the real editor on a disposable VM (Qt 6.11.1, offscreen) rather than reading the diff alone. What holds up. The gutter measures right: with the bundled font at 20px the cell is exactly 12px, and Three defects, one root cause: 1. Redo is silently discarded, and the text is unrecoverable. Type 2. One undo strips the document's typography. Open a document, change the desktop text size, press Ctrl+Z once. 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 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:
At 720px with 3x text scale the body column is no longer centred: Finally, a note rather than a defect: the new Heads-up on overlap you did not cause: #12 also edits |
Greptile SummaryThe PR outdents Markdown heading markers while keeping heading text aligned with body paragraphs and adds history-aware typography updates.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up review scope. No blocking failure remains.
|
| 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
Reviews (3): Last reviewed commit: "Preserve editor history with scalable he..." | Re-trigger Greptile
|
I addressed the review findings around heading layout and editor history, so it now:
Two design choices1. Keep the leading spaces in headingsOne 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 tradeoffDesktop 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:
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. |
|
Re-reviewed at ba40a1d on a disposable VM (Qt 6.11.2, offscreen). Correction to my last comment. I told you the Findings 2 and 3 are fixed, verified by exercising them rather than reading the change. After 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 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: Finding 4 — extra spaces after the markers pushing heading text right — I measured again ( 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 Nothing pushed to your branch. Waiting on the maintainer for the history-reset tradeoff; nothing outstanding on you. |
|
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. |
|
Re-reviewed at d23f21a on a disposable Omarchy VM (Qt 6.11.2). 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 No block is touched by a text-size change. A second probe snapshots every Undo and redo still walk one user edit at a time after the rework. Four edits, one of which prefixes The #21 interaction: your claim holds, and I checked it on the merged code rather than by reading both diffs. The merge-order consequence is small and mechanical, and it lands on whoever is second. Only 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 Extra spaces after the markers. Unchanged from your position and unchanged from mine: One new low finding. A line beginning with Second opinion: codex at xhigh reasoning reviewed the branch independently and found no defect in the typography/history claim, in 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. |
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:
Tests
Here's the list of tests I performed manually:
Implementation approach