Updateable Chip Span (allows for asynchronously loading images from urls/libraries) #39
Conversation
|
Bryan Gintz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
I had a git email screwup and some of the commits are from an unassociated account. I can resubmit a new PR if needed from the correct user (bagintz) |
|
@bagintz tnx for the pointer, I will check this today |
|
@simon-tse-hs Is the lack of CLA (because of using the wrong email on my end) prevent you from even looking at this? If so, I can resubmit. |
There was a problem hiding this comment.
@bagintz thanks for the PR. I read through the entire thing. I noticed the biggest change is:
- Inclusion of the StateListDrawable (which is just a drawable)
- Adding new attributes for the view
I'll make more comments in the actual code.
| private CharSequence mText; | ||
| private String mTextToDraw; | ||
|
|
||
| private StateListDrawable mIcon; |
There was a problem hiding this comment.
StateListDrawable is just a subclass of Drawable. Why couldn't you achieve this change by keeping this a Drawable?
There was a problem hiding this comment.
@simon-tse-hs it needs to be a StateListDrawable to support the changeIcon method (line 488). That method (and it's required StateListDrawable) is the main difference between the ChipSpan and UpdateableChipSpan.
| * @param text the text for the ChipSpan to display | ||
| * @param icon an optional icon (can be {@code null}) for the ChipSpan to display | ||
| */ | ||
| public UpdateableChipSpan(@NonNull Context context, @NonNull CharSequence text, @Nullable StateListDrawable icon, Object data) { |
There was a problem hiding this comment.
❓ This constructor is the essentially the same as the original one (just below this constructor). What are benefits are you getting by creating this new constructor? Can they be merged?
There was a problem hiding this comment.
Yes, you can just use the StateListDrawable constructor and make sure to pass in a single element StateListDrawable. Which should be fine since the only reason you would want to use the UpdateableChipSpan vs the regular ChipSpan is that you want to asynchronously update the image.
|
@bagintz if the constructors can be merged, then I argue that your small change could be made into the original |
|
@simon-tse-hs see my responses to your comments |
Summary
This PR has a new ChipSpan clone... UpdateableChipSpan which swaps out the Drawable Icon for a StateListDrawable. This StateListDrawable is initiated with the placeholder image (which is effectively the same as ChipSpan. Out of the box it will act the same as ChipSpan.
The difference is that you can use Picasso, Glide, Fresco or any other libraries that asynchronously return a drawable to update the Chip/ImageSpan by calling the new
UpdateableChipSpan#changeIcon(Drawable newIcon)method. You must call NachoTextView#invalidateChips() on your NachoTextView for it to properly update the view.It seems to work seamlessly with no side effects. I am sure there are more elegant ways to implement this, but it seemed the least invasive at the time.
*I noticed that the NachoTextView#init(AttributeSet attrs) defaults to the ChipSpan class, so I am not sure if there needs to be another method added there for this one????
Test Plan
Was going to duplicate the ChipSpanChipCreatorTest, but was unable to get it to pass locally so I am willing to write tests if I can get some direction/help with that