-
-
Notifications
You must be signed in to change notification settings - Fork 239
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
oh-colorpicker: Fix color picker sends commands on external state change #2860
Conversation
#2464 Bundle Size — 10.86MiB (~-0.01%).5937054(current) vs e32700f main#2463(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
Current #2464 |
Baseline #2463 |
|
---|---|---|
Initial JS | 1.9MiB (-0.02% ) |
1.9MiB |
Initial CSS | 577.31KiB |
577.31KiB |
Cache Invalidation | 17.47% |
20.01% |
Chunks | 226 |
226 |
Assets | 249 |
249 |
Modules | 2939 |
2939 |
Duplicate Modules | 152 |
152 |
Duplicate Code | 1.8% |
1.8% |
Packages | 96 |
96 |
Duplicate Packages | 2 |
2 |
Bundle size by type 1 change
1 improvement
Current #2464 |
Baseline #2463 |
|
---|---|---|
JS | 9.07MiB (~-0.01% ) |
9.07MiB |
CSS | 864KiB |
864KiB |
Fonts | 526.1KiB |
526.1KiB |
Media | 295.6KiB |
295.6KiB |
IMG | 140.74KiB |
140.74KiB |
HTML | 1.38KiB |
1.38KiB |
Other | 871B |
871B |
Bundle analysis report Branch florian-h05:colorpicker Project dashboard
Generated by RelativeCI Documentation Report issue
e80dd33
to
8c021a2
Compare
this bug has caused me so many headaches when testing Color items......thank you ! |
@digitaldan You’re welcome — Can you please test the fix and also check if you notice any regressions? |
@florian-h05 many thanks for this. I see that your current code in this PR is focussed on supressing the second POST during a certain time window. This is certainly a good step forward. However in the case of #2849 there is a further refinement which I hope you can add to this PR. It concerns the case where there is both a color temperature picker and a color picker on the same UI widget. Background: Color temperatures are a very specific subset of the full CIE color space. To be specific they are colors lying on the Planckian locus in the CIE color XY space. So when the UI widget sends a color temperature POST command, it causes the binding to respond with TWO event notifications for two attributes -- namely 1) the just changed color temperature (in Kelvin) plus 2) the corresponding color XY point on the Planckian locus. The specific issue in #2849 is that the notification 2) is an accurate location that lies precisely on the Planckian locus so the notification comprises HSB values with double precision to several decimal places. However when the UI widget sends the second (unwanted) POST it rounds the double precision HSB values to the nearest integers which it POSTs back to the binding. This causes the color XY to move to a slightly different point in the CIE color space so it may 'fall off' the Planckian locus. And that in turn causes the binding to notify that the color temperature has now become UNDEF. So my request to you is for two things in this PR:
|
The second POST should always be suppresed, the UI should never round an incoming state and POST it. Have you been able to give it a try? |
If the Item receives an update, oh-colorpicker will ignore any change from the f7 color picker for 10 ms, which should be enough time to catch the f7 color picker changing because it was updated to the new state. |
^
Ok. Building it now.. |
Succeeded with building? |
Yes. But.. Maybe I built it wrong with the original code. Or maybe your new code is still wrong.. Screen_Recording_20241105_144142_Chrome.mp4 |
Can you please check the Main UI Commit on the About page? |
|
That’s not this PR, seems something went wrong when building or deploying this PR. |
I just tried another build. But it still doe not work. I think I have to wait untilthere is a proper snapshot and then I can test that. |
I built the UI bundle with my change for you: org.openhab.ui-4.3.0-SNAPSHOT.zip I don't think it will work to just put it in the addons folder, instead put it somehwere else and install it with |
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
8c021a2
to
5937054
Compare
Many thanks @florian-h05 .. I will test it tomorrow. |
@florian-h05 many apologies but I am really struggling with running the tests. Since I could not figure out the syntax of EDIT: bundle:refresh / daemon-reload / clean-cache / restart seems to have installed it! The bundle versions seem not to have changed, but the Help About now shows PR 8c021a2 .. and .. apparently there are no more unwanted POSTs :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Great! As I haven‘t noticed any regressions, let‘s merge this now and have it tested a bit further in the snapshots before milestone 3 on sunday. |
Fixes #1268.
Fixes #2849.