Merge lp:~bregma/geis/lp-891731 into lp:geis
Status: | Merged |
---|---|
Merged at revision: | 178 |
Proposed branch: | lp:~bregma/geis/lp-891731 |
Merge into: | lp:geis |
Diff against target: |
602 lines (+511/-12) 5 files modified
libutouch-geis/backend/xcb/geis_xcb_backend_token.c (+28/-12) testsuite/geis2/lp891731.py (+209/-0) testsuite/geis2/test_device1.events (+210/-0) testsuite/geis2/test_device1.prop (+32/-0) testsuite/geis2/test_device2.prop (+32/-0) |
To merge this branch: | bzr merge lp:~bregma/geis/lp-891731 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Chase Douglas (community) | 2011-12-06 | Approve on 2011-12-07 | |
Jussi Pakkanen (community) | Approve on 2011-12-07 | ||
Review via email:
|
This proposal supersedes a proposal from 2011-11-25.
Description of the change
Properly handles the case of no matching devices in a filter (LP: #891731).
Includes a semi-automatic test case testsuite/
This merge request provides the same fix code but an entirely new test driver program. there is no need for touch hardware on the test system but a working X server is required.
Stephen M. Webb (bregma) wrote : | # |
Added test case documentation (in the source code for the test case).
Chase Douglas (chasedouglas) wrote : | # |
The comment is good. I didn't really understand what the test was doing until now.
I think the test case would be easier to run if it did the following:
1. If a direct touch device is found, print a message like "SKIP (Reason: Touchscreen device must not be present)", then exit.
2. If an indirect touch device also is not found, print a message like "SKIP (Reason: Indirect touch device must be present)", then exit.
3. Prompt the user with something like: "Place two touches on an attached multitouch device. An error will be printed if the test fails. If no message is printed, the test passed."
For extra kudos, you could detect the devices, and if one class is present and the other is missing you could then do the subscription for the missing class. This would allow testing on a tablet, for example.
Jussi Pakkanen (jpakkane) wrote : | # |
Looks good. The only change I would consider is naming match_count into something like matched_devices or matched_
Chase Douglas (chasedouglas) wrote : | # |
This comment gave me a hard time:
# need to guarabtee there is a device we can feed events to
I figured it out after a few seconds :). Otherwise it looks good!
It's not the cleanest fix, but it gets the job done. I'm assuming Stephen has tried to think of cleaner fixes, so this is probably the easiest we can do without a big overhaul of some piece.
I think the code and testcase are fine, but the testcase needs documentation. Any manual testcase needs to have a description of the required environment, how to run the test, and expected results.