Fix the async image loading logic inside the SR view mappers by mariusc83 · Pull Request #1619 · DataDog/dd-sdk-android

mariusc83 · 2023-09-15T15:19:00Z

What does this PR do?

A brief description of the change being made with this pull request.

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 · 2023-09-19T17:35:34Z

Codecov Report

Merging #1619 (a9b96c3) into develop (9ef76a9) will increase coverage by 0.09%.
Report is 6 commits behind head on develop.
The diff coverage is 92.80%.

@@             Coverage Diff             @@
##           develop    #1619      +/-   ##
===========================================
+ Coverage    83.78%   83.86%   +0.09%     
===========================================
  Files          452      455       +3     
  Lines        15668    15680      +12     
  Branches      2326     2326              
===========================================
+ Hits         13126    13150      +24     
+ Misses        1918     1915       -3     
+ Partials       624      615       -9

Files Changed	Coverage Δ
...id/sessionreplay/material/SliderWireframeMapper.kt	`100.00% <ø> (ø)`
...adog/android/sessionreplay/SessionReplayPrivacy.kt	`100.00% <ø> (ø)`
...sionreplay/internal/async/RecordedDataQueueRefs.kt	`75.00% <0.00%> (ø)`
...ay/internal/recorder/mapper/BaseWireframeMapper.kt	`73.33% <ø> (-12.58%)`	⬇️
...nternal/recorder/mapper/MaskInputTextViewMapper.kt	`80.00% <ø> (-3.33%)`	⬇️
...internal/recorder/mapper/MaskNumberPickerMapper.kt	`100.00% <ø> (ø)`
...lay/internal/recorder/mapper/NumberPickerMapper.kt	`99.13% <ø> (ø)`
...internal/recorder/mapper/SeekBarWireframeMapper.kt	`100.00% <ø> (ø)`
.../internal/recorder/mapper/UnsupportedViewMapper.kt	`100.00% <ø> (ø)`
...l/recorder/mapper/ViewScreenshotWireframeMapper.kt	`100.00% <ø> (ø)`
... and 18 more

... and 18 files with indirect coverage changes

mariusc83 · 2023-09-20T08:11:17Z

    private val viewWireframeMapper: ViewWireframeMapper,
    private val uniqueIdentifierGenerator: UniqueIdentifierGenerator = UniqueIdentifierGenerator
-) : BaseWireframeMapper<View, MobileSegment.Wireframe.ShapeWireframe>() {
+) : WireframeMapper<View, MobileSegment.Wireframe> {


we do not need the BaseWireframeMapper anymore here

mariusc83 · 2023-09-20T08:33:29Z

+) : AsyncJobStatusCallback {
+    fun map(view: View, mappingContext: MappingContext):
        List<MobileSegment.Wireframe> {
-        (mapper as? BaseWireframeMapper<View, *>)?.registerAsyncImageProcessingCallback(this)


this was the main issue here as we were still registering a callback

0xnm

looks good!

0xnm · 2023-09-20T08:18:31Z

+abstract class com.datadog.android.sessionreplay.internal.recorder.mapper.BaseAsyncBackgroundWireframeMapper<T: android.view.View> : BaseWireframeMapper<T, com.datadog.android.sessionreplay.model.MobileSegment.Wireframe>
+  constructor(com.datadog.android.sessionreplay.utils.StringUtils = StringUtils, com.datadog.android.sessionreplay.utils.ViewUtils = ViewUtils)
+  override fun map(T, com.datadog.android.sessionreplay.internal.recorder.MappingContext, com.datadog.android.sessionreplay.internal.AsyncJobStatusCallback): List<com.datadog.android.sessionreplay.model.MobileSegment.Wireframe>
+  companion object 


if it doesn't expose public members, then we can make it internal

actually not as we have the TextViewMapper class which extends it and it is public to be used in the other module

0xnm · 2023-09-20T08:26:03Z

+    ):
+        List<MobileSegment.Wireframe> {


Suggested change

):

List<MobileSegment.Wireframe> {

): List<MobileSegment.Wireframe> {

0xnm · 2023-09-20T08:28:14Z

+    override fun map(
+        view: T,
+        mappingContext: MappingContext,
+        asyncJobStatusCallback: AsyncJobStatusCallback
+    ):


we need to add a good documentation for this method in the future, because it is not clear why do we have such API - we return some result instantly and at the same time have an async callback with some notification mechanism. Usually APIs have either of the two - return result in a blocking way, or give it in the callback.

I do agree, maybe once we will have a stable version, meaning the new resources endpoint in please (which will remove the base64 logic) and the fine grained masking we could simplify a bit this part. My plan is to assess after if we still need this queue or not. We already copy the drawable now so I am thinking to actually create this ViewPlacehdolder model that will be processed into wireframe at the Processor level. For now we are not ready for this switch as we do not know if copying the drawable will always be possible and also need to see how the logic will look like with the new resources endpoint in place.

0xnm · 2023-09-20T08:34:50Z

     * @param mappingContext in which we provide useful information regarding the current
-     * system state.
+     * @param asyncJobStatusCallback a callback that can be called when the mapper starts or
+     * finishes processing an async job. This is useful to keep track of the current async jobs


since both jobStarted and jobFinished doesn't have any arguments like job ID and result, maybe we can elaborate why it is useful? Without any arguments passed it can probably be served as a counter only.

jonathanmos · 2023-09-20T10:39:33Z


    @Test
-    fun `M resolve a TextWireframe with background ShapeWireframe W map { TextView with ColorDrawable }`(
+    fun `M resolve a TextWireframe with background ShapeWireframe W map {with ColorDrawable back}`(


Suggested change

fun `M resolve a TextWireframe with background ShapeWireframe W map {with ColorDrawable back}`(

fun `M resolve a TextWireframe with background ShapeWireframe W map() { ColorDrawable background }`(

jonathanmos · 2023-09-20T10:40:37Z

    }

+    @Test
+    fun `M resolve a TextWireframe with background ShapeWireframe W map {no ColorDrawable back}`(


Suggested change

fun `M resolve a TextWireframe with background ShapeWireframe W map {no ColorDrawable back}`(

fun `M resolve a TextWireframe with background ShapeWireframe W map() { not a ColorDrawable background }`(

0xnm

lgtm. I've added few minor suggestions.

0xnm · 2023-09-20T12:12:56Z

+import com.datadog.android.sessionreplay.utils.ViewUtils
+
+@Suppress("UndocumentedPublicClass")
+abstract class BaseAsyncBackgroundWireframeMapper<T : View>(


minor: does it make sense to use word async here in the class naming? WireframeMapper interface already hints that it supports async jobs by having async callback argument in map method.

yeah it tells that the background will be handled in async way, hmm maybe I can find a better name for this if not will leave it like this.

0xnm · 2023-09-20T12:14:50Z

+     * not be consumed until all the wireframes are updated by the async jobs. The main reason we
+     * needed this functionality was to be able to unblock the main thread whenever we needed
+     * heavy operations like image capture or serialisation.


I would re-word this sentence about why it is needed. Not to say "we needed this functionality", but something like "it can be used to offload heavy work from the calling thread (like main) ... ", in such fashion.

mariusc83 force-pushed the mconstantin/rum-1064/fix-the-base64-image-resolver-logic branch 2 times, most recently from d110cee to 908d8af Compare September 19, 2023 14:57

mariusc83 self-assigned this Sep 19, 2023

mariusc83 force-pushed the mconstantin/rum-1064/fix-the-base64-image-resolver-logic branch 3 times, most recently from ecc4e35 to adc96b0 Compare September 19, 2023 16:46

mariusc83 marked this pull request as ready for review September 19, 2023 17:34

mariusc83 requested a review from a team as a code owner September 19, 2023 17:34

mariusc83 force-pushed the mconstantin/rum-1064/fix-the-base64-image-resolver-logic branch from adc96b0 to edeaa29 Compare September 20, 2023 07:01

mariusc83 commented Sep 20, 2023

View reviewed changes

0xnm previously approved these changes Sep 20, 2023

View reviewed changes

mariusc83 dismissed 0xnm’s stale review via 2339318 September 20, 2023 10:56

mariusc83 force-pushed the mconstantin/rum-1064/fix-the-base64-image-resolver-logic branch from edeaa29 to 2339318 Compare September 20, 2023 10:56

jonathanmos previously approved these changes Sep 20, 2023

View reviewed changes

mariusc83 dismissed jonathanmos’s stale review via 7c10d5a September 20, 2023 12:05

mariusc83 force-pushed the mconstantin/rum-1064/fix-the-base64-image-resolver-logic branch 2 times, most recently from 7c10d5a to 519310a Compare September 20, 2023 12:08

mariusc83 requested review from 0xnm and jonathanmos September 20, 2023 12:08

0xnm previously approved these changes Sep 20, 2023

View reviewed changes

RUM-1619 Fix the async image loading logic inside the SR view mappers

a9b96c3

mariusc83 dismissed 0xnm’s stale review via a9b96c3 September 20, 2023 12:29

mariusc83 force-pushed the mconstantin/rum-1064/fix-the-base64-image-resolver-logic branch from 519310a to a9b96c3 Compare September 20, 2023 12:29

mariusc83 requested a review from 0xnm September 20, 2023 12:29

0xnm approved these changes Sep 20, 2023

View reviewed changes

mariusc83 merged commit 956f75e into develop Sep 20, 2023

mariusc83 deleted the mconstantin/rum-1064/fix-the-base64-image-resolver-logic branch September 20, 2023 13:01

xgouchet added this to the 2.2.0 milestone Dec 13, 2023

	):
	List<MobileSegment.Wireframe> {
	): List<MobileSegment.Wireframe> {

	fun `M resolve a TextWireframe with background ShapeWireframe W map {with ColorDrawable back}`(
	fun `M resolve a TextWireframe with background ShapeWireframe W map() { ColorDrawable background }`(

Uh oh!

Conversation

mariusc83 commented Sep 15, 2023

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Sep 19, 2023 •

edited

Loading