Send wl_surface_enter when wl_output is bound late

[kennylevinsen]

If a client has not bound a wl_output at the time where the compositor calls wlr_surface_send_enter, then no wl_surface_enter event is sent.

As the compositor is unaware of this, and no logic is present to retry when a wl_output object is finally bound by the client, this leaves the client unaware of its current output association.

A solution to this would be to have a wlr_surface_(enter|leave)_output API, where instead of blindly sending an event, we'd instead add the output to a list on the surface, allowing us to send late enter events, and possibly also to send leave events when an output disappears.

Application issue stemming from this: https://codeberg.org/dnkl/foot/issues/182

[emersion]

Lazy IRC log dump of a discussion about this

22:50 <kennylevinsen> Hmm, wlroots is discarding the wl_surface::enter event sway is trying to send
22:51 <kennylevinsen> wlr_surface_send_enter looks for a wl_output resource for the client, finds none, and does nothing.
22:52 <emersion> eh, that's normal then
22:52 <kennylevinsen> sway moves on with its life thinking all went well of course, and thus the client is never notified of having entered an output despite being visible on one.
22:52 <emersion> the client needs to bind to the output, otherwise wlroots can't send any event
22:52 <emersion> (having no wl_output object to tie the event to)
22:53 <emersion> what we could do though is send wl_output.enter when the client binds to a wl_output
22:53 <emersion> i've been thining about a better way to handle "entered" outputs in wlroots
22:54 <emersion> iirc weston just has a per-surface list of entered outputs
22:57 <emersion> right, weston_view.output_mask
22:59 <kennylevinsen> it's normal that there's no object, but for newly appearing outputs we seem to be racing with the client, and never trying again if it fails
23:00 <kennylevinsen> so yeah - we'd need to send output enter once the client binds it then
23:07 <emersion> i wonder how (if?) we can nicely implement this in wlroots 
23:07 <emersion> wlr_surface and wlr_output are completely separate
23:09 <emersion> we need to somehow store a ref to the outputs entered in the surfaces (be it a list of a mask), but have to be careful about correctly handling output destroy
23:09 <emersion> *be it a list or a mask
23:11 <emersion> the naive approach would be to have a linked list of (wlr_output, output destroy listener) in wlr_surface, but that's not too great
23:12 <emersion> the weston approach with a bitfield would be tricky for wlroots, because even if we had "output indices", we'd somehow still need to cleanup bits on output destroy
23:14 <emersion> maybe some of this bookkeeping could be handled by wlr_compositor
23:14 <ammen99_> emersion, what is the issue of having a wl_list of outputs in the surface?
23:15 <kennylevinsen> if we have a wlr_surface_enter_output(struct wlr_surface *, struct wlr_output *), we could just have a linked list and listen for output destroy
23:15 <emersion> well… my perfectionist side says "it's not efficient at all"
23:15 <ammen99_> yeah ^
23:15 <emersion> but… yeah i'm probably overthinking it
23:16 <emersion> on  wlr_surface_enter_output it means you need to iterate
23:16 <kennylevinsen> well, main thing that annoys me there is that both compositor and library is keeping track of similar state (what surfaces on what output), but exposing "client bound global XYZ" to the compositor seems too low level
23:16 <emersion> you have n_outputs*n_surfaces extra allocs
23:17 <kennylevinsen> what iteration are you thinking of?
23:17 <emersion> yeah i'd prefer all of this to be handled by wlroots
23:17 <emersion> on wlr_surface_enter_output, don't add the output to the list if already there
23:18 <emersion> on wlr_surface_leave_output, you need to iterate too
23:18 <ammen99_> I think the list would contain like at most 2 entries on average
23:18 <ammen99_> and will be called quite rarely
23:18 <kennylevinsen> we can't say "if you use wlr_surface_(enter|leave)_output, don't do stupid things?" :/
23:18 <emersion> ideally i'd like to make it possible for compositors to call it very often
23:18 <kennylevinsen> right
23:18 <emersion> like each frame or something
23:19 <ammen99_> why would you need that?
23:19 <emersion> because you don't need to to complicated things in the compositor anymore
23:19 <emersion> right now compositors need to do the work of figuring out when a surface changes its output
23:20 <emersion> if you could "mark" the surface when rendering it on an output
23:20 <emersion> and convince wlroots that surfaces not "marked" are no longer on the output
23:20 <emersion> that would reduce complexity in the compositor
23:21 <emersion> figuring out when a view changes output is easy enough, doing it for all surfaces (incl. sub-surfaces, popups, etc) is more involved
23:21 <kennylevinsen> right, so it's not that you think it makes things more complicated, just using the opportunity to also simplify api - gotcha
23:22 <emersion> yeah… i'm not sure how it can work out in practice
23:22 <emersion> maybe it can't work well, and we need to maintain the status quo
23:22 <emersion> i don't know how you detect output changes for surfaces in wayfire, ammen99_?
23:23 <emersion> sway has it easy for tiled views :P
23:23 <emersion> i think sway doesn't get it right when a surface with sub-surfaces spans over different outputs
23:26 <kennylevinsen> I suppose we should add such silly example to wleird
23:27 <emersion> eh, not a bad idea
23:27 <emersion> a client with multiple surfaces printing on which outputs a surface is according to enter/leave
23:36 <kennylevinsen> btw, we could reduce it to N_surfaces mallocs by just having a malloc'd array of output pointers instead of using a linked list with malloc'd element wrappers.
23:37 <emersion> kennylevinsen: i though of that, but (1) we need listeners (2) realloc wouldn't play well with that, or would it?
23:39 <kennylevinsen> what, why wouldn't realloc play well with a normal allocation sized for N_outputs pointers?
23:43 <kennylevinsen> oh wait I get what you mean
23:43 <kennylevinsen> Yeah, all the links would have to get updated after realloc
23:44 <kennylevinsen> Wait, no, I confused myself for a moment. As long as it's plain wlr_output pointers, no problem. If have have linked list elements on it, that'll be an issue.
23:44 <emersion> wl_listener has a link field
23:45 <kennylevinsen> yeah, so that won't work for a list of listeners
23:45 <emersion> yup. so we can track wlr_outputs in an array, but can't track when they are destroyed
23:46 <emersion> anyways, the internal details would probably depend on the actual API
23:47 <emersion> as ammen99_ said, if we keep the current status quo, no need for anything more complicated than a wl_list per surface, i gues
23:47 <emersion> s
23:48 <kennylevinsen> no, not unless we're trying to minimize heap allocations
23:49 <kennylevinsen> but maybe that's micro-optimization - let's not get into weird things with embedding first N entries and spilling over to heap...
23:51 <emersion> ahah
23:51  * emersion was just thinking about something like this
23:53 <kennylevinsen> I mean, I'm open to it, but we should be be sure if we want the complexity.
23:54 <kennylevinsen> it just makes code a lot uglier
23:55 <emersion> oh, yeah, we should definitely not do it
23:55 <emersion> or only do it if we really run into perf issues