Skip to content

Conversation

@Keitio
Copy link

@Keitio Keitio commented Jul 4, 2019

Definitely don't merge this.
This is an attempt of mine at creating a layout system for this GUI system.
This is largely uncommented code, with some bits taken from the examples.
My take is that the layouts take an Env as parameter, and give back a map of string to Env.
The map keys are like "tags" for the sub Envs.
You could have a Scroller with "body", "header" and "footer" areas for example.
The Layouts are then basically acting as sophisticated Muxes.

If this idea takes on, some refactoring is needed because a lot of code for Layout is pasted from Mux.

@faiface
Copy link
Owner

faiface commented Jul 7, 2019

Hey, thanks for this! This is definitely a step in the right direction, even if not polished. Sorry for not responding earlier.

If you wish to finish this into something that could be merged, I'll gladly provide code reviews.

The idea of returning a map is interesting. What would be the advantages of this over simply returning a slice of Envs?

@Keitio
Copy link
Author

Keitio commented Jul 7, 2019

Actually it was a slice of Envs originally, but it was a mess to manage.
For example, how do you address a grid ? you would need to remember the row length to address it.
Documentation can also be clearer this way, rather than saying "1 is the main body".
I'm working on a second revision with a Zone interface, which is just a String() string, used as map keys, which would make it even clearer.
I'll try to push it tomorrow or the day after that.

@faiface
Copy link
Owner

faiface commented Jul 7, 2019

Actually it was a slice of Envs originally, but it was a mess to manage. For example, how do you address a grid ?

Good point, but how often do you need to address it? Don't you just create elements and let it go? I'm not arguing, just wanna get more info on this :D

@Keitio
Copy link
Author

Keitio commented Jul 7, 2019

You're right, we only address them once, but it's mainly for clarity purposes.
For simple layouts, like a grid or a list, a slice is perfectly fine. But if you take a more complex layout, like a Card, using title and image is way clearer than 1 and 0 for me. This way you can know what your code is doing at a glance.

@faiface
Copy link
Owner

faiface commented Jul 7, 2019

What about something like this?

var left, middle, right gui.Env
hsplit := layout.NewHSplit(env, &left, &middle, &right)

And this

var (
    topLeft, top, topRight          gui.Env
    left, middle, right             gui.Env
    bottomLeft, bottom, bottomRight gui.Env
)

grid := layout.NewGrid(
    env,
    []*gui.Env{
    	{&topLeft, &top, &topRight},
    	{&left, &middle, &right},
    	{&bottomLeft, &bottom, &bottomRight},
    },
)

EDIT:

And this:

var title, image gui.Env
card := layout.NewCard(env, &layout.CardConfig{
    Title: &title,
    Image: &image,
})

@Keitio
Copy link
Author

Keitio commented Jul 7, 2019

This is a great alternative i hadn't thought about, and it would make a lot of things easier.
I'll implement that instead, thanks.

@Keitio
Copy link
Author

Keitio commented Jul 9, 2019

Done, i also introduced a Box layout and a true layout.Mux.
I think there is an abstraction to be found for merging gui.Mux and layout.Mux, and this still needs some documenting, but otherwise i think it's quite good for a start.
I'm also unsure as how to do options for layouts correctly, as there is quite some boilerplate and typing involved.

@faiface
Copy link
Owner

faiface commented Jul 10, 2019

Great idea with the Layout interface!

I am quite busy right now (and gonna be tomorrow as well), but a few quick thoughts:

  1. It would make sense to make a SplitFunc type for splitting functions.
  2. I think the SplitFunc would be more flexible if it accepted not just a width, but minX and maxX. Otherwise you need to modify all returned rectangles.
  3. The usage of the Layout interface is kinda obfuscated right now. For example, NewGrid return gui.Env, but uses NewMux to do so, where NewMux accepts a Layout, which a *Grid is so it passes itself in there. How about NewGrid just returned a Layout? Then it would be quite easier to understand.
  4. Types like *Grid and *Box probably don't have to be exported.
  5. Yeah, I agree that the functional options make things a little verbose here. If the layout only accepts a few parameters (especially if they are distinguished by type), it would make sense to simply accept them as parameters.

Thanks for the great work so far, btw :)

@Keitio
Copy link
Author

Keitio commented Jul 11, 2019

Thanks !
I'll create the SplitFunc type, as it will be more understandable and documentable this way.
For its arguments, I think it's fine as-is, because you need to create Rectangles from ints anyway, so having to do less work function side is a win by me.
For the NewGrid() using an layout.Mux internally, unfortunately I kind of need to do either that or add some kind of Init() func to the Layout interface. But I agree, it's very obfuscated right now.
And you're right, I'll change the layouts to be unexported, exporting them only made sense when there was no constructor.

@Keitio
Copy link
Author

Keitio commented Jul 11, 2019

I was also thinking of splitting the Layout interface with a Redrawer interface, that could be used for a widgets package in the future. This Redrawer would be in the gui package.
Would that make any sense to you ?

@Keitio
Copy link
Author

Keitio commented Jul 12, 2019

I refactored quite a bit and added an Items() []*gui.Env func to the Layout interface, this way this is less obfuscated and feels less like magic.

@faiface
Copy link
Owner

faiface commented Jul 14, 2019

Hm, I'm not sure I like the Items() method, it seems like a hack. I'll have to think more about this. If you come up with new ideas, please share!

I finally have more time again, so I'll be able to devote more time to this. I'll share new ideas as soon as I get them.

@Keitio
Copy link
Author

Keitio commented Jul 15, 2019

I agree, it feels like a hack, but it felt less like a hack than some Init() func.
It was more for lack of a better idea.
I think we could add a parameter to NewMux, like that:

layout.NewMux(
	mux.MakeEnv(),
	[]*gui.Env{&top, &left, &right}, // !
	layout.NewGrid(
		[][]*gui.Env{
			{&top},
			{&left, &right},
		},
	),
)

And voilà, no need for magic.
It feels repetitive, but nothing unbearable, especially if we provide helpers.
Does this feel alright with you ?

There is still a problem though, because it's not really tied to the Layout.
We would need to provide the Envs in the same order that the Lay() will give them, so we need to enforce some kind of ordering.

EDIT:
we could even

layout.NewMux(
	mux.MakeEnv(),
	[]*gui.Env{&top, &left, &right},
	layout.NewGrid(
		[]int{ 1, 2 }, // for row length
	),
)

because the Envs values are not used inside the Layouts, and it would make the Layouts independent from gui.

@Keitio
Copy link
Author

Keitio commented Jul 15, 2019

Well after implementing it it works really well, there's just something strange I can't quite put my finger on.
Also I changed the initialization from functional options to struct literals with sane defaults.
It's better documentable this way, and less contrived for simple cases.

@Keitio
Copy link
Author

Keitio commented Jul 16, 2019

I found two major problems with this approach:

  • a Layout can't ask for a childs redraw by itself
  • a Layout can't intercept Events

The first thing that would come to mind is making the Layout some kind of Event middleware, with a Transform(ev <-chan Event) <-chan Event {} func.
It would then be able to re-emit the last resize event for childs redrawing, and use Events for itself, for example some win.MoScroll for a scroll layout.

But if some better idea comes out, I'll take it for sure.

@Keitio
Copy link
Author

Keitio commented Aug 7, 2019

I didn't get much time on this project lately, but here it is, mostly done and usable.
If it gets merged into master, it should definitely be marked as experimental and subject to change (well, even more than the rest).
The only major problem I found is a performance one on the Scroller layout, but it works now.

@Keitio Keitio marked this pull request as ready for review August 7, 2019 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants