Streamlining CellFactories

[thomasnield]

I wonder if there is opportunity to improve the cell factories for TableView and other data controls.

For instance, setting a mutable column in TableView to use a ComboBoxTableCell feels like it could be streamlined.

column("CATEGORY", ParsedTransaction::categoryProperty).apply {
    cellFactory = ComboBoxTableCell.forTableColumn(Category.all)
    setOnEditCommit {
        it.tableView.items[it.tablePosition.row].category = it.newValue
    }
}

It really should be just as easy as giving a List<R> or an ObservableList<R> for a given TableColumn<T,R> and the ComboBox just plugs in automagically.

column("CATEGORY", ParsedTransaction::categoryProperty).apply {
        applyComboBox(Category.all)
}

[thomasnield]

Here are the Cell Factories for TableView:

• CheckBoxTableCell
• ChoiceBoxTableCell
• ComboBoxTableCell
• ProgressBarTableCell
• TextFieldTableCell

I'm sure there are other cell factories for other data controls.

[thomasnield]

By the way, how exactly do I properly bind the CheckBoxTableCell to a given Property? The value does not persist when I scroll and it sometimes is just downright random, which I believe means the state is not being bound at all to the Property.

column("CONFLICTED", ParsedTransaction::conflictProperty).apply {
    cellFactory = CheckBoxTableCell.forTableColumn(this)

    setOnEditCommit {
        it.tableView.items[it.tablePosition.row].conflict = it.newValue
    }
}

[edvin]

That's a good idea. It should be possible to add a function that will be called on commit as well, for example to automatically save changes etc.

[thomasnield]

Yes, and literally just seconds before you I just posted an issue I was having with CheckBoxTableCell and the value not reflecting the model at all. This could be made more intuitive so people like me don't make mistakes or get confused : )

[edvin]

I think the default behavior is to cancel the commit when you just leave focus from the cell, which is counter to what users expect. Is this what you're experiencing?

[thomasnield]

Maybe, although the values are being pushed from the model and not the user inputs. Let me see if I can post an SSCCE later, which we can also use for testing when we make these factories.

[edvin]

Great! My experience is that you only need to do what you listed above, unless you want to override the default behavior of cancelling the edit on focus lost.

[edvin]

I've started working on an implementation here in the meantime :)

[edvin]

Initial suggestion for makeComboBox with support for optional operation after commit:

fun <S, T> TableColumn<S, T>.makeComboBox(items: ObservableList<T>, afterCommit: (TableColumn.CellEditEvent<S, T>.() -> Unit)? = null): TableColumn<S, T> {
    cellFactory = ComboBoxTableCell.forTableColumn(items)
    setOnEditCommit {
        val property = it.tableColumn.getCellObservableValue(it.rowValue) as ObjectProperty<T>
        property.value = it.newValue
        afterCommit?.invoke(it)
    }
    return this
}

[thomasnield]

Excellent! Just played with it and this is exactly what I was looking for.

[edvin]

Commited makeComboBox, makeTextField, makeChoiceBox, makeProgressBar and makeCheckbox now :) I will test more tomorrow and look for a solution for the cancel behavior you ran into!

[thomasnield]

Awesome, I'll test it with my application today.

[edvin]

Great! Let me know how it works, or if you can identify the problem you saw as well!

[edvin]

I think I got the names wrong. make might suggest that we create a new column, while we actually just change the cellFactory. This "clashes" with `makeIndexColumn" which actually creates a column. Suggestions for better names for these methods?

[thomasnield]

Maybe use or apply?

[edvin]

useComboBox, applyComboBox, asComboBox, toComboBox. Many options!

[thomasnield]

Hahaha, I'm biased towards use but I'll leave it up to you. Do you think we can apply a DatePicker one too?

[edvin]

use it is then! I'll make the change :) DatePicker is a bigger undertaking, we'd have to create a TableCellColumn with editing support. That's an UI component, which currently TornadoFX has none of. I can created it in tornadofx-controls and add special support for it. I was thinking of integrating them because of the Form support in tornadofx-controls anyway. That would be an optional dependency then.

[edvin]

Just crated a DatePickerTableCell @thomasnield: https://github.com/edvin/tornadofx-controls/releases/tag/v1.0.4

I will add TornadoFX Controls as an optional dependency and support useDatePicker for TableColumn.

[edvin]

About naming.. We already have TableColumn.makeEditable which actually does kind of the same thing as these functions. We need to make sure the API stays clean when we introduce these new functions. Not sure about that right approach.

[thomasnield]

I need to look again at your controls project as I think I need it for some things. And thanks for the DatePicker!

So you're saying makeXXX is not an ideal naming convention?

[thomasnield]

I think there's some interesting behaviors with CheckBoxTableCell and read-only properties, such as Bindings. I don't think it has to do with TornadoFX but JavaFX in general. It works find when I give it SimpleXXXProperty types, but it doesn't reflect values from read-only types. Therefore I wouldn't worry about CheckBoxTableCell.

[edvin]

Sometimes it's nice to be able to show checkboxes that are not editable for boolean properties, I'll see if I can get it working! I little short on time today, but I'll fix it tomorrow :) Nice catch!

[edvin]

Oh, and yeah, there is something with the naming that seems off to me. I'll discuss with a couple of colleagues tomorrow to try to get some ideas and report back. Hoping to land these features and maybe do a release before the weekend. Have a few other functions I'd like to get in as well :)

[thomasnield]

Awesome, I'll let you know if I have any ideas. Sorry I've been busy this past two days, I'll see if I can allocate some time to ponder this too.

[edvin]

Just renamed the functions like we talked about :) I will close this now, the CheckBoxTableCell is really a JavaFX issue, so I won't use any more time on it now.

[thomasnield]

Agreed. Awesome.