Page MenuHomePhabricator

Efl.Canvas.Gesture_Recognizer
Closed, ResolvedPublic

Description

Base abstract class for Gesture Recognizers. For internal use only.       
                                                                          
Gesture recognizers listen to events that occur on a target object        
to see if a particular gesture has occurred.                              
                                                                          
Recognizer-specific configuration values can be modified through @.config.
Default configuration values are taken from the system's configuration.   

- (M) add
- (M) recognize
- (M) reset
- (P) config

Let's use this task to track also all derived classes:

  • flick
  • tap
  • triple_tap
  • long_tap
  • zoom
  • double_tap
  • momentum
segfaultxavi triaged this task as Normal priority.
segfaultxavi moved this task from Backlog to Evaluating on the efl: api board.
segfaultxavi added subscribers: bu5hm4n, cedric, zmike and 2 others.

Documentation is detailed.

zmike added a comment.Dec 24 2019, 6:33 AM

If this is internal-only why are we evaluating it for stabilization?

You want to have that in case you want to write a custom recognizer.

However:

  • I am not sure why there is add + the first argument in recognize, it sounds simpler to me that recognize returns "some" object, that is only valid until the next call to recognize ? (So the recognizer can handle on its own what it wants to do with the returned object)
  • reset can be removed i guess ?
  • Is config really needed here ? Its just redirecting to the manager object ...

Additionally:

  • We should review the list + hash usages here, we could save some memory here
  • We *must* review the hash usages, eina_hash_add is used here multiple times without checking if there is already something registered to the type
  • We also must review the event usages here, i think this could cross the ability to write "custom" recognizers.
zmike added a comment.Dec 24 2019, 7:10 AM

The above comment is in reference to the statement that this class is for internal use in the OP text, so we can disregard I guess.

Another thing:

  • The privat data struct of this is defined in efl_canvas_gesture_privat.h, in efl_canvas_gesture_manager.c This struct is used to get the flag continues which is a problem IMO as recognizers written in binded languages cannot access this struct.
  • _Efl_Canvas_Gesture_Recognizer_Data also contains finger_size which is also not available form binded languages
  • _Efl_Canvas_Gesture_Recognizer_Data contains gesture and manager, which are not assigned or duplicated.

All the items listed here are addressed now.

Current:

abstract Efl.Canvas.Gesture_Recognizer
├ (P) continues :: set@protected
├ (M) add
├ (M) recognize
├ (M) reset
  • I think add is misleading since this is actually creating a new gesture object with the type that the recognizer recognizes, so probably it should be renamed.
  • reset isn't really implemented by many recognizers, but we may want to change it so that the cleanup code in the recognizers uses a more unified codepath for resetting internal state. This would involve more eo lookups in the event processing hotpath though, so that's possibly something to consider.
  • I think add is misleading since this is actually creating a new gesture object with the type that the recognizer recognizes, so probably it should be renamed.

Hmmm... I agree. Probably create_gesture or new_gesture are clearer.

I guess we use efl_add to create new objects in EFL and our API is suffering from that.

Tbh the ideal here would be to just have a "gesture_event_type_get' which returns the event description since this is only a wrapper to pass a hardcoded event type into another function. But that requires us to expose event descriptions to bindings.

Maybe expose them as a string name ? Event descriptions are going to be a pain :/

zmike added a comment.Jan 31 2020, 7:08 AM

Actually...I have an idea.

zmike added a comment.Jan 31 2020, 9:42 AM
abstract Efl.Canvas.Gesture_Recognizer
├ (P) type
├ (P) continues :: set@protected
├ (M) recognize
├ (M) reset
zmike added a comment.Jan 31 2020, 9:44 AM

Now all that needs to be done is make a decision on reset. We can either remove this method or we can implement it for all the recognizers and change it so that the manager object calls this method on recognizers any time they return cancel/finish.

I think beside the reset thing, everything in here looks pretty fine. All the recognizers also do not have any properties left, which is good.

ali.alzyod added a subscriber: ali.alzyod.EditedFeb 3 2020, 6:58 AM

there are some queries (I hope they are useful.)

  • long_tap: does this mean long_press?
  • tap(single_tap), double_tap, trible_tap : all combined in a single class with property for tap count?
  • zoom: does not pinch seems more related to gesture than one of its uses?
  • how about rotate gesture? as you use in images to rotate them (I think in IOS gallery such feature)
  • how about pan gesture? for example when you use map application you pan to move scroller to a new location in the map.
zmike added a subscriber: CHAN.Feb 3 2020, 9:05 AM

there are some queries (I hope they are useful.)

  • long_tap: does this mean long_press?
  • tap(single_tap), double_tap, trible_tap : all combined in a single class with property for tap count?

Could be worthwhile to have this as more of a tap_repeater style recognizer where it just triggers every time there's a tap and cancels on longer press or timeout. Need @CHAN feedback.

  • zoom: does not pinch seems more related to gesture than one of its uses?

No, pinch is only the pinching part of a zoom.

  • how about rotate gesture? as you use in images to rotate them (I think in IOS gallery such feature)

Out of scope.

  • how about pan gesture? for example when you use map application you pan to move scroller to a new location in the map.

Out of scope, though also this is just momentum.

ali.alzyod added a comment.EditedFeb 3 2020, 9:10 AM
  • zoom: does not pinch seems more related to gesture than one of its uses?

No, pinch is only the pinching part of a zoom.

Maybe my English is not good :), let me rephrase. (Pinch is something related to your finger movement while zoom is not something related to your finger, it is a reflection to something you do with your finger in screen).
For example you can pinch someone face, but you can not zoom it (in real life :) )

zmike added a comment.Feb 3 2020, 9:15 AM

I know what you meant. But a pinch is specifically taking two fingers and moving them closer together. The zoom gesture includes that as well as the inverse movement, which is absolutely not a pinch.

ali.alzyod added a comment.EditedFeb 3 2020, 9:16 AM
In T8503#150370, @zmike wrote:

I know what you meant. But a pinch is specifically taking two fingers and moving them closer together. The zoom gesture includes that as well as the inverse movement, which is absolutely not a pinch.

What I mean is that gestures is something you do with your fingers like tap,press,hold but what is Zoom (is it fingers gesture?)

For example double tab could mean zoom, normally when you double tab image in your gallery application it normally zoom in. So is this what we mean by Zoom gesture event, or it is the event where you pinch your fingers

I like to mention that some SDK already use pinch name for the gesture :
https://developer.apple.com/documentation/uikit/uipinchgesturerecognizer?language=objc

zmike added a comment.Feb 3 2020, 10:30 AM

While this is true, I don't think pinch is a correct name here since the gesture includes the inverse of a pinch gesture.

In T8503#150373, @zmike wrote:

While this is true, I don't think pinch is a correct name here since the gesture includes the inverse of a pinch gesture.

How about spread, or spread_pinch

Beside the name discussion: The API looks fine.

To summarize the ideas (after remove out of scope parts)

  • Long_tap: is not long_press more desciptive? press means we care about pressing down, while tap is something similar to the click (down and up)
  • Tap :single_tap, double_tap, trible_tap : all combined in a single class with property for tap count?
  • Zoom: rename into spread or spread_pinch. where we are naming after touch gesture rather than expected behaviour.
zmike added a comment.Feb 18 2020, 7:02 AM

long_press seems like it would be a useful change to bring some consistency with the rest of EFL, but I'm still awaiting @CHAN or @woohyun to give feedback regarding the other items.

zmike added a comment.Feb 19 2020, 9:36 AM

An item I missed: there's a value zoom_step as part of the zoom recognizer which reports zooms as multiples of a provided step value. This currently has no way to be set, but it seems like something we should have a config value for at some point, which throws us back into the efl.config disaster.

bu5hm4n closed this task as Resolved.Mar 3 2020, 1:56 AM
bu5hm4n claimed this task.