Refactoring Vis module about ktx HOT 13 CLOSED

Bonus: Fragment API

While builders are great they tend to create nested hell of clousers, this is attempt to prevent this by allowing to easily extract UI pieces to so called fragments.

API declaration:

fun fragment(ui: () -> Actor): VisFragment = object : VisFragment() {
    override val ui = ui()
}

fun tableFragment(buildUi: KVisTable.() -> Unit): VisFragment = object : VisFragment() {
    override val ui = table { buildUi(this) }
}

abstract class VisFragment {
    open val ui: Actor = Actor()
}

fun <T : VisFragment> WidgetFactory.fragment(fragment: T): T {
    actor(fragment.ui, {})
    return fragment
}

New extension methods for CellAccessor:

    fun <T : VisFragment> T.cell(): Cell<Actor> {
        return this.ui.userObject as Cell<Actor>
    }

    fun <T : VisFragment> T.cell(initCell: Cell<Actor>.() -> Unit): T {
        cell().initCell()
        return this
    }

    fun <T : VisFragment> T.row(): T {
        cell().row()
        return this
    }

Usage:
Declaring fragment as class. This won't compile with above subset of new vis api but its taken straight out my project, only changed direct cell modification to new .cell { }

class ExampleFragment : VisFragment() {
    lateinit var pakFileView: PakFileView
    lateinit var listView: ListView<File>
    lateinit var fileAdapter: FilteredFileAdapter
    lateinit var filterEntry: VisTextField

    override val ui = table {
        background = VisUI.getSkin().getDrawable("window-bg")
        horizontalGroup {
            label("Filter ")
            filterEntry = textField("")
        }.cell { growX() }.row()

        fileAdapter = FilteredFileAdapter(extension, File(baseFateCPKUnpackPath, "pack").walk().toList())

        table {
            defaults().growY()
            listView = listView(fileAdapter).cell { prefWidth(300f) }
            pakFileView = add(PakFileView()).cell { grow() }
        }.cell { growX() }


        listView.setItemClickListener {
             pakFileView.loadPak(it)
         }
        filterEntry.onChange { changeEvent, field ->
            fileAdapter.filterList(field.text)
        }
    }
}

Or just create fragment with method declaration

fun fragmentOnDemand(): VisFragment = tableFragment {
    label("A")
    label("B")
    label("C")
}

fun fragmentOnDemand2(): VisFragment = fragment {
    horizontalGroup {
        label("A")
        label("B")
        label("C")
    }
}

Adding fragments to UI:

fun buildUI() {
    table {
        val frag: ExampleFragment = fragment(ExampleFragment()).cell { growX() }.row()
        fragment(fragmentOnDemand()).cell { growX() }
        fragment(fragmentOnDemand2()).cell { growX() }
    }
}

Alternatives

Instead of fragments is possible to just use standard widgets but this creates API like this

class TestTable : VisTable(false) {  // class must be referenced by name
    init {
        table {  // no way to start UI building without call to base builder function

        }
    }
}

Function version is similar to fragment though

fun testTableFunction(): Table = table {
    label("A")
}

Adding to UI

table {
    actor(TestTable()).cell { growX() }
    actor(testTableFunction()).cell { growX() }
}

Other method with exposing K* widgets.

class TestTable2 : KVisTable() { // KVisTable must be made `open` and directly referenced
    init { // no need to call base builder method
        label("A")
    }
}

from ktx.

I'm not a fan of manipulating Cell properties outside of the building block of the actual actor. This leads to actor properties being scattered at two places.

table {
  table {
    setTouchable(Touchable.disabled) // 1
    table {
       // More actors...
       // More actors...
       // More actors...
    }
  }.cell(expand = true) // 2
}

This might not seem like that big of a deal while the UI is still small, but imagine the table had 8 children. When configuring actors at the bottom of the building block, you visually lose track of what's actually configured - when the actor's properties are at the top of the building block, you automatically associate them with this particular actor. I find the approach of "open building block, configure, add children" more readable and I'd try to refactor VisUI to expose table cells and tree nodes as building block parameters (available as it and optionally renamed).

window(title = "Hello world!") {
  button { cell ->
    // Changing button properties - button available as "this":
    color = Color.CORAL
    // Changing cell properties:
    cell.fillX().row()
  }
}

from ktx.

I disagree about losing track of what is actually configured. When creating UI you also have to look how Cell of other elements are configured. In my approach, it ends up in standard place after closing parenthesis, easy to notice. It's on the same indent level as actor declaration, that's makes it easier to see what is configured.
In case of your approach it will end up in the middle of actor configuration and adding more children.

There is also important factor to consider: should you create such big structures? They will create big nested blocks hard to read anyway. That's why I'm also proposing easier way to extract fragments.

from ktx.

I disagree about losing track of what is actually configured.

I guess it comes down to preferences and code style. For me, configuring cell properties is not unlike managing the actor's internals - that's why I like to keep it together, close to the actor declaration.

They will create big nested blocks hard to read anyway. That's why I'm also proposing easier way to extract fragments.

Not sure if the benefits justify introducing another API that the users would have to learn. I think extracting common code to utility file-scope functions would do fine without the additional complexity of another API - this would feel very familiar to introducing custom actor builders. I don't see why you couldn't approach it like this:

fun KTable.greeting(): Cell<Table> =
  table {
    // Configure some special actors.
  }.cell()

// Usage:
table {
  greeting().expand()
  // Other actors.
}

There is also important factor to consider: should you create such big structures?

Depends on the application and use case, really. Reusability is one thing, but if you want to separate UI building code into functions for the sake of having 6 methods with 5 lines (that you'll never use again) instead of one building block with 30 lines - that's when I think having it in one place is actually more readable. I think building blocks of complex views up to as much as 60 lines might be acceptable - and that's when you can lose track of things and cells after the blocks are not as readable. Maybe that's because I think of KTX views in terms of a markup language (like HTML) rather than actual application logic - you always configure the HTML tags with attributes in the declaration.

But you know what? The best thing about Kotlin flexibility is that we can use both approaches simultaneously. cell method usage would be optional, and lambda parameter could be ignored without consequences. This would allow the users to choose the approach that they prefer, depending on the situation.

table {
   label("No API clash!") { cell ->
     cell.row()
   }
   label("Yup.").cell(expand = true)
}

What do you think?

from ktx.

Not sure if the benefits justify introducing another API that the users would have to learn.

Yeah, that's why I tried to keep API simple. I like the way with extension method, seems better. Fragments was more targetted to help with extracting part of view to separate class that will have some extra fields and methods related to it, avoiding cluttering current class.

Of course extracting fragments just for the sake of is not useful but in real application you will probably have UI blocks that would be beneficial to have separated. Even if they won't be reusable. For example left section of Vis file chooser with disk list and favourites, it would have some method that update view with new favourites and so on.

But you know what? The best thing about Kotlin flexibility is that we can use both approaches simultaneously.

Well, I guess that just solves this.

from ktx.

Well, I guess that just solves this.

In fact, now I plan on adding scoped cell and node methods to children of Table/Tree widgets to ktx-scene2d, although I don't think they should return Cell/Node instances. Instead, I'll add cell function (that allows to configure Cell setting with parameters and cell property (that returns the actual cell if the user needs it for some reason).

table {
   val label: Label = label("Cell settings example.").cell(expand = true)
   val cell: Cell<Label> = label("Cell instance reference.").cell
   val combined: Cell<Label> = label("Awkward, but powerful.").cell(expand = true).cell
}

This will hopefully blur the difference between ktx-scene2d and ktx-vis API. Can you investigate if you can easily add support for Cell lambda parameter in building blocks of ktx-vis?

from ktx.

Can you investigate if you can easily add support for Cell lambda parameter in building blocks of ktx-vis?

Kind of

//WidgetFactory changes
fun label(text: String, styleName: String = DEFAULT_STYLE, init: VisLabel.(R) -> Unit = {}): R

fun <T : Actor> actor(actor: T, init: T.(R) -> Unit): R {
    val result = addActorToWidgetGroup(actor)
    actor.init(result)
    return result
}
//TableWidgetFactory
override fun label(text: String, styleName: String, init: VisLabel.(Cell<*>) -> Unit): Cell<VisLabel>
//WidgetGroupWidgetFactory
override fun label(text: String, styleName: String, init: VisLabel.(Actor) -> Unit): VisLabel = super.label(text, styleName, init) as VisLabel

And then usage

table {
  label("A") {
    val a: VisLabel = this
    val b: Cell<*> = it
  }
}

horizontalGroup {
  label("A") {
    val a: VisLabel = this
    val b: Actor = it
  }
}

The question is, do we really want to keep this current implementation instead of doing it ktx-scene2d way?

from ktx.

I don't think inlined builder methods are a huge advantage over regular lambdas in this case, since the UI is usually built once and the additional cost would be pretty much negligible. Rewriting a module that just works is a huge undertaking for little gain. That said, it would be nice if the two libraries could share some common code - ktx-vis seems like a natural extension of ktx-scene2d.

Estimate how much work it would take to add lambda parameters and to completely switch to ktx-scene2d approach of inlined blocks. Find out if any APIs would be simplified in the process. I honestly don't mind if the two modules are slightly different and mutually exclusive, and I certainly don't want you working on something that you don't think makes sense.

from ktx.

To be honest, now that I've investigated Kotlin 1.1 features, I consider ktx-scene2d incomplete. You can proceed with the estimation after #46 and #48 are done. I'll try to make it this weekend.

from ktx.

Thanks, I'm glad that now ktx-scene2d and ktx-vis APIs are pretty much consistent. I think we can close this issue after updating CHANGELOG.md.

from ktx.

Yup, though implementation is quite different.

Some CHANGELOG.md points apply both to ktx-vis and ktx-scene2d. I guess we can mention two modules for one point?

Edit: Never mind, I noticed it's already done for ktx-style.

from ktx.

Updated changelog. @czyzby fell free to close.

from ktx.

Thanks.

from ktx.