Added slider control #81
Conversation
| @@ -109,6 +109,12 @@ fun Pane.progressbar(property: Property<Double>, op: (ProgressBar.() -> Unit)? = | |||
| progressProperty().bind(property) | |||
| } | |||
|
|
|||
| fun Pane.slider(min: Double? = null, max: Double? = null, value: Double? = null, op: (Slider.() -> Unit)? = null) = opcr(this, Slider().apply { | |||
There was a problem hiding this comment.
Or should I split it into two constructor (like JavaFX) with no support for null parameters? It would slightly optimize the calls to adjustValues()
There was a problem hiding this comment.
I don't really see a problem with how you did it here, but if you wanted to invoke the other constructor I don't see the harm.
You'll have to be the judge if it makes it too complicated or not. I suppose you could also just call the constructor with those 3 parameters always, and use 0 as a default values instead of null (I don't know if Kotlin will handle the null -> primitive conversion or throw an error).
Either way, let me know when you're happy with it and I'll merge.
There was a problem hiding this comment.
I thought about that, but Slider has a lot of magic numbers in it for defaults, so if they ever changed we'd have to keep ours up to date. I think the way I did it is probably the best way to do it as it will only create the properties it needs, and they have way more overhead than the adjustValues() calls.
I think I like it how it is now (unless you think I should remove the orientation param).
There was a problem hiding this comment.
In that case, I agree. regarding the orientation parameter I'm not sure I feel one way or the other. That can always be done in the op block but we try to streamline common usages which I guess this could be. What do you think @edvin?
There was a problem hiding this comment.
I would not add the orientation, use the default orientation or adapt in the op block (my 0.2 cents)
|
Sorry, I just realized my last commit voided the comment thread. In short, I think the way it is has the smallest amount of overhead (the extra calls to Unless you think the orientation param should be removed, I'm happy with it. |
|
Haha no problem, this is what I said on that voided thread just in case:
|
|
Since the parameters are only applied when they are non-null I think this is a good approach. I don't see the problem with the orientation parameter, it is apart from min/max/value the most important property IMHO. You can use it any way you like now, so as far as I understand all these are valid ways to use it: slider(0.0, 100.0, 50.0)
slider(0.0, 100.0, orientation = HORIZONTAL)
slider(max = 100.0) {
orientation = HORIZONTAL
} |
Plus some of IntelliJ's auto-formatting :)