Workflow visualizer UI improvement #1366
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,26 +7,52 @@ package com.squareup.workflow1.traceviewer.model | |
| * | ||
| * TBD what more metadata should be involved with each node, e.g. (props, states, # of render passes) | ||
| */ | ||
| internal data class Node( | ||
| val name: String, | ||
| val id: String, | ||
| val parent: String, | ||
| val parentId: String, | ||
| val props: String, | ||
| val state: String, | ||
| val rendering: String = "", | ||
| @Transient val children: LinkedHashMap<String, Node> = LinkedHashMap() | ||
| ) { | ||
|
There was a problem hiding this comment. This class looks like it's intended to basically be a database table, where the properties are referenced dynamically by the UI. Can these properties just be stored in a map, with keys defined as constants? No need to use reflection. There was a problem hiding this comment. These properties are all here for the purpose of moshi being able to parse a file into these objects. Unless there is a way to use moshi in the way you're describing it. Even then, I'm not sure what the difference is. Could you help me explain the use case of having a data class like this vs using a map and treating it like a database table? There was a problem hiding this comment. The map woudl be just key : value where both are strings. Since everything but children here is a string, the idea would be that essentially we are using this As for Moshi I imagine they have a default adapter that just maps JSON properties to a map. Where it gets slightly more complicated is with the You could do something like: and then you'd have to change the parsing. ultimately probably that would be the 'best' fit for the data (and you'd be able to avoid using reflection as the properties list change since you'd always iterate over all the keys). As for whether you do that now, I'm not too worried either way 🤷🏻 There was a problem hiding this comment. understood, this makes sense. However, I still don't understand the distinction that @zach-klippenstein made originally about classes and "database table classes". Are data classes not meant to be used in a way that just holds data? There was a problem hiding this comment. ya they are meant for that. I think the distinction comes down to the type of data. if you have properties of different strongly types vs if you just have a set of There was a problem hiding this comment. ah gotcha, I understand now, thanks for clarifying that. |
||
|
|
||
| override fun toString(): String { | ||
| return "Node(name='$name', parent='$parent', children=$children)" | ||
| } | ||
|
|
||
| override fun equals(other: Any?): Boolean { | ||
| if (this === other) return true | ||
| if (other !is Node) return false | ||
| return this.id == other.id | ||
| } | ||
|
|
||
| override fun hashCode(): Int { | ||
| return id.hashCode() | ||
| } | ||
|
|
||
| companion object { | ||
| fun getNodeField(node: Node, field: String): String { | ||
| return when (field.lowercase()) { | ||
| "name" -> node.name | ||
| "id" -> node.id | ||
| "parent" -> node.parent | ||
| "parentid" -> node.parentId | ||
| "props" -> node.props | ||
| "state" -> node.state | ||
| "rendering" -> node.rendering | ||
| "children" -> node.children.toString() | ||
| else -> throw IllegalArgumentException("Unknown field: $field") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| internal fun Node.addChild(child: Node): Node { | ||
| return copy(children = LinkedHashMap(this.children.plus(child.id to child))) | ||
| } | ||
|
|
||
| internal fun Node.replaceChild(child: Node): Node { | ||
| return addChild(child) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package com.squareup.workflow1.traceviewer.model | ||
|
|
||
| /** | ||
| * Represents the difference between the current and previous state of a node in the workflow trace. | ||
| * This will be what is passed as a state between UI to display the diff. | ||
| * | ||
| * If it's the first node in the frame, [previous] will be null and there is no difference to show. | ||
| */ | ||
| internal class NodeUpdate( | ||
| val current: Node, | ||
| val previous: Node?, | ||
| ) |
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 also wonder if this is not needed?
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.
+1 - do we know why this is needed? if not, lets remove.
Uh oh!
There was an error while loading. Please reload this page.
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.
it seems like it's needed for registering and running junit tests in a multiplatform app?
https://kotlinlang.org/docs/jvm-test-using-junit.html#add-dependencies
But I havent removed the logging parts afterwards, those aren't necessary.
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.
Great. sounds good.