Added basic double-tap, based off view click by cleverchuk. Addresses #1642 and #1576#1681
Added basic double-tap, based off view click by cleverchuk. Addresses #1642 and #1576#1681DavidGrath wants to merge 8 commits intoopen-telemetry:mainfrom
Conversation
|
Okay, the steps I took before opening this PR include running |
|
I'll make the relevant fixes and revert within 48 hours |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1681 +/- ##
==========================================
+ Coverage 62.09% 62.31% +0.21%
==========================================
Files 159 160 +1
Lines 3411 3460 +49
Branches 345 355 +10
==========================================
+ Hits 2118 2156 +38
- Misses 1199 1205 +6
- Partials 94 99 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| internal const val APP_SCREEN_DOUBLE_TAP_EVENT_NAME = "app.screen.doubletap" | ||
| internal const val VIEW_DOUBLE_TAP_EVENT_NAME = "app.widget.doubletap" |
There was a problem hiding this comment.
Could we not use a generic click/tap event name and use attribute to ie distinguish between the type ie single/double click
There was a problem hiding this comment.
Thanks for the review.
The idea is to eventually expand to other gestures including scroll and pinch, that's the reasoning behind me leaving single tap distinct from double tap
There was a problem hiding this comment.
I have no problem with scroll & pinch having dedicated name as they would also have different attributes. However in the case of clicks (single vs double) they would have the same attributes and the number of definitions triples when you consider that it could also be a left vs right click when using a mouse.
There was a problem hiding this comment.
Okay, I see it working out like this:
- Add the first source and class from
InputDevice.getSources()source as 2 attributes - Add
MotionEvent.getToolType()as an attribute - Add
MotionEvent.getButtonState()as an attribute if it's greater than 0 - Merge what I've done so far into the original view click and delete the new double tap folder
I'd be translating the returned integers into strings
What do you think?
There was a problem hiding this comment.
Oh, and also add click.type for either single or double
There was a problem hiding this comment.
I like that as otherwise we would have had so many different event names.
The question is attribute names. I am thinking:
hw.pointer.typefor the tool type as it is describing the hardware that was usedhw.pointer.buttonfor the button state with stylus_primary shortened to primary etc.hw.pointer.clicksfor how many clicks ie 1 or 2
There was a problem hiding this comment.
Okay, I assume we're only focusing on SOURCE_CLASS_POINTER since that's where gestures typically come from. I think it's good, although I'd prefer hardware in full instead of shortened.
There was a problem hiding this comment.
I'll try and get back to this on or before the 18th
There was a problem hiding this comment.
No worries, note hw.* is a defined namespace in semconv. 😉
There was a problem hiding this comment.
Ah. Thanks for pointing that out. Noted
…double tap both now rely on `GestureDetector`
|
Main points of discussion:
|
|
@thompson-tomo , please how do the changes look? |
| val buttonStateInt = motionEvent.buttonState | ||
| val toolTypeHasButtons = toolTypeInt == MotionEvent.TOOL_TYPE_MOUSE || toolTypeInt == MotionEvent.TOOL_TYPE_STYLUS | ||
| val isButtonPrimary = buttonStateInt == MotionEvent.BUTTON_PRIMARY || buttonStateInt == MotionEvent.BUTTON_STYLUS_PRIMARY | ||
| if(toolTypeHasButtons && !isButtonPrimary) { | ||
| return false | ||
| } | ||
| val buttonState = buttonStateToString(buttonStateInt) | ||
|
|
||
| if(buttonState != null) { | ||
| appEvent.setAttribute(HARDWARE_POINTER_BUTTON, buttonState) | ||
| } |
There was a problem hiding this comment.
Not sure if we need to restrict it to primary given we already have an attribute to indicate which button.
| val buttonStateInt = motionEvent.buttonState | |
| val toolTypeHasButtons = toolTypeInt == MotionEvent.TOOL_TYPE_MOUSE || toolTypeInt == MotionEvent.TOOL_TYPE_STYLUS | |
| val isButtonPrimary = buttonStateInt == MotionEvent.BUTTON_PRIMARY || buttonStateInt == MotionEvent.BUTTON_STYLUS_PRIMARY | |
| if(toolTypeHasButtons && !isButtonPrimary) { | |
| return false | |
| } | |
| val buttonState = buttonStateToString(buttonStateInt) | |
| if(buttonState != null) { | |
| appEvent.setAttribute(HARDWARE_POINTER_BUTTON, buttonState) | |
| } | |
| val toolTypeHasButtons = toolTypeInt == MotionEvent.TOOL_TYPE_MOUSE || toolTypeInt == MotionEvent.TOOL_TYPE_STYLUS | |
| val buttonState = null | |
| if(toolTypeHasButtons) { | |
| val buttonStateInt = motionEvent.buttonState | |
| buttonState = buttonStateToString(buttonStateInt) | |
| } | |
| if(buttonState != null) { | |
| appEvent.setAttribute(HARDWARE_POINTER_BUTTON, buttonState) | |
| } |
There was a problem hiding this comment.
Okay, I'll need to delete the corresponding test I wrote for this as well
There was a problem hiding this comment.
Sure, note the change should be replicated to single click.
| .logRecordBuilder() | ||
| .setEventName(name) | ||
|
|
||
| private fun createViewAttributes(view: View): Attributes { |
There was a problem hiding this comment.
Should we also be adding the hw.pointer.* attributes on her as well?
There was a problem hiding this comment.
I think it might not be necessary, since app.screen.click is always sent
There was a problem hiding this comment.
And it already has the same attributes
There was a problem hiding this comment.
That is true but I was thinking of the use case of where the collector is filtering events and might filter out app.click hence the view click event should be self sufficient.
cleverchuk
left a comment
There was a problem hiding this comment.
May need to standardize the new attributes in semconv otherwise LGTM!
| .setAttribute(HARDWARE_POINTER_CLICKS, 2) | ||
| .setAttribute(HARDWARE_POINTER_TYPE, toolType) | ||
|
|
||
| val toolTypeHasButtons = toolTypeInt == MotionEvent.TOOL_TYPE_MOUSE || toolTypeInt == MotionEvent.TOOL_TYPE_STYLUS |
There was a problem hiding this comment.
I would consider driving this and other information via an enum in ViewUtils as that will reduce the amount of logic in this class. Here's a non-exhaustive example of what I mean:
enum class TapEvent(
val motionEvent: Int,
val toolType: Int,
) {
val toolTypeDescription: String // derive from MotionEvent.TOOL_TYPE_*
val hasButtons: Boolean = toolTypeInt == MotionEvent.TOOL_TYPE_MOUSE || toolTypeInt == MotionEvent.TOOL_TYPE_STYLUS
}
There was a problem hiding this comment.
Thank you, I've applied the other suggestions. I'll look into this and give my feedback by Wednesday or Thursday
Co-authored-by: Jamie Lynch <fractalwrench@gmail.com>
The core difference between this and the original View Click is the use of a
GestureDetectorinstead of relying onMotionEventdirectly. As brought up in my comments on the issue, I'd like to know if it's desirable to add configurations to selectively track the double-tap event per-view via some form of allowlist. Then there's the issue of interplay between this and the originalview.click