eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
862 stars 787 forks source link

[core] Added min, max and step parameters for slider widget #6777

Closed FlorianSW closed 5 years ago

FlorianSW commented 5 years ago

Like for the setpoint, the min and max value for a slider can differ based on the underlying thing that is being controlled, so it shouldn't be hardcoded and a user should be able to change these values.

The default values, if the parameters are not given, are 0 (minValue) and 100 (maxValue), like they're currently, so that users shouldn't see a difference when they're not changing their configurations.

Fixes #6776

Signed-off-by: Florian florian.schmidt.welzow@t-online.de

lolodomo commented 5 years ago

Could you please adjust Classic UI too ?

kaikreuzer commented 5 years ago

The slider widget was originally only introduced for Dimmer items and no others. What does it do for number items right now? If we want to extend its use to number items, shouldn't we also introduce a "step" parameter as for the Setpoints?

FlorianSW commented 5 years ago

Could you please adjust Classic UI too ?

I looked at it, too, however, I've two questions here:

  1. The classic UI renders sliders like setpoints, to there're only two buttons, I'm not sure where to put the min and max values there (no-where?)
  2. Where is the non-minified source code of the "Logic.js" where I would expect to put some min/max-value handling?

What does it do for number items right now?

E.g. the temperature (like in the openHAB example) or, in the actual use case, controlling the brightness of an item, which expects the brightness as a range from 0 to 255 instead of 0 to 100. Currently it would need to be "translated" in a rule to work with a slider.

If we want to extend its use to number items, shouldn't we also introduce a "step" parameter as for the Setpoints?

Hmm, I don't think so. The step parameter in the setpoint means, that when pressing the minus or plus to increase or decrease the current value by the value of step. Whereas I would expect a slider to change the value based on the relative position on the slider. Is it understandable what I mean here? :D

maggu2810 commented 5 years ago

For a slider you need to know the minimum step size, too. Or how do you want decide which values the slider position should be assigned to?

FlorianSW commented 5 years ago

Why? The assignment of the values doesn't change when you make the min and Max values configurable (the slider had min and Max values before, too, they were just hardcoded). The slider had a specific width and based on that width the range between the min andax values are assigned to the slider relatively. Let's say, that the slider width is 100%, then each step of the slider position is 1%, it doesn't really matter what the absolute values are then, they are calculated by the relative position of the slider.

maggu2810 commented 5 years ago

I did not have a look at the code so I could be wrong and if you already checked it... I had in mind that a slider has a fixed minimum of 0 a maximum of 100 and a step size of 1. So, you cannot set e.g. 3,75 using a slider (but I really don't know if this is correct). My assumption has been, if we could change the minimum and maximum (e.g. min=0 and max=1) we need also to set if the step size should be 1, 0.1, 0.01, 0.001 or so on. It would not make sense to allow a more granular step size if it is not handled on the item's linked channel.

FlorianSW commented 5 years ago

So, for now the slider has a default step size (in basic UI only), which is 1 (see the mdl documentation), so it would be possible to add a step parameter, too. I really do not care so much about this part of it, so I'm ok with adding a step parameter, too (which should default to 1 I think).

FlorianSW commented 5 years ago

The latest commit changes the slider to have another new parameter (step) and made changes to the basic UI to take this into account.

I didn't changed the classic UI so far because I want to check with you guys first about one thing: If we make this change to the classic ui (min, max and step), then the widget in the classic UI renders really like the setpoint widget. The only difference now is, that the slider currently uses the "magic" INCREASE and DECREASE command to set the new value, the setpoint pre-calculates the new value, as it needs to take the step parameter into account.

My solution here would be to use the same widget for a slider and a setpoint in the classic UI, is that ok? Any other opinions? :)

maggu2810 commented 5 years ago

I am not a user of the basic or classic UI, so hopefully another one can comment.

kaikreuzer commented 5 years ago

@FlorianSW I do not think that anything should be changed in the Classic UI, because the widget just works fine and min/max do not have any impact on it. Completely changing its behavior to not send INCREASE/DECREASE anymore would rather be a breaking change that might annoy some users.

kaikreuzer commented 5 years ago

May I ask you to also do an update to https://www.openhab.org/docs/configuration/sitemaps.html#element-type-slider?

FlorianSW commented 5 years ago

Of course, I'll submit a PR :) May I ask if there's already a plan when (not exactly of course, just an estimation) this will be included in an ESH release, as well as an openHAB release?

kaikreuzer commented 5 years ago

Will be in the latest openHAB distro snapshots later this week, so the docs can be adapted right away!

maniac103 commented 5 years ago

Just jumping in here to thank you for not forgetting about step ;-)

Why? The assignment of the values doesn't change when you make the min and Max values configurable (the slider had min and Max values before, too, they were just hardcoded). The slider had a specific width and based on that width the range between the min andax values are assigned to the slider relatively. Let's say, that the slider width is 100%, then each step of the slider position is 1%, it doesn't really matter what the absolute values are then, they are calculated by the relative position of the slider.

That's all right, but still, step is absolutely needed in order to have a defined granularity. Say the 0..100% slider is 100 pixels wide: the minimum increment then would be 1%. If the slider grows to 500 pixels (presentation is up to the UI after all), the minimum increment suddenly becomes 0.2%. Since the UI doesn't know whether the underlying number should be integer or floating point, what increment between data points should it use? Having 'step' solves that uncertaincy, so thanks for not making my life hard when implementing this in the Android app :+1: