openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
897 stars 415 forks source link

[sitemap] Add labelcolor and visibility to a single button of a Buttongrid #4173

Closed Hajuskin closed 7 hours ago

Hajuskin commented 4 months ago

Which UI are you reporting an issue for?

The problem

I use the new Buttongrid element to build a page in Basic UI as an overview for most of my switchable items. This is a great way to have a dense layout. I use a rule to actually switch the items represented by the buttons, but the state of a switch is of course not shown.

Your suggestion

Having the ability to use the labelcolor and visibility syntax for every single button of a Buttongrid could be very useful for having a compact overview of e.g. open doors/windows, triggered motion sensors or in my case switchable items with their states represented by the buttons label color.

So the syntax could be something like: buttons=[row_1:column_1:command_1="description_1":labelcolor=[switch_item1==ON="green", switch_item1==OFF="gray"], ... or buttons=[row_1:column_1:command_1="description_1":visibility=[tamper_item1==ON], ...

Your environment

Additional information

lolodomo commented 3 months ago

@kaikreuzer : please move this RFC in openhab-core repo and please add the "sitemap" tag.

lolodomo commented 2 months ago

labelcolor, iconcolor, visibility parameters can only be applied to a sitemap element. It is handled at sitemap element level in core framework. To achieve your request, we should create a new sitemap element Button as a sub element of a Buttongrid. The current buttons parameter of Buttongrid will then be replaced by a list of Button inside Buttongrid. Then each Button element, like any other sitemap element could have parameters labelcolor, iconcolor, visibility. The syntax would be something like that:

Buttongrid [label="xxx"] {
    Button line=X column=Y item=xxx command=xxx label="xxx" [icon=xxx] [labelcolor=[xxx]] [iconcolor=[xxx]] [visibility=[xxx]]
    Button line=X column=Y item=xxx command=xxx label="xxx" [icon=xxx] [labelcolor=[xxx]] [iconcolor=[xxx]] [visibility=[xxx]]
    Button line=X column=Y item=xxx command=xxx label="xxx" [icon=xxx] [labelcolor=[xxx]] [iconcolor=[xxx]] [visibility=[xxx]]
}

It even makes the syntax easier for the user. It has also the advantage that each button can be linked to a different item. Today, I believe we should have gone in that direction at the beginning.

We could keep backward compatibility by making the buttons and item parameters optional on the Buttongrid element. If no Button are declared as child of Buttongrid but buttons and item parameters are set, we can consider the current behaviour.

It is relatively easy to enhance in the core framework (sitemap syntax), I can do it. The biggest changes have to be implemented in each sitemap UI. I could do it for Basic UI. @openhab/android-maintainers : are you interested to implement this enhancement in the Android app ?

@openhab/ios-maintainers for information (if I am not wrong, Buttongrid is not yet implemented in the iOS app).

mueller-ma commented 2 months ago

The suggested syntax is much more readable than the current one đź‘Ť

Adapting the Android app should be doable.

lolodomo commented 2 months ago

@mueller-ma : it is probably too late for 4.2 regarding all the changes to do ? I am not sure if we break the sitemap REST API or if we keep the current API response structure when buttons parameter is used and have a new one when buttons are sub-elements of the Buttongrid ? I imagine it will be better to have a (new) unique structure of response not depending on whether the buttons were declared in the buttons parameter or as sub-elements. But then this will become a breaking change for the Android app. WDYT ?

mueller-ma commented 2 months ago

Yes, maybe it's the best to update the API when using the new syntax.

lolodomo commented 2 months ago

This will lead to something even more powerful than today, with buttons in the grid that can be linked to different items, with buttons that can be stateless or stateful, and with conditional icon / color / visibility.

lolodomo commented 2 months ago

In the core PR I just proposed, I increased the version of REST API so that you can know if new structure is supported by the server. Sitemap REST API response still contains a list of MappingDTO in field mappings if the old buttons parameter is used. It includes a list of WidgetDTO in field widgets in case the grid contains Button elements.

It makes the change backward compatible with existing UIs.

lolodomo commented 2 months ago

It is almost implemented in Basic UI. Dynamic colors for label and icon is working. Stateful / stateless is done. Press & release is done. Dynamic icon should work but still need to be tested. I even accept a combination of old and new ways to provide buttons for the same Buttongrid. Remains the visibility to implement.

Regarding core framework, I need to change something in the REST/SSE API regarding returned label and icon for the Button element. They should not be inherited from the item like the other sitemap elements. If no label is provided, the label of the button should be its "command". If no icon is provided, we should consider a button with text and no icon.

lolodomo commented 2 months ago

The proposed syntax for the Button is :

Button item=<item> [label=<label>] [icon=<icon>] row=<row> column=<column> [stateless] click=<cmd> [release=<cmd>] [labelcolor=[...]] [iconcolor=[...]] [visibility=[...]]
Button item=<item> [label=<label>] [staticIcon=<icon>] row=<row> column=<column> [stateless] click=<cmd> [release=<cmd>] [labelcolor=[...]] [iconcolor=[...]] [visibility=[...]]
Button item=<item> [label=<label>] [icon=[...]] row=<row> column=<column> [stateless] click=<cmd> [release=<cmd>] [labelcolor=[...]] [iconcolor=[...]] [visibility=[...]]

Explanations of the parameters:

Such Button element is valid only inside a Buttongrid element.

lolodomo commented 2 months ago

Fully implemented in Basic UI now.

The only current limit is that Basic UI only supports one button at a certain position in the grid. So you can now use the visibility parameter to show or hide a button in the grid but you can't define several buttons at the same position with different visibility conditions in order to switch between buttons at a certain position in the grid. I could try to enhance that later but with the constraint that there will be always only one visible button at a position in the grid.

lolodomo commented 1 month ago

Adapting the Android app should be doable.

@mueller-ma : it is now merged, so you can work on it for Android when you like. I will update sitemap documentation.

mherwege commented 1 month ago

@lolodomo Do I get this right that the UI sitemap builder needs to be changed as well to support this additional functonality?

lolodomo commented 1 month ago

Yes @mherwege , correct.

mherwege commented 1 month ago

And the old version of the API still works as well? I don’t think that is appropriate for UI configuration, leaving that choice. I need to think about how to cope with this. It lay be to just always provide the new format from UI configuration.

lolodomo commented 1 month ago

And the old version of the API still works as well?

Yes, both syntaxes are accepted. You can even combine both.

lolodomo commented 1 month ago

The only current limit is that Basic UI only supports one button at a certain position in the grid. So you can now use the visibility parameter to show or hide a button in the grid but you can't define several buttons at the same position with different visibility conditions in order to switch between buttons at a certain position in the grid. I could try to enhance that later but with the constraint that there will be always only one visible button at a position in the grid.

Finally implemented: openhab/openhab-webui#2580

lolodomo commented 1 month ago

I discovered an issue in the sitemap validation code I added, when the Buttongrid + Button are in a sub-page. It leads to this error when the sitemap is loaded and validated:

Linkable widget should not contain Button, Button is allowed only in Buttongrid

and the Buttongrid is then not rendered in Basic UI. Of course the error is wrong as all my Button elements are in a Buttongrid element. I will investigate.

The old style Buttongrid is still correctly supported in a sub-page.

jimtng commented 1 month ago

@lolodomo a few questions:

  1. Is the old style buttongrid without sub-widgets going to continue to be supported, or will it be deprecated?
  2. Is there a plan to make the Button widget to "default" to the item specified in its parentButtongrid when one is not specified? In a way this would reduce the flexibility but at the same time also saves typing / duplication when we still want to control the same item but with the added flexibility of using the Button widgets (e.g. custom coloring)
jimtng commented 1 month ago

I discovered an issue in the sitemap validation code I added, when the Buttongrid + Button are in a sub-page. It leads to this error when the sitemap is loaded and validated:

Linkable widget should not contain Button, Button is allowed only in Buttongrid

and the Buttongrid is then not rendered in Basic UI.

I noticed this when the Buttongrid isn't assigned an item like this:

sitemap test {
  Frame {
    Buttongrid {
      Button item=TestSwitch1 row=1 column=1 click="ON" label="ON" visibility=[TestSwitch1!=ON]
      Button item=TestSwitch1 row=1 column=1 click="OFF" label="OFF" visibility=[TestSwitch1==ON]
    }
  }
}

But when I assigned an item to Buttongrid, it worked fine. In both cases, they're nested inside a Frame.

sitemap test {
  Frame {
    Buttongrid item=TestSwitch1 {
      Button item=TestSwitch1 row=1 column=1 click="ON" label="ON" visibility=[TestSwitch1!=ON]
      Button item=TestSwitch1 row=1 column=1 click="OFF" label="OFF" visibility=[TestSwitch1==ON]
    }
  }
}
lolodomo commented 1 month ago

Yes, I noticed the same. Strange that it depends on presence of item parameter...

lolodomo commented 1 month ago

In fact, the problem is when there is no parameter set for the Buttongrid element. Adding any parameter (item or label or labelcolor or iconcolor ...) solves the issue. It does not depend if the Buttongrid element is on the main page or a sub-page. I did not notice the problem because my Buttongrid elements have at least one parameter when testing (even if some have no item parameter).

lolodomo commented 1 month ago

@lolodomo a few questions:

1. Is the old style buttongrid without sub-widgets going to continue to be supported, or will it be deprecated?

To be decided but Basic UI currently supports the old style, the new style and even a combination of both.

2. Is there a plan to make the `Button` widget to "default" to the item specified in its parent`Buttongrid` when one is not specified? In a way this would reduce the flexibility but at the same time also saves typing / duplication when we still want to control the same item but with the added flexibility of using the `Button` widgets (e.g. custom coloring)

I hesitated when implementing. I don't remember exactly but I probably give up because it could lead to have no item neither on Button nor on Buttongrid. But thinking of it, in this case, the UI could simply log a message and do nothing when clicking a button. I am ok to change that.

lolodomo commented 1 month ago

In fact, the problem is when there is no parameter set for the Buttongrid element. Adding any parameter (item or label or labelcolor or iconcolor ...) solves the issue. It does not depend if the Buttongrid element is on the main page or a sub-page. I did not notice the problem because my Buttongrid elements have at least one parameter when testing (even if some have no item parameter).

Bug is fixed by #4240

lolodomo commented 1 month ago
2. Is there a plan to make the `Button` widget to "default" to the item specified in its parent`Buttongrid` when one is not specified? In a way this would reduce the flexibility but at the same time also saves typing / duplication when we still want to control the same item but with the added flexibility of using the `Button` widgets (e.g. custom coloring)

I hesitated when implementing. I don't remember exactly but I probably give up because it could lead to have no item neither on Button nor on Buttongrid. But thinking of it, in this case, the UI could simply log a message and do nothing when clicking a button. I am ok to change that.

See #4241

jimtng commented 1 month ago
2. Is there a plan to make the `Button` widget to "default" to the item specified in its parent`Buttongrid` when one is not specified? In a way this would reduce the flexibility but at the same time also saves typing / duplication when we still want to control the same item but with the added flexibility of using the `Button` widgets (e.g. custom coloring)

I hesitated when implementing. I don't remember exactly but I probably give up because it could lead to have no item neither on Button nor on Buttongrid. But thinking of it, in this case, the UI could simply log a message and do nothing when clicking a button. I am ok to change that.

See #4241

Thanks! It sounds like you were planning to implement the inference logic on client side, which means the logic also needs to be implemented on android / ios too. Wouldn't it be easier to just make the Button widget to set its item parameter to its parent's at creation time when one is not provided? Is it because the code is generated by xtext?

jimtng commented 1 month ago

Wouldn't it be easier to just make the Button widget to set its item parameter to its parent's at creation time when one is not provided? Is it because the code is generated by xtext?

Although it's not quite "pure" in logic, we can sneak that code into here: https://github.com/openhab/openhab-core/blob/873bb53cbc12bf7624499f66170bfc90a10c35d6/bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/validation/SitemapValidator.xtend#L123

lolodomo commented 1 month ago

@jimtng : I give up the idea because I don't want to "patch" the model (I don't think we already did that in the past) and we absolutely need the item set at each Button level to have all the stuff working. Everything is based on attachment of a widget to an item. It is what triggers the update of a widget in particular.

lolodomo commented 2 weeks ago

Documentation is now completed. IMHO, this feature request can now be closed.

lolodomo commented 8 hours ago

@openhab/core-maintainers : is it possible to close this feature request as completed?