Grail

Merge lp://staging/~oif-team/grail/eventprinter into lp://staging/grail

  1. eventprinter
  2. Merge into trunk
Proposed by Jussi Pakkanen
Status: Merged
Merged at revision: 167
Proposed branch: lp://staging/~oif-team/grail/eventprinter
Merge into: lp://staging/grail
Diff against target: 253 lines (+240/-0)
2 files modified
tools/Makefile.am (+1/-0)
tools/grail-eventprinter.c (+239/-0)
To merge this branch: bzr merge lp://staging/~oif-team/grail/eventprinter
Reviewer Review Type Date Requested Status
Chase Douglas (community) Approve
Review via email: mp+72400@code.staging.launchpad.net

Description of the change

This branch adds a test application that prints Grail events in plaintext. It is meant as a complement to the existing Grail event printer application. That application prints every piece of information on every event. That makes it useful for detail inspection.

This application prints gesture progression less detailed but more "logical" way. It is meant to quickly answer questions such as "does this trace produce a rotation gesture", "in which order are the drag and pinch gestures detected/reported" and so on.

"Make install" does not install this app, because it is not yet polished enough for end user consumption.

To post a comment you must log in.
Revision history for this message
Chase Douglas (chasedouglas) wrote :

I would like to see it cleaned up before merging. If it's in the source tree, then people might look to it as an example of how to use grail.

For the same reason, I would like to get rid of including grail-impl.h. What do you need it for?

review: Needs Fixing
Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

What sorts of cleanups did you have in mind? As an aside: the parts that use Grail are almost directly copypasted from grail-gesture.c.

I #include grail-impl.h because I need to access the private structure to set fast_fileread. I assume that we don't want to expose that in the API. Or if we do, it needs its own branch.

lp://staging/~oif-team/grail/eventprinter updated
171. By Jussi Pakkanen

Removed spurious parentheses.

172. By Jussi Pakkanen

No not install eventprinter. For real this time.

Revision history for this message
Chase Douglas (chasedouglas) wrote :

The thing that stuck out for me was the grail-impl.h include and the line near the end:

//grail_filter_abs_events(&ge, 1);

I think we should delete that line. For grail-impl.h, we should have a comment stating why it's included here so people know that this is an exception for a test tool that ships with the library.

Other than that, if it compiles cleanly with -Wall -Werror that I think it's ready to be merged.

lp://staging/~oif-team/grail/eventprinter updated
173. By Jussi Pakkanen

Clarify that other people should not include Grail's internal headers.

Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

I clarified the comment. Is it OK now?

Grail itself does not build with -Wall -Werror but building with -Wall did not seem to produce any warnings, so it should be good.

Revision history for this message
Chase Douglas (chasedouglas) wrote :

Please put a note in the comment about why the header is included, i.e. about the fast playback of events. Otherwise, I don't really know why its there, and I'm likely to forget :).

Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

How about now?

lp://staging/~oif-team/grail/eventprinter updated
174. By Jussi Pakkanen

Clearer description of private header include.

Revision history for this message
Chase Douglas (chasedouglas) wrote :

Looks good! Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: