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

oh-colorpicker: Fix color picker sends commands on external state change #2860

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

florian-h05
Copy link
Contributor

Fixes #1268.
Fixes #2849.

Copy link

relativeci bot commented Nov 4, 2024

#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  Change 2 changes Improvement 1 improvement
                 Current
#2464
     Baseline
#2463
Improvement  Initial JS 1.9MiB(-0.02%) 1.9MiB
No change  Initial CSS 577.31KiB 577.31KiB
Change  Cache Invalidation 17.47% 20.01%
No change  Chunks 226 226
No change  Assets 249 249
No change  Modules 2939 2939
No change  Duplicate Modules 152 152
No change  Duplicate Code 1.8% 1.8%
No change  Packages 96 96
No change  Duplicate Packages 2 2
Bundle size by type  Change 1 change Improvement 1 improvement
                 Current
#2464
     Baseline
#2463
Improvement  JS 9.07MiB (~-0.01%) 9.07MiB
No change  CSS 864KiB 864KiB
No change  Fonts 526.1KiB 526.1KiB
No change  Media 295.6KiB 295.6KiB
No change  IMG 140.74KiB 140.74KiB
No change  HTML 1.38KiB 1.38KiB
No change  Other 871B 871B

Bundle analysis reportBranch florian-h05:colorpickerProject dashboard


Generated by RelativeCIDocumentationReport issue

@digitaldan
Copy link
Contributor

digitaldan commented Nov 4, 2024

this bug has caused me so many headaches when testing Color items......thank you !

@florian-h05
Copy link
Contributor Author

@digitaldan You’re welcome — Can you please test the fix and also check if you notice any regressions?

@andrewfg
Copy link

andrewfg commented Nov 5, 2024

@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:

  1. Try to suppress the second POST wherever possible (which is what you are already doing), plus
  2. In the case that the notifications are delayed outside the time window so that the second POST is not suppressed, then please do not round the POST'ed values to the nearest integer, but simply repost the same values that were originally provided in the notification.

@florian-h05
Copy link
Contributor Author

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?

@andrewfg
Copy link

andrewfg commented Nov 5, 2024

The second POST should always be suppresed, the UI should never round an incoming state and POST it.

I agree with your statement as an aspiration. But from just looking at your code I was wondering if the aspiration has yet become reality? Admittedly my knowledge of f7 is vanishingly small.

image

image

@florian-h05
Copy link
Contributor Author

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.
The rounding is only applied when actually using the color picker to control the Item.

@andrewfg
Copy link

andrewfg commented Nov 5, 2024

^

Have you been able to give it a try?

Ok. Building it now..

@florian-h05
Copy link
Contributor Author

Succeeded with building?

@andrewfg
Copy link

andrewfg commented Nov 5, 2024

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

@florian-h05
Copy link
Contributor Author

Can you please check the Main UI Commit on the About page?

@andrewfg
Copy link

andrewfg commented Nov 5, 2024

the Main UI Commit on the About page?

1612d50

@florian-h05
Copy link
Contributor Author

That’s not this PR, seems something went wrong when building or deploying this PR.

@andrewfg
Copy link

andrewfg commented Nov 5, 2024

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.

@florian-h05
Copy link
Contributor Author

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 bundle:update through the Karaf console.

@andrewfg
Copy link

andrewfg commented Nov 5, 2024

built the UI bundle with my change for you

Many thanks @florian-h05 .. I will test it tomorrow.

@andrewfg
Copy link

andrewfg commented Nov 6, 2024

@florian-h05 many apologies but I am really struggling with running the tests. Since I could not figure out the syntax of bundle:update on Windows, I ended up making a new linux install on an RPi. I installed it with OH latest snapshot dated '202411030259' and I updated the UI bundle with the jar you posted above dated '202411022308' (see below). However the UI Help About is still showing 'Main UI Commit 1612d50' and the UI is still sending unwanted POSTs for the color picker.

image


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 :)

Copy link

@andrewfg andrewfg left a comment

Choose a reason for hiding this comment

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

LGTM

@florian-h05
Copy link
Contributor Author

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.

@florian-h05 florian-h05 marked this pull request as ready for review November 6, 2024 13:10
@florian-h05 florian-h05 requested a review from ghys as a code owner November 6, 2024 13:10
@florian-h05 florian-h05 added this to the 4.3 milestone Nov 6, 2024
@florian-h05 florian-h05 added bug Something isn't working main ui Main UI labels Nov 6, 2024
@florian-h05 florian-h05 merged commit 1e4e69e into openhab:main Nov 6, 2024
8 checks passed
@florian-h05 florian-h05 deleted the colorpicker branch November 6, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working main ui Main UI
Projects
None yet
3 participants