Add number of touches on Android and iOS if needed #152

osdnk · 2018-04-23T11:54:52Z

Motivation

#134
This PR refers to changes made someday by @tsapeta involving passing natively number of touches given out-of-box on iOS
I just provide same mechanism on Android which seems to be quite useful.

Changes

Added addditional field in GestureHandler.java, getter and few lines to update this field. Now it's also exported to js bridge

kmagiera

Can you also update description and link to the issue this is resolving.

Also, can you please respond to my inline comment>

kmagiera · 2018-04-24T08:42:31Z

android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerModule.java

@@ -133,6 +133,7 @@ public void configure(NativeViewGestureHandler handler, ReadableMap config) {
    @Override
    public void extractEventData(NativeViewGestureHandler handler, WritableMap eventData) {
      eventData.putBoolean("pointerInside", handler.isWithinBounds());
+      eventData.putDouble("numberOfTouches", handler.getMaxNumberOfPointers());


Not sure if it was intended to provide a max number of pointers here but my intuition for this particular event param would be that it represents the current number of touches (much like pointerInside refers to the current state of the handler).
Also consider the edge case when this method is used for the state change event and make sure this is consistent with the behaviour on iOS.

I did observe its behaviour on iOS. Well, you're right that numberOfTouches return number oft touches in current situation and I gonna change it.
However, don't you consider introduction the mechanism of returning total number of touches involved in gesture?

kmagiera

Almost there 💪

Just a few final comments

kmagiera · 2018-04-24T16:36:46Z

android/lib/src/main/java/com/swmansion/gesturehandler/GestureHandler.java

@@ -161,6 +166,9 @@ public final void handle(MotionEvent event) {
      }
      return;
    }
+    if (mNumberOfPointers != event.getPointerCount()) {


this check doesn't save much, can you get rid of it?

kmagiera · 2018-04-24T16:40:28Z

android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerModule.java

@@ -133,6 +133,7 @@ public void configure(NativeViewGestureHandler handler, ReadableMap config) {
    @Override
    public void extractEventData(NativeViewGestureHandler handler, WritableMap eventData) {
      eventData.putBoolean("pointerInside", handler.isWithinBounds());
+      eventData.putDouble("numberOfTouches", handler.getNumberOfPointers());


OMG. I don't know how this slipped into here under this name. Do you mind renaming to numberOfPointers here and on iOS for consistency?

Renaming is not a good idea since some people are already using this. I just borrowed the name that is used on iOS 🤔 If you really want to change it, it's better to set a getter for numberOfTouches in nativeEvent that returns numberOfPointers and warns that it's deprecated.

@tsapeta, I do not consider it as good motivation. Well, I don't suppose we need to care about fields which are not documented and appear on the only one platform
However, gonna wait with this change until @kmagiera opinion

Fields returned in the event are not documented at all, so certainly people are checking what it has and trying on their own. If someone has reported a problem, it means that someone is using it.
Also, we don't have a changelog where we could explain that change and how to fix it.

the lib is still in alpha so breaking changes is just sth people will have to deal with when upgrading. Let's make sure to communicate this change to people reporting the issue and also when the next release comes out. We don't have capacity at the moment to support backward compatibility.

Also, we now publish change logs in the releases section on GH

Also, we have event field definition in typescript def file, so lets make sure to update it there too

kmagiera · 2018-04-24T16:42:13Z

android/lib/src/main/java/com/swmansion/gesturehandler/GestureHandler.java

@@ -35,6 +35,7 @@
  private float mHitSlop[];

  private boolean mShouldCancelWhenOutside;
+  private int mNumberOfPointers = 1;


Doesn't matter too much but 0 would be a more appropriate initial value as there are no pointers when handler gets instantiated

kmagiera · 2018-04-24T16:42:54Z

android/lib/src/main/java/com/swmansion/gesturehandler/GestureHandler.java

@@ -161,6 +166,9 @@ public final void handle(MotionEvent event) {
      }
      return;
    }
+    if (mNumberOfPointers != event.getPointerCount()) {
+      mNumberOfPointers = event.getPointerCount();


You may want to move it up a bit as there is some logic that may cause the gesture to fail in which case it would emit an event with an incorrect number of pointers

kmagiera · 2018-04-25T06:12:23Z

android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerModule.java

@@ -370,6 +378,8 @@ public void extractEventData(RotationGestureHandler handler, WritableMap eventDa
      eventData.putDouble("anchorX", PixelUtil.toDIPFromPixel(handler.getAnchorX()));
      eventData.putDouble("anchorY", PixelUtil.toDIPFromPixel(handler.getAnchorY()));
      eventData.putDouble("velocity", handler.getVelocity());
+      eventData.putDouble("numberOfTouches", handler.getNumberOfPointers());


Also, I'm curious why did you skip fling, tap and long press handlers when adding this new parameter? Looks like we can add it directly to the base class's HandlerFactory.extractEventData and call super in all subclasses (you can use https://developer.android.com/reference/android/support/annotation/CallSuper.html to ensure that it gets called)

@kmagiera ,
Well, I wish to follow current iOS state and not to add this field where it was not before.
I found it also quite useles on fling as long as iOS requires exact number of pointers.
However, I will add it everywhere for consciousness

osdnk · 2018-04-25T10:43:58Z

I will add corresponing numberOfPointers on Tap on iOS when PR related to changes in Tap will be merged

kmagiera

Almost there. Added a few comments inline

kmagiera · 2018-04-26T06:25:08Z

ios/Handlers/RNFlingHandler.m

@@ -27,5 +27,11 @@ - (void)configure:(NSDictionary *)config
    }
 }

+- (RNGestureHandlerEventExtraData *)eventExtraData:(UISwipeGestureRecognizer *)recognizer


Note that this changes behavior for Fling (same for long press). They've previously been using base class implementation from RNGestureHandler.m:

It sends position and absolute position. You perhaps want to add "withNumberOfTouches" there instead

kmagiera · 2018-04-26T06:25:54Z

ios/Handlers/RNNativeViewHandler.m

@@ -99,21 +99,21 @@ - (void)handleTouchDown:(UIView *)sender forEvent:(UIEvent *)event

    [self sendEventsInState:RNGestureHandlerStateActive
             forViewWithTag:sender.reactTag
-              withExtraData:[RNGestureHandlerEventExtraData forPointerInside:YES]];
+              withExtraData:[RNGestureHandlerEventExtraData forPointerInside:YES withNumberOfTouches: _recognizer.numberOfTouches]];


nit:
withNumberOfTouches: _recognizer.numberOfTouches]];

should be:
withNumberOfTouches:_recognizer.numberOfTouches]];

(no space after colon)

Same applies below

kmagiera · 2018-04-26T06:26:50Z

ios/RNGestureHandlerEvents.h

-+ (RNGestureHandlerEventExtraData *)forPointerInside:(BOOL)pointerInside;
+ (RNGestureHandlerEventExtraData *)forPointerInside:(BOOL)pointerInside
+                                 withNumberOfTouches:(NSUInteger)numberOfTouches;
+ (RNGestureHandlerEventExtraData *)withNumberOfTouches:(NSUInteger)numberOfTouches;


This won't be necessary if you add withNumberOfTouches to the very first method and use it from base class RNGestureHandler

kmagiera · 2018-05-07T07:15:37Z

ios/Handlers/RNNativeViewHandler.m

@@ -99,21 +99,21 @@ - (void)handleTouchDown:(UIView *)sender forEvent:(UIEvent *)event

    [self sendEventsInState:RNGestureHandlerStateActive
             forViewWithTag:sender.reactTag
-              withExtraData:[RNGestureHandlerEventExtraData forPointerInside:YES]];
+              withExtraData:[RNGestureHandlerEventExtraData forPointerInside:YES withNumberOfTouches:_recognizer.numberOfTouches]];


I believe numberOfTouches should be extracted from event not _recognizer in this case

Or just stop sending numberOfPoiners for UIControl subclasses as it may not be that important

kmagiera · 2018-05-07T07:16:44Z

ios/RNGestureHandlerEvents.m

+                           @"numberOfPointers": @(numberOfTouches)}];
+}
+
+ (RNGestureHandlerEventExtraData *)withNumberOfTouches:(NSUInteger)numberOfTouches


kmagiera

🎉

Add max number of touches on Android

71482d8

kmagiera requested changes Apr 24, 2018

View reviewed changes

Change android max number of pointers to current number of pointers

7720454

osdnk changed the title ~~Add max number of touches on Android~~ Add number of touches on Android Apr 24, 2018

kmagiera requested changes Apr 24, 2018

View reviewed changes

Fix issues

37d0fc6

kmagiera reviewed Apr 25, 2018

View reviewed changes

Add number of pointers on missing android and iOS

2381880

osdnk changed the title ~~Add number of touches on Android~~ Add number of touches on Android and iOS if needed Apr 25, 2018

kmagiera requested changes Apr 26, 2018

View reviewed changes

I hope the last fix

fbb623d

kmagiera requested changes May 7, 2018

View reviewed changes

CR fixes

81920cc

kmagiera approved these changes May 7, 2018

View reviewed changes

kmagiera merged commit 6dca457 into master May 7, 2018

kmagiera deleted the number-of-touches-android branch May 7, 2018 08:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add number of touches on Android and iOS if needed #152

Add number of touches on Android and iOS if needed #152

osdnk commented Apr 23, 2018 •

edited

Loading

kmagiera left a comment

kmagiera Apr 24, 2018

osdnk Apr 24, 2018

kmagiera left a comment

kmagiera Apr 24, 2018

kmagiera Apr 24, 2018

tsapeta Apr 24, 2018

osdnk Apr 24, 2018 •

edited

Loading

tsapeta Apr 24, 2018 •

edited

Loading

kmagiera Apr 25, 2018

kmagiera Apr 25, 2018

kmagiera Apr 24, 2018

kmagiera Apr 24, 2018

kmagiera Apr 25, 2018

osdnk Apr 25, 2018

osdnk commented Apr 25, 2018

kmagiera left a comment

kmagiera Apr 26, 2018

kmagiera Apr 26, 2018

kmagiera Apr 26, 2018

kmagiera May 7, 2018

kmagiera May 7, 2018

kmagiera May 7, 2018

kmagiera left a comment

Add number of touches on Android and iOS if needed #152

Add number of touches on Android and iOS if needed #152

Conversation

osdnk commented Apr 23, 2018 • edited Loading

Motivation

Changes

kmagiera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmagiera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osdnk Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

tsapeta Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osdnk commented Apr 25, 2018

kmagiera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmagiera left a comment

Choose a reason for hiding this comment

osdnk commented Apr 23, 2018 •

edited

Loading

osdnk Apr 24, 2018 •

edited

Loading

tsapeta Apr 24, 2018 •

edited

Loading