Mariusthvdb / custom-ui

Add templates and icon_color to Home Assistant UI
161 stars 30 forks source link

_updateHass are called with the same unchanged states #123

Closed bratanon closed 8 months ago

bratanon commented 8 months ago

Describe the bug We are high-jacking the _updateHass method on the homeassistant class to insert our custom-ui logic to change icon, and or icon colors.

So every time some state change, we are there and looping thru all states and checking if the entity the state belongs to have template in the list of attributes.

See: https://github.com/Mariusthvdb/custom-ui/blob/b9eba31d9f2e83f22193c89c7a770fcc90da7ef7/custom-ui.js#L135

If that entity has template in the list of attributes, go through all its children and trying to evaluate the value of that child.

If we get a result from that evaluation (and we usually are as the the only time we don't is when "null" is return which I can't really find a use case for) we will create a new entity object, with some of the old entity properties, and some of our evaluated props.

We then compare this new entity with the old one to see if we need to update the old (the real) entity.

if (hass.states && entity !== hass.states[key]) {
  obj.states[key] = newEntity;
} else if (entity !== newEntity) {
  obj.states[key] = newEntity;
}

and then calls the original _updateHass with the obj.

This comparison is not working as intended. To compare non primitiv types we need to do a "deepEqual". Checking what is inside of the objects and compare that.

Currently this if statements will only be handled by the first hass.states as the other comparisons will always be true.

console.log(entity === hass.states[key]) // will be false.
console.log(entity == hass.states[key]) // will be false.
console.log(Object.is(entity, hass.states[key])) // will be false.
console.log(entity === newEntity) // will be false.
console.log(entity == newEntity) // will be false.
console.log(Object.is(entity, newEntity)) // will be false.

This creates calls to _updateHass with obj that is not actually changed, but a copy. Home assistant does know this, and will trigger an update for them with whatever they decide to do.

If you have customized a device_tracker-entity, and are looking at the Map provide by HA, and a state change happens, the map will zoom level and location will be reset to the default zoom and location.

For larger setups, which has many entities that can potentially update every second this becomes a problem as the map will kind of be unusable.

To Reproduce Steps to reproduce the behavior:

  1. Add a customization for icon_color to a device_tracker-entity.
  2. Make sure that entity is listed on the map.
  3. Make sure you have other entities that updates (add a time_date sensor and way a minute if you don't have any other.
  4. Go to the big map (/map)
  5. Change location and zoom level, and wait for an entity update.

Expected behavior The map should stay on the same zoom level and location.

Desktop:

bratanon commented 8 months ago

This also seems related https://github.com/home-assistant/frontend/issues/16666

Mariusthvdb commented 8 months ago

doesnt the 16666 prove it has nothing to do with custom-ui? and rather its a more generic HA issue? unless the user is also using custom-ui, it would look for solving that in HA

doesnt mean I am not hoping custom-ui could be optimized, and I have indicated before there's a constant updating going I would love to prevent

as you say:

So every time some state change, we are there and looping thru all states and checking if the entity the state belongs to have template in the list of attributes.

if this PR you suggest would actually stop that from happening, and only check the required states, that would be awesome.

bratanon commented 8 months ago

doesnt the 16666 prove it has nothing to do with custom-ui? and rather its a more generic HA issue? unless the user is also using custom-ui, it would look for solving that in HA

Yes and no. As custom-ui is constantly "updating" all its customization's object because of the broken if statements, that is making the HA bug even worse. Custom-ui is constantly triggering the bug by its unnecessary updates.

doesnt mean I am not hoping custom-ui could be optimized, and I have indicated before there's a constant updating going I would love to prevent

This doesn't solve all of them, but at least one of them.

if this PR you suggest would actually stop that from happening, and only check the required states, that would be awesome.

No it doesn't, this PR fixes the BROKEN if statement that will prevent the updates later down the stack if there is checks in place. To me it seems like you don't understand the code in PR at all, is that why you are unsure about this?

At least it will prevent custom-ui to change states that it doesn't need to be changed. Remember, we are high-jacking the method so we don't "own" anything here.

Mariusthvdb commented 8 months ago

its not that I dont understand, its more the fact I want to make sure we are not tailor-made 'fixing' issues in other software that should be generically fixed in that other software.

I can see the value ofc of preventing those useless updates. as a matter of fact, I believe the complete plugin should be reviewed on that aspect, and adjusted.

tbh, Ive stopped using it altogether, moved my icon changes to card-mod, and have created a tiny separate custom-icon-color plugin to do only that (it still uses the old code, attaching the attribute and changing the state badge with it)

having said all of that, I'll approve, and would like to ask if you can write a short and succinct description please, without referring to the Map issue in HA.

thanks for you in depth explanation above, and effort to keep this plugin up to date.

hope you will be able to have a look at the other open issue, where the templates, or more specifically the templates results no longer show as a result when templated in Jinja. Ill tag you there, maybe you can have a look

bratanon commented 8 months ago

....would like to ask if you can write a short and succinct description please

Addressed an issue where state objects were inadvertently updated as a result of a comparison error.