[PATCH wayland-protocols] xdg-output: deprecate the xdg_output.done event

Sun Apr 28 09:19:29 UTC 2019

On Sunday, April 28, 2019 12:04 PM, Olivier Fourdan <ofourdan at redhat.com> wrote:
> Hi,
>
> On Sat, Apr 27, 2019 at 9:44 AM Simon Ser <contact at emersion.fr> wrote:
>
> > This commit makes it so a wl_output.done event is guaranteed to be sent with a
> > xdg_output.done event.
>
> Humm, I am not sure I like changing xdg-output to rely on another protocol
> event, it's looks like a weird mixup to me...

As noted below, this is idiomatic in the Wayland protocol. The
wl_surface.commit request applies xdg_surface's state.

> > This protocol change has been discussed in a recent xorg-devel discussions [1].
> >
> > First let's recap why a change is needed: Xwayland listens to both wl_output and
> > xdg_output changes. When an output's properties change, Xwayland expects to
> > receive both a wl_output.done event and an xdg_output.done event. If that's not
> > the case, Xwayland doesn't update its state (so old state is still exposed to
> > X11 clients).
> >
> > Most of the time, both objects will be updated at the same time (e.g. the
> > current mode is changed, so both wl_output.mode and xdg_output.logical_size are
> > sent) so this won't be an issue. However in some situations only one of
> > wl_output or xdg_output changes. For instance:
> >
> > - The mode is changed at the same time as the scale, resulting in the same
> >   logical_size.
>
> If the mode or scale changes, wl_output events should be sent. If the
> resulting xdg-output remains the same, that's fine, no xdg-output
> event would be sent and Xwayland should be fixed for that case.
>
> Same for rotation actually, I remember seing displays (Barco, iirc)
> used by ATM that were square (both physically and in resolution),
> applying a rotation to such an output device would not change the
> resulting xdg-output definition.
>
> > - The compositor doesn't expose the outputs' position via wl_output, so whenever
> >   the position changes only xdg_output is updated.
>
> That sounds like a bug in the compositor, wl_output is core protocol
> and the geometry event should be sent “[...] whenever any of the
> properties change.”

No, this is not a bug, this is deliberate. wlroots doesn't expose the
output positions to regular clients, because regular clients shouldn't
care about it. If they do, either it's a client bug, or either they
should use xdg-output.

GNOME also doesn't expose all output properties IIRC: the whole list of
output modes is not sent.

> > Both KDE [2] and wlroots [3] have experienced this issue.
>
> To me, that feels like changing the protocol definitions to please
> specific implementations  or work around bugs.

No, this proposal aims to add a way to atomically apply output
properties. Since output properties include both wl_output properties
and xdg_output properties, the proposal suggests to remove
xdg_output.done (since two atomic events can't be combined into one).

> > For this reason, I'd like to update the xdg-output protocol to make it mandatory
> > to always send a wl_output.done event after xdg_output changes. This effectively
> > makes wl_output.done atomically apply all output state (including the state of
> > add-on objects like xdg_output). This approach is pretty similar to
> > wl_surface.commit: this request will atomically apply surface state including
> > the state of e.g. the xdg_surface object tied to the wl_surface.
> >
> > To update the protocol to reflect this new requirement we can either:
> >
> > - **Bump xdg_output version**. The current protocol doesn't specify that
> >   wl_output.done must be sent, adding this new requirement would be a breaking
> >   change. We need to fix Xwayland for the current xdg_output version (maybe make
> >   it non-atomic for the current version, atomic for the new one?). Should we
> >   deprecate xdg_output.done in the new version?
> > - **Don't bump xdg_output version**. This clarifies what is expected in practice
> >   by Xwayland, a major xdg_output consumer, and what is currently implemented by
> >   all compositors.
> >
> > There's one issue with the "don't bump" approach: indeed in practice compositors
> > always send wl_output.done and xdg_output.done in pairs, however the ordering
> > between those two events is not guaranteed. This means some compositors might
> > send this sequence:
> >
> >     wl_output.geometry(…)
> >     wl_output.done()
> >     xdg_output.logical_position(…)
> >     xdg_output.done()
> >
> > In this case the wl_output.done event fails to atomically apply the xdg_output
> > state.
> >
> > For this reason, I think bumping the version is a better approach.
>
> I'd rather we try harder to fix the implementations. In the worse
> case, Xwayland would send two XRandr events instead of one, which
> IMHO is no big deal.

The Wayland protocol really tries hard to make properties atomic when
useful. IMHO it would be unfortunate to say "it's no big deal if it's
not atomic", especially when the intermediate state *is* invalid.

Additionally, these protocols are not only used by Xwayland -- other
clients (e.g. slurp, a desktop selection tool) will draw invalid frames
if output properties are not atomically applied.

> Cheers,
> Olivier
>
> > This commit also deprecates xdg_output.done, which doesn't have any purpose
> > anymore.
> >
> > [1]: https://lists.x.org/archives/xorg-devel/2019-April/058148.html
> > [2]: https://phabricator.kde.org/D19253
> > [3]: https://github.com/swaywm/sway/issues/4064
> >
> > Signed-off-by: Simon Ser <contact at emersion.fr>
> > ---
> >  unstable/xdg-output/xdg-output-unstable-v1.xml | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/unstable/xdg-output/xdg-output-unstable-v1.xml b/unstable/xdg-output/xdg-output-unstable-v1.xml
> > index ccbfe1c..01ac7a7 100644
> > --- a/unstable/xdg-output/xdg-output-unstable-v1.xml
> > +++ b/unstable/xdg-output/xdg-output-unstable-v1.xml
> > @@ -54,7 +54,7 @@
> >      reset.
> >    </description>
> >
> > -  <interface name="zxdg_output_manager_v1" version="2">
> > +  <interface name="zxdg_output_manager_v1" version="3">
> >      <description summary="manage xdg_output objects">
> >        A global factory interface for xdg_output objects.
> >      </description>
> > @@ -77,7 +77,7 @@
> >      </request>
> >    </interface>
> >
> > -  <interface name="zxdg_output_v1" version="2">
> > +  <interface name="zxdg_output_v1" version="3">
> >      <description summary="compositor logical output region">
> >        An xdg_output describes part of the compositor geometry.
> >
> > @@ -157,6 +157,10 @@
> >
> >         This allows changes to the xdg_output properties to be seen as
> >         atomic, even if they happen via multiple events.
> > +
> > +       For objects version 3 onwards, this event is deprecated. Compositors
> > +       are not required to send it anymore and must send wl_output.done
> > +       instead.
> >        </description>
> >      </event>
> >
> > --
> > 2.21.0
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel