multi-touch-dev team mailing list archive

[Bryce Harrington <bryce@xxxxxxxxxxxxx>]

On Fri, Aug 20, 2010 at 01:49:34PM -0400, Chase Douglas wrote:
> On Fri, 2010-08-20 at 10:45 -0700, Bryce Harrington wrote:
> > On Fri, Aug 20, 2010 at 12:48:05PM -0400, Chase Douglas wrote: 
> > > Since you chimed in, would you be willing to review the changes and
> > > upload to ubuntu when everything is good?
> > 
> > Sure, just give me nudge when it's ready to look at.
> 
> I think it's ready for a packaging/code review now. I just pushed the
> latest stuff with changes for Henrik's comments (none of which changed
> code functionality) to ppa:utouch-team/unstable for testing.

Depends: ${shlibs:Depends}, ${misc:Depends}, ${xinpdriver:Depends},
- libutouch-grail1 (>= 1.0.10)
+ libutouch-grail1 (>= 1.0.10), xserver-xorg-core (>= 2:1.8.99.905-1ubuntu2)

Hmm, I'd question if a dependency on the xserver itself is appropriate,
I'm a bit concerned it could lead to a circular dependency or make
upgrades tricky in certain cases.  I assume this is for ensuring ABI
correctness?  Could you give some additional detail on this?  Maybe
there's a better way to solve it.

In any case, the version here should probably match the xserver-xorg-dev
on the source package.


Everything else looks good.  I have a few really minor questions on the
patches:

Patch 101, src/evdev-grail.c
++    InputInfoPtr pInfo = grail->priv;
++    EvdevPtr pEvdev = pInfo->private;

Would null pointer checks make sense here, or are these mostly
guaranteed to always be non-null?


++    memset(clients, 0, sizeof(struct grail_client_info) * max_clients);

Since max_clients is a function parameter and a signed int, would bounds
checking be worthwhile?


++        xf86Msg(X_WARNING, "Warning: Failed to write entire gesture event\n");

I think saying 'Warning' here is redundant, the line will be prepended
with "(WW)" already.

Also, I think users might find this warning a bit cryptic.  Is there a
way it could be phrased that would help them in understanding what
problem(s) this warning could signify?

>From the code it sounds like it could crop up if there is a problem with
the client application; if that's correct, then for instance it might be
helpful to indicate that they might want to look at the state of the
client app.

Would this warning message be printed out repeatedly if the user keeps
making gestures that the client can't take?  If so, and if that'd spam
up the Xorg.0.log, it might be worth making the warning only print out a
few times and then suppress further messages.


+-    for (axis = ABS_X; axis <= ABS_MAX; axis++) {
++    for (axis = ABS_X; axis <= ABS_MISC; axis++) {

I'm sure this is fine, but I'm curious about this change.


Bryce