[PATCH wayland] protocol: Add a surface_has_existing_role error

[Pekka Paalanen]

On Mon, 25 Aug 2014 08:59:51 +0300
Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Fri, 22 Aug 2014 10:48:02 -0700
> Jason Ekstrand <jason at jlekstrand.net> wrote:
> 
> > As I mentioned on IRC, I don't really like adding and using an enum value
> > without bumping protocol version.  However, the only time we ever use it is
> > to kill the client so the worst thing that can happen is that the client
> > doesn't report the error correctly.  I think I'm ok with it.
> 
> Yeah, I don't see a problem in adding values to an error enum.
> 
> However, I do wonder if this is logical.
> 
> Let's say xdg_shell used this. A client issues request
> xdg_shell at 33.get_xdg_surface(wl_surface at 20). There are quite likely
> several other requests on wl_surface at 20, and since I assume
> get_xdg_surface will be raising a protocol error, there will also be an
> earlier request on some other interface already giving a role to
> wl_surface at 20. And all these requests can be in the same burst.
> 
> Using the below enum value, the compositor would send the error on
> wl_surface at 20, but you don't know which interface call caused it.
> 
> Using an error enum value in xdg_shell, the compositor would be sending
> the error on xdg_shell at 33, but you don't know for which surface.
> 
> One solution to both "don't know"s is to add the missing information to
> the error message for humans to see.
> 
> Both ways could work in practice, but this would be the first (legal)
> case of sending the error on a different object than whose request
> caused it to trigger.
> 
> It may make sense in this one case, but if we do it here, we generally
> allow it everywhere, which I believe can bring confusion on what errors
> can be raised where. It will no longer be true, that the possible
> errors are the ones defined in the interface containing the request,
> plus wl_display errors. You will be also adding all errors on all
> argument objects to the set, per request.
> 
> That is why I am hesitant to commit this. I'm sure someone will find
> a case where this would make something like writing language bindings
> more difficult for him, even if it seems obscure or even silly to us.
> 
> It's not like we share e.g. pixel format enums between, say, wl_shm and
> wl_drm, even though they are the same AFAIK, and there is a
> considerable amount of duplication. (Yes, that is a bit different case,
> because there we would be breaking also the namespacing.)
> 
> 
> Thanks,
> pq
> 
> > On Fri, Aug 22, 2014 at 10:37 AM, Jasper St. Pierre <jstpierre at mecheye.net>
> > wrote:
> > 
> > > This will be used by implementations to send out errors if clients try
> > > to set roles on surfaces that already have roles.
> > > ---
> > >  protocol/wayland.xml | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> > > index bb457bc..f6eb54d 100644
> > > --- a/protocol/wayland.xml
> > > +++ b/protocol/wayland.xml
> > > @@ -983,6 +983,7 @@
> > >        </description>
> > >        <entry name="invalid_scale" value="0" summary="buffer scale value
> > > is invalid"/>
> > >        <entry name="invalid_transform" value="1" summary="buffer transform
> > > value is invalid"/>
> > > +      <entry name="surface_has_existing_role" value="2" summary="the
> > > surface passed already has a role"/>
> > >      </enum>

Hi,

does anyone still want to defend this patch, or should we add "existing
role" error definitions to all interfaces assigning roles instead?


Thanks,
pq