-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New table fragment (#185) #368
Conversation
I know this is a draft, but perhaps we can squash it into one commit? |
Go ahead! I'd use |
@Baret do you think this will work with Java too? |
Sorry yeah, that's what I meant. |
Oh well umm... I guess you should be able to pass lambdas there, too. But I will try to port the example to java and see how it feels there. |
I'm starting to think that we should drop Java support. As others have stated Java/LibGDX and the like are pretty much dead WRT game development, but a multiplatform Kotlin setup is an interesting niche. |
If we're humoring this we should split this discussion off into a separate issue :) |
Yeah. Dropping Java support would just mean that from now on we no longer care about the DX of Java users, we don't need to refactor anything. 😄 |
Oops, I meant to write this comment in the issue here. It contains an explanation of what I intended to do here :) BTW I am not able to see why the build fails. Can someone give me an error message or hint? |
Using |
Oh ummm.... Oops? :D |
@@ -0,0 +1,6 @@ | |||
package org.hexworks.zircon.examples.fragments.table | |||
|
|||
enum class Gender { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might seem silly, but do you mind if we drop this particular column from the example? Seems needlessly complicating :)
If you just need an enum example, how about, um...Height { SHORT, AVERAGE, TALL }
.
): TableColumn<M, *, Label> = | ||
textColumn(name, width) { | ||
format | ||
.format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, so String#format
is purely a JVM construct, looking at this SO question. My recommendation would be to just drop this method completely and perhaps expose a (M) -> String
method in the API instead? Something to let the user either use Kotlin's string templates directly, or to access a ObservableValue.
import org.hexworks.zircon.api.builder.fragment.TilesetSelectorBuilder | ||
import org.hexworks.zircon.api.component.ColorTheme | ||
import org.hexworks.zircon.api.component.Fragment | ||
import org.hexworks.zircon.api.fragment.Selector | ||
import org.hexworks.zircon.api.resource.TilesetResource | ||
import kotlin.jvm.JvmStatic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out this line is required in order for this file to build 😄 if you put it back in it should work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm great find, why didn't IntelliJ tell me? :'(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No lie, I'm surprised that putting what looks like JVM-specific code in commonMain works at all. I guess it's just a no-op in the other targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the kotlin.jvm
package instantly looks suspicious ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have organizing the imports at commit enabled which makes this import disappear... annoying -.-
* | ||
* @param M the type of the underlying model representing this table's data. | ||
*/ | ||
interface Table<M: Any>: Fragment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Table<M: Any>
I see the missing space here as a recurring issue in this change. I'd double-check you're using the official code guidelines in IntelliJ, because the recommendation is to format this as Table<M : Any>
. Minor, for sure, but it's good to be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I totally agree. I just got used to refrain from formatting whole files. That leads to some lines sneaking through with ugly parts ;)
/** | ||
* Creates a [TableColumn] that represents it's cell values with a [Label]. The text of the label | ||
* is the toString() of the value provided by [valueAccessor]. | ||
* |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Not creative enough to find a good enum of people... ^^
Ok, I finally got it into a state where it builds and I can say: have a review please :) |
// TODO: Improve this loop to not loop over all elements | ||
data.forEach { model -> | ||
val newRow = newRowFor(model) | ||
if(remainingHeight > 0) { | ||
currentRows.add(addComponent(newRow)) | ||
} | ||
remainingHeight -= newRow.height + rowSpacing | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use a conventional for loop and break when remainingHeight <= 0?
...n.core/src/commonMain/kotlin/org/hexworks/zircon/internal/fragment/impl/table/TableColumn.kt
Outdated
Show resolved
Hide resolved
* @param V the type of the value of each cell in this column | ||
* @param C type of the [Component] used to represent each cell | ||
*/ | ||
open class TableColumn<M : Any, V : Any, C : Component>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be open
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to give the users of the API as much flexibility as possible. You should not be restricted to only use the predefined TableColumns
or this constructor. Maybe you have your own fancy implementation and want to use it over and over again so I thought it should be possible to extend this class.
/** | ||
* This example shows the usage of the table fragment. | ||
*/ | ||
object TableExample { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we attach a screenshot of what this looks like to the review somewhere? And/or a gif?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attached it to the issue as I think it is more "historically relevant" as this PR: #185 (comment)
|
||
// Some names, most of which are credited to https://github.com/imsky/wordlists | ||
|
||
private val firstNames: List<String> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ehhh...is this really necessary? How many do we really need?
zircon.core/src/commonMain/kotlin/org/hexworks/zircon/api/Columns.kt
Outdated
Show resolved
Hide resolved
zircon.core/src/commonMain/kotlin/org/hexworks/zircon/api/Columns.kt
Outdated
Show resolved
Hide resolved
....core/src/commonMain/kotlin/org/hexworks/zircon/internal/fragment/impl/table/DefaultTable.kt
Outdated
Show resolved
Hide resolved
require(data.isNotEmpty()) { | ||
"A table may not be empty! Please feed it some data to display." | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice to lift this requirement. Would that be possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought "sure, why not!", but when I just tried it I realized that there is selectedElement: Property<M>
. With an empty list we would need to handle that and I don't know how complicated that would become. I think Property
can not contain nullable values...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm...maybe the caller would have to provide a "null object pattern" object then? Not really a fan, but might be the only way?
I don't see it being terribly common but this does seem like something callers would expect to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @nanodeath . Imagine that you have filter fields above your table and the table itself has an ObservableList
so when you type something the list is updated accordingly. It might happen that there are no items that correspond to your query and the table will be empty.
Selection is also a different concept. You can have a table that has n
elements but none of them are selected.
See my comment above for a workaround for this problem that would also alleviate the need for null
s and null objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, @Baret ! I've left some comments in the code for some possible improvements. I can't wait to push Merge on this!
@@ -39,4 +40,7 @@ object Fragments { | |||
theme: ColorTheme | |||
): ColorThemeSelectorBuilder = ColorThemeSelectorBuilder.newBuilder(width, theme) | |||
|
|||
fun <M: Any> table(data: List<M>): TableBuilder<M> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for not using ObservableList
here?
/** | ||
* This object provides different presets for [TableColumn]s. | ||
*/ | ||
object TableColumns { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add @Beta
to this and all other new classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with 500e8cb
* When the value returned is an [ObservableValue] or a [Property] the textProperty of the label | ||
* will update accordingly. | ||
*/ | ||
fun <M : Any, V : Any> textColumn(name: String, width: Int, valueAccessor: (M) -> V): TableColumn<M, V, Label> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to have a version of textColumn
that explicitly has an ObservableValue
as its valueAccessor
? Just for good DX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not fully satisfied with the name, but added TableColumns.textColumnObservable()
with 7937969
import org.hexworks.zircon.internal.fragment.impl.table.DefaultTable | ||
import org.hexworks.zircon.internal.fragment.impl.table.TableColumn | ||
|
||
class TableBuilder<M : Any>(private val data: List<M>): FragmentBuilder<Table<M>, TableBuilder<M>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: can we use an ObservableList
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already wanted to do this but somehow forgot. The basics are there with 19518d0 but I feel like I want to further improve it ^^
At least I want to change the TableExample to start out with a table not fully filled and a + and - button to add and remove table entries to demonstrate the observable data.
require(data.isNotEmpty()) { | ||
"A table may not be empty! Please feed it some data to display." | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @nanodeath . Imagine that you have filter fields above your table and the table itself has an ObservableList
so when you type something the list is updated accordingly. It might happen that there are no items that correspond to your query and the table will be empty.
Selection is also a different concept. You can have a table that has n
elements but none of them are selected.
See my comment above for a workaround for this problem that would also alleviate the need for null
s and null objects.
/** | ||
* The element representing the currently selected row as [ObservableValue]. | ||
*/ | ||
val selectedRowValue: ObservableValue<M> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can kill two birds with one stone and modify this to be an ObservableList
and name it selectedRowsValue
. Then an empty list means no selection and you can also have multiple selected rows. You can change selectedRow
accordingly.
) | ||
} | ||
|
||
private fun dataPanel(panelSize: Size): VBox { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a dataPanel
? It is not clear from this.
@@ -4,7 +4,7 @@ import org.hexworks.zircon.api.Components | |||
import org.hexworks.zircon.api.component.Fragment | |||
import org.hexworks.zircon.api.data.Size | |||
|
|||
class Table<M : Any>( | |||
class TableOld<M : Any>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poor old table. Will it ever be young again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will carry it to its grave ⚰
Just forgot to do it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with 9870759
because I feel like it defines the table API instead of being an internal implementation. It is intended to be extended by zircon users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Basic solution for #185