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
Allows settings to override language contributed bracket pair configurations. #134503
Conversation
@hediet Could you please fix the tests. |
d9336b0
to
df994ef
Compare
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 closingBrackets = new Map</* closingText */ string, { openingBrackets: SmallImmutableSet<OpeningBracketId>, first: OpeningBracketId }>(); | ||
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
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.
@@ -27,6 +27,7 @@ import { URI } from 'vs/base/common/uri'; | |||
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: