RUM-1012 Fix RippleDrawables#1600
Conversation
7f88d90 to
b7e6b45
Compare
| return result | ||
| } | ||
|
|
||
| private fun resolveDrawableProperties(view: View, drawable: Drawable?): DrawableProperties { |
| width, | ||
| height, | ||
| view.background, | ||
| view.background?.constantState?.newDrawable(), |
There was a problem hiding this comment.
why do you create a new drawable here ? isn't this costly for each view ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
I need to set the bounds before I can draw to the canvas
There was a problem hiding this comment.
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 Report
@@ 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
|
xgouchet
left a comment
xgouchet
left a comment
There was a problem hiding this comment.
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?
e8430b3 to
2bc4cfd
Compare
0dac84c to
564979a
Compare
e6aa860 to
3b0b069
Compare
3b0b069 to
6e89a6d
Compare
6e89a6d to
ffb98cd
Compare
4 participants
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)