Add entity translation strings for ScreenLogic #130708
Conversation
|
Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| "calcium_hardness": { | ||
| "name": "Calcium Hardness" | ||
| }, | ||
| "cya": { | ||
| "name": "Cyanuric Acid" | ||
| }, | ||
| "total_alkalinity": { | ||
| "name": "Total Alkalinity" | ||
| }, | ||
| "salt_tds_ppm": { |
There was a problem hiding this comment.
FYI, the names need to be Sentence case
There was a problem hiding this comment.
Ah, ok. And it's ok that this would be a change from the current Title Case?
There was a problem hiding this comment.
So, the user-selected names that come from the device are all Title-Case. Do I need to filter all of those down to Sentence-case?
| ), | ||
| ScreenLogicPushBinarySensorDescription( | ||
| subscription_code=CODE.CHEMISTRY_CHANGED, | ||
| data_root=(DEVICE.INTELLICHEM, GROUP.ALERT), | ||
| key=VALUE.ORP_LIMIT, | ||
| translation_key="chem_limit", | ||
| translation_placeholders={"chem": "ORP"}, |
There was a problem hiding this comment.
What's ORP? Should we write it out? Does pH have different translations for other languages? Maybe it would make sense to keep them as part of the translation string
There was a problem hiding this comment.
ORP stands for Oxidation Reduction Potential. Neither appear to be translated for other languages. Looking at the Pentair Europe site and cycling through the available languages, neither ORP or pH are ever referred to by their full name, or translated.
| @@ -55,7 +55,8 @@ def __init__( | |||
| self._data_path = (*self.entity_description.data_root, self._data_key) | |||
| mac = self.mac | |||
| self._attr_unique_id = f"{mac}_{generate_unique_id(*self._data_path)}" | |||
| self._attr_name = self.entity_data[ATTR.NAME] | |||
| if not entity_description.translation_key: | |||
| self._attr_name = self.entity_data[ATTR.NAME] | |||
There was a problem hiding this comment.
Currently all entities get their name from the api. The referenced change leaves that convention in place for entities that get their name from the device (user named circuits/features), which as it applies to the integration, are switch and light entities, and sensor entities for the pump(s), which are named by the api after the first circuit associated with them in the device.
There was a problem hiding this comment.
Can you add some examples with what you want to achieve where? With translations set, you usually don't need to use _attr_name anymore, except for when the entity needs to be the device name
There was a problem hiding this comment.
The api lib screenlogicpy (which I maintain) originally provided the majority of data that HA uses, for each value intended to be supported in HA.
Example of data provided by the api lib:
"air_temperature": {
"name": "Air Temperature",
"value": 62,
"unit": "\u00b0F",
"device_type": "temperature",
"state_type": "measurement"
},
"ph": {
"name": "pH",
"value": 7.5,
"unit": "pH",
"state_type": "measurement"
},
"orp": {
"name": "ORP",
"value": 726,
"unit": "mV",
"state_type": "measurement"
},
Since moving to EntityDescriptions though, the majority of that extra data is no longer used, except for the name. Prior to this PR adding entity translations, all entities were sill using the name provided by the api lib. That's where the self._attr_name = self.entity_data[ATTR.NAME] came from.
Some of the entities this integration exposes (like the example above) are statically named, meaning the integration knows ahead of time what value it is exposing, but there are other entities that are created dynamically. Because the ScreenLogic gateway device supports various equipment configurations there can be different amounts of circuits, pumps, and heaters (along with other equipment either present or not). Additionally, the pool controller allows the user to custom name the individual circuits within the device. It is intended for entities that expose these circuits (switch, light, and some sensor) to use the name the user has set in the pool controller device.
Therefore for the entities known ahead of time I've provided translations for them, but for the ones not known ahead of time (where I haven't provided a translation_key), the integration "falls-back" to the name provided by the api lib, which is what the end user has set within the device itself.
Sorry it's a bit long-winded, but I thought it might be helpful to have a little context as to how the code got to where it is now.
There was a problem hiding this comment.
Okay this explains it well. I'll check the code on how we can do this the best. How do the devices currently look in HA? Is every pump its own device? Or is everything thrown under 1 device?
There was a problem hiding this comment.
Everything is currently under one device.
There's a mix of conventions within the Pentair ScreenLogic system that made breaking out individual devices messy and in my opinion confusing. Some equipment is presented as a stand-alone device, some are only referenced through the central controller, and some are presented as multiple devices that are only active depending on what mode the central controller is in.
Because the official app presents as one point of connection/device I elected to start there.
There was a problem hiding this comment.
Ideally we have one device per physical device. I don't think there's a big lack of user experience if we can't do that and just put it on the controller when that's not possible. How do you mean with the device being active?
Using devices is quite a nice way to not use a lot of placeholders. We can use via_device to neatly connect them to the main controller.
(Also, I don't see this as a must, if we can't do that and the user experience is worse, we shouldn't, I'm just exploring at this point :) )
Proposed change
screenlogic.config_entryname+description to common.Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: