Skip to content

RUM-3460: Fix padding and resizing issue for image view mapper#2372

Merged
ambushwork merged 1 commit into
developfrom
yl/sr-mnt/fix-image-view-padding
Nov 5, 2024
Merged

RUM-3460: Fix padding and resizing issue for image view mapper#2372
ambushwork merged 1 commit into
developfrom
yl/sr-mnt/fix-image-view-padding

Conversation

@ambushwork

@ambushwork ambushwork commented Nov 4, 2024

Copy link
Copy Markdown
Member

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

codecov-commenter commented Nov 5, 2024

Copy link
Copy Markdown

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

@xgouchet xgouchet left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
@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