Page MenuHomePhabricator

Gesture Manager: Add gestures and fix gesture managing, recognizer logic.
Needs ReviewPublic

Authored by CHAN on Jan 10 2019, 1:16 AM.

Details

Summary

https://phab.enlightenment.org/T7544

Provides a way for a user to get a gesture manager, recognizer instance.

Supports different recognizer properties for each target(Eo).

Gesture, Touch Class Life-cycle re-implementation. for supporting multiple touches.

Add below gestures.
efl_canvas_gesture_tap
efl_canvas_gesture_double_tap
efl_canvas_gesture_triple_tap
efl_canvas_gesture_long_tap
efl_canvas_gesture_momentum
efl_canvas_gesture_zoom
efl_canvas_gesture_flick

Test Plan

Simple test -> test_gesture_framework.c
More test cases will upload.

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D7579
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10193
Build 8173: arc lint + arc unit
CHAN created this revision.Jan 10 2019, 1:16 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

CHAN requested review of this revision.Jan 10 2019, 1:16 AM
CHAN edited the summary of this revision. (Show Details)Jan 10 2019, 1:22 AM
CHAN edited the test plan for this revision. (Show Details)
CHAN added reviewers: woohyun, smohanty.
segfaultxavi requested changes to this revision.Jan 10 2019, 2:48 AM
segfaultxavi added a subscriber: segfaultxavi.

Several missing docs. Please mark each inline comment as done as you fix them so none is forgotten.

src/lib/evas/gesture/efl_canvas_gesture_double_tap.eo
3

Shouldn't it be "EFL Gesture Double Tap class"?

Also, what's the purpose of this class? Objects emitting Double-tap events inherit from it? Then this should be explained here. Same thing for all other gesture classes.

Also, what is a "double tap"? This deserves at least a one-line explanation: "A double tap is two taps in quick succession." Is the time between taps configurable? If not, how much time is it?

5

Is this eo_prefix correct?

src/lib/evas/gesture/efl_canvas_gesture_long_tap.eo
3–4

How long must be a tap for it to be considered "long"? Is it configurable?

8

Event for the Long tap gesture.

src/lib/evas/gesture/efl_canvas_gesture_manager.eo
19

Missing docs for the method and its parameter and return value.

src/lib/evas/gesture/efl_canvas_gesture_recognizer_double_tap.eo
7

Docs (properties, keys, values) and indentation!

src/lib/evas/gesture/efl_canvas_gesture_recognizer_long_tap.eo
6

Docs for property, keys and values!

src/lib/evas/gesture/efl_canvas_gesture_recognizer_tap.eo
3–4

Please explain how this class is used (also the other gesture recognizer classes).

This revision now requires changes to proceed.Jan 10 2019, 2:48 AM

comments for rebase

src/lib/evas/gesture/efl_canvas_gesture_double_tap.eo
1

To rebase on the latest, this should be as follows.

class Efl.Canvas.Gesture_Double_Tap extends Efl.Canvas.Gesture

src/lib/evas/gesture/efl_canvas_gesture_recognizer_double_tap.eo
2

To rebase on the latest, this should be as follows.

class Efl.Canvas.Gesture_Recognizer_Double_Tap extends Efl.Canvas.Gesture_Recognizer

CHAN updated this revision to Diff 19426.Feb 14 2019, 10:23 PM

Added more gestures.

CHAN updated this revision to Diff 19427.Feb 14 2019, 11:11 PM

Code fix to follow latest code base.

CHAN updated this revision to Diff 19428.Feb 14 2019, 11:16 PM

Patch clean up.

CHAN updated this revision to Diff 19429.Feb 14 2019, 11:17 PM

Delete white space.

I made several comments regarding the docs which have not been addressed. Please correct them.
I also suggest you mark each comment as "Done" so no comment is forgotten.

CHAN updated this revision to Diff 19900.Sun, Mar 3, 11:29 PM

Add Zoom gesture.

CHAN updated this revision to Diff 19901.Sun, Mar 3, 11:32 PM

Add zoom gesture to meson build.

CHAN updated this revision to Diff 19904.Mon, Mar 4, 12:09 AM

Code clean up.

CHAN updated this revision to Diff 19905.Mon, Mar 4, 12:09 AM

Code clean up.

CHAN added a comment.Mon, Mar 4, 12:14 AM

I will upload whole doc.

Jaehyun_Cho added a comment.EditedFri, Mar 8, 3:18 AM

@CHAN @smohanty

I have checked the code and tested the functionality.
And I found out that win cannot add gesture event callback.
Is it originally intended?

In my mind, it would be much easier and convenient if we add gesture event callback to win. Because we do not need to add rectangle only for gesture event callback.
Please let me know if I miss something or disadvantages of using win to add gesture event callback.

@CHAN @smohanty

Regardless of this patch, what do you think about changing method name "create" in Efl.Canvas.Gesture.Recognizer to "gesture_create"?

src/lib/evas/gesture/efl_canvas_gesture_recognizer.eo
16

Basically this is not related to this patch.
But what do you think about changing this method name to "gesture_add"?
If the method name remains "create", then language binding provides recognizer.Create() which does not look like creating gesture instance...

segfaultxavi requested changes to this revision.Sat, Mar 9, 4:06 AM

Some of my comments regarding documentation have not been addressed yet...

src/lib/evas/gesture/efl_canvas_gesture_recognizer.eo
16

I agree Create will be confusing for the bindings.

This revision now requires changes to proceed.Sat, Mar 9, 4:06 AM
Jaehyun_Cho added inline comments.Sun, Mar 10, 7:40 PM
src/lib/evas/gesture/efl_canvas_gesture_recognizer.eo
16

I mean "gesture_create" not "gesture_add" :)

This is a minor suggestion of moving code inside the conditional expression.

src/lib/evas/gesture/efl_canvas_gesture_recognizer_double_tap.c
125

how about moving this inside the conditional expression if (pd->timeout) ?

139

how about moving this inside the conditional expression if (pd->timeout) ?

src/lib/evas/gesture/efl_canvas_gesture_recognizer_long_tap.c
128

how about moving this inside the conditional expression if (pd->timeout) ?

163

how about moving this inside the conditional expression if (pd->timeout) ?

src/lib/evas/gesture/efl_canvas_gesture_recognizer_tap.c
65

how about moving this inside the conditional expression if (pd->timeout) ?

src/lib/evas/gesture/efl_canvas_gesture_recognizer_triple_tap.c
125

how about moving this inside the conditional expression if (pd->timeout) ?

139

how about moving this inside the conditional expression if (pd->timeout) ?

This comment was removed by Jaehyun_Cho.
CHAN updated this revision to Diff 20452.Mon, Mar 11, 11:37 PM
CHAN marked 12 inline comments as done.

Doc update and added code comment.

CHAN added a comment.Mon, Mar 11, 11:42 PM

@segfaultxavi @Jaehyun_Cho

Thanks for giving review here.

I update code and doc.

I checked window target case.
It does not work well. The window has diff event generate logic.
I will be working independently from the current commit.

thanks.

src/lib/evas/gesture/efl_canvas_gesture_double_tap.eo
3

When user got gesture callback they need to use gesture methods for each gestures.

It defined a specific gesture event such as "tap" and user can get gesture infos.

More info about the gesture configure value will explain in recognizer class.

5

It generated to "EFL_EVENT_GESTURE_DOUBLE_TAP". does it wrong?

src/lib/evas/gesture/efl_canvas_gesture_long_tap.eo
3–4

It depends on the profile.

segfaultxavi resigned from this revision.Tue, Mar 12, 2:39 AM

I have no further documentation concerns. I will let others do the code review.
Thanks!