edvin / tornadofx

Lightweight JavaFX Framework for Kotlin
Apache License 2.0
3.67k stars 269 forks source link

Feature: Bind collections to Layouts #261

Closed dominiks closed 7 years ago

dominiks commented 7 years ago

I often encounter a situation in my application where I would like to fill a Pane with Fragments by binding it to a model collection. For this I have implemented a BoundLayout using some of the code from the DataGrid.

This fragment uses an ObservableCollection<T> and a factory method (t) -> Node to fill the Pane it uses as its root.

See the class here: https://gist.github.com/dominiks/2d66e46acb91a6383e6aea9647dc7f84

Is there interest to integrate and improve this construct into tornadofx? I'm sure there are many things to improve as it is only a working prototype that seems to fit my needs. Also I am not quite satisfied with the usage of it within a builder.

Here is an example of the usage with an ObservableCollection<String>, using the default VBox as the root:

    this += BoundLayout(recentProjects).apply {
        root.apply {
            spacing = 5.0
            addClass(Styles.recentList)
        }
        nodeFactory = { RecentProjectEntry(it).root }
    }
edvin commented 7 years ago

Can you show me a use case for this, preferably some runnable code?

thomasnield commented 7 years ago

I'm going to say it... this sounds like an Rx task.

dominiks commented 7 years ago

Here is a quick example: https://gist.github.com/dominiks/efe2c9b886a6cdc6ff914cfdb951ea0f

Start the ProjectListView to see it in action. You have a VBox that is filled with ProjectFragments by editing a ObservableList<Project>.

How would one do this with Rx? I haven't had the time yet to dive into the whole Rx topic.

thomasnield commented 7 years ago

Okay... so if I'm understanding correctly. You want to take an ObservableList<T> and map it to an ObservableList<Node> more or less., and keep it always synchronized... while disposing Nodes that are no longer in view.

Here is how you can do with it with RxJavaFX or RxKotlinFX. If this solves your problem, check out the free eBook Learning RxJava with JavaFX to learn more about reactive JavaFX techniques, which shows examples in both JavaFX and TornadoFX.

rxjavafx_listview

import javafx.collections.FXCollections
import javafx.scene.control.Label
import rx.javafx.kt.actionEvents
import rx.javafx.kt.additions
import rx.javafx.kt.removals
import rx.lang.kotlin.toObservable
import tornadofx.View
import tornadofx.button
import tornadofx.listview
import tornadofx.vbox

class Item(val id: Int, val description: String)

class ItemLabel(val item: Item): Label("${item.id} - ${item.description}") {
    fun dispose() = println("Disposing ${item.id} - ${item.description}")
}

class MyView: View() {

    val hiddenItems = FXCollections.observableArrayList(
            Item(1,"Alpha"),
            Item(2,"Beta"),
            Item(3,"Gamma"),
            Item(4,"Delta"),
            Item(5,"Gamma")
    )

    val currentItems = FXCollections.observableArrayList<Item>()

    override val root = vbox {
        listview<ItemLabel> {

            //handle additions
            currentItems.additions()
                    .map(::ItemLabel)
                    .subscribe { items.add(it) }

            //handle removals
            currentItems.removals()
                    .flatMap { removedItem ->
                        items.toObservable()
                                .filter { it.item.id == removedItem.id }
                                .last()
                    }
                    .subscribe {
                        it.dispose()
                        items.remove(it)
                    }
        }

        button("ADD").actionEvents()
                .filter { hiddenItems.size > 0 } //skip event if there are no items to add
                .subscribe {
                    val addItem= hiddenItems[0]
                    currentItems.add(addItem)
                    hiddenItems.removeAt(0)
                }

        button("REMOVE").actionEvents()
                .filter { currentItems.size > 0 } //skip event if there are no items to remove
                .subscribe {
                    val removeItem = currentItems[currentItems.size - 1]
                    hiddenItems.add(removeItem)
                    currentItems.remove(removeItem)
                }
    }
}
thomasnield commented 7 years ago

Another way to achieve this is to "rebuild" all contents for ListView<ItemLabel> each time the ListView<Item> changes. Depending on how expensive each Node is, this may or may not be desirable.

import javafx.collections.FXCollections
import javafx.scene.control.Label
import rx.javafx.kt.actionEvents
import rx.javafx.kt.onChangedObservable
import rx.lang.kotlin.toObservable
import tornadofx.View
import tornadofx.button
import tornadofx.listview
import tornadofx.vbox

class Item(val id: Int, val description: String)

class ItemLabel(val item: Item): Label("${item.id} - ${item.description}") {
    fun dispose() = println("Disposing ${item.id} - ${item.description}")
}

class MyView: View() {

    val hiddenItems = FXCollections.observableArrayList(
            Item(1,"Alpha"),
            Item(2,"Beta"),
            Item(3,"Gamma"),
            Item(4,"Delta"),
            Item(5,"Gamma")
    )

    val currentItems = FXCollections.observableArrayList<Item>()

    override val root = vbox {
        listview<ItemLabel> {

            //handle changes
            currentItems.onChangedObservable()
                    .flatMap {
                        it.toObservable()
                                .map(::ItemLabel)
                                .toList()
                    }
                    .subscribe {
                        items.forEach { it.dispose() }
                        items.setAll(it)
                    }
        }

        button("ADD").actionEvents()
                .filter { hiddenItems.size > 0 } //skip event if there are no items to add
                .subscribe {
                    val addItem= hiddenItems[0]
                    currentItems.add(addItem)
                    hiddenItems.removeAt(0)
                }

        button("REMOVE").actionEvents()
                .filter { currentItems.size > 0 } //skip event if there are no items to remove
                .subscribe {
                    val removeItem = currentItems[currentItems.size - 1]
                    hiddenItems.add(removeItem)
                    currentItems.remove(removeItem)
                }
    }
}
dominiks commented 7 years ago

Thank you for the links and code examples. Having these lists synchronized or the disposal of items is not the primary issue there, but not using a ListView is. I want to add a list of model objects to a pane and tell it how to render these items so I dont have to manage the children of that pane by hand.

Imagine something like this. The rectangular items there would be the fragments that are within a FlowPane or TilePane.

thomasnield commented 7 years ago

@dominiks Ah so it is a custom control you are look for then...

dominiks commented 7 years ago

Yes, it may very well be a custom control that I constructed as a fragment.

edvin commented 7 years ago

I think we could get even more from this if we made it a more general utility.

What if we create an observable list wrapper than can wrap ObservableList<T> and keep another ObservableList<R> in sync via a mapping function. This could be used to turn a list of T into nodes, which could update the children list of the desired control.

Consider the following, code. (I'm adding types so it is easier to see what's going on)

// The target Node
val vbox: VBox = vbox()

// Our list of input objects
val projects: ObservableList<Project> = projectController.listProjects()

// Sync projects into the vbox children node list
projects.sync(vbox.children) { project -> ProjectFragment(project).root }

On top of that we could add some overrides that knows how to turn a list of UIComponent (Fragment/View) into a list of Nodes, so the above could be written:

projects.sync(vbox.children) { ProjectFragment(it) }

The code you have could mostly be a base for this, but we must make sure that we keep the items at the same index in both lists.

With this approach we avoid dictating the layout container, because we can sync to any kind of observable list. It could be used to generate tabs in a TabPane just as well as vbox children.

What do you guys think?

For situations where the list of input elements never change, this is completely overkill of course. You'd just do something like:

vbox {
    projectController.projects.forEach {
        add(ProjectFragment(it))
    }
}
dominiks commented 7 years ago

This seems to be an elegant solution.

I took another look at the JavaFX documentation, especially the bindings, and there is bindContent(List<E> list1, ObservableList<? extends E> list2) which can be used to bind the contents of an ObservableList<Node> to the childrens property. The problem is then reduced to the issue of having a list of models synchronized to this list of nodes.

edvin commented 7 years ago

