multi-touch-dev team mailing list archive

[Chase Douglas <chase.douglas@xxxxxxxxxxxxx>]

On Fri, 2010-08-20 at 12:36 -0700, Bryce Harrington wrote:
> 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.

I chatted with jcristau on #ubuntu-x and he suggested bumping
xorg-server/debian/serverminver to 2:1.8.99.904-1ubuntu3. I'll remove
the depends addition here since the serverminver file is used.

The last remaining part is to add a build deps for evdev against the new
server package so the right serverminver is picked up.

> 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?

This is cut and paste from all over in evdev.c. It should be guaranteed
to always be valid.

> ++    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?

Probably. I've added a bounds check for this.

> ++        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.

To be honest, I wondered if I should even put anything here. In the
server itself, no return value check is performed (no, really! grep for
WriteToClient in dix/). Maybe this is the reason they don't check for
values, so warnings don't end up in Xorg.log. I'll remove the message
since it doesn't seem to hurt anything anywhere else.

> +-    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.

Values above ABS_MISC are multitouch. We don't want evdev attempting to
handle those values, so we ensure it never checks for them.

Thanks for the review. I've pushed the changes noted above to the repo.

-- Chase