Fix part of #5663: Use local thumbnails in dev builds#6136
Fix part of #5663: Use local thumbnails in dev builds#6136harshsomankar123-tech wants to merge 6 commits intooppia:developfrom
Conversation
|
Apologies--hoping to look at this tomorrow. |
BenHenning
left a comment
There was a problem hiding this comment.
@harshsomankar123-tech I left one comment on the PR and another on the issue. I don't think that this is being approached quite the correct way, PTAL.
Also, your PR description says that you've solved this with a custom ContentProvider but no ContentProvider has actually been proposed in code here.
| listOf() | ||
| } | ||
| if (lessonThumbnail.thumbnailFilename.isNotEmpty()) { | ||
| if (lessonThumbnail.thumbnailFilename.isNotEmpty() && loadThumbnailsFromGcs) { |
There was a problem hiding this comment.
Have you tested this with proto lessons @harshsomankar123-tech? I wouldn't expect this to work, actually. I left more thoughts in #5663 (comment). PTAL.
|
@BenHenning Thanks for the suggestions. I’ve made the changes mentioned in your comments.PTAL |
|
Unassigning @harshsomankar123-tech since a re-review was requested. @harshsomankar123-tech, please make sure you have addressed all review comments. Thanks! |
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @harshsomankar123-tech! This is definitely on the right track. I have a bunch of comments, PTAL.
Also, after you address all of my comments could you add a video to the PR description showing loading with protos having correct thumbnails? Please make sure to correctly set up loading via CachingModule since protos aren't loaded by default in local dev builds currently.
| sortOrder: String? | ||
| ): Cursor? = null | ||
|
|
||
| override fun getType(uri: Uri): String = "image/png" |
There was a problem hiding this comment.
Is this correct? Perhaps these could be SVGs, instead?
| "baker.img" to R.drawable.lesson_thumbnail_graphic_baker, | ||
| "child_with_book.img" to R.drawable.lesson_thumbnail_graphic_child_with_book, | ||
| "child_with_cupcakes.img" to R.drawable.lesson_thumbnail_graphic_child_with_cupcakes, | ||
| "child_with_fractions_homework.img" to | ||
| R.drawable.lesson_thumbnail_graphic_child_with_fractions_homework, | ||
| "duck_and_chicken.img" to R.drawable.lesson_thumbnail_graphic_duck_and_chicken, | ||
| "person_with_pie_chart.img" to R.drawable.lesson_thumbnail_graphic_person_with_pie_chart |
There was a problem hiding this comment.
Perhaps it would be cleaner to represent these as SVGs? We converted them to drawables originally because of how they were hooked up, but SVGs could be embedded as assets and returned directly (avoiding any temporarily file creation or loading needed).
| @@ -0,0 +1,111 @@ | |||
| package org.oppia.android.app.application.dev | |||
There was a problem hiding this comment.
I think org.oppia.android.app.utility.imageloading is probably an okay place for now. This is a new pattern so we don't have an obvious place for it to go, but the current package is meant for top-level application setup which doesn't seem like the correct place.
The new package will need to be a proper Bazel package (i.e. with a BUILD.bazel) file, and you'll need to reference that new library target as a dependency for the top-level //:oppia_dev target as one of its dependencies.
| is_published: true | ||
| has_practice_questions: true | ||
| topic_thumbnail { | ||
| thumbnail_filename: "child_with_fractions_homework.img" |
There was a problem hiding this comment.
Here & elsewhere: how did you decide on which thumbnails to use? We ought to match exactly the current JSON behavior to make it an easier before-and-after check.
app/src/main/AndroidManifest.xml
Outdated
| <provider | ||
| android:name=".app.application.dev.ThumbnailContentProvider" | ||
| android:authorities="org.oppia.android.provider" | ||
| android:exported="false" /> |
There was a problem hiding this comment.
This needs to go into its own AndroidManifest.xml in the same package as the content provider. We will rely on the manifest merge to include this at the end. Adding it here means we need to ship the provider in production which we absolutely don't want to do.
| @DefaultGcsPrefix | ||
| @Singleton | ||
| fun provideDefaultGcsPrefix(): String { | ||
| return "content://org.oppia.android.provider" |
There was a problem hiding this comment.
Let's use content://org.oppia.android.provider/gcs so as to not pollute future content provider potential.
| kt_android_library( | ||
| name = "dev_image_parsing_module", | ||
| srcs = [ | ||
| "DevImageParsingModule.kt", |
There was a problem hiding this comment.
The convention in the codebase is to use 'debug' for developer dependencies, and to put it at the end of the name before module, e.g. ImageParsingDebugModule. Please make sure to also update the library target name.
| addURI(AUTHORITY, "*/*/*/*/#", THUMBNAIL_MATCH) | ||
| addURI(AUTHORITY, "*/*/*/*", THUMBNAIL_MATCH) | ||
| addURI(AUTHORITY, "thumbnail/*", THUMBNAIL_MATCH) | ||
| addURI(AUTHORITY, "#", THUMBNAIL_MATCH) |
There was a problem hiding this comment.
Are all of these needed? It seems like we will subpath based on <entity_type>/<entity_id>/assets/<image|thumbnail>/<file.name> which seems like it only needs a single URI matcher, or am I missing something?
…arsingDebugModule, update authority
|
Hi @harshsomankar123-tech, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
|
@harshsomankar123-tech, please feel free to share any issues you are facing with this PR here or in Github Discussions. |
|
@adhiamboperes The CI is failing right now. I’ll work on fixing it and provide an update shortly. |
Explanation
Fix #5663 Part 01: Fixes activity thumbnail loading in developer builds by introducing a debug-only ContentProvider.
Context:
Currently, when
thumbnailFilenameis set in the proto files, it constructs a GCS URL and tries to fetch the image from the internet. Because developer builds don't have access to these GCS assets, the thumbnail loading fails. (The JSON loading path previously avoided this by falling back to localLessonThumbnailGraphicdrawables, but that path is being removed).Changes in this PR:
ContentProviderthat intercepts thumbnail requests. For developer builds, it serves the local drawable assets instead of attempting to fetch GCS images.(Note: This PR strictly handles the thumbnail fix for dev builds. The JSON loading path removal is handled separately).
Essential Checklist