Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Logbook Card #6976

Merged
merged 10 commits into from
Nov 11, 2020
Merged

Logbook Card #6976

merged 10 commits into from
Nov 11, 2020

Conversation

zsarnett
Copy link
Contributor

@zsarnett zsarnett commented Sep 13, 2020 •
edited
Loading

Proposed change

image
image

Type of change

  • New feature (thank you!)
  • Code quality improvements to existing code or addition of tests

Example configuration

type: logbook
entities:
  - fan.ceiling_fan
  - fan.living_room_fan
  - light.ceiling_lights
hours_to_show: 24

Additional information

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

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

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

@zsarnett
Copy link
Contributor Author

zsarnett commented Sep 13, 2020 •
edited
Loading

Couple things:

  • Currently, in the Logbook Element we are basing the icon off of the current state of the entity. Not the state of the entity at the time of the logbook entry
    To do this, we need the logbook API to return the state that the entity was set to. Right now we return a message that includes the state but in a "was set to On" form.

  • We are unable, at least to my testing and knowledge, to specify more than one entity for the logbook. Its either 1 entity or all entities. I would like this card to work with multiple entities. Example: All motion entities.

@bdraco Worked with you recently to help us with the More info logbook. Would you be able to help me with these changes in the backend to better support this card and the logbook as a whole?

@bdraco
Copy link
Member

bdraco commented Sep 13, 2020

Getting the icon is expensive because we have to decode the json. If there can be only one icon in the attributes we can usually avoid the json decode. I think that's the case, but I need to validate that.

Can you provide a bit more detail on the multi entity use case?

@zsarnett
Copy link
Contributor Author

zsarnett commented Sep 13, 2020 •
edited
Loading

We are already decoding the object that is sent to the frontend. (I don't know much about the backend section) but if you have the was turned on message. I don't know how that's built but if you send in the last on as a state property we get the icon in the frontend. I don't know if that's what you mean when you say expensive.

Use case would be this card. Being able to see a log book for all Motion Sensor entities and building a security dashboard of sorts with the card. All Windows. We would be allowing the users to choose which entities to query for instead of just one or all of them

@bdraco bdraco mentioned this pull request Sep 13, 2020
Merged
21 tasks
@bdraco
Copy link
Member

bdraco commented Sep 13, 2020

For the icon, can you try

home-assistant/core#40039

@bdraco
Copy link
Member

bdraco commented Sep 13, 2020

For the icon, can you try

home-assistant/core#40039

I think that will only work if there is a custom icon set. To calculate the default icon on the frontend, I think you need the 'state' as well?

@bdraco
Copy link
Member

bdraco commented Sep 13, 2020

For the icon, can you try
home-assistant/core#40039

I think that will only work if there is a custom icon set. To calculate the default icon on the frontend, I think you need the 'state' as well?

I adjusted the PR to provide state and icon

@bdraco
Copy link
Member

bdraco commented Sep 13, 2020

We have this giant block to create the message https://github.com/home-assistant/core/blob/aa07171200ac06a89845464ba6b9f917d7ab4b8a/homeassistant/components/logbook/__init__.py#L551. Ideally we send state from the backend and no message and the frontend will generate the message (localized) instead. This would get us a smaller response.

I'd also like to drop "domain" as it appears to be unused by the frontend and means we end up sending the string again since its already in "entity_id"

@bdraco
Copy link
Member

bdraco commented Sep 13, 2020

I dont think we need to send Icon or domain. We can get those from Hass in the frontend

I can drop icon but I think its needed to fully match what the frontend does:
https://github.com/home-assistant/frontend/blob/dev/src/common/entity/state_icon.ts#L22

@zsarnett
Copy link
Contributor Author

Actually, Icon would be helpful actually. Re reading the code

@zsarnett
Copy link
Contributor Author

We don't need domain. We currently use domain but we don't need to

@bdraco
Copy link
Member

bdraco commented Sep 13, 2020 •
edited
Loading

What do you think about doing this part on the frontend?
https://github.com/home-assistant/core/blob/aa07171200ac06a89845464ba6b9f917d7ab4b8a/homeassistant/components/logbook/__init__.py#L551

Edit: even if the first step is just porting the code and it isn't localized, it would be an improvement as the backend wouldn't have to send those text blobs for each state.

@zsarnett
Copy link
Contributor Author

I much prefer us to do that in the Frontend as we can run translations (I actually don't know if that done in the backend or not)

@bramkragten (who is probably in bed) would be a better fit to say whether we should or not.

I personally think it should be in the frontend though

@zsarnett zsarnett mentioned this pull request Sep 13, 2020
Merged
4 tasks
@zsarnett
Copy link
Contributor Author

zsarnett commented Sep 13, 2020 •
edited
Loading

I pulled the changes that don't include adding the Logbook card so that the PRs are easier to review

I will come back to this PR once the other is merged

@zsarnett zsarnett marked this pull request as draft September 14, 2020 01:46
@bdraco bdraco mentioned this pull request Sep 14, 2020
Merged
21 tasks
@zsarnett zsarnett changed the title Logbook Card and style updates to Logbook Element Logbook Card Sep 22, 2020
@zsarnett zsarnett marked this pull request as ready for review October 20, 2020 01:28
bramkragten
bramkragten reviewed Oct 20, 2020
View reviewed changes
src/panels/lovelace/cards/hui-logbook-card.ts Outdated
oldHass &&
this._configEntities!.some(
(entity) =>
"entity" in entity &&
Copy link
Member

Choose a reason for hiding this comment

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

Why have this here but not in shouldupdate?

Copy link
Member

Choose a reason for hiding this comment

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

And do we need this check here if we already did it in shouldUpdate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need both. Since to get to updated it can be more than just if the state obj changed

bramkragten
bramkragten reviewed Oct 23, 2020
View reviewed changes
src/panels/lovelace/cards/hui-logbook-card.ts Outdated
Comment on lines 131 to 132
(oldHass && oldHass.themes !== this.hass.themes) ||
(oldConfig && oldConfig.theme !== this._config.theme)
Copy link
Member

Choose a reason for hiding this comment

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

Now we don't apply on the first update?

bramkragten
bramkragten reviewed Oct 28, 2020
View reviewed changes
src/panels/lovelace/cards/hui-logbook-card.ts Outdated
Comment on lines 129 to 130
const oldHass = changedProperties.get("hass") as HomeAssistant | undefined;
const oldConfig = changedProperties.get("_config") as LogbookCardConfig;
Copy link
Member

Choose a reason for hiding this comment

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

We could skip these if they are not in changedProperties, but 🤷‍♂️

bramkragten
bramkragten reviewed Oct 28, 2020
View reviewed changes
src/panels/lovelace/cards/hui-logbook-card.ts
`
: this._logbookEntries.length
? html`
<ha-logbook
Copy link
Member

Choose a reason for hiding this comment

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

Should we virtualize this list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk how to do that :)

Copy link
Member

Choose a reason for hiding this comment

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

Add the virtualize attribute ;-) https://github.com/home-assistant/frontend/blob/dev/src/panels/logbook/ha-logbook.ts#L92-L93

@zsarnett zsarnett mentioned this pull request Oct 30, 2020
Merged
8 tasks
@bramkragten bramkragten merged commit 78f1bb3 into dev Nov 11, 2020
@bramkragten bramkragten deleted the logbook-card branch November 11, 2020 10:49
@bramkragten bramkragten mentioned this pull request Nov 11, 2020
Merged
@ShaneQi
Copy link
Contributor

ShaneQi commented Jun 6, 2021 •
edited
Loading

@zsarnett

Question about the users/persons handling of this card:
I see the this card fetches persons’ info to translate context.user_id.

  1. Why don’t we also fetch users? (Logbook panel fetches both)
  2. Why do we iterate all entities to find person entities instead of using fetchUsers and fetchPersons like logbook panel does? Are we trying to avoid async calls?

Appreciate your work adding this card!

@frenck
Copy link
Member

frenck commented Jun 7, 2021

This PR is 9 months old...

If you have questions, please use or community forums or discord chat. If you found an issue, please raise an issue in the issue tracker.

Thanks!

../Frenck

@home-assistant home-assistant locked and limited conversation to collaborators Jun 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bramkragten bramkragten bramkragten approved these changes

Assignees
No one assigned
Labels
cla-signed Documentation Added
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zsarnett @bdraco @ShaneQi @frenck @bramkragten @homeassistant