oyvindberg / tui-scala

Beautiful Text-based User Interfaces for Scala
MIT License
207 stars 11 forks source link

Make `Layout` a widget (fixes #41) #50

Open oyvindberg opened 1 year ago

oyvindberg commented 1 year ago

Massively improved DX aside, this is the first step towards more react-like components in tui.

Next steps I'll try:

oyvindberg commented 1 year ago

other things:

oyvindberg commented 1 year ago

Thoughts on this @ckipp01 ?

ckipp01 commented 1 year ago

Thoughts on this @ckipp01 ? Ah nice! I'll see if I can get some time later today to check this out, if not it might first be tomorrow.

ckipp01 commented 1 year ago

So in general, I really like this. The swapping of the block widget to have children makes a ton of sense to me, and just in general the layout widget is much more intuitive and ergonomic. Apart from that it's hard to really tell since there are quite a few changes here. Do you foresee doing much more in this pr? If no, I think I'll try porting skan over to this, and I think that will give me the best feeling of it.

oyvindberg commented 1 year ago

Thanks! Those were the only "real" changes in here, and I'm happy you agree it'll be an improvement.

I'll leave the PR at this, modulo some cleanups, and explore further in followup PRs.

oyvindberg commented 1 year ago

Hey @ckipp01 - Thanks a lot for the comments! You pin-point some very real issues here that we need to solve! I'll just do a small brain-dump on the topic :)

Let's talk about render first

This tripped me up too while updating the examples, I think I broke 2 of them by forgetting a render where there must be one. So the proposed solution here is clearly not optimal in that sense.

Short-term solution

We can do a bit less in this PR and demand .render() everywhere for now. at least that way it's consistent.

A bit longer-term solution

What I have in mind which could solve this is a TuiElement, an analogue to ReactElement in react if you're familiar. Basically it's the result of applying a component, which is then passed back to the rest of react and vdom. Queue magnet pattern with a few implicit conversions from Widget, maybe Text or whatever else we want to render and that can probably work well. Maybe this TuiElement type can just be Widget even (but with a new name in that case)

The main problem with this solution as I see it is that there are two different use cases: let's call them low-level and high-level. All the current widgets are low-level, they imperatively paint to particular coordinates. I think what I want is to separate out high-level "widgets" where you don't care about coordinates. You specify a layout, and the runtime will position things for you. Since low-level widgets are imperative they should return Unit, while high-level widgets should return TuiElement or similar. My hope is that a separation like this could hide complexity from the user if we supply enough low-level widgets that you don't typically need to drop down to that level. Let's see how it pans out though :)

> So I'm assuming the main reason you actually do the Widget here is so that you can get access to the area for the x and y for the cursor?

Yes. It was my hypothesis that it was not necessary to know the area at that level. I still think it may be right in fact, if we add more low-level widgets. In the case of UserInputExample that would be a SetCursor widget which sets the location relative to the surrounding area.

Short-term solution

Make Layout.detailed accept a sequence of Constraint -> (Rect, Buffer) => Widget/Unit instead, so we have the same expressivity. So there would be three levels of more and more complex types depending on which Layout constructor function you (must) use.

Longer-term solution

Another idea that I had was to thread this info through a Context Function BufferSizeStateAndWhateverElse ?=> TuiElement. We could call that Context and buy ourselves a neat way to thread through state, maybe do some event things.


So as you see there is a lot of unfinished thoughts, and a lot of fun experiments which can be done here. Followup with any comments you may have as they come to you :)

Unless you have other ideas I'll probably update the PR to the stated short-term solutions over the next week sometime.

ckipp01 commented 1 year ago

Let's talk about render first

This tripped me up too while updating the examples, I think I broke 2 of them by forgetting a render where there must be one. So the proposed solution here is clearly not optimal in that sense.

Short-term solution

We can do a bit less in this PR and demand .render() everywhere for now. at least that way it's consistent.

🤔 yea that's probably best to just avoid confusion, although I like that you don't need to call render in most places. It feels natural that if you call render on a Layout that you wouldn't need to on the individual widgets. I'm open to both here.

A bit longer-term solution

What I have in mind which could solve this is a TuiElement, an analogue to ReactElement in react if you're familiar. Basically it's the result of applying a component, which is then passed back to the rest of react and vdom. Queue magnet pattern with a few implicit conversions from Widget, maybe Text or whatever else we want to render and that can probably work well. Maybe this TuiElement type can just be Widget even (but with a new name in that case)

The main problem with this solution as I see it is that there are two different use cases: let's call them low-level and high-level. All the current widgets are low-level, they imperatively paint to particular coordinates. I think what I want is to separate out high-level "widgets" where you don't care about coordinates. You specify a layout, and the runtime will position things for you. Since low-level widgets are imperative they should return Unit, while high-level widgets should return TuiElement or similar. My hope is that a separation like this could hide complexity from the user if we supply enough low-level widgets that you don't typically need to drop down to that level. Let's see how it pans out though :)

I think this make sense, but also a bit hard for me to imagine as I'm not super familiar with React.

Short-term solution

Make Layout.detailed accept a sequence of Constraint -> (Rect, Buffer) => Widget/Unit instead, so we have the same expressivity. So there would be three levels of more and more complex types depending on which Layout constructor function you (must) use.

Yup, I like this idea.

Unless you have other ideas I'll probably update the PR to the stated short-term solutions over the next week sometime.

No I think what you have make sense here. Overall they are all positive changes in my opinion, so it provides a nice base to just continually improve the warts.