Repository navigation
Replies: 1 comment 3 replies
|
This is the right way to raise it, thank you. A registered template is on the real path, since Of your three directions: "declare it" is more machinery than the evidence supports yet, and "skip it" throws away a check that works for many templates. I would rather fix the proxy. Indent is a stand-in for "this line is a list item", and a line that opens with a list marker is direct evidence of the same thing. So I would take a PR with the glyph test alongside the indent test, with two limits you already named: the Unicode-only marker set (no ASCII |
|
Agreed on all four limits: Unicode-only marker set, lone-marker branch untouched, glyph test beside the indent test, and a docstring caveat naming the orphan rule. Here are the two shapes to reproduce before the PR. Both are an invented gardener's CV ("Toby Mercer"; every name, place, employer and date is made up), built on the stock
The geometry behind both, from The four files:
The PR will follow once you have reproduced it. It keeps upstream's branch structure: |
|
Reproduced both shapes exactly, on your PDFs and on my own lualatex builds of the two The fix shape we agreed on, though, does not survive the stock template, and that is on me for agreeing to it before measuring. The stock What does discriminate across all three geometries (yours, stock, and plain moderncv defaults) is the x of the text, not of the line: strip a leading lone marker word, then compare the remaining text's x on page 1's last line with page 2's first non-marker line. Equal x (72.1 = 72.1) is a wrapped bullet, clean; more indented (59.5 -> 72.1 on yours, 70.4 -> 81.3 on stock) is an orphaned header. That keeps the Unicode-only marker set and leaves the lone-marker branch alone, as before. One case it cannot separate without font information is a stock |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Filing this as an Idea rather than a PR, because CONTRIBUTING is clear that a fix has to reproduce on the real path and the trigger is a template registered via
/add-template, which this repo does not ship. A reproduction and a candidate patch exist; I would rather hear whether the direction is wanted than send a PR you would have to decline.The observation
find_orphans()intools/verify_layout.pyuses left indent as its proxy for "this line is a list item":That proxy is a property of the stock moderncv bbox output, not of CVs in general. The docstring already warns that a template registered via
/add-template"may need them retuned", and gives the phantom-hole/footer example. The orphan rule is a different kind of case: it is not a threshold that can be retuned.When a template emits its bullets as a separate list below the entry header rather than inside the entry, Poppler derives each word's yMin/yMax from font ascent/descent rather than glyph ink, so the bullet marker and the item's first line land in the same rounded bucket and merge into one bbox line — whose left edge is the marker's, not the text's. Continuation lines start at the item indent. Measured on a compiled PDF:
So
indented(last)is False andindented(first)is True, and the wrapped bullet is reported as an orphaned\cventry. No value ofINDENT_PTseparates the two: the marker line sits below the header indent and the continuation above it, so any threshold either misses real orphans or invents them.It cuts both ways
The false positive is the annoying half. The quiet half is worse — the rule also goes silent on a genuine orphan, which is the defect the tool exists to catch:
Page 1 there ends on a two-line entry header with zero bullets under it.
masteris silent because the first bullet's bbox line is atdoc_leftitself, underINDENT_PT, soindented(first)is False.Neither shape is a knife edge. Sweeping the profile-statement length over 40 content lengths on the same document, the false positive appeared at 23 of them, and the false negative at 36 consecutive lengths.
What I tried, and the control that matters
Adding a glyph test alongside the indent test — a line whose text opens with a list marker is a list item whatever its indent — clears both shapes, and is inert on the shipped templates:
Both still report exactly what they report today (the stock CV's "p2 is the last page and 77% empty", which is the by-design case named in the docstring). It is inert because the stock template wraps each
\cventryin an outer itemize, so entry headers already sit atdoc_left + 11.43, pastINDENT_PT, and the stock bullets live inside the entry's own box, so a page break cannot separate a stock marker from its text. Across the stock CV, the stock cover letter and the two reproduction documents, only the reproduction documents' output changes.All CI gates pass on the patched tree (470 tests,
check_framework_version,security_guards).Two things I would want reviewed in any version of this:
not indented(last)), which would be more than a reorder. Unreachable on the stock template, but it would be a behaviour change, not a refactor.-and*should count as markers. A word-bearing un-indented line that merely starts with a hyphen would then suppress a real orphan report. A Unicode-only set (–—•‣▪●·) would be safer and still cover the case.The actual question
I am not asking you to take my patch as it stands. The question is narrower:
Should
verify_layout's orphan rule know what template is active?ACTIVE-TEMPLATEalready declares a compile command and a page limit. Three directions I can see:/add-templatestates its list geometry, andfind_orphansuses that instead of guessing from indent.find_orphansreports "orphan check skipped: non-stock template" rather than guessing — the same fail-loud-not-wrong posture the-bboxdegradation already takes withskipped:exit 2.Happy with "document it" as the answer. The reason I am raising it at all is that the current failure is silent in the direction that matters: the false negative looks exactly like a clean document.
Reproduction files are linked in the comment below.
All reactions