* instead of the pair direction, positiveDirection, I'd go for a single enum with 4 values, what do you think? * we talked yesterday about the possibility to add a time threshold as well (within which the touch must cross the distance threshold), do you think you could add it here or in a later change? * there seems to be a bunch of things common with the QtWidgets QGestureRecognizer (state names, for example) [1] - do you think it would make sense to converge on the same terminology? * could you add some newlines to tests/plugins/UbuntuGestures/CMakeLists.txt for readability? ===== 360 +add_definitions(-DUBUNTUGESTURES_LIBRARY) 361 + 362 +add_library(UbuntuGestureQml MODULE ${UbuntuGestureQml_SOURCES}) 749 +class UBUNTUGESTURES_EXPORT DirectionalDragArea : public QQuickItem { 856 +#if defined(UBUNTUGESTURES_LIBRARY) 857 +# define UBUNTUGESTURES_EXPORT Q_DECL_EXPORT 858 +#else 859 +# define UBUNTUGESTURES_EXPORT Q_DECL_IMPORT 860 +#endif 973 + LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}/plugins/Ubuntu/Gestures 982 +target_link_libraries(DirectionalDragAreaTestExec UbuntuGestureQml) maybe directly use the .cpp in test sources instead? We don't really want it to be a library, do we. ===== 538 + if (touchPoint->state() == Qt::TouchPointReleased) { is it not possible here for touchPoint to be invalid? why wouldn't QTouchEvent::touchPoints() be enough here? ===== I'm wondering if the "pointInside..." calculations couldn't be simplified by using a qreal factor instead of the angle - you'd just need to make sure that: -d <= Δy <= Δx * f + d where y is perpendicular to the gesture direction (horizontal positive / left-to-right in that case), d is max deviation, f is the factor; with f = 1 that would mean 45° widening angle; what do you think? ===== 666 +bool DirectionalDragArea::movingInRightDirection(const QPointF &point) 667 +{ 668 + if (m_orientation == Horizontal) { 669 + if (m_positiveDirection) { 670 + return point.x() >= m_previousPos.x(); shouldn't this take some deviation into account? i.e. if your finger goes slightly back in Recognizing state, won't that mean rejecting the gesture altogether? ===== 744 + recognition will fail. It will also fail it the drag or flick is too short. E.g. a → "...fail if the drag..." ===== 740 +/* 741 + An area that detects axis-aligned single-finger drag gestures could we ask for a little more documentation on the properties? ===== 755 + // stuff that will be set in stone at some point as discussed, I do agree we need strong defaults here, but allowing advanced use where those would be modifiable could be possible as well ===== 768 + bool positiveDirection() const { return m_positiveDirection; } 773 + qreal maxDeviation() const {return m_maxDeviation;} 776 + qreal wideningAngle() const {return m_wideningAngle;} 779 + qreal distanceThreshold() const {return m_distanceThreshold;} please use spaces after/before curly braces consistently ===== 764 + enum Orientation { Horizontal = Qt::Horizontal, Vertical = Qt::Vertical }; 812 + enum { please consider using strongly-typed enums (although I don't think we've checked how QML currently deals with passing them to/from QML - so we might need to wait there) ===== 939 +# QML tests that do not require graphical capabitlies. 940 +add_custom_target(qmlunittests) 941 + 942 +# QML tests that require graphical capabitlies. 943 +add_custom_target(qmluitests) 944 + 945 +add_custom_target(qmltests) 946 +add_dependencies(qmltests qmlunittests qmluitests) 947 + 948 add_custom_target(alltests) 949 -add_dependencies(alltests check) 950 +add_dependencies(alltests check qmltests) 1611 -add_custom_target(qmltests) 1612 -add_custom_target(qmlunittests) 1613 -add_custom_target(qmluitests) 1614 - 1615 -add_dependencies(alltests qmltests) 1616 - 1624 -set(qmltest_DEFAULT_TARGETS qmlunittests qmltests) 1625 +set(qmltest_DEFAULT_TARGETS qmlunittests) 1633 -set(qmltest_DEFAULT_TARGETS qmluitests qmltests) 1634 +set(qmltest_DEFAULT_TARGETS qmluitests) please extract those changes to a separate MP ===== 968 + ${CMAKE_CURRENT_SOURCE_DIR}/../../../plugins/Ubuntu/Gestures ${CMAKE_SOURCE_DIR}/plugins/Ubuntu/Gestures looks cleaner ===== 986 +add_custom_target(UbutunGesturesTestQmlFiles ALL → "UbuntuGestures..." ===== ninja: error: '../tests/plugins/UbuntuGestures/*.qml', needed by 'tests/plugins/UbuntuGestures/CMakeFiles/UbutunGesturesTestQmlFiles', missing and no known rule to make it ninja seems to have issues with that rule, it might be unable to expand "*.qml" internally - probably best to use CMake's GLOB. ===== 1372 + //width: units.gu(40) 1373 + //height: units.gu(71) hmm... any way to make the example work with a pointer? can we enable pointer composition? do you have a Nexus 10 to try it out? I'm getting quite some false negatives with the examples also, tapping in the edge area results in the red rectangle to stay on screen ===== Awesome work, BTW! [1] http://qt-project.org/doc/qt-5.0/qtwidgets/qgesturerecognizer.html