Retokenize on custom font token setting change#287577
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issues where changes to custom font token settings in editor.tokenColorCustomizations were not triggering retokenization in the editor. The fix ensures that when font-related properties (fontFamily, fontSize, lineHeight) are modified in token color customizations, both the main thread and worker thread tokenizers are properly updated and retokenize the content.
Changes:
- Extended token rule comparison to include font-related properties
- Added worker thread notification mechanism for custom token rule changes
- Implemented retokenization trigger in worker tokenizer when custom rules change
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/vs/workbench/services/textMate/browser/textMateTokenizationFeatureImpl.ts | Extended equalsTokenRules function to compare font properties (lineHeight, fontSize, fontFamily) in addition to existing properties |
| src/vs/workbench/services/textMate/browser/backgroundTokenization/worker/textMateWorkerTokenizer.ts | Added onTextmateCustomRulesChanged method to trigger tokenization reset when custom rules change |
| src/vs/workbench/services/textMate/browser/backgroundTokenization/worker/textMateTokenizationWorker.worker.ts | Added $acceptTextmateCustomRulesChanged method to handle custom rule changes and forward to the appropriate model |
| src/vs/workbench/services/textMate/browser/backgroundTokenization/textMateWorkerTokenizerController.ts | Added configuration change listener for editor.tokenColorCustomizations to notify the worker when changes occur |
| const s2 = r2.settings; | ||
| if (s1 && s2) { | ||
| if (s1.fontStyle !== s2.fontStyle || s1.foreground !== s2.foreground || s1.background !== s2.background) { | ||
| if (s1.fontStyle !== s2.fontStyle || s1.foreground !== s2.foreground || s1.background !== s2.background || s1.lineHeight !== s2.lineHeight || s1.fontSize !== s2.fontSize || s1.fontFamily !== s2.fontFamily) { |
There was a problem hiding this comment.
The line added to check font-related properties (lineHeight, fontSize, fontFamily) is very long and violates readability guidelines. Consider breaking this into multiple comparisons or extracting into a helper function for better maintainability.
alexdima
left a comment
alexdima
left a comment
There was a problem hiding this comment.
This doesn't seem like the right way to do this. IMHO this should be refreshed in the same way a color change or a rule change for syntax colors causes a refresh.
fixes #286154
fixes #284684