Conversation
…mental highlighting (#649) NSTextStorage.editedRange resets to NSNotFound after processEditing(), but textDidChange fires after processEditing. This meant editedRange was always nil in textDidChange, causing every keystroke to trigger a full re-highlight instead of incremental. For large files (>50KB) this caused visible highlight disappearance during editing. Fix: add NSTextStorageDelegate to Coordinator that captures editedRange in textStorage(_:didProcessEditing:range:changeInLength:) before it resets. textDidChange now consumes this pendingEditedRange for proper incremental highlighting via highlightEditedAsync.
batonogov
left a comment
There was a problem hiding this comment.
REQUEST CHANGES (self-review)
1. lineStartsCache тоже нуждается в pendingEditedRange (и pendingChangeInLength)
PR правильно идентифицирует root cause: NSTextStorage.editedRange уже NSNotFound к моменту textDidChange. Но блок lineStartsCache (строки 1025-1036 в main) также читает storage.editedRange и storage.changeInLength напрямую:
if var cache = lineStartsCache,
let storage = textView.textStorage,
storage.editedRange.location != NSNotFound { // ← всегда NSNotFound!
cache.update(
editedRange: storage.editedRange,
changeInLength: storage.changeInLength,
...После этого PR инкрементальное обновление lineStartsCache никогда не сработает — всегда будет full rebuild. Технически этот баг существовал и до PR, но раз мы чиним root cause, надо чинить всё.
Fix: Захватывать changeInLength вместе с editedRange в delegate:
var pendingEditedRange: NSRange?
var pendingChangeInLength: Int = 0
func textStorage(_ textStorage: NSTextStorage,
didProcessEditing editedMask: NSTextStorageEditActions,
range editedRange: NSRange, changeInLength delta: Int) {
if editedMask.contains(.editedCharacters), !isProgrammaticTextChange {
pendingEditedRange = editedRange
pendingChangeInLength = delta
}
}И использовать pendingEditedRange / pendingChangeInLength в блоке lineStartsCache:
if var cache = lineStartsCache, let er = pendingEditedRange {
cache.update(editedRange: er, changeInLength: pendingChangeInLength,
in: textView.string as NSString)
lineStartsCache = cache
} else {
lineStartsCache = LineStartsCache(text: textView.string)
}2. Конфликт с #653 (undo fix)
PR #653 вставляет isUndoRedoInProgress и значительно модифицирует textDidChange. Оба PR трогают один участок CodeEditorView.swift. Мерж-конфликт гарантирован — кто мержится вторым, разрешает руками. Не блокер, но учитывай порядок.
3. Всё остальное — ОК
NSTextStorageDelegateреализован корректно:didProcessEditing— правильный callbackeditedMask.contains(.editedCharacters)правильно отсекает attribute-only editsisProgrammaticTextChangeguard корректенpendingEditedRange = nilсбрасывается в обоих путях — утечки нетtextStorage.delegateставится после настройки текста- Тесты покрывают 4 кейса: capture, consume, programmatic clear, attribute-only skip
Вердикт: REQUEST CHANGES — исправить п.1 (lineStartsCache + changeInLength), остальное LGTM.
… update storage.editedRange is already reset to NSNotFound by the time textDidChange fires. Capture changeInLength alongside editedRange in NSTextStorageDelegate and use both pending values for the incremental lineStartsCache.update() call.
✅ Code Coverage: 71.6%Threshold: 70% Coverage is above the minimum threshold. Generated by CI — see job summary for detailed file-level breakdown. |
batonogov
left a comment
There was a problem hiding this comment.
APPROVE. pendingChangeInLength захватывается в NSTextStorageDelegate вместе с pendingEditedRange. lineStartsCache корректно использует pending-значения вместо storage.editedRange. Сброс в 3 местах (programmatic updateContentIfNeeded, programmatic textDidChange, нормальный textDidChange). 4 теста покрывают все пути.
Summary
NSTextStorage.editedRangeresets toNSNotFoundafterprocessEditing(), buttextDidChangefires afterprocessEditing. SoeditedRangewas alwaysnil, forcing every keystroke to use full re-highlight instead of incrementalhighlightEditedAsyncNSTextStorageDelegateand captureseditedRangeintextStorage(_:didProcessEditing:range:changeInLength:)before it resets.textDidChangeconsumes thispendingEditedRangefor proper incremental highlightingTest plan
pendingEditedRange_capturedByTextStorageDelegate— verifies delegate captures range on character editspendingEditedRange_consumedByTextDidChange— verifies textDidChange consumes and clears the rangependingEditedRange_clearedOnProgrammaticTextChange— verifies range is cleared during programmatic text replacementpendingEditedRange_notSetForAttributeOnlyEdits— verifies attribute-only edits (highlighting) don't set the range