From f096b1d3260319bb0599692b29e9beb330841d92 Mon Sep 17 00:00:00 2001 From: Jishnu <64526117+JishnuGoyal@users.noreply.github.com> Date: Mon, 3 Oct 2022 05:38:04 +0530 Subject: [PATCH] Fix #4450: Add next and previous card options on revision screen (#4554) * add functionality for next/prev navigation for revision cards * update according to changes from develop * fix bad merge with develop * make toolbar orange * refactor code + add tests * lint/shorten test name * lint/shorten test name * add all refactor jobs * requested changes * add 'scroll to' to tests * add min height and width * remove accidentally added code that led to test failure * pre-load subtopic list size * lint * piping was not correct * lint * lint * disable accessibility checks * finish up small left bits * lint * commit before checkout * partial: Add changes requested in review. * lint * partial: suggested changes * kt to binding * kt to xml binding * change colors of status bar and toolbar color without using logic * lint * fix todo spacing * wrap up with review comments and fix CI * lint * text alignment should be right when only next card is visible * add kdocs * finish up kdocs --- app/src/main/AndroidManifest.xml | 2 +- .../customview/LessonThumbnailImageView.kt | 8 +- .../app/testing/TopicRevisionTestActivity.kt | 10 +- .../android/app/testing/TopicTestActivity.kt | 9 +- .../app/testing/TopicTestActivityForStory.kt | 9 +- .../app/topic/RouteToRevisionCardListener.kt | 7 +- .../oppia/android/app/topic/TopicActivity.kt | 10 +- .../TopicRevisionFragmentPresenter.kt | 12 +- .../revisioncard/RevisionCardActivity.kt | 33 +++- .../RevisionCardActivityPresenter.kt | 9 +- .../revisioncard/RevisionCardFragment.kt | 20 ++- .../RevisionCardFragmentPresenter.kt | 49 +++--- .../revisioncard/RevisionCardViewModel.kt | 150 +++++++++++++++-- .../layout-sw600dp/revision_card_fragment.xml | 149 +++++++++++++++-- .../res/layout/revision_card_activity.xml | 2 +- .../res/layout/revision_card_fragment.xml | 155 ++++++++++++++++-- app/src/main/res/values/color_defs.xml | 1 + app/src/main/res/values/color_palette.xml | 1 + app/src/main/res/values/component_colors.xml | 2 +- app/src/main/res/values/strings.xml | 1 + app/src/main/res/values/themes.xml | 4 + .../revisioncard/RevisionCardActivityTest.kt | 8 +- .../revisioncard/RevisionCardFragmentTest.kt | 153 +++++++++++++---- .../RevisionCardActivityLocalTest.kt | 4 +- .../assets/kdoc_validity_exemptions.textproto | 1 - 25 files changed, 670 insertions(+), 139 deletions(-) diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 19049fee697..1f2250c4152 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -236,7 +236,7 @@ + android:theme="@style/RevisionCardActivityTheme" /> { diff --git a/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivity.kt b/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivity.kt index cff4ff50cc5..27a94b1a706 100644 --- a/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivity.kt +++ b/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivity.kt @@ -7,6 +7,7 @@ import org.oppia.android.app.activity.ActivityComponentImpl import org.oppia.android.app.activity.InjectableAppCompatActivity import org.oppia.android.app.model.ScreenName.REVISION_CARD_ACTIVITY import org.oppia.android.app.player.exploration.BottomSheetOptionsMenuItemClickListener +import org.oppia.android.app.topic.RouteToRevisionCardListener import org.oppia.android.app.topic.conceptcard.ConceptCardListener import org.oppia.android.util.logging.CurrentAppScreenNameIntentDecorator.decorateWithScreenName import javax.inject.Inject @@ -16,6 +17,7 @@ class RevisionCardActivity : InjectableAppCompatActivity(), ReturnToTopicClickListener, ConceptCardListener, + RouteToRevisionCardListener, BottomSheetOptionsMenuItemClickListener { @Inject @@ -30,7 +32,13 @@ class RevisionCardActivity : "Expected topic ID to be included in intent for RevisionCardActivity." } val subtopicId = intent.getIntExtra(SUBTOPIC_ID_EXTRA_KEY, -1) - revisionCardActivityPresenter.handleOnCreate(internalProfileId, topicId, subtopicId) + val subtopicListSize = intent.getIntExtra(SUBTOPIC_LIST_SIZE_EXTRA_KEY, -1) + revisionCardActivityPresenter.handleOnCreate( + internalProfileId, + topicId, + subtopicId, + subtopicListSize + ) } } @@ -42,23 +50,44 @@ class RevisionCardActivity : internal const val INTERNAL_PROFILE_ID_EXTRA_KEY = "RevisionCardActivity.internal_profile_id" internal const val TOPIC_ID_EXTRA_KEY = "RevisionCardActivity.topic_id" internal const val SUBTOPIC_ID_EXTRA_KEY = "RevisionCardActivity.subtopic_id" + internal const val SUBTOPIC_LIST_SIZE_EXTRA_KEY = "RevisionCardActivity.subtopic_list_size" /** Returns a new [Intent] to route to [RevisionCardActivity]. */ fun createRevisionCardActivityIntent( context: Context, internalProfileId: Int, topicId: String, - subtopicId: Int + subtopicId: Int, + subtopicListSize: Int ): Intent { return Intent(context, RevisionCardActivity::class.java).apply { putExtra(INTERNAL_PROFILE_ID_EXTRA_KEY, internalProfileId) putExtra(TOPIC_ID_EXTRA_KEY, topicId) putExtra(SUBTOPIC_ID_EXTRA_KEY, subtopicId) + putExtra(SUBTOPIC_LIST_SIZE_EXTRA_KEY, subtopicListSize) decorateWithScreenName(REVISION_CARD_ACTIVITY) } } } + override fun routeToRevisionCard( + internalProfileId: Int, + topicId: String, + subtopicId: Int, + subtopicListSize: Int + ) { + startActivity( + createRevisionCardActivityIntent( + this, + internalProfileId, + topicId, + subtopicId, + subtopicListSize + ) + ) + this.finish() + } + override fun onReturnToTopicClicked() { onBackPressed() } diff --git a/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivityPresenter.kt b/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivityPresenter.kt index 9fb6a2e4094..1c612583774 100644 --- a/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivityPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivityPresenter.kt @@ -38,7 +38,12 @@ class RevisionCardActivityPresenter @Inject constructor( private lateinit var topicId: String private var subtopicId: Int = 0 - fun handleOnCreate(internalProfileId: Int, topicId: String, subtopicId: Int) { + fun handleOnCreate( + internalProfileId: Int, + topicId: String, + subtopicId: Int, + subtopicListSize: Int + ) { val binding = DataBindingUtil.setContentView( activity, R.layout.revision_card_activity @@ -72,7 +77,7 @@ class RevisionCardActivityPresenter @Inject constructor( if (getReviewCardFragment() == null) { activity.supportFragmentManager.beginTransaction().add( R.id.revision_card_fragment_placeholder, - RevisionCardFragment.newInstance(topicId, subtopicId, profileId) + RevisionCardFragment.newInstance(topicId, subtopicId, profileId, subtopicListSize) ).commitNow() } } diff --git a/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragment.kt b/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragment.kt index 1f663756944..61975b04d27 100755 --- a/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragment.kt +++ b/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragment.kt @@ -18,21 +18,24 @@ class RevisionCardFragment : InjectableDialogFragment() { companion object { private const val TOPIC_ID_ARGUMENT_KEY = "RevisionCardFragment.topic_id" private const val SUBTOPIC_ID_ARGUMENT_KEY = "RevisionCardFragment.subtopic_id" + private const val SUBTOPIC_LIST_SIZE_ARGUMENT_KEY = "RevisionCardFragment.subtopic_list_size" private const val PROFILE_ID_ARGUMENT_KEY = "RevisionCardFragment.profile_id" /** * Returns a new [RevisionCardFragment] to display the specific subtopic for the given topic & * profile. */ - fun newInstance(topicId: String, subtopicId: Int, profileId: ProfileId): RevisionCardFragment { - return RevisionCardFragment().apply { - arguments = Bundle().apply { - putString(TOPIC_ID_ARGUMENT_KEY, topicId) - putInt(SUBTOPIC_ID_ARGUMENT_KEY, subtopicId) - putProto(PROFILE_ID_ARGUMENT_KEY, profileId) + fun newInstance(topicId: String, subtopicId: Int, profileId: ProfileId, subtopicListSize: Int): + RevisionCardFragment { + return RevisionCardFragment().apply { + arguments = Bundle().apply { + putString(TOPIC_ID_ARGUMENT_KEY, topicId) + putInt(SUBTOPIC_ID_ARGUMENT_KEY, subtopicId) + putProto(PROFILE_ID_ARGUMENT_KEY, profileId) + putInt(SUBTOPIC_LIST_SIZE_ARGUMENT_KEY, subtopicListSize) + } } } - } } @Inject @@ -58,8 +61,9 @@ class RevisionCardFragment : InjectableDialogFragment() { } val subtopicId = args.getInt(SUBTOPIC_ID_ARGUMENT_KEY, -1) val profileId = args.getProto(PROFILE_ID_ARGUMENT_KEY, ProfileId.getDefaultInstance()) + val subtopicListSize = args.getInt(SUBTOPIC_LIST_SIZE_ARGUMENT_KEY, -1) return revisionCardFragmentPresenter.handleCreateView( - inflater, container, topicId, subtopicId, profileId + inflater, container, topicId, subtopicId, profileId, subtopicListSize ) } diff --git a/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentPresenter.kt b/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentPresenter.kt index 35d2c496e22..57931980e48 100755 --- a/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentPresenter.kt @@ -9,7 +9,6 @@ import org.oppia.android.app.model.ProfileId import org.oppia.android.app.topic.conceptcard.ConceptCardFragment import org.oppia.android.app.topic.conceptcard.ConceptCardFragment.Companion.CONCEPT_CARD_DIALOG_FRAGMENT_TAG import org.oppia.android.app.translation.AppLanguageResourceHandler -import org.oppia.android.app.viewmodel.ViewModelProvider import org.oppia.android.databinding.RevisionCardFragmentBinding import org.oppia.android.domain.oppialogger.OppiaLogger import org.oppia.android.domain.translation.TranslationController @@ -26,9 +25,9 @@ class RevisionCardFragmentPresenter @Inject constructor( private val htmlParserFactory: HtmlParser.Factory, @DefaultResourceBucketName private val resourceBucketName: String, @TopicHtmlParserEntityType private val entityType: String, - private val viewModelProvider: ViewModelProvider, private val translationController: TranslationController, - private val appLanguageResourceHandler: AppLanguageResourceHandler + private val appLanguageResourceHandler: AppLanguageResourceHandler, + private val revisionCardViewModelFactory: RevisionCardViewModel.Factory ) : HtmlParser.CustomOppiaTagActionListener { private lateinit var profileId: ProfileId @@ -37,7 +36,8 @@ class RevisionCardFragmentPresenter @Inject constructor( container: ViewGroup?, topicId: String, subtopicId: Int, - profileId: ProfileId + profileId: ProfileId, + subtopicListSize: Int ): View? { this.profileId = profileId @@ -48,9 +48,13 @@ class RevisionCardFragmentPresenter @Inject constructor( /* attachToRoot= */ false ) val view = binding.revisionCardExplanationText - val viewModel = getReviewCardViewModel() + val viewModel = revisionCardViewModelFactory.create( + topicId, + subtopicId, + profileId, + subtopicListSize + ) - viewModel.initialize(topicId, subtopicId, profileId) logRevisionCardEvent(topicId, subtopicId) binding.let { @@ -59,22 +63,21 @@ class RevisionCardFragmentPresenter @Inject constructor( } viewModel.revisionCardLiveData.observe( - fragment, - { ephemeralRevisionCard -> - val pageContentsHtml = - translationController.extractString( - ephemeralRevisionCard.revisionCard.pageContents, - ephemeralRevisionCard.writtenTranslationContext - ) - view.text = htmlParserFactory.create( - resourceBucketName, entityType, topicId, imageCenterAlign = true, - customOppiaTagActionListener = this, - displayLocale = appLanguageResourceHandler.getDisplayLocale() - ).parseOppiaHtml( - pageContentsHtml, view, supportsLinks = true, supportsConceptCards = true + fragment + ) { ephemeralRevisionCard -> + val pageContentsHtml = + translationController.extractString( + ephemeralRevisionCard.revisionCard.pageContents, + ephemeralRevisionCard.writtenTranslationContext ) - } - ) + view.text = htmlParserFactory.create( + resourceBucketName, entityType, topicId, imageCenterAlign = true, + customOppiaTagActionListener = this, + displayLocale = appLanguageResourceHandler.getDisplayLocale() + ).parseOppiaHtml( + pageContentsHtml, view, supportsLinks = true, supportsConceptCards = true + ) + } return binding.root } @@ -88,10 +91,6 @@ class RevisionCardFragmentPresenter @Inject constructor( } } - private fun getReviewCardViewModel(): RevisionCardViewModel { - return viewModelProvider.getForFragment(fragment, RevisionCardViewModel::class.java) - } - private fun logRevisionCardEvent(topicId: String, subTopicId: Int) { oppiaLogger.logImportantEvent(oppiaLogger.createOpenRevisionCardContext(topicId, subTopicId)) } diff --git a/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardViewModel.kt b/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardViewModel.kt index 01e859e2d1a..78117b3b9d5 100755 --- a/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardViewModel.kt @@ -1,46 +1,131 @@ package org.oppia.android.app.topic.revisioncard -import android.view.View import androidx.appcompat.app.AppCompatActivity import androidx.lifecycle.LiveData import androidx.lifecycle.Transformations -import org.oppia.android.app.fragment.FragmentScope import org.oppia.android.app.model.EphemeralRevisionCard +import org.oppia.android.app.model.EphemeralSubtopic +import org.oppia.android.app.model.EphemeralTopic import org.oppia.android.app.model.ProfileId +import org.oppia.android.app.topic.RouteToRevisionCardListener import org.oppia.android.app.viewmodel.ObservableViewModel import org.oppia.android.domain.oppialogger.OppiaLogger import org.oppia.android.domain.topic.TopicController +import org.oppia.android.domain.translation.TranslationController import org.oppia.android.util.data.AsyncResult import org.oppia.android.util.data.DataProviders.Companion.toLiveData +import org.oppia.android.util.parser.html.TopicHtmlParserEntityType import javax.inject.Inject -/** [ObservableViewModel] for revision card, providing rich text and worked examples */ -@FragmentScope -class RevisionCardViewModel @Inject constructor( +/** + * [ObservableViewModel] for revision card, providing rich text and worked examples + * + * @property entityType the entity type corresponding to loaded images + * @property topicId the ID of the topic containing the subtopic being viewed + * @property subtopicId the ID of the subtopic being viewed + * @property profileId the ID of the user profile + * @property subtopicListSize the number of subtopics in the parent topic. This is used to determine + * whether or not to show the next/previous cards. + */ +class RevisionCardViewModel private constructor( activity: AppCompatActivity, private val topicController: TopicController, - private val oppiaLogger: OppiaLogger + private val oppiaLogger: OppiaLogger, + val entityType: String, + private val translationController: TranslationController, + val topicId: String, + val subtopicId: Int, + val profileId: ProfileId, + val subtopicListSize: Int ) : ObservableViewModel() { - private lateinit var topicId: String - private var subtopicId: Int = 0 - private lateinit var profileId: ProfileId - private val returnToTopicClickListener: ReturnToTopicClickListener = - activity as ReturnToTopicClickListener + private val routeToReviewListener = activity as RouteToRevisionCardListener + /** Called when the previous navigation card is clicked. */ + fun onPreviousCardClicked() { + routeToReviewListener.routeToRevisionCard( + profileId.internalId, + topicId, + subtopicId - 1, + subtopicListSize + ) + } + + /** Called when the next navigation card is clicked. */ + fun onNextCardClicked() { + routeToReviewListener.routeToRevisionCard( + profileId.internalId, + topicId, + subtopicId + 1, + subtopicListSize + ) + } + + /** The [LiveData] corresponding to the [EphemeralRevisionCard] that is currently being viewed. */ val revisionCardLiveData: LiveData by lazy { processRevisionCardLiveData() } - fun clickReturnToTopic(@Suppress("UNUSED_PARAMETER") v: View) { - returnToTopicClickListener.onReturnToTopicClicked() + private val topicLiveData: LiveData> by lazy { + getTopicResultLiveData() + } + + private fun getTopicResultLiveData(): LiveData> { + return topicController.getTopic(profileId, topicId).toLiveData() } - /** Initializes this view model with necessary identifiers. */ - fun initialize(topicId: String, subtopicId: Int, profileId: ProfileId) { - this.topicId = topicId - this.subtopicId = subtopicId - this.profileId = profileId + /** + * The [LiveData] that will correspond to the next revision card that may be navigated to (as a + * [EphemeralSubtopic]), or default instance of there isn't one. + */ + val nextSubtopicLiveData: LiveData by lazy { + Transformations.map(topicLiveData, ::processNextSubtopicData) + } + + /** + * The [LiveData] that will correspond to the previous revision card that may be navigated to (as + * a [EphemeralSubtopic]), or default instance of there isn't one. + */ + val previousSubtopicLiveData: LiveData by lazy { + Transformations.map(topicLiveData, ::processPreviousSubtopicData) + } + + /** Returns the localised title of the subtopic. */ + fun computeTitleText(subtopic: EphemeralSubtopic?): String { + return subtopic?.let { + translationController.extractString( + subtopic.subtopic.title, + subtopic.writtenTranslationContext + ) + } ?: "" + } + + private fun processPreviousSubtopicData( + topicLiveData: AsyncResult + ): EphemeralSubtopic { + return when (topicLiveData) { + is AsyncResult.Success -> { + val topic = topicLiveData.value + topic.subtopicsList.find { + it.subtopic.subtopicId == subtopicId - 1 + } ?: EphemeralSubtopic.getDefaultInstance() + } + else -> EphemeralSubtopic.getDefaultInstance() + } + } + + private fun processNextSubtopicData( + topicLiveData: AsyncResult + ): EphemeralSubtopic { + return when (topicLiveData) { + is AsyncResult.Success -> { + val topic = topicLiveData.value + topic.subtopicsList.find { + it.subtopic.subtopicId == subtopicId + 1 + } ?: EphemeralSubtopic.getDefaultInstance() + } + else -> EphemeralSubtopic.getDefaultInstance() + } } private val revisionCardResultLiveData: LiveData> by lazy { @@ -65,4 +150,33 @@ class RevisionCardViewModel @Inject constructor( is AsyncResult.Success -> revisionCardResult.value } } + + /** Factory to create new [RevisionCardViewModel]s. */ + class Factory @Inject constructor( + private val activity: AppCompatActivity, + private val topicController: TopicController, + private val oppiaLogger: OppiaLogger, + @TopicHtmlParserEntityType private val entityType: String, + private val translationController: TranslationController + ) { + /** Returns a new [RevisionCardViewModel]. */ + fun create( + topicId: String, + subtopicId: Int, + profileId: ProfileId, + subtopicListSize: Int + ): RevisionCardViewModel { + return RevisionCardViewModel( + activity, + topicController, + oppiaLogger, + entityType, + translationController, + topicId, + subtopicId, + profileId, + subtopicListSize + ) + } + } } diff --git a/app/src/main/res/layout-sw600dp/revision_card_fragment.xml b/app/src/main/res/layout-sw600dp/revision_card_fragment.xml index c982805e7ae..81e4600ac43 100644 --- a/app/src/main/res/layout-sw600dp/revision_card_fragment.xml +++ b/app/src/main/res/layout-sw600dp/revision_card_fragment.xml @@ -1,4 +1,5 @@ - + @@ -34,21 +35,139 @@ style="@style/Body" android:layout_marginTop="@dimen/revision_card_fragment_layout_text_margin_top" /> -