-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Sitemaps: support Button widgets inside buttongrids #272
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jimmy Tanagra <[email protected]>
9a61409
to
8770042
Compare
Signed-off-by: Jimmy Tanagra <[email protected]>
8770042
to
c4fc240
Compare
lib/openhab/dsl/sitemaps/builder.rb
Outdated
# | ||
# @since openHAB 4.2 | ||
# | ||
def button(button = nil, **kwargs, &block) |
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.
Seems like it would be nice if you could use the 4.1 syntax in 4.2 and vice versa to make an easy transition (as long as you don't specify an item or release when on 4.1)z
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.
4.2 supports both widget and non-widget implementation, so both syntaxes are supported.
Using 4.2 syntax, without item or release, on 4.1 is doable. I'll make the changes and push.
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.
After pondering about it further, I don't think it's a good idea.
If we supported kwargs in 4.1 (without item), e.g.:
button row: 4, column: 1, click: ON, label: "ON", icon: "switch-on"
when user upgrades to 4.2, this will be created as non-widget button (the same as it was in 4.1), however, this means we can't warn / raise an error when item
is missing when the user intended to create a widget button.
The check for missing item
isn't currently implemented, and perhaps it should be. However, we could also explore another direction: #button
with kwargs with omitted item
arg could default to the item specified in the parent buttongrid, hence it is again implemented as widget. This inferencing may be implemented in core, or if not, in Ruby. I'm waiting to see how it develops in core, and I have mentioned it here
So, long story short, perhaps we should leave it as is for now. It makes a clear distinction: array args => non widget, vs kwargs => widget.
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.
except item isn't required (openhab/openhab-core#4241) on a button. by supporting both styles of params, you could change ButtongridBuilder#initialize
to not have buttons itself, just call buttons.each { |b| button *b }
, and have ButtonBuilder#initialize
support array style as well. Then you could add a private children(parent)
method to LinkableWidgetBuilder
that defaults to parent.children
, but on ButtongridBuilder
on 4.1, it returns parent.buttons
. It really unifies things so you don't have two completely different codepaths that are not really disjoint, making it hard to reason about.
I don't think users of openHAB really care if their button is a true Button widget, or just part of the Buttongrid, since the former is a superset of the latter. I'd be more worried if say the iOS app only supported 4.1 style buttongrids, and choked on actual widgets, with no forthcoming fix. But it doesn't support button grids at all! BasicUI on the web supports both fully. And I expect the Android app, if it doesn't already, to soon support both fully.
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.
So you're saying that in 4.2, implement both array style and kwargs style as widgets, ignoring the non-widget version completely? Currently the Android app doesn't yet support button widgets though, so doing this will break android on 4.2.
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've refactored it to your suggested/requested change. However, it doesn't work on android app with oh 4.2 because the app hasn't implemented button widgets.
c6abfc9
to
d1f2440
Compare
This also makes buttons always as widgets in OH4.2+ Signed-off-by: Jimmy Tanagra <[email protected]>
d1f2440
to
55a0973
Compare
Buttongrid can now contain
Button
widgets which allows each button to be linked to individual item, plus each being set to different color / visibilityopenhab/openhab-core#4223
Currently, the "old"
button
option for buttongrid is still supported too, so it's backwards compatible with oh4.1Adding buttons to a buttongrid with positional arguments
Adding buttons to a buttongrid with keyword arguments
Mixing positional and keyword arguments
openHAB 4.2+ supports assigning different items to buttons, along with additional features