-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Separate entity, device and area name in more info dialog header #21951
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several utility functions for computing names associated with areas, devices, and entities in a Home Assistant environment. New files are created to define these functions, which enhance the retrieval and presentation of entity-related information. Additionally, modifications are made to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
38e8471
to
5456a65
Compare
5456a65
to
8464b00
Compare
8464b00
to
88f84f7
Compare
0e076ec
to
8f597ff
Compare
8f597ff
to
7ba7a74
Compare
@@ -0,0 +1,5 @@ | |||
import type { DeviceRegistryEntry } from "../../data/device_registry"; | |||
|
|||
export const computeDeviceName = ( |
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.
We already have a computeDeviceName
function https://github.com/home-assistant/frontend/blob/dev/src/data/device_registry.ts#L68
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.
Yeah, in my opinion this function does too much with the entities fallback and translation fallback.
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.
entities is optional, we could make the translation fallback optional too. We can also wrap the functions (have a computeDeviceNameWithFallback()
), but we can not have 2 functions with the same name in the code base, that will go wrong with importing and is highly confusing.
@@ -24,6 +24,7 @@ export interface EntityRegistryDisplayEntry { | |||
translation_key?: string; | |||
platform?: string; | |||
display_precision?: number; | |||
has_entity_name?: boolean; |
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 doesn't seem used now right?
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.
It was used in entity picker but I removed it from this PR. I will be useful if we want to compute something similar to friendly_name in the front-end directly.
Co-authored-by: Bram Kragten <[email protected]>
entry: EntityRegistryDisplayEntry | EntityRegistryEntry, | ||
hass: HomeAssistant | ||
): string | undefined => { | ||
const name = entry.name || undefined; |
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 think, not 100% sure, that behavior between EntityRegistryDisplayEntry
and EntityRegistryEntry
is different here, EntityRegistryDisplayEntry
will use EntityRegistryEntry.name
and if that is not available use EntityRegistryEntry.original_name
I think.
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.
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 will check that in detail. May be we should only use the EntityRegistryDisplayEntry
.
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.
@bramkragten I added fallback to original name so we can also use EntityRegistryEntry
. I checked the backend and the original_name
fallback is only for EntityRegistryDisplayEntry
.
) => { | ||
for (const entity of entities || []) { | ||
const entityId = typeof entity === "string" ? entity : entity.entity_id; | ||
const stateObj = hass.states[entityId]; |
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 guess this should use entry.name || entry.original_name
first, but maybe for later
|
||
// If the device name is the same as the entity name, consider empty entity name | ||
if (deviceName === name) { | ||
return undefined; |
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.
Isn't it weird that if we request the entity name we get undefined if the name is the same as the device? I feel like the default behavior of computeEntityName
should return the name always, and not returning it would be another function or option?
Proposed change
An attempt to improve more info dialog for entities.
For now it uses the backward compatibility with
friendly_name
. We should use entry registry for that buthas_entity_name
should be exposed inEntityRegistryDisplayEntry
for that.Needs home-assistant/core#125832
More info dialog
Some examples depending of the different combinations (entity, device, area)
entity, device, area

entity, device, no area

no entity name, device, area

entity, no device, area

entity only

Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
MoreInfoDialog
to display more structured information, including device and area names alongside the main entity name.Bug Fixes
Style
MoreInfoDialog
with updated styling for title and subtitle presentation.