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

Add icons for binary_sensors #318

Closed
felipecrs opened this issue Jul 25, 2022 · 11 comments
Closed

Add icons for binary_sensors #318

felipecrs opened this issue Jul 25, 2022 · 11 comments
Labels

Comments

@felipecrs
Copy link
Contributor

All binary sensors currently don't have an icon set. I think we should leverage our existing icons set to binary_sensor as well.

What do you guys think?

@felipecrs
Copy link
Contributor Author

felipecrs commented Jul 25, 2022

The caveat is that there will be no more state-based icons, like motion on or motion off. But I think this is worth the price.

@NickM-27
Copy link
Sponsor Collaborator

I'm not sure if I'm onboard, users can easily change the icon to what they want if it's better to be more clear and the automatic icon is not important.

If the automatic icon is disabled then for a user who wants that behavior, it would be practically impossible besides creating a separate template entity to do the same thing.

@felipecrs
Copy link
Contributor Author

felipecrs commented Jul 25, 2022

I understand, and I think your concern is legit. The best to address this would be to have a feature in HA that would allow the user to "unset" an icon provided by the integration.

Have in mind some things though:

  • We are already overriding the icon for sensors, so if we go for consistency, we should either remove them from sensors or add them to binary_sensors.
  • It's not practical for the user to set custom icons for all the supported entities. In my instance, I have 76 of them that I would have to tweak manually:

image

So, if the goal is to allow the user to keep the original icons (which will have state-based icons), I suggest the following:

  • To add a new option in the integration configuration called Set custom icons for object entities, that would cause all sensors and binary_sensors to have custom icons set by the integration. The user could toggle it as they wish.

But personally, I would not advocate for it because I think the custom icons are better than one icon for all entities (which are state-based). That's because:

  • Custom icons helps a lot to find entities when searching for them
  • And binary_sensors already are colored according to their state, which is, IMO, more than good enough to replace "state-based" icons functionality:
chrome_sX18s4uznH.mp4

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Jul 25, 2022

We are already overriding the icon for sensors, so if we go for consistency, we should either remove them from sensors or add them to binary_sensors.

Sensors don't have a switching behavior or a default icon so I think it is a different case.

Custom icons helps a lot to find entities when searching for them
And binary_sensors already are colored according to their state, which is, IMO, more than good enough to replace "state-based" icons functionality:

I mean sure, I don't necessarily disagree for my own usage, but with the number of posts I see on Reddit and other places asking how to do switching icons based on state I don't think it's easy to dismiss it so quickly based on color being satisfactory personally.

When the inevitable user comes and asks how to make the icons change based on state like their ONVIF integration (just a hypothetical example) I don't feel telling them to make a template entity is a great answer nor do I want to support them as they try templates.

All that being said, it has occurred to me that as an integration we can still easily provide custom binary sensor icons by providing the icon as a property and setting it based on the current entity state.

@felipecrs
Copy link
Contributor Author

felipecrs commented Jul 25, 2022

Sensors don't have a switching behavior or a default icon so I think it is a different case.

Got it. Makes sense.

I mean sure, I don't necessarily disagree for my own usage, but with the number of posts I see on Reddit and other places asking how to do switching icons based on state I don't think it's easy to dismiss it so quickly based on color being satisfactory personally.

Got it, I never stumbled upon this.

I mean sure, I don't necessarily disagree for my own usage, but with the number of posts I see on Reddit and other places asking how to do switching icons based on state I don't think it's easy to dismiss it so quickly based on color being satisfactory personally.

When the inevitable user comes and asks how to make the icons change based on state like their ONVIF integration (just a hypothetical example) I don't feel telling them to make a template entity is a great answer nor do I want to support them as they try templates.

I guess you should consider my suggestion of adding it as per the integration configuration then if you care about this situation.

All that being said, it has occurred to me that as an integration we can still easily provide custom binary sensor icons by providing the icon as a property and setting it based on the current entity state.

I did not understand how this would help...

@NickM-27
Copy link
Sponsor Collaborator

NickM-27 commented Jul 25, 2022

I did not understand how this would help...

Right now the icon for every "presence" entity is a house with an outlined one being off and filled in one being on

Screen Shot 2022-07-25 at 9 47 25 AM

You could instead do

    @property
    def icon(self) -> str:
        """Return the icon of the sensor."""
       if self._is_on:
         return ICON_PERSON

       return ICON_PERSON_MISSING

this way the binary sensors still have the state based icon and the icons can be more relevant like you are hoping to achieve here

@felipecrs
Copy link
Contributor Author

Oh yeah. Got it. Unfortunately, only a few icons provide meaningful variant to be able to leverage it. Still, we could do such a thing for the ones that we can, like "Person", "Car", "Dog". We could also request new icons (like horse-off, cat-off) for the ones missing, and add them in a later version once they get added to MDI.

@felipecrs
Copy link
Contributor Author

So, amazing, I totally agree, it's a great idea!

@dermotduffy
Copy link
Collaborator

Sounds like a good compromise to me. Where there's a good on/off variant pair, we use that -- otherwise we use the default (still allows a user to customize them to something different, non-state related, if they so choose).

@felipecrs
Copy link
Contributor Author

I think this can be closed, right?

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Jun 10, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants