-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
Thing page: Support invoking Thing actions & Viewing their output #2818
Conversation
…utput Refs openhab/openhab-core#4392. Closes openhab#2817. This adds a new section "Actions" to the Thing tab of the Thing page, which provides a button for each UI-supported Thing action. Clicking on that button will open a popup, where action input can be configured and action output can be viewed. Currently, action output is displayed pretty for the `result`, `qrPairingCode` and `manualPairingCode` keys of the response object. In addition to that, the raw output can be viewed. Signed-off-by: Florian Hotze <dev@florianhotze.com>
@digitaldan WDYT? |
#2388 Bundle Size — 10.84MiB (+0.04%).cd8abd9(current) vs 85c3c31 main#2386(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
Current #2388 |
Baseline #2386 |
|
---|---|---|
Initial JS | 1.9MiB |
1.9MiB |
Initial CSS | 577.21KiB |
577.21KiB |
Cache Invalidation | 23.61% |
22.77% |
Chunks | 226 |
226 |
Assets | 249 |
249 |
Modules | 2923 (+0.1% ) |
2920 |
Duplicate Modules | 149 |
149 |
Duplicate Code | 1.8% |
1.8% |
Packages | 96 |
96 |
Duplicate Packages | 2 |
2 |
Bundle size by type 1 change
1 regression
Current #2388 |
Baseline #2386 |
|
---|---|---|
JS | 9.06MiB (+0.05% ) |
9.05MiB |
CSS | 864.03KiB |
864.03KiB |
Fonts | 526.1KiB |
526.1KiB |
Media | 295.6KiB |
295.6KiB |
IMG | 140.74KiB |
140.74KiB |
HTML | 1.38KiB |
1.38KiB |
Other | 871B |
871B |
Bundle analysis report Branch florian-h05:thing-actions Project dashboard
Generated by RelativeCI Documentation Report issue
I assume you display the action output label. |
Sounds good 👍 |
Do not forget to consider actions without any input. I am bluffled, you did that so fast! |
I will have to check again all existing thing actions with output(s) to check that output "name" is properly defined. |
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.
No notes.
If it's good for everybody, it's good for me.
Those are considered as well as actions without any output — in both cases a specific message is displayed instead of the inputs or output.
The only thing missing at the moment is to render the outputs based on the output definition provided by the API — currently the pretty output rendering is hard-coded, but the raw output can be seen. |
Signed-off-by: Florian Hotze <dev@florianhotze.com>
The output rendering now renders all keys of the response object and takes the labels from the output definition. @digitaldan For the matter binding, you can either define: @ActionOutputs({ @ActionOutput(name = "qrPairingCode", label = "QR Pairing Code", type = "qrCode"), @ActionOutput(name = "manualPairingCode", label = "Manual Pairing Code", type = "java.lang.String") }) or @ActionOutputs({ @ActionOutput(name = "qrCode", label = "QR Pairing Code", type = "java.lang.String"), @ActionOutput(name = "manualPairingCode", label = "Manual Pairing Code", type = "java.lang.String") }) |
Signed-off-by: Florian Hotze <dev@florianhotze.com>
5239dbc
to
cd8abd9
Compare
FYI it seems the ActionsOutput annotation is used wrong in many bindings given the core implementation of annotation parsing, I have created a doc PR to fix that: openhab/openhab-docs#2388 |
We will have to check and fix all existing thing actions returning something. Do we really need the @ActionsOutputs annotation when there is only one @ActionsOutput annotation? |
I find 0 trace of ActionOutputs in our code base. |
If I understand the current docs, the annotation is only to be added if you return a Map<String,Object>. My correction to the docs is, that if you return a Map<…>, you need to annotate with ActionInputs for core to recognise the annotation — see the linked core code in my doc PR. |
@lolodomo But let us continue this discussion in openhab/openhab-docs#2388 as this is not related to the UI. |
@digitaldan I think I will merge this PR so you can use it in the latest snapshot, from my testing it works fine. Please refer to #2818 (comment) for the required annotation for the Thing action. |
@florian-h05, @lolodomo - I just updated to latest snapshot to have a look at Energi Data Service actions: Can you help me assess:
The first issue is the method overloads, i.e. multiple actions with the same name. This cannot be seen in the list in the UI, so I think this needs to be improved in the UI, since there are no rules (at least to my knowledge) against method overloads for actions? Next, I have this action: This results in:
I don't think there have been any rules against returning any particular types, like And of course, I now have to add the |
This cannot be improved in the UI as the UI only gets what core provides. For the Fronius Thing actions, I have overloads in place to allow the use of ZonedDateTime in addition to LocalTime for convenience of scripting, but I have only annotated the Thing actions with LocalTime so only those show up in the UI. The UI will only support pretty input for LocalTime, for ZonedDateTime there is text input only. In your case, I would consider only annotating the overload that supports the most parameters. Set
FYI, the docs state:
But I strongly agree that it wouldn't make sense to only allow this return value as scripts can utilise other return values as well. The problem with outputs that are not |
I tested quickly yesterday evening and I encountered two difficulties. First problem: decimals are limited to 2 decimals. If I try to fill 123,456, it is rejected. 123,45 is accepted. Is this restriction hardcoded in UI? Second oroblem: it looks like the execution of action is failing for java primitive types if I don't fill a value while there is a default value displayed. Can you tell me what you do in that case ? Maybe I should set required to false in core to avoid this issue? |
Type of yes and no - it default to that, but can be modified by setting
The default value needs to be applied inside core. The UI has never applied the default, it is displaying it only. |
Can you then at least block and do not call the action if one required parameters is not yet filled ? |
What value do you suggest ? |
If a parameter is marked as required in the annotation, this is already done in the UI. If a required parameter is not filled, the UI displays an error and does not execute the action.
That’s difficult to say that generic. Stepsize is for example |
@lolodomo Wrt to decimal parameters: Refs #2832. |
Looks like it does not work like that, I mean the action is always called even if required parameters are not filled and it leads to an exception. |
Looks like a good idea. |
I have tested that during development IIRC. If the config description says it’s required, the UI will display a warning if it’s missing.
This is already possible, we just have to set it to that in the ActionInputHelper and document it. I can do that. |
Ok, I let you do it. Do not forget to adjust unit tests. |
Unfortunately, it does not work. Try with this action:
Click on the action to open the popup and just click on the Execute Action link. There is no warning, the action is run and it leads to this exception:
IMHO, the UI should detect that some required parameters are not filled and should show a warning and certainly not call the action. Now if I enter 1 in the input field and click the Execute Action link, then it works. The result is the same with this action, that is not a surprise but I wanted to test it also:
|
An improvement would be to sort the actions by their label when displaying them. Maybe it should be done by the REST API ? With a new parameter ? And maybe you could also display the action description ? Maybe below the action label in smaller font ? I let you think to something consistent with what we have in Main UI elsewhere. |
I also tried with this action with an unsupported input type to see how you handle that:
The action is listed but nothing happens when you click on it. I like that you display it. Maybe the text color could be different to distinguish supported and unsupported actions ? Or something else to distinguish like a "supported" badge for example in front of the action ? |
…tions Follow-up for openhab#2818. Signed-off-by: Florian Hotze <dev@florianhotze.com>
I don't think that is the UI's fault. The config description parameter tells the UI there is a default value of 0. It has never been the case with the config sheet that the UI applies that default to what it sends to the server, instead it is the job of the server to apply the default he announces himself. This is not the case here, this has to be fixed inside openHAB Core.
Sure 👍
It should not be displayed IMO as the UI isn't able to use it - I will fix that and hide it instead. The user will notice there are unsupported actions when checking the log. #2834 implements these improvements. openhab/openhab-core#4424 implements the step size 0 param and also applies the default values for primitives as announced by the ActionInputsHelper when creating config descriptions. |
@florian-h05 : what will happen for an input parameter being marked as required and with no default (not a java primitive type) ? Will the action be called anyway, meaning the "required" property is in practice ignored by UI ? I am going to test again all your last changes but I am sure we are tending to something very good. |
Excellent. Will test that. |
Refs openhab/openhab-core#4392.
Closes #2817.
This adds a new section "Actions" to the Thing tab of the Thing page, which provides a button for each UI-supported Thing action.
Clicking on that button will open a popup, where action input can be configured and action output can be viewed.
All keys of the action output response object from REST are rendered as list Items, the labels are taken from the action output definitions and fallback to the key.
If the key is
qrCode
or its output type is defined asqrCode
, its value is rendered as QR code.For actions without inputs or without outputs, messages are shown indicating that there is no such.
This PR marks the cold related to the old config actions as deprecated, my wish is to get rid of those config actions and have Thing actions used instead.