Implement cohesive topic & exploration asset downloader #169
Assignees
Labels
enhancement
End user-perceivable enhancements.
Impact: Medium
Moderate perceived user impact (non-blocking bugs and general improvements).
Issue: Needs Break-down
Indicates that an issue is too large and should be broken into smaller chunks.
Issue: Needs Clarification
Indicates that an issue needs more detail in order to be able to be acted upon.
Priority: Essential
This work item must be completed for its milestone.
Work: Medium
The means to find the solution is clear, but it isn't at good-first-issue level yet.
Z-ibt
Temporary label for Ben to keep track of issues he's triaged.
Comments
BenHenning
added
Type: Improvement
Priority: Essential
|
This is more or less blocked on all other controllers being built first since it requires extensive reworking of those controllers once they're established. |
1 task
BenHenning
added
Priority: Important
BenHenning
modified the milestones:
Prototype -- Infra,
Prototype -- Important fast-follows
This was referenced Oct 20, 2019
This was referenced Nov 17, 2019
|
NB: This needs to also include ensuring we properly parse all classifier inputs. |
|
Note that this is changing slightly. We're going to build a version of this system out of the app so that we can properly bundle topics with the app (though the alpha actually does support downloading relevant assets today). For new topics & topic updates, the backend will be responsible for collecting assets. I'm working on the first part, @Srikanth-Kb will be working on the second part. |
8 tasks
Broppia
added
issue_type_infrastructure
Impact: Low
BenHenning
added
issue_user_developer
Impact: Medium
seanlip
added
enhancement
adhiamboperes
added
the
Work: Medium
BenHenning
changed the title
BenHenning
added a commit
that referenced
this issue
May 29, 2024
… for (multiple) classrooms & topic dependencies, and prepare for #4885 (#5398) ## Explanation Fixes #1547 Fixes part of #169 Fixes part of #5344 Fixes part of #5365 Fixes part of #5411 The main purpose of this PR is to introduce support for multiple classrooms in the data layer of the app (with minimal domain integration to avoid the change extending beyond the lesson structures). However, the PR is also doing a few more things including preparing the Android codebase for introducing an asset download script (#4885) and other peripheral cleanups of code (rather than updating it) that was noticed along the way. ### Preparing for multiple classrooms This addresses part of #5365. #5344 is introducing support for multiple classrooms in the app. To help prepare for those changes, this PR introduces the necessary data structure and domain loading changes to load a new proto structure: ```proto message ClassroomList { repeated ClassroomRecord classrooms = 1; } message ClassroomRecord { string id = 1; map<string, TranslationMapping> written_translations = 2; SubtitledHtml translatable_title = 3; LessonThumbnail classroom_thumbnail = 4; map<string, TopicIdList> topic_prerequisites = 5; message TopicIdList { repeated string topic_ids = 1; } } ``` Rather than just a flat topic list. Some important details to note: - The recommended topics structure has been updated to use this new ``topic_prerequisites`` value being loaded from classrooms. This will also extend to production assets since the asset download script from #4885 is also being updated to include support for this multiple classrooms structure to address the remainder of #5365. - To minimize domain changes, the new loading code assumes only **one** classroom is present. TODOs have been added on #5344 to extend this to support multiple classrooms. - Current loading code (including for JSON) is ignoring all but: ``classrooms``, ``id``, and ``topic_prerequisites`` (including ``topic_ids``) from the protos above. These other fields are expected to have supported added as part of #5344. - There were some color simplifications made in ``TopicListController``. These largely shouldn't have a major impact outside of developer assets. These changes were made to ensure non-specificity to the previous lesson classroom. In general, all of this should eventually be removed in favor of loading colors from lesson assets, but that will need to wait until the JSON pipeline is completely removed. ### Asset priming removal This addresses part of #169. ``PrimeTopicAssetsController`` and its implementation were responsible for hackily pre-loading all lesson images and audio to be on-device to enable offline support. This was the first attempt at offline support early in the app's development, but it had a few significant drawbacks: - It required preloading everything upon first app open. Since it can take a while for loading to occur, some robustness needed to be built in for pausing, cancelling, and resuming. I'm not certain if these were even 100% handled in the current implementation. - It doesn't perform strong compatibility checks until you're in the app which means lesson incompatibilities would just cause the app to get stuck rather than addressing it during lesson import time (e.g. via an asset downloader script). - It required very significant workarounds to existing loading pipelines that aren't ideal to keep in the codebase long-term (code smell). - There's no guarantee the user even has enough disk space to download all the needed assets (particularly audio), or if they'll have sufficient internet connectivity & bandwidth to perform those downloads upon first app open. This approach was abandoned after the earliest alpha releases for an asset download script (which is now being migrated over to this codebase per #5411. This removal unfortunately required removing a module that was referenced in a lot of tests throughout the codebase. While the removal itself was fairly simple, it does affect a lot of files. Other areas changed (but unaffected by tests since these flows didn't have automated tests): - ``SplashActivityPresenter`` for enabling the downloader to start and block the UI using a dialog box while the downloading occurred. - ``AudioPlayerController` for removing the special loading logic for primed audio files (the app now no longer supports loading audio files from disk as we don't yet have a good long-term solution for offline-available audio files due to their significant size). - ``GlideImageLoader`` for removing support for loading locally cached images (only through this flow; the flow we use for the asset download script uses a different annotation and is still supported). As alluded to above, two annotations were removed as part of this cleanup: - ``CacheAssetsLocally``: a boolean indicating whether the prime download flow should be enabled. - ``TopicListToCache``: this specified the list of topics to pre-download if the flow was enabled. ### GAE structure cleanup & preparing for asset script Preparing for #4885 included a few other changes, some of which were nice-to-have: - Introducing support for incorporating the protos from https://github.com/oppia/oppia-proto-api (specifically oppia/oppia-proto-api#1 since they are still a work-in-progress). - Adding ``java_proto`` versions for many of the app's proto structures (since the download script runs in the JVM and doesn't use the javalite proto environment). - Removed all of the unused GAE services and models (addressing #1547 by essentially making it obsolete), plus their mocks. These were never hooked up, and they're never going to be: the long-term plan for the codebase is to use new proto endpoints that will be added to Oppia web. These Retrofit endpoints have actually been rebuilt and repurposed to be used in the asset download script as a medium-term stop-gap until the permanent proto endpoints can be added to Oppia web. - ``RemoteAuthNetworkInterceptorTest`` was updated to use a different example service since ``TopicService`` has been removed. The services for platform parameters and user feedback are being kept. - Some test file & KDoc exemptions have been removed since their corresponding files have been deleted. Note that the new Java proto targets & oppia-proto-api targets aren't being used yet, but they will be in future PRs. This just provides the basis of support for the asset download script while also helping to split up the code to review across multiple PRs. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only This is essentially only data infrastructural except for a couple of points: - Topic loading is now happening through a classrooms structure rather than a tropic ID list. Since there's only one test & one production classroom, this shouldn't actually change the UX. - An at-app-open flow for predownloading image & audio assets has been removed. This hasn't been used since the very earliest alpha releases of the app, so it won't actually affect any users. - Some colors for developer lesson and topic thumbnails were updated--see above.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The current GAE service modeling & the way domain controllers are split up, each piece of the learning experience (topics, explorations, etc.) is currently downloaded directly when needed. This is due to the initial Android implementation reflecting the base web interface. However, in order to properly support offline support, topics need to be downloaded at one time including corresponding exploration images.
There are two ways to do such a download:
Download a specified topic, recurse through all constituent pieces (including non-sound exploration assets), and download each one to provide a single repository for all things corresponding to that topic.
Update Oppia backend to provide a single endpoint that downloads the entire blob of data.
Option (2) is likely to be more performant and reliable, since it can package everything into a compressed zip file and provide a content length ahead of time (which is useful for indicating download progress per #45).
The intended approach is to implement (1) first, and later move to (2) via an effort on the backend.
The text was updated successfully, but these errors were encountered: