Skip to content

RUM:4717: Improve CheckableTextViewMapper#2115

Merged
ambushwork merged 1 commit into
feature/session-replay/compound-button-mappersfrom
yl/improve-checkbox-mapper
Jul 2, 2024
Merged

RUM:4717: Improve CheckableTextViewMapper#2115
ambushwork merged 1 commit into
feature/session-replay/compound-button-mappersfrom
yl/improve-checkbox-mapper

Conversation

@ambushwork

@ambushwork ambushwork commented Jun 28, 2024

Copy link
Copy Markdown
Member

What does this PR do?

Summary

This PR improves the session replay CheckableTextViewMapper, tries to generate the wireframes which are much closer to the real UI elements.
Since CheckableTextViewMapper is a base class of the other mappers, following UI components are affected in session replay:

  • CheckedTextView
  • RadioButton
  • Checkbox

Main changes in the PR:

  • In CheckableWireFrameMapper, merge resolveCheckedCheckable() and resolveNotCheckedCheckable() functions into one function resolveCheckable, because we don't generate different ShapeWireframe for the "checked" or "not checked" states, instead, we retrieve the drawable to generate ImageWireframe.
  • In CheckableTextViewMapper, Inside the implementation of resolveCheckable we try to retrieve the drawable of "checked" and "not checked", and then set the ColorStateList of the view on the drawable to have the correct tint.
  • In CheckableCompoundButton, the bounds is adjusted for the drawable rendering, getCheckableDrawable is implemented to retrieve the drawable for any CompoundButton
  • In CheckedTextView getCheckableDrawable is implemented to retrieve the drawable for CheckedTextView.
  • Based on the facts above, no specific unit test is needed for each component to verify if the ShapeWireframe is correctly generated, we only need to test that ImageWireFrameHelper.createImageWireframe is called.
  • Add more UI demo in the sample application.

Demo

Sample Application Session Replay
Screen_recording_20240628_100222.webm https://mobile-integration.datadoghq.com/rum/replay/sessions/f187f5ef-23af-492e-abf6-69f4a248a441?applicationId=38030dde-f9f9-4e52-9443-b9804a030080&seed=141f3702-f9b4-47e9-876f-61219f0cf05c&ts=1719561697402

Motivation

  • RUM-4718 - Improve CheckedTextViewMapper
  • RUM:4717 - Improve CheckBoxMapper
  • RUM:4716 - Improve RadioButton

Additional Notes

Remain issue

  • Masked wireframe to decide
  • Resource 401 issue for CompoundButtonMapper for the version under 23

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 June 28, 2024 08:09
@ambushwork
ambushwork force-pushed the yl/improve-checkbox-mapper branch from 229a205 to 2f59c04 Compare June 28, 2024 08:54
@codecov-commenter

codecov-commenter commented Jun 28, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 60.00000% with 36 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature/session-replay/compound-button-mappers@7b69c91). Learn more about missing BASE report.
Report is 34 commits behind head on feature/session-replay/compound-button-mappers.

Additional details and impacted files 
@@                                Coverage Diff                                @@
##             feature/session-replay/compound-button-mappers    #2115   +/-   ##
=================================================================================
  Coverage                                                  ?   69.05%           
=================================================================================
  Files                                                     ?      700           
  Lines                                                     ?    26209           
  Branches                                                  ?     4416           
=================================================================================
  Hits                                                      ?    18098           
  Misses                                                    ?     6899           
  Partials                                                  ?     1212
Files Coverage Δ
.../sessionreplay/internal/DefaultRecorderProvider.kt 93.16% <100.00%> (ø)
...nreplay/internal/recorder/mapper/CheckBoxMapper.kt 100.00% <100.00%> (ø)
...ternal/recorder/mapper/CheckableWireframeMapper.kt 100.00% <100.00%> (ø)
...play/internal/recorder/mapper/RadioButtonMapper.kt 66.67% <100.00%> (ø)
...lay/internal/recorder/mapper/SwitchCompatMapper.kt 69.77% <ø> (ø)
...nternal/recorder/mapper/CheckableTextViewMapper.kt 55.56% <95.65%> (ø)
...roid/sessionreplay/internal/utils/DrawableUtils.kt 93.75% <50.00%> (ø)
.../internal/recorder/mapper/CheckedTextViewMapper.kt 78.12% <55.56%> (ø)
...l/recorder/mapper/CheckableCompoundButtonMapper.kt 48.28% <36.17%> (ø)

0xnm
0xnm approved these changes Jul 2, 2024
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.

lgtm. Although I don't have a deep knowledge of SR, so a review from someone else who is closer to the Session Replay topic than I am is preferable.

Comment thread .../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Comment thread .../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Comment thread .../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Comment thread ...om/datadog/android/sessionreplay/internal/recorder/mapper/BaseCheckableTextViewMapperTest.kt Outdated
Comment thread ...om/datadog/android/sessionreplay/internal/recorder/mapper/BaseCheckableTextViewMapperTest.kt
Comment thread sample/kotlin/src/main/res/color/checkbox_state_tint.xml Outdated
@ambushwork

ambushwork commented Jul 2, 2024

Copy link
Copy Markdown
Member Author

lgtm. Although I don't have a deep knowledge of SR, so a review from someone else who is closer to the Session Replay topic than I am is preferable.

Agree, @xgouchet @jonathanmos do you want to take a look at this?

@ambushwork
ambushwork force-pushed the yl/improve-checkbox-mapper branch from 2f59c04 to e3e842a Compare July 2, 2024 08:00
jonathanmos
jonathanmos reviewed Jul 2, 2024
View reviewed changes
Comment thread .../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt Outdated
Comment thread .../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Comment thread .../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Comment thread sample/kotlin/src/main/res/color/checkbox_state_tint.xml
@ambushwork
ambushwork force-pushed the yl/improve-checkbox-mapper branch 2 times, most recently from 8ffda54 to fa4d075 Compare July 2, 2024 09:46
@ambushwork
ambushwork force-pushed the yl/improve-checkbox-mapper branch from fa4d075 to 79dce7f Compare July 2, 2024 10:03
@ambushwork
ambushwork merged commit 8491d83 into feature/session-replay/compound-button-mappers Jul 2, 2024
@ambushwork
ambushwork deleted the yl/improve-checkbox-mapper branch July 2, 2024 13:20
@xgouchet xgouchet added this to the 2.12.x milestone Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xnm 0xnm 0xnm approved these changes
@jonathanmos jonathanmos jonathanmos 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.12.x

Development

Successfully merging this pull request may close these issues.

5 participants

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