Code review comment for lp:~dandrader/unity/phablet_edgeDragGesture

W dniu 16.05.2013 01:04, Daniel d'Andrada pisze:
>> * 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?
>
> You mean those signals?
>
> void dragStarted();
> void directionalDragRecognized();
> void dragRejected();
> void dragEnded();
>
> Compared to this enum?
>
> enum ResultFlag { Ignore, MayBeGesture, TriggerGesture, FinishGesture, CancelGesture, ConsumeEventHint }
>
> Well, they are intented for different audiences (the former for application code and the latter for the gesture fw engine) in a bit different contexts. I preffer the signal naming the way they are, as they look very clear in meaning IMHO. One thing I see could change though is s/dragRejected/dragCancelled

Your call.

>> =====
>>
>> 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.
>
> Why not? Makes testing simpler. Recompiling the whole plugin in the test directory seems wasteful.

There's just a lot here that suggests this is meant to be used as a
shared library, which it isn't. But I can live with that. We should
probably simply find a way to use the plugins from C++.

>> 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)
>
> Yeah, I think we might need to wait indeed.

I've used them successfully in [1] - they end up as numbers in QML, so
there's some casting involved, but they seem to work fine.

>> do you have a Nexus 10 to try it out? I'm getting quite some false negatives
>> with the examples
>
> No, I don't. Do you have a Galaxy Nexus to try it out and see if results differ? That's the device I have.
>
> It seems that drags coming from the top, left and right borders are being filtered out by mir as those borders are reserved for the shell and qmlscene is not the shell? Because no input events come at all in those situations. If you start dragging already from withing the window it works.
> Also from the bottom border it always works, I guess it's because the bottom edge is for application's tool bars.

You can stop qml-phone-shell by removing its line from
/etc/phone-services on the device and `sudo restart ubuntu-session`.
You'll get all the input then.

>> also, tapping in the edge area results in the red rectangle to stay on screen
>
> Details, details... But I couldn't reproduce it myself

;P

Testing again.

[1]
https://code.launchpad.net/~saviq/unity-api/add-shell-notifications-api/+merge/163557

--
Michał Sawicz <email address hidden>
Canonical Services Ltd.