SD-1830 - use fragment.lines for click position in multi-column layouts#1963
SD-1830 - use fragment.lines for click position in multi-column layouts#1963mattConnHarbour wants to merge 1 commit intomainfrom
Conversation
mattConnHarbour
force-pushed
the
SD-1830
branch
from
c7d5002 to
f018a4c
Compare
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
nice one @mattConnHarbour!
let's try to simplify a bit the code here
|
|
||
| const line = lines[lineIndex]; | ||
| // Convert to absolute index when using fragment-local lines | ||
| if (lines === fragment.lines) { |
There was a problem hiding this comment.
minor: using reference equality (lines === fragment.lines) for branch detection is fragile -- if someone copies the array, this silently breaks
a const isRemeasured boolean flag set at the branch point would be more explicit.
low risk but either way worth double checking
|
|
||
| expect(result).not.toBeNull(); | ||
| expect(result?.blockId).toBe('two-column-para'); | ||
| expect(result?.pos).toBeGreaterThanOrEqual(1); |
There was a problem hiding this comment.
the range assertions (1-12) accept any position in the paragraph -- they prove the crash is fixed but wouldn't catch a regression where clicking line 2 ("world") maps to a line 1 position.
tightening to expect(result?.pos).toBeGreaterThanOrEqual(7) for the second-line test would verify the correct line was hit.
| // Use fragment-specific lines when available (remeasured fragments in multi-column layouts). | ||
| // When a paragraph is remeasured for column width, fragment.lines contains the actual | ||
| // rendered lines which may differ from measure.lines. | ||
| let lines: Line[]; |
There was a problem hiding this comment.
nit: the renderer already handles this in one line (renderer.ts:2034):
const lines = fragment.lines ?? measure.lines.slice(fragment.fromLine, fragment.toLine);
this would replace the 3 let declarations + if/else block, and findLineIndexAtY can just take (lines, pageY, 0, lines.length).
the slice cost is negligible for a click handler. also removes the need for the reference equality check at line 991.
| lineIndex = fragment.fromLine + lineIndex; | ||
| } | ||
|
|
||
| // Guard against undefined line (defensive check) |
There was a problem hiding this comment.
findLineIndexAtY already returns null when lines[i] is undefined (line 2127-2128), and if it returns a non-null index, every line in the range was verified to exist during iteration.
this guard can never trigger -- removing it avoids making a reader wonder "wait, can this actually be undefined?"
There was a problem hiding this comment.
stale jsdoc: @parammeasure - The paragraph measure containing line data -- the parameter was renamed to lines: Line[] but the doc still references measure.
2 participants
No description provided.