Skip to content

Conversation

@Ninglo
Copy link
Contributor

@Ninglo Ninglo commented Jul 26, 2024

Resolve #223920

@Ninglo Ninglo changed the title fix: update config editor.wordSegmenterLocales don't take effect in SCM and chat input editors Fix editor.wordSegmenterLocales configuration don't take effect in simpleWidget editors (like chat or SCM input Editor) Jul 26, 2024
@meganrogge meganrogge assigned roblourens and unassigned meganrogge Jul 26, 2024
]
],

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.)."),
Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Only anyOf:
image

With type and items here:
image


So, which approach do you think we should adopt?

  1. Just remove the type and items fields.
  2. Keep type, items, and anyOf.
  3. Keep type, items, and anyOf, and add a deprecationMessage for the object in anyOf where the type is 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
}

Copy link
Member

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')) {
Copy link
Member

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'

Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alexdima alexdima requested a review from lszomoru July 15, 2025 20:51
@alexdima alexdima assigned lszomoru and unassigned alexdima Jul 15, 2025
@alexdima alexdima requested a review from roblourens July 15, 2025 20:51
@vs-code-engineering vs-code-engineering bot added this to the July 2025 milestone Jul 15, 2025
@lszomoru lszomoru merged commit e8ddbe4 into microsoft:main Jul 22, 2025
27 of 28 checks passed
@Ninglo Ninglo deleted the feat/wordSegmenterLocales branch September 3, 2025 11:55
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Sep 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

editor.wordSegmenterLocales configuration don't take effect in simpleWidget editors (like chat or SCM input Editor)

6 participants