-
Notifications
You must be signed in to change notification settings - Fork 104
Workflow visualizer prototype #1335
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
Conversation
- Allows users to freely zoom and pan to view different sections of the tree
The attributes of each node is tightly coupled with the content of the json, so any changes to the JSON will lead to an error unless the WorkflowNode class is also changed accordingly.
…ow node - arrows start with a weird offset in the beginning relative to where the nodes are
…Snapshot Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@@ -5,5 +5,12 @@ import androidx.compose.runtime.Composable | |||
|
|||
@Composable | |||
fun App() { | |||
Text("Hello world!") | |||
val jsonString = object {}.javaClass.getResource("/workflow-simple.json")?.readText() |
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 think getClass().getResource should work here
- if modifier is set to unbounded, content also needs to be aligned.
- Density of displays caused weird errors of how the arrows appear - Scaling and translating the arrow positions was also separate from the movement of the nodes
- Figuring out how to lay out arrows was unnecessarily difficult, and ended up being too distracting, so utilizing the column space each node and its children are in was a much cleaner way to represent a subtree relationship - Figure will closely resemble a treemap
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@@ -23,6 +23,8 @@ kotlin { | |||
implementation(compose.desktop.currentOs) | |||
implementation(libs.kotlinx.coroutines.swing) | |||
implementation(compose.materialIconsExtended) | |||
implementation(libs.moshi.kotlin) | |||
implementation("io.github.vinceglb:filekit-dialogs-compose:0.10.0-beta03") |
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.
Add this to gradle/libs.versions.toml
and then reference it here rather than the direct include (eases maintenance).
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
modifier | ||
.fillMaxSize() | ||
.pointerInput(Unit) { | ||
// this allows for user's panning to view different parts of content |
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 allows for user's panning to view different parts of content | |
// This allows for user's panning to view different parts of content. |
All comments should be sentences and use punctuation as such.
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.
Here and elsewhere.
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 and fixed all other grammatical errors in comments.
.fillMaxSize() | ||
.pointerInput(Unit) { | ||
// this allows for user's panning to view different parts of content | ||
awaitEachGesture { |
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 this from a blog post or ref article or something? If so, it can be valuable to post the link in a comment as then we can follow up during maintenance.
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.
Added note that code is AI generated.
if (drag != null && drag.pressed) { | ||
var prev = drag.position | ||
while (true) { | ||
val nextEvent = awaitPointerEvent() |
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.
there is no way to get stuck here right? As in, is it possible we already captured event.changes but those were all of them and there is nothign to await()
? or is this await()
ing changes we already know we have?
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.
Changed functionality with higher-level onDragGestures
API from Android
import io.github.vinceglb.filekit.dialogs.compose.rememberFilePickerLauncher | ||
|
||
@Composable | ||
public fun UploadFile( |
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.
Needs kdoc.
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
* All the caught exceptions should be handled by the caller, and appropriate UI feedback should be | ||
* provided to user | ||
*/ | ||
public fun fetchRoot( |
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.
Maybe change name to parseTrace
or something? fetchRoot
does not communicate all that it is doing, even though the root WorkflowNode?
is what is returned.
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
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
c968d7f
to
a1099dc
Compare
This was a miss on a previous PR (the Kotlin 2.0 one and my own one adding this module...) but we need to add |
@wardellbagby , do you mean |
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. A couple things still before merge:
- Add the
.klib
to.gitignore
as @wardellbagby was suggesting - get sign off from someone more well versed with Compose then me (@wardellbagby , @zach-klippenstein, etc.)
detectDragGestures { _, translation-> | ||
offset += translation | ||
} |
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.
Nice!
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.
Left a few comments for you but all in all, this does look great!
gradle/libs.versions.toml
Outdated
@@ -3,7 +3,10 @@ | |||
agpVersion = "8.8.0" | |||
|
|||
compileSdk = "34" | |||
# Any version above 0.10.0-beta03 requires Compose 1.8.0 or higher. | |||
filekitDialogsComposeVersion = "0.10.0-beta03" |
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.
Let's stick with the same naming convention we have here. Android SDK versions get camel case but dependency versions use kebab-case. In that same vein, dependencies should be named without the version
suffix.
So this would be filekit-dialogs-compose
, Moshi would be moshi-kotlin
, and uiGraphicsAndroidVersion would be androidx-ui-graphics-android
.
This is also is roughly alphabetical order, with spacings denoting group separations for when we have many dependencies all related to the same thing, like androidx
, so this and the other new versions should be listed in that same order.
It's ultimately a small thing admittedly, but it's always good to be consistent 'cause then things tend to stay how you'd expect them to be when you come back later!
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
modifier: Modifier = Modifier | ||
) { | ||
Box { | ||
val selectedFile = remember { mutableStateOf<PlatformFile?>(null) } |
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.
Prefer using Kotlin delegation here.
var selectedFile by remember { mutableStateOf<PlatformFile?>(null) }
That'll let you do simple reassignments without having to do the whole .value
stuff to access or change the value. Android Studio will try to prompt you to import the getValue
and setValue
extension methods that make that possible, but if it doesn't, you may need to manually import them.
An example of this syntax is here: https://github.com/squareup/android-emulator-runner/blob/92d600d5e9cb215e3509ee215881e55a42fb6fac/desktop-app/src/jvmMain/kotlin/com/wardellbagby/aer/ui/App.kt#L192
The imports you may need to add manually are:
import androidx.compose.runtime.getValue
import androidx.compose.runtime.setValue
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
* All the caught exceptions should be handled by the caller, and appropriate UI feedback should be | ||
* provided to user. | ||
*/ | ||
public fun parseTrace( |
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.
JSON parsing can be long running. This would be better as a suspend fun
that wraps this work in a withContext(Dispatchers.IO)
block. Then, instead of having a jsonString
being used as state in your WorkflowContent
Composable, you'd have a nullable WorkflowNode
.
That would make WorkflowContent
look something like this:
@Composable
private fun WorkflowContent(file: PlatformFile?) {
var rootNode by remember { mutableStateOf<WorkflowNode?>(null) }
LaunchedEffect(file) {
if(file != null) {
rootNode = parseTrace(file?.readString()) // in the future, a try-catch here to switch to an error state!
}
}
if (rootNode != null) {
DrawWorkflowTree(root)
} else {
Text("Empty data or failed to parse data") // TODO: proper handling of error
}
}
For the future, when you get to the point where you might wanna show an error on parse failures, you'd instead just let this throw and expect it to be caught in the LaunchedEffect
you call this from so that the consumer could switch to some error state. But that's looking ahead so don't stress that now!
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.
Working on this in another branch!
} | ||
|
||
@Composable | ||
private fun WorkflowContent(file: PlatformFile?) { |
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.
Nit: No need for file
to be nullable here. I imagine it's because Kotlin thinks .value
can change but we know it won't so it's okay to use !!
here to force it to be non-null.
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
node: WorkflowNode, | ||
modifier: Modifier = Modifier, | ||
) { | ||
Column( |
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 works fine for now but eventually, we're probably gonna have to get lazy with it. I'm not familiar with it but I recently learned that Compose has a LazyLayout
Composable that, similar to LazyRow
and LazyColumn
, lets you render a bunch of stuff but is smart enough to only render things that are actually in view. Since these traces are gonna get big, we'll start to see lagging when scrolling. We might already with your workflow-300
trace!
I can probably bully one of the UI System folks for more info on how we could potentially use that here, since the documentation is really sparse and LazyLayout
is still experimental.
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.
workflow-300
seems to be fine when scrolling for now... but I will keep this in mind in the future once the traces get very large.
@steve-the-edwards Nah, the whole |
Given snapshots of all workflows, the visualizer shows a tree-like structure to allow developers to have an easier time of debugging through state changes and render passes
Current progress
WorkflowNode
, a basic tree structure can be drawn; these nodes can also be opened and closed upon clicking