Binding extension functions are confusing

[aleksandar-stefanovic]

Let me preface this by saying that this is a discussion whether this issue exists, and if it confirmed, I can submit a pull request for it. I'm just creating this issue to discuss and confirm the issue.

Utility functions that help create bindings, such as:

• stringBinding
• objectBinding
• booleanBinding
• doubleBinding
• floatBinding

could be simplifed.

For every type, there is a pair of functions:

• The extension function, e.g. ObservableValue<T>.stringBinding(vararg dependencies: Observable, op: (T?) -> String?)
• The non-extension function, e.g. stringBinding(receiver: T, vararg dependencies: Observable, op: T.() -> String?)

What's confusing is the parameter of receiver on the non-extension function.

As seen from it's body:

Bindings.createStringBinding(Callable { receiver.op() }, *createObservableArray(receiver, *dependencies))

the receiver ends up in the same observable array as dependencies, and the only thing that differentiates it from the dependencies is that is used in the Callable. Seems reasonable to split them up, right? Well, this ends up really ugly:

val prop1 = SimpleDoubleProperty()
val prop2 = SimpleDoubleProperty()
val prop3 = SimpleDoubleProperty()

val product = doubleBinding(prop1, prop2, prop3) {
    val value1 = this.value
    val value2 = prop2.value
    val value3 = prop3.value
    value1 * value2 * value3
}

Why does receiver receive the "special" treatment? And by the "special treatment", I mean "no treatment at all", because it's just passing the whole observable into the lambda, and you have to extract the value by using this.value, same as using prop1.value. In my opinion, this split between receiver and dependencies is redundant, for these reasons:

1. It creates confusion when using the function. When seeing the sigunature stringBinding(receiver: ..., dependencies: ..., op: ...), you can be confused about what is the importance of the receiver, how is it different from the dependencies, and why does op use this receiver, but not the dependencies. Then you have to read the source code to find out the difference, just to realize that there is essentially no difference between them.
2. It is redundant when the extension function exists. In the case of extension function, you explicitly differentiate the receiver by making it a receiver of an extension function, and in that way, you're implicitly stating that the receiver is more important than other dependencies. Also, the Callable contains op(this.value) instead of receiver.op(), which means that the parameter of lambda is the actual value of the observable, and not the observable itself (that makes much more sense).

My proposal

1. Regarding the non-extension function, merge the receiver and dependencies and provide no parameters within the lambda. Users can access all the properties directly by referring to observable variables outside the lambda. That way, all dependencies are equal, and the API is simpler.
2. Regarding the extension function, remove dependencies from the signature. Why? Because, if you called it as an extension function, you're implicitly stating that you're making a "mapping" between the receiver and the resulting value, and that that mapping is relying only on the receiver to be calculated. If, on the other hand, you want to have multiple dependencies in your binding, use the non-extension function instead, because all dependencies are equal and there shouldn't be one (extension function receiver) that is different than others.

Before & After

While the resulting API is pretty similar in practice, the change it is definitely breaking the compatibility with previous versions.

Before:

fun <T> ObservableValue<T>.stringBinding(vararg dependencies: Observable, op: (T?) -> String?): StringBinding
        = Bindings.createStringBinding(Callable { op(value) }, this, *dependencies)

fun <T : Any> stringBinding(receiver: T, vararg dependencies: Observable, op: T.() -> String?): StringBinding =
        Bindings.createStringBinding(Callable { receiver.op() }, *createObservableArray(receiver, *dependencies))

After:

    fun <T> ObservableValue<T>.stringBinding(op: (T) -> String?): StringBinding
            = Bindings.createStringBinding({ op(value) }, this)

    fun stringBinding(vararg dependencies: Observable, op: () -> String?): StringBinding =
        Bindings.createStringBinding(op, *dependencies)

The change is most visible when reading the parameters of the function, but here's a (simplified) example usage:

    val prop1 = SimpleDoubleProperty()
    val prop2 = SimpleDoubleProperty()
    val prop3 = SimpleDoubleProperty()

    val product = doubleBinding(prop1, prop2, prop3) {
        val value1 = prop1.value
        val value2 = prop2.value
        val value3 = prop3.value
        value1 * value2 * value3
    }

    val prop1Squared = prop1.doubleBinding { it.pow(2) }

P.S. and while we're at it, I might as well add some documentation to the functions.

[SKeeneCode]

Firstly, thank you for the well put together proposal. I agree with you and would like to see this done. I've seen multiple people confused by the lack of documentation for the binding extension functions. There seems to be so few people using tornadofx2 compared to the original repo that we can (and should) be liberal with breaking changes, especially as we haven't released a proper version yet.

Would you be able to make a pull request?

[aleksandar-stefanovic]

Thank you for considering the proposal, I will get started on the pull request right away.

[aleksandar-stefanovic]

@SKeeneCode while refactoring, I've found many instances where the bindings are done on lists, e.g. DataGridTest.kt:75:

label(stringBinding(datagrid.selectionModel.selectedItems) { joinToString(", ") })

After the API change, it will look like this:

label(datagrid.selectionModel.selectedItems.stringBinding { datagrid.selectionModel.selectedItems.joinToString(", ") })

(The extension function can't be used in this case because the receiver is ObservableValue, while the ObservableList inherits only Observable).

On the one hand, the expression is clear and unambiguous, but on the other hand, statements can grow unwieldy, especially when accessing nested members like in aforementioned case. What do you think about adding a third type of binding functions, one that works specifically on lists? It would look something like this:

fun <T> ObservableList<T>.stringBinding(op: (ObservableList<T>) -> String?): StringBinding
        = Bindings.createStringBinding(Callable {op(this)}, this)

While it would reduce the unwieldiness of the list bindings (the aforementioned example would look like this:),

label(datagrid.selectionModel.selectedItems.stringBinding { it.joinToString(", ") })

it could also be something that is over-engineered, and increases code bloat.

What are your opinions on this?

[SKeeneCode]

I think it's a good idea to include the extra extensions because it keeps the API consistent between ObservableValue and ObservableList. Keeping the experience consistent for the programmer is probably more important than concerns of code bloat. Besides, tornadofx has always been pretty generous with helper extension functions across the codebase.

The real bloat comes from packaging things like a restful API or the JSON part with tornadofx when they really should be their own optional modules. ctadlock was working on this here. Thinking about finding a spare week at some point and doing it myself...

[pavlus]

@aleksandar-stefanovic Previous code

fun <T> ObservableValue<T>.stringBinding(vararg dependencies: Observable, op: (T?) -> String?): StringBinding
        = Bindings.createStringBinding(Callable { op(value) }, this, *dependencies)

adds change listener to the listed dependecies, while

fun <T> ObservableValue<T>.stringBinding(op: (T) -> String?): StringBinding
            = Bindings.createStringBinding({ op(value) }, this)

doesn't, so if dependency was changed, receiver won't be notified about it.

Because, if you called it as an extension function, you're implicitly stating that you're making a "mapping" between
the receiver and the resulting value, and that that mapping is relying only on the receiver to be calculated

Only if you call it without dependencies, otherwise it isn't.

It's ok to have change-listener with dependencies and unbound parameter. For example, you had a lambda that formats dependencies via StringProperty that contains format-template, like from locale-specific message bundle. You then bind this format-template property with something like:

label(interestingMessageTemplateProperty.stringBinding(foo, bar){ it.format(foo.value, bar.value) })

and then update that property when locale changes.

.stringBinding(foo, bar){ it.format(foo.value, bar.value) } is uninteresting here, it's piping, you read label(interestingMessageTemplateProperty. that's all you needed at the moment, while exploring sources, because you've seen intent: make a label that shows "interesting message".

And maybe you've seen many others like it below and above, you are reading form description, forms can be big. This way it's easy to read, but now you'll need to read this:

label(stringBinding(interestingMessageTemplateProperty, foo, bar){ interestingMessageTemplateProperty.value.format(foo.value, bar.value) })

More code, harder to read, and sometimes can go like this, to make things even worse:

label(stringBinding(foo, bar, interestingMessageTemplateProperty){ interestingMessageTemplateProperty.value.format(foo.value, bar.value) })

[aleksandar-stefanovic]

@pavlus

I understand what you're saying. There certainly are cases where the previous API worked better. However, we must either compromise on API clarity with the previous implementation or compromise on code conciseness with the current version. I'm honestly not sure which one I like best, but I can definitely say that the previous API looked like a "hack" or a "shortcut" to achieve code conciseness (which is not a bad thing by itself, but should be considered). Maybe we could come up with something better, that is the middle-ground between these two implementations?

It crossed my mind that we could do something kind of dirty implementation-wise, but elegant usage-wise. The idea was to utilize list destructuring to expose values within the lambda, like so:

fun integerBinding(vararg dependencies: Observable, op: (values: List<Any>) -> Int): IntegerBinding
        = Bindings.createIntegerBinding(Callable { op(dependencies.map { it.value }) }, *dependencies)

(note that this code will not compile, it's only for demonstration purposes)

In the ideal world, this implementation would allow something like this:

label(stringBinding(foo, bar, interestingMessageTemplateProperty) { (foo, bar, template) -> template.format(foo, bar)})

However, this code is impossible, because there is no way for the compiler to deduce the types of the values of the dependencies — it wouldn't be able to deduce that the template is of String type, etc.

So, I thought that we could maybe substitute the generalized vararg with specific implementations, up to 5 elements (more than 5 elements and it uses the vararg version instead), and this would, for example, be the 3-parameter variant:

fun <T1, T2, T3> integerBinding(
    dep1: ObservableValue<T1>,
    dep2: ObservableValue<T2>,
    dep3: ObservableValue<T3>,
    op: (val1: T1, val2: T2, val3: T3) -> Int
): IntegerBinding
        = Bindings.createIntegerBinding(Callable { op(dep1.value, dep2.value, dep3.value) }, dep1, dep2, dep3)

This would allow calls like the one that was impossible above.

HOWEVER this has two glaring issues:

• It would cause an explosion in the lines of code: for every type of binding, there would have to 7 functions (instead of current 2)
• It would cause issues with objects that are Observable, but are not ObservableValue (the current implementation supports Observables as well, and by issues I mean that it would revert to the vararg variant, that doesn't expose the values in the lambda, which can cause API usage confusion, that we tried to prevent in the first place.

---

Not to draw attention away from the original discussion, we definitely need to figure out which version is generally better, however, I see this as a moot point, unless someone with higher authority (like @edvin) makes a decision. Of course, Edvin wrote (or at least ratified) the original versions, so it would seem that the original is the way to go, but maybe Edvin didn't think about the arguments that I've provided in the issue, and would agree that the implementation could be like the one proposed.