Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RUM-7018] RN Image Recording Support #2435

Open
wants to merge 9 commits into
base: jmoskovich/rum-6195/caches-any-key
Choose a base branch
from

Conversation

marco-saia-datadog
Copy link
Member

@marco-saia-datadog marco-saia-datadog commented Dec 5, 2024

What does this PR do?

This PR introduces changes required for granting support to new Session Replay features on React Native.

What changed

Open Points

  • Drawables are not cloned anymore in drawOnCanvas (see comments)
  • BaseAsyncBackgroundWireframeMapper now exposes some methods as protected open. Should we add clues that they are only intended for internal usage? What's the best way to do that?
  • What do you think about exposing ImageViewUtils? We currently re-use most of it in React Native and perhaps we could avoid duplicating that code

TODO

  • Lint and checks

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)

@marco-saia-datadog marco-saia-datadog changed the title RUM-7018: RN Image Recording Support [RUM-7018] RN Image Recording Support (DRAFT) Dec 5, 2024
@marco-saia-datadog marco-saia-datadog changed the base branch from jmoskovich/rum-6195/caches-any-key to develop December 6, 2024 10:13
@marco-saia-datadog marco-saia-datadog changed the base branch from develop to jmoskovich/rum-6195/caches-any-key December 6, 2024 10:14
@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-7018/rn-image-recording-support branch 3 times, most recently from 1eefd82 to 6b6e99d Compare December 6, 2024 11:38
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 65.21739% with 8 lines in your changes missing coverage. Please review.

Project coverage is 69.79%. Comparing base (1dcc621) to head (5349533).
Report is 1 commits behind head on jmoskovich/rum-6195/caches-any-key.

Files with missing lines Patch % Lines
...m/datadog/android/internal/utils/ImageViewUtils.kt 0.00% 5 Missing ⚠️
...d/sessionreplay/recorder/mapper/ImageViewMapper.kt 40.00% 3 Missing ⚠️
Additional details and impacted files
@@                          Coverage Diff                           @@
##           jmoskovich/rum-6195/caches-any-key    #2435      +/-   ##
======================================================================
- Coverage                               70.24%   69.79%   -0.45%     
======================================================================
  Files                                     767      768       +1     
  Lines                                   28513    28517       +4     
  Branches                                 4782     4781       -1     
======================================================================
- Hits                                    20028    19902     -126     
- Misses                                   7149     7277     +128     
- Partials                                 1336     1338       +2     
Files with missing lines Coverage Δ
.../java/com/datadog/android/internal/utils/IntExt.kt 0.00% <ø> (ø)
...java/com/datadog/android/internal/utils/LongExt.kt 0.00% <ø> (ø)
.../sessionreplay/internal/DefaultRecorderProvider.kt 93.70% <100.00%> (+0.05%) ⬆️
...ssionreplay/internal/recorder/ViewUtilsInternal.kt 100.00% <ø> (ø)
...ternal/recorder/callback/RecorderWindowCallback.kt 91.95% <ø> (ø)
...eplay/internal/recorder/mapper/BasePickerMapper.kt 100.00% <ø> (ø)
...l/recorder/mapper/CheckableCompoundButtonMapper.kt 37.50% <ø> (ø)
...nternal/recorder/mapper/CheckableTextViewMapper.kt 63.79% <ø> (ø)
.../internal/recorder/mapper/CheckedTextViewMapper.kt 75.00% <ø> (ø)
...rnal/recorder/mapper/ProgressBarWireframeMapper.kt 90.24% <ø> (ø)
... and 14 more

... and 25 files with indirect coverage changes

@marco-saia-datadog marco-saia-datadog changed the title [RUM-7018] RN Image Recording Support (DRAFT) [RUM-7018] RN Image Recording Support Dec 6, 2024
@marco-saia-datadog marco-saia-datadog marked this pull request as ready for review December 6, 2024 13:58
@marco-saia-datadog marco-saia-datadog requested review from a team as code owners December 6, 2024 13:58
@jonathanmos jonathanmos force-pushed the jmoskovich/rum-6195/caches-any-key branch from 56c49e9 to ebafe41 Compare December 8, 2024 13:12
@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-7018/rn-image-recording-support branch 4 times, most recently from 5f5d088 to 7d66065 Compare December 9, 2024 15:05
@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-7018/rn-image-recording-support branch from 7d66065 to 881456a Compare December 9, 2024 15:44
Copy link
Member

@jonathanmos jonathanmos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the drawOnCanvas copying of the drawables, it should be safe as the only place that references it in the call chain looks like it already copies the drawable. The methods that are protected I think it's enough indication that they are internal. Regarding the ImageViewUtils, if we can move it to the internal module and you can access it from there it will probably be the best solution

customResourceIdCacheKey = null,
asyncJobStatusCallback = mockAsyncJobStatusCallback
)
wireframes[0] as MobileSegment.Wireframe.ImageWireframe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result is not stored anywhere, is it expected?

@marco-saia-datadog
Copy link
Member Author

@jonathanmos

I had to do some refactoring to move the ImageViewUtils to the dd-sdk-android-internal package, because the class was relying on Session Replay MobileSegment.WireframeClip class. I have refactored it to make it use Rect instead, and added an extension function to easily convert a Rect to a MobileSegment.WireframeClip, and modified the code and tests accordingly.

@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-7018/rn-image-recording-support branch 2 times, most recently from c510e0e to f602b9b Compare December 10, 2024 12:16
@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-7018/rn-image-recording-support branch 2 times, most recently from cf032ee to adb475d Compare December 10, 2024 13:29
@marco-saia-datadog marco-saia-datadog force-pushed the marcosaia/RUM-7018/rn-image-recording-support branch from adb475d to bf7e7da Compare December 10, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants