Remove Gesture Handler limits on Android #2672

m-bert · 2023-11-09T08:49:42Z

Description

While looking at PRs I've stumbled upon this one. Turns out that SIMULTANEOUS_GESTURE_HANDLER_LIMIT does not refer to simultaneous gestures, but to all gesture handlers present in app. The upper limit shouldn't exist - this PR aims to remove it.

Test plan

Tested on example app.

tomekzaw

Looks good to me (although I haven't tested it)

android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt

j-piasecki · 2023-11-09T16:42:41Z

android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt

+  private val gestureHandlers = arrayListOf<GestureHandler<*>?>()
+  private val awaitingHandlers = arrayListOf<GestureHandler<*>?>()
+  private val preparedHandlers = arrayListOf<GestureHandler<*>?>()
+  private val handlersToCancel = arrayListOf<GestureHandler<*>?>()


At this point do we need to allow null values? It was nullable because the size of the array was constant to allow for nothing is here value. If we're going to use ArrayList we can simply remove things that shouldn't be there, no?

You're right. I left null possibility because of how removing handler worked before last commit, but now I think it should be fine (6957bb1)

But I would be grateful if you could check if that approach is correct (line 74)

I'm not sure what you mean, there's a closing bracket on line 74 😅

j-piasecki · 2023-11-14T18:38:40Z

android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt

      if (isFinished(handler.state) && !handler.isAwaiting) {
-        gestureHandlers[i] = null
-        shouldCleanEmptyCells = true
+        gestureHandlers.remove(handler)


Can we modify the collection while iterating over it?

I'm pretty sure that calling Collection.reversed() returns new collection - in that case it should be fine (see docs for context)

j-piasecki · 2023-11-14T18:44:22Z

android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt

+  private val gestureHandlers = arrayListOf<GestureHandler<*>?>()
+  private val awaitingHandlers = arrayListOf<GestureHandler<*>?>()
+  private val preparedHandlers = arrayListOf<GestureHandler<*>?>()
+  private val handlersToCancel = arrayListOf<GestureHandler<*>?>()


I'm not sure what you mean, there's a closing bracket on line 74 😅

j-piasecki · 2023-11-16T15:13:59Z

android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt

-      val otherHandler = gestureHandlers[i]!!
+    for (otherHandler in gestureHandlers) {
      if (shouldHandlerBeCancelledBy(otherHandler, handler)) {
-        handlersToCancel[toCancelCount++] = otherHandler
+        handlersToCancel.add(otherHandler)
      }
    }
-    for (i in toCancelCount - 1 downTo 0) {
-      handlersToCancel[i]!!.cancel()
+
+    for (otherHandler in handlersToCancel.reversed()) {
+      otherHandler.cancel()
    }

+    handlersToCancel.clear()


Do we need handlersToCancel at all here? I think you can iterate over gestureHandlers.asReversed() and just cancel relevant handlers inside the loop.

I agree - done in da2c421

j-piasecki

🚀

j-piasecki · 2023-11-17T11:02:43Z

android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt

@@ -160,19 +159,12 @@ class GestureHandlerOrchestrator(
      activationIndex = [email protected]++
    }

-    // Cancel all handlers that are required to be cancel upon current handler's activation
-    for (otherHandler in gestureHandlers) {
+    for (otherHandler in gestureHandlers.reversed()) {


Suggested change

for (otherHandler in gestureHandlers.reversed()) {

for (otherHandler in gestureHandlers.asReversed()) {

I don't think the list is being mutated here, so we shouldn't need a copy.

## Description While working on removing [gesture handlers limit on android](#2672) I've though that we can also remove `handlersToCancel` array on web. ## Test plan Tested on example app.

## Description A while ago we decided to remove limit of gesture handlers on android (#2672). We missed one thing - `awaitngHandlers` can be modified while iterating over its items (inside [this loop](https://github.com/software-mansion/react-native-gesture-handler/blob/2e5df1c9a8595929b9879dc1246a7fd78de0549a/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt#L105)). This resulted in `ConcurrentModificationException`. This PR changes logic so that now we iterate over copy of `awaitingHandlers`, avoiding modification of collection that we iterate through. Even though it looks like it changes a lot, the only difference beside adding copy is changing ```jsx for(otherHandler in currentlyAwaitingHandlers){ if (shouldHandlerWaitForOther(otherHandler, handler)) { ... } } ``` into ```jsx for(otherHandler in currentlyAwaitingHandlers){ if (!shouldHandlerWaitForOther(otherHandler, handler)) { continue } ... } ``` to remove one level of nested `ifs`. ## Test plan <details> <summary> Modified code from #2739 </summary> ```jsx import React from 'react'; import { ScrollView, StyleSheet, Text, View } from 'react-native'; import { Gesture, GestureDetector } from 'react-native-gesture-handler'; import Animated from 'react-native-reanimated'; const Showcase = () => { const [lastActions, setLastActions] = React.useState([]); const addAction = (action) => { setLastActions((actions) => { const date = new Date(); const time = date.toLocaleTimeString(); if (actions.length >= 3) { actions.pop(); } return [action + ': ' + time, ...actions]; }); }; const longPressGesture = Gesture.LongPress() .onStart(() => { addAction('long press'); }) .runOnJS(true); const doubleTapGesture = Gesture.Tap() .numberOfTaps(2) .onStart(() => { addAction('double tap'); }) .runOnJS(true); const gesture = Gesture.Race(doubleTapGesture, longPressGesture); return ( <View style={{ flex: 1, backgroundColor: 'white', marginHorizontal: 20, marginVertical: 60, }}> <ScrollView contentContainerStyle={{ gap: 20 }}> <GestureDetector gesture={gesture}> <Animated.View style={styles.outer}> <Blue onAddAction={addAction} /> </Animated.View> </GestureDetector> <View> {lastActions.map((action, index) => ( <Text key={index}> {index} - {action} </Text> ))} </View> <View style={{ gap: 20 }}> <Text> <Bold>DoubleTap</Bold>: on all rectangles - should print "double tap" - no tap gesture should be recognized ✅ </Text> <Text> <Bold>LongPress</Bold>: on all rectangles - should print "long press" - no tap gesture should be recognized ✅ </Text> <Text> <Bold>Tap</Bold>: on red - should do nothing ✅ </Text> <Text> <Bold>Tap</Bold>: on blue - should print "Blue: tap" ✅ </Text> <Text> <Bold>Tap</Bold>: on orange - should print "Orange: tap" ⛔️ - Crashes on Android </Text> </View> </ScrollView> </View> ); }; export default Showcase; const Blue = ({ onAddAction }) => { const tapGesture = Gesture.Tap() .onStart(() => { onAddAction('Blue: tap'); }) .runOnJS(true); const doubleTapGesture = Gesture.Tap() .numberOfTaps(2) .onStart(() => { onAddAction('double tap'); }) .runOnJS(true); const composedGesture = Gesture.Exclusive(doubleTapGesture, tapGesture); return ( <GestureDetector gesture={composedGesture}> <Animated.View style={styles.blue}> <Orange onAddAction={onAddAction} /> </Animated.View> </GestureDetector> ); }; const Orange = ({ onAddAction }) => { const tapGesture = Gesture.Tap() .onStart(() => { onAddAction('Orange: tap'); }) .runOnJS(true); const doubleTapGesture = Gesture.Tap() .numberOfTaps(2) .onStart(() => { onAddAction('double tap'); }) .runOnJS(true); return ( <GestureDetector gesture={Gesture.Exclusive(doubleTapGesture, tapGesture)}> <Animated.View style={styles.orange}></Animated.View> </GestureDetector> ); }; const Bold = ({ children }) => { return <Text style={{ fontWeight: 'bold' }}>{children}</Text>; }; const styles = StyleSheet.create({ outer: { padding: 20, backgroundColor: 'red', width: 300, height: 200, }, blue: { width: '80%', height: '80%', backgroundColor: 'blue', flexDirection: 'row', flexWrap: 'wrap', }, orange: { margin: 5, width: '50%', height: '50%', backgroundColor: 'orange', }, }); ``` </details> <details> <summary> Smaller example </summary> ```jsx import React from 'react'; import { StyleSheet, View } from 'react-native'; import { Gesture, GestureDetector } from 'react-native-gesture-handler'; export default function App() { const t3 = Gesture.Tap() .numberOfTaps(3) .onEnd(() => console.log('t3')); const t1_1 = Gesture.Tap().onEnd(() => console.log('t1_1')); const t1_2 = Gesture.Tap().onEnd(() => console.log('t1_2')); const g = Gesture.Exclusive(t3, t1_1, t1_2); return ( <GestureDetector gesture={g}> <View style={styles.box} /> </GestureDetector> ); } const styles = StyleSheet.create({ box: { width: 250, height: 250, backgroundColor: 'crimson', }, }); ``` </details>

Change implementation to ArrayList

7e3cc86

tomekzaw approved these changes Nov 9, 2023

View reviewed changes

m-bert marked this pull request as ready for review November 9, 2023 14:54

m-bert requested a review from j-piasecki November 9, 2023 14:54

j-piasecki reviewed Nov 9, 2023

View reviewed changes

m-bert added 2 commits November 14, 2023 08:44

Some changes in orchestrator

6957bb1

Remove compactHandlersIf

93c8fb7

m-bert requested a review from j-piasecki November 14, 2023 08:09

m-bert mentioned this pull request Nov 14, 2023

Raise limit on simultaneous gesture handlers from 20 to 50 #2664

Closed

piaskowyk approved these changes Nov 14, 2023

View reviewed changes

j-piasecki reviewed Nov 14, 2023

View reviewed changes

Change how handlers are removed

892c095

m-bert requested a review from j-piasecki November 15, 2023 14:05

j-piasecki reviewed Nov 16, 2023

View reviewed changes

Remove handlersToCancel

da2c421

j-piasecki approved these changes Nov 17, 2023

View reviewed changes

reversed to asReversed

3d3d241

m-bert merged commit 25ba4b8 into main Nov 20, 2023
3 checks passed

m-bert deleted the @mbert/remove-simultaneous-limit branch November 20, 2023 07:15

m-bert mentioned this pull request Nov 20, 2023

Remove handlersToCancel on web. #2679

Merged

m-bert mentioned this pull request Feb 7, 2024

Fix ConcurrentModificationException. #2750

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Gesture Handler limits on Android #2672

Remove Gesture Handler limits on Android #2672

m-bert commented Nov 9, 2023

tomekzaw left a comment

j-piasecki Nov 9, 2023

m-bert Nov 14, 2023

m-bert Nov 14, 2023

j-piasecki Nov 14, 2023

j-piasecki Nov 14, 2023

m-bert Nov 15, 2023

j-piasecki Nov 14, 2023

j-piasecki Nov 16, 2023

m-bert Nov 16, 2023

j-piasecki left a comment

j-piasecki Nov 17, 2023

	for (otherHandler in gestureHandlers.reversed()) {
	for (otherHandler in gestureHandlers.asReversed()) {

Remove Gesture Handler limits on Android #2672

Remove Gesture Handler limits on Android #2672

Conversation

m-bert commented Nov 9, 2023

Description

Test plan

tomekzaw 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-piasecki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment