Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

### Performance Improvements

- Speed up touch gesture target detection on deeply nested view hierarchies by hit-testing in local coordinates instead of calling `getLocationOnScreen` per view ([#5595](https://github.com/getsentry/sentry-java/pull/5595))

## 8.45.0

### Features
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
package io.sentry.android.core.internal.gestures;

import android.content.res.Resources;
import android.graphics.Matrix;
import android.view.MotionEvent;
import android.view.View;
import android.view.ViewGroup;
import io.sentry.android.core.SentryAndroidOptions;
import io.sentry.internal.gestures.GestureTargetLocator;
import io.sentry.internal.gestures.UiElement;
import java.util.LinkedList;
import java.util.ArrayDeque;
import java.util.List;
import java.util.Queue;
import org.jetbrains.annotations.ApiStatus;
Expand All @@ -17,39 +18,62 @@
@ApiStatus.Internal
public final class ViewUtils {

private static final int[] coordinates = new int[2];

/**
* Verifies if the given touch coordinates are within the bounds of the given view.
* Verifies if the given touch coordinates, expressed in the view's own local coordinate space,
* are within the bounds of the given view.
*
* @param view the view to check if the touch coordinates are within its bounds
* @param x - the x coordinate of a {@link MotionEvent}
* @param y - the y coordinate of {@link MotionEvent}
* @param localX - the x coordinate of the touch, relative to the view's top-left corner
* @param localY - the y coordinate of the touch, relative to the view's top-left corner
* @return true if the touch coordinates are within the bounds of the view, false otherwise
*/
private static boolean touchWithinBounds(
final @Nullable View view, final float x, final float y) {
final @Nullable View view, final float localX, final float localY) {
if (view == null) {
return false;
}

view.getLocationOnScreen(coordinates);
int vx = coordinates[0];
int vy = coordinates[1];
final int w = view.getWidth();
final int h = view.getHeight();

int w = view.getWidth();
int h = view.getHeight();
return !(localX < 0 || localX > w || localY < 0 || localY > h);
}

return !(x < vx || x > vx + w || y < vy || y > vy + h);
/**
* Maps a touch point expressed in the parent's local coordinate space into the child's local
* coordinate space. This mirrors how {@link ViewGroup} dispatches touch events to its children
* and lets us hit-test the whole tree with a single downward traversal, instead of calling {@link
* View#getLocationOnScreen(int[])} (which walks up to the root) for every view.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🥇

*/
private static @NotNull ViewWithLocation mapToChild(
final @NotNull View child,
final float parentX,
final float parentY,
final int parentScrollX,
final int parentScrollY) {
float childX = parentX + parentScrollX - child.getLeft();
float childY = parentY + parentScrollY - child.getTop();

final @Nullable Matrix matrix = child.getMatrix();
if (matrix != null && !matrix.isIdentity()) {
final Matrix inverse = new Matrix();
if (matrix.invert(inverse)) {
final float[] point = {childX, childY};
inverse.mapPoints(point);
childX = point[0];
childY = point[1];
}
}
return new ViewWithLocation(child, childX, childY);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i guess the next step to improve this could be to have a pool of reusable ViewWithLocations to reduce the number of allocations? but that could be done separately (if it makes sense at all)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i always dreamed of the day I would need to use a pool to fix a real performance problem. it's a good point. can add a ticket for a follow up investigation to see if this makes sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Claude says:

❯ Would it make sense to add a ticket for a follow up to add a pool for the ViewWithLocation objects?

⏺ Honestly, I'd lean against it — and I wouldn't file the ticket speculatively.

Reasoning:

  • These are textbook ephemeral allocations. Each ViewWithLocation is ~24 bytes (one ref + two floats) and dies immediately after the
    traversal. ART's young-gen allocates these with a bump pointer (TLAB) and collects them almost for free. Pooling trades that for a managed
    pool with reset/lifecycle logic, thread-safety questions, and live objects the GC now has to keep scanning — on Android, hand-rolled pools for
    small short-lived objects often net-lose.
  • The count is bounded and small — one per view on the hit-test path, and the traversal prunes whole subtrees as soon as a view doesn't
    contain the touch, so it's nowhere near the full tree. This fires at interaction frequency (taps/scrolls), not per-frame render.
  • The real win in this PR was removing the per-view getLocationOnScreen walk (O(depth) up to the root, each call). The wrapper allocation is
    noise next to that.

}

/**
* Finds a target view, that has been selected/clicked by the given coordinates x and y and the
* given {@code viewTargetSelector}.
*
* @param decorView - the root view of this window
* @param x - the x coordinate of a {@link MotionEvent}
* @param y - the y coordinate of {@link MotionEvent}
* @param x - the x coordinate of a {@link MotionEvent}, relative to the decor view
* @param y - the y coordinate of {@link MotionEvent}, relative to the decor view
* @param targetType - the type of target to find
* @return the {@link View} that contains the touch coordinates and complements the {@code
* viewTargetSelector}
Expand All @@ -62,25 +86,35 @@ private static boolean touchWithinBounds(
final UiElement.Type targetType) {

final List<GestureTargetLocator> locators = options.getGestureTargetLocators();
final Queue<View> queue = new LinkedList<>();
queue.add(decorView);
final Queue<ViewWithLocation> queue = new ArrayDeque<>();
// The touch coordinates from the MotionEvent are already relative to the decor view, i.e. in
// its local coordinate space.
queue.add(new ViewWithLocation(decorView, x, y));

@Nullable UiElement target = null;
while (queue.size() > 0) {
final View view = queue.poll();
while (!queue.isEmpty()) {
final ViewWithLocation current = queue.poll();
final View view = current.view;

if (!touchWithinBounds(view, x, y)) {
if (!touchWithinBounds(view, current.x, current.y)) {
// if the touch is not hitting the view, skip traversal of its children
continue;
}

if (view instanceof ViewGroup) {
final ViewGroup viewGroup = (ViewGroup) view;
final int scrollX = viewGroup.getScrollX();
final int scrollY = viewGroup.getScrollY();
for (int i = 0; i < viewGroup.getChildCount(); i++) {
queue.add(viewGroup.getChildAt(i));
final @Nullable View child = viewGroup.getChildAt(i);
if (child != null) {
queue.add(mapToChild(child, current.x, current.y, scrollX, scrollY));
}
}
}

// Locators receive the original decor-view-relative coordinates, as the Compose locator
// hit-tests against window coordinates.
for (int i = 0; i < locators.size(); i++) {
final GestureTargetLocator locator = locators.get(i);
final @Nullable UiElement newTarget = locator.locate(view, x, y, targetType);
Expand All @@ -96,6 +130,18 @@ private static boolean touchWithinBounds(
return target;
}

private static final class ViewWithLocation {
final @NotNull View view;
final float x;
final float y;

ViewWithLocation(final @NotNull View view, final float x, final float y) {
this.view = view;
this.x = x;
this.y = y;
}
}

/**
* Retrieves the human-readable view id based on {@code view.getContext().getResources()}, falls
* back to a hexadecimal id representation in case the view id is not available in the resources.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@ import android.content.res.Resources
import android.view.MotionEvent
import android.view.View
import android.view.Window
import kotlin.math.abs
import org.mockito.kotlin.any
import org.mockito.kotlin.doAnswer
import org.mockito.kotlin.doReturn
import org.mockito.kotlin.mock
import org.mockito.kotlin.whenever
Expand Down Expand Up @@ -35,31 +32,17 @@ internal inline fun <reified T : View> mockView(
context: Context? = null,
finalize: (T) -> Unit = {},
): T {
val coordinates = IntArray(2)
if (!touchWithinBounds) {
coordinates[0] = (event.x).toInt() + 10
coordinates[1] = (event.y).toInt() + 10
} else {
coordinates[0] = (event.x).toInt() - 10
coordinates[1] = (event.y).toInt() - 10
}
// The decor-view-relative touch point used in these tests is (0, 0), and child views are mocked
// at offset (0, 0), so the point reaches every view unchanged. A view therefore contains the
// touch iff its width/height are non-negative; a negative size marks the touch as outside.
val size = if (touchWithinBounds) 10 else -1
val mockView: T = mock {
whenever(it.id).thenReturn(id)
whenever(it.context).thenReturn(context)
whenever(it.isClickable).thenReturn(clickable)
whenever(it.visibility).thenReturn(if (visible) View.VISIBLE else View.GONE)

whenever(it.getLocationOnScreen(any())).doAnswer {
val array = it.arguments[0] as IntArray
array[0] = coordinates[0]
array[1] = coordinates[1]
null
}

val diffPosX = abs(event.x - coordinates[0]).toInt()
val diffPosY = abs(event.y - coordinates[1]).toInt()
whenever(it.width).thenReturn(diffPosX + 10)
whenever(it.height).thenReturn(diffPosY + 10)
whenever(it.width).thenReturn(size)
whenever(it.height).thenReturn(size)

finalize(this.mock)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,19 @@ package io.sentry.android.core.internal.gestures

import android.content.Context
import android.content.res.Resources
import android.graphics.Matrix
import android.view.View
import android.view.ViewGroup
import androidx.test.ext.junit.runners.AndroidJUnit4
import io.sentry.android.core.SentryAndroidOptions
import io.sentry.internal.gestures.UiElement
import io.sentry.util.LazyEvaluator
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFailsWith
import kotlin.test.assertNotNull
import kotlin.test.assertNull
import org.junit.runner.RunWith
import org.mockito.kotlin.any
import org.mockito.kotlin.doReturn
import org.mockito.kotlin.doThrow
Expand All @@ -14,12 +23,13 @@ import org.mockito.kotlin.never
import org.mockito.kotlin.verify
import org.mockito.kotlin.whenever

@RunWith(AndroidJUnit4::class)
class ViewUtilsTest {
@Test
fun `getResourceId returns resourceId when available`() {
val view =
mock<View> {
whenever(it.id).doReturn(View.generateViewId())
whenever(it.id).doReturn(0x7f010001)

val context = mock<Context>()
val resources = mock<Resources>()
Expand Down Expand Up @@ -80,6 +90,94 @@ class ViewUtilsTest {
verify(context, never()).resources
}

@Test
fun `findTarget hit-tests children in their own local coordinate space`() {
val child = clickableChild()
val decorView =
mock<ViewGroup> {
whenever(it.width).thenReturn(1000)
whenever(it.height).thenReturn(1000)
whenever(it.childCount).thenReturn(1)
whenever(it.getChildAt(0)).thenReturn(child)
}
val options = optionsWithViewLocator()

// (120, 220) maps to (20, 20) in the child's space -> inside its 50x50 bounds.
assertNotNull(ViewUtils.findTarget(options, decorView, 120f, 220f, UiElement.Type.CLICKABLE))

// (90, 220) maps to (-10, 20) in the child's space -> outside, despite being inside the decor.
assertNull(ViewUtils.findTarget(options, decorView, 90f, 220f, UiElement.Type.CLICKABLE))
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

l: Nice! Beyond covering the top/left path, might be worth adding quick cases for parent scrollX/scrollY and a non-identity child matrix. The gesture listener mocks won't catch regressions there and they'd cover the other two mapToChild branches.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added that test below! good point

@Test
fun `findTarget accounts for parent scroll when mapping into a child`() {
val child = clickableChild()
val decorView =
mock<ViewGroup> {
whenever(it.width).thenReturn(1000)
whenever(it.height).thenReturn(1000)
whenever(it.scrollX).thenReturn(30)
whenever(it.scrollY).thenReturn(40)
whenever(it.childCount).thenReturn(1)
whenever(it.getChildAt(0)).thenReturn(child)
}
val options = optionsWithViewLocator()

// With scroll (30, 40), (90, 180) maps to (90 + 30 - 100, 180 + 40 - 200) = (20, 20) -> inside.
assertNotNull(ViewUtils.findTarget(options, decorView, 90f, 180f, UiElement.Type.CLICKABLE))

// The same point without accounting for scroll would map to (-10, -20) -> outside the child.
assertNull(ViewUtils.findTarget(options, decorView, 50f, 140f, UiElement.Type.CLICKABLE))
}

@Test
fun `findTarget applies the inverse of a non-identity child matrix`() {
// The child is visually translated by (40, 40) within its parent, so a parent-space point is
// mapped back by (-40, -40) to reach the child's own coordinate space.
val matrix = Matrix().apply { setTranslate(40f, 40f) }
val child = clickableChild { whenever(it.matrix).thenReturn(matrix) }
val decorView =
mock<ViewGroup> {
whenever(it.width).thenReturn(1000)
whenever(it.height).thenReturn(1000)
whenever(it.childCount).thenReturn(1)
whenever(it.getChildAt(0)).thenReturn(child)
}
val options = optionsWithViewLocator()

// (180, 280) lands at (80, 80) before the matrix (outside 50x50), but the inverse pulls it to
// (40, 40) -> inside.
assertNotNull(ViewUtils.findTarget(options, decorView, 180f, 280f, UiElement.Type.CLICKABLE))

// (130, 230) lands at (30, 30) before the matrix (inside), but the inverse pushes it to
// (-10, -10) -> outside.
assertNull(ViewUtils.findTarget(options, decorView, 130f, 230f, UiElement.Type.CLICKABLE))
}

// A clickable child positioned at (100, 200) within its parent, 50x50 in size.
private fun clickableChild(finalize: (View) -> Unit = {}): View {
val context = mock<Context>()
val resources = mock<Resources>()
whenever(context.resources).thenReturn(resources)
whenever(resources.getResourceEntryName(any())).thenReturn("child")
return mock {
whenever(it.id).thenReturn(0x7f010001)
whenever(it.context).thenReturn(context)
whenever(it.isClickable).thenReturn(true)
whenever(it.visibility).thenReturn(View.VISIBLE)
whenever(it.left).thenReturn(100)
whenever(it.top).thenReturn(200)
whenever(it.width).thenReturn(50)
whenever(it.height).thenReturn(50)
finalize(this.mock)
}
}

private fun optionsWithViewLocator(): SentryAndroidOptions =
SentryAndroidOptions().apply {
gestureTargetLocators = listOf(AndroidViewGestureTargetLocator(LazyEvaluator { true }))
}

@Test
fun `getResourceIdWithFallback falls back to hexadecimal id when resource not found`() {
val view =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import io.sentry.compose.boundsInWindow
import io.sentry.internal.gestures.GestureTargetLocator
import io.sentry.internal.gestures.UiElement
import io.sentry.util.AutoClosableReentrantLock
import java.util.LinkedList
import java.util.ArrayDeque
import java.util.Queue

@OptIn(InternalComposeUiApi::class)
Expand Down Expand Up @@ -45,7 +45,7 @@ public class ComposeGestureTargetLocator(private val logger: ILogger) : GestureT
val rootLayoutNode = root.root

// Pair<Node, ParentTag>
val queue: Queue<Pair<LayoutNode, String?>> = LinkedList()
val queue: Queue<Pair<LayoutNode, String?>> = ArrayDeque()
queue.add(Pair(rootLayoutNode, null))

// the final tag to return, only relevant for clicks
Expand Down Expand Up @@ -92,7 +92,10 @@ public class ComposeGestureTargetLocator(private val logger: ILogger) : GestureT
}
}
}
queue.addAll(node.zSortedChildren.asMutableList().map { Pair(it, tag) })
val children = node.zSortedChildren.asMutableList()
for (index in children.indices) {
queue.add(Pair(children[index], tag))
}
}
}

Expand Down
Loading