Added long press to click detection

[DavidGrath]

References #1576

@thompson-tomo , are these new attributes fine by you?

[codecov]

Codecov Report

❌ Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.77%. Comparing base (a26eda2) to head (54688f1).
⚠️ Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
...trumentation/view/click/ViewClickEventGenerator.kt	89.47%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1741      +/-   ##
==========================================
- Coverage   62.06%   61.77%   -0.30%     
==========================================
  Files         159      158       -1     
  Lines        3443     3443              
  Branches      347      348       +1     
==========================================
- Hits         2137     2127      -10     
- Misses       1207     1217      +10     
  Partials       99       99              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fractalwrench]

This might be overkill, but I wonder whether it's worth creating an enum that holds TapEvent and clicks and returns the attributes that should be used for each event. IMO it could avoid logic such as if(isTapEvent) growing if we continue to add more touch events to the instrumentation.

[DavidGrath]

Would it be something like this?

//TapEvent renamed to TapEventMetadata
sealed class Gesture(
    val tapEventMetadata: TapEventMetadata
) {
    class LongPress(val t: TapEvent): Gesture(t)
    class Click(val t: TapEvent, clicks: Int): Gesture(t)
    class Scale(val t: TapEvent, val scaleFactor: Double): Gesture(t)
    class Scroll(val t: TapEvent, val velocityX: Double, val velocityY: Double): Gesture(t)
}

[fractalwrench]

Yep, something along those lines. Gesture could even define val attributes: Map<String, Any> and generate the attributes that are required for each event.

[DavidGrath]

Noted. I'll apply these changes over the weekend

[DavidGrath]

@fractalwrench , so is this alright or should I stick with a plain Map instead of Attributes? I feel the dangling builder object is something I might need to walk back on

[fractalwrench]

I would lean slightly to just returning Attributes rather than a builder but I don't think that needs to block.

[breedx-splk]

I don't think there's much value in holding a mutable object in an immutable val, versus just having a mutable private var. So, my recommendation is to just have

var attributes = Attributes.empty()

...and then in the 3 places where you need to update it, you can do

attributes = attributes.toBuilder().put(xxx, yyy).build().

[DavidGrath]

Okay, done. Thanks for the review

[fractalwrench]

LGTM

[fractalwrench]

I would lean slightly to just returning Attributes rather than a builder but I don't think that needs to block.

[breedx-splk]

Suggested change

[breedx-splk]

Had one small feedback about the attributes builder, but otherwise looking good. Thanks for the contribution!

[marandaneto]

sharing as context
eg https://github.com/PostHog/posthog-android/blob/d2cc7f7a9bb715a2902b454e33bc4912cba585b9/posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt#L303-L323
i've seen errors where acessing MotionEvent may trigger native errors, so its better to call MotionEvent.obtain and later recycle
this was the case if accessing or processing in a background thread, not sure if its the case here

[DavidGrath]

If the change is to simply use an obtain to duplicate it, then I think I can do that. I'd need to take my time to look at your own code so I can reproduce a test case for it, too, so it'll be a bit

[LikeTheSalad]

Thank you! 🙏