From cbcba031611756d36668dd89d34fb25079f686ca Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sat, 16 Nov 2019 18:21:44 -0800 Subject: [PATCH 1/8] Ensured progress, thumbnails, and stats are consistent. This change ensures that completed progress is consistent and correct everywhere for fractions and ratios topics. It also ensures that there are thumbnails defined for chapters, and that all thumbnails are consistent regardless of which screen topics, stories, or chapters are viewed from. It updates the reported lesson count to be correct. --- .../home/topiclist/TopicSummaryViewModel.kt | 2 +- .../main/res/layout/topic_summary_view.xml | 2 +- .../domain/topic/StoryProgressController.kt | 16 +- .../org/oppia/domain/topic/TopicController.kt | 136 +++------ .../oppia/domain/topic/TopicListController.kt | 274 +++++++++++------- .../topic/StoryProgressControllerTest.kt | 4 +- .../oppia/domain/topic/TopicControllerTest.kt | 6 +- .../domain/topic/TopicListControllerTest.kt | 67 +++-- model/src/main/proto/topic.proto | 3 - 9 files changed, 268 insertions(+), 242 deletions(-) diff --git a/app/src/main/java/org/oppia/app/home/topiclist/TopicSummaryViewModel.kt b/app/src/main/java/org/oppia/app/home/topiclist/TopicSummaryViewModel.kt index 722ec79a86b..ae9e05fe946 100755 --- a/app/src/main/java/org/oppia/app/home/topiclist/TopicSummaryViewModel.kt +++ b/app/src/main/java/org/oppia/app/home/topiclist/TopicSummaryViewModel.kt @@ -19,7 +19,7 @@ class TopicSummaryViewModel( private val topicSummaryClickListener: TopicSummaryClickListener ) : HomeItemViewModel() { val name: String = topicSummary.name - val canonicalStoryCount: Int = topicSummary.canonicalStoryCount + val totalChapterCount: Int = topicSummary.totalChapterCount @ColorInt val backgroundColor: Int = retrieveBackgroundColor() @ColorInt diff --git a/app/src/main/res/layout/topic_summary_view.xml b/app/src/main/res/layout/topic_summary_view.xml index c1cbe77b872..1ceaebd1975 100755 --- a/app/src/main/res/layout/topic_summary_view.xml +++ b/app/src/main/res/layout/topic_summary_view.xml @@ -100,7 +100,7 @@ android:layout_width="match_parent" android:layout_height="wrap_content" android:fontFamily="sans-serif-light" - android:text="@{@plurals/lesson_count(viewModel.canonicalStoryCount, viewModel.canonicalStoryCount)}" + android:text="@{@plurals/lesson_count(viewModel.totalChapterCount, viewModel.totalChapterCount)}" android:textColor="@color/white_80" android:textSize="14sp" android:textStyle="italic" diff --git a/domain/src/main/java/org/oppia/domain/topic/StoryProgressController.kt b/domain/src/main/java/org/oppia/domain/topic/StoryProgressController.kt index 12e83674a30..d6018070241 100644 --- a/domain/src/main/java/org/oppia/domain/topic/StoryProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/StoryProgressController.kt @@ -28,6 +28,9 @@ const val RATIOS_EXPLORATION_ID_0 = "2mzzFVDLuAj8" const val RATIOS_EXPLORATION_ID_1 = "5NWuolNcwH6e" const val RATIOS_EXPLORATION_ID_2 = "k2bQ7z5XHNbK" const val RATIOS_EXPLORATION_ID_3 = "tIoSb3HZFN6e" +private val FRACTIONS_COMPLETED_CHAPTERS = listOf(FRACTIONS_EXPLORATION_ID_0) +private val RATIOS_COMPLETED_CHAPTERS = listOf() +val COMPLETED_EXPLORATIONS = FRACTIONS_COMPLETED_CHAPTERS + RATIOS_COMPLETED_CHAPTERS /** Controller that records and provides completion statuses of chapters within the context of a story. */ @Singleton @@ -68,6 +71,11 @@ class StoryProgressController @Inject constructor( } } + // TODO(#21): Hide this functionality behind a data provider rather than punching a hole in this controller. + internal fun retrieveStoryProgress(storyId: String): StoryProgress { + return createStoryProgressSnapshot(storyId) + } + private fun trackCompletedChapter(storyId: String, explorationId: String) { check(storyId in trackedStoriesProgress) { "No story found with ID: $storyId" } trackedStoriesProgress.getValue(storyId).markChapterCompleted(explorationId) @@ -91,19 +99,13 @@ class StoryProgressController @Inject constructor( private fun createStoryProgressForJsonStory(fileName: String, index: Int): TrackedStoryProgress { val storyData = jsonAssetRetriever.loadJsonFromAsset(fileName)?.getJSONArray("story_list")!! - if (storyData.length() < index) { - return TrackedStoryProgress( - chapterList = listOf(), - completedChapters = setOf() - ) - } val explorationIdList = getExplorationIdsFromStory( storyData.getJSONObject(index).getJSONObject("story") .getJSONObject("story_contents").getJSONArray("nodes") ) return TrackedStoryProgress( chapterList = explorationIdList, - completedChapters = setOf() + completedChapters = COMPLETED_EXPLORATIONS.filter(explorationIdList::contains).toSet() ) } diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt index 51ec851bde0..733625a94e9 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt @@ -7,8 +7,6 @@ import org.json.JSONObject import org.oppia.app.model.ChapterPlayState import org.oppia.app.model.ChapterSummary import org.oppia.app.model.ConceptCard -import org.oppia.app.model.LessonThumbnail -import org.oppia.app.model.LessonThumbnailGraphic import org.oppia.app.model.Question import org.oppia.app.model.SkillSummary import org.oppia.app.model.StorySummary @@ -62,23 +60,35 @@ private const val QUESTION_DATA_PROVIDER_ID = "QuestionDataProvider" class TopicController @Inject constructor( private val dataProviders: DataProviders, private val jsonAssetRetriever: JsonAssetRetriever, - private val stateRetriever: StateRetriever + private val stateRetriever: StateRetriever, + private val storyProgressController: StoryProgressController ) { /** Returns the [Topic] corresponding to the specified topic ID, or a failed result if no such topic exists. */ fun getTopic(topicId: String): LiveData> { return MutableLiveData( - when (topicId) { - TEST_TOPIC_ID_0 -> AsyncResult.success(createTestTopic0()) - TEST_TOPIC_ID_1 -> AsyncResult.success(createTestTopic1()) - FRACTIONS_TOPIC_ID -> AsyncResult.success(createTopicFromJson( - "fractions_topic.json", "fractions_skills.json", "fractions_stories.json")) - RATIOS_TOPIC_ID -> AsyncResult.success(createTopicFromJson( - "ratios_topic.json", "ratios_skills.json", "ratios_stories.json")) - else -> AsyncResult.failed(IllegalArgumentException("Invalid topic ID: $topicId")) + try { + AsyncResult.success(retrieveTopic(topicId)) + } catch (e: Exception) { + AsyncResult.failed(e) } ) } + // TODO(#21): Expose this as a data provider, or omit if it's not needed. + internal fun retrieveTopic(topicId: String): Topic { + return when (topicId) { + TEST_TOPIC_ID_0 -> createTestTopic0() + TEST_TOPIC_ID_1 -> createTestTopic1() + FRACTIONS_TOPIC_ID -> createTopicFromJson( + "fractions_topic.json", "fractions_skills.json", "fractions_stories.json" + ) + RATIOS_TOPIC_ID -> createTopicFromJson( + "ratios_topic.json", "ratios_skills.json", "ratios_stories.json" + ) + else -> throw IllegalArgumentException("Invalid topic ID: $topicId") + } + } + // TODO(#173): Move this to its own controller once structural data & saved progress data are better distinguished. /** Returns the [StorySummary] corresponding to the specified story ID, or a failed result if there is none. */ @@ -314,14 +324,7 @@ class TopicController @Inject constructor( .addSkill(createTestTopic0Skill1()) .addSkill(createTestTopic0Skill2()) .addSkill(createTestTopic0Skill3()) - .setTopicThumbnail(createTestTopic0Thumbnail()) - .build() - } - - private fun createTestTopic0Thumbnail(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_BOOK) - .setBackgroundColorRgb(0xd5836f) + .setTopicThumbnail(createTopicThumbnail0()) .build() } @@ -337,7 +340,7 @@ class TopicController @Inject constructor( ) .addStory(createTestTopic1Story2()) .addSkill(createTestTopic1Skill0()) - .setTopicThumbnail(createTestTopic1Thumbnail()) + .setTopicThumbnail(createTopicThumbnail1()) .build() } @@ -345,13 +348,14 @@ class TopicController @Inject constructor( * a key called 'topic' that holds the topic data. */ private fun createTopicFromJson(topicFileName: String, skillFileName: String, storyFileName: String): Topic { val topicData = jsonAssetRetriever.loadJsonFromAsset(topicFileName)?.getJSONObject("topic")!! + val topicId = topicData.getString("id") return Topic.newBuilder() - .setTopicId(topicData.getString("id")) + .setTopicId(topicId) .setName(topicData.getString("name")) .setDescription(topicData.getString("description")) .addAllSkill(createSkillsFromJson(skillFileName)) .addAllStory(createStoriesFromJson(storyFileName)) - .setTopicThumbnail(createTopicThumbnail(topicFileName)) + .setTopicThumbnail(TOPIC_THUMBNAILS.getValue(topicId)) .build() } @@ -394,55 +398,39 @@ class TopicController @Inject constructor( } private fun createStoryFromJson(storyData: JSONObject): StorySummary { + val storyId = storyData.getString("id") return StorySummary.newBuilder() - .setStoryId(storyData.getString("id")) + .setStoryId(storyId) .setStoryName(storyData.getString("title")) .addAllChapter( createChaptersFromJson( - storyData.getJSONObject("story_contents").getJSONArray("nodes") + storyId, storyData.getJSONObject("story_contents").getJSONArray("nodes") ) ) .build() } - private fun createChaptersFromJson(chapterData: JSONArray): List { + private fun createChaptersFromJson(storyId: String, chapterData: JSONArray): List { val chapterList = mutableListOf() + val storyProgress = storyProgressController.retrieveStoryProgress(storyId) + val chapterProgressMap = storyProgress.chapterProgressList.map { progress -> + progress.explorationId to progress + }.toMap() for (i in 0 until chapterData.length()) { val chapter = chapterData.getJSONObject(i) + val explorationId = chapter.getString("exploration_id") chapterList.add( ChapterSummary.newBuilder() - .setExplorationId(chapter.getString("exploration_id")) + .setExplorationId(explorationId) .setName(chapter.getString("title")) - .setChapterPlayState(ChapterPlayState.NOT_STARTED) + .setChapterPlayState(chapterProgressMap.getValue(explorationId).playState) + .setChapterThumbnail(EXPLORATION_THUMBNAILS.getValue(explorationId)) .build() ) } return chapterList } - private fun createTopicThumbnail(fileName: String): LessonThumbnail { - return when (fileName) { - "fractions_topic.json" -> LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) - .setBackgroundColorRgb(0xf7bf73) - .build() - "ratios_topic.json" -> LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.DUCK_AND_CHICKEN) - .setBackgroundColorRgb(0xf7bf73) - .build() - else -> LessonThumbnail.newBuilder().setThumbnailGraphic(LessonThumbnailGraphic.UNRECOGNIZED) - .setBackgroundColorRgb(0xf7bf73) - .build() - } - } - - private fun createTestTopic1Thumbnail(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_CUPCAKES) - .setBackgroundColorRgb(0xf7bf73) - .build() - } - private fun createTestTopic0Story0(): StorySummary { return StorySummary.newBuilder() .setStoryId(TEST_STORY_ID_0) @@ -457,14 +445,7 @@ class TopicController @Inject constructor( .setName("Prototype Exploration") .setSummary("This is the prototype exploration to verify interaction functionality.") .setChapterPlayState(ChapterPlayState.COMPLETED) - .setChapterThumbnail(createTestTopic0Story0Chapter0Thumbnail()) - .build() - } - - private fun createTestTopic0Story0Chapter0Thumbnail(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) - .setBackgroundColorRgb(0x494276) + .setChapterThumbnail(createChapterThumbnail0()) .build() } @@ -484,7 +465,7 @@ class TopicController @Inject constructor( .setName("Second Exploration") .setSummary("This is the second exploration summary") .setChapterPlayState(ChapterPlayState.COMPLETED) - .setChapterThumbnail(createTestTopic0Story1ChapterThumbnail1()) + .setChapterThumbnail(createChapterThumbnail1()) .build() } @@ -494,7 +475,7 @@ class TopicController @Inject constructor( .setName("Third Exploration") .setSummary("This is the third exploration summary") .setChapterPlayState(ChapterPlayState.NOT_STARTED) - .setChapterThumbnail(createTestTopic0Story1ChapterThumbnail2()) + .setChapterThumbnail(createChapterThumbnail2()) .build() } @@ -504,31 +485,7 @@ class TopicController @Inject constructor( .setName("Fourth Exploration") .setSummary("This is the fourth exploration summary") .setChapterPlayState(ChapterPlayState.NOT_PLAYABLE_MISSING_PREREQUISITES) - .setChapterThumbnail(createTestTopic0Story1ChapterThumbnail3()) - .build() - } - - /** Returns the [LessonThumbnail] associated for each chapter in story 1. */ - private fun createTestTopic0Story1ChapterThumbnail1(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.DUCK_AND_CHICKEN) - .setBackgroundColorRgb(0xa5d3ec) - .build() - } - - /** Returns the [LessonThumbnail] associated for each chapter in story 1. */ - private fun createTestTopic0Story1ChapterThumbnail2(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) - .setBackgroundColorRgb(0xffeebe) - .build() - } - - /** Returns the [LessonThumbnail] associated for each chapter in story 1. */ - private fun createTestTopic0Story1ChapterThumbnail3(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.PERSON_WITH_PIE_CHART) - .setBackgroundColorRgb(0x76d1ca) + .setChapterThumbnail(createChapterThumbnail3()) .build() } @@ -545,14 +502,7 @@ class TopicController @Inject constructor( .setExplorationId(TEST_EXPLORATION_ID_4) .setName("Fifth Exploration") .setChapterPlayState(ChapterPlayState.NOT_STARTED) - .setChapterThumbnail(createTestTopic1Story2Chapter0Thumbnail()) - .build() - } - - private fun createTestTopic1Story2Chapter0Thumbnail(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.PERSON_WITH_PIE_CHART) - .setBackgroundColorRgb(0x7eb3ad) + .setChapterThumbnail(createChapterThumbnail4()) .build() } diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt index 1f58865b41c..507738794c6 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt @@ -2,10 +2,14 @@ package org.oppia.domain.topic import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData +import org.json.JSONObject +import org.oppia.app.model.ChapterPlayState import org.oppia.app.model.LessonThumbnail import org.oppia.app.model.LessonThumbnailGraphic import org.oppia.app.model.OngoingStoryList import org.oppia.app.model.PromotedStory +import org.oppia.app.model.StorySummary +import org.oppia.app.model.Topic import org.oppia.app.model.TopicList import org.oppia.app.model.TopicSummary import org.oppia.domain.util.JsonAssetRetriever @@ -18,13 +22,37 @@ const val TEST_TOPIC_ID_0 = "test_topic_id_0" const val TEST_TOPIC_ID_1 = "test_topic_id_1" const val FRACTIONS_TOPIC_ID = "GJ2rLXRKD5hw" const val RATIOS_TOPIC_ID = "omzF4oqgeTXd" +val TOPIC_IDS = listOf(FRACTIONS_TOPIC_ID, RATIOS_TOPIC_ID) +val TOPIC_THUMBNAILS = mapOf( + FRACTIONS_TOPIC_ID to createTopicThumbnail0(), + RATIOS_TOPIC_ID to createTopicThumbnail1() +) +val STORY_THUMBNAILS = mapOf( + FRACTIONS_STORY_ID_0 to createStoryThumbnail0(), + RATIOS_STORY_ID_0 to createStoryThumbnail1(), + RATIOS_STORY_ID_1 to createStoryThumbnail2() +) +val EXPLORATION_THUMBNAILS = mapOf( + FRACTIONS_EXPLORATION_ID_0 to createChapterThumbnail0(), + FRACTIONS_EXPLORATION_ID_1 to createChapterThumbnail1(), + RATIOS_EXPLORATION_ID_0 to createChapterThumbnail2(), + RATIOS_EXPLORATION_ID_1 to createChapterThumbnail3(), + RATIOS_EXPLORATION_ID_2 to createChapterThumbnail4(), + RATIOS_EXPLORATION_ID_3 to createChapterThumbnail5() +) +val TOPIC_SKILL_ASSOCIATIONS = mapOf( + FRACTIONS_TOPIC_ID to listOf(FRACTIONS_SKILL_ID_0, FRACTIONS_SKILL_ID_1, FRACTIONS_SKILL_ID_2), + RATIOS_TOPIC_ID to listOf(RATIOS_SKILL_ID_0) +) private val EVICTION_TIME_MILLIS = TimeUnit.DAYS.toMillis(1) /** Controller for retrieving the list of topics available to the learner to play. */ @Singleton class TopicListController @Inject constructor( - private val jsonAssetRetriever: JsonAssetRetriever + private val jsonAssetRetriever: JsonAssetRetriever, + private val topicController: TopicController, + private val storyProgressController: StoryProgressController ) { /** * Returns the list of [TopicSummary]s currently tracked by the app, possibly up to @@ -43,45 +71,42 @@ class TopicListController @Inject constructor( } private fun createTopicList(): TopicList { - return TopicList.newBuilder() - .setPromotedStory(createPromotedStory1()) + val topicListBuilder = TopicList.newBuilder() .addTopicSummary(createTopicSummary0()) .addTopicSummary(createTopicSummary1()) .addTopicSummary(createFractionsTopicSummary()) .addTopicSummary(createRatiosTopicSummary()) - .setOngoingStoryCount(2) - .build() + val ongoingStoryList = createOngoingStoryList() + if (ongoingStoryList.recentStoryCount > 0) { + topicListBuilder.promotedStory = ongoingStoryList.recentStoryList.first() + } + return topicListBuilder.build() } private fun createFractionsTopicSummary(): TopicSummary { val fractionsJson = jsonAssetRetriever.loadJsonFromAsset("fractions_topic.json")?.getJSONObject("topic")!! - return TopicSummary.newBuilder() - .setTopicId(FRACTIONS_TOPIC_ID) - .setName(fractionsJson.getString("name")) - .setVersion(fractionsJson.getInt("version")) - .setSubtopicCount(fractionsJson.getJSONArray("subtopics").length()) - .setCanonicalStoryCount(fractionsJson.getJSONArray("canonical_story_references").length()) - .setUncategorizedSkillCount(fractionsJson.getJSONArray("uncategorized_skill_ids").length()) - .setAdditionalStoryCount(fractionsJson.getJSONArray("additional_story_references").length()) - .setTotalSkillCount(3) - .setTotalChapterCount(2) - .setTopicThumbnail(createFractionsThumbnail()) - .build() + return createTopicSummaryFromJson(FRACTIONS_TOPIC_ID, fractionsJson) } private fun createRatiosTopicSummary(): TopicSummary { - val fractionsJson = jsonAssetRetriever.loadJsonFromAsset("ratios_topic.json")?.getJSONObject("topic")!! + val ratiosJson = jsonAssetRetriever.loadJsonFromAsset("ratios_topic.json")?.getJSONObject("topic")!! + return createTopicSummaryFromJson(RATIOS_TOPIC_ID, ratiosJson) + } + + private fun createTopicSummaryFromJson(topicId: String, jsonObject: JSONObject): TopicSummary { + val topic = topicController.retrieveTopic(topicId) + val totalChapterCount = topic.storyList.map(StorySummary::getChapterCount).reduceRight(Int::plus) return TopicSummary.newBuilder() - .setTopicId(RATIOS_TOPIC_ID) - .setName(fractionsJson.getString("name")) - .setVersion(fractionsJson.getInt("version")) - .setSubtopicCount(fractionsJson.getJSONArray("subtopics").length()) - .setCanonicalStoryCount(fractionsJson.getJSONArray("canonical_story_references").length()) - .setUncategorizedSkillCount(fractionsJson.getJSONArray("uncategorized_skill_ids").length()) - .setAdditionalStoryCount(fractionsJson.getJSONArray("additional_story_references").length()) - .setTotalSkillCount(1) - .setTotalChapterCount(4) - .setTopicThumbnail(createRatiosThumbnail()) + .setTopicId(topicId) + .setName(jsonObject.getString("name")) + .setVersion(jsonObject.getInt("version")) + .setSubtopicCount(jsonObject.getJSONArray("subtopics").length()) + .setCanonicalStoryCount(jsonObject.getJSONArray("canonical_story_references").length()) + .setUncategorizedSkillCount(jsonObject.getJSONArray("uncategorized_skill_ids").length()) + .setAdditionalStoryCount(jsonObject.getJSONArray("additional_story_references").length()) + .setTotalSkillCount(TOPIC_SKILL_ASSOCIATIONS.getValue(topicId).size) + .setTotalChapterCount(totalChapterCount) + .setTopicThumbnail(TOPIC_THUMBNAILS.getValue(topicId)) .build() } @@ -116,98 +141,127 @@ class TopicListController @Inject constructor( } private fun createOngoingStoryList(): OngoingStoryList { - return OngoingStoryList.newBuilder() - .addRecentStory(createPromotedStory1()) - .addRecentStory(createPromotedStory2()) - .addOlderStory(createPromotedStory3()) - .build() + //COMPLETED_EXPLORATIONS + // TODO(#21): Thoroughly test the construction of this list based on lesson progress. + val ongoingStoryListBuilder = OngoingStoryList.newBuilder() + for (topicId in TOPIC_IDS) { + val topic = topicController.retrieveTopic(topicId) + for (storySummary in topic.storyList) { + val storyId = storySummary.storyId + val storyProgress = storyProgressController.retrieveStoryProgress(storyId) + val completedChapterCount = storyProgress.chapterProgressList.count { progress -> + progress.playState == ChapterPlayState.COMPLETED + } + if (completedChapterCount > 0) { + // TODO(#21): Track when a lesson was completed to determine to which list its story should be added. + val nextChapterId = storyProgress.chapterProgressList.find { progress -> + progress.playState == ChapterPlayState.NOT_STARTED + }?.explorationId + val nextChapterSummary = + storySummary.chapterList.find { chapterSummary -> chapterSummary.explorationId == nextChapterId } + ongoingStoryListBuilder.addRecentStory( + createPromotedStory( + storyId, topic, completedChapterCount, storyProgress.chapterProgressCount, nextChapterSummary?.name + ) + ) + } + } + } + return ongoingStoryListBuilder.build() } - private fun createPromotedStory1(): PromotedStory { - return PromotedStory.newBuilder() - .setStoryId(TEST_STORY_ID_1) - .setStoryName("Second Story") - .setTopicId(TEST_TOPIC_ID_0) - .setTopicName("First Topic") - .setNextChapterName("The Meaning of Equal Parts") - .setCompletedChapterCount(1) - .setTotalChapterCount(3) - .setLessonThumbnail(createStoryThumbnail()) - .build() + private fun createPromotedStory( + storyId: String, topic: Topic, completedChapterCount: Int, totalChapterCount: Int, nextChapterName: String? + ): PromotedStory { + val storySummary = topic.storyList.find { summary -> summary.storyId == storyId }!! + val promotedStoryBuilder = PromotedStory.newBuilder() + .setStoryId(storyId) + .setStoryName(storySummary.storyName) + .setTopicId(topic.topicId) + .setTopicName(topic.name) + .setCompletedChapterCount(completedChapterCount) + .setTotalChapterCount(totalChapterCount) + .setLessonThumbnail(STORY_THUMBNAILS.getValue(storyId)) + if (nextChapterName != null) { + promotedStoryBuilder.nextChapterName = nextChapterName + } + return promotedStoryBuilder.build() } +} - private fun createPromotedStory2(): PromotedStory { - return PromotedStory.newBuilder() - .setStoryId(TEST_STORY_ID_0) - .setStoryName("Equal Ratios") - .setTopicId(TEST_TOPIC_ID_1) - .setTopicName("Ratios and Proportions") - .setNextChapterName("Matthew Goes to the Bakery") - .setCompletedChapterCount(1) - .setTotalChapterCount(3) - .setLessonThumbnail(createStoryThumbnail1()) - .build() - } +internal fun createTopicThumbnail0(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) + .setBackgroundColorRgb(0xf7bf73) + .build() +} - private fun createPromotedStory3(): PromotedStory { - return PromotedStory.newBuilder() - .setStoryId(TEST_STORY_ID_0) - .setStoryName("Types of Angles") - .setTopicId(TEST_TOPIC_ID_1) - .setTopicName("Geometrical Figures") - .setNextChapterName("Miguel Reads a Book") - .setCompletedChapterCount(1) - .setTotalChapterCount(3) - .setLessonThumbnail(createStoryThumbnail2()) - .build() - } +internal fun createTopicThumbnail1(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.DUCK_AND_CHICKEN) + .setBackgroundColorRgb(0xf7bf73) + .build() +} - private fun createTopicThumbnail0(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_BOOK) - .setBackgroundColorRgb(0xd5836f) - .build() - } +internal fun createStoryThumbnail0(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.DUCK_AND_CHICKEN) + .setBackgroundColorRgb(0xa5d3ec) + .build() +} - private fun createTopicThumbnail1(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_CUPCAKES) - .setBackgroundColorRgb(0xf7bf73) - .build() - } +internal fun createStoryThumbnail1(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) + .setBackgroundColorRgb(0xd3a5ec) + .build() +} - private fun createFractionsThumbnail(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) - .setBackgroundColorRgb(0xf7bf73) - .build() - } +internal fun createStoryThumbnail2(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_CUPCAKES) + .setBackgroundColorRgb(0xa5ecd3) + .build() +} - private fun createRatiosThumbnail(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.DUCK_AND_CHICKEN) - .setBackgroundColorRgb(0xf7bf73) - .build() - } +internal fun createChapterThumbnail0(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) + .setBackgroundColorRgb(0xa5d3ec) + .build() +} - private fun createStoryThumbnail(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.DUCK_AND_CHICKEN) - .setBackgroundColorRgb(0xa5d3ec) - .build() - } +internal fun createChapterThumbnail1(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.DUCK_AND_CHICKEN) + .setBackgroundColorRgb(0xf7bf73) + .build() +} - private fun createStoryThumbnail1(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) - .setBackgroundColorRgb(0xd3a5ec) - .build() - } +internal fun createChapterThumbnail2(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.PERSON_WITH_PIE_CHART) + .setBackgroundColorRgb(0xd3a5ec) + .build() +} - private fun createStoryThumbnail2(): LessonThumbnail { - return LessonThumbnail.newBuilder() - .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_CUPCAKES) - .setBackgroundColorRgb(0xa5ecd3) - .build() - } +internal fun createChapterThumbnail3(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_CUPCAKES) + .setBackgroundColorRgb(0xa5d3ec) + .build() +} + +internal fun createChapterThumbnail4(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.BAKER) + .setBackgroundColorRgb(0xa5ecd3) + .build() +} + +internal fun createChapterThumbnail5(): LessonThumbnail { + return LessonThumbnail.newBuilder() + .setThumbnailGraphic(LessonThumbnailGraphic.DUCK_AND_CHICKEN) + .setBackgroundColorRgb(0xd3a5ec) + .build() } diff --git a/domain/src/test/java/org/oppia/domain/topic/StoryProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/topic/StoryProgressControllerTest.kt index 7ad72407ad1..91b24330188 100644 --- a/domain/src/test/java/org/oppia/domain/topic/StoryProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/topic/StoryProgressControllerTest.kt @@ -73,9 +73,9 @@ class StoryProgressControllerTest { val storyProgress = storyProgressLiveData.value!!.getOrThrow() assertThat(storyProgress.chapterProgressCount).isEqualTo(2) assertThat(storyProgress.getChapterProgress(0).explorationId).isEqualTo(FRACTIONS_EXPLORATION_ID_0) - assertThat(storyProgress.getChapterProgress(0).playState).isEqualTo(NOT_STARTED) + assertThat(storyProgress.getChapterProgress(0).playState).isEqualTo(COMPLETED) assertThat(storyProgress.getChapterProgress(1).explorationId).isEqualTo(FRACTIONS_EXPLORATION_ID_1) - assertThat(storyProgress.getChapterProgress(1).playState).isEqualTo(NOT_PLAYABLE_MISSING_PREREQUISITES) + assertThat(storyProgress.getChapterProgress(1).playState).isEqualTo(NOT_STARTED) } @Test diff --git a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt index 7d8d3911855..4404a1ea41c 100644 --- a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt @@ -167,7 +167,7 @@ class TopicControllerTest { val topicLiveData = topicController.getTopic(TEST_TOPIC_ID_0) val topic = topicLiveData.value!!.getOrThrow() - assertThat(topic.topicThumbnail.thumbnailGraphic).isEqualTo(LessonThumbnailGraphic.CHILD_WITH_BOOK) + assertThat(topic.topicThumbnail.thumbnailGraphic).isEqualTo(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) } @Test @@ -192,7 +192,7 @@ class TopicControllerTest { val topicLiveData = topicController.getTopic(TEST_TOPIC_ID_1) val topic = topicLiveData.value!!.getOrThrow() - assertThat(topic.topicThumbnail.thumbnailGraphic).isEqualTo(LessonThumbnailGraphic.CHILD_WITH_CUPCAKES) + assertThat(topic.topicThumbnail.thumbnailGraphic).isEqualTo(LessonThumbnailGraphic.DUCK_AND_CHICKEN) } @Test @@ -325,7 +325,7 @@ class TopicControllerTest { val story = storyLiveData.value!!.getOrThrow() val chapter = story.getChapter(0) - assertThat(chapter.chapterThumbnail.thumbnailGraphic).isEqualTo(LessonThumbnailGraphic.PERSON_WITH_PIE_CHART) + assertThat(chapter.chapterThumbnail.thumbnailGraphic).isEqualTo(LessonThumbnailGraphic.BAKER) } @Test diff --git a/domain/src/test/java/org/oppia/domain/topic/TopicListControllerTest.kt b/domain/src/test/java/org/oppia/domain/topic/TopicListControllerTest.kt index 00e3ba9d2a9..5dd4cd37109 100644 --- a/domain/src/test/java/org/oppia/domain/topic/TopicListControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/topic/TopicListControllerTest.kt @@ -9,12 +9,18 @@ import dagger.BindsInstance import dagger.Component import dagger.Module import dagger.Provides +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestCoroutineDispatcher import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.oppia.app.model.LessonThumbnailGraphic +import org.oppia.util.threading.BackgroundDispatcher +import org.oppia.util.threading.BlockingDispatcher import org.robolectric.annotation.Config import javax.inject.Inject +import javax.inject.Qualifier import javax.inject.Singleton /** Tests for [TopicListController]. */ @@ -66,7 +72,7 @@ class TopicListControllerTest { val topicList = topicListLiveData.value!!.getOrThrow() val firstTopic = topicList.getTopicSummary(0) - assertThat(firstTopic.topicThumbnail.thumbnailGraphic).isEqualTo(LessonThumbnailGraphic.CHILD_WITH_BOOK) + assertThat(firstTopic.topicThumbnail.thumbnailGraphic).isEqualTo(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) } @Test @@ -94,7 +100,7 @@ class TopicListControllerTest { val topicList = topicListLiveData.value!!.getOrThrow() val secondTopic = topicList.getTopicSummary(1) - assertThat(secondTopic.topicThumbnail.thumbnailGraphic).isEqualTo(LessonThumbnailGraphic.CHILD_WITH_CUPCAKES) + assertThat(secondTopic.topicThumbnail.thumbnailGraphic).isEqualTo(LessonThumbnailGraphic.DUCK_AND_CHICKEN) } @Test @@ -170,8 +176,8 @@ class TopicListControllerTest { val topicList = topicListLiveData.value!!.getOrThrow() val promotedStory = topicList.promotedStory - assertThat(promotedStory.storyId).isEqualTo(TEST_STORY_ID_1) - assertThat(promotedStory.storyName).isEqualTo("Second Story") + assertThat(promotedStory.storyId).isEqualTo(FRACTIONS_STORY_ID_0) + assertThat(promotedStory.storyName).isEqualTo("Matthew") } @Test @@ -180,8 +186,8 @@ class TopicListControllerTest { val topicList = topicListLiveData.value!!.getOrThrow() val promotedStory = topicList.promotedStory - assertThat(promotedStory.topicId).isEqualTo(TEST_TOPIC_ID_0) - assertThat(promotedStory.topicName).isEqualTo("First Topic") + assertThat(promotedStory.topicId).isEqualTo(FRACTIONS_TOPIC_ID) + assertThat(promotedStory.topicName).isEqualTo("Fractions") } @Test @@ -191,15 +197,7 @@ class TopicListControllerTest { val topicList = topicListLiveData.value!!.getOrThrow() val promotedStory = topicList.promotedStory assertThat(promotedStory.completedChapterCount).isEqualTo(1) - assertThat(promotedStory.totalChapterCount).isEqualTo(3) - } - - @Test - fun testRetrieveTopicList_hasMultipleOngoingLessons() { - val topicListLiveData = topicListController.getTopicList() - - val topicList = topicListLiveData.value!!.getOrThrow() - assertThat(topicList.ongoingStoryCount).isEqualTo(2) + assertThat(promotedStory.totalChapterCount).isEqualTo(2) } @Test @@ -216,7 +214,7 @@ class TopicListControllerTest { val ongoingStoryListLiveData = topicListController.getOngoingStoryList() val ongoingStoryList = ongoingStoryListLiveData.value!!.getOrThrow() - assertThat(ongoingStoryList.recentStoryCount).isEqualTo(2) + assertThat(ongoingStoryList.recentStoryCount).isEqualTo(1) } @Test @@ -225,8 +223,8 @@ class TopicListControllerTest { val ongoingStoryList = ongoingStoryListLiveData.value!!.getOrThrow() val recentLesson = ongoingStoryList.getRecentStory(0) - assertThat(recentLesson.storyId).isEqualTo(TEST_STORY_ID_1) - assertThat(recentLesson.storyName).isEqualTo("Second Story") + assertThat(recentLesson.storyId).isEqualTo(FRACTIONS_STORY_ID_0) + assertThat(recentLesson.storyName).isEqualTo("Matthew") } @Test @@ -235,8 +233,8 @@ class TopicListControllerTest { val ongoingStoryList = ongoingStoryListLiveData.value!!.getOrThrow() val recentLesson = ongoingStoryList.getRecentStory(0) - assertThat(recentLesson.topicId).isEqualTo(TEST_TOPIC_ID_0) - assertThat(recentLesson.topicName).isEqualTo("First Topic") + assertThat(recentLesson.topicId).isEqualTo(FRACTIONS_TOPIC_ID) + assertThat(recentLesson.topicName).isEqualTo("Fractions") } @Test @@ -246,7 +244,7 @@ class TopicListControllerTest { val ongoingStoryList = ongoingStoryListLiveData.value!!.getOrThrow() val recentLesson = ongoingStoryList.getRecentStory(0) assertThat(recentLesson.completedChapterCount).isEqualTo(1) - assertThat(recentLesson.totalChapterCount).isEqualTo(3) + assertThat(recentLesson.totalChapterCount).isEqualTo(2) } @Test @@ -263,7 +261,7 @@ class TopicListControllerTest { val ongoingStoryListLiveData = topicListController.getOngoingStoryList() val ongoingStoryList = ongoingStoryListLiveData.value!!.getOrThrow() - assertThat(ongoingStoryList.olderStoryCount).isEqualTo(1) + assertThat(ongoingStoryList.olderStoryCount).isEqualTo(0) } private fun setUpTestApplicationComponent() { @@ -273,6 +271,9 @@ class TopicListControllerTest { .inject(this) } + @Qualifier + annotation class TestDispatcher + // TODO(#89): Move this to a common test application component. @Module class TestModule { @@ -281,6 +282,28 @@ class TopicListControllerTest { fun provideContext(application: Application): Context { return application } + + @ExperimentalCoroutinesApi + @Singleton + @Provides + @TestDispatcher + fun provideTestDispatcher(): CoroutineDispatcher { + return TestCoroutineDispatcher() + } + + @Singleton + @Provides + @BackgroundDispatcher + fun provideBackgroundDispatcher(@TestDispatcher testDispatcher: CoroutineDispatcher): CoroutineDispatcher { + return testDispatcher + } + + @Singleton + @Provides + @BlockingDispatcher + fun provideBlockingDispatcher(@TestDispatcher testDispatcher: CoroutineDispatcher): CoroutineDispatcher { + return testDispatcher + } } // TODO(#89): Move this to a common test application component. diff --git a/model/src/main/proto/topic.proto b/model/src/main/proto/topic.proto index abc8729a85e..8d5ea97051b 100644 --- a/model/src/main/proto/topic.proto +++ b/model/src/main/proto/topic.proto @@ -128,9 +128,6 @@ message TopicList { // All topics that are available to the player. repeated TopicSummary topic_summary = 2; - - // The total number of ongoing stories by the player. - int32 ongoing_story_count = 3; } // Corresponds to the list of stories the player is currently playing. From f8be226b8975562af7e4eb7e0bf6f1bf059beb7f Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sat, 16 Nov 2019 18:35:13 -0800 Subject: [PATCH 2/8] Use a different color for the fractions topic vs. ratios. --- .../src/main/java/org/oppia/domain/topic/TopicListController.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt index 507738794c6..ab6e117de2d 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt @@ -192,7 +192,7 @@ class TopicListController @Inject constructor( internal fun createTopicThumbnail0(): LessonThumbnail { return LessonThumbnail.newBuilder() .setThumbnailGraphic(LessonThumbnailGraphic.CHILD_WITH_FRACTIONS_HOMEWORK) - .setBackgroundColorRgb(0xf7bf73) + .setBackgroundColorRgb(0xd5836f) .build() } From e43b3f41a418ad93e1decb1de715568cd4273200 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sat, 16 Nov 2019 18:38:55 -0800 Subject: [PATCH 3/8] Remove dev comment. --- .../src/main/java/org/oppia/domain/topic/TopicListController.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt index ab6e117de2d..2b6375028a1 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt @@ -141,7 +141,6 @@ class TopicListController @Inject constructor( } private fun createOngoingStoryList(): OngoingStoryList { - //COMPLETED_EXPLORATIONS // TODO(#21): Thoroughly test the construction of this list based on lesson progress. val ongoingStoryListBuilder = OngoingStoryList.newBuilder() for (topicId in TOPIC_IDS) { From 1f2f7308e795a3d08109656d7309ae4629345e1a Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sat, 16 Nov 2019 19:18:22 -0800 Subject: [PATCH 4/8] Compute actual JSON disk size requirement for each topic. --- .../topic/overview/TopicOverviewViewModel.kt | 11 +++++++- .../res/layout/topic_overview_fragment.xml | 2 +- .../org/oppia/domain/topic/TopicController.kt | 28 +++++++++++++++++++ .../oppia/domain/util/JsonAssetRetriever.kt | 8 ++++++ model/src/main/proto/topic.proto | 3 ++ 5 files changed, 50 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewViewModel.kt b/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewViewModel.kt index 2b4f64c85c8..5d50f820073 100644 --- a/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewViewModel.kt +++ b/app/src/main/java/org/oppia/app/topic/overview/TopicOverviewViewModel.kt @@ -6,12 +6,21 @@ import org.oppia.app.R import org.oppia.app.fragment.FragmentScope import org.oppia.app.model.Topic import org.oppia.app.viewmodel.ObservableViewModel +import java.text.DecimalFormat import javax.inject.Inject /** [ViewModel] for showing topic overview details. */ @FragmentScope class TopicOverviewViewModel @Inject constructor() : ObservableViewModel() { + private val decimalFormat: DecimalFormat = DecimalFormat("#.###") + val topic = ObservableField(Topic.getDefaultInstance()) - var downloadStatusIndicatorDrawableResourceId = ObservableField(R.drawable.ic_available_offline_primary_24dp) + var downloadStatusIndicatorDrawableResourceId = ObservableField(R.drawable.ic_available_offline_primary_24dp) + + /** Returns the number of megabytes of disk space this topic requires, formatted for display. */ + fun getTopicSizeMb(): String { + val topicSizeMb: Double = (topic.get()?.diskSizeBytes ?: 0) / (1024.0 * 1024.0) + return decimalFormat.format(topicSizeMb) + } } diff --git a/app/src/main/res/layout/topic_overview_fragment.xml b/app/src/main/res/layout/topic_overview_fragment.xml index bf7a59b8ebb..7d2abed53c6 100644 --- a/app/src/main/res/layout/topic_overview_fragment.xml +++ b/app/src/main/res/layout/topic_overview_fragment.xml @@ -142,7 +142,7 @@ android:layout_marginTop="12dp" android:fontFamily="sans-serif" android:gravity="top" - android:text="@{String.format(@string/topic_download_text, viewModel.topic.getSerializedSize())}" + android:text="@{String.format(@string/topic_download_text, viewModel.getTopicSizeMb())}" android:textColor="@color/oppiaPrimaryText" android:textSize="18sp" android:textStyle="italic" diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt index 733625a94e9..a9d07f5652d 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt @@ -22,6 +22,7 @@ import org.oppia.domain.util.StateRetriever import org.oppia.util.data.AsyncResult import org.oppia.util.data.DataProvider import org.oppia.util.data.DataProviders +import java.io.File import javax.inject.Inject import javax.inject.Singleton @@ -52,6 +53,26 @@ const val FRACTIONS_QUESTION_ID_8 = "AciwQAtcvZfI" const val FRACTIONS_QUESTION_ID_9 = "YQwbX2r6p3Xj" const val FRACTIONS_QUESTION_ID_10 = "NNuVGmbJpnj5" const val RATIOS_QUESTION_ID_0 = "QiKxvAXpvUbb" +private val TOPIC_FILE_ASSOCIATIONS = mapOf( + FRACTIONS_TOPIC_ID to listOf( + "fractions_exploration0.json", + "fractions_exploration1.json", + "fractions_questions.json", + "fractions_skills.json", + "fractions_stories.json", + "fractions_topic.json" + ), + RATIOS_TOPIC_ID to listOf( + "ratios_exploration0.json", + "ratios_exploration1.json", + "ratios_exploration2.json", + "ratios_exploration3.json", + "ratios_questions.json", + "ratios_skills.json", + "ratios_stories.json", + "ratios_topic.json" + ) +) private const val QUESTION_DATA_PROVIDER_ID = "QuestionDataProvider" @@ -356,9 +377,16 @@ class TopicController @Inject constructor( .addAllSkill(createSkillsFromJson(skillFileName)) .addAllStory(createStoriesFromJson(storyFileName)) .setTopicThumbnail(TOPIC_THUMBNAILS.getValue(topicId)) + .setDiskSizeBytes(computeTopicSizeBytes(TOPIC_FILE_ASSOCIATIONS.getValue(topicId))) .build() } + private fun computeTopicSizeBytes(constituentFiles: List): Long { + // TODO(#169): Compute this based on protos & the combined topic package. + // TODO(#386): Incorporate audio & image files in this computation. + return constituentFiles.map(jsonAssetRetriever::getAssetSize).map(Int::toLong).reduceRight(Long::plus) + } + /** Utility to create the skill list of a topic from its json representation. The json file is expected to have * a key called 'skill_list' that contains an array of skill objects, each with the key 'skill'. */ private fun createSkillsFromJson(fileName: String): List { diff --git a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt index 6db29f1b04f..acd869ded28 100644 --- a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt @@ -15,6 +15,14 @@ class JsonAssetRetriever @Inject constructor(private val context: Context) { return JSONObject(jsonContents) } + /** Returns the on-disk size of the specified asset, in bytes. */ + fun getAssetSize(assetName: String): Int { + // Unfortunately, the entire file needs to be read to retrieve the asset size since JSON files are compressed in the + // apk. See: https://stackoverflow.com/a/6187097. + // TODO(#386): Use an asset retriever to prefetch and cache these to avoid needing to keep re-reading them. + return context.assets.open(assetName).use { it.readBytes() }.size + } + fun getStringsFromJSONArray(jsonData: JSONArray): List { val stringList = mutableListOf() for (i in 0 until jsonData.length()) { diff --git a/model/src/main/proto/topic.proto b/model/src/main/proto/topic.proto index 8d5ea97051b..7ea9b06faa7 100644 --- a/model/src/main/proto/topic.proto +++ b/model/src/main/proto/topic.proto @@ -29,6 +29,9 @@ message Topic { // The thumbnail corresponding to this topic. LessonThumbnail topic_thumbnail = 6; + + // The number of on-disk bytes this topic consumes. + int64 disk_size_bytes = 7; } // Corresponds to a concept card that can be displayed for a specific skill. From a938c74091406d04af55fb23495a04f858e8021a Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sun, 17 Nov 2019 14:36:17 -0800 Subject: [PATCH 5/8] Introduce AssetRepository to prefetch JSON files, and support offline caching for audio files. --- domain/src/main/assets/about_oppia.json | 1 + domain/src/main/assets/oppia_exploration.json | 1 + .../main/assets/prototype_exploration.json | 1 + domain/src/main/assets/welcome.json | 1 + .../domain/audio/AudioPlayerController.kt | 36 +++++- .../exploration/ExplorationRetriever.kt | 13 +- .../org/oppia/domain/topic/TopicController.kt | 2 +- .../oppia/domain/topic/TopicListController.kt | 109 +++++++++++++++- .../org/oppia/domain/util/AssetRepository.kt | 120 ++++++++++++++++++ .../oppia/domain/util/JsonAssetRetriever.kt | 10 +- 10 files changed, 278 insertions(+), 16 deletions(-) create mode 100644 domain/src/main/java/org/oppia/domain/util/AssetRepository.kt diff --git a/domain/src/main/assets/about_oppia.json b/domain/src/main/assets/about_oppia.json index 1202f9a506e..b195f7a461c 100644 --- a/domain/src/main/assets/about_oppia.json +++ b/domain/src/main/assets/about_oppia.json @@ -1,4 +1,5 @@ { + "exploration_id": "1", "author_notes": "", "blurb": "", "category": "Welcome", diff --git a/domain/src/main/assets/oppia_exploration.json b/domain/src/main/assets/oppia_exploration.json index ebf01b482d4..d569575d418 100755 --- a/domain/src/main/assets/oppia_exploration.json +++ b/domain/src/main/assets/oppia_exploration.json @@ -1,4 +1,5 @@ { + "exploration_id": "3", "author_notes": "", "blurb": "", "category": "Welcome", diff --git a/domain/src/main/assets/prototype_exploration.json b/domain/src/main/assets/prototype_exploration.json index aa077230c3d..42a39890f4d 100644 --- a/domain/src/main/assets/prototype_exploration.json +++ b/domain/src/main/assets/prototype_exploration.json @@ -1,4 +1,5 @@ { + "exploration_id": "2", "language_code": "en", "param_specs": {}, "param_changes": [], diff --git a/domain/src/main/assets/welcome.json b/domain/src/main/assets/welcome.json index 7030ee04c9e..992af67d990 100644 --- a/domain/src/main/assets/welcome.json +++ b/domain/src/main/assets/welcome.json @@ -1,4 +1,5 @@ { + "exploration_id": "0", "author_notes": "", "blurb": "", "category": "Welcome", diff --git a/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt b/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt index bb396f5aaa5..0877cac4732 100644 --- a/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt +++ b/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt @@ -1,6 +1,8 @@ package org.oppia.domain.audio +import android.media.MediaDataSource import android.media.MediaPlayer +import android.os.Build import androidx.annotation.VisibleForTesting import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData @@ -9,6 +11,7 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.launch +import org.oppia.domain.util.AssetRepository import org.oppia.util.data.AsyncResult import org.oppia.util.logging.Logger import org.oppia.util.threading.BackgroundDispatcher @@ -28,6 +31,7 @@ import kotlin.concurrent.withLock @Singleton class AudioPlayerController @Inject constructor( private val logger: Logger, + private val assetRepository: AssetRepository, @BackgroundDispatcher private val backgroundDispatcher: CoroutineDispatcher ) { @@ -125,7 +129,37 @@ class AudioPlayerController @Inject constructor( private fun prepareDataSource(url: String) { try { - mediaPlayer.setDataSource(url) + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { + val mediaDataSource: MediaDataSource = object : MediaDataSource() { + private val audioFileBuffer: ByteArray by lazy { + // Ensure that the download occurs off the main thread to avoid strict mode violations for + // cases when we need to stream audio. + assetRepository.loadRemoteBinaryAsset(url)() + } + + // https://medium.com/@jacks205/implementing-your-own-android-mediadatasource-e67adb070731. + override fun readAt(position: Long, buffer: ByteArray?, offset: Int, size: Int): Int { + checkNotNull(buffer) + val intPosition = position.toInt() + if (intPosition >= audioFileBuffer.size) { + return -1 + } + val availableData = audioFileBuffer.size - intPosition + val adjustedSize = size.coerceIn(0 until availableData) + audioFileBuffer.copyInto(buffer, offset, intPosition, intPosition + adjustedSize) + return adjustedSize + } + + override fun getSize(): Long { + return audioFileBuffer.size.toLong() + } + + override fun close() {} + } + mediaPlayer.setDataSource(mediaDataSource) + } else { + mediaPlayer.setDataSource(url) + } mediaPlayer.prepareAsync() } catch (e: IOException) { logger.e("AudioPlayerController", "Failed to set data source for media player", e) diff --git a/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt b/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt index 0e501d6ca5c..c9427baa825 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt @@ -14,10 +14,10 @@ import org.oppia.domain.util.StateRetriever import java.io.IOException import javax.inject.Inject -const val TEST_EXPLORATION_ID_5 = "DIWZiVgs0km-" -const val TEST_EXPLORATION_ID_6 = "test_exp_id_6" -const val TEST_EXPLORATION_ID_30 = "30" -const val TEST_EXPLORATION_ID_7 = "test_exp_id_7" +const val TEST_EXPLORATION_ID_5 = "0" +const val TEST_EXPLORATION_ID_6 = "1" +const val TEST_EXPLORATION_ID_30 = "2" +const val TEST_EXPLORATION_ID_7 = "3" // TODO(#59): Make this class inaccessible outside of the domain package except for tests. UI code should not be allowed // to depend on this utility. @@ -27,9 +27,9 @@ class ExplorationRetriever @Inject constructor( private val jsonAssetRetriever: JsonAssetRetriever, private val stateRetriever: StateRetriever ) { + // TODO(#169): Force callers of this method on a background thread. /** Loads and returns an exploration for the specified exploration ID, or fails. */ - @Suppress("RedundantSuspendModifier") // Force callers to call this on a background thread. - internal suspend fun loadExploration(explorationId: String): Exploration { + internal fun loadExploration(explorationId: String): Exploration { return when (explorationId) { TEST_EXPLORATION_ID_5 -> loadExplorationFromAsset("welcome.json") TEST_EXPLORATION_ID_6 -> loadExplorationFromAsset("about_oppia.json") @@ -50,6 +50,7 @@ class ExplorationRetriever @Inject constructor( try { val explorationObject = jsonAssetRetriever.loadJsonFromAsset(assetName) ?: return Exploration.getDefaultInstance() return Exploration.newBuilder() + .setId(explorationObject.getString("exploration_id")) .setTitle(explorationObject.getString("title")) .setLanguageCode(explorationObject.getString("language_code")) .setInitStateName(explorationObject.getString("init_state_name")) diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt index a9d07f5652d..53840bff940 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt @@ -53,7 +53,7 @@ const val FRACTIONS_QUESTION_ID_8 = "AciwQAtcvZfI" const val FRACTIONS_QUESTION_ID_9 = "YQwbX2r6p3Xj" const val FRACTIONS_QUESTION_ID_10 = "NNuVGmbJpnj5" const val RATIOS_QUESTION_ID_0 = "QiKxvAXpvUbb" -private val TOPIC_FILE_ASSOCIATIONS = mapOf( +val TOPIC_FILE_ASSOCIATIONS = mapOf( FRACTIONS_TOPIC_ID to listOf( "fractions_exploration0.json", "fractions_exploration1.json", diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt index 2b6375028a1..7b0a158504a 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt @@ -1,19 +1,38 @@ package org.oppia.domain.topic +import android.os.SystemClock import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.async +import kotlinx.coroutines.launch import org.json.JSONObject +import org.oppia.app.model.AnswerGroup import org.oppia.app.model.ChapterPlayState +import org.oppia.app.model.ChapterSummary +import org.oppia.app.model.Exploration import org.oppia.app.model.LessonThumbnail import org.oppia.app.model.LessonThumbnailGraphic import org.oppia.app.model.OngoingStoryList +import org.oppia.app.model.Outcome import org.oppia.app.model.PromotedStory +import org.oppia.app.model.State import org.oppia.app.model.StorySummary +import org.oppia.app.model.SubtitledHtml import org.oppia.app.model.Topic import org.oppia.app.model.TopicList import org.oppia.app.model.TopicSummary +import org.oppia.app.model.Voiceover +import org.oppia.app.model.VoiceoverMapping +import org.oppia.domain.exploration.ExplorationRetriever +import org.oppia.domain.util.AssetRepository import org.oppia.domain.util.JsonAssetRetriever import org.oppia.util.data.AsyncResult +import org.oppia.util.gcsresource.DefaultResource +import org.oppia.util.logging.Logger +import org.oppia.util.threading.BackgroundDispatcher import java.util.concurrent.TimeUnit import javax.inject.Inject import javax.inject.Singleton @@ -52,8 +71,50 @@ private val EVICTION_TIME_MILLIS = TimeUnit.DAYS.toMillis(1) class TopicListController @Inject constructor( private val jsonAssetRetriever: JsonAssetRetriever, private val topicController: TopicController, - private val storyProgressController: StoryProgressController + private val storyProgressController: StoryProgressController, + private val explorationRetriever: ExplorationRetriever, + @BackgroundDispatcher private val backgroundDispatcher: CoroutineDispatcher, + @DefaultResource private val gcsResource: String, + logger: Logger, + assetRepository: AssetRepository ) { + private val backgroundScope = CoroutineScope(backgroundDispatcher) + private val loadAssetJob: Job + + init { + // TODO(#169): Download data reactively rather than during start-up to avoid blocking the main thread on the whole + // load operation. + + // Ensure all JSON files are available in memory for quick retrieval. + val allFiles = TOPIC_FILE_ASSOCIATIONS.values.flatten() + val primeAssetJobs = allFiles.map { + backgroundScope.async { + assetRepository.primeTextFileFromLocalAssets(it) + } + } + + // The following job encapsulates all startup loading. NB: We don't currently wait on this job to complete because + // it's fine to try to load the assets at the same time as priming the cache, and it's unlikely the user can get + // into an exploration fast enough to try to load an asset that would trigger a strict mode crash. + loadAssetJob = backgroundScope.launch { + primeAssetJobs.forEach { it.await() } + + // Only download the audio assets for one fractions lesson. The others can still be streamed. + val explorationIds = listOf(FRACTIONS_EXPLORATION_ID_1) + val voiceoverUrls = collectAllDesiredVoiceoverUrls(loadExplorations(explorationIds)) + logger.d("AssetRepo", "Downloading up to ${voiceoverUrls.size} voiceovers") + val startTime = SystemClock.elapsedRealtime() + val voiceoverDownloadJobs = voiceoverUrls.map { url -> + backgroundScope.async { + assetRepository.primeRemoteBinaryAsset(url) + } + } + voiceoverDownloadJobs.forEach { it.await() } + val endTime = SystemClock.elapsedRealtime() + logger.d("AssetRepo", "Finished downloading voiceovers in ${endTime - startTime}ms") + } + } + /** * Returns the list of [TopicSummary]s currently tracked by the app, possibly up to * [EVICTION_TIME_MILLIS] old. @@ -186,6 +247,52 @@ class TopicListController @Inject constructor( } return promotedStoryBuilder.build() } + + private fun loadExplorations(explorationIds: Collection): Collection { + return explorationIds.map(explorationRetriever::loadExploration) + } + + private fun collectionExplorationIdsForTopic(topic: Topic): Collection { + return topic.storyList.flatMap(::collectExplorationIdsForStory) + } + + private fun collectExplorationIdsForStory(story: StorySummary): Collection { + return story.chapterList.map(ChapterSummary::getExplorationId) + } + + private fun collectAllDesiredVoiceoverUrls(explorations: Collection): Collection { + return explorations.flatMap(::collectDesiredVoiceoverUrls) + } + + private fun collectDesiredVoiceoverUrls(exploration: Exploration): Collection { + return extractDesiredVoiceovers(exploration).map { voiceover -> getUriForVoiceover(exploration.id, voiceover) } + } + + private fun extractDesiredVoiceovers(exploration: Exploration): Collection { + val states = exploration.statesMap.values + val mappings = states.flatMap(::getDesiredVoiceoverMapping) + return mappings.flatMap { it.voiceoverMappingMap.values } + } + + private fun getDesiredVoiceoverMapping(state: State): Collection { + val voiceoverMappings = state.recordedVoiceoversMap + val contentIds = extractDesiredContentIds(state).filter(String::isNotEmpty) + return voiceoverMappings.filterKeys(contentIds::contains).values + } + + /** Returns all collection IDs from the specified [State] that can actually be played by a user. */ + private fun extractDesiredContentIds(state: State): Collection { + val stateContentSubtitledHtml = state.content + val defaultFeedbackSubtitledHtml = state.interaction.defaultOutcome.feedback + val answerGroupOutcomes = state.interaction.answerGroupsList.map(AnswerGroup::getOutcome) + val answerGroupsSubtitledHtml = answerGroupOutcomes.map(Outcome::getFeedback) + val targetedSubtitledHtmls = answerGroupsSubtitledHtml + stateContentSubtitledHtml + defaultFeedbackSubtitledHtml + return targetedSubtitledHtmls.map(SubtitledHtml::getContentId) + } + + private fun getUriForVoiceover(explorationId: String, voiceover: Voiceover): String { + return "https://storage.googleapis.com/$gcsResource/exploration/$explorationId/assets/audio/${voiceover.fileName}" + } } internal fun createTopicThumbnail0(): LessonThumbnail { diff --git a/domain/src/main/java/org/oppia/domain/util/AssetRepository.kt b/domain/src/main/java/org/oppia/domain/util/AssetRepository.kt new file mode 100644 index 00000000000..028b05ce9fc --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/util/AssetRepository.kt @@ -0,0 +1,120 @@ +package org.oppia.domain.util + +import android.content.Context +import org.oppia.util.logging.Logger +import java.io.File +import java.io.InputStream +import java.io.OutputStream +import java.net.URL +import java.util.concurrent.locks.ReentrantLock +import javax.inject.Inject +import javax.inject.Singleton +import kotlin.concurrent.withLock + +// TODO(#169): Leverage this repository or a version of it for caching all topic contents in a proto. It may also be +// worth keeping a version of this repository for caching audio files within certain size limits for buffering during +// an exploration. +/** + * A generic repository for access local APK asset files, and downloading remote binary files. This repository aims to + * centralize caching management of external asset files to simplify downstream code, and allow assets to be retrieved + * quickly and synchronously. + */ +@Singleton +class AssetRepository @Inject constructor(private val context: Context, private val logger: Logger) { + private val repositoryLock = ReentrantLock() + + /** Map of asset names to file contents for text file assets. */ + private val textFileAssets = mutableMapOf() + + /** Returns the whole text contents of the file corresponding to the specified asset name. */ + fun loadTextFileFromLocalAssets(assetName: String): String { + repositoryLock.withLock { + primeTextFileFromLocalAssets(assetName) + return textFileAssets.getValue(assetName) + } + } + + /** Ensures the contents corresponding to the specified asset are available for quick retrieval. */ + fun primeTextFileFromLocalAssets(assetName: String) { + repositoryLock.withLock { + if (assetName !in textFileAssets) { + logger.d("AssetRepo", "Caching local text asset: $assetName") + textFileAssets[assetName] = context.assets.open(assetName).bufferedReader().use { it.readText() } + } + } + } + + /** + * Returns a function to retrieve the stream of the binary asset corresponding to the specified URL, to be called on a + * background thread. + */ + fun loadRemoteBinaryAsset(url: String): () -> ByteArray { + return { + val stream = openLocalCacheFileForRead(url) ?: openCachingStreamToRemoteFile(url) + stream.use { it.readBytes() } + } + } + + /** Ensures the contents corresponding to the specified URL are available for quick retrieval. */ + fun primeRemoteBinaryAsset(url: String) { + if (!getLocalCacheFile(url).exists()) { + // Otherwise, download it remotely and cache it locally. + logger.d("AssetRepo", "Downloading binary asset: $url") + val contents = openRemoteStream(url).use { it.readBytes() } + saveLocalCacheFile(url, contents) + } + } + + private fun openRemoteStream(url: String): InputStream { + return URL(url).openStream() + } + + /** Returns an [InputStream] that also saves its results to a local file. */ + private fun openCachingStreamToRemoteFile(url: String): InputStream { + val urlInStream = openRemoteStream(url) + val fileOutStream = openLocalCacheFileForWrite(url) + return object: InputStream() { + override fun read(): Int { + val byte = urlInStream.read() + if (byte != -1) { + fileOutStream.write(byte) + } + return byte + } + + override fun read(b: ByteArray?, off: Int, len: Int): Int { + val count = urlInStream.read(b, off, len) + fileOutStream.write(b, off, count) + return count + } + + override fun close() { + super.close() + fileOutStream.flush() + fileOutStream.close() + urlInStream.close() + } + } + } + + private fun openLocalCacheFileForRead(identifier: String): InputStream? { + val cacheFile = getLocalCacheFile(identifier) + return if (cacheFile.exists()) cacheFile.inputStream() else null + } + + private fun saveLocalCacheFile(identifier: String, contents: ByteArray) { + getLocalCacheFile(identifier).writeBytes(contents) + } + + private fun openLocalCacheFileForWrite(identifier: String): OutputStream { + return getLocalCacheFile(identifier).outputStream() + } + + private fun getLocalCacheFile(identifier: String): File { + return File(context.cacheDir, convertIdentifierToCacheFileName(identifier)) + } + + private fun convertIdentifierToCacheFileName(identifier: String): String { + return "${identifier.hashCode()}.cache" + } +} diff --git a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt index acd869ded28..2c29488d586 100644 --- a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt @@ -1,26 +1,22 @@ package org.oppia.domain.util -import android.content.Context import org.json.JSONArray import org.json.JSONObject import javax.inject.Inject /** Utility that retrieves JSON assets and converts them to JSON objects. */ -class JsonAssetRetriever @Inject constructor(private val context: Context) { +class JsonAssetRetriever @Inject constructor(private val assetRepository: AssetRepository) { /** Loads the JSON string from an asset and converts it to a JSONObject */ fun loadJsonFromAsset(assetName: String): JSONObject? { - val assetManager = context.assets - val jsonContents = assetManager.open(assetName).bufferedReader().use { it.readText() } - return JSONObject(jsonContents) + return JSONObject(assetRepository.loadTextFileFromLocalAssets(assetName)) } /** Returns the on-disk size of the specified asset, in bytes. */ fun getAssetSize(assetName: String): Int { // Unfortunately, the entire file needs to be read to retrieve the asset size since JSON files are compressed in the // apk. See: https://stackoverflow.com/a/6187097. - // TODO(#386): Use an asset retriever to prefetch and cache these to avoid needing to keep re-reading them. - return context.assets.open(assetName).use { it.readBytes() }.size + return assetRepository.loadTextFileFromLocalAssets(assetName).toByteArray(Charsets.UTF_8).size } fun getStringsFromJSONArray(jsonData: JSONArray): List { From dc50525d374dd69cae050226f8b130d07f70efc4 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sun, 17 Nov 2019 16:47:49 -0800 Subject: [PATCH 6/8] Introduce custom local image caching. --- .../domain/audio/AudioPlayerController.kt | 2 +- .../oppia/domain/topic/TopicListController.kt | 100 ++++++++++++++---- .../oppia/domain/util/JsonAssetRetriever.kt | 1 + utility/build.gradle | 1 + .../oppia/util/caching}/AssetRepository.kt | 14 ++- .../org/oppia/util/parser/GlideImageLoader.kt | 11 +- .../oppia/util/parser/ImageAssetFetcher.kt | 10 ++ .../util/parser/RepositoryGlideModule.kt | 17 +++ .../util/parser/RepositoryModelLoader.kt | 51 +++++++++ .../org/oppia/util/parser/UrlImageParser.kt | 6 +- 10 files changed, 187 insertions(+), 26 deletions(-) rename {domain/src/main/java/org/oppia/domain/util => utility/src/main/java/org/oppia/util/caching}/AssetRepository.kt (93%) create mode 100644 utility/src/main/java/org/oppia/util/parser/ImageAssetFetcher.kt create mode 100644 utility/src/main/java/org/oppia/util/parser/RepositoryGlideModule.kt create mode 100644 utility/src/main/java/org/oppia/util/parser/RepositoryModelLoader.kt diff --git a/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt b/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt index 0877cac4732..62a60b77b3c 100644 --- a/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt +++ b/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt @@ -11,7 +11,7 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.launch -import org.oppia.domain.util.AssetRepository +import org.oppia.util.caching.AssetRepository import org.oppia.util.data.AsyncResult import org.oppia.util.logging.Logger import org.oppia.util.threading.BackgroundDispatcher diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt index 7b0a158504a..0744523d57b 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt @@ -1,6 +1,9 @@ package org.oppia.domain.topic import android.os.SystemClock +import android.text.Html +import android.text.Spannable +import android.text.style.ImageSpan import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import kotlinx.coroutines.CoroutineDispatcher @@ -11,13 +14,15 @@ import kotlinx.coroutines.launch import org.json.JSONObject import org.oppia.app.model.AnswerGroup import org.oppia.app.model.ChapterPlayState -import org.oppia.app.model.ChapterSummary import org.oppia.app.model.Exploration +import org.oppia.app.model.Hint +import org.oppia.app.model.Interaction import org.oppia.app.model.LessonThumbnail import org.oppia.app.model.LessonThumbnailGraphic import org.oppia.app.model.OngoingStoryList import org.oppia.app.model.Outcome import org.oppia.app.model.PromotedStory +import org.oppia.app.model.Solution import org.oppia.app.model.State import org.oppia.app.model.StorySummary import org.oppia.app.model.SubtitledHtml @@ -27,11 +32,14 @@ import org.oppia.app.model.TopicSummary import org.oppia.app.model.Voiceover import org.oppia.app.model.VoiceoverMapping import org.oppia.domain.exploration.ExplorationRetriever -import org.oppia.domain.util.AssetRepository +import org.oppia.util.caching.AssetRepository import org.oppia.domain.util.JsonAssetRetriever import org.oppia.util.data.AsyncResult import org.oppia.util.gcsresource.DefaultResource import org.oppia.util.logging.Logger +import org.oppia.util.parser.DefaultGcsPrefix +import org.oppia.util.parser.DefaultGcsResource +import org.oppia.util.parser.ImageDownloadUrlTemplate import org.oppia.util.threading.BackgroundDispatcher import java.util.concurrent.TimeUnit import javax.inject.Inject @@ -64,6 +72,11 @@ val TOPIC_SKILL_ASSOCIATIONS = mapOf( RATIOS_TOPIC_ID to listOf(RATIOS_SKILL_ID_0) ) +private const val CUSTOM_IMG_TAG = "oppia-noninteractive-image" +private const val REPLACE_IMG_TAG = "img" +private const val CUSTOM_IMG_FILE_PATH_ATTRIBUTE = "filepath-with-value" +private const val REPLACE_IMG_FILE_PATH_ATTRIBUTE = "src" + private val EVICTION_TIME_MILLIS = TimeUnit.DAYS.toMillis(1) /** Controller for retrieving the list of topics available to the learner to play. */ @@ -74,7 +87,9 @@ class TopicListController @Inject constructor( private val storyProgressController: StoryProgressController, private val explorationRetriever: ExplorationRetriever, @BackgroundDispatcher private val backgroundDispatcher: CoroutineDispatcher, - @DefaultResource private val gcsResource: String, + @DefaultGcsPrefix private val gcsPrefix: String, + @DefaultGcsResource private val gcsResource: String, + @ImageDownloadUrlTemplate private val imageDownloadUrlTemplate: String, logger: Logger, assetRepository: AssetRepository ) { @@ -99,19 +114,25 @@ class TopicListController @Inject constructor( loadAssetJob = backgroundScope.launch { primeAssetJobs.forEach { it.await() } - // Only download the audio assets for one fractions lesson. The others can still be streamed. - val explorationIds = listOf(FRACTIONS_EXPLORATION_ID_1) - val voiceoverUrls = collectAllDesiredVoiceoverUrls(loadExplorations(explorationIds)) - logger.d("AssetRepo", "Downloading up to ${voiceoverUrls.size} voiceovers") + // Only download binary assets for one fractions lesson. The others can still be streamed. + val explorations = loadExplorations(listOf(FRACTIONS_EXPLORATION_ID_1)) + val voiceoverUrls = collectAllDesiredVoiceoverUrls(explorations).toSet() + val imageUrls = collectAllImageUrls(explorations).toSet() + logger.d("AssetRepo", "Downloading up to ${voiceoverUrls.size} voiceovers and ${imageUrls.size} images") val startTime = SystemClock.elapsedRealtime() val voiceoverDownloadJobs = voiceoverUrls.map { url -> backgroundScope.async { assetRepository.primeRemoteBinaryAsset(url) } } - voiceoverDownloadJobs.forEach { it.await() } + val imageDownloadJobs = imageUrls.map { url -> + backgroundScope.async { + assetRepository.primeRemoteBinaryAsset(url) + } + } + (voiceoverDownloadJobs + imageDownloadJobs).forEach { it.await() } val endTime = SystemClock.elapsedRealtime() - logger.d("AssetRepo", "Finished downloading voiceovers in ${endTime - startTime}ms") + logger.d("AssetRepo", "Finished downloading voiceovers and images in ${endTime - startTime}ms") } } @@ -252,14 +273,6 @@ class TopicListController @Inject constructor( return explorationIds.map(explorationRetriever::loadExploration) } - private fun collectionExplorationIdsForTopic(topic: Topic): Collection { - return topic.storyList.flatMap(::collectExplorationIdsForStory) - } - - private fun collectExplorationIdsForStory(story: StorySummary): Collection { - return story.chapterList.map(ChapterSummary::getExplorationId) - } - private fun collectAllDesiredVoiceoverUrls(explorations: Collection): Collection { return explorations.flatMap(::collectDesiredVoiceoverUrls) } @@ -290,8 +303,59 @@ class TopicListController @Inject constructor( return targetedSubtitledHtmls.map(SubtitledHtml::getContentId) } + private fun collectAllImageUrls(explorations: Collection): Collection { + return explorations.flatMap(::collectImageUrls) + } + + private fun collectImageUrls(exploration: Exploration): Collection { + val subtitledHtmls = collectSubtitledHtmls(exploration) + val imageSources = subtitledHtmls.flatMap(::getImageSourcesFromHtml) + return imageSources.toSet().map { imageSource -> + getUriForImage(exploration.id, imageSource) + } + } + + private fun collectSubtitledHtmls(exploration: Exploration): Collection { + val states = exploration.statesMap.values + val stateContents = states.map(State::getContent) + val stateInteractions = states.map(State::getInteraction) + val stateSolutions = stateInteractions.map(Interaction::getSolution).map(Solution::getExplanation) + val stateHints = stateInteractions.map(Interaction::getHint).map(Hint::getHintContent) + val answerGroupOutcomes = stateInteractions.flatMap(Interaction::getAnswerGroupsList).map(AnswerGroup::getOutcome) + val defaultOutcomes = stateInteractions.map(Interaction::getDefaultOutcome) + val outcomeFeedbacks = (answerGroupOutcomes + defaultOutcomes).map(Outcome::getFeedback) + val allSubtitledHtmls = stateContents + stateSolutions + stateHints + outcomeFeedbacks + return allSubtitledHtmls.filter { it != SubtitledHtml.getDefaultInstance() } + } + + private fun getImageSourcesFromHtml(subtitledHtml: SubtitledHtml): Collection { + val parsedHtml = parseHtml(replaceCustomOppiaImageTag(subtitledHtml.html)) + val imageSpans = parsedHtml.getSpans(0, parsedHtml.length, ImageSpan::class.java) + return imageSpans.toList().map(ImageSpan::getSource) + } + + private fun parseHtml(html: String): Spannable { + return if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.N) { + Html.fromHtml(html, Html.FROM_HTML_MODE_LEGACY) as Spannable + } else { + Html.fromHtml(html) as Spannable + } + } + + private fun replaceCustomOppiaImageTag(html: String): String { + return html.replace(CUSTOM_IMG_TAG, REPLACE_IMG_TAG) + .replace(CUSTOM_IMG_FILE_PATH_ATTRIBUTE, REPLACE_IMG_FILE_PATH_ATTRIBUTE) + .replace("&quot;", "") + } + private fun getUriForVoiceover(explorationId: String, voiceover: Voiceover): String { - return "https://storage.googleapis.com/$gcsResource/exploration/$explorationId/assets/audio/${voiceover.fileName}" + return "https://storage.googleapis.com/${gcsResource}exploration/$explorationId/assets/audio/${voiceover.fileName}" + } + + private fun getUriForImage(explorationId: String, imageFileName: String): String { + return gcsPrefix + gcsResource + String.format( + imageDownloadUrlTemplate, "exploration", explorationId, imageFileName + ) } } diff --git a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt index 2c29488d586..1bd48d609ce 100644 --- a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt @@ -2,6 +2,7 @@ package org.oppia.domain.util import org.json.JSONArray import org.json.JSONObject +import org.oppia.util.caching.AssetRepository import javax.inject.Inject /** Utility that retrieves JSON assets and converts them to JSON objects. */ diff --git a/utility/build.gradle b/utility/build.gradle index d660073493e..e8e8d65f4bd 100644 --- a/utility/build.gradle +++ b/utility/build.gradle @@ -58,6 +58,7 @@ dependencies { 'org.robolectric:robolectric:4.3', ) kapt( + 'com.github.bumptech.glide:compiler:4.9.0', 'com.google.dagger:dagger-compiler:2.24' ) kaptTest( diff --git a/domain/src/main/java/org/oppia/domain/util/AssetRepository.kt b/utility/src/main/java/org/oppia/util/caching/AssetRepository.kt similarity index 93% rename from domain/src/main/java/org/oppia/domain/util/AssetRepository.kt rename to utility/src/main/java/org/oppia/util/caching/AssetRepository.kt index 028b05ce9fc..846f3f7a9dc 100644 --- a/domain/src/main/java/org/oppia/domain/util/AssetRepository.kt +++ b/utility/src/main/java/org/oppia/util/caching/AssetRepository.kt @@ -1,4 +1,4 @@ -package org.oppia.domain.util +package org.oppia.util.caching import android.content.Context import org.oppia.util.logging.Logger @@ -74,6 +74,10 @@ class AssetRepository @Inject constructor(private val context: Context, private val urlInStream = openRemoteStream(url) val fileOutStream = openLocalCacheFileForWrite(url) return object: InputStream() { + override fun available(): Int { + return urlInStream.available() + } + override fun read(): Int { val byte = urlInStream.read() if (byte != -1) { @@ -82,9 +86,15 @@ class AssetRepository @Inject constructor(private val context: Context, private return byte } + override fun read(b: ByteArray?): Int { + return read(b, 0, b!!.size) + } + override fun read(b: ByteArray?, off: Int, len: Int): Int { val count = urlInStream.read(b, off, len) - fileOutStream.write(b, off, count) + if (count > -1) { + fileOutStream.write(b, off, count) + } return count } diff --git a/utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt b/utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt index 602b26c560e..f546a9bab25 100644 --- a/utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt +++ b/utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt @@ -4,14 +4,21 @@ import android.content.Context import android.graphics.Bitmap import com.bumptech.glide.Glide import com.bumptech.glide.request.target.CustomTarget +import org.oppia.util.caching.AssetRepository import javax.inject.Inject /** An [ImageLoader] that uses Glide. */ -class GlideImageLoader @Inject constructor(private val context: Context) : ImageLoader { +class GlideImageLoader @Inject constructor( + private val context: Context, private val assetRepository: AssetRepository +) : ImageLoader { override fun load(imageUrl: String, target: CustomTarget) { Glide.with(context) .asBitmap() - .load(imageUrl) + .load(object : ImageAssetFetcher { + override fun fetchImage(): ByteArray = assetRepository.loadRemoteBinaryAsset(imageUrl)() + + override fun getImageIdentifier(): String = imageUrl + }) .into(target) } } diff --git a/utility/src/main/java/org/oppia/util/parser/ImageAssetFetcher.kt b/utility/src/main/java/org/oppia/util/parser/ImageAssetFetcher.kt new file mode 100644 index 00000000000..1c21e0ee02c --- /dev/null +++ b/utility/src/main/java/org/oppia/util/parser/ImageAssetFetcher.kt @@ -0,0 +1,10 @@ +package org.oppia.util.parser + +/** Fetcher for image assets from the app's local asset repository. */ +internal interface ImageAssetFetcher { + /** Fetches an image asset. Must be called on a background thread. */ + fun fetchImage(): ByteArray + + /** Returns the identifier corresponding to this image. */ + fun getImageIdentifier(): String +} diff --git a/utility/src/main/java/org/oppia/util/parser/RepositoryGlideModule.kt b/utility/src/main/java/org/oppia/util/parser/RepositoryGlideModule.kt new file mode 100644 index 00000000000..a4cb6651bfc --- /dev/null +++ b/utility/src/main/java/org/oppia/util/parser/RepositoryGlideModule.kt @@ -0,0 +1,17 @@ +package org.oppia.util.parser + +import android.content.Context +import com.bumptech.glide.Glide +import com.bumptech.glide.Registry +import com.bumptech.glide.annotation.GlideModule +import com.bumptech.glide.module.AppGlideModule +import org.oppia.util.caching.AssetRepository +import java.io.InputStream + +/** Custom [AppGlideModule] to enable loading images from [AssetRepository] via Glide. */ +@GlideModule +class RepositoryGlideModule: AppGlideModule() { + override fun registerComponents(context: Context, glide: Glide, registry: Registry) { + registry.prepend(ImageAssetFetcher::class.java, InputStream::class.java, RepositoryModelLoader.Factory()) + } +} diff --git a/utility/src/main/java/org/oppia/util/parser/RepositoryModelLoader.kt b/utility/src/main/java/org/oppia/util/parser/RepositoryModelLoader.kt new file mode 100644 index 00000000000..8b393cbdfb7 --- /dev/null +++ b/utility/src/main/java/org/oppia/util/parser/RepositoryModelLoader.kt @@ -0,0 +1,51 @@ +package org.oppia.util.parser + +import com.bumptech.glide.Priority +import com.bumptech.glide.load.DataSource +import com.bumptech.glide.load.Options +import com.bumptech.glide.load.data.DataFetcher +import com.bumptech.glide.load.model.ModelLoader +import com.bumptech.glide.load.model.ModelLoaderFactory +import com.bumptech.glide.load.model.MultiModelLoaderFactory +import com.bumptech.glide.signature.ObjectKey +import java.io.ByteArrayInputStream +import java.io.InputStream + +// https://bumptech.github.io/glide/tut/custom-modelloader.html#an-empty-implementation. +/** ModelLoader for loading assets from the app's local asset repository. */ +internal class RepositoryModelLoader : ModelLoader { + override fun buildLoadData( + model: ImageAssetFetcher, + width: Int, + height: Int, + options: Options + ): ModelLoader.LoadData? { + return ModelLoader.LoadData(ObjectKey(model.getImageIdentifier()), RepositoryDataFetcher(model)) + } + + override fun handles(model: ImageAssetFetcher): Boolean = true + + private class RepositoryDataFetcher(private val fetcher: ImageAssetFetcher): DataFetcher { + override fun getDataClass(): Class = InputStream::class.java + + override fun cleanup() {} + + override fun getDataSource(): DataSource = DataSource.REMOTE + + override fun cancel() {} + + override fun loadData(priority: Priority, callback: DataFetcher.DataCallback) { + val imageData = fetcher.fetchImage() + callback.onDataReady(ByteArrayInputStream(imageData)) + } + } + + /** [ModelLoaderFactory] for creating new [RepositoryModelLoader]s. */ + internal class Factory: ModelLoaderFactory { + override fun build(multiFactory: MultiModelLoaderFactory): ModelLoader { + return RepositoryModelLoader() + } + + override fun teardown() {} + } +} diff --git a/utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt b/utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt index e274b8b0eee..693ebbe9a3a 100644 --- a/utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt +++ b/utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt @@ -20,7 +20,7 @@ class UrlImageParser private constructor( private val context: Context, @DefaultGcsPrefix private val gcsPrefix: String, @DefaultGcsResource private val gcsResource: String, - @ImageDownloadUrlTemplate private var imageDownloadUrlTemplate: String, + @ImageDownloadUrlTemplate private val imageDownloadUrlTemplate: String, private val htmlContentTextView: TextView, private val entityType: String, private val entityId: String, @@ -32,7 +32,7 @@ class UrlImageParser private constructor( * @return Drawable : Drawable representation of the image. */ override fun getDrawable(urlString: String): Drawable { - imageDownloadUrlTemplate = String.format(imageDownloadUrlTemplate, entityType, entityId, urlString) + val imageDownloadUrlTemplate = String.format(imageDownloadUrlTemplate, entityType, entityId, urlString) val urlDrawable = UrlDrawable() val target = BitmapTarget(urlDrawable) imageLoader.load( @@ -48,7 +48,7 @@ class UrlImageParser private constructor( } override fun onResourceReady(resource: Bitmap, transition: Transition?) { - val drawable = BitmapDrawable(context.resources, resource) + val drawable = BitmapDrawable(context.resources, resource) htmlContentTextView.post { val drawableHeight = drawable.intrinsicHeight val drawableWidth = drawable.intrinsicWidth From 718b4c9dd64025781be381d9a823dea497401782 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sun, 17 Nov 2019 17:04:54 -0800 Subject: [PATCH 7/8] Add module to disable local asset caching by default, and ensure tests pass. --- .../app/application/ApplicationComponent.kt | 3 +- .../domain/audio/AudioPlayerController.kt | 6 +- .../oppia/domain/topic/TopicListController.kt | 65 ++++++++++--------- .../domain/audio/AudioPlayerControllerTest.kt | 5 ++ .../ExplorationProgressControllerTest.kt | 16 +++++ .../topic/StoryProgressControllerTest.kt | 47 ++++++++++++++ .../oppia/domain/topic/TopicControllerTest.kt | 23 +++++++ .../domain/topic/TopicListControllerTest.kt | 45 +++++++++++++ .../oppia/util/caching/CachingAnnotations.kt | 6 ++ .../org/oppia/util/caching/CachingModule.kt | 12 ++++ .../org/oppia/util/parser/GlideImageLoader.kt | 16 +++-- 11 files changed, 205 insertions(+), 39 deletions(-) create mode 100644 utility/src/main/java/org/oppia/util/caching/CachingAnnotations.kt create mode 100644 utility/src/main/java/org/oppia/util/caching/CachingModule.kt diff --git a/app/src/main/java/org/oppia/app/application/ApplicationComponent.kt b/app/src/main/java/org/oppia/app/application/ApplicationComponent.kt index b81f7e60780..32b64f4e689 100644 --- a/app/src/main/java/org/oppia/app/application/ApplicationComponent.kt +++ b/app/src/main/java/org/oppia/app/application/ApplicationComponent.kt @@ -13,6 +13,7 @@ import org.oppia.domain.classify.rules.multiplechoiceinput.MultipleChoiceInputMo import org.oppia.domain.classify.rules.numberwithunits.NumberWithUnitsRuleModule import org.oppia.domain.classify.rules.numericinput.NumericInputRuleModule import org.oppia.domain.classify.rules.textinput.TextInputRuleModule +import org.oppia.util.caching.CachingModule import org.oppia.util.gcsresource.GcsResourceModule import org.oppia.util.logging.LoggerModule import org.oppia.util.parser.GlideImageLoaderModule @@ -29,7 +30,7 @@ import javax.inject.Singleton ContinueModule::class, FractionInputModule::class, ItemSelectionInputModule::class, MultipleChoiceInputModule::class, NumberWithUnitsRuleModule::class, NumericInputRuleModule::class, TextInputRuleModule::class, InteractionsModule::class, GcsResourceModule::class, GlideImageLoaderModule::class, ImageParsingModule::class, - HtmlParserEntityTypeModule::class + HtmlParserEntityTypeModule::class, CachingModule::class ]) interface ApplicationComponent { @Component.Builder diff --git a/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt b/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt index 62a60b77b3c..eae9fcba0e4 100644 --- a/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt +++ b/domain/src/main/java/org/oppia/domain/audio/AudioPlayerController.kt @@ -12,6 +12,7 @@ import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.launch import org.oppia.util.caching.AssetRepository +import org.oppia.util.caching.CacheAssetsLocally import org.oppia.util.data.AsyncResult import org.oppia.util.logging.Logger import org.oppia.util.threading.BackgroundDispatcher @@ -32,7 +33,8 @@ import kotlin.concurrent.withLock class AudioPlayerController @Inject constructor( private val logger: Logger, private val assetRepository: AssetRepository, - @BackgroundDispatcher private val backgroundDispatcher: CoroutineDispatcher + @BackgroundDispatcher private val backgroundDispatcher: CoroutineDispatcher, + @CacheAssetsLocally private val cacheAssetsLocally: Boolean ) { inner class AudioMutableLiveData : MutableLiveData>() { @@ -129,7 +131,7 @@ class AudioPlayerController @Inject constructor( private fun prepareDataSource(url: String) { try { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { + if (cacheAssetsLocally && Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { val mediaDataSource: MediaDataSource = object : MediaDataSource() { private val audioFileBuffer: ByteArray by lazy { // Ensure that the download occurs off the main thread to avoid strict mode violations for diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt index 0744523d57b..d824d164c03 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicListController.kt @@ -34,8 +34,8 @@ import org.oppia.app.model.VoiceoverMapping import org.oppia.domain.exploration.ExplorationRetriever import org.oppia.util.caching.AssetRepository import org.oppia.domain.util.JsonAssetRetriever +import org.oppia.util.caching.CacheAssetsLocally import org.oppia.util.data.AsyncResult -import org.oppia.util.gcsresource.DefaultResource import org.oppia.util.logging.Logger import org.oppia.util.parser.DefaultGcsPrefix import org.oppia.util.parser.DefaultGcsResource @@ -87,6 +87,7 @@ class TopicListController @Inject constructor( private val storyProgressController: StoryProgressController, private val explorationRetriever: ExplorationRetriever, @BackgroundDispatcher private val backgroundDispatcher: CoroutineDispatcher, + @CacheAssetsLocally private val cacheAssetsLocally: Boolean, @DefaultGcsPrefix private val gcsPrefix: String, @DefaultGcsResource private val gcsResource: String, @ImageDownloadUrlTemplate private val imageDownloadUrlTemplate: String, @@ -94,45 +95,47 @@ class TopicListController @Inject constructor( assetRepository: AssetRepository ) { private val backgroundScope = CoroutineScope(backgroundDispatcher) - private val loadAssetJob: Job init { // TODO(#169): Download data reactively rather than during start-up to avoid blocking the main thread on the whole // load operation. - - // Ensure all JSON files are available in memory for quick retrieval. - val allFiles = TOPIC_FILE_ASSOCIATIONS.values.flatten() - val primeAssetJobs = allFiles.map { - backgroundScope.async { - assetRepository.primeTextFileFromLocalAssets(it) - } - } - - // The following job encapsulates all startup loading. NB: We don't currently wait on this job to complete because - // it's fine to try to load the assets at the same time as priming the cache, and it's unlikely the user can get - // into an exploration fast enough to try to load an asset that would trigger a strict mode crash. - loadAssetJob = backgroundScope.launch { - primeAssetJobs.forEach { it.await() } - - // Only download binary assets for one fractions lesson. The others can still be streamed. - val explorations = loadExplorations(listOf(FRACTIONS_EXPLORATION_ID_1)) - val voiceoverUrls = collectAllDesiredVoiceoverUrls(explorations).toSet() - val imageUrls = collectAllImageUrls(explorations).toSet() - logger.d("AssetRepo", "Downloading up to ${voiceoverUrls.size} voiceovers and ${imageUrls.size} images") - val startTime = SystemClock.elapsedRealtime() - val voiceoverDownloadJobs = voiceoverUrls.map { url -> + if (cacheAssetsLocally) { + // Ensure all JSON files are available in memory for quick retrieval. + val allFiles = TOPIC_FILE_ASSOCIATIONS.values.flatten() + val primeAssetJobs = allFiles.map { backgroundScope.async { - assetRepository.primeRemoteBinaryAsset(url) + assetRepository.primeTextFileFromLocalAssets(it) } } - val imageDownloadJobs = imageUrls.map { url -> - backgroundScope.async { - assetRepository.primeRemoteBinaryAsset(url) + + // The following job encapsulates all startup loading. NB: We don't currently wait on this job to complete because + // it's fine to try to load the assets at the same time as priming the cache, and it's unlikely the user can get + // into an exploration fast enough to try to load an asset that would trigger a strict mode crash. + backgroundScope.launch { + primeAssetJobs.forEach { it.await() } + + // Only download binary assets for one fractions lesson. The others can still be streamed. + val explorations = loadExplorations(listOf(FRACTIONS_EXPLORATION_ID_1)) + val voiceoverUrls = collectAllDesiredVoiceoverUrls(explorations).toSet() + val imageUrls = collectAllImageUrls(explorations).toSet() + logger.d( + "AssetRepo", "Downloading up to ${voiceoverUrls.size} voiceovers and ${imageUrls.size} images" + ) + val startTime = SystemClock.elapsedRealtime() + val voiceoverDownloadJobs = voiceoverUrls.map { url -> + backgroundScope.async { + assetRepository.primeRemoteBinaryAsset(url) + } + } + val imageDownloadJobs = imageUrls.map { url -> + backgroundScope.async { + assetRepository.primeRemoteBinaryAsset(url) + } } + (voiceoverDownloadJobs + imageDownloadJobs).forEach { it.await() } + val endTime = SystemClock.elapsedRealtime() + logger.d("AssetRepo", "Finished downloading voiceovers and images in ${endTime - startTime}ms") } - (voiceoverDownloadJobs + imageDownloadJobs).forEach { it.await() } - val endTime = SystemClock.elapsedRealtime() - logger.d("AssetRepo", "Finished downloading voiceovers and images in ${endTime - startTime}ms") } } diff --git a/domain/src/test/java/org/oppia/domain/audio/AudioPlayerControllerTest.kt b/domain/src/test/java/org/oppia/domain/audio/AudioPlayerControllerTest.kt index b7cf589819c..54689476dc3 100644 --- a/domain/src/test/java/org/oppia/domain/audio/AudioPlayerControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/audio/AudioPlayerControllerTest.kt @@ -27,6 +27,7 @@ import org.mockito.Mockito.verify import org.mockito.junit.MockitoJUnit import org.mockito.junit.MockitoRule import org.oppia.domain.audio.AudioPlayerController.PlayStatus +import org.oppia.util.caching.CacheAssetsLocally import org.oppia.util.data.AsyncResult import org.oppia.util.logging.EnableConsoleLog import org.oppia.util.logging.EnableFileLog @@ -476,6 +477,10 @@ class AudioPlayerControllerTest { @GlobalLogLevel @Provides fun provideGlobalLogLevel(): LogLevel = LogLevel.VERBOSE + + @CacheAssetsLocally + @Provides + fun provideCacheAssetsLocally(): Boolean = false } // TODO(#89): Move this to a common test application component. diff --git a/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt index 317aae070e8..7b76696af1c 100644 --- a/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt @@ -42,6 +42,10 @@ import org.oppia.domain.classify.rules.numberwithunits.NumberWithUnitsRuleModule import org.oppia.domain.classify.rules.numericinput.NumericInputRuleModule import org.oppia.domain.classify.rules.textinput.TextInputRuleModule import org.oppia.util.data.AsyncResult +import org.oppia.util.logging.EnableConsoleLog +import org.oppia.util.logging.EnableFileLog +import org.oppia.util.logging.GlobalLogLevel +import org.oppia.util.logging.LogLevel import org.oppia.util.threading.BackgroundDispatcher import org.oppia.util.threading.BlockingDispatcher import org.robolectric.annotation.Config @@ -1248,6 +1252,18 @@ class ExplorationProgressControllerTest { fun provideBlockingDispatcher(@TestDispatcher testDispatcher: TestCoroutineDispatcher): CoroutineDispatcher { return testDispatcher } + + @EnableConsoleLog + @Provides + fun provideEnableConsoleLog(): Boolean = true + + @EnableFileLog + @Provides + fun provideEnableFileLog(): Boolean = false + + @GlobalLogLevel + @Provides + fun provideGlobalLogLevel(): LogLevel = LogLevel.VERBOSE } // TODO(#89): Move this to a common test application component. diff --git a/domain/src/test/java/org/oppia/domain/topic/StoryProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/topic/StoryProgressControllerTest.kt index 91b24330188..459ea9ddc75 100644 --- a/domain/src/test/java/org/oppia/domain/topic/StoryProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/topic/StoryProgressControllerTest.kt @@ -9,14 +9,24 @@ import dagger.BindsInstance import dagger.Component import dagger.Module import dagger.Provides +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestCoroutineDispatcher import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.oppia.app.model.ChapterPlayState.COMPLETED import org.oppia.app.model.ChapterPlayState.NOT_PLAYABLE_MISSING_PREREQUISITES import org.oppia.app.model.ChapterPlayState.NOT_STARTED +import org.oppia.util.logging.EnableConsoleLog +import org.oppia.util.logging.EnableFileLog +import org.oppia.util.logging.GlobalLogLevel +import org.oppia.util.logging.LogLevel +import org.oppia.util.threading.BackgroundDispatcher +import org.oppia.util.threading.BlockingDispatcher import org.robolectric.annotation.Config import javax.inject.Inject +import javax.inject.Qualifier import javax.inject.Singleton /** Tests for [StoryProgressController]. */ @@ -245,6 +255,9 @@ class StoryProgressControllerTest { .inject(this) } + @Qualifier + annotation class TestDispatcher + // TODO(#89): Move this to a common test application component. @Module class TestModule { @@ -253,6 +266,40 @@ class StoryProgressControllerTest { fun provideContext(application: Application): Context { return application } + + @ExperimentalCoroutinesApi + @Singleton + @Provides + @TestDispatcher + fun provideTestDispatcher(): CoroutineDispatcher { + return TestCoroutineDispatcher() + } + + @Singleton + @Provides + @BackgroundDispatcher + fun provideBackgroundDispatcher(@TestDispatcher testDispatcher: CoroutineDispatcher): CoroutineDispatcher { + return testDispatcher + } + + @Singleton + @Provides + @BlockingDispatcher + fun provideBlockingDispatcher(@TestDispatcher testDispatcher: CoroutineDispatcher): CoroutineDispatcher { + return testDispatcher + } + + @EnableConsoleLog + @Provides + fun provideEnableConsoleLog(): Boolean = true + + @EnableFileLog + @Provides + fun provideEnableFileLog(): Boolean = false + + @GlobalLogLevel + @Provides + fun provideGlobalLogLevel(): LogLevel = LogLevel.VERBOSE } // TODO(#89): Move this to a common test application component. diff --git a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt index 4404a1ea41c..35499ca42d7 100644 --- a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt @@ -39,8 +39,13 @@ import org.oppia.app.model.SkillSummary import org.oppia.app.model.StorySummary import org.oppia.app.model.Topic import org.oppia.domain.exploration.TEST_EXPLORATION_ID_30 +import org.oppia.util.caching.CacheAssetsLocally import org.oppia.util.data.AsyncResult import org.oppia.util.data.DataProviders +import org.oppia.util.logging.EnableConsoleLog +import org.oppia.util.logging.EnableFileLog +import org.oppia.util.logging.GlobalLogLevel +import org.oppia.util.logging.LogLevel import org.oppia.util.threading.BackgroundDispatcher import org.oppia.util.threading.BlockingDispatcher import org.robolectric.annotation.Config @@ -762,6 +767,24 @@ class TopicControllerTest { fun provideBlockingDispatcher(@TestDispatcher testDispatcher: CoroutineDispatcher): CoroutineDispatcher { return testDispatcher } + + // TODO(#59): Either isolate these to their own shared test module, or use the real logging + // module in tests to avoid needing to specify these settings for tests. + @EnableConsoleLog + @Provides + fun provideEnableConsoleLog(): Boolean = true + + @EnableFileLog + @Provides + fun provideEnableFileLog(): Boolean = false + + @GlobalLogLevel + @Provides + fun provideGlobalLogLevel(): LogLevel = LogLevel.VERBOSE + + @CacheAssetsLocally + @Provides + fun provideCacheAssetsLocally(): Boolean = false } // TODO(#89): Move this to a common test application component. diff --git a/domain/src/test/java/org/oppia/domain/topic/TopicListControllerTest.kt b/domain/src/test/java/org/oppia/domain/topic/TopicListControllerTest.kt index 5dd4cd37109..e532ef9ab7d 100644 --- a/domain/src/test/java/org/oppia/domain/topic/TopicListControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/topic/TopicListControllerTest.kt @@ -16,6 +16,14 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.oppia.app.model.LessonThumbnailGraphic +import org.oppia.util.caching.CacheAssetsLocally +import org.oppia.util.logging.EnableConsoleLog +import org.oppia.util.logging.EnableFileLog +import org.oppia.util.logging.GlobalLogLevel +import org.oppia.util.logging.LogLevel +import org.oppia.util.parser.DefaultGcsPrefix +import org.oppia.util.parser.DefaultGcsResource +import org.oppia.util.parser.ImageDownloadUrlTemplate import org.oppia.util.threading.BackgroundDispatcher import org.oppia.util.threading.BlockingDispatcher import org.robolectric.annotation.Config @@ -304,6 +312,43 @@ class TopicListControllerTest { fun provideBlockingDispatcher(@TestDispatcher testDispatcher: CoroutineDispatcher): CoroutineDispatcher { return testDispatcher } + + @CacheAssetsLocally + @Provides + fun provideCacheAssetsLocally(): Boolean = false + + @Provides + @DefaultGcsPrefix + @Singleton + fun provideDefaultGcsPrefix(): String { + return "https://storage.googleapis.com/" + } + + @Provides + @DefaultGcsResource + @Singleton + fun provideDefaultGcsResource(): String { + return "oppiaserver-resources/" + } + + @Provides + @ImageDownloadUrlTemplate + @Singleton + fun provideImageDownloadUrlTemplate(): String { + return "%s/%s/assets/image/%s" + } + + @EnableConsoleLog + @Provides + fun provideEnableConsoleLog(): Boolean = true + + @EnableFileLog + @Provides + fun provideEnableFileLog(): Boolean = false + + @GlobalLogLevel + @Provides + fun provideGlobalLogLevel(): LogLevel = LogLevel.VERBOSE } // TODO(#89): Move this to a common test application component. diff --git a/utility/src/main/java/org/oppia/util/caching/CachingAnnotations.kt b/utility/src/main/java/org/oppia/util/caching/CachingAnnotations.kt new file mode 100644 index 00000000000..c9f1232867b --- /dev/null +++ b/utility/src/main/java/org/oppia/util/caching/CachingAnnotations.kt @@ -0,0 +1,6 @@ +package org.oppia.util.caching + +import javax.inject.Qualifier + +/** Corresponds to an injectable boolean indicating whether lesson assets should be cached locally. */ +@Qualifier annotation class CacheAssetsLocally \ No newline at end of file diff --git a/utility/src/main/java/org/oppia/util/caching/CachingModule.kt b/utility/src/main/java/org/oppia/util/caching/CachingModule.kt new file mode 100644 index 00000000000..f167b295862 --- /dev/null +++ b/utility/src/main/java/org/oppia/util/caching/CachingModule.kt @@ -0,0 +1,12 @@ +package org.oppia.util.caching + +import dagger.Module +import dagger.Provides + +/** Provides dependencies corresponding to the app's caching policies. */ +@Module +class CachingModule { + @Provides + @CacheAssetsLocally + fun provideCacheAssetsLocally(): Boolean = false +} diff --git a/utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt b/utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt index f546a9bab25..32c362d4fa2 100644 --- a/utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt +++ b/utility/src/main/java/org/oppia/util/parser/GlideImageLoader.kt @@ -5,20 +5,26 @@ import android.graphics.Bitmap import com.bumptech.glide.Glide import com.bumptech.glide.request.target.CustomTarget import org.oppia.util.caching.AssetRepository +import org.oppia.util.caching.CacheAssetsLocally import javax.inject.Inject /** An [ImageLoader] that uses Glide. */ class GlideImageLoader @Inject constructor( - private val context: Context, private val assetRepository: AssetRepository + private val context: Context, + @CacheAssetsLocally private val cacheAssetsLocally: Boolean, + private val assetRepository: AssetRepository ) : ImageLoader { override fun load(imageUrl: String, target: CustomTarget) { - Glide.with(context) - .asBitmap() - .load(object : ImageAssetFetcher { + val model: Any = if (cacheAssetsLocally) { + object : ImageAssetFetcher { override fun fetchImage(): ByteArray = assetRepository.loadRemoteBinaryAsset(imageUrl)() override fun getImageIdentifier(): String = imageUrl - }) + } + } else imageUrl + Glide.with(context) + .asBitmap() + .load(model) .into(target) } } From 94af9d573c0609108d9e02409b57d39b88be24fc Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 19 Nov 2019 16:47:16 -0800 Subject: [PATCH 8/8] Post-merge fix. --- .../main/java/org/oppia/domain/util/JsonAssetRetriever.kt | 8 -------- 1 file changed, 8 deletions(-) diff --git a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt index d6d914dced2..1bd48d609ce 100644 --- a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt @@ -20,14 +20,6 @@ class JsonAssetRetriever @Inject constructor(private val assetRepository: AssetR return assetRepository.loadTextFileFromLocalAssets(assetName).toByteArray(Charsets.UTF_8).size } - /** Returns the on-disk size of the specified asset, in bytes. */ - fun getAssetSize(assetName: String): Int { - // Unfortunately, the entire file needs to be read to retrieve the asset size since JSON files are compressed in the - // apk. See: https://stackoverflow.com/a/6187097. - // TODO(#386): Use an asset retriever to prefetch and cache these to avoid needing to keep re-reading them. - return context.assets.open(assetName).use { it.readBytes() }.size - } - fun getStringsFromJSONArray(jsonData: JSONArray): List { val stringList = mutableListOf() for (i in 0 until jsonData.length()) {