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:4717: Improve CheckableTextViewMapper #2115

Conversation

ambushwork
Copy link
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
Copy link

codecov-commenter commented Jun 28, 2024

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%> (ø)

Copy link
Member

@0xnm 0xnm left a comment

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.

@ambushwork
Copy link
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
@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
20 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants