Refactor ScriptExecution to plain Java ScriptExtension #3155

J-N-K · 2022-11-09T20:18:28Z

JSR-223 automation add-ons often use the script actions from the org.openhab.core.model.script bundle. The downside is that the actions are tied to XText, making it impossible for the automation add-ons to modify/wrap these actions without explicit dependency on XText (see #3128).

This is the first step to refactor these actions to plain Java, making them available as ScriptExtension via an import-preset ScriptAction which is not imported by default to prevent unwanted shadowing of other components. They are wrapped for DSL rules, to stay backward compatible.

Signed-off-by: Jan N. Klug github@klug.nrw

Signed-off-by: Jan N. Klug <github@klug.nrw>

openhab-bot · 2022-11-09T20:18:52Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/js-scripting-why-are-we-deprecated-createtimer/140748/15

J-N-K · 2022-11-09T20:42:43Z

@florian-h05 Is that what you would expect?

florian-h05 · 2022-11-09T21:39:42Z

Wow, that was fast Jan 👍
Looks good on the first view, but I have to checkout to have a closer look.

ccutrer

This is much nicer. I was just working with createTimer from the jrubyscripting library yesterday, and was wondering why it wasn't just using a Runnable.

...e.script/src/main/java/org/openhab/core/automation/module/script/action/ScriptExecution.java

Signed-off-by: Jan N. Klug <github@klug.nrw>

jlaur · 2022-11-10T21:36:02Z

...e.script/src/main/java/org/openhab/core/automation/module/script/action/ScriptExecution.java

+     * @param closure the code block to execute
+     * @return a handle to the created timer, so that it can be canceled or rescheduled
+     */
+    Timer createTimer(ZonedDateTime instant, Runnable closure);


The naming of the ZonedDateTime as instant triggers me to ask the question: Would an Instant be sufficient here, since the the point in time doesn't care about time-zone? If ZonedDateTime is more convenient, consider if parameter should be renamed for clarity, and/or if a variant taking Instant would be relevant.

The docs actually say it should be an AbstractInstant.

AbstractInstant is surely wrong because that's a class from joda-time which we don't use anymore. So the docs need to be updated.

IMO an Instant would also be fine (also see my comment on #2898) but I re-implemented what we have in DSL and that is a ZonedDateTime. If we try to solve that now, we won't get a solution before next release.

Out of interest: Why can't we get a solution if we try that before next release?

I always felt that the docs references to abstract classes like this was confusing to new users anyway. I'm not sure Instant is any better but it's at least consistent. When/if this ever changes here to Instant, we should also change the persistence actions which are documented as using ZonedDateTime to be consistent.

That would be my proposal. IMO user-facing ZDT and Instant don‘t make much difference, so using Instant internally in DateTimeType and ZDT in rules is fine. Does it make a difference in JS?

Does it make a difference in JS?

Regarding the compatibility, no.
We are using JS-Joda as date and time library, which provides the same functionality that the Joda time library had (ZonedDateTime, Instant and so on).
When core moves from ZonedDateTime to Instant, we only need to use Instant from JS-Joda instead of ZonedDateTime from JS-Joda.

If there is no real benefit (I don't see one for DSL or JavaRule), we should probably stay where we are.

The only difference I can see is, which argument and which string the parse function takes and that instance has function like ofEpochMillis. But we have a nice utility function that parses many different date and/or time representations to a ZonedDateTime, so I can’t see any benefit in using an Instant.

Given the large number of scripts that have to be updated when that’s changed to an Instant, I agree that we should probably stay on ZonedDateTime.
The question for now is: should we rename the parameter from instant to zdt or zonedDateTime or something like that?

Yes, I‘ll update that.

jlaur · 2022-11-10T21:37:36Z

...org.openhab.core.model.script/src/org/openhab/core/model/script/actions/ScriptExecution.java

-            closure.apply();
-        }, identifier);
+    @ActionDoc(text = "create an identifiable timer ")
+    public static Timer createTimer(@Nullable String identifier,  ZonedDateTime zonedDateTime, Procedures.Procedure0 closure) {


Suggested change

public static Timer createTimer(@Nullable String identifier, ZonedDateTime zonedDateTime, Procedures.Procedure0 closure) {

public static Timer createTimer(@Nullable String identifier, ZonedDateTime zonedDateTime, Procedures.Procedure0 closure) {

jlaur · 2022-11-10T21:37:47Z

...org.openhab.core.model.script/src/org/openhab/core/model/script/actions/ScriptExecution.java

-            closure.apply(arg1);
-        }, identifier);
+    @ActionDoc(text = "create an identifiable timer with argument")
+    public static Timer createTimerWithArgument(@Nullable String identifier,  ZonedDateTime zonedDateTime, Object arg1, Procedures.Procedure1 closure) {


Suggested change

public static Timer createTimerWithArgument(@Nullable String identifier, ZonedDateTime zonedDateTime, Object arg1, Procedures.Procedure1 closure) {

public static Timer createTimerWithArgument(@Nullable String identifier, ZonedDateTime zonedDateTime, Object arg1, Procedures.Procedure1 closure) {

florian-h05 · 2022-11-10T22:24:09Z

@J-N-K
I had the time to check this out, and this is exactly what I expected.
Thanks 👍
JS Scripting PR is now open: openhab/openhab-addons#13695

Just one thing I am wondering: I've seen that you implemented a ScriptExtensionProvider for the actions, but I am wondering how I could access this script extension from JS Scripting (the JS code). I have dumped the object returned by require('@runtime') (this is the way of accessing itemRegistry, events and so on from JS code), and I can neither see ScriptAction nor scriptExtension.
FYI this is technically not required for the timers, but could you maybe point me in the right direction?

openhab-bot · 2022-11-10T22:46:13Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/js-scripting-why-are-we-deprecated-createtimer/140748/17

florian-h05 · 2022-11-12T14:34:47Z

@J-N-K What do you think, when does this PR get merged?

J-N-K · 2022-11-12T15:55:55Z

I'm probably the last person with an answer to this: I can't review and merge my own PR.

florian-h05 · 2022-11-12T22:42:41Z

@kaikreuzer @cweitkamp @wborn Just asking, WDYT when can you review and merge this PR? openhab/openhab-addons#13695 depends on this.

Signed-off-by: Jan N. Klug <github@klug.nrw>

kaikreuzer

lgtm, thanks!

* Refactor ScriptExecution to plain Java ScriptExtension * keep callScript backward compatible Signed-off-by: Jan N. Klug <github@klug.nrw> GitOrigin-RevId: c340e8d

Refactor ScriptExecution to plain Java ScriptExtension

8ba8dac

Signed-off-by: Jan N. Klug <github@klug.nrw>

J-N-K added the automation label Nov 9, 2022

J-N-K requested a review from a team as a code owner November 9, 2022 20:18

ccutrer reviewed Nov 10, 2022

View reviewed changes

...e.script/src/main/java/org/openhab/core/automation/module/script/action/ScriptExecution.java Outdated Show resolved Hide resolved

keep callScript backward compatible

b379246

Signed-off-by: Jan N. Klug <github@klug.nrw>

jlaur reviewed Nov 10, 2022

View reviewed changes

florian-h05 mentioned this pull request Nov 11, 2022

[jsscripting] Reimplement timer creation method of ScriptExecution openhab/openhab-addons#13695

Merged

J-N-K added the enhancement An enhancement or new feature of the Core label Nov 13, 2022

address review comments

ec13286

Signed-off-by: Jan N. Klug <github@klug.nrw>

kaikreuzer approved these changes Nov 19, 2022

View reviewed changes

kaikreuzer merged commit c340e8d into openhab:main Nov 19, 2022

kaikreuzer added this to the 3.4 milestone Nov 19, 2022

J-N-K deleted the feature-javascriptexecution branch November 19, 2022 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ScriptExecution to plain Java ScriptExtension #3155

Refactor ScriptExecution to plain Java ScriptExtension #3155

J-N-K commented Nov 9, 2022 •

edited

Loading

openhab-bot commented Nov 9, 2022

J-N-K commented Nov 9, 2022

florian-h05 commented Nov 9, 2022

ccutrer left a comment

jlaur Nov 10, 2022

rkoshak Nov 14, 2022

J-N-K Nov 14, 2022 •

edited

Loading

florian-h05 Nov 14, 2022 •

edited

Loading

rkoshak Nov 14, 2022

J-N-K Nov 14, 2022

florian-h05 Nov 15, 2022

J-N-K Nov 15, 2022

florian-h05 Nov 15, 2022 •

edited

Loading

J-N-K Nov 15, 2022

jlaur Nov 10, 2022

jlaur Nov 10, 2022

florian-h05 commented Nov 10, 2022 •

edited

Loading

openhab-bot commented Nov 10, 2022

florian-h05 commented Nov 12, 2022

J-N-K commented Nov 12, 2022

florian-h05 commented Nov 12, 2022

kaikreuzer left a comment

	public static Timer createTimer(@Nullable String identifier, ZonedDateTime zonedDateTime, Procedures.Procedure0 closure) {
	public static Timer createTimer(@Nullable String identifier, ZonedDateTime zonedDateTime, Procedures.Procedure0 closure) {

	public static Timer createTimerWithArgument(@Nullable String identifier, ZonedDateTime zonedDateTime, Object arg1, Procedures.Procedure1 closure) {
	public static Timer createTimerWithArgument(@Nullable String identifier, ZonedDateTime zonedDateTime, Object arg1, Procedures.Procedure1 closure) {

Refactor ScriptExecution to plain Java ScriptExtension #3155

Refactor ScriptExecution to plain Java ScriptExtension #3155

Conversation

J-N-K commented Nov 9, 2022 • edited Loading

openhab-bot commented Nov 9, 2022

J-N-K commented Nov 9, 2022

florian-h05 commented Nov 9, 2022

ccutrer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-N-K Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

florian-h05 Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florian-h05 Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florian-h05 commented Nov 10, 2022 • edited Loading

openhab-bot commented Nov 10, 2022

florian-h05 commented Nov 12, 2022

J-N-K commented Nov 12, 2022

florian-h05 commented Nov 12, 2022

kaikreuzer left a comment

Choose a reason for hiding this comment

J-N-K commented Nov 9, 2022 •

edited

Loading

J-N-K Nov 14, 2022 •

edited

Loading

florian-h05 Nov 14, 2022 •

edited

Loading

florian-h05 Nov 15, 2022 •

edited

Loading

florian-h05 commented Nov 10, 2022 •

edited

Loading