-
-
Notifications
You must be signed in to change notification settings - Fork 422
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
Enhance ThingActions UI support #4392
base: main
Are you sure you want to change the base?
Conversation
For the following action
The GET API now returns:
An example with the POST API using these input values in API explorer:
The logs:
And the returned response body:
|
...ation.rest/src/main/java/org/openhab/core/automation/rest/internal/ThingActionsResource.java
Outdated
Show resolved
Hide resolved
...ation.rest/src/main/java/org/openhab/core/automation/rest/internal/ThingActionsResource.java
Outdated
Show resolved
Hide resolved
...ation.rest/src/main/java/org/openhab/core/automation/rest/internal/ThingActionsResource.java
Outdated
Show resolved
Hide resolved
7bf450e
to
2ca7f24
Compare
2ca7f24
to
9e04605
Compare
@lolodomo The |
I don't understand, Thing actions can already be invoked from rules. ??? |
After reading #1745 completely, yes they could in theory - the problem is however the same we attempt to fix with this PR: The Thing action module types only contain the inputs like the |
@lolodomo Please do not force push from now on - I will start working on your PR as well and create a PR to your PR branch afterwards. |
@lolodomo I have created lolodomo#3, which adds support for invoking Thing actions through UI-based rules. Please merge it with rebase merge to keep commit history. |
f582e92
to
f1e4101
Compare
Thanks for integrating my changes ... now we have to update the PR description: Enhance ThingActions UI supportFixes #1745. Return config description parameters for the ActionInputs of ThingActions for the REST GET Enhance the REST POST This will be used by the UI's Thing page and rule editor to allow invoking Thing actions through the UI or adding them to UI-bases rules. |
When PR is ready for review, I will squash everything, provide a proper description and mention you as co-author. |
...on/src/main/java/org/openhab/core/automation/util/mapper/SerialisedInputsToActionInputs.java
Outdated
Show resolved
Hide resolved
.../java/org/openhab/core/automation/util/mapper/ActionInputsToConfigDescriptionParameters.java
Outdated
Show resolved
Hide resolved
.../java/org/openhab/core/automation/util/mapper/ActionInputsToConfigDescriptionParameters.java
Show resolved
Hide resolved
47e8144
to
c6d8c3c
Compare
.../java/org/openhab/core/automation/util/mapper/ActionInputsToConfigDescriptionParameters.java
Outdated
Show resolved
Hide resolved
@florian-h05 : I believe it is almost ready now. Can you please have a look to my last commits before I squash everything and update the 2 first messages ? |
I am tempted to change the expected format for "datetime" context. |
I will do.
Agreed, having seconds is better than not having seconds - and as it is not yet used, there should be no problems. |
Done. I also updated my second message with a full example when using GET and POST API. |
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, thanks for your work as well as improving what I have contributed!
Please update the PR title and description, I have already proposed a new one in #4392 (comment),
and add a joint sign-off.
Just two comments:
* | ||
* @return true if the unit of measurements of this parameter is amongst a list of predefined values or false if not | ||
*/ | ||
public boolean isUnitRestricted() { |
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.
For what is that unit restriction required?
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.
I believe it is to check that a developer did not enter an invalid unit in a binding XML file. The XSD probably defines a particular list of accepted values for the unit attribute.
But I agree that removing this check would be another option.
...in/java/org/openhab/core/automation/thingsupport/AnnotatedThingActionModuleTypeProvider.java
Outdated
Show resolved
Hide resolved
// fallback to configuration as this is where the UI stores the input values | ||
if (value == null) { | ||
value = SerialisedInputsToActionInputs.map(moduleType, moduleType.getInputs().get(i), | ||
module.getConfiguration().get(inputAnnotation.name()), null); | ||
} |
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.
@florian-h05 : you added these lines but to be honnest with you I don't understand in which conditions this is used. As unit provider is not provided in this case, I would like to understand the potential impact.
PS: I am going to add a log to see if this is used when I use the REST API but in principle it should not be required for this usage.
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.
UnitProvider should be used in this case as well.
This change is required to make Thing actions usable in UI-based rules.
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.
If I used null, that means UnitProvider was not available...
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.
Can you change that? Or have you already changed it? I cannot have a look at the code as I’m currently away from keyboard …
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.
Not yet.
I have to think how I could retrieve this service in that class...
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.
This is a problem only for QuantityType inputs, you loose the ability to provide a simple number.
By the way, I have an improvement to apply, unitProvider is not required in case the input is a string containing the number AND a unit.
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.
The current workaround in Main UI is that you create a string containing the value and the unit when calling the action, instead of just providing a number.
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.
The current workaround in Main UI is that you create a string containing the value and the unit when calling the action, instead of just providing a number.
In that case let’s keep this PR as is and we will do a follow-up PR where we will introduce „real“ QuantityType input support in the UI.
Fixes openhab#1745 Return config description parameters for the ActionInputs of ThingActions for the REST GET /action/{thingUID} and REST GET /module-types endpoints. The config description parameters are only provided if all input parameters have a type that can be mapped to a config description parameter (String, boolean, Boolean, byte, Byte, short, Short, int, Integer, long, Long, float, Float, double, Double, Number, DecimalType, QuantityType<?>, LocalDateTime, LocalDate, LocalTime, ZonedDateTime, Date, Instant and Duration). Enhance the REST POST /actions/{thingUID}/{actionUid} endpoint (allows invoking Thing actions via REST) and the AnnotationActionHandler (allows invoking Thing actions from UI-rules) in order to be more flexible regarding the type of each provided argument value and to map the value to the expected data type. Number and string values will be accepted as inputs and the expected data type will be created from this value. This will be used by the UI's Thing page and rule editor to allow invoking Thing actions through the UI or adding them to UI-bases rules. Signed-off-by: Laurent Garnier <[email protected]> Signed-off-by: Florian Hotze <[email protected]>
95ddb01
to
b308cd7
Compare
Now ready for an "official" review. |
Also factorize the log of conversion failing. Signed-off-by: Laurent Garnier <[email protected]>
Fixes #1745
Return config description parameters for the ActionInputs of ThingActions for the REST GET
/action/{thingUID}
and REST GET/module-types endpoints
.The config description parameters are only provided if all input parameters have a type that can be mapped to a config description parameter (String, boolean, Boolean, byte, Byte, short, Short, int, Integer, long, Long, float, Float, double, Double, Number, DecimalType,
QuantityType<?>
, LocalDateTime, LocalDate, LocalTime, ZonedDateTime, Date, Instant and Duration).Enhance the REST POST
/actions/{thingUID}/{actionUid}
endpoint (allows invoking Thing actions via REST) and theAnnotationActionHandler
(allows invoking Thing actions from UI-rules) in order to be more flexible regarding the type of each provided argument value and to map the value to the expected data type. Number and string values will be accepted as inputs and the expected data type will be created from this value.This will be used by the UI's Thing page and rule editor to allow invoking Thing actions through the UI or adding them to UI-bases rules.
Signed-off-by: Laurent Garnier [email protected]
Signed-off-by: Florian Hotze [email protected]