[PULL] unreviewed patches

[Peter Hutterer]

On Fri, Oct 26, 2012 at 04:04:21PM -0700, Keith Packard wrote:
> Peter Hutterer <peter.hutterer at who-t.net> writes:
> 
> > All these need review lovin'. 
> >
> > Pull request is on top of my for-keith branch
> > from last Friday.
> > http://lists.freedesktop.org/archives/xorg-devel/2012-October/034064.html
> >
> > The following changes since commit c5396ec05a5c6cab6608ba677f703c5227b1de13:
> >
> >   xf86: Fix build against recent Linux kernel (2012-10-19 13:12:33 +1000)
> >
> > are available in the git repository at:
> >
> >   git://people.freedesktop.org/~whot/xserver unreviewed
> >
> > for you to fetch changes up to c97467d449c30da7529bdcc68c3ed344828a6baa:
> >
> >   dix: don't filter RawEvents if the grab window is not the root window (#53897) (2012-10-25 15:54:02 +1000)
> >
> > ----------------------------------------------------------------
> > Peter Hutterer (7):
> >       dix: don't allow disabling XTest devices
> 
> I'd say the error values for the two code paths should be the same. I'd
> suggest that BadMatch might be a slightly better value, but I'm easy if
> you prefer BadAccess.

We've had BadAccess for VCP/VCK for a while now, so I'm hesitant to change
it for little benefit. Plus, the error codes for properties are a bit of a
desaster, I should've added specific error codes for the various errors that
can actually happen.

For XChangeDeviceControl, we actually have it documented that BadMatch is a
valid error code (and from my reading it appears to be the best choice
here).

> Also, the comment in DeviceSetProperty should be updated to reflect that
> the xtest devices are also not subject to disabling.

done, thanks.

> I did verify that these are the only relevant code paths.
> 
> Mostly-happy-with: Keith Packard <keithp at keithp.com>
> 
> >       xkb: ProcesssPointerEvent must work on the VCP if it gets the VCP
> 
> Yeah, this looked right when I looked at it a while ago.
> 
> Reviewed-by: Keith Packard <keithp at keithp.com>
> 
> >       Xi: set xChangeDeviceControlReply.status to Success by default
> 
> Reviewed-by: Keith Packard <keithp at keithp.com>
> 
> >       Xi: don't deliver TouchEnd to a client waiting for TouchBegin (#55738)
> 
> Following the state changes in exevents.c was entertaining, but there
> doesn't seem to be any way to get past LISTENER_AWAITING_BEGIN without
> delivering a TouchBegin event. Therefore, this simple patch seems
> correct to me.
> 
> Reviewed-by: Keith Packard <keithp at keithp.com>
> 
> >       dix: fix zaphod screen scrossing (#54654)
> 
> Reviewed-by: Keith Packard <keithp at keithp.com>
> 
> >       xfree86: remove unused variable sigstate
> 
> Reviewed-by: Keith Packard <keithp at keithp.com>
> 
> >       dix: don't filter RawEvents if the grab window is not the root window (#53897)
> 
> I'd have to say that 'FilterRawEvents' is a pretty miserable function;
> the semantics of its return value are muddled due to how it gets
> invoked, in particular, it "knows" that an event may have been delivered
> through the grab and that it must, therefore, suppress an extra event in
> that case. Ick.
> 
> I have the feeling that it should be using the return value from
> DeliverGrabbedEvent to help decide whether to deliver another event,
> something like:
> 
>         if (deliveries && SameClient(grab, client))
>                 don't deliver another event
> 
> Then, the question is 2.0 vs 2.1; for the 2.0 case a grab always
> prevents delivery
> 
>         else if (2.0 && grab)
>                 don't deliver an event
> 
>         else
>                 deliver an event
> 
> Just sticking these tests right inside DeliverRawEvent would keep all of
> the conditions together, instead of having some split out to
> FilterRawEvent. Keeping the Xi version check in a separate function
> makes good sense though; that's really the only complexity in
> FilterRawEvents at present.

I'll have a look again and see if I can clean this up. One day.

> (I'm hoping I got the semantics of raw events right; the protocol spec
> seems a lot less ambiguous than the code does...)

if there's any ambiguity in the spec let me know and I can amend them. I
know what was _intended_, so we can fix the spec to be understood by
people now directly involved in the development. A luxury that we don't have
on all extensions...

fwiw, the semantics are fairly simple:
XI 2.0: raw events except if some other client has a grab
XI 2.1: raw events at all times

The code is more complicated largely to avoid duplicate raw events.

Cheers,
   Peter