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.

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

Avoid re-rendering map with "sources" #18635

Merged
merged 11 commits into from
Nov 28, 2023
Merged

Avoid re-rendering map with "sources" #18635

merged 11 commits into from
Nov 28, 2023

Conversation

amitfin
Copy link
Contributor

@amitfin amitfin commented Nov 12, 2023

Breaking change

Proposed change

Map card can be configured with geo_location_sources. It was impossible to interact with the map when auto_fit was also used. The map would reset its view every second (or so).
The fix is to keep the object of the map's entity list, unless there is a real change.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

type: map
entities: []
geo_location_sources:
  - geo_json_events
auto_fit: true

Additional information

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

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:

@karwosts
Copy link
Contributor

Maybe I'm misunderstanding but this change doesn't look right to me.

You've replaced updating on hasConfigOrEntitiesChanged with just updating when an entity state string changes (e.g. "home" -> "away"). But this seems like it will reject update when an entity moves around without changing zone,

The map should redraw anytime one of the watched entities moves, right?

The was another issue I tried to work on once that would have had the map stop autofitting once a user manually interacted with the map, like manually trying to move it. I couldn't quite figure out how to make that work right so I gave up on it. Maybe that would be a better solution for you if it could be made to work?

When you were debugging did you see what was triggering it to rerender constantly, were the geo sources moving or something?

@amitfin
Copy link
Contributor Author

amitfin commented Nov 13, 2023

You've replaced updating on hasConfigOrEntitiesChanged with just updating when an entity state string changes (e.g. "home" -> "away"). But this seems like it will reject update when an entity moves around without changing zone,

You're right. The comparison should be identical to the one that is here and include all of the state object, including the "latitude" and "longitude" attributes. The code was changed accordingly. Thanks!

The was another issue I tried to work on once that would have had the map stop auto-fitting once a user manually interacted with the map, like manually trying to move it. I couldn't quite figure out how to make that work right so I gave up on it. Maybe that would be a better solution for you if it could be made to work?

The use case I'm trying to address is geo_location entities which are coming and going (they have a lifetime of 10 minutes). These entities have constant coordinates. We do want to auto-fit the map when there is a new entity, but allow the user to interact with the map (e.g. zoom out on in) while the data on the map hasn't changed.

When you were debugging did you see what was triggering it to rerender constantly, were the geo sources moving or something?

The debugging was focused on the use case described above ("source" entities with constant coordinates.) However, this change should be correct for any scenario. There is no need to re-render the element when there is no change in the data (causing the element to auto-fit for no reason, when this option is used.)

@bramkragten
Copy link
Member

The issue probably is _getEntities, it takes hass.states and thus will return a new array on every state change, this should be made smarter so it only returns a new array if an entity is actually changed

@amitfin
Copy link
Contributor Author

amitfin commented Nov 17, 2023

Below are additional details on the change and its approach.

shouldUpdate has 2 places for checking if relevant states have changed:

  1. For entities configured in this._config.entities it's done by calling hasConfigOrEntitiesChanged. The function compares the state of the selected entities.
  2. For entities coming via this._config.geo_location_sources: the old implementation checked if there is a state change for any entity in the system. The new implementation is comparing only relevant entities.

To summarize: the change is to make shouldUpdate logic equal when checking any entity state change of hui-map-card.

(There was no attempt to make a different use or a change in the memoizeOne logic.)

@bramkragten bramkragten merged commit 9b20e1c into home-assistant:dev Nov 28, 2023
13 checks passed
@amitfin amitfin deleted the map-sources-render branch November 28, 2023 19:31
@karwosts
Copy link
Contributor

I believe this change broke auto-fit for regular entities, the map no longer follows them.

Previously ha-map would run autofit anytime the entities property changed, but now it looks like we only update entities on setConfig, and not during each render.

Should willUpdate not be conditioned on having geo_location_sources?

 protected willUpdate(changedProps: PropertyValues): void {
    super.willUpdate(changedProps);
    if (
      changedProps.has("hass") &&
      this._config?.geo_location_sources &&
      !deepEqual(
        this._getSourceEntities(changedProps.get("hass")?.states),
        this._getSourceEntities(this.hass.states)
      )
    ) {
      this._mapEntities = this._getMapEntities();
    }
  }

@karwosts karwosts mentioned this pull request Nov 28, 2023
9 tasks
@karwosts
Copy link
Contributor

Fixing with #18799

@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants