Thing "actions" config param handling made generic #1498
Conversation
Job #547: Bundle Size — 11.33MB (~+0.01%).Metrics (2 changes)
Total size by type (1 change)
|
mbronk
force-pushed
the
private/mbronk/FR_1491
branch
from
1d1a90d to
c32ee3b
Compare
bundles/org.openhab.ui/web/src/pages/settings/things/thing-details.vue
Outdated
Show resolved
Hide resolved
| this.zwaveActions = this.configDescriptions.parameters.filter((p) => p.groupName === 'actions') | ||
| this.configDescriptions.parameters = this.configDescriptions.parameters.filter((p) => p.groupName !== 'actions') | ||
| // special treatment for actions (heuristic match by group name/label) | ||
| if (this.configDescriptions.parameterGroups.filter((pg) => pg.name === 'actions').every((pg) => pg.label === 'Actions' && pg.description === 'Actions')) { |
There was a problem hiding this comment.
This heuristic is good enough but since parameter groups have a "context" attribute:
I'm wondering whether this could perhaps be the way to go - look for e.g. an "actions" context in the parameter group definition instead of checking for labels & descriptions - it certainly would be more elegant IMO.
We could still make an exception for the Z-Wave things unless @cdjackson agrees to add the "actions" context for the actions parameter group for them.
I'm not sure how easy that is to add such contexts to parameter groups, whether in declarative or imperative form.
There was a problem hiding this comment.
Indeed, there seems to be a "param group context" defined, though not documented too widely. Per this it would seem it may be a freeform string (the per-param context seems to be a pre-defined list).
Let me try to see how easy it is to set/use and report back (may take me about a week to do so, though)...
I 100% agree it would be more elegant than this heuristic.
That said, I think the ROI is there when ZWave binding also adopts it, otherwise we're back into heuristic-land :). Not sure what'd be the timeline, but how about a 2-step approach? I.e. this heuristic for now (possibly with an "or" for a by-context match) and follow through with a proper update if/once ZWave binding changes?
There was a problem hiding this comment.
I.e. this heuristic for now (possibly with an "or" for a by-context match) and follow through with a proper update if/once ZWave binding changes?
Works for me!
There was a problem hiding this comment.
The "by-context" method does work indeed, so I added it as primary.
Any param group constructed with .withContext("actions") will now be picked as action container.
As per our discussion, I've left the heuristic match by name and label for compat with today's Z-wave binding.
Note I had to pull the "by description" criterion, since Z-Wave doesn't use it uniformly and only sets it on non-controller devices...
--
While at it, I attempted to make it a bit more generic (hopefully the code explains the intent well enough). Do ask if it raises any concerns.
| let thing = this.thing | ||
| let save = this.save | ||
| if (action.type !== 'BOOLEAN') return | ||
| this.$f7.dialog.confirm( | ||
| `${action.label}?`, | ||
| `${action.description ?? action.label}?${action.verify ? '<p><small><strong class="text-color-yellow">WARNING:</strong> This action may be dangerous!</small></p>' : ''}`, |
There was a problem hiding this comment.
| `${action.description ?? action.label}?${action.verify ? '<p><small><strong class="text-color-yellow">WARNING:</strong> This action may be dangerous!</small></p>' : ''}`, | |
| `${action.description ?? action.label} ? ${action.verify ? '<p><small><strong class="text-color-yellow">WARNING:</strong> This action may be dangerous!</small></p>' : ''}`, |
I'm not sure I understand this...
There was a problem hiding this comment.
This is a (rather naive indeed) attempt to bring back some of the semantics that verify has.
Ref:
verify | Specifies that this is parameter requires a verification stage with the user before sending.
| Parameters flagged with *verify=true* could be considered dangerous and should be protected
| from accidental use by a UI - e.g. by adding an "Are you sure" prompt (optional).
The "actions" behaviour of Main UI does ask for a confirmation for all the actions, of course... so I figured to bring back at least some of the "verify" semantics, and add a notion of "hey, the binding developer considered this dangerous".
I am the first to agree this may be way too naive for a prompt, but would say... we should retain some of this semantics, otherwise all actions get "level" which is not the case (prime example is zwave: enabling inclusion mode (safe) and hard controller reset (destructive) ... yield the same kind of confirmation)
There was a problem hiding this comment.
Oh. and the abovementioned code was not a ternary IF statement! It is just a good old questionmark (to be rendered as-is in the output :) ).
There was a problem hiding this comment.
The "actions" behaviour of Main UI does ask for a confirmation for all the actions, of course... so I figured to bring back at least some of the "verify" semantics, and add a notion of "hey, the binding developer considered this dangerous".
Wow I completely forgot this "verify" attribute existed... 🤯
Fully agree with your assessment - this particular line of code I considered quite "convoluted" though with the muliple ternary expressions. If I understand it right it does display the description or label, or when there's none, displays a warning message if the verify flag is true? Is that your intent?
There was a problem hiding this comment.
or when there's none....
As per my 2nd comment... the ? here is not a ternary expresssion, actually, rather the same questionmark that existed in the original. The intent is to:
- Display description (and fallback to label if none)
- (orthogonal to the above): If
verify==true--> add an extra warning.
I now see how poorly it reads code-wise (it was much better in IDE, with coloring, etc.). Unfortunately, I have literally never written anything in vue, so while I assume using some templated text/conditional controls would make it more readable, it would take me some extra learning to find the syntax out. Do you happen to have any tips in particular?
There was a problem hiding this comment.
Extracted to separate variables - hope it'll be more readable now.
mbronk
force-pushed
the
private/mbronk/FR_1491
branch
3 times, most recently
from
d75fd7c to
97ecf63
Compare
|
@mbronk Please review the ESLint errors: https://github.com/openhab/openhab-webui/actions/runs/3165087894/jobs/5209020387 If you can, use an editor that can highlight ESLint errors, for instance VS Code with the ESLint plugin - it's easier! |
mbronk
force-pushed
the
private/mbronk/FR_1491
branch
from
97ecf63 to
041add1
Compare
|
@ghys Yeah, sorry about that! Anyways, eslint should be happy now. Could you please approve another CI build? EDIT: Argh.. missclicked and updated this branch w/ merge vs. rebase. Let me fix that in just a sec. [DONE] |
For Main UI's Thing-Details view.
Matching is by group param context ("actions") first, then falls back to
"by name" for Z-Wave binding backwards-compatibility.
Fixes openhab#1491
Signed-off-by: Mateusz Bronk <bronk.m+gh@gmail.com>
mbronk
force-pushed
the
private/mbronk/FR_1491
branch
from
4c7d17a to
354ac11
Compare
Signed-off-by: Yannick Schaus <github@schaus.net>
There was a problem hiding this comment.
Thanks - LGTM!
I just removed the tooltip with the description on the action buttons because it didn't look too good on mobile IMO, especially with potentially long descriptions. You have that description on the confirmation dialog anyway so it's probably enough...?
Nice new feature!
(build failures were temporary, the build actually succeeded: checks/webpack-stats are green)
For Main UI's Thing-Details view.
Fixes #1491
Signed-off-by: Mateusz Bronk bronk.m+gh@gmail.com