Add Static component for rendering permanent output #66
Conversation
mosaic-runtime/src/main/kotlin/com/jakewharton/mosaic/Static.kt
Outdated
Show resolved
Hide resolved
| if (other.width > 0 && other.height > 0) { | ||
| canvas.static.add(other) | ||
| } | ||
|
|
||
| // Propagate any static content of the display. | ||
| canvas.static.addAll(other.static) |
There was a problem hiding this comment.
This is my least favorite part of the change. The fact that a render causes a side-effect to collect these other renders and then something at the call site magically emits that first.
It's hard to argue with the output, but I think I would like to try and find something better for this. I'm not going to block merging here because the output is truly fantastic and it's all implementation detail.
Render (well, renderTo) is a tree walk, and I think perhaps we separate out the static tree walk to its own function which can be less reliant on side-effects + mutation. It also hopefully means the canvas isn't aware of these static renders.
There was a problem hiding this comment.
Yeah, this was definitely a result of not wanting to make a bunch of render things Static specific or introduce multiple render passes. Thankfully as you said it is all implementation detail so can definitely be iterated.
Render (well, renderTo) is a tree walk, and I think perhaps we separate out the static tree walk to its own function which can be less reliant on side-effects + mutation.
Are you thinking something like 2 renderTo passes, one for static and the other for "normal" content? Or find all the Static nodes in the render function and render those to the canvas first somehow? I can try a couple different things - if you want - but maybe a little pseudocode of what you are thinking might help.
It also hopefully means the canvas isn't aware of these static renders.
The tricky part here was that AnsiOutput needs to know the static and non-static parts so it can set the lastHeight to only the non-static part. So whatever gets sent to the output, needs to know that additional detail.
There was a problem hiding this comment.
I was thinking a whole separate function, like renderTo() and renderStaticTo() or something. Basically just splitting the responsibilities and each node would only realistically implement one of them.
| // Remove any items which have been rendered. | ||
| pending.removeIf { it.rendered } |
There was a problem hiding this comment.
This isn't super efficient but it's probably fine. One thing to note is that the trues are contiguous starting at index 0 so we could find the first false index and then do subList(0, firstFalse).clear() for an operation that is likely the most optimal.
There was a problem hiding this comment.
That's a good idea. I always forget that modifications on subList are reflected in the full list. Also worth noting that just pending.clear() worked in all of the test samples I created but I wasn't convinced that would work in all cases so I added the wrapper Item class to be safe.
|
Slamming it home. We can iterate as needed. Thanks for this! |
Resolves #65
Probably went a little overboard on the
jestsample updates so let me know if you want me to roll anything back. Also I'm not stuck on any naming, so if you have suggestions let me know.