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

Fix ButtonCompositionGroupMapper crash when calculating the corner radius #2173

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

ambushwork
Copy link
Member

What does this PR do?

Fix the crash caused by ButtonCompositionGroupMapper when calculating the corner radius

Motivation

A crash is found when I test with an Android open source application,

here is the trace:

 java.lang.IllegalStateException: Size is unspecified
       at androidx.compose.ui.geometry.Size.getWidth-impl(Size.kt:48)
       at androidx.compose.ui.geometry.Size.getMinDimension-impl(Size.kt:128)
       at androidx.compose.foundation.shape.PercentCornerSize.toPx-TmRCtEA(CornerSize.kt:100)
       at com.datadog.android.sessionreplay.compose.internal.mappers.ButtonCompositionGroupMapper.map(ButtonCompositionGroupMapper.kt:42)
       at com.datadog.android.sessionreplay.compose.internal.mappers.AbstractCompositionGroupMapper.map(AbstractCompositionGroupMapper.kt:37)

Demo

In the demo we can see the corner radius of the button is correct

Application screen Session Replay Screen
image image

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 requested review from a team as code owners August 8, 2024 08:36
@ambushwork ambushwork force-pushed the yl/fix/compose-button-mapper-crash branch from f1ef9ee to ea6a88a Compare August 8, 2024 08:59
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.27%. Comparing base (56b3fe3) to head (ea6a88a).
Report is 3 commits behind head on feature/sr_compose.

Files Patch % Lines
...e/internal/mappers/ButtonCompositionGroupMapper.kt 0.00% 4 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           feature/sr_compose    #2173      +/-   ##
======================================================
+ Coverage               82.13%   82.27%   +0.14%     
======================================================
  Files                     512      512              
  Lines                   18295    18289       -6     
  Branches                 2814     2814              
======================================================
+ Hits                    15026    15047      +21     
+ Misses                   2506     2481      -25     
+ Partials                  763      761       -2     
Files Coverage Δ
...e/internal/mappers/ButtonCompositionGroupMapper.kt 7.14% <0.00%> (-0.55%) ⬇️

... and 28 files with indirect coverage changes

@@ -33,13 +33,19 @@ internal class ButtonCompositionGroupMapper(
var cornerRadius: Number? = null
var colors: ButtonColors? = null

// Size of the button must be specified in order to calculate the corner radius,
// when [CornerSize] is a percentage.
Copy link
Member

Choose a reason for hiding this comment

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

In which cases can a CornerSize be a percentage (or not)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It totally depends on the choice of our clients (app developers) how they want to implement UI, either they can use a corner with specific dimension size, so no matter what the size of the button is, the corner keeps same. Either they can use percentage like I show in the screenshot, so the corner radius adapt with button size.

In this case, we must support both of them.

Copy link
Member

@mariusc83 mariusc83 left a comment

Choose a reason for hiding this comment

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

Good catch

@ambushwork ambushwork merged commit f2ffae5 into feature/sr_compose Aug 12, 2024
20 checks passed
@ambushwork ambushwork deleted the yl/fix/compose-button-mapper-crash branch August 12, 2024 07:55
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.

4 participants