-
Notifications
You must be signed in to change notification settings - Fork 534
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 #5631: Fix thumbnail loading for multiple classrooms #5642
Conversation
…ctly inflate xml view)
Hey @BenHenning, as discussed in chat, we can use this PR to test again with the production assets. PTAL. |
Coverage ReportResultsNumber of files assessed: 11 Exempted coverageFiles exempted from coverage
|
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 4868 bytes (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 4619 bytes (Added) Method count: 260202 (old), 260231 (new), 29 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6818 (old), 6818 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 4872 bytes (Added) Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 39 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 40 KiB (Removed) Method count: 116281 (old), 115770 (new), 511 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 39 KiB (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 38 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 40 KiB (Removed) Method count: 116287 (old), 115776 (new), 511 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 10 MiB (new), 38 KiB (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 38 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 39 KiB (Removed) Method count: 116287 (old), 115776 (new), 511 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 10 MiB (new), 38 KiB (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
Hi @theMr17, 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. |
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 @theMr17! Can you please attach screenshots to the main comment showing that this works with multiple classrooms enabled at least for test data locally? I think you may need to check out an earlier version of develop (before Gradle was removed) and merge this in to verify (since thumbnails don't currently load for Bazel builds--which is something we will need to fix eventually).
Otherwise, the changes seem quite sensible! Only had a few comments--PTAL.
app/src/main/java/org/oppia/android/app/classroom/ClassroomListViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/classroom/ThumbnailImage.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Ben Henning <ben@oppia.org>
Coverage ReportResultsNumber of files assessed: 34 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
Hey @BenHenning, thanks for the review! I've addressed your comments and added screenshots to the PR description, PTAL. I didn't need to check out older commits for the Gradle build; image loading works fine for me with Bazel builds as well. Regarding the classroom thumbnails, they already have a background. The production assets should have the correct background color set for them to display properly. |
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 2300 bytes (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 801 bytes (Added) Method count: 260219 (old), 260231 (new), 12 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6818 (old), 6818 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 2300 bytes (Added) Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 39 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 43 KiB (Removed) Method count: 116287 (old), 115770 (new), 517 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 39 KiB (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 39 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 40 KiB (Removed) Method count: 116293 (old), 115776 (new), 517 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 10 MiB (new), 39 KiB (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 39 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 41 KiB (Removed) Method count: 116293 (old), 115776 (new), 517 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 10 MiB (new), 39 KiB (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
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 @theMr17! I'm actually surprised to hear that the Bazel build actually has working thumbnails. Perhaps it's because the JSON assets are still being loaded.
This otherwise LGTM--really happy to see this get in quickly as it will allow us to test multiple classrooms in alpha. :)
Merging with the latest develop & enabling auto-merge. |
Unassigning @BenHenning since they have already approved the PR. |
Assigning @adhiamboperes for code owner reviews. Thanks! |
Coverage ReportResultsNumber of files assessed: 11 Exempted coverageFiles exempted from coverage
|
Explanation
Fixes #5631
This PR introduces a new
ThumbnailImage
composable that integrates theLessonThumbnailImageView
within a Compose layout. The changes include:ThumbnailImage
composable.ThumbnailImage
composable.LessonThumbnailImageView
.Note: Initial attempts were made to use the Glide Compose library, but was found to be incompatible with other dependencies #5631 (comment).
Screenshots
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: