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

[BasicUI] icon.png minimal fix #2232

Merged
merged 1 commit into from Jan 2, 2024
Merged

Conversation

mherwege
Copy link
Contributor

See comment #2231 (comment)

This is the minimal fix to the issue.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege requested a review from a team as a code owner December 22, 2023 11:34
@mherwege mherwege changed the title {BasicUI] icon.png minimal fix [BasicUI] icon.png minimal fix Dec 22, 2023
@kaikreuzer
Copy link
Member

Yes, but I actually like the other fix. As we have changed all icons to svg, why should we keep the png file in use here?

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

I like this one more.

@lolodomo
Copy link
Contributor

@kaikreuzer : is it still time to merge this ?

@lolodomo
Copy link
Contributor

lolodomo commented Dec 22, 2023

why should we keep the png file in use here?

You could change it later, the idea here is rather to avoid requesting the icon servlet.

@kaikreuzer
Copy link
Member

No, release build is already running.
Why do you like it more?

@lolodomo
Copy link
Contributor

lolodomo commented Dec 22, 2023

Why do you like it more?

Because with the previous quick fix the icon servlet can be requested even for stuff not related to icons ! Like image or chart elements rendering.

@lolodomo
Copy link
Contributor

I am even not sure that none.png in images has the same size as the icon.

@lolodomo
Copy link
Contributor

But it is not dramatic if not changed in 4.1.

@lolodomo
Copy link
Contributor

I am not sure of the rendering (size) of the image when the image element will use icon none.svg.

@florian-h05 florian-h05 added bug Something isn't working basic ui Basic UI labels Dec 26, 2023
@lolodomo
Copy link
Contributor

lolodomo commented Jan 2, 2024

I am going to merge this fix because after analysing the impact of the changes done by #2231 very quickly at the last minute, I realized that one of the changes broke something !

This fix reverting most of the previous fix is necessary for both reasons:

  1. It makes no sense to request the icon servlet when the purpose is to render an empty image but with absolutely no link with icon stuff. This was the reason of the static image.
  2. For real icon management, the broken case by [BasicUI] Fix icon none #2231 is if the user wants to do dynamic icon with icon "none". The icon will no more be updated with new state arriving. I agree that the probability that one user defines custom icons with none as prefix is low but there is no valid reason to not cover that case.

@lolodomo lolodomo merged commit 898b519 into openhab:main Jan 2, 2024
3 checks passed
@lolodomo lolodomo added this to the 4.2 milestone Jan 2, 2024
@lolodomo
Copy link
Contributor

lolodomo commented Jan 2, 2024

Now, yes an improvement is still possible, consisting in replacing the static none.png by a static none.svg.

@lolodomo
Copy link
Contributor

lolodomo commented Jan 2, 2024

I am going to cherry-pick this PR in branch 4.1.x.

lolodomo pushed a commit that referenced this pull request Jan 2, 2024
See comment
#2231 (comment)

This is the minimal fix to the issue.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@lolodomo lolodomo added the patch A PR that has been cherry-picked to a patch release branch label Jan 2, 2024
@mherwege mherwege deleted the none-image branch January 2, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
basic ui Basic UI bug Something isn't working patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants