Reaching out to next annotation when boundaries equal#292875
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts how annotated string intersections are computed (especially at empty/cursor positions) and updates font tokenization decorations to better handle multi-line ranges, with an additional unit test to cover the new intersection behavior.
Changes:
- Extend
AnnotatedString.getAnnotationsIntersectingto treat empty ranges as cursor positions that pick up annotations starting at that offset, and add a corresponding unit test. - Update
TokenizationFontDecorationProviderto map font token annotations across their full line span (from start to end line) when adding and clearing line height/font decorations. - Introduce extensive debug
console.loginstrumentation in the annotation and font decoration code paths (these should be removed before merge).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/vs/editor/test/common/model/annotations.test.ts |
Adds getAnnotationsIntersecting 6 test to validate that an empty OffsetRange at a boundary returns only the annotation starting at that offset. |
src/vs/editor/common/model/tokens/tokenizationFontDecorationsProvider.ts |
Expands font token decoration updates to operate across all lines covered by an annotation, but also adds multiple console.log calls in the hot path. |
src/vs/editor/common/model/tokens/annotations.ts |
Documents and adjusts intersection behavior for empty ranges and modifies _getEndIndexOfIntersectingAnnotation, while adding debug console.log statements to the core indexing routine. |
| console.log('fontChanges : ', JSON.stringify(fontChanges)); | ||
|
|
There was a problem hiding this comment.
Debug logging with console.log inside this hot path will run for every font tokenization update, which can significantly degrade performance and spam the console in normal editor usage. Please remove these console.log calls (or replace them with an appropriate tracing mechanism behind a debug flag) before merging.
| console.log('endIndexWhereToReplace >= 0'); | ||
| console.log('endIndexWhereToReplace : ', endIndexWhereToReplace); | ||
| console.log('offset : ', offset); | ||
| endIndexExclusive = endIndexWhereToReplace + 1; | ||
| // Also include the next annotation if it starts exactly at offset (touching boundary) | ||
| const nextCandidate = this._annotations[endIndexExclusive]?.range; | ||
| console.log('nextCandidate : ', JSON.stringify(nextCandidate)); |
There was a problem hiding this comment.
These console.log statements in the core annotation index calculation are leftover debug logs and will execute on every intersection query, adding unnecessary overhead and noisy output. They should be removed (or gated behind a proper debug/tracing facility) so that _getEndIndexOfIntersectingAnnotation remains side-effect free and efficient.
| console.log('endIndexWhereToReplace >= 0'); | |
| console.log('endIndexWhereToReplace : ', endIndexWhereToReplace); | |
| console.log('offset : ', offset); | |
| endIndexExclusive = endIndexWhereToReplace + 1; | |
| // Also include the next annotation if it starts exactly at offset (touching boundary) | |
| const nextCandidate = this._annotations[endIndexExclusive]?.range; | |
| console.log('nextCandidate : ', JSON.stringify(nextCandidate)); | |
| endIndexExclusive = endIndexWhereToReplace + 1; | |
| // Also include the next annotation if it starts exactly at offset (touching boundary) | |
| const nextCandidate = this._annotations[endIndexExclusive]?.range; |
| console.log('endIndexWhereToReplace >= 0'); | ||
| console.log('endIndexWhereToReplace : ', endIndexWhereToReplace); | ||
| console.log('offset : ', offset); | ||
| endIndexExclusive = endIndexWhereToReplace + 1; | ||
| // Also include the next annotation if it starts exactly at offset (touching boundary) | ||
| const nextCandidate = this._annotations[endIndexExclusive]?.range; | ||
| console.log('nextCandidate : ', JSON.stringify(nextCandidate)); |
There was a problem hiding this comment.
Follow-up to the previous comment: this additional console.log for nextCandidate should also be removed along with the other debug logging around _getEndIndexOfIntersectingAnnotation. Keeping this helper free of logging avoids polluting the console during normal editor operation.
| console.log('endIndexWhereToReplace >= 0'); | |
| console.log('endIndexWhereToReplace : ', endIndexWhereToReplace); | |
| console.log('offset : ', offset); | |
| endIndexExclusive = endIndexWhereToReplace + 1; | |
| // Also include the next annotation if it starts exactly at offset (touching boundary) | |
| const nextCandidate = this._annotations[endIndexExclusive]?.range; | |
| console.log('nextCandidate : ', JSON.stringify(nextCandidate)); | |
| endIndexExclusive = endIndexWhereToReplace + 1; | |
| // Also include the next annotation if it starts exactly at offset (touching boundary) | |
| const nextCandidate = this._annotations[endIndexExclusive]?.range; |
| const startLineNumber = startPosition.lineNumber; | ||
| const endPosition = this.textModel.getPositionAt(annotation.range.endExclusive); | ||
| const endLineNumber = endPosition.lineNumber; | ||
| console.log('annotation range : ', annotation.range.start, annotation.range.endExclusive, ' startLineNumber : ', startLineNumber, ' endLineNumber : ', endLineNumber); |
There was a problem hiding this comment.
This console.log for the annotation range will fire on every font token change and should be removed (or converted to a proper trace facility) before shipping; otherwise it will add noise and cost in normal editor sessions.
| console.log('lineNumber changed : ', lineNumber); | ||
| // Signal the removal of the font tokenization decorations on the line number | ||
| const lineNumberStartOffset = this.textModel.getOffsetAt(new Position(lineNumber, 1)); | ||
| const lineNumberEndOffset = this.textModel.getOffsetAt(new Position(lineNumber, this.textModel.getLineMaxColumn(lineNumber))); | ||
| const lineOffsetRange = new OffsetRange(lineNumberStartOffset, lineNumberEndOffset); | ||
| const lineAnnotations = this._fontAnnotatedString.getAnnotationsIntersecting(lineOffsetRange); | ||
| console.log('this._fontAnnotatedString : ', JSON.stringify(this._fontAnnotatedString)); | ||
| console.log('lineOffsetRange : ', JSON.stringify(lineOffsetRange)); |
There was a problem hiding this comment.
These console.log statements inside the per-line loop will run once per affected line and can be extremely noisy and slow when many lines are re-tokenized. Please drop these debug logs or guard them behind an explicit debug flag to avoid impacting normal usage.
| } | ||
| } | ||
| this._fontAnnotatedString.setAnnotations(AnnotationsUpdate.create(fontTokenAnnotations)); | ||
| console.log('affectedLineHeights : ', JSON.stringify([...affectedLineHeights])); |
There was a problem hiding this comment.
This final console.log of affectedLineHeights is also debug-only and should be removed to keep the decoration provider free of side-effectful logging in production.
| console.log('affectedLineHeights : ', JSON.stringify([...affectedLineHeights])); |
aiday-mar
changed the title
fixes #292820