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

added continously automatic icon color updates for tile card #9

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chillymattster
Copy link

Hi,

after creating issue #8 "Icon color of tile card only updated on page refresh" and catching up with the discussion #3 "add icon-color to Tile card" I spend some more time on how automatic update could be done for tile card without the need for page refresh or card_mod.
I found a way which I would like to present with this pull request.

Setting and modifiying icon_color attribute for entities with templates and showing those entities with tile cards for me results in exactly the same behaviour as the default behaviour if tile card with entities that change icon color on their own, e.g. lights.

I'm happy to take any comments and hope you like my contribution 😄

@Mariusthvdb
Copy link
Owner

Mariusthvdb commented Apr 11, 2024

thanks for your contribution!

let me chime in @elchininet, he's the technical brains behind our plugin.
I can't yet oversee the extra workload this might cause

some thoughts:
this seems like moving the regular card_mod config to the plugin.
Not sure if that is what we should aim for, as for that, card_mod itself is used best.

because: card_mod is only used when the actual entity is displayed in the frontend, and, custom-ui is evaluated upon each state change. Minimizing its processes to me seems of eminent priority.

I tested this locally and indeed it updates the icon and icon background accordingly very nicely, and see no errors.
will give it some time to see if efficiency is negatively influenced or not.

let's wait and see what dev says.

@elchininet
Copy link

Hey, I‘ll check it when I get some time but @chillymattster, if you can make the changes in a gist, @Mariusthvdb will be able to test them. He has a set up with a lot of custom icon colors so he can spot an issue or an unwanted behaviour quickly.

@chillymattster
Copy link
Author

chillymattster commented Apr 12, 2024

Hi,
I didn't yet use gist, but maybe it is time to do so now 😉
@Mariusthvdb Do you need a gist? I'm asking, because you wrote, that you are already testing the change of this pull request.

Let me briefly explain the rational I had in mind when proposing this change.
Carefully crawling through the readme of custom-icon-color, I got the impression that this extension should be something like an easy-to-use component for being able to add and change entity colors including a visible result in dedicated standard cards without involving any other extensions or component. That's why in custom-icon-color.js there are dedicated this.style?.setProperty(..) calls for standard cards to reflect the icon_color-attribute in some standard cards.
So please apologize if I'm misstaken here but I'm a bit struggling to follow your argument about preferring card_mod. If the attribute should be separated from the styling, then consequently there shouldn't be any this.style?.setProperty(..) calls at all. What makes custom_icon_color so interesting for me as a user is especially the feature, that I can set an individual icon color without the need to do deal with css styling and card_mod. I can set a color and it is shown in my UI, amazing! 😄
The problem I faced specifically with the tile card is that the current this.style?.setProperty(..) call for the tile card unfortunately destroys the default bhttps://github.com/jesserockz/esphome-external-component-examples/blob/main/components/empty_uart_component/empty_uart_component.hehaviour of the tile card, e.g. no animation and just the icon itself without the background which made it look strange beside other entities that change their color based on state by default.

I might be completely wrong with my understanding and if so, please apologize and I'm happy to adopt and learn more about the goals and principles of custom_icon_color 😃

From a technical perspective I tried to understand how cards in home assistant work to copy the behaviour for icon colors. Therefore I followed the developer docs for custom cards. My current understanding is, that set hass(hass) as a function of each card is called automatically from the home assistant frontend basesystem (or however this is called 😉 ) on each change / update of the hass-object. Each card is asked to do whatever is needed to do for reflecting a possible change in the hass-object which usually is updating the contained shadow dom.
I mirrored this default behaviour because my understanding is that the frequent updates of the hass-objects and therefore frequent calls of set hass(hass) on each card also trigger the frequent call of entityCard.prototype.updated in custom-icon-color.js. That's why I added updating the shadow dom as it is should be exactly the same what each card does anyway seems to kind of "natural" and in-line with basic principles.

@Mariusthvdb
Copy link
Owner

It's ok like this , testing as we speak .

I see the tile cards being updated live. So that is progress

What is still buggy is when you pull down to refresh (iOS app)

Then they lose some colorization and we need to navigate to another dashboard and back again to see them updating again

const shadowRoot = this.shadowRoot;
if (shadowRoot?.childNodes) {
for (const node of shadowRoot.childNodes) {
if (node.nodeName?.localeCompare('ha-card', 'en', { sensitivity: 'base' }) == 0) {

Choose a reason for hiding this comment

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

Is there a specific reason to use localeCompare to compare the strings? Is it faster?

If the reason is performance it would be good to instantiate a Int.Collator somewhere and make the comparison using that instance.

Copy link

@elchininet elchininet Apr 24, 2024

Choose a reason for hiding this comment

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

Hi @chillymattster,

I have compared the three. Using an Int.Collator instance is almost 30 times faster than the localeCompare method. But a strict equality keeps wining by far. As this code will be potentially executed recurrently it is good to keep performance as high as possible.

image

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @elchininet
would this also be a result of the Mariusthvdb/custom-ui#122 that was done on the original custom-ui, exactly to prevent excessive updating ?

If yes, should that be reverted like you say with that strict equality

Choose a reason for hiding this comment

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

@Mariusthvdb,
No, not related. This is just about the regular strict equality (or even the regular equality) to compare two strings performs better than Int.Collator or localeCompare that is being introduced here. It is not about the number of updates but to the speed of those updates.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @elchininet,
I updated the PR following your suggestion 😄, see also my comment from Oct 20th.

Copy link
Owner

@Mariusthvdb Mariusthvdb left a comment

Choose a reason for hiding this comment

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

What is still buggy is when you pull down to refresh (iOS app)

Then they lose some colorization and we need to navigate to another dashboard and back again to see them updating again

@Mariusthvdb Mariusthvdb marked this pull request as draft August 9, 2024 14:24
@chillymattster
Copy link
Author

Hi,
sorry for being absent this long, real life kicked in 😉

Is there a specific reason to use localeCompare to compare the strings?

I used localCompare as I liked the idea of being case-insensitive in order to be a bit more independent of possible changes or inconsistencies in HA and therefore to increase stability. But it seems that it comes with high costs so I doubt that my decision was a good trade-of.

What is still buggy is when you pull down to refresh (iOS app)

Unfortunately I won't be able to work on this or fix it as I don't own any iOS device.

What I observe on Android is, that your describes behaviour occurs from time to time on Android as well. In such a case there is not just the color not working correct any more but also data updates in general are affected. My observation is that this happens when the connection to HA is flaky / unstable in general which leads to a general mess in the whole underlying routines to update the hass-object correctly and constantly.

chillymattster and others added 3 commits October 20, 2024 22:43
update for tile card.

Signed-off-by: chillymattster <[email protected]>
Signed-off-by: chillymattster <[email protected]>
@chillymattster
Copy link
Author

Hi @Mariusthvdb,

do you have any idea on how to proceed with the iOS problem you encountered?
Does the switch to the faster comparison maybe already improve the behaviour?

@Mariusthvdb
Copy link
Owner

Mariusthvdb commented Nov 20, 2024

Hi @Mariusthvdb,

do you have any idea on how to proceed with the iOS problem you encountered? Does the switch to the faster comparison maybe already improve the behaviour?

No I dont tbh, I didnt check this anymore awaiting your works on the PR.
as for the new changes, did you test that locally?

As for the generic implication of the PR: I am very worried this 'continuously automatic' aspect will cause a burden on the system.

Given the way HA is moving, I did decide to no longer update custom-ui and its siblings (like this plugin), and follow HA dev advice to not burden the system with extra attributes that need constant evaluations.

the way to go is with core Frontend options, and where they fall short, use card-mod (for as long that still works, because the author seems to have no interest in updating his resources anymore either).

Aware this is not the same as what we do here (we set a true attribute icon_color and show it in all cards, including more-info etc etc).

Unfortunately I feel it is the only way forward.

Maybe @elchininet can share his thoughts on that.

Anyways, first test your own PR in your own local instance so you are 100% sure it works and does not break the system (check inspector logs)

@chillymattster
Copy link
Author

Hi,
sure, I tested locally before making the latest push and I use it constantly in my HA instance with browser and Android App. So far, I didn't encounter any issues, for me it works the same as before.

Thanks @Mariusthvdb for sharing your thoughts about further development. I have to admit that I'm not really deep into HA development so this is new knowledge for me. I fully understand if you don't want to further develop custom-ui.
Just out if curiosity, is there already a similar functionality in core frontend that achieves the same result? I know that there is a lot going on but up to now I didn't notice any anouncement that enables coloring of items / tiles as custom-icon-color does.

@Mariusthvdb
Copy link
Owner

So far, I didn't encounter any issues, for me it works the same as before.

ok, but is it any better than the current release. Not much use making these changes id there is no progress..

Just out if curiosity, is there already a similar functionality in core frontend that achieves the same result? I know that there > is a lot going on but up to now I didn't notice any anouncement that enables coloring of items / tiles as custom-icon-color > does.

please see https://github.com/Mariusthvdb/custom-ui/blob/master/README.md#please-read-this-before-using-custom-ui
it explains the current state of affairs

@palves
Copy link

palves commented Jan 8, 2025

So far, I didn't encounter any issues, for me it works the same as before.

ok, but is it any better than the current release. Not much use making these changes id there is no progress..

What I understood he meant was, that the new changes work as well as his previous changes. Which is strictly better than the current release.

Is there something else holding this back?

@Mariusthvdb
Copy link
Owner

yes,
I need to await the review of @elchininet on the latest changes.
he is a busy person, so not sure if able to check any time soon.

@elchininet
Copy link

yes, I need to await the review of @elchininet on the latest changes. he is a busy person, so not sure if able to check any time soon.

Hi @Mariusthvdb,
Latest changes are very small, just the change from localeCompare to strict equality and the other one was to fix a typo. LGTM 👍

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

Successfully merging this pull request may close these issues.

4 participants