Add a locale to the alexa remote capability #121636

CrazyMan2000 · 2024-07-09T23:33:39Z

Proposed change

This PR adds the feature to use the locale from the alexa configuration when creating non default capability labels.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

…g locale. This local is read from the users config.

home-assistant · 2024-07-09T23:33:46Z

Hey there @home-assistant/cloud, @ochlocracy, @jbouwh, mind taking a look at this pull request as it has been labeled with an integration (alexa) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of alexa can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign alexa Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

jbouwh · 2024-07-13T14:26:42Z

homeassistant/components/alexa/resources.py

    def add_mode(self, value: str, labels: list[str]) -> None:
+        """Add mode to the supportedModes object."""
+        self.add_mode_with_locale(value, {label: "" for label in labels})
+
+    def add_mode_with_locale(self, value: str, labels: dict[str, str]) -> None:
        """Add mode to the supportedModes object."""
        self._supported_modes.append({"value": value, "labels": labels})


We do we implement 2 methods here as we do not seem to use the empty label.

Sorry i think i don't fully understand you.
So we have 2 methods here, one to add just a label and one to add the label with a locale. Because the _supported_modes attribute does only accept a label with a locale a placeholder is set as locale.
Later when the _supported_modes field is added to the capabilities object the placeholder is replaced by the default locale en-US. See line 244 in resources.py.
I did this because then there is only one place that uses the default locale. If you think that is confusing i can just replace the placeholder with the default locale.

jbouwh · 2024-07-13T14:26:55Z

homeassistant/components/alexa/resources.py

    def add_preset(self, value: float, labels: list[str]) -> None:
+        """Add preset to configuration presets array."""
+        self.add_preset_with_locale(value, {label: "" for label in labels})
+
+    def add_preset_with_locale(self, value: float, labels: dict[str, str]) -> None:
        """Add preset to configuration presets array."""
        self._presets.append({"value": value, "labels": labels})


jbouwh

Beside of some tests that fail. Be aware that predefined labels in Home Assistant are English. An integration can add backend translations to the states. I am not sure if we should set the locale to the labels as it should be the job of the integration to do the translations IMO.

home-assistant · 2024-07-13T14:33:40Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

CrazyMan2000 · 2024-07-13T14:58:55Z

Beside of some tests that fail.

I don't think they are related to my PR, do you? At least it does not look like they are.

Everything is fine after merging the latest changes.

CrazyMan2000 · 2024-07-13T15:14:07Z

Be aware that predefined labels in Home Assistant are English. An integration can add backend translations to the states. I am not sure if we should set the locale to the labels as it should be the job of the integration to do the translations IMO.

I think the problem are not the predefined labels but the generic ones. Especially the remote can have any name for an activity. Those activity names are most probably not english but in the language of the user. That's why i added the users locale to the label.
I agree with you that this feature needs to be used very carefully because most of the labels are english. But since some are not this feature allows to change the locale for those special labels.

jbouwh · 2024-07-13T17:03:25Z

The language of the user is not always the locale in Alexa, As e.g. Dutch is not supported.
The CI failures could be related, not sure what triggers them. Did you run the tests locally?

CrazyMan2000 · 2024-07-13T17:15:39Z

The CI failures could be related, not sure what triggers them. Did you run the tests locally?

Sure. Tests work fine now after a merge with dev.

CrazyMan2000 · 2024-07-13T17:17:41Z

The language of the user is not always the locale in Alexa, As e.g. Dutch is not supported.

Hm okay, but wouldn't it still make more sense to set the Alexa locale as the labels locale instead of the default english one?

jbouwh · 2024-07-13T17:22:14Z

The language of the user is not always the locale in Alexa, As e.g. Dutch is not supported.

Hm okay, but wouldn't it still make more sense to set the Alexa locale as the labels locale instead of the default english one?

Well as integrations also should try to localise and we are aiming to use English in the base, it seems obvious the activity could have translations in the integration. So that is why I think we should not go this path

CrazyMan2000 · 2024-07-13T17:31:53Z

Okay i see what you mean.

Maybe instead of assuming which local should be used we could let the user configure it.
I think there about an entity configuration just like the name and the description, but instead let the user map an activity name to a label and a locale. That would basically mimic the alexa api and would allow the user to specify its own alexa name for an activty.

Something like that:

entity_config:
      remote.remote_entity:
        name: "Custom Name for Alexa"
        activities:
          TV:
            label: Fernseher
            local: de-DE

And if this configuration is not available we could just use the english locale with the activity name (just like its done right now).

jbouwh · 2024-07-13T21:28:09Z

Okay i see what you mean.

Maybe instead of assuming which local should be used we could let the user configure it. I think there about an entity configuration just like the name and the description, but instead let the user map an activity name to a label and a locale. That would basically mimic the alexa api and would allow the user to specify its own alexa name for an activty.

Something like that:
entity_config:
      remote.remote_entity:
        name: "Custom Name for Alexa"
        activities:
          TV:
            label: Fernseher
            local: de-DE
And if this configuration is not available we could just use the english locale with the activity name (just like its done right now).

I see what you mean, but IMO we should not create special functionality for locales in this case. I'll add a tag to ask for a second opinion.

CrazyMan2000 · 2024-07-14T09:43:14Z

IMO we should not create special functionality for locales in this case.

May I ask you why not?

The benefit from using the right locale with a label is that Alexa is performing way better.
That is not a problem when the label is one of the predefined ones from Alexa, because Alexa already has multiple translations for those labels (I think they are called assets). But when using a text as label Alexa must understand it correctly and that is working way better when setting the right locale with the text.

jbouwh · 2024-07-14T10:29:09Z

Okay i see what you mean.
Maybe instead of assuming which local should be used we could let the user configure it. I think there about an entity configuration just like the name and the description, but instead let the user map an activity name to a label and a locale. That would basically mimic the alexa api and would allow the user to specify its own alexa name for an activty.
Something like that:
entity_config:
      remote.remote_entity:
        name: "Custom Name for Alexa"
        activities:
          TV:
            label: Fernseher
            local: de-DE
And if this configuration is not available we could just use the english locale with the activity name (just like its done right now).
I see what you mean, but IMO we should not create special functionality for locales in this case. I'll add a tag to ask for a second opinion.

https://github.com/home-assistant/architecture/blob/master/adr/0007-integration-config-yaml-structure.md: We should not increase disparity.

https://github.com/home-assistant/architecture/blob/master/adr/0009-translations-2.0.md: Remove support for integrations providing platform translations (that includes alexa).

https://github.com/home-assistant/architecture/blob/master/adr/0021-YAML-integration-configuration-deprecation-policy.md: Any additions to the YAML schema means we need to maintain the code, and when it changes, allow a deprecation period.
As for alexa remotes, I do not think introducing additional YAML config is warranted. It would open a path to extend yaml confog for other specific domains, which is very hard to maintain.

Finally:
entity_config is a generic schema mapping to be used for all domains, not only remote. IMO we should not extend it for configuration of specific entity attributes.

CrazyMan2000 added 2 commits July 9, 2024 21:35

Update the CapabilityResource to support labels with the correspondin…

ef6b694

…g locale. This local is read from the users config.

Add a test if the locale for the remote's modes is correctly set.

3bda332

home-assistant bot added cla-signed core has-tests integration: alexa new-feature labels Jul 9, 2024

home-assistant bot assigned ochlocracy and jbouwh Jul 9, 2024

home-assistant bot added the Quality Scale: No score label Jul 9, 2024

CrazyMan2000 marked this pull request as ready for review July 10, 2024 07:10

CrazyMan2000 requested review from ochlocracy and jbouwh as code owners July 10, 2024 07:10

jbouwh reviewed Jul 13, 2024

View reviewed changes

jbouwh requested changes Jul 13, 2024

View reviewed changes

home-assistant bot marked this pull request as draft July 13, 2024 14:33

Merge branch 'dev' into alexa-mode-controller-locale

c9fee34

jbouwh added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Jul 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a locale to the alexa remote capability #121636

Add a locale to the alexa remote capability #121636

CrazyMan2000 commented Jul 9, 2024 •

edited

Loading

home-assistant bot commented Jul 9, 2024

jbouwh Jul 13, 2024

CrazyMan2000 Jul 13, 2024

jbouwh Jul 13, 2024

jbouwh left a comment

home-assistant bot commented Jul 13, 2024

CrazyMan2000 commented Jul 13, 2024 •

edited

Loading

CrazyMan2000 commented Jul 13, 2024

jbouwh commented Jul 13, 2024

CrazyMan2000 commented Jul 13, 2024

CrazyMan2000 commented Jul 13, 2024

jbouwh commented Jul 13, 2024

CrazyMan2000 commented Jul 13, 2024

jbouwh commented Jul 13, 2024

CrazyMan2000 commented Jul 14, 2024

jbouwh commented Jul 14, 2024

Add a locale to the alexa remote capability #121636

Are you sure you want to change the base?

Add a locale to the alexa remote capability #121636

Conversation

CrazyMan2000 commented Jul 9, 2024 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Jul 9, 2024

jbouwh Jul 13, 2024

Choose a reason for hiding this comment

CrazyMan2000 Jul 13, 2024

Choose a reason for hiding this comment

jbouwh Jul 13, 2024

Choose a reason for hiding this comment

jbouwh left a comment

Choose a reason for hiding this comment

home-assistant bot commented Jul 13, 2024

CrazyMan2000 commented Jul 13, 2024 • edited Loading

CrazyMan2000 commented Jul 13, 2024

jbouwh commented Jul 13, 2024

CrazyMan2000 commented Jul 13, 2024

CrazyMan2000 commented Jul 13, 2024

jbouwh commented Jul 13, 2024

CrazyMan2000 commented Jul 13, 2024

jbouwh commented Jul 13, 2024

CrazyMan2000 commented Jul 14, 2024

jbouwh commented Jul 14, 2024

CrazyMan2000 commented Jul 9, 2024 •

edited

Loading

CrazyMan2000 commented Jul 13, 2024 •

edited

Loading