-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
picture cards: add person image support #20593
picture cards: add person image support #20593
Conversation
5bfedb7
to
53476d6
Compare
53476d6
to
73193a2
Compare
WalkthroughWalkthroughThe changes enhance the representation of "Paulus" and a battery sensor across various Lovelace cards by adding new entity configurations and refining the handling of person entities. The introduction of Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files skipped from review as they are similar to previous changes (2)
Additional comments not posted (7)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 2
73193a2
to
86b38ba
Compare
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.
Actionable comments posted: 0
Outside diff range comments (11)
gallery/src/pages/lovelace/picture-elements-card.ts (1)
Line range hint
22-22
: Detected a potential security issue with an API key exposed in the code. Consider using environment variables or secure storage mechanisms to handle sensitive keys.- access_token: "2f5bb163fb91cd8770a9494fa5e7eab172d8d34f4aba806eb6b59411b8c720b8", + access_token: process.env.DEMO_CAMERA_ACCESS_TOKEN,src/panels/lovelace/editor/config-elements/hui-picture-glance-card-editor.ts (1)
Line range hint
84-84
: Replace non-null assertions with optional chaining to prevent runtime errors.- this.hass!.localize(...) + this.hass?.localize(...)Also applies to: 88-88, 124-124, 137-137, 139-139, 143-143, 147-147
src/panels/lovelace/cards/hui-picture-elements-card.ts (2)
Line range hint
66-79
: Remove unnecessary else clauses to simplify control flow.- } else if (condition) { + } if (condition) {Also applies to: 77-79
Line range hint
201-201
: Replace non-null assertion with optional chaining to prevent runtime errors.- this._elements! + this._elements?src/panels/lovelace/cards/hui-picture-entity-card.ts (1)
Line range hint
163-163
: Replace non-null assertions with optional chaining to prevent runtime errors.- this.hass! + this.hass?Also applies to: 164-164, 226-226, 226-226, 226-226
src/panels/lovelace/cards/hui-picture-glance-card.ts (1)
Line range hint
105-105
: Address multiple instances of forbidden non-null assertions and excessive complexity detected in the code.- this.hass! + this.hass? - handleAction(this, this.hass!, config, ev.detail.action!); + handleAction(this, this.hass?, config, ev.detail.action);Also, consider refactoring the function to reduce its complexity.
Also applies to: 107-107, 117-117, 211-211, 212-212, 236-236, 241-241, 254-254, 265-265, 297-297, 315-315, 315-315, 316-316, 316-316
src/panels/lovelace/common/generate-lovelace-config.ts (5)
Line range hint
65-65
: Refactor to reduce cognitive complexity.The complexity of this function exceeds the recommended limit, potentially making maintenance and future updates more challenging.
Consider breaking down the function into smaller, more manageable functions or simplifying the logic where possible.
Line range hint
223-226
: Avoid assignments within expressions.Assignments within expressions can lead to code that is harder to read and maintain.
Consider revising these lines to separate the condition check from the assignment for clearer logic.
Also applies to: 237-240
Line range hint
130-130
: Address excessive function complexity.Several functions in this file exceed the recommended complexity limits, which could hinder understanding and maintenance.
Refactor these functions to simplify the logic, possibly by extracting parts of the code into separate functions or reducing the number of conditional branches.
Also applies to: 251-251, 349-349, 491-491
Line range hint
419-419
: Optimize by avoiding thedelete
operator.Using
delete
can lead to performance issues due to deoptimization of the underlying data structures in JavaScript engines.- delete allEntities[entity.entity_id]; + allEntities[entity.entity_id] = undefined;This change avoids the performance penalty associated with the
delete
operator.
Line range hint
620-620
: Replace non-null assertions with optional chaining.Non-null assertions can lead to runtime errors if the assumptions about data presence are incorrect.
[REFACTOR_Suggestion]- const grid = energyPrefs.energy_sources.find(source => source.type === "grid") as GridSourceTypeEnergyPreference; + const grid = energyPrefs.energy_sources.find(source => source.type === "grid")?. as GridSourceTypeEnergyPreference;This change adds a safety check to ensure that the object exists before attempting to access its properties.
Also applies to: 626-626
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.
Actionable comments posted: 1
export interface PersonEntity extends HassEntityBase { | ||
attributes: PersonEntityAttributes; | ||
} | ||
|
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.
Ensure the attributes
field from fetchPersons
is properly handled in all calling contexts.
Would you like me to help implement or verify this handling?
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.
yes
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.
@Quentame, it appears that the attributes
field from the PersonEntity
is not directly referenced or manipulated in the ha-config-person.ts
file or other related components in the src/panels/config/person/
directory. This suggests that the handling of the attributes
field might be abstracted elsewhere or not required in the current context of the fetchPersons
usage.
If there are specific functionalities or components where you expect the attributes
field to be used, please let me know, and I can further investigate those areas. Otherwise, the current implementation in ha-config-person.ts
seems to focus primarily on sorting and managing person data without directly interacting with the detailed attributes of PersonEntity
.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all files that call `fetchPersons` and display the relevant code snippets.
rg "fetchPersons" --context 5
Length of output: 2122
Script:
#!/bin/bash
# Search for direct references to the `attributes` field in `ha-config-person.ts` and other related components.
rg "attributes" --files-with-matches src/panels/config/person/
Length of output: 62
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.
Thank you for the contribution. 👍
Could you refactor person_entity
into image_entity
so that image_entity
can be either a person or a image entity in the picture element card and picture elements card? That would align it with the picture card where entity
is either a camera, image or person entity and be clear to the user that setting image_entity
will set the background of the card to the image of the entity regardless if it's a person or image entity.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
@silamon |
@Quentame, you can provide multiple domains as a list (https://www.home-assistant.io/docs/blueprint/selectors/#entity-selector). Not sure if it works without the |
bf44289
to
b38d0f8
Compare
OK, thanks ! As my dev env is not ready to test that, I am leaving it as at the moment. |
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.
Actionable comments posted: 1
src/panels/lovelace/editor/config-elements/hui-picture-glance-card-editor.ts
Outdated
Show resolved
Hide resolved
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've tested the code below, should be good to add:
src/panels/lovelace/editor/config-elements/hui-picture-glance-card-editor.ts
Outdated
Show resolved
Hide resolved
src/panels/lovelace/editor/config-elements/hui-picture-card-editor.ts
Outdated
Show resolved
Hide resolved
image = computeImageUrl(stateObj as ImageEntity); | ||
break; | ||
case "person": | ||
image = (stateObj as PersonEntity).attributes.entity_picture; |
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.
Apparently, picture entity cards are already used for person entities. The default generated lovelace config adds picture entity cards for every known person. If that person entity doesn't have a picture, entity_picture is undefined and it should fallback to the this._config.image.
image = (stateObj as PersonEntity).attributes.entity_picture; | |
if ((stateObj as PersonEntity).attributes.entity_picture) { | |
image = (stateObj as PersonEntity).attributes.entity_picture; | |
} |
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.
Co-authored-by: Simon Lamon <[email protected]>
…mage (that works)
Doc updated to latest changes: home-assistant/home-assistant.io#32733 |
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. Thank you for working on this!
@Quentame Is there a way to diable this fuctionality, the person image does not support "moving" GIFs, my picture entity card used to be a looping gif, now its just a static image. Update: |
Proposed change
Add person profile picture support for picture cards, this avoid finding ths profile picture URL in Web Dev Tools, and doing so every time the profile pic is changed/updated.
Picture Card
Picture Elements Card
Picture Entity Card
Picture Glance Card
Type of change
Example configuration
Already in
gallery/src/pages/lovelace
filesAdditional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
UI Enhancements
Configuration
Translations