RUMM-2777 SR add dialogs recording support by mariusc83 · Pull Request #1206 · DataDog/dd-sdk-android

mariusc83 · 2022-12-22T09:00:45Z

What does this PR do?

We were currently not recording the DialogFragment (pop - ups) for session replay. In this PR we are adding this new feature which implies the following architectural changes:

The ScreenRecorder will now be able to record multiple windows in the same time with the same ownerActivity. This actually covers the case of a DialogFragment.
The SnapshotProcessor will be able to handle a list of snapshots now instead of one single snapshot at a time.

I also addressed a bug in this PR : we were not using the MotionEvent absolute X,Y positions when recording the user gestures for Session Replay.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

codecov-commenter · 2022-12-23T15:02:08Z

Codecov Report

Merging #1206 (cb91a83) into feature/sdkv2 (932cecb) will increase coverage by 0.02%.
The diff coverage is 92.62%.

@@                Coverage Diff                @@
##           feature/sdkv2    #1206      +/-   ##
=================================================
+ Coverage          82.59%   82.61%   +0.02%     
=================================================
  Files                351      353       +2     
  Lines              12480    12537      +57     
  Branches            2058     2075      +17     
=================================================
+ Hits               10307    10357      +50     
- Misses              1533     1539       +6     
- Partials             640      641       +1

Impacted Files	Coverage Δ
...tadog/android/sessionreplay/processor/Processor.kt	`0.00% <0.00%> (ø)`
...sionreplay/recorder/callback/NoOpWindowCallback.kt	`6.25% <ø> (ø)`
...id/sessionreplay/SessionReplayLifecycleCallback.kt	`87.10% <75.00%> (-6.65%)`	⬇️
...rder/callback/RecorderFragmentLifecycleCallback.kt	`91.67% <91.67%> (ø)`
...nreplay/recorder/listener/WindowsOnDrawListener.kt	`93.94% <93.94%> (ø)`
...g/android/sessionreplay/recorder/ScreenRecorder.kt	`95.45% <96.55%> (+1.34%)`	⬆️
...d/sessionreplay/processor/RecordedDataProcessor.kt	`93.88% <100.00%> (+0.33%)`	⬆️
...essionreplay/recorder/callback/MotionEventUtils.kt	`100.00% <100.00%> (ø)`
...replay/recorder/callback/RecorderWindowCallback.kt	`89.47% <100.00%> (ø)`
...n/com/datadog/android/v2/core/SdkInternalLogger.kt	`86.11% <0.00%> (-5.56%)`	⬇️
... and 13 more

0xnm

lgtm! I've added some comments, but they are non-blocking.

0xnm · 2022-12-26T09:29:51Z

+                recorder.startRecording(windows, ownerActivity)
+                recordCallback.onStartRecording()


minor: It seems in the code recorder.startRecording is always followed by the recordCallback.onStartRecording (same for the stop sequence). Does it make sense to put recordCallback inside recorder?

good point, let me check

0xnm · 2022-12-26T09:43:25Z


    @Test
-    fun `M start recording activity W onActivityResumed()`() {
+    fun `M start register fragment lifecycle W onActivityCreated(){FragmentActivity}`() {


Suggested change

fun `M start register fragment lifecycle W onActivityCreated(){FragmentActivity}`() {

fun `M register fragment lifecycle W onActivityCreated(){FragmentActivity}`() {

0xnm · 2022-12-26T09:45:51Z

        // Then
-        verify(mockRecoder).startRecording(mockActivity)
-        assertThat(testedCallback.resumedActivities).containsKey(mockActivity)
+        verify(mockRecoder).startRecording(eq(listOf(mockWindow)), eq(mockActivity))


minor: eq calls are not needed here and in some tests below as well, they are only needed when one of the arguments is capture() call.

You are right, reason why I added those like that was cause I thought it will do the assertion by reference in cause of the lists. Apparently is doing it by value anyway without the eq

0xnm · 2022-12-26T09:50:04Z

@@ -1,4 +1,4 @@
-/*
+ /*


seems like redundant change

0xnm · 2022-12-26T09:59:53Z

+            it.decorView.viewTreeObserver.inOrder {
+                verify().addOnDrawListener(captor.capture())
+                verify().removeOnDrawListener(captor.firstValue)
+                verify().addOnDrawListener(captor.capture())


we don't assert the result of the 2nd capture call, is it expected?

yes because as the test is saying I just want to test that the previously registered windows (first call) were un - registered.

yes, just semantically on this line we capture something what we don't use later, so maybe the cleaner thing will be something like verify().addOnDrawListener(any())

0xnm · 2022-12-26T10:00:49Z

-        val mockViewTreeObserver: ViewTreeObserver = mock()
-        val mockActivity = mockActivity(forge, mockViewTreeObserver)
-        testedRecorder.startRecording(mockActivity)
+        val fakeWindowsActivityPairs: List<Pair<List<Window>, Activity>> = forge.aList {


kotlin cannot infer the type here? given that types of aMockedWindowsList() and aMockedActivity() are known.

let me check

xgouchet · 2022-12-28T07:53:16Z

+import android.os.Build
+import android.view.MotionEvent
+
+internal class MotionEventUtils {


It seems like this could be an object, or just an ensemble of extension fun on MotionEvent

I will make it an object, I don't like extensions as they are making your life harder when you have to mock them.

xgouchet · 2022-12-28T07:57:55Z

+        }
+    }
+
+    private fun Fragment.asValidDialogFragment(): DialogFragment? {


It seems all usage of this method are followed by ?.let { … }, you could make this method look like this:

fun Fragment.asValidDialogFragment(block:(DialogFragment) ->) { if (this is DialogFragment && context != null) { block(this) } }

0xnm · 2022-12-28T10:51:13Z

+            it.decorView.viewTreeObserver.inOrder {
+                verify().addOnDrawListener(captor.capture())
+                verify().removeOnDrawListener(captor.firstValue)
+                verify().addOnDrawListener(captor.capture())


yes, just semantically on this line we capture something what we don't use later, so maybe the cleaner thing will be something like verify().addOnDrawListener(any())

0xnm · 2022-12-28T10:53:14Z

-        verify(mockViewTreeObserver).addOnDrawListener(captor.capture())
-        verify(mockViewTreeObserver).removeOnDrawListener(captor.firstValue)
+        fakeWindowsActivityPairs.map { it.first }.flatten().forEach {
+            val captor = argumentCaptor<ViewTreeObserver.OnDrawListener>()


We don't need captor here, because we don't run any assertions on the values captured. We can use simply verify().addOnDrawListener(any())

well actually I am doing it here:
verify(it.decorView.viewTreeObserver).removeOnDrawListener(captor.firstValue). I know it could work with any too but I would rather use an explicit verification.

Nevermind, you are right, I was looking at the wrong line actually.

mariusc83 force-pushed the mconstantin/rumm-2777/start-recording-dialogs branch 5 times, most recently from 823cabb to fb96550 Compare December 23, 2022 13:48

mariusc83 self-assigned this Dec 23, 2022

mariusc83 marked this pull request as ready for review December 23, 2022 13:52

mariusc83 requested a review from a team as a code owner December 23, 2022 13:52

mariusc83 force-pushed the mconstantin/rumm-2777/start-recording-dialogs branch from fb96550 to 6c6dc8c Compare December 23, 2022 14:38

0xnm approved these changes Dec 26, 2022

View reviewed changes

mariusc83 force-pushed the mconstantin/rumm-2777/start-recording-dialogs branch 3 times, most recently from 4203c9f to dfe5da8 Compare December 26, 2022 16:37

mariusc83 requested a review from 0xnm December 26, 2022 16:37

xgouchet suggested changes Dec 28, 2022

View reviewed changes

0xnm approved these changes Dec 28, 2022

View reviewed changes

RUMM-2777 SR add dialogs recording support

cb91a83

mariusc83 force-pushed the mconstantin/rumm-2777/start-recording-dialogs branch from dfe5da8 to cb91a83 Compare January 2, 2023 09:32

mariusc83 requested a review from xgouchet January 2, 2023 09:49

xgouchet approved these changes Jan 2, 2023

View reviewed changes

mariusc83 merged commit 555764f into feature/sdkv2 Jan 2, 2023

mariusc83 deleted the mconstantin/rumm-2777/start-recording-dialogs branch January 2, 2023 13:57

xgouchet added this to the 1.17.0 milestone Dec 13, 2023

		recorder.startRecording(windows, ownerActivity)
		recordCallback.onStartRecording()

	fun `M start register fragment lifecycle W onActivityCreated(){FragmentActivity}`() {
	fun `M register fragment lifecycle W onActivityCreated(){FragmentActivity}`() {

Uh oh!

Conversation

mariusc83 commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Dec 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mariusc83 commented Dec 22, 2022 •

edited

Loading

codecov-commenter commented Dec 23, 2022 •

edited

Loading