Thoughts on configure/ack_configure flow in xdg-surface and layer-surface

[vyivel]

configure/ack_configure flow is currently used by two objects: xdg_surface and zwlr_layer_surface_v1. wlroots implements that flow differently for these objects and provides different APIs for them, which I believe is something that needs to be fixed.

---

xdg_surface configure events are delayed and aren't sent immediately. This allows for xdg_surface extensions/roles to be configured effectively, avoiding sending too many configure events and sending useless events. The current implementation of this mechanism is imperfect however, as it generates a serial in advance without knowing if that serial will actually be used.

18:41:13 <ifreund> the fact that we might decide to cancel the configure is still a nasty edge case though
18:41:30 <ifreund> because with the current code serials may not actually end up being used
18:42:05 <ifreund> however they are still returned to the compositor, which has no way to know the serial won't be used
...
18:50:56 <vyivel> let's say we call set_fullscreen(false) on a fullscreen view
18:51:19 <vyivel> that's a different state, so it schedules a configure and returns a newly created serial
18:51:40 <vyivel> then we call set_fullscreen(true) without dropping to the event loop
18:51:52 <vyivel> what should it return?
18:52:54 <ifreund> the serial that will be used for the next configure that actually gets sent I believe
18:53:50 <vyivel> assuming we don't configure anything after that, compositor would receive a serial that is never going to be used
18:54:37 <ifreund> it will be used next time the compositor causes any configure to be sent
18:55:02 <ifreund> but I agree this is probably not what the compositor author would expect

On top of that, 0 serial is returned wrongly as an indication of that a configure isn't necessary, even though 0 is a perfectly valid configure serial.

---

zwlr_layer_surface_v1 configure events are sent immediately on wlr_layer_surface_v1_configure(). This means that calling

wlr_layer_surface_v1_configure(new_size);
wlr_layer_surface_v1_configure(current_size);

will result in sending two configures, and, as the actual state didn't change, the client can ignore both of them.

Ref #3201 (comment)

---

For the purposes of unifying the API and avoiding code duplication, I propose introducing wlr_configurable to deal with this logic.

Draft:

struct wlr_configurable {
	struct wl_display *display;
	const struct wlr_configurable_impl *impl;

	struct wl_list sources; // wlr_configure_source::link
	bool force;
	struct wl_event_source *idle;
};

struct wlr_configurable_impl {
	void (*send_event)(struct wlr_configurable *configurable,
			uint32_t serial);
};

struct wlr_configure_source {
	struct wl_list link; // wlr_configurable::sources

	struct {
		// Sends a pointer to serial if must wait
		// for an ack, NULL otherwise
		struct wl_signal configure; // uint32_t *
	} events;
};

struct wlr_configure_source {
	void (*needs_configure)(struct wlr_configure_source *source);
};

static void send_configure(void *data) {
	struct wlr_configurable *configurable = data;
	configurable->idle = NULL;

	bool needs_event = false;
	uint32_t serial;

	if (configurable->force) {
		serial = wl_display_next_serial(configurable->display);
		needs_event = true;
		configurable->force = false;
	}

	struct wlr_configure_source *source, *tmp;
	wl_list_for_each_safe (source, tmp, &configurable->sources, link) {
		if (source->impl->needs_configure(source)) {
			if (!needs_event) {
				serial = wl_display_next_serial(configurable->display);
				needs_event = true;
			}
			// `serial` could be wrapped in a struct here
			wlr_signal_emit_safe(&source->events.configure, &serial);
		} else {
			wlr_signal_emit_safe(&source->events.configure, NULL);
		}
		wl_list_remove(&source->link);
		wl_list_init(&source->link);
	}

	if (needs_event) {
		configurable->impl->send_event(configurable, serial);
	}
}

void wlr_configurable_schedule(struct wlr_configurable *configurable,
		struct wlr_configure_source *source) {
	if (configurable->idle == NULL) {
		struct wl_event_loop *loop =
			wl_display_get_event_loop(configurable->display);
		configurable->idle = wl_event_loop_add_idle(loop,
				send_configure, configurable);
	}
	if (source) {
		wl_list_remove(&source->link);
		wl_list_insert(&configurable->sources, &source->link);
	} else {
		// Scheduling a configure without a source is always
		// successful, can be used to e.g. respond to set_maximized
		configurable->force = true;
	}
}

// Example wlr usage

struct wlr_xdg_surface {
	/* ... */
	struct wlr_configurable configurable;
};

// wlr_xdg_surface.configurable.impl.send_event
static void xdg_surface_configurable_send_event(
		struct wlr_configurable *configurable, uint32_t serial) {
	struct wlr_xdg_surface *xdg_surface = wl_container_of(
			configurable, xdg_surface, configurable);
	xdg_surface_send_configure(xdg_surface->resource, serial);
}

struct wlr_xdg_toplevel {
	/* ... */
	struct wlr_configure_source configure_source;

	// Listens to wlr_xdg_toplevel.configure_source.events.configure
	struct wl_listener configure;
};

// wlr_xdg_toplevel.configure_source.impl.needs_configure
static void xdg_toplevel_needs_configure(
		struct wlr_configure_source *source) {
	struct wlr_xdg_toplevel *toplevel = wl_container_of(
			source, toplevel, configure_source);
	// Compare scheduled and last sent states here
	return xdg_toplevel_state_changed(toplevel);
}

// wlr_xdg_toplevel.configure.notify
static void xdg_toplevel_configure_source_handle_configure(
		struct wl_listener *listener, void *data) {
	struct wlr_xdg_toplevel *toplevel = wl_container_of(
		listener, toplevel, configure);
	if (data) {
		uint32_t serial = *(uint32_t *)data;
		// Allocate a new wlr_xdg_toplevel_configure here,
		// set its serial to `serial` and wait
		xdg_toplevel_configure(toplevel->resource, width, height, states);
	} else {
		// A configure might or might not be sent, but
		// xdg-toplevel state didn't change, so we do nothing
	}
}

void wlr_xdg_toplevel_set_maximized(
		struct wlr_xdg_toplevel *toplevel, bool maximized) {
	toplevel->scheduled.maximized = maximized;
	// Could be wrapped in wlr_xdg_toplevel_schedule_configure()
	wlr_configurable_schedule(&toplevel->surface->configurable,
		&toplevel->configure_source);
}

Acking a configure is done by the wlr_configure_source implementation by listening to its ack_configure signal, which could be moved to wlr_configurable.

---

A compositor wishing to implement atomic layout updates would do the following:

1. Schedule configures for all modified configure sources, mark them as "scheduled to configure"
2. Wait for wlr_configurable.idle to be handled
3. Listen to wlr_configure_source.events.configure
4. If a configure source gets NULL serial pointer, mark as "ready" immediately
5. Otherwise mark as "configuring"
6. Wait for configures to be acked and committed, mark as "ready"
7. When all configure sources are "ready", apply the transaction

---

cc @ifreund

---

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

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

[ifreund]

IRC logs of discussion on this issue for those without an IRC presence who are interested:

15:13 <ifreund> vyivel: I feel like extracting the configure code to a new wlr_configureable thing adds more complexity than it is worth. Since emersion reminded me of #2330 I think we should take the approach of *always* scheduling a configure if the compositor does any set_fullscreen/size function call and just use the idle event source for batching
15:13 <ifreund> i.e. get rid of the logic that prevents noop configures from being sent
15:09 <emersion> yeah that makes sense to me
15:14 <ifreund> this would also resolve our woes with schedule_configure() returning a serial that doesn't make sense
15:10 <vyivel> and the compositor is responsible for checking if the state has changed
15:11 <vyivel> well, that's sure is easier to implement
15:15 <ifreund> I don't think the compositor needs to check if the state has changed in general, only when doing special things like atomic transactions
15:15 <ifreund> I'm coming to thing that a few extra configure events that don't change any state being sent to clients is fine
15:16 <ifreund> and all the attempts to avoid that cause more issues than they are worth
15:16 <ifreund> s/thing/think/
15:14 <emersion> yeah
15:14 <vyivel> understandable
15:14 <vyivel> but it still would be good to make layer surface configures be scheduled as well
15:14 <vyivel> for consistency™
15:18 <ifreund> sure, that's fine with me
15:19 <ifreund> if you plan on implementing this, would you mind fixing schedule_configure() to check if allocation of the wl_event_source failed?
15:15 <emersion> is there anything to batch for layer surfaces?
15:19 <ifreund> we could handle that by skipping the batching
15:19 <ifreund> emersion: only if you set size twice before yielding to the event loop
15:20 <ifreund> maybe that's not worth it
15:16 <vyivel> eh, skipping the batching doesn't sound good to me
15:17 <vyivel> i'd rather send an oom error
15:20 <ifreund> ok yeah we can also just kill the client

[vyivel]

λ ~/opt/wlroots grep "// First find the ack'ed configure" types -R | wc -l
3

:P

[vyivel]

A simpler proposal:

struct wlr_configure {
	uint32_t serial;
	struct wlr_addon_set addons;
	struct wl_list link; // wlr_configurable::configures
};

struct wlr_configurable_interface {
	void (*send_configure)(struct wlr_configurable *conf, uint32_t serial);
};

struct wlr_configurable {
	struct wl_display *display; // for scheduling
	const struct wlr_configurable_interface *impl;

	struct wl_list configures; // wlr_configure::link
	struct wl_event_source *idle;
	uint32_t next_serial;
	
	struct {
		struct wl_signal configure; // wlr_configure *
		struct wl_signal ack_configure; // wlr_configure *
	} events;
};

// Generates a new serial and schedules allocating a new wlr_configure,
// firing wlr_configurable.events.configure and calling
// wlr_configurable_interface.send_configure()
uint32_t wlr_configurable_schedule_configure(struct wlr_configurable *conf);

// Pops a configure with specified serial,
// fires wlr_configurable.events.ack_configure and destroys configure
void wlr_configurable_ack_configure(struct wlr_configurable *conf, uint32_t serial);

Example usage:

// wlr_xdg_surface.c

struct wlr_xdg_surface {
	/* ... */
	struct wlr_configurable conf;
};

// wlr_xdg_surface.conf.impl.send_configure()
static void xdg_surface_configurable_send_configure(
		struct wlr_configurable *conf, uint32_t serial) {
	struct wlr_xdg_surface *surface = wl_container_of(conf, surface, conf);
	xdg_surface_send_configure(surface->resource, serial);
}

static void xdg_surface_handle_ack_configure(struct wl_client *client,
		struct wl_resource *resource, uint32_t serial) {
	struct wlr_xdg_surface *surface = wlr_xdg_surface_from_resource(resource);
	wlr_configurable_ack_configure(&surface->conf, serial);
}

// wlr_xdg_toplevel.c

struct wlr_xdg_toplevel_configure {
	/* ... */
	struct wlr_addon addon; // wlr_configure.addons
};

uint32_t wlr_xdg_toplevel_set_maximized(struct wlr_xdg_surface *surface,
		bool maximized) {
	assert(surface->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL);
	surface->toplevel->scheduled.maximized = maximized;
	return wlr_configurable_schedule_configure(&surface->conf);
}

static void xdg_surface_configurable_handle_configure(
		struct wl_listener *listener, void *data) {
	struct wlr_xdg_surface *surface =
		wl_container_of(listener, surface, conf.events.configure);
	struct wlr_configure *configure = data;
	// Create a wlr_xdg_toplevel_configure, add to configure.addons,
	// call xdg_toplevel_send_configure()
	/* ... */
}

static void xdg_surface_configurable_handle_ack_configure(
		struct wl_listener *listener, void *data) {
	struct wlr_xdg_surface *surface =
		wl_container_of(listener, surface, conf.events.ack_configure);
	struct wlr_configure *configure = data;
	struct wlr_addon *addon = wlr_addon_find(&configure->addons, surface,
		&xdg_toplevel_configure_addon_impl);
	if (!addon) {
		return;
	}
	// Get wlr_xdg_toplevel_configure from addon and apply it
	/* ... */
}

wlr_configurable keeps track of sent configures, creates and destroys them, and is responsible for scheduling sending configure event. This would avoid having 3(!) implementations of the exactly same logic.