-
Notifications
You must be signed in to change notification settings - Fork 37.4k
Fix editor.wordSegmenterLocales configuration don't take effect in simpleWidget editors (like chat or SCM input Editor)
#223921
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
… SCM and chat input editors
editor.wordSegmenterLocales don't take effect in SCM and chat input editorseditor.wordSegmenterLocales configuration don't take effect in simpleWidget editors (like chat or SCM input Editor)
| ] | ||
| ], | ||
|
|
||
| description: nls.localize('wordSegmenterLocales', "Locales to be used for word segmentation when doing word related navigations or operations. Specify the BCP 47 language tag of the word you wish to recognize (e.g., ja, zh-CN, zh-Hant-TW, etc.)."), |
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.
Why is this change needed?
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 see that we are missing the description and it should go here. You can also delete the other two descriptions. But I think we shouldn't have the type and items here since we have the anyOf?
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.
But I think we shouldn't have the type and items here since we have the anyOf?
If we only set anyOf for this schema, we can't update settings in settingsEditor2, which I find inconvenient.
Moreover, I don't think the type string | string[] is a good design. Since we set the default value to [], adding strings to this array is sufficient. The string type seems a bit redundant.
However, to maintain compatibility, we might have to keep anyOf for now.
So, which approach do you think we should adopt?
- Just remove the
typeanditemsfields. - Keep
type,items, andanyOf. - Keep
type,items, andanyOf, and add adeprecationMessagefor the object inanyOfwhere thetypeis string.
// 1. just remove `type` and `items` fields
const choice1 = {
anyOf: [
{
type: 'string',
},
{
type: 'array',
items: {
type: 'string'
}
}
],
// other properties
};
// 2. keep `type`, `items` **and** `anyOf`
const choice2 = {
anyOf: [
{
type: 'string',
},
{
type: 'array',
items: {
type: 'string'
}
}
],
type: 'array',
items: {
type: 'string',
},
// other properties
}
// 3. keep `type`, `items` **and** `anyOf`, and add `deprecationMessage` for object in `anyOf` which `type` is string
const choice3 = {
anyOf: [
{
type: 'string',
deprecated: true,
deprecationMessage: nls.localize('wordSegmenterLocales.deprecateStringType', "Use an array of strings instead."),
},
{
type: 'array',
items: {
type: 'string'
}
}
],
type: 'array',
items: {
type: 'string',
},
// other properties
}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.
2 doesn't really work, because setting the type at the top level seems to just override anyOf. Having this setting work in the settings editor is nice but I'll leave that to @alexdima, whether he wants to change the type.
| this._register(this.configurationService.onDidChangeConfiguration(e => { | ||
| if (e.affectsConfiguration(AccessibilityVerbositySettingId.Chat)) { | ||
| this.inputEditor.updateOptions({ ariaLabel: this._getAriaLabel() }); | ||
| } else if (e.affectsConfiguration('editor.wordSegmenterLocales')) { |
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 shouldn't be 'else if'
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.
LGTM


Resolve #223920