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-1012 Fix RippleDrawables #1600

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

jonathanmos
Copy link
Contributor

@jonathanmos jonathanmos commented Sep 6, 2023

What does this PR do?

Fixes RippleDrawables not converting correctly to base64 (caused by it's dimensions being -1, and RippleDrawable not being the actual drawable from which we can create a bitmap). In this fix, the InsetDrawable is extracted from the RippleDrawable, and the GradientDrawable is extracted from that - we then use the dimensions of the view as the dimensions of the drawable (this doesn't affect the created bitmap so much, since the bitmap is rescaled to 10kbs or less before caching).

Motivation

What inspired you to submit this pull request?

Additional Notes

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)

@jonathanmos jonathanmos requested a review from a team as a code owner September 6, 2023 09:40
@jonathanmos jonathanmos force-pushed the jmoskovich/rum-1012/fix-rippledrawables branch from 7f88d90 to b7e6b45 Compare September 6, 2023 09:52
@@ -129,6 +135,17 @@ internal class ImageWireframeHelper(
return result
}

private fun resolveDrawableProperties(view: View, drawable: Drawable?): DrawableProperties {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice ;)

@@ -180,7 +180,7 @@ abstract class BaseWireframeMapper<T : View, S : MobileSegment.Wireframe>(
y = bounds.y,
width,
height,
view.background,
view.background?.constantState?.newDrawable(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you create a new drawable here ? isn't this costly for each view ?

Copy link
Contributor Author

@jonathanmos jonathanmos Sep 6, 2023

Choose a reason for hiding this comment

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

preferably we wouldn't need to do this, however if the drawable is not moved to a new instance before we create the bitmap, then modifying the drawable to draw the bitmap modifies the view in the app itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok I see so you need to take a copy ...hmmm...I wonder how expensive this is for each view. Sorry for asking but why do you need to modify the Drawable to create the bitmap from it ?

Copy link
Contributor Author

@jonathanmos jonathanmos Sep 6, 2023

Choose a reason for hiding this comment

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

I need to set the bounds before I can draw to the canvas

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say that if profiling it shows decent number we can go with this like that. @0xnm, @xgouchet waiting for your thoughts on this one. I searched for a way to go around it, but apparently there's not, the only other way is to setBounds when before drawing and try to put the drawable back into its previous state after. Not sure how reliable this is and honestly I think I would rather go with the copy.

@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2023

Codecov Report

Merging #1600 (ffb98cd) into develop (aa1c2e1) will decrease coverage by 0.11%.
Report is 20 commits behind head on develop.
The diff coverage is 73.48%.

@@             Coverage Diff             @@
##           develop    #1600      +/-   ##
===========================================
- Coverage    83.67%   83.55%   -0.11%     
===========================================
  Files          451      452       +1     
  Lines        15563    15625      +62     
  Branches      2318     2321       +3     
===========================================
+ Hits         13021    13055      +34     
- Misses        1928     1948      +20     
- Partials       614      622       +8     
Files Changed Coverage Δ
...replay/internal/recorder/wrappers/BitmapWrapper.kt 14.29% <17.65%> (-5.71%) ⬇️
...replay/internal/recorder/wrappers/CanvasWrapper.kt 15.79% <23.08%> (+1.50%) ⬆️
...ay/internal/recorder/mapper/BaseWireframeMapper.kt 84.06% <33.33%> (-2.71%) ⬇️
...replay/internal/recorder/wrappers/Base64Wrapper.kt 25.00% <37.50%> (+5.00%) ⬆️
...y/internal/recorder/base64/ImageWireframeHelper.kt 96.70% <89.47%> (-0.77%) ⬇️
...eplay/internal/recorder/base64/Base64Serializer.kt 94.59% <95.92%> (+10.07%) ⬆️
...essionreplay/internal/recorder/LayerDrawableExt.kt 100.00% <100.00%> (ø)
...nreplay/internal/recorder/base64/Base64LRUCache.kt 75.61% <100.00%> (-1.13%) ⬇️
...ssionreplay/internal/recorder/base64/BitmapPool.kt 87.50% <100.00%> (+0.23%) ⬆️
...roid/sessionreplay/internal/utils/DrawableUtils.kt 96.00% <100.00%> (+0.26%) ⬆️

... and 18 files with indirect coverage changes

Copy link
Collaborator

@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.

I'm not sure I understand the need to have the ripple drawables converted to base64?
By definition a RippleDrawable is an animated drawable used to show feedback when tapping on interactive views (e.g. buttons).
Saving those as bitmaps means we'll either have to save the whole animation because everytime we draw them the animation state will be different, or it's going to only store a single ripple state which will look weird in the replay.
Either way, should'n we instead save them as a solid background for now, and wait for a better solution to record/replay the ripple effect?

@jonathanmos jonathanmos force-pushed the jmoskovich/rum-1012/fix-rippledrawables branch 2 times, most recently from e8430b3 to 2bc4cfd Compare September 7, 2023 11:17
@jonathanmos jonathanmos force-pushed the jmoskovich/rum-1012/fix-rippledrawables branch 2 times, most recently from 0dac84c to 564979a Compare September 10, 2023 21:45
mariusc83
mariusc83 previously approved these changes Sep 12, 2023
xgouchet
xgouchet previously approved these changes Sep 13, 2023
@jonathanmos jonathanmos force-pushed the jmoskovich/rum-1012/fix-rippledrawables branch from 6e89a6d to ffb98cd Compare September 13, 2023 13:20
@jonathanmos jonathanmos merged commit a38fab8 into develop Sep 14, 2023
5 checks passed
@jonathanmos jonathanmos deleted the jmoskovich/rum-1012/fix-rippledrawables branch September 14, 2023 06:49
@xgouchet xgouchet added this to the 2.2.0 milestone Dec 13, 2023
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