Make wlr_output_layout_output private

[acrisci]

The wlr_output_layout_output was designed to be private to the wlr_output_layout. There's no reason to expose it to user code.

Looking through the rootston code, it looks like the only use for it is to get the layout x/y coordinates of the output in the layout. This can be accomplished much more cleanly with wlr_output_layout_get_box() now.

Is there any use case for exposing the wlr_output_layout_output to user code?

---

wlroots has migrated to gitlab.freedesktop.org. This issue has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/1013

[VincentVanlaer]

The reason why I would expose it, is to make tweaking the behavior of the wlr_output_layout to the library users needs. For example, in my relative layout PR (#1002), rootston uses these and the way they are arranged in the output list to do less bookkeeping. Mostly it allows rootston to reuse the 'child follows parent immediately'-ordering of the output list. Note that apart from the configuration and the wlr_output, none of the members of the wlr_output_layout_output are used. and it wold even be possible to hide the configuration and output behind a getter.

While it would certainly be possible to do all of this without the wlr_output_layout_output, it would make me start to wonder why I'm using the wlr_output_layout. I would copy the whole thing, adapt it to my compositors needs, than interfacing with it.

I'm certainly not against making parts of the wlr_output_layout_output internal, but I think that keeping parts of it around would help

[Timidger]

This seems like a good idea, since right now that's all the output layout output is doing.

On the other hand @VincentVanlaer has a good point about the extensibility of keeping it around. Though those extensions you have proposed @VincentVanlaer doesn't need the wkr_output_layout_output to be exposed does it? It could work just as well as with wlr_box.

I have no strong feelings either way, but the API surface of wlroots is huge and unless we have good reason to we shouldn't be afraid to make things simpler (since we can always make it more complicated later)

[VincentVanlaer]

In the more exposed case, I see two possible interfaces. The first one would be to hide most of the specifics of the wlr_output_layout_output in the state, but still keep the structure around. This allows the library user to iterate over the outputs in the layout with the normal wl_list functions. Having an explicit ordering in the API will enforce it in the implementation though.

struct wlr_output_layout_state;

struct wlr_output_layout {
	struct wl_list outputs;
	struct wlr_output_layout_state *state;

	struct {
		struct wl_signal add;
		struct wl_signal change;
		struct wl_signal destroy;
	} events;

	void *data;
};

struct wlr_output_layout_output {
	struct wl_list link;
	struct wlr_output_layout_output_state *state;

	struct {
		struct wl_signal destroy;
	} events;

	void *data;
};

...

struct wlr_output_layout_output *wlr_output_layout_get(
		struct wlr_output_layout *layout, struct wlr_output *reference);

struct wlr_box *wlr_output_layout_get_box(struct wlr_output_layout *layout,
		struct wlr_output_layout_output *reference);

...

In case we don't explicitly want to expose the list of wlr_output_layout_output and completely hide the struct, we could also have an iterator. This iterator can have a specific ordering attached to it, but this doesn't need to be reflected in the underlying implementation anymore.

struct wlr_output_layout_state;

struct wlr_output_layout {
	struct wlr_output_layout_state *state;

	struct {
		struct wl_signal add;
		struct wl_signal change;
		struct wl_signal destroy;
	} events;

	void *data;
};

struct wlr_output_layout_output;

...

struct wlr_output_layout_output *wlr_output_layout_get(
		struct wlr_output_layout *layout, struct wlr_output *reference);

struct wlr_output_layout_output *wlr_output_layout_iterate(
		struct wlr_output_layout *layout,
		struct wlr_output_layout_output *l_output);

struct wlr_box *wlr_output_layout_get_box(struct wlr_output_layout *layout,
		struct wlr_output_layout_output *reference);

...

I might also be worth noting that wlr_cursor currently uses the destroy event in wlr_output_layout_output.

[VincentVanlaer]

After some discussion on IRC and #550 (comment) I'm no longer against this.

[acrisci]

Ah ok, not having the ability to iterate over a list of outputs in the layout isn't good and I don't think I like the iterator approach. I'm starting to reconsider this.

[Timidger]

Not a fan of that iterator approach either, it's a little weird. Plus I'm not sure how well it can be wrapped in wlroots-rs (especially if there needs to be clean up? Never dealt with FFI and the iterator traits before).

[acrisci]

Yeah, I think we need it as a list structure. But I think it would be better if that's all it did.

Right now, it gets emitted on the layout add event. I would like to change the type that emits to a plain wlr_output and add a remove event that people can use instead of the OLO destroy event. Also I would like to make wlr_output_layout_get() private.

[emersion]

What libwayland does to handle this kind of issue is defining wlr_output_layout_output as an opaque type, adding struct wl_list *wlr_output_layout_output_get_link() and defining a wlr_output_layout_output_for_each macro.