fix(editor): lay out wrapped text once per row instead of once per prefix - #2165
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…uadratic # Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What you see now
A very long single line no longer makes the app balloon in memory and stall or get killed when word
wrap is on. Wrapped text is laid out once instead of once per wrapped row.
Found while investigating #2158. Different subsystem and different trigger, so it ships on its own.
Root cause
suggestLineBreakreturns an absolute offset into the run being typeset, not a length:CTTypesetter+SuggestLineBreak.swift:54:breakIndex = startingOffset + CTTypesetterSuggestClusterBreak(...):78: the word-break path does the same withsubrange.location + ...:17says "An offset relative to the entire string"Typesetter.swift:184passed that straight through as anNSRangelength:On the first fragment
startOffsetis 0, so the two coincide and nothing looks wrong. From the secondfragment on, the typeset range is too long by exactly
startOffset, so fragment k typesets roughlyk × w characters instead of w.
CTTypesetterCreateLineonly clamps at the end of the string, sonothing catches it. Every over-long
CTLineis then retained inLineFragment.contents(
TypesetContext.swift:51), which makes both the glyph work and the retained memory grow with thesquare of the line length.
Fragment ranges were never affected:
appendTextadvances withlineBreak + typesettingRange.location(TypesetContext.swift:56), which is correct under absolutesemantics. That is why the existing wrap test passes today and why this went unnoticed. Only the
CTLine range, its measured width, and what derives from them were wrong.
Measured on this fix's own test shape (1,000 chars at
maxWidth150): 10,920 glyphs before, 1,000after. A prior lane measured 260,000 chars producing ~160.9M glyphs instead of 260,000.
The change
One expression, matching how
appendTextalready advances:This is identical to upstream's open PR CodeEditApp/CodeEditTextView#122 ("Fix Word Duplication at
Wrap Boundaries"), which patches the same blob. The user-visible bug upstream reports, duplicated
words at wrap boundaries, is the drawing half of the same defect.
Deliberately not clamped with
max(0, ...): aCFRangelength of 0 means "to the end of thestring", so a zero clamp would silently restore the huge line.
lineBreak > startOffsetholdsanyway, because
CTTypesetterSuggestClusterBreakreturns at least 1 for any offset below the length,and the
currentPosition < range.maxloop guard excludes the equal case.Also deliberately untouched: the
if lineBreak == 1guard on the next line has the same unitsconfusion, but a length-based guard there can spin forever, since
popCurrentData()pluscontinuedoes not advance
currentPosition. It needs its own analysis and is recorded as follow-up.Reachability
Needs
wrapLines = true, otherwisemaxLineLayoutWidthis.greatestFiniteMagnitude(
TextLayoutManager.swift:115), the loop runs once per run, andstartOffsetstays 0.Always-wrapped surfaces:
JSONCodeEditor.swift:46,AIChatCodeBlockView.swift:200,SQLReviewSheet.swift:303. The SQL editor wraps only when the Word Wrap setting is on, whichdefaults to off (
EditorSettings.swift:84). The minimap mirrors the text view. Severity scales withthe longest single line, so a long JSON value or a big one-line paste is the worst case.
Files
LocalPackages/CodeEditTextView/Sources/CodeEditTextView/TextLine/Typesetter/Typesetter.swiftTableProTests/Views/Editor/TypesetterWrapLengthTests.swift(new)CHANGELOG.mdVerification
generate,buildtest TypesetterWrapLengthTestspre-fixtest TypesetterWrapLengthTests GutterHighlightTests SQLEditorCoordinatorTestsafterswift test --filter TypesetterTests(the package's own suite)swiftlintThe new suite lives in
TableProTestsbecause the TablePro scheme does not runCodeEditTextViewTests, so a test there would never gate a regression. It asserts an algorithmicinvariant, not wall-clock time, so it cannot flake on a loaded machine:
Upstream's 9 existing typesetter tests still pass, including the wrap test that asserts fragment
ranges, which confirms fragment ranges are unchanged.
Limitations
re-measured here. What this PR measured directly is the glyph count, 10,920 vs 1,000 on the test
shape.
Typesetter.swift, which is otherwise byte-identical toupstream. A future package sync must not revert it. Upstream PR Remove Create Table UI feature #122 is still open.
LineFragmentViewsizing andmaxLineWidth. Existing attachment tests pass, but they use short strings wherestartOffsetis 0.as not started rather than as passed.