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.

Sign up for GitHub

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

Jump to bottom

Add ColorUtil for better support of xyY conversion #3434

Merged
merged 3 commits into from
Mar 16, 2023
Merged

Add ColorUtil for better support of xyY conversion #3434

merged 3 commits into from
Mar 16, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Mar 7, 2023

This has been refactored to align with the usually used conversion by a lot of ZigBee products like Hue or deconz.

@J-N-K J-N-K requested a review from a team as a code owner March 7, 2023 18:34
J-N-K
J-N-K commented Mar 7, 2023
View reviewed changes
bundles/org.openhab.core/src/test/java/org/openhab/core/library/types/HSBTypeTest.java
Comment on lines +132 to +133
assertEquals(14.65, xy[0].doubleValue(), 0.01);
assertEquals(11.56, xy[1].doubleValue(), 0.01);
Copy link
Member Author

@J-N-K J-N-K Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These colors are nearly the same. Using e.g. https://www.easyrgb.com/en/convert.php#inputFORM to convert both colors to HTML color code results in #021645 and #001849 which is very close.

In general this test could be removed since it is part of the ColorUtilTest in a similar way.

@J-N-K
Add ColorUtil for better support of xyY conversion
bcc2d3b 
This has been refactored to align with the usually used conversion by a lot of ZigBee products like Hue or Deconz.

Signed-off-by: Jan N. Klug <[email protected]>
J-N-K
J-N-K commented Mar 7, 2023
View reviewed changes
bundles/org.openhab.core/src/test/java/org/openhab/core/library/types/HSBTypeTest.java
}

@Test
public void testCreateFromXY() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I have removed this test because it was invalid. The JavaDoc of HSBType.fromXY states that the parameters should be between 0 and 1.

@J-N-K J-N-K mentioned this pull request Mar 7, 2023
Merged
@holgerfriedrich
Copy link
Member

Thanks @J-N-K, I was preparing a PR as well, but of course you were faster and already included the HSBType.... 👍

@jlaur jlaur mentioned this pull request Mar 7, 2023
Closed
@holgerfriedrich holgerfriedrich mentioned this pull request Mar 8, 2023
Closed
30 tasks
@holgerfriedrich
Copy link
Member

@kaikreuzer It would be helpful to include this PR in the first milestone build, as we would like to use it in addons....

@kaikreuzer
Copy link
Member

@holgerfriedrich Ok. But it seems that you did some review comments, so I'll first wait for @J-N-K to resolve those.

@J-N-K J-N-K requested a review from holgerfriedrich March 10, 2023 16:14
holgerfriedrich
holgerfriedrich approved these changes Mar 10, 2023
View reviewed changes
Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

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

LGTM

Just a remark: I am not sure if checking the range of x and y to be in 0..1 will break a binding implementation. Looking at the usage of HSBType.fromXY it is hard to tell. EspMilightHubHandler looks a bit suspicious, as it scales up the values by 100.

@J-N-K
Copy link
Member Author

J-N-K commented Mar 10, 2023

If it does break something that should be fixed, because it was invalid before.

@J-N-K J-N-K added enhancement An enhancement or new feature of the Core rebuild Triggers the Jenkins PR build and removed rebuild Triggers the Jenkins PR build labels Mar 11, 2023
@J-N-K
Copy link
Member Author

J-N-K commented Mar 15, 2023

@kaikreuzer I think everything is addressed now.

kaikreuzer
kaikreuzer approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Great, thank you.

@kaikreuzer kaikreuzer merged commit a32f1e0 into openhab:main Mar 16, 2023
@kaikreuzer kaikreuzer added this to the 4.0 milestone Mar 16, 2023
@J-N-K J-N-K deleted the colorutil branch March 16, 2023 22:01
@florian-h05
Copy link
Contributor

@J-N-K
Probably related to this PR: Hue Binding Tests are broken, see https://github.com/openhab/openhab-addons/actions/runs/4454698602/jobs/7824057913#step:9:219

@jlaur jlaur mentioned this pull request Mar 18, 2023
Merged
@NorbertHD
Copy link

Fixes for color test are missing for MQTT Things and Channels and TRÅDFRI Binding too.

This was referenced Mar 18, 2023
Merged
Merged
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
@J-N-K @splatch
Add ColorUtil for better support of xyY conversion (openhab#3434)
61cfac8 
* Add ColorUtil for better support of xyY conversion

This has been refactored to align with the usually used conversion by a lot of ZigBee products like Hue or Deconz.

Signed-off-by: Jan N. Klug <[email protected]>
GitOrigin-RevId: a32f1e0
@jimtng jimtng mentioned this pull request Oct 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holgerfriedrich holgerfriedrich holgerfriedrich approved these changes

@kaikreuzer kaikreuzer kaikreuzer approved these changes

Assignees
No one assigned
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@J-N-K @holgerfriedrich @kaikreuzer @florian-h05 @NorbertHD