bindChildren breaks when constructor contains builder methods

[NIKE3500]

I my current project I try to use bindChildren to fill a VBox with custom controls. These controls use builders in their constructor to set themselves up. Unfortunately it turns out this breaks bindChildren. There are two ways this can break:

1. The application throws an exception (case 1) or
2. a node that should be added will not be added to its parent (case 2). I will attach some sample code that highlights both issues.

I did some debugging and the cause of this seems to be the handling of the FX.ignoreParentBuilder flag. MutableList<TargetType>.bind sets this to avoid adding a control to its parent when calling a builder and thus opcr in bindChildren (or comparable). Doing so would cause an exception as the binding will try to add that control as well, which then leads to a duplicate. However, a call chain in the form of bindChildren->builder->constructor->builder causes the inner builder to consume the FX.ignoreParentBuilder, which leads to two issues:

1. the inner builder does not actually add a control
2. the setup of the outer builder breaks thus leading to adding a duplicate in bindChildren The second problem might not be immediately apparent depending on the setup (as can be seen in the sample below). The issue only arises when adding an element to an already bound collection, as the initial binding clears the target parent.

I created a sample for you to verify this behavior. It can simply be used as the MainView of a TornadoFX application. In my opinion it should be possible to use builders in constructors and the FX.ignoreParentBuilder flag seems to fragile to properly work. Unfortunately I have to admit that currently I have no idea how to fix this properly...

Here is the sample code:

package org.bindChildren.issue.view

import javafx.collections.FXCollections
import javafx.event.EventTarget
import javafx.scene.layout.Region
import javafx.scene.layout.VBox
import tornadofx.*

class MainView : View("Hello TornadoFX") {
    private lateinit var vBox: VBox

    override val root = borderpane {
        prefHeight = 200.0

        center = scrollpane {
            vBox = vbox {
            }
        }

        bottom = vbox {
            // This works perfectly fine and is just for demonstration purposes...
            button("Add single") {
                action {
                    vBox.apply {
                        myCustomControl("single")
                    }
                }
            }

            // This add no children will be added
            // as the FX.ignoreParentBuilder flag is consumed by the label() builder
            // inside MyCustomControl.init.
            // However, no exception will be thrown as the binding clears vBox.children
            button("Set via bindChildren (case 1)") {
                action {
                    vBox.apply {
                        val values = FXCollections.observableArrayList<String>()
                        values.add("testValue1")

                        bindChildren(values) {
                            myCustomControl(it)
                        }
                    }
                }
            }

            // This add no children will be added
            // as the FX.ignoreParentBuilder flag is consumed by the label() builder
            // inside MyCustomControl.init.
            // Additionally an exception will be thrown because adding to an already
            // bound list will not clear vBox.children thus leading to a duplicate.
            button("Add to bound children (case 2)") {
                action {
                    vBox.apply {
                        val values = FXCollections.observableArrayList<String>()

                        bindChildren(values) {
                            myCustomControl(it)
                        }

                        values.add("testValue1")
                    }
                }
            }
        }
    }
}

/**
 * Builder for adding a MyCustomControl
 */
fun EventTarget.myCustomControl(text: String, op: MyCustomControl.() -> Unit = {}): MyCustomControl {
    val ctrl = MyCustomControl(text)
    return ctrl.attachTo(this, op)
}

/**
 * A simple control using a builder in its constructor, thus breaking bindChildren.
 */
class MyCustomControl(text: String) : Region() {
    init {
        label(text) {}
    }
}

[edvin]

Thanks for the thorough explanation. We need to make ignoreParentBuilder component aware I think. Will look into it.

[NIKE3500]

I also think this would be the best solution. However, I don't see how this should work when directly binding a collection (e.g. the children collection of a component)

[edvin]

Correct, we need another solution for this case.

[NIKE3500]

I thought a bit about all this and came to the conclusion that the MutableList.bind() method cannot be saved. In its current state it has a side effect that on the one hand might not be expected and on the other hand does not serve the purpose it was created for (as can be seen above). The side effect can also not be fixed as one cannot be sure in which context the method is called.

My suggestion would therefore be to remove or at least deprecate the method. If a "bind" method for normal lists is required it should be implemented without such global side effects. When using it with builders for binding a children collection it should simply cause the "duplicate" exception. Using builders to create and attach a component and then attaching it again is an issue that is debuggable and should be more or less easily visible for developers.

For binding children lists I would suggest an overload for bindChildren

• EventTarget.bindChildren(targetSet, sourceSet, converter)
• EventTarget.bindChildren(sourceSet, converter) = bindChildren(getChildList(), sourceSet, converter)

In their documentation it should be clearly stated that these methods can be used with builders and should be used if builders are a requirement. In the methods an attached property (ideally a counter) on "this" should keep track on whether builder should currently create objects. opcr should check and respect that flag such that existing builders still work (at least the ones provided by the framework). Having two methods here allows ditching the normal bind method as for example for a BorderPane the center.bind call would simply become bindChildren(center, ...). I admit that this is not as elegantly written, but in my opinion the tradeoff is worth it.