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

Patch mdi:home-assistant icon #4587

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RoboMagus
Copy link
Contributor

@RoboMagus RoboMagus commented Aug 18, 2024

Summary

Replace the old mdi:home-assistant icon with the new one, wherever the MDI icon is used. (e.g. entity icons in WearOS app, and custom notification icons)

As the used mikepenz.iconics icon library uses Fonts as the underlying mechanism that is used throughout this codebase, a 1 glyph font is added and a wrapper class that provides that font with the interfaces used to render it through the aforementioned library.

It does feel like a bit of a workaround, but the alternative would likely require much more changes throughout the codebase and this change is as contained as can be.

Screenshots

Based on work done in #4556, as I'm not sure what other places the mdi:home-assistant icon could appear.
image

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

This PR would be a prerequisite for #4556.

@RoboMagus RoboMagus force-pushed the patch_mdi_home_assistant_icon branch 4 times, most recently from de8abab to 59ff38a Compare August 18, 2024 21:12
@RoboMagus RoboMagus force-pushed the patch_mdi_home_assistant_icon branch from 59ff38a to f95c209 Compare August 18, 2024 21:22
@jpelgrom
Copy link
Member

Can you share how you generated the font?

I'm not sure what other places the mdi:home-assistant icon could appear.

In other places where the IconicsDrawable(context, name) constructor is used. Just searching for the constructor you can find multiple usages in the code base so I'm honestly not sure what you're doing now is that effective. Is there any reason not to replace all of those with CommunityMaterial.getIconByMdiName?

@RoboMagus
Copy link
Contributor Author

Can you share how you generated the font?

That was honestly a lot of just messy trial and error using some online font generators to get the HA icon in the mikepenz CommunityMaterial Fonts replaced with the new SVG logo from the HA assets repo, and getting it lined up with the font borders so it would render at the same size and offset as all other icons in that set. After I figured out we'd only need the one glyph in the font I used pyftsubset from fonttools to remove anything but the one glyph required.

... I'm honestly not sure what you're doing now is that effective. Is there any reason not to replace all of those with CommunityMaterial.getIconByMdiName?

What this PR covers thus far is the direct use of the old icon (CommunityMaterial.Icon2.cmd_home_assistant), what is directly queried by CommunityMaterial.getIconByMdiName() and the calls to entity.getIcon().
It does look like more places need to be covered and this could be done as you suggest by using CommunityMaterial.getIconByMdiName, but it'd be a shame to invest effort into tackling all the edge cases if the concept of this change would be shot down anyways.

If we can reach consensus about the best way of globally updating the old mdi:home-assistant icon with the new one I'll update the rest of the codebase where these icons could be queried.

Broad use of the IconicsDrawable that is provided by the mikepenz library throughout the codebase makes adding a custom font that can be used by this library the least invasive change from what I can tell. But I'm open to alternative solutions for this issue.

@jpelgrom
Copy link
Member

It does look like more places need to be covered and this could be done as you suggest by using CommunityMaterial.getIconByMdiName, but it'd be a shame to invest effort into tackling all the edge cases if the concept of this change would be shot down anyways.

If we can reach consensus about the best way of globally updating the old mdi:home-assistant icon with the new one I'll update the rest of the codebase where these icons could be queried.

Broad use of the IconicsDrawable that is provided by the mikepenz library throughout the codebase makes adding a custom font that can be used by this library the least invasive change from what I can tell. But I'm open to alternative solutions for this issue.

I agree with this solution and would accept it, but it needs to be implemented consistently/everywhere which is why I commented. The mdi prefix is used to determine the font so patching needs to use 'our' function to override the icon returned.

@dshokouhi any objections?

@dshokouhi
Copy link
Member

any objections?

none from me :)

Comment on lines 306 to 310
val mdiIcon = icon.split(":")[1]
if (mdiIcon == "home-assistant") {
return HaIconTypeface.Icon.mdi_home_assistant
}
return IconicsDrawable(context, "cmd-$mdiIcon").icon ?: CommunityMaterial.Icon.cmd_bookmark
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for not using CommunityMaterial.getIconByMdiName here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants