[actions] Add wrappers around actions to avoid type problems

[rkoshak]

I've encountered this issue on the forum a couple of times already and suspect it's only going to become more of a problem as more people adopt JS Scripting.

Take actions.Semantic for example. Each and every method on this Action requires a an OH Java Item Object, which we've gone out of our way to try to hide from the end users. It's jarring to the end user and we've not even documented that .rawItem exists so the users are left scratching their heads.

Obviously the simple thing to do would be to document that rawItem exists and that it's needed when calling the core actions. But it seems like we could create a wrapper or monkey patch those core actions that present this sort of type problem so they will accept our JS Item Object or even the Item name and accept other similar pure JS types in places that require openHAB Java Types (e.g. PercentType is required for actions that adjust the sound volume).

I think that time.ZonedDateTime already works just fine for those Actions that require a java.time.ZonedDateTime like createTimer() and getBankHolidayName(). Binding provided actions can fend for themselves. So the only troublesome actions are:

• Audio actions that take a PercentType for the volume
• maybe the BusEvent storeStates and restoreStates actions, though those may be OK as is
• executeCommandLine that takes a Duration, need to verify that time.Duration works instead of java.time.Duration
• the Semantic actions
• Voice.say actions that take a PercentType to change the volume

This seems to fall within the scope of what this library should do but want to raise the discussion before I start writing a PR for it. I personally don't use any of these Actions so I'll have to set up a testbed to verify them all.

[digitaldan]

Right, this is problematic. We could try and wrap all actions, but i fear this is going to be a maintenance issue as additional actions are added.

In the binding as you know we translate both js-joda ZonedDateTime and Duration parameters to Java native versions like:

    .targetTypeMapping(Value.class, ZonedDateTime.class, (v) -> v.hasMember("withFixedOffsetZone"), v -> {
        return ZonedDateTime
                .parse(v.invokeMember("withFixedOffsetZone").invokeMember("toString").asString());
    }, HostAccess.TargetMappingPrecedence.LOW)

    // Translate JS-Joda Duration to java.time.Duration
    .targetTypeMapping(Value.class, Duration.class,
            // picking two members to check as Duration has many common function names
            (v) -> v.hasMember("minusDuration") && v.hasMember("toNanos"), v -> {
                return Duration.ofNanos(v.invokeMember("toNanos").asLong());
            }, HostAccess.TargetMappingPrecedence.LOW)

we could add something like this to match Items

.targetTypeMapping(Value.class, Item.class,
    (v) -> v.hasMember("rawItem"), v -> {
        return v.getMember("rawItem").as(Item.class);
    }, HostAccess.TargetMappingPrecedence.LOW)

[rkoshak]

I didn't know that that mapping happened in the binding. That's useful to know. I thought maybe GraalVM handled it somehow.

Should I move this issue to the add-on then? I think implementing it there as you suggest would be more consistent with what's already been done for these other types.

[digitaldan]

Should I move this issue to the add-on then? I think implementing it there as you suggest would be more consistent with what's already been done for these other types.

Sounds good! I'll take a stab at this tonight and get a PR and jar posted soon.

[digitaldan]

updated binding at https://github.com/digitaldan/openhab-addons/releases/tag/jsscripting-item-proxy

I don't use these actions either , but this seems to fix the Semantic compatibility.

Voice.say actions that take a PercentType to change the volume

Hmm, i wonder how most people use that? Is volume something that is pulled from an item, or do most people hardcode this in their script ? I can probably add something for this as well like:

.targetTypeMapping(Value.class, PercentType.class, (v) -> v.fitsInInt(), v -> {
      return new PercentType(v.asInt());
  }, HostAccess.TargetMappingPrecedence.LOW)

It would be nice to have a standard mapping of native OH types instead of doing them one by one as we need them, but i'm not clear what that would be atm.

[rkoshak]

I don't know if people often pull this from an Item. I get the impression that most people want to set the volume for the say followed by restoring the volume back to what it was. PercentType is used because that's what the action requires. It makes some sense since the volume is usually represented as a percent.

It would be nice to have a standard mapping of native OH types instead of doing them one by one as we need them, but i'm not clear what that would be atm.

I agree. The troublesome types will be the compound types like HSBType and QuantityType. Do we have other places besides the Actions where this type conversion is required?

[digitaldan]

I agree. The troublesome types will be the compound types like HSBType and QuantityType. Do we have other places besides the Actions where this type conversion is required?

I'm not sure, I don't do much with those in my own rules.

Let me know if the percent fix works for you.

[florian-h05]

So the only troublesome actions are:

• Audio actions that take a PercentType for the volume
• maybe the BusEvent storeStates and restoreStates actions, though those may be OK as is
• executeCommandLine that takes a Duration, need to verify that time.Duration works instead of java.time.Duration
• the Semantic actions
  -Voice.say actions that take a PercentType to change the volume

• For Audio and Voice actions, I would wrap them in the library to accept integers. I can’t imagine how to do that in the binding without getting into trouble at some other places. I would extend the core actions to also accept float: [actions] Accept float for Audio and Voice actions in addition to PercentType openhab-core#3331 / Actions: Add method overloads for PercentType to Audio & Voice openhab-core#3352.
• BusEvent storeStates and restoreStates needs especially for storeStates the Java Item, I would implement a JS Item -> Java Item conversion in the Addon
• executeCommandLine works with JS-Joda Duration, this is already converted in the Addon
• Semantics actions are unlikely to be used as you can access most of their functionality on the item itself, but we are missing a note in the Semantics actions docs that reference the Item.semantics member, see [actions] Semantics: Recommend to use Item.semantics #236

We should also convert the new JS Quantity to QuantityType inside the addon.

FYI: We are already wrapping a few actions to do Argument type checks and avoid wrong arguments going to the Java layer, because the exception from there is difficult to trace back; and to get type definitions for actions.

[florian-h05]

Closing as both the core PR to accept floats for Audio and Voice actions volume (openhab/openhab-core#3352) and the addon PR to convert JS Item & Quantity to the Java counterparts (openhab/openhab-addons#14335) are merged.