Capture click events for non-compose views#953
Conversation
cleverchuk
force-pushed
the
cc/view-click
branch
from
3dda5ad to
5503e0d
Compare
| return !(x < vx || x > vx + w || y < vy || y > vy + h) | ||
| } | ||
|
|
||
| private fun isJetpackComposeView(view: View): Boolean = view::class.java.name.startsWith("androidx.compose.ui.platform.ComposeView") |
There was a problem hiding this comment.
if minify is enabled, this might fail? is androidx.compose.ui.platform.ComposeView minified or an exception (Added to proguard etc?)
There was a problem hiding this comment.
There was a problem hiding this comment.
yea, the test will fail and result in traversing compose view tree. however, i don't see how an exception will result here.
There was a problem hiding this comment.
that's good, let's just add this to the consumer-rules exception then so we can validate isJetpackComposeView even if minify is enabled?
There was a problem hiding this comment.
Not sure, I understand what you're asking. However, it sounds like you're saying I should create a consumer rules with this as the content?
There was a problem hiding this comment.
yes otherwise androidx.compose.ui.platform.ComposeView is gonna be minified and this condition will always return false
There was a problem hiding this comment.
What if there's another class that implements AbstractComposeView directly?
There was a problem hiding this comment.
@bidetofevil it looks like that doesn't exist in the Android compose framework yet and we can address them when that changes?
| builder.put(xCoordinateAttr, view.x.toDouble()) | ||
| builder.put(yCoordinateAttr, view.y.toDouble()) |
There was a problem hiding this comment.
should we also call getLocationInWindow or getLocationOnScreen here?
There was a problem hiding this comment.
i don't think it's necessary here because we're not doing bound checks here.
There was a problem hiding this comment.
ok, i'm not sure if view.x and view.y are relative to the parent or absolute to the window, if its relative to the parent, you'd need getLocationInWindow right otherwise the position will be wrong
There was a problem hiding this comment.
From what i understand from the doc, it's absolute.
| return target | ||
| } | ||
|
|
||
| private fun isValidClickTarget(view: View): Boolean = view.isClickable && view.isVisible |
There was a problem hiding this comment.
this can be a bit more clever. not sure if needed though.
eg https://github.com/PostHog/posthog-android/blob/d232586ce6b5043d716f108d06316d8de4714c31/posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt#L479-L520
There was a problem hiding this comment.
no need for that.
| ) { | ||
| private var windowRef: WeakReference<Window>? = null | ||
|
|
||
| private val viewCoordinates = IntArray(2) |
There was a problem hiding this comment.
The underlying coordinates are Longs - why not go with a Pair<Long, Long>?
There was a problem hiding this comment.
view.getLocationInWindow API demands it.
| import android.view.Window.Callback | ||
| import androidx.annotation.RequiresApi | ||
|
|
||
| class WindowCallbackWrapper( |
There was a problem hiding this comment.
You may just be able to annotate the class rather than the methods so it'll detect the creation
There was a problem hiding this comment.
I remember trying that and it not appeasing the sniffer 🤷♂️
bidetofevil
left a comment
bidetofevil
left a comment
There was a problem hiding this comment.
LGTM. Some non-blocking comments, but the one about using a data structure that stores Longs instead of Ints for the coordinates is probably a good idea to do before merging.
breedx-splk
left a comment
breedx-splk
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
6 participants
Fixes #930
Adds
android.view.Viewclick capture instrumentation. This instrumentation uses theWindow.CallbackAPI to intercept window motion events. During the interception, it walks the view tree to figure out which view was clicked by doing a boundary overlap check of the each view position against the touch position.ComposeViewaren't eligible for click capturing in this approach.