[PATCH] Xwayland: fix output changes not getting applied in some, cases

[Simon Ser]

[Paging wayland-devel@ for the protocol discussion at the end of this
e-mail]

On Monday, April 22, 2019 3:21 PM, Ferdi265 <theferdi265 at gmail.com> wrote:
> On 22/04/2019 14:01, Roman Gilg wrote:
> > On Mon, Apr 22, 2019 at 1:30 PM Ferdi265 theferdi265 at gmail.com wrote:
> > > Hello,
> > > I investigated a bug I encountered while using the Sway Compositor and
> > > traced it back to some behaviour in Xwayland:
> > > The xdg_output and wl_output events are handled in such a way that
> > > output changes (e.g. position) sometimes don't get applied if only
> > > xdg_output is sent instead of both xdg_output and wl_output. (Sway does
> > > this)
> >
> > This issue was something we noted in KWin as well and fixed it
> > downstream by making sure the events get always sent together.
> > More info here: https://phabricator.kde.org/D19253
> > Would a downstream fix work for you as well? Not saying it can't be
> > fixed upstream in XWayland, but I haven't yet looked at your patch in
> > detail and I just remember the output code in Xwayland being somewhat
> > difficult, so maybe a downstream fix would be easier.
>
> The change to remove some information from the wl_output event and
> instead rely only on xdg_output in wlroots was deliberate as it
> simplifies the compositor code and might not even make much sense for
> some types of compositors (e.g. 3D desktop systems).
>
> The reason the events now don't fire together is that no information in
> wl_output actually changed, while the information in xdg_output did.
>
> the downstream discussion for this is here
> https://github.com/swaywm/wlroots/issues/1610
>
> I can't really speak for the whole wlroots / sway project, so I added
> Simon Ser aka "emersion" as CC, who is one of the main developers of
> sway and wlroots.
>
> Personally, I think Xwayland shouldn't attach too much meaning to when
> these events happen, and if they happen together. They are separate
> events after all, and should be treated this way.
>
> Assuming that this information is always updated at the same time or
> even re-sending it without a change (e.g. changing the output position
> in sway doesn't change any information in the wl_output event) just so
> the events happen together just sounds backwards to me.

I agree that Xwayland should be a good client and should handle
xdg_output.done sent even when wl_output.done isn't sent.

However I'm not sure this is the right fix: this patch breaks
atomicity. For instance the first time the output is advertised to
Xwayland, it will first update its internal state with the metadata
from wl_output and then take into account xdg-output and apply a second
update. This results in invalid intermediate state. I'm not sure how to
fix it, the only idea I can come up with is to have a timer to debounce
updates (yeah, it's not a good idea).

[Below is the part for wayland-devel@]

Wild idea: maybe xdg_output.done should never have existed, and
xdg_output should have used the wl_output.done event to atomically
submit state -- just like wl_surface.commit is used by add-on
interfaces like xdg_surface. This would more-or-less result in the
workaround Roman suggested on the server side, but it would be nice to
formalize it if we want to go down this road. Thoughts?