Great catch! We might even switch to using that for several other constructs that does similar stuff to lists.

edvin commented 7 years ago

I just implemented this and it seems to work surprisingly well. Will integrate it so you can play with it soon :)

dominiks commented 7 years ago

Wow, I was just about to start implementing it myself, researching how to properly handle the permutation change in the listener. I completely overthought it instead of just doing it by index like you do - very nice. Though it's not "perfect" as it re-creates the nodes but it should be perfectly fine in 99% of use cases.

Also I find your wasRemoved handler very impressive and a great display of proper Kotlin usage:

if (change.wasRemoved()) {
    list.subList(change.from, change.from + change.removedSize).clear()
}

Thanks for your quick (and exemplary) work!

edvin commented 7 years ago

Sweet! We could expand this with caching if the need arise. What's nice now is it only recreates the node if the objects are changed, so you can control caching/flushing to some degree already.

I'm not sold on the name I gave it, I'll need some help to decide the proper name. Do you have a suggestion? I'm thinking about creating a short screencast to showcase this and posting it to the Slack channel to get some feedback as well.

edvin commented 7 years ago

I thought about it some more, you can actually quite easily cache inside the converter function already, since you get passed the input element. Then you can decide if you want to recreate or keep, even when a permutation event occurs.

dominiks commented 7 years ago

I think the name .bindAndConvert is completely fine for what it does. An alternative would be bindMapped as it maps the values to the other list. For the ListConversionListener I have no idea.

Additionally I think it might make sense to add a convenience method fun <SourceType> Pane.bindItems(items: ObservableList<SourceType>, op: (SourceType) -> Node), named after the items Property of ListView and the other views. This method can be used a starting point for documentation of this feature in its "natural" use case.

hbox(10.0).bindItems(projects) { ProjectFragment(it).root }

Should then be possible.

// edit: An even more convenient function would allow hbox(10.0).bindItems(projects, ProjectFragment::class) ;)

edvin commented 7 years ago

Good points. But then maybe we should switch this around so that we do target.bindAndConvert(source) so it aligns better with pane.bindItems(source)? We have an EventTarget.getChildList() that knows how to extract the items/children list from lots more than just Pane, so we can actually put the bindItems onto EventTarget to make it even more useful.

edvin commented 7 years ago

I tried swapping it around, and it works but now the name bindAndConvert feels a little weird.

val numbers = FXCollections.observableArrayList("One", "Two", "Three")
val nodes = FXCollections.observableArrayList<Node>()
nodes.bindAndConvert(numbers, ::Label)
println(nodes)

Output:

[Label@6fd02e5[styleClass=label]'One', Label@5bcab519[styleClass=label]'Two', Label@e45f292[styleClass=label]'Three']
edvin commented 7 years ago

The semantics is now more correct though, as it matches JavaFX's bind. Maybe the name is OK? Or bindConverted/bindMapped perhaps?

edvin commented 7 years ago

I have to step away for awhile, so I renamed it to bindConverted so you can see how you like it :)

dominiks commented 7 years ago

Staying consistent with the binding semantics of JavaFX is a good point. Keep it as bindConverted or go another step and just call it bind.

edvin commented 7 years ago

Let's go with bind :)

edvin commented 7 years ago

I also added EventTarget.bindChildren which will find the children list of the event target and bind that to the given list + converter. Unfortunately I couldn't use the same name for the version that binds to UIComponent and extracts the root node, so I called it EventTarget.bindComponents - I'm really bad at naming stuff :)

edvin commented 7 years ago

I created a short screencast to show this in action:

https://youtu.be/0Q-BUldFZjA

edvin commented 7 years ago

Are you happy with this, should we close the issue?

dominiks commented 7 years ago

I watched the screencast, updated my project to 1.7 and this seems like a perfect solution - even more capapable than what I had initially needed. I am very happy with this.

Thanks for your efforts in ever improving tornadofx.

edvin commented 7 years ago

Great :) Thanks for bringing this up, it was a fun project to work on :)