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

[blockly] Fix ZDT comparison between block on GraalJS #2040

Merged
merged 1 commit into from
Aug 26, 2023

Conversation

Dooleweerdt
Copy link
Contributor

Fixes #2038

Signed-off-by: Brian Dooleweerdt Rasmussen <brian.rasmussen80@gmail.com>
@Dooleweerdt Dooleweerdt requested a review from a team as a code owner August 23, 2023 17:59
@Dooleweerdt
Copy link
Contributor Author

Another way of solving this could be to move the original line in 740 to 750 (into the else clause of isGraalJS):
let zdtCompare = addDateComparisonSupportNashorn()
zdtCompare is only used in this part of the function.

Copy link
Contributor

@stefan-hoehn stefan-hoehn left a comment

Choose a reason for hiding this comment

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

Thx @Dooleweerdt to catch this and provide the fix.

@openhab/webui-maintainers This fix looks good to me as it is similar to how it is done in "oh_zdt_compare". Do you mind merging it?

@relativeci
Copy link

relativeci bot commented Aug 26, 2023

Job #1129: Bundle Size — 15.75MiB (-0.03%).

fccf40b(current) vs c80db9e main#1128(baseline)

⚠️ Bundle contains 16 duplicate packages

Metrics (no changes)
                 Current
Job #1129
     Baseline
Job #1128
Initial JS 1.67MiB 1.67MiB
Initial CSS 608.89KiB 608.89KiB
Cache Invalidation 93.94% 93.94%
Chunks 219 219
Assets 689 689
Modules 1700 1700
Duplicate Modules 90 90
Duplicate Code 1.95% 1.95%
Packages 138 138
Duplicate Packages 15 15
Total size by type (2 changes)
                 Current
Job #1129
     Baseline
Job #1128
CSS 859.34KiB 859.34KiB
Fonts 526.1KiB 526.1KiB
HTML 1.23KiB 1.23KiB
IMG 140.74KiB 140.74KiB
JS 9.25MiB (-0.03%) 9.25MiB
Media 295.6KiB 295.6KiB
Other 4.72MiB (-0.02%) 4.72MiB

View job #1129 reportView main branch activity

Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@florian-h05 florian-h05 changed the title Add correct zdtCompare code for DateComparison Between block [blockly] Fix DateComparison Between block for GraalJS Aug 26, 2023
@florian-h05 florian-h05 changed the title [blockly] Fix DateComparison Between block for GraalJS [blockly] Fix ZDT comparison Between block on GraalJS Aug 26, 2023
@florian-h05 florian-h05 changed the title [blockly] Fix ZDT comparison Between block on GraalJS [blockly] Fix ZDT comparison between block on GraalJS Aug 26, 2023
@florian-h05 florian-h05 merged commit 9599550 into openhab:main Aug 26, 2023
4 checks passed
@florian-h05 florian-h05 added bug Something isn't working main ui Main UI labels Aug 26, 2023
@florian-h05 florian-h05 added this to the 4.1 milestone Aug 26, 2023
@MarkusThur
Copy link

question...
How to get the fix, once it is merged, as i did run into that issue also :-)

Intermediate workarounds I found:

Variant a: set scripting language to ECMA - 262 Edition 5.1 (Nashorn)

Variant b: dont use the IS - AFTER block when between block was used => reformulate it as IS - BETWEEN - AND block, which should be possible in most cases
AFTER equals between and <something which is definitely in the future, like testitem + 1 year>
BEFORE equals <something definitely far in the past like testitem - 1 year> and
If after or equal or before or equal are critical or the precision is critical, the one needs also proper mathematical transformations of that to be aplied to the testitem and the timepoint to test against.

@florian-h05
Copy link
Contributor

How to get the fix, once it is merged, as i did run into that issue also :-)

Either install the openHAB 4.1.0 SNAPSHOTs (I do not recommend this unless you are one of the very technical openHAB users and willing to regularly take care of it) or wait for a 4.0.x patch release. I will backport this.

@florian-h05 florian-h05 added patch A PR that has been cherry-picked to a patch release branch and removed patch A PR that has been cherry-picked to a patch release branch labels Aug 27, 2023
@MarkusThur
Copy link

How to get the fix, once it is merged, as i did run into that issue also :-)

Either install the openHAB 4.1.0 SNAPSHOTs (I do not recommend this unless you are one of the very technical openHAB users and willing to regularly take care of it) or wait for a 4.0.x patch release. I will backport this.

Thank you, somehow I am normally the guy for snapshots, but as I didn't understand or didn't investigate deep enough how to setup a full working dev environment, and not fully in the picture on the release logic, I think I will wait for the back port, especially as i have a working solution, which is not even that unelegant. Where maybe the reverse workaround formulating between as after and before clause is even more powerful.
Just a short warning. Think i did find a bug with "copy of datetime" today also.. Have to try to reproduce it.. Wanted to create last midnight, 8 am, 4 pm and next midnight, by using "copy of block" with now and set 0 hours, 0 minutes, 0 seconds.. But the result was that the milliseconds still remained, so it was 00:00:00.xxx 08:00:00.xxx 16:00:00.xxx and next day 00:00:00.xxx where xxx is the miliseconds when now was evaluated first time.. I tried to set Millis, micros and Nanos also to zero.. No difference.. It stayed that my created times are always exactly the amount to late that now is after the full second on evaluation time.. But i have to test that further and produce a better descriprion

@stefan-hoehn
Copy link
Contributor

Think i did find a bug with "copy of datetime" today also..

Please, in this case open a new issue for that with an example on how to reproduce it. Thanks.

florian-h05 added a commit that referenced this pull request Aug 29, 2023
Fixes #2038

Signed-off-by: Brian Dooleweerdt Rasmussen
<brian.rasmussen80@gmail.com>
@florian-h05 florian-h05 added the patch A PR that has been cherry-picked to a patch release branch label Aug 29, 2023
stefan-hoehn pushed a commit to stefan-hoehn/openhab-webui that referenced this pull request Sep 23, 2023
Fixes openhab#2038

Signed-off-by: Brian Dooleweerdt Rasmussen <brian.rasmussen80@gmail.com>
Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
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 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.

[Blockly] Adding a DateComparison block always adds the Nashorn JS code
4 participants