Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow minDurationMs=0 in LongPressGestureHandler #1216

Merged
merged 2 commits into from
Nov 17, 2020

Conversation

jacobp100
Copy link
Contributor

@jacobp100 jacobp100 commented Oct 21, 2020

Description

For scrub gestures (on iOS at least), I used a LongPressGestureHandler with a minDurationMs set to 0 so it activates immediately. However, it seems a bit flaky on Android. This seems to fix it.

Test plan

/*
To test this change run example on android, click anywhere on the screen and notice that square doesn't change color. 
Now, move your finger a bit and you'll notice that gesture is recognized.
*/
import React from 'react';
import { AppRegistry, View, useWindowDimensions } from 'react-native';
import { LongPressGestureHandler, State } from 'react-native-gesture-handler';

const Example = () => {
  const { width, height } = useWindowDimensions();
  const [x, setX] = React.useState(undefined);
  const [y, setY] = React.useState(undefined);
  const yRows = 8;
  const xRows = 4;

  const backgroundContent = Array.from(new Array(yRows), (_, yPos) => (
    <View key={yPos} style={{ flex: 1, flexDirection: 'row' }}>
      {Array.from(new Array(xRows), (__, xPos) => {
        const isActive = xPos === x && yPos === y;
        const backgroundColor = isActive ? '#eee' : 'pink';

        return (
          <View key={xPos} style={{ flex: 1, backgroundColor, margin: 2 }} />
        );
      })}
    </View>
  ));

  return (
    <LongPressGestureHandler
      onGestureEvent={e => {
        setX(Math.trunc((e.nativeEvent.x * xRows) / width));
        setY(Math.trunc((e.nativeEvent.y * yRows) / height));
      }}
      onHandlerStateChange={e => {
        if (e.nativeEvent.state === State.END) {
          setX(undefined);
          setY(undefined);
        }
      }}
      minDurationMs={0}
      maxDist={1e9}
      shouldCancelWhenOutside={false}>
      <View style={{ flex: 1 }}>{backgroundContent}</View>
    </LongPressGestureHandler>
  );
};

AppRegistry.registerComponent('Example', () => Example);

@jakub-gonet
Copy link
Member

Could you please provide some small example code to test this change? GIFs with gesture you want to implement would be really appreciated.

@jacobp100
Copy link
Contributor Author

jacobp100 commented Oct 22, 2020

Sure! I've attached a video of the keyboard in the app TechniCalc.

You press your finger down on the the number 7 and it highlights that key immediately (minDurationMs = 0). You can also move your finger onto other keys, and it highlights those instead. When you release, it inputs the highlighted key.

The gesture is applied to the whole keyboard rather than each individual key.

ezgif-4-2123783d0ee6

@jakub-gonet
Copy link
Member

Thanks for the GIF. Could you add a code example as well? I want to test this change locally.

@jacobp100
Copy link
Contributor Author

Put a small demo that works similar to that keyboard here - you can clone the repo if that helps

Without this change, the square does not light up on Android until you move your finger a bit. With the change, it lights the square immediately (like on iOS)

@jakub-gonet
Copy link
Member

Thanks for example. I modified it a bit to make testing easier (colored panes, less of them, changed | 0 to Math.truncate):

/*
To test this change run example on android, click anywhere on the screen and notice that square doesn't change color. 
Now, move your finger a bit and you'll notice that gesture is recognized.
*/
import React from 'react';
import { AppRegistry, View, useWindowDimensions } from 'react-native';
import { LongPressGestureHandler, State } from 'react-native-gesture-handler';

const Example = () => {
  const { width, height } = useWindowDimensions();
  const [x, setX] = React.useState(undefined);
  const [y, setY] = React.useState(undefined);
  const yRows = 8;
  const xRows = 4;

  const backgroundContent = Array.from(new Array(yRows), (_, yPos) => (
    <View key={yPos} style={{ flex: 1, flexDirection: 'row' }}>
      {Array.from(new Array(xRows), (__, xPos) => {
        const isActive = xPos === x && yPos === y;
        const backgroundColor = isActive ? '#eee' : 'pink';

        return (
          <View key={xPos} style={{ flex: 1, backgroundColor, margin: 2 }} />
        );
      })}
    </View>
  ));

  return (
    <LongPressGestureHandler
      onGestureEvent={e => {
        setX(Math.trunc((e.nativeEvent.x * xRows) / width));
        setY(Math.trunc((e.nativeEvent.y * yRows) / height));
      }}
      onHandlerStateChange={e => {
        if (e.nativeEvent.state === State.END) {
          setX(undefined);
          setY(undefined);
        }
      }}
      minDurationMs={0}
      maxDist={1e9}
      shouldCancelWhenOutside={false}>
      <View style={{ flex: 1 }}>{backgroundContent}</View>
    </LongPressGestureHandler>
  );
};

AppRegistry.registerComponent('Example', () => Example);

but I'm not convinced this is the right use-case for LongPressGestureHandler - this is basically a panning gesture and using maxDist={1e9} is showing that you're using a hack here.

@jacobp100
Copy link
Contributor Author

jacobp100 commented Oct 23, 2020

@jakub-gonet I don't think a pan gesture is the right choice here, because you'd have to move your finger a bit for it to activate. This gesture needs to activate the moment your finger touches the screen, and send an end event if you tap (without moving your finger in the process)

@jacobp100
Copy link
Contributor Author

@jakub-gonet Did you have suggestions on how you would handle this gesture?

@jakub-gonet
Copy link
Member

This use case should be done by using pan handler, but we may have a bug in PanGestureHandler with maxDist set to 0. IMO it should activate immediately and you should be able to extract x and y from its event data. I'll discuss this internally and create an issue if that's what we want.

I'll also bring up this PR, it's quite feasible to allow using minDurationMs=0.
Could you please change else to else if(mMinDurationMs == 0)? Current code doesn't activate on negative values and your change would activate the handler immediately in that situation.

@jakub-gonet jakub-gonet self-assigned this Nov 6, 2020
@jacobp100
Copy link
Contributor Author

@jakub-gonet Sorry about the delay. I've done this now

Copy link
Member

@jakub-gonet jakub-gonet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jakub-gonet jakub-gonet merged commit 19cddc5 into software-mansion:master Nov 17, 2020
@jacobp100 jacobp100 deleted the patch-1 branch November 17, 2020 15:24
braincore pushed a commit to braincore/react-native-gesture-handler that referenced this pull request Mar 4, 2021
## Description

For scrub gestures (on iOS at least), I used a `LongPressGestureHandler` with a `minDurationMs` set to `0` so it activates immediately. However, it seems a bit flaky on Android. This seems to fix it.

## Test plan

```jsx
/*
To test this change run example on android, click anywhere on the screen and notice that square doesn't change color. 
Now, move your finger a bit and you'll notice that gesture is recognized.
*/
import React from 'react';
import { AppRegistry, View, useWindowDimensions } from 'react-native';
import { LongPressGestureHandler, State } from 'react-native-gesture-handler';

const Example = () => {
  const { width, height } = useWindowDimensions();
  const [x, setX] = React.useState(undefined);
  const [y, setY] = React.useState(undefined);
  const yRows = 8;
  const xRows = 4;

  const backgroundContent = Array.from(new Array(yRows), (_, yPos) => (
    <View key={yPos} style={{ flex: 1, flexDirection: 'row' }}>
      {Array.from(new Array(xRows), (__, xPos) => {
        const isActive = xPos === x && yPos === y;
        const backgroundColor = isActive ? '#eee' : 'pink';

        return (
          <View key={xPos} style={{ flex: 1, backgroundColor, margin: 2 }} />
        );
      })}
    </View>
  ));

  return (
    <LongPressGestureHandler
      onGestureEvent={e => {
        setX(Math.trunc((e.nativeEvent.x * xRows) / width));
        setY(Math.trunc((e.nativeEvent.y * yRows) / height));
      }}
      onHandlerStateChange={e => {
        if (e.nativeEvent.state === State.END) {
          setX(undefined);
          setY(undefined);
        }
      }}
      minDurationMs={0}
      maxDist={1e9}
      shouldCancelWhenOutside={false}>
      <View style={{ flex: 1 }}>{backgroundContent}</View>
    </LongPressGestureHandler>
  );
};

AppRegistry.registerComponent('Example', () => Example);
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants