-
Notifications
You must be signed in to change notification settings - Fork 506
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
Fix #1039: Introduce custom type oppia image [WIP] #2012
Fix #1039: Introduce custom type oppia image [WIP] #2012
Conversation
urlDrawable, { it } | ||
) | ||
private fun findImageType(urlDrawable: UrlDrawable, oppiaImage: OppiaImage): CustomImageTarget { | ||
return when(oppiaImage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am stuck here, what should be the type Argument here for CustomImageTarget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenHenning Can you please help me here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want the target to be an OppiaImage
but you need to update the Glide pipeline to support OppiaImage as a valid image type so that it knows how to bind it to a target (e.g. by extracting its bitmap or picture).
The idea behind OppiaImage
is that it's self-service: you just load an Oppia image and bind it to a CustomImageTarget
and Oppia's Glide integration takes care of the rest.
You can take a look at how we set up the custom support for Picture today--Glide doesn't support SVGs out of the box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenHenning I need a bit more help here. How could I update Glide pipeline to support our OppiaImage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prayutsu take a look at RepositoryGlideModule. It registers a couple of custom type translators: SVG -> PictureDrawable, and InputStream -> SVG. We select this custom pipeline in GlideImageLoader by asking Glide to load a 'PictureDrawable' and it knows how to map from PictureDrawable to SVG to InputStream (which it will retrieve & provide for us). My suggestion is investigating whether we could set up a similar pipeline from Bitmaps/Pictures to the custom Oppia image type so that we can load that from Glide directly.
Note that we would probably still specialize between SVG & Bitmap, e.g.:
Glide.with(context)
.`as`(OppiaImage.BitmapImage::class.java)
.load(model)
.intoTarget(target)
Glide.with(context)
.`as`(OppiaImage.SvgImage::class.java)
.fitCenter()
.apply(RequestOptions.diskCacheStrategyOf(DiskCacheStrategy.NONE))
.load(model)
.intoTarget(target)
Does that help clarify?
@BenHenning Is the PR progressing in the right direction? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @prayutsu! I think this is on the right track, and I left some design comments.
Also, I suggest thinking through how you're going to test this. I don't think the current image loading pipeline has any tests, and it would be very helpful to get some added. Let me know if you run into any problems with that.
urlDrawable, { it } | ||
) | ||
private fun findImageType(urlDrawable: UrlDrawable, oppiaImage: OppiaImage): CustomImageTarget { | ||
return when(oppiaImage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want the target to be an OppiaImage
but you need to update the Glide pipeline to support OppiaImage as a valid image type so that it knows how to bind it to a target (e.g. by extracting its bitmap or picture).
The idea behind OppiaImage
is that it's self-service: you just load an Oppia image and bind it to a CustomImageTarget
and Oppia's Glide integration takes care of the rest.
You can take a look at how we set up the custom support for Picture today--Glide doesn't support SVGs out of the box.
class BitmapImage(val urlDrawable: UrlImageParser.UrlDrawable) : OppiaImage() | ||
class SvgImage(val urlDrawable: UrlImageParser.UrlDrawable) : OppiaImage() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be referencing UrlImageParser
things here--that's a dependency inversion. OppiaImage
is part of image loading, and UrlImageParser
depends on image loading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* internet. | ||
*/ | ||
fun loadSvg(imageUrl: String, target: ImageTarget<PictureDrawable>) | ||
fun loadOppiaImage(imageUrl: String, target: ImageTarget<OppiaImage>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's okay to have both loadBitmap and loadSvg--we probably shouldn't assume a URL ends with svg. This is a not-so-great assumption by UrlImageParser
, but better to do it at the HTML level than in the actual image loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, reverted these changes
Actually, right now I have no idea how will we test this but as the PR gets closer to correct implementation, I'll try it. |
@prayutsu it's fine to approach tests after getting a working implementation, though we should keep the PR in draft until your tests are also finalized & ready for review. |
Closing the PR as unassigning myself from the issue |
Explanation
Fixes #1039:
Custom Type OppiaImage is introduced
Checklist