-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Allows settings to override language contributed bracket pair configurations. #134503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@hediet Could you please fix the tests. |
d9336b0 to
df994ef
Compare
alexdima
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left notes, we can also discuss.
src/vs/editor/common/model/bracketPairColorizer/bracketPairColorizer.ts
Outdated
Show resolved
Hide resolved
| const openingBrackets = new Set</* openingText */ string>(); | ||
|
|
||
| for (const [openingText, closingText] of brackets) { | ||
| if (openingText === '' || closingText === '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to move the validation of brackets (i.e. that empty brackets are not allowed to ResolvedLanguageConfiguration). Otherwise, we would need to check for empty strings in all consumers of ResolvedLanguageConfiguration.
src/vs/workbench/contrib/codeEditor/browser/workbenchReferenceSearch.ts
Outdated
Show resolved
Hide resolved
src/vs/editor/standalone/browser/referenceSearch/standaloneReferenceSearch.ts
Outdated
Show resolved
Hide resolved
| return this._brackets; | ||
| } | ||
| function getCustomizedLanguageConfig(languageIdentifier: LanguageIdentifier, resource: URI | undefined, configurationService: IConfigurationService): LanguageConfiguration { | ||
| // TODO how should these keys be registered to the JSON schema? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can define them together with the other non-editor options yet still editor.* settings at
| const editorConfiguration: IConfigurationNode = { |
| this.config = computeConfig(this.languageId, this.resource, this.configurationService, this.modeService); | ||
|
|
||
| this._register(this.configurationService.onDidChangeConfiguration((e) => { | ||
| this.update(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!e.affectsConfiguration('editor')) {
return;
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do it like this now:
const customizedLanguageConfigKeys = {
brackets: 'editor.language.brackets',
colorizedBracketPairs: 'editor.language.colorizedBracketPairs'
};
// ...
const languageConfigKeys = new Set(Object.values(customizedLanguageConfigKeys));
const globalConfigChanged = e.change.keys.some((k) =>
languageConfigKeys.has(k)
);
const localConfigChanged = e.change.overrides
.filter(([overrideLangName, keys]) =>
keys.some((k) => languageConfigKeys.has(k))
)
.map(([overrideLangName]) => this.modeService.getLanguageIdentifier(overrideLangName));
if (globalConfigChanged) {
this.configurations.clear();
this.onDidChangeEmitter.fire(new LanguageConfigurationServiceChangeEvent(undefined));
} else {
for (const languageIdentifier of localConfigChanged) {
if (languageIdentifier) {
this.configurations.delete(languageIdentifier.id);
this.onDidChangeEmitter.fire(new LanguageConfigurationServiceChangeEvent(languageIdentifier));
}
}
}eeaa845 to
c3e8f38
Compare
10aef2b to
6650201
Compare
# Conflicts: # src/vs/editor/common/modes/languageConfigurationRegistry.ts
alexdima
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good! Just one last problem I just noticed.
| import { IUndoRedoService } from 'vs/platform/undoRedo/common/undoRedo'; | ||
| import { Iterable } from 'vs/base/common/iterator'; | ||
| import { ResourceFileEdit } from 'vs/editor/browser/services/bulkEditService'; | ||
| import { TestLanguageConfigurationService } from 'vs/editor/test/common/modes/testLanguageConfigurationService'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a test, we should not use the TestLanguageConfigurationService here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
This PR fixes #131412
TODOs: