Logbook: Localize Message & Style updates #6978

zsarnett · 2020-09-13T22:41:33Z

Proposed change

Type of change

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

Checklist

The code change is tested and works locally.
There is no commented out code in this PR.

Core PR is reliant on the frontend PR merging before it can be merged as it is a breaking change: home-assistant/core#40070

zsarnett · 2020-09-13T22:42:05Z

Previous Conversation

zsarnett 2 hours ago •
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
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
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 (J. Nick Koston)
For the icon, can you try

home-assistant/core#40039

@bdraco
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

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

We have this giant block to create the message home-assistant/core@aa07171/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
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

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

@zsarnett

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

@bdraco

What do you think about doing this part on the frontend?
home-assistant/core@aa07171/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

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 · 2020-09-13T22:42:52Z

I have pulled out the changes for the logbook into here. Original Discussion was here: #6976

zsarnett · 2020-09-13T22:51:21Z

I have started the logbook message in the frontend but it still needs localized

bdraco · 2020-09-15T03:04:28Z

It looks like we are missing icons for start/stop/restart events in the homeassistant domain

bdraco · 2020-09-15T03:06:07Z

{
	"when": "2020-09-15T02:59:41+00:00",
	"name": "Home Assistant",
	"message": "stopped",
	"domain": "homeassistant"
}, {
	"when": "2020-09-15T03:00:05+00:00",
	"name": "Masterbed TV Cabinet 32 Restart",
	"state": "off",
	"entity_id": "switch.masterbed_tv_cabinet_32_restart",
	"icon": "mdi:restart"
}, {
	"when": "2020-09-15T03:00:05+00:00",
	"name": "Gate Trigger 32 Relay",
	"state": "off",
	"entity_id": "binary_sensor.gate_trigger_32_relay"
}

zsarnett · 2020-09-15T03:10:30Z

@bdraco This should be fixed in the latest commit

bdraco · 2020-09-15T03:22:54Z

Looks good.

We did loose the exact time on the more info pop up. Was that one purpose?

zsarnett · 2020-09-15T03:37:58Z

Yes that was on purpose

ludeeus · 2020-09-15T06:55:07Z

src/panels/logbook/ha-logbook.ts

+          .entityId=${item.entity_id}
+          @click=${this._entityClicked}
+        >
+          ${!this.narrow


Suggested change

${!this.narrow

${!this.narrow && !this.relativeTime

It's redundant to show 2 types of time for the same element.

hmm I disagree. I was thinking about adding it to the narrow version as well

Narrow being: Relative_Time - Absolute_Time

Reason being that some people want to know when but I don't want to calculate how many hours/mins ago that was

Put the absolute time in a tooltip?

Tooltip or something like this?

yeah that could also work, but the logbook lines can be long, and they wrap now, you don't want a secondary line then I guess...

From that statement of no secondary then you don't want the relative time in the secondary with the tooltip either? You are saying keep it to the left?

Personally the time to the left makes it look disconnected but idk. I like having both times. Tooltip would be okay. But sometime I want to know the absolute and sometimes I want to know the relative time without having to calculate it

bramkragten · 2020-09-16T08:33:15Z

src/common/entity/domain_icon.ts

+    case "cover": {
+      const open = compareState !== "closed";


Why did you move all domain-specific icon code in here instead of importing the function?

We need to default after wards. If the conditions aren't met we run the last lines of the domain icon

Wouldnt this on top be enough? If the domain has a specific icon function let that handle it...

if (domain in domainIcons) { return domainIcons[domain](state); }

Some domains have a specific icon but also non specific. Those are a fallback

bramkragten · 2020-09-16T08:34:02Z

src/data/logbook.ts

+  stateObj: HassEntity,
+  domain: string
+): string => {
+  const localizePath = "ui.components.logbook.messages";


Move it out of the function.

bramkragten · 2020-09-16T08:37:39Z

src/data/logbook.ts

+        case "opening":
+        case "window":
+          if (isOn) {
+            return hass.localize(`${localizePath}.was_opened`);


Can we make the localize names:
ui.components.logbook.messages.${domain}.${device_class || "_"}.${state}

That's what we also use for the state translations, and requires a lot less code.

The state translations are built by core for all the states. Is it even possible to do that here? like in the code there are a lot of if this state do this otherwise this. Doing it your way would require me to do this for all states? no?

You would move the logic to the translations instead of the code, not more complicated I guess?

How do I do the logic in Json?

src/panels/logbook/ha-logbook.ts

zsarnett · 2020-09-22T20:27:46Z

The link styles just look weird. Especially with two on the same line.

I can put the time on the right. Just made sense to me to have them as the secondary information.

zsarnett · 2020-09-22T21:15:09Z

Actually removing the link was my main reason off the PR

This just looks dated

bramkragten · 2020-09-24T12:20:42Z

But in your proposal, you would have a click target (automation) in a click target (entity)? And it would not be clear what a click would do?

I think since they perform the same action, open the more info dialog, they should look the same?

zsarnett · 2020-09-24T16:02:04Z

Well in my mind and the reasoning was the main action of the row click was the Entity that was changed. The more info for the most important entity of the row. The Auatomation/script that changed it is supporting information and is displayed as such. Clicking the link more info to the supporting and clicking the row more info to the main entity.

Is there no better solution than Anchor links 🤮 ... I guess not

zsarnett · 2020-09-24T20:36:18Z

… logbook-style-and-rework

bdraco · 2020-09-25T12:41:29Z

Once this is good to go, I can start working on making the triggers localizable.

bramkragten · 2020-09-28T11:23:05Z

Does this need a backend change or is this a bug? Am getting an error:

2020-09-28 13:20:08 ERROR (SyncWorker_27) [homeassistant.components.recorder.util] Error executing query: unsupported operand type(s) for |: 'NoneType' and 'BinaryExpression'
2020-09-28 13:20:08 ERROR (MainThread) [aiohttp.server] Error handling request
Traceback (most recent call last):
  File "/Users/bram/Documents/projects/home-assistant/core/venv/lib/python3.7/site-packages/aiohttp/web_protocol.py", line 418, in start
    resp = await task
  File "/Users/bram/Documents/projects/home-assistant/core/venv/lib/python3.7/site-packages/aiohttp/web_app.py", line 458, in _handle
    resp = await handler(request)
  File "/Users/bram/Documents/projects/home-assistant/core/venv/lib/python3.7/site-packages/aiohttp/web_middlewares.py", line 119, in impl
    return await handler(request)
  File "/Users/bram/Documents/projects/home-assistant/core/homeassistant/components/http/request_context.py", line 18, in request_context_middleware
    return await handler(request)
  File "/Users/bram/Documents/projects/home-assistant/core/homeassistant/components/http/ban.py", line 72, in ban_middleware
    return await handler(request)
  File "/Users/bram/Documents/projects/home-assistant/core/homeassistant/components/http/auth.py", line 127, in auth_middleware
    return await handler(request)
  File "/Users/bram/Documents/projects/home-assistant/core/homeassistant/components/http/view.py", line 129, in handle
    result = await result
  File "/Users/bram/Documents/projects/home-assistant/core/homeassistant/components/logbook/__init__.py", line 254, in get
    return await hass.async_add_executor_job(json_events)
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/bram/Documents/projects/home-assistant/core/homeassistant/components/logbook/__init__.py", line 250, in json_events
    entity_matches_only,
  File "/Users/bram/Documents/projects/home-assistant/core/homeassistant/components/logbook/__init__.py", line 477, in _get_events
    filters.entity_filter() | (Events.event_type != EVENT_STATE_CHANGED)
TypeError: unsupported operand type(s) for |: 'NoneType' and 'BinaryExpression'

bramkragten · 2020-09-28T11:25:26Z

Still think the icons should be vertically centered

zsarnett · 2020-09-28T13:58:13Z

Works for me on the latest core dev

bdraco · 2020-09-28T14:07:24Z

Does this need a backend change or is this a bug? Am getting an error:

Does this still happen with home-assistant/core#40387 ?

bdraco · 2020-09-28T14:29:28Z

Does this need a backend change or is this a bug? Am getting an error:

Does this still happen with home-assistant/core#40387 ?

@bramkragten Fixed here home-assistant/core#40565. b7859a0 (#40565)

src/panels/logbook/ha-logbook.ts

Co-authored-by: Bram Kragten <[email protected]>

Log book style updates rework functionality

666c88a

homeassistant added the cla-signed label Sep 13, 2020

zsarnett mentioned this pull request Sep 13, 2020

Extract the icon and state for logbook state changed events home-assistant/core#40039

Merged

21 tasks

Add automation to the mix

5daa112

bdraco mentioned this pull request Sep 14, 2020

Remove message and domain from logbook state_changed events home-assistant/core#40070

Merged

21 tasks

zsarnett added 5 commits September 14, 2020 16:53

Changes

5d0afad

Create Translations

8fda888

Domain Icon Changes

9ab3587

update state label badge to use new domain icon

4522a5f

localize state

009d01d

zsarnett requested a review from bramkragten September 15, 2020 03:02

zsarnett changed the title ~~Log book style updates rework functionality~~ Logbook: Localize Message & Style updates Sep 15, 2020

Fix no entity id icon

3d0a68d

zsarnett added 2 commits September 14, 2020 22:17

Add lock remove unneeded automation message

e21b147

remove unneeded parameter

425f267

ludeeus reviewed Sep 16, 2020

View reviewed changes

bramkragten reviewed Sep 16, 2020

View reviewed changes

src/panels/logbook/ha-logbook.ts Outdated Show resolved Hide resolved

bramkragten reviewed Sep 16, 2020

View reviewed changes

src/panels/logbook/ha-logbook.ts Outdated Show resolved Hide resolved

bramkragten reviewed Sep 16, 2020

View reviewed changes

src/panels/logbook/ha-logbook.ts Outdated Show resolved Hide resolved

zsarnett added 4 commits September 24, 2020 15:36

comments

ed9e695

Remove row click

a90697c

Merge branch 'dev' of https://github.com/home-assistant/frontend into…

789fb81

… logbook-style-and-rework

fix merge

563b39c

make bram happy ;)

16b0e89

bdraco mentioned this pull request Sep 28, 2020

Fix logbook with empty filter and remove unused code home-assistant/core#40565

Merged

21 tasks

bramkragten reviewed Sep 28, 2020

View reviewed changes

src/panels/logbook/ha-logbook.ts Outdated Show resolved Hide resolved

zsarnett and others added 5 commits September 28, 2020 11:17

Update src/panels/logbook/ha-logbook.ts

92703cc

Co-authored-by: Bram Kragten <[email protected]>

Restore src/common/entity/binary_sensor_icon.ts

133cd5f

Restore src/common/entity/cover_icon.ts

382888c

Restore src/common/entity/sensor_icon.ts

2ca91bd

icon changes

86a4a8b

bramkragten approved these changes Sep 30, 2020

View reviewed changes

bramkragten merged commit e2427c8 into dev Sep 30, 2020

bramkragten deleted the logbook-style-and-rework branch September 30, 2020 08:24

bramkragten mentioned this pull request Sep 30, 2020

20200930.0 #7174

Merged

This was referenced Jun 10, 2021

Fixed Logbook Card/Panel/Dialog Incorrect Entires for input_datetime Entities #9399

Merged

Logbook Card/Panel/Dialog Incorrect Entires Texts for input_datetime Entities #9405

Closed

github-actions bot locked and limited conversation to collaborators Jul 5, 2022

Logbook: Localize Message & Style updates #6978

Logbook: Localize Message & Style updates #6978

Conversation

zsarnett commented Sep 13, 2020 • edited Loading

Proposed change

Type of change

Checklist

zsarnett commented Sep 13, 2020 • edited Loading

zsarnett commented Sep 13, 2020

zsarnett commented Sep 13, 2020

bdraco commented Sep 15, 2020

bdraco commented Sep 15, 2020

zsarnett commented Sep 15, 2020

bdraco commented Sep 15, 2020

zsarnett commented Sep 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsarnett commented Sep 22, 2020 • edited Loading

zsarnett commented Sep 22, 2020 • edited Loading

bramkragten commented Sep 24, 2020 • edited Loading

zsarnett commented Sep 24, 2020

zsarnett commented Sep 24, 2020 • edited Loading

bdraco commented Sep 25, 2020

bramkragten commented Sep 28, 2020

bramkragten commented Sep 28, 2020

zsarnett commented Sep 28, 2020

bdraco commented Sep 28, 2020

bdraco commented Sep 28, 2020

zsarnett commented Sep 13, 2020 •

edited

Loading

zsarnett commented Sep 13, 2020 •

edited

Loading

zsarnett commented Sep 22, 2020 •

edited

Loading

zsarnett commented Sep 22, 2020 •

edited

Loading

bramkragten commented Sep 24, 2020 •

edited

Loading

zsarnett commented Sep 24, 2020 •

edited

Loading