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

Push map strategy logic down into map card #24303

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

karwosts
Copy link
Contributor

Proposed change

This PR pushes the special logic for the map strategy (auto-show all entities) into a new option in map card (show_all).

This allows user to take control of map panel without losing any features. Currently once you take control of the map panel, the list of entities becomes frozen in time, and user has to then manually update any future added persons or zones, which is not ideal.

This makes it easier to customize the map panel (e.g. add history tracing lines) without compromise.

Also I might follow up to add an option to unfocus the home zone (to go back to 2025.1 behavior), and that would be another reason to take control of map dashboard.

The other direction this could go would be to just add more options to the map strategy via a custom dialog without having the user take control, but that just seems like we will forever have to implement all map features in two places; and users will complain any time the features diverge between the two. Seems like it makes sense to just have one way to configure map features (via editing the card).

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

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:

Copy link
Contributor

@MindFreeze MindFreeze left a comment

Choose a reason for hiding this comment

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

Docs should be updated to mention this option

@MindFreeze MindFreeze added the Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Feb 20, 2025
? processConfigEntities<MapEntityConfig>(config.entities)
if (config.show_all && (config.entities || config.geo_location_sources)) {
throw new Error(
"Cannot specify show_all and entities or geo_location_sources"
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 considered if this should be allowed (like if you want to show_all and additionally apply some custom config to a specific entity), but it got a little confusing with the mix of entities and geo_sources, and I'm not sure if there is demand for this, so I punted for now for simplicity.

}
this._config = { ...config };
if (this.hass && config.show_all) {
this._config.entities = this._getAllEntities();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feels a bit odd but I guess there's nothing wrong with a card modifying its own config?

I think this is needed so hasConfigOrEntitiesChanged tracks these auto-entities correctly.

@karwosts
Copy link
Contributor Author

karwosts commented Feb 20, 2025

Docs should be updated to mention this option

Sure, will PR once the functionality is agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants