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-3460: Fix padding and resizing issue for image view mapper #2372

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

ambushwork
Copy link
Member

@ambushwork ambushwork commented Nov 4, 2024

What does this PR do?

In app development, clients usually use paddings to resize the content drawable in a given size image view, prior to this PR we didn't taken into account these paddings to resize our image wireframe, which leads some images/icons bigger than real.

So the updates in this PR:

  • Add paddings for a demo image view in sample app
  • Add paddings calculation in resolveParentRectAbsPosition function
  • Fix the issue that GradientDrawable didn't take into account the width&height after resizing.

Motivation

RUM-3460

Demo

image

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)

@ambushwork ambushwork marked this pull request as ready for review November 4, 2024 17:04
@ambushwork ambushwork requested review from a team as code owners November 4, 2024 17:04
@ambushwork ambushwork force-pushed the yl/sr-mnt/fix-image-view-padding branch from 6333f4c to 95ab882 Compare November 5, 2024 07:41
@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.37%. Comparing base (b1d6ebb) to head (39c3e59).
Report is 5 commits behind head on develop.

Files with missing lines Patch % Lines
...replay/internal/recorder/mapper/ImageViewMapper.kt 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2372      +/-   ##
===========================================
+ Coverage    70.30%   70.37%   +0.07%     
===========================================
  Files          744      744              
  Lines        27721    27726       +5     
  Branches      4630     4630              
===========================================
+ Hits         19488    19512      +24     
+ Misses        6947     6936      -11     
+ Partials      1286     1278       -8     
Files with missing lines Coverage Δ
.../recorder/resources/DefaultImageWireframeHelper.kt 95.30% <ø> (-0.03%) ⬇️
...oid/sessionreplay/internal/utils/ImageViewUtils.kt 97.44% <100.00%> (+0.09%) ⬆️
...replay/internal/recorder/mapper/ImageViewMapper.kt 92.11% <33.33%> (-5.12%) ⬇️

... and 31 files with indirect coverage changes

@ambushwork ambushwork force-pushed the yl/sr-mnt/fix-image-view-padding branch from 95ab882 to fe466c9 Compare November 5, 2024 12:12
Copy link
Member

@xgouchet xgouchet left a comment

Choose a reason for hiding this comment

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

Note

Shouldn't we have some unit tests around those functions ?

@ambushwork ambushwork force-pushed the yl/sr-mnt/fix-image-view-padding branch from fe466c9 to 39c3e59 Compare November 5, 2024 17:11
@ambushwork
Copy link
Member Author

Note

Shouldn't we have some unit tests around those functions ?

@xgouchet there are unit tests around these, I just updated with the padding case to cover it better, and instrumented test payloads are regenerated for them.

@ambushwork ambushwork merged commit ab89e64 into develop Nov 5, 2024
24 checks passed
@ambushwork ambushwork deleted the yl/sr-mnt/fix-image-view-padding branch November 5, 2024 19:34
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