openhab / openhab-webui

Web UIs of openHAB
Eclipse Public License 2.0
224 stars 239 forks source link

edit metadata not transfering entries properly into the Code #880

Closed muelli1967 closed 3 years ago

muelli1967 commented 3 years ago

When you edit the metadata in the UI for visibility options, the code is not properly transferred into the code so the entry is not valid.

image

image

Expected behavior

i believe it should be translated into

image

Steps to reproduce

Your environment

Browser console

Browser network traffic

Additional information

hubsif commented 3 years ago

Hi!

@muelli1967: The current workaround would be to enter =false instead of just false, then it works.

As for the bug itself, I think there are two ways to address this:

hubsif commented 3 years ago

Looking at the code I found two (three) ways to address this:

  1. prefix a missing '=' when the item widget is displayed:

    diff --git a/bundles/org.openhab.ui/web/src/components/widgets/widget-mixin.js b/bundles/org.openhab.ui/web/src/components/widgets/widget-mixin.js
    index 3278cd0b..546be093 100644
    --- a/bundles/org.openhab.ui/web/src/components/widgets/widget-mixin.js
    +++ b/bundles/org.openhab.ui/web/src/components/widgets/widget-mixin.js
    @@ -54,7 +54,9 @@ export default {
     },
     visible () {
       if (this.context.editmode || !this.context.component.config) return true
    -      const visible = this.evaluateExpression('visible', this.context.component.config.visible)
    +      var visible = this.context.component.config.visible
    +      if (typeof visible === 'string' && !visible.startsWith('=')) visible = '=' + visible
    +      visible = this.evaluateExpression('visible', visible)
       const visibleTo = this.context.component.config.visibleTo
       if (visible === undefined && visibleTo === undefined) return true
       if (visible === false) return false
  2. prefix a missing '=' on input. this "live"-adds a '=' if not entered. (disadvantage IMO: for manual edit in yaml still requires users to start with a '=')

    
    diff --git a/bundles/org.openhab.ui/web/src/components/item/metadata/item-metadata-widget.vue b/bundles/org.openhab.ui/web/src/components/item/metadata/item-metadata-widget.vue
    index d0480a31..71c52c83 100644
    --- a/bundles/org.openhab.ui/web/src/components/item/metadata/item-metadata-widget.vue
    +++ b/bundles/org.openhab.ui/web/src/components/item/metadata/item-metadata-widget.vue
    @@ -231,6 +231,9 @@ export default {
         if (!this.metadata.config[key] && this.defaultComponent.config[key]) this.$set(this.metadata.config, key, '')
         else if (this.metadata.config[key] === undefined || this.metadata.config[key] === null) delete this.metadata.config[key]
  1. like mentioned above, simply change the hint from "false" to "=false" (and do that for all translations)
muelli1967 commented 3 years ago

thanks for looking into this. whatever you find suitable...

eikowagenknecht commented 3 years ago

I also stumbled upon this today. I think solution 1 or 3 are okay, 1 being more user friendly. Are there any cases where prefixing "=" would break an otherwise working entry here? If not I'd go with that.

Solution 2 is a bit off for me because it requires different input in UI and YAML and since users might use both it's hard to remember what to enter where.

hubsif commented 3 years ago

@ghys What do you think? Once decided, I can create a PR for the solution chosen.

ghys commented 3 years ago

Maybe just add checks for the strings "false" or "true" and treat them as boolean instead of strings?

hubsif commented 3 years ago

Maybe just add checks for the strings "false" or "true" and treat them as boolean instead of strings?

Also possible. Hopefully it's not misleading to not require a '=' for true and false but for everything else to be evaluated. But then, that's the case throughout the UI. Do you think it should be added as general evaluation rule in widget-mixin or for visible only?

ghys commented 3 years ago

I could be wrong, but for Vue component props it might already work. So I'd start with the "visible" computed property as a first step and then see whether or not it's needed elsewhere. It's also somewhat "special" since although it's a BOOLEAN property, the config ui is a textbox by default on that screen instead of a toggle switch (since expresssions are the primary use case).