-
-
Notifications
You must be signed in to change notification settings - Fork 472
perf(android): Hit-test gestures without getLocationOnScreen #5595
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
base: main
Are you sure you want to change the base?
Changes from all commits
85302a3
f59d814
fa294a2
0eb4cd6
f201931
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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; | ||
|
|
@@ -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. | ||
| */ | ||
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
| } | ||
|
|
||
| /** | ||
| * 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} | ||
|
|
@@ -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); | ||
|
|
@@ -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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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>() | ||
|
|
@@ -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)) | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