RUM-6195: Add support for Compose Checkbox #2414

jonathanmos · 2024-11-25T13:53:05Z

What does this PR do?

Adds support for the Checkbox Semantics role. Resolves the path of the checkbox to a bitmap and sends as an imageWireframe, but if unsuccessful will create a ShapeWireframe approximation. This pr includes:

• Creation of checked/unchecked state for the checkbox.
• Fallback to a manual shape wireframe if we couldn't resolve the checkmark path.
• Caching of the checkmark resourceId in the ResourcesLruCache.
• Generation of a cache key for the checkmark using PathMeasure on the path to fingerprint it by sampling at intervals. This is bounded by a configurable maximum number of samples.

Unchecked state:

Checked state if succesful in resolving the path:

Checked state fallback if path resolution is unsuccessful:

Motivation

Part of the initiative to support Jetpack Compose.

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 · 2024-11-26T10:07:35Z

Codecov Report

Attention: Patch coverage is 77.25000% with 91 lines in your changes missing coverage. Please review.

Project coverage is 69.96%. Comparing base (4e7d937) to head (c1b4f66).
Report is 6 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ay/internal/recorder/resources/ResourceResolver.kt	51.67%	28 Missing and 1 partial ⚠️
.../android/sessionreplay/internal/utils/PathUtils.kt	84.51%	7 Missing and 4 partials ⚠️
...y/compose/internal/reflection/ComposeReflection.kt	0.00%	9 Missing ⚠️
...sessionreplay/compose/internal/utils/ColorUtils.kt	18.18%	9 Missing ⚠️
.../sessionreplay/compose/internal/utils/PathUtils.kt	25.00%	9 Missing ⚠️
...onreplay/compose/internal/utils/ReflectionUtils.kt	0.00%	7 Missing ⚠️
...ionreplay/compose/internal/utils/SemanticsUtils.kt	76.67%	0 Missing and 7 partials ⚠️
...l/mappers/semantics/CheckboxSemanticsNodeMapper.kt	96.83%	1 Missing and 3 partials ⚠️
...d/sessionreplay/recorder/wrappers/BitmapWrapper.kt	40.00%	3 Missing ⚠️
...ndroid/sessionreplay/utils/ImageWireframeHelper.kt	33.33%	2 Missing ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2414      +/-   ##
===========================================
+ Coverage    69.92%   69.96%   +0.04%     
===========================================
  Files          781      785       +4     
  Lines        28915    29278     +363     
  Branches      4835     4884      +49     
===========================================
+ Hits         20217    20483     +266     
- Misses        7358     7431      +73     
- Partials      1340     1364      +24

Files with missing lines	Coverage Δ
...ernal/mappers/semantics/RootSemanticsNodeMapper.kt	`75.31% <100.00%> (+0.31%)`	⬆️
...nreplay/internal/recorder/SessionReplayRecorder.kt	`97.10% <100.00%> (+0.02%)`	⬆️
...y/internal/recorder/resources/ImageTypeResolver.kt	`100.00% <100.00%> (ø)`
...roid/sessionreplay/internal/utils/DrawableUtils.kt	`95.38% <100.00%> (+0.30%)`	⬆️
...d/sessionreplay/recorder/wrappers/CanvasWrapper.kt	`28.00% <100.00%> (ø)`
.../recorder/resources/DefaultImageWireframeHelper.kt	`98.33% <98.04%> (-0.09%)`	⬇️
...ndroid/sessionreplay/utils/ImageWireframeHelper.kt	`62.50% <33.33%> (ø)`
...d/sessionreplay/recorder/wrappers/BitmapWrapper.kt	`12.90% <40.00%> (ø)`
...l/mappers/semantics/CheckboxSemanticsNodeMapper.kt	`96.83% <96.83%> (ø)`
...onreplay/compose/internal/utils/ReflectionUtils.kt	`1.82% <0.00%> (-0.27%)`	⬇️
... and 6 more

... and 35 files with indirect coverage changes

0xnm

I briefly went through the production files, didn't cover tests

detekt_custom.yml

...adog/android/sessionreplay/compose/internal/mappers/semantics/CheckboxSemanticsNodeMapper.kt

...ompose/src/main/kotlin/com/datadog/android/sessionreplay/compose/internal/utils/PathUtils.kt

...e/src/main/kotlin/com/datadog/android/sessionreplay/compose/internal/utils/SemanticsUtils.kt

features/dd-sdk-android-session-replay/api/apiSurface

...adog/android/sessionreplay/compose/internal/mappers/semantics/CheckboxSemanticsNodeMapper.kt

ambushwork · 2024-11-26T15:01:49Z

...in/kotlin/com/datadog/android/sessionreplay/compose/internal/reflection/ComposeReflection.kt

@@ -37,6 +37,16 @@ internal object ComposeReflection {
    val ColorField = BackgroundElementClass?.getDeclaredFieldSafe("color")
    val ShapeField = BackgroundElementClass?.getDeclaredFieldSafe("shape")

+    val CheckDrawingCacheClass = getClassSafe("androidx.compose.material.CheckDrawingCache")
+    val CheckboxKtClass = getClassSafe("androidx.compose.material.CheckboxKt\$CheckboxImpl\$1\$1")


+1, updating proguard consumer rules is necessary

0xnm

nice! I left some comments/suggestions, but nothing major.

dd-sdk-android-internal/src/main/java/com/datadog/android/internal/utils/ImageViewUtils.kt

...e/src/main/kotlin/com/datadog/android/sessionreplay/compose/internal/utils/SemanticsUtils.kt

...adog/android/sessionreplay/compose/internal/mappers/semantics/CheckboxSemanticsNodeMapper.kt

0xnm · 2024-12-17T12:53:49Z

...adog/android/sessionreplay/compose/internal/mappers/semantics/CheckboxSemanticsNodeMapper.kt

+            }
+        }
+
+        if (wireframes.isNotEmpty()) {


minor: you probably can just do return wireframes.ifEmpty { createManualCheckedWireframe }

...adog/android/sessionreplay/compose/internal/mappers/semantics/CheckboxSemanticsNodeMapper.kt

...e/src/test/kotlin/com/datadog/android/sessionreplay/compose/internal/utils/ColorUtilsTest.kt

...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/resources/ResourceResolver.kt

...session-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/PathUtils.kt

...ssion-replay/src/main/kotlin/com/datadog/android/sessionreplay/utils/ImageWireframeHelper.kt

...datadog/android/sessionreplay/internal/recorder/resources/DefaultImageWireframeHelperTest.kt

0xnm · 2024-12-18T13:03:09Z

.../android/sessionreplay/compose/internal/mappers/semantics/CheckboxSemanticsNodeMapperTest.kt

+            .thenReturn(TextAndInputPrivacy.MASK_ALL_INPUTS)
+
+        // When
+        val wireframes = testedMapper.map(


since return type is SemanticsWireframe should this one be called wireframe? or checkboxWireframe.

Otherwise wireframes.wireframes below looks strange.

0xnm · 2024-12-18T13:04:03Z

.../android/sessionreplay/compose/internal/mappers/semantics/CheckboxSemanticsNodeMapperTest.kt

+        // Then
+        assertThat(wireframes.wireframes).hasSize(1)
+        val actualWireframe = wireframes.wireframes[0] as MobileSegment.Wireframe.ShapeWireframe
+        assertThat(actualWireframe).isNotNull


is it possible to have null at all? the type here is MobileSegment.Wireframe.ShapeWireframe, not MobileSegment.Wireframe.ShapeWireframe?

0xnm · 2024-12-18T13:07:41Z

...e/src/test/kotlin/com/datadog/android/sessionreplay/compose/internal/utils/ColorUtilsTest.kt

+    fun `M return input value W convertRgbaToArgb() { length is less than 8 }`(
+        forge: Forge
+    ) {
+        // Given
+        val input = forge.anAsciiString(1)
+
+        // Then
+        assertThat(testedUtils.convertRgbaToArgb(input)).isEqualTo(input)
+    }


so if we pass rgb string, the output will be wrong? Shouldn't we check in the convertRgbaToArgb method that input is exactly 8 characters (or 6 if we want to support rgb?

Otherwise we may have this function trying to compose something for the strings of format we don't expect.

jonathanmos force-pushed the jmoskovich/rum-6195/checkbox-semantics-mapper branch 2 times, most recently from 33e6311 to 66d7340 Compare November 26, 2024 09:40

jonathanmos force-pushed the jmoskovich/rum-6195/checkbox-semantics-mapper branch from 66d7340 to 9d12c72 Compare November 26, 2024 12:28

jonathanmos marked this pull request as ready for review November 26, 2024 13:23

jonathanmos requested review from a team as code owners November 26, 2024 13:23

0xnm reviewed Nov 26, 2024

View reviewed changes

ambushwork reviewed Nov 26, 2024

View reviewed changes

jonathanmos force-pushed the jmoskovich/rum-6195/checkbox-semantics-mapper branch from 9d12c72 to f23bb5f Compare November 27, 2024 12:11

jonathanmos force-pushed the jmoskovich/rum-6195/checkbox-semantics-mapper branch from f23bb5f to 8864b27 Compare December 9, 2024 08:41

jonathanmos changed the base branch from develop to jmoskovich/rum-6195/caches-any-key December 9, 2024 08:41

jonathanmos force-pushed the jmoskovich/rum-6195/checkbox-semantics-mapper branch 3 times, most recently from 4411582 to a02fc56 Compare December 11, 2024 13:36

jonathanmos changed the base branch from jmoskovich/rum-6195/caches-any-key to marcosaia/RUM-7018/react-native-support December 11, 2024 13:37

jonathanmos requested review from 0xnm and ambushwork December 12, 2024 13:43

marco-saia-datadog force-pushed the marcosaia/RUM-7018/react-native-support branch 4 times, most recently from 427dafe to 7a92c24 Compare December 12, 2024 14:28

jonathanmos force-pushed the jmoskovich/rum-6195/checkbox-semantics-mapper branch from b35cace to 0da9659 Compare December 16, 2024 11:28

marco-saia-datadog force-pushed the marcosaia/RUM-7018/react-native-support branch 5 times, most recently from 7f56e9a to a12c7ad Compare December 17, 2024 08:46

Base automatically changed from marcosaia/RUM-7018/react-native-support to develop December 17, 2024 10:50

jonathanmos force-pushed the jmoskovich/rum-6195/checkbox-semantics-mapper branch from 0da9659 to 2ef05ac Compare December 17, 2024 12:22

jonathanmos force-pushed the jmoskovich/rum-6195/checkbox-semantics-mapper branch from 2ef05ac to 8b092b1 Compare December 17, 2024 12:26

0xnm reviewed Dec 17, 2024

View reviewed changes

marco-saia-datadog and others added 4 commits December 18, 2024 10:05

RUM-6195: Add support for Compose Checkbox

9aaadcd

RUM-6195: Add FGM and proguard rules

c5216f9

RUM-6195: Implement caching logic for path based images

6bd6400

RUM-6195: Improve background color on unchecked state

1f3e4af

jonathanmos force-pushed the jmoskovich/rum-6195/checkbox-semantics-mapper branch 2 times, most recently from cd5a77c to 11155d6 Compare December 18, 2024 10:42

jonathanmos requested a review from 0xnm December 18, 2024 11:27

0xnm reviewed Dec 18, 2024

View reviewed changes

RUM-6195: Small refactoring

c1b4f66

jonathanmos force-pushed the jmoskovich/rum-6195/checkbox-semantics-mapper branch from 11155d6 to c1b4f66 Compare December 18, 2024 13:50

0xnm approved these changes Dec 18, 2024

View reviewed changes

jonathanmos merged commit f9bc2b5 into develop Dec 18, 2024
24 checks passed

jonathanmos deleted the jmoskovich/rum-6195/checkbox-semantics-mapper branch December 18, 2024 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-6195: Add support for Compose Checkbox #2414

RUM-6195: Add support for Compose Checkbox #2414

jonathanmos commented Nov 25, 2024 •

edited

Loading

codecov-commenter commented Nov 26, 2024 •

edited

Loading

0xnm left a comment

ambushwork Nov 26, 2024

0xnm left a comment

0xnm Dec 17, 2024

0xnm Dec 18, 2024

0xnm Dec 18, 2024

0xnm Dec 18, 2024

RUM-6195: Add support for Compose Checkbox #2414

RUM-6195: Add support for Compose Checkbox #2414

Conversation

jonathanmos commented Nov 25, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov-commenter commented Nov 26, 2024 • edited Loading

Codecov Report

0xnm left a comment

Choose a reason for hiding this comment

ambushwork Nov 26, 2024

Choose a reason for hiding this comment

0xnm left a comment

Choose a reason for hiding this comment

0xnm Dec 17, 2024

Choose a reason for hiding this comment

0xnm Dec 18, 2024

Choose a reason for hiding this comment

0xnm Dec 18, 2024

Choose a reason for hiding this comment

0xnm Dec 18, 2024

Choose a reason for hiding this comment

jonathanmos commented Nov 25, 2024 •

edited

Loading

codecov-commenter commented Nov 26, 2024 •

edited

Loading