Skip to content

RUM-949 Support ImageView scaletypes#1677

Merged
jonathanmos merged 1 commit into
developfrom
jmoskovich/rum-950/fix-imageview-resizing
Nov 13, 2023
Merged

RUM-949 Support ImageView scaletypes#1677
jonathanmos merged 1 commit into
developfrom
jmoskovich/rum-950/fix-imageview-resizing

Conversation

@jonathanmos

@jonathanmos jonathanmos commented Oct 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

This PR restores imageview support (that was previously reverted on develop).
It then primarily adds support for all ImageView scaletypes.
In addition:
• Adds a screen to the example app focused on the different scaletypes
• Cleans up the image components screen in the example app to reorder the ImageViews there and remove basic Views (until/unless we support them)
• Fixes an issue with ImageViews where opacity wasn't computed correctly (WireframeExt)
• Support for ImageView cropping through the 'crop' property of the ImageWireFrame (WireframeUtils)
• Adds a boolean flag ignoreIsPII so that background images can no longer be blocked as PII (ImageWireframeHelper)
• Increases PII threshold from 49 to 100dp
• Extracts logic out of WireframeUtils into the new BoundsUtils in order to simplify testing
• Updates the functional tests payloads
• Increase bitmap scaling limit from 10kb to 15kb (to improve image quality and align with ios)

Things that will be added in later PRs:
• Functional tests for the new scaling screen - in RUM-1647
• Unit tests for BoundsUtils - in RUM-1648

The PR is large, but that's due in a large part to the revert and the boilerplate of the additional screen/functional tests.

Motivation

Fix a resizing issue with ImageViews

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)

@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-950/fix-imageview-resizing branch from d9aba07 to 1f4066d Compare October 18, 2023 22:40
@codecov-commenter

codecov-commenter commented Oct 18, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 85.20548% with 54 lines in your changes missing coverage. Please review.

Project coverage is 83.53%. Comparing base (7e80c37) to head (ee7cd5f).
Report is 1975 commits behind head on develop.

Files with missing lines Patch % Lines
...id/sessionreplay/internal/processor/BoundsUtils.kt 0.00% 36 Missing and 1 partial ⚠️
...eplay/internal/recorder/base64/Base64Serializer.kt 86.11% 9 Missing and 1 partial ⚠️
...oid/sessionreplay/internal/utils/ImageViewUtils.kt 97.32% 0 Missing and 3 partials ⚠️
...y/internal/recorder/base64/WebPImageCompression.kt 60.00% 2 Missing ⚠️
...y/internal/recorder/base64/ImageWireframeHelper.kt 96.88% 0 Missing and 1 partial ⚠️
...replay/internal/recorder/mapper/ImageViewMapper.kt 95.24% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1677      +/-   ##
===========================================
- Coverage    83.73%   83.53%   -0.20%     
===========================================
  Files          463      465       +2     
  Lines        15896    16025     +129     
  Branches      2371     2376       +5     
===========================================
+ Hits         13310    13386      +76     
- Misses        1951     1994      +43     
- Partials       635      645      +10
Files with missing lines Coverage Δ
...adog/android/sessionreplay/SessionReplayPrivacy.kt 100.00% <100.00%> (ø)
...essionreplay/internal/processor/WireframeBounds.kt 100.00% <100.00%> (ø)
...sessionreplay/internal/processor/WireframeUtils.kt 97.30% <100.00%> (-2.70%) ⬇️
...play/internal/recorder/base64/ImageTypeResolver.kt 100.00% <100.00%> (ø)
...order/mapper/BaseAsyncBackgroundWireframeMapper.kt 95.24% <100.00%> (+0.32%) ⬆️
...roid/sessionreplay/internal/utils/DrawableUtils.kt 98.46% <100.00%> (+2.46%) ⬆️
...droid/sessionreplay/internal/utils/WireframeExt.kt 88.89% <100.00%> (-0.58%) ⬇️
...y/internal/recorder/base64/ImageWireframeHelper.kt 94.92% <96.88%> (+0.22%) ⬆️
...replay/internal/recorder/mapper/ImageViewMapper.kt 94.59% <95.24%> (ø)
...y/internal/recorder/base64/WebPImageCompression.kt 78.57% <60.00%> (-11.43%) ⬇️
... and 3 more

... and 21 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jonathanmos
jonathanmos marked this pull request as ready for review October 18, 2023 23:40
@jonathanmos
jonathanmos requested a review from a team as a code owner October 18, 2023 23:40
xgouchet
xgouchet reviewed Oct 19, 2023
View reviewed changes
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/ImageTypeResolver.kt Outdated
Comment thread ...in/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/ImageWireframeHelper.kt Outdated
Comment thread ...on-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/ImageViewUtils.kt Outdated
mariusc83
mariusc83 previously approved these changes Oct 23, 2023
View reviewed changes

@mariusc83 mariusc83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

0xnm
0xnm reviewed Oct 23, 2023
View reviewed changes
Comment thread detekt_custom.yml Outdated
Comment thread ...n-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/processor/BoundsUtils.kt Outdated
Comment thread .../main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/ImageTypeResolver.kt
0xnm
0xnm previously approved these changes Oct 24, 2023
View reviewed changes

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks nice! and big :) nothing blocking for me, I left some suggestions though.

Comment thread ...lay/src/test/kotlin/com/datadog/android/sessionreplay/forge/WireframeBoundsForgeryFactory.kt Outdated
Comment thread ...y/src/test/kotlin/com/datadog/android/sessionreplay/internal/processor/WireframeUtilsTest.kt Outdated
Comment thread ...t/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/ImageTypeResolverTest.kt
Comment thread ...dd-sdk-android-session-replay/src/test/kotlin/com/datadog/android/utils/InternalViewUtils.kt Outdated
Comment thread sample/kotlin/src/main/kotlin/com/datadog/android/sample/sessionreplay/ImageScalingFragment.kt Outdated
Comment thread sample/kotlin/src/main/kotlin/com/datadog/android/sample/sessionreplay/ImageScalingFragment.kt Outdated
jonathanmos
jonathanmos commented Oct 25, 2023
View reviewed changes
Comment thread ...ion-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/utils/DrawableUtils.kt Outdated

internal companion object {
private const val MAX_BITMAP_SIZE_IN_BYTES = 10240 // 10kb
private const val MAX_BITMAP_SIZE_IN_BYTES = 15000 // 15kb

@jonathanmos jonathanmos Oct 25, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

notice that I've modified this in this pr, since it's a small change and relates directly to imageviews

@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-950/fix-imageview-resizing branch from b74e13a to 2311d97 Compare October 25, 2023 13:12
@jonathanmos
jonathanmos requested a review from 0xnm October 26, 2023 08:19
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-950/fix-imageview-resizing branch from 2311d97 to f703f86 Compare October 29, 2023 08:32
@jonathanmos jonathanmos changed the title RUM-950 Support ImageView scaletypes RUM-949 Support ImageView scaletypes Nov 7, 2023
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-950/fix-imageview-resizing branch from 857bb8d to ca69ae5 Compare November 11, 2023 21:01
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-950/fix-imageview-resizing branch from 81b8836 to ee7cd5f Compare November 13, 2023 10:56
@jonathanmos
jonathanmos merged commit 43c2c3e into develop Nov 13, 2023
@jonathanmos
jonathanmos deleted the jmoskovich/rum-950/fix-imageview-resizing branch November 13, 2023 11:42
@xgouchet xgouchet added this to the 2.3.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mariusc83 mariusc83 mariusc83 approved these changes
@0xnm 0xnm 0xnm approved these changes
+1 more reviewer
@xgouchet xgouchet xgouchet approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.3.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jonathanmos @codecov-commenter @xgouchet @mariusc83 @0xnm