From 3b7ddb91bf67fcbc9abace491f680fdfcd2aa538 Mon Sep 17 00:00:00 2001 From: Vishwajith Shettigar <76042077+Vishwajith-Shettigar@users.noreply.github.com> Date: Mon, 26 Aug 2024 17:16:18 +0530 Subject: [PATCH] Fix #4470, #4472, #4473 : Handle configuration change using onSavedInstance. (#5478) ## Explanation Fixes #4470, Fixes #4472, Fixes #4473. This PR enables the retention of input when the device configuration changes using onSavedInstance. List of interactions covered in this PR: 1. Image Region selection interaction. 2. Drag and Drop interaction | Drag and Drop | ------------------- https://github.com/user-attachments/assets/1919f99d-d56d-4bbe-b0bc-6092c488165e | Image Region | ------------------- https://github.com/user-attachments/assets/ac1ebcd8-92cd-47ca-82af-1208751a7093 Rest are covered in this PR #5458 ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing --- .../ImageRegionSelectionInteractionView.kt | 10 +- .../DragAndDropSortInteractionViewModel.kt | 66 ++++++++-- ...mageRegionSelectionInteractionViewModel.kt | 54 ++++++--- .../app/utility/ClickableAreasImage.kt | 18 ++- .../android/app/utility/RegionClickEvent.kt | 2 +- ...mage_region_selection_interaction_item.xml | 1 + .../app/player/state/StateFragmentTest.kt | 113 ++++++++++++++++++ model/src/main/proto/exploration.proto | 6 +- 8 files changed, 243 insertions(+), 27 deletions(-) diff --git a/app/src/main/java/org/oppia/android/app/player/state/ImageRegionSelectionInteractionView.kt b/app/src/main/java/org/oppia/android/app/player/state/ImageRegionSelectionInteractionView.kt index 53cb5fd3b1b..e7f2dbe3e2e 100644 --- a/app/src/main/java/org/oppia/android/app/player/state/ImageRegionSelectionInteractionView.kt +++ b/app/src/main/java/org/oppia/android/app/player/state/ImageRegionSelectionInteractionView.kt @@ -9,6 +9,7 @@ import androidx.core.view.forEachIndexed import androidx.fragment.app.Fragment import androidx.fragment.app.FragmentManager import org.oppia.android.app.model.ImageWithRegions +import org.oppia.android.app.model.UserAnswerState import org.oppia.android.app.shim.ViewBindingShim import org.oppia.android.app.utility.ClickableAreasImage import org.oppia.android.app.utility.OnClickableAreaClickedListener @@ -52,6 +53,8 @@ class ImageRegionSelectionInteractionView @JvmOverloads constructor( private lateinit var imageUrl: String private lateinit var clickableAreas: List + private lateinit var userAnswerState: UserAnswerState + /** * Sets the URL for the image & initiates loading it. This is intended to be called via * data-binding. @@ -61,6 +64,10 @@ class ImageRegionSelectionInteractionView @JvmOverloads constructor( maybeInitializeClickableAreas() } + fun setUserAnswerState(userAnswerrState: UserAnswerState) { + this.userAnswerState = userAnswerrState + } + fun setEntityId(entityId: String) { this.entityId = entityId maybeInitializeClickableAreas() @@ -121,7 +128,8 @@ class ImageRegionSelectionInteractionView @JvmOverloads constructor( onRegionClicked, bindingInterface, isAccessibilityEnabled = accessibilityService.isScreenReaderEnabled(), - clickableAreas + clickableAreas, + userAnswerState ) areasImage.addRegionViews() performAttachment(areasImage) diff --git a/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/DragAndDropSortInteractionViewModel.kt b/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/DragAndDropSortInteractionViewModel.kt index aa205417099..11d87f4bed0 100644 --- a/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/DragAndDropSortInteractionViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/DragAndDropSortInteractionViewModel.kt @@ -48,7 +48,8 @@ class DragAndDropSortInteractionViewModel private constructor( val isSplitView: Boolean, private val writtenTranslationContext: WrittenTranslationContext, private val resourceHandler: AppLanguageResourceHandler, - private val translationController: TranslationController + private val translationController: TranslationController, + userAnswerState: UserAnswerState ) : StateItemViewModel(ViewType.DRAG_DROP_SORT_INTERACTION), InteractionAnswerHandler, OnItemDragListener, @@ -71,10 +72,18 @@ class DragAndDropSortInteractionViewModel private constructor( subtitledHtml.contentId to translatedHtml } + private var answerErrorCetegory: AnswerErrorCategory = AnswerErrorCategory.NO_ERROR + private val _originalChoiceItems: MutableList = - computeChoiceItems(contentIdHtmlMap, choiceSubtitledHtmls, this, resourceHandler) + computeOriginalChoiceItems(contentIdHtmlMap, choiceSubtitledHtmls, this, resourceHandler) - private val _choiceItems = _originalChoiceItems.toMutableList() + private val _choiceItems = computeSelectedChoiceItems( + contentIdHtmlMap, + choiceSubtitledHtmls, + this, + resourceHandler, + userAnswerState + ) val choiceItems: List = _choiceItems private var pendingAnswerError: String? = null @@ -99,6 +108,7 @@ class DragAndDropSortInteractionViewModel private constructor( pendingAnswerError = null, inputAnswerAvailable = true ) + checkPendingAnswerError(userAnswerState.answerErrorCategory) } override fun onItemDragged( @@ -160,6 +170,7 @@ class DragAndDropSortInteractionViewModel private constructor( * updates the error string based on the specified error category. */ override fun checkPendingAnswerError(category: AnswerErrorCategory): String? { + answerErrorCetegory = category pendingAnswerError = when (category) { AnswerErrorCategory.REAL_TIME -> null AnswerErrorCategory.SUBMIT_TIME -> @@ -232,9 +243,9 @@ class DragAndDropSortInteractionViewModel private constructor( } private fun getSubmitTimeError(): DragAndDropSortInteractionError { - return if (_originalChoiceItems == _choiceItems) + return if (_originalChoiceItems == _choiceItems) { DragAndDropSortInteractionError.EMPTY_INPUT - else + } else DragAndDropSortInteractionError.VALID } @@ -263,13 +274,30 @@ class DragAndDropSortInteractionViewModel private constructor( isSplitView, writtenTranslationContext, resourceHandler, - translationController + translationController, + userAnswerState ) } } + override fun getUserAnswerState(): UserAnswerState { + if (_choiceItems == _originalChoiceItems) { + return UserAnswerState.newBuilder().apply { + this.answerErrorCategory = answerErrorCetegory + }.build() + } + return UserAnswerState.newBuilder().apply { + val htmlContentIds = _choiceItems.map { it.htmlContent } + listOfSetsOfTranslatableHtmlContentIds = + ListOfSetsOfTranslatableHtmlContentIds.newBuilder().apply { + addAllContentIdLists(htmlContentIds) + }.build() + answerErrorCategory = answerErrorCetegory + }.build() + } + companion object { - private fun computeChoiceItems( + private fun computeOriginalChoiceItems( contentIdHtmlMap: Map, choiceStrings: List, dragAndDropSortInteractionViewModel: DragAndDropSortInteractionViewModel, @@ -293,4 +321,28 @@ class DragAndDropSortInteractionViewModel private constructor( }.toMutableList() } } + + private fun computeSelectedChoiceItems( + contentIdHtmlMap: Map, + choiceStrings: List, + dragAndDropSortInteractionViewModel: DragAndDropSortInteractionViewModel, + resourceHandler: AppLanguageResourceHandler, + userAnswerState: UserAnswerState + ): MutableList { + return if (userAnswerState.listOfSetsOfTranslatableHtmlContentIds.contentIdListsCount == 0) { + _originalChoiceItems.toMutableList() + } else { + userAnswerState.listOfSetsOfTranslatableHtmlContentIds.contentIdListsList + .mapIndexed { index, contentId -> + DragDropInteractionContentViewModel( + contentIdHtmlMap = contentIdHtmlMap, + htmlContent = contentId, + itemIndex = index, + listSize = choiceStrings.size, + dragAndDropSortInteractionViewModel = dragAndDropSortInteractionViewModel, + resourceHandler = resourceHandler + ) + }.toMutableList() + } + } } diff --git a/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/ImageRegionSelectionInteractionViewModel.kt b/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/ImageRegionSelectionInteractionViewModel.kt index aa56b9548e9..6440685f19f 100644 --- a/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/ImageRegionSelectionInteractionViewModel.kt +++ b/app/src/main/java/org/oppia/android/app/player/state/itemviewmodel/ImageRegionSelectionInteractionViewModel.kt @@ -30,7 +30,8 @@ class ImageRegionSelectionInteractionViewModel private constructor( private val errorOrAvailabilityCheckReceiver: InteractionAnswerErrorOrAvailabilityCheckReceiver, val isSplitView: Boolean, private val writtenTranslationContext: WrittenTranslationContext, - private val resourceHandler: AppLanguageResourceHandler + private val resourceHandler: AppLanguageResourceHandler, + userAnswerState: UserAnswerState ) : StateItemViewModel(ViewType.IMAGE_REGION_SELECTION_INTERACTION), InteractionAnswerHandler, OnClickableAreaClickedListener { @@ -43,6 +44,12 @@ class ImageRegionSelectionInteractionViewModel private constructor( schemaObject?.customSchemaValue?.imageWithRegions?.labelRegionsList ?: listOf() } + val observableUserAnswrerState by lazy { + ObservableField(userAnswerState) + } + + private var answerErrorCetegory: AnswerErrorCategory = AnswerErrorCategory.NO_ERROR + val imagePath: String by lazy { val schemaObject = interaction.customizationArgsMap["imageAndRegions"] schemaObject?.customSchemaValue?.imageWithRegions?.imagePath ?: "" @@ -68,10 +75,10 @@ class ImageRegionSelectionInteractionViewModel private constructor( pendingAnswerError = null, inputAnswerAvailable = true ) + checkPendingAnswerError(userAnswerState.answerErrorCategory) } override fun onClickableAreaTouched(region: RegionClickedEvent) { - when (region) { is DefaultRegionClickedEvent -> { answerText = "" @@ -88,6 +95,7 @@ class ImageRegionSelectionInteractionViewModel private constructor( /** It checks the pending error for the current image region input, and correspondingly updates the error string based on the specified error category. */ override fun checkPendingAnswerError(category: AnswerErrorCategory): String? { + answerErrorCetegory = category when (category) { AnswerErrorCategory.REAL_TIME -> { pendingAnswerError = null @@ -110,18 +118,35 @@ class ImageRegionSelectionInteractionViewModel private constructor( return pendingAnswerError } - override fun getPendingAnswer(): UserAnswer = UserAnswer.newBuilder().apply { - val answerTextString = answerText.toString() - answer = InteractionObject.newBuilder().apply { - clickOnImage = parseClickOnImage(answerTextString) + override fun getUserAnswerState(): UserAnswerState { + return UserAnswerState.newBuilder().apply { + if (answerText.isNotEmpty()) { + this.imageLabel = answerText.toString() + } + this.answerErrorCategory = answerErrorCetegory }.build() - plainAnswer = resourceHandler.getStringInLocaleWithWrapping( - R.string.image_interaction_answer_text, - answerTextString - ) - this.writtenTranslationContext = - this@ImageRegionSelectionInteractionViewModel.writtenTranslationContext - }.build() + } + + override fun getPendingAnswer(): UserAnswer { + // Resetting Observable UserAnswerState to its default instance to ensure that + // the ImageRegionSelectionInteractionView reflects no image region selection. + // This is necessary because ImageRegionSelectionInteractionView is not recreated every time + // the user submits an answer, causing it to retain the old UserAnswerState. + observableUserAnswrerState.set(UserAnswerState.getDefaultInstance()) + + return UserAnswer.newBuilder().apply { + val answerTextString = answerText.toString() + answer = InteractionObject.newBuilder().apply { + clickOnImage = parseClickOnImage(answerTextString) + }.build() + plainAnswer = resourceHandler.getStringInLocaleWithWrapping( + R.string.image_interaction_answer_text, + answerTextString + ) + this.writtenTranslationContext = + this@ImageRegionSelectionInteractionViewModel.writtenTranslationContext + }.build() + } private fun parseClickOnImage(answerTextString: String): ClickOnImage { val region = selectableRegions.find { it.label == answerTextString } @@ -204,7 +229,8 @@ class ImageRegionSelectionInteractionViewModel private constructor( answerErrorReceiver, isSplitView, writtenTranslationContext, - resourceHandler + resourceHandler, + userAnswerState ) } } diff --git a/app/src/main/java/org/oppia/android/app/utility/ClickableAreasImage.kt b/app/src/main/java/org/oppia/android/app/utility/ClickableAreasImage.kt index 1a0081db7ee..fa77fa271de 100644 --- a/app/src/main/java/org/oppia/android/app/utility/ClickableAreasImage.kt +++ b/app/src/main/java/org/oppia/android/app/utility/ClickableAreasImage.kt @@ -10,6 +10,7 @@ import androidx.core.view.forEachIndexed import androidx.core.view.isVisible import org.oppia.android.R import org.oppia.android.app.model.ImageWithRegions.LabeledRegion +import org.oppia.android.app.model.UserAnswerState import org.oppia.android.app.player.state.ImageRegionSelectionInteractionView import org.oppia.android.app.shim.ViewBindingShim import kotlin.math.roundToInt @@ -21,11 +22,19 @@ class ClickableAreasImage( private val listener: OnClickableAreaClickedListener, bindingInterface: ViewBindingShim, private val isAccessibilityEnabled: Boolean, - private val clickableAreas: List + private val clickableAreas: List, + userAnswerState: UserAnswerState ) { + private var imageLabel: String? = null private val defaultRegionView by lazy { bindingInterface.getDefaultRegion(parentView) } - init { imageView.initializeShowRegionTouchListener() } + init { + imageView.initializeShowRegionTouchListener() + + if (userAnswerState.imageLabel.isNotBlank()) { + imageLabel = userAnswerState.imageLabel + } + } /** * Called when an image is clicked. @@ -41,7 +50,7 @@ class ClickableAreasImage( defaultRegionView.setBackgroundResource(R.drawable.selected_region_background) defaultRegionView.x = x defaultRegionView.y = y - listener.onClickableAreaTouched(DefaultRegionClickedEvent()) + listener.onClickableAreaTouched(DefaultRegionClickedEvent(x, y)) } } @@ -104,6 +113,9 @@ class ClickableAreasImage( newView.isFocusableInTouchMode = true newView.tag = clickableArea.label newView.initializeToggleRegionTouchListener(clickableArea) + if (clickableArea.label.equals(imageLabel)) { + showOrHideRegion(newView = newView, clickableArea = clickableArea) + } if (isAccessibilityEnabled) { // Make default region visibility gone when talkback enabled to avoid any accidental touch. defaultRegionView.isVisible = false diff --git a/app/src/main/java/org/oppia/android/app/utility/RegionClickEvent.kt b/app/src/main/java/org/oppia/android/app/utility/RegionClickEvent.kt index 59ae1a2445c..e8e290c74f1 100644 --- a/app/src/main/java/org/oppia/android/app/utility/RegionClickEvent.kt +++ b/app/src/main/java/org/oppia/android/app/utility/RegionClickEvent.kt @@ -17,4 +17,4 @@ data class NamedRegionClickedEvent(val regionLabel: String, val contentDescripti * Class to be used in case when [OnClickableAreaClickedListener] is called with an unspecified * region that is when any other is tapped on which wasn't defined by creator. */ -class DefaultRegionClickedEvent : RegionClickedEvent() +class DefaultRegionClickedEvent(val x: Float, val y: Float) : RegionClickedEvent() diff --git a/app/src/main/res/layout/image_region_selection_interaction_item.xml b/app/src/main/res/layout/image_region_selection_interaction_item.xml index 7224cabb202..0f338b4b69d 100644 --- a/app/src/main/res/layout/image_region_selection_interaction_item.xml +++ b/app/src/main/res/layout/image_region_selection_interaction_item.xml @@ -58,6 +58,7 @@ app:clickableAreas="@{viewModel.selectableRegions}" app:entityId="@{viewModel.entityId}" app:imageUrl="@{viewModel.imagePath}" + app:userAnswerState="@{viewModel.observableUserAnswrerState}" app:onRegionClicked="@{(region) -> viewModel.onClickableAreaTouched(region)}" app:overlayView="@{interactionContainerFrameLayout}" /> diff --git a/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt index eb141d43015..6bb74d198a5 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt @@ -1118,6 +1118,84 @@ class StateFragmentTest { } } + @Test + @RunOn(TestPlatform.ESPRESSO) // TODO(#1612): Enable for Robolectric. + fun testStateFragment_loadDragDropExp_retainStateOnConfigurationChange() { + setUpTestWithLanguageSwitchingFeatureOff() + launchForExploration(TEST_EXPLORATION_ID_4, shouldSavePartialProgress = false).use { + startPlayingExploration() + dragAndDropItem(fromPosition = 0, toPosition = 1) + rotateToLandscape() + scrollToViewType(DRAG_DROP_SORT_INTERACTION) + onView( + atPositionOnView( + recyclerViewId = R.id.drag_drop_interaction_recycler_view, + position = 0, + targetViewId = R.id.drag_drop_content_text_view + ) + ).check(matches(withText(containsString("a camera at the store")))) + } + } + + @Test + @RunOn(TestPlatform.ESPRESSO) // TODO(#1612): Enable for Robolectric. + fun testStateFragment_loadDragDropExp_mergeItems_dargAndDrop_retainStateOnConfigurationChange() { + setUpTestWithLanguageSwitchingFeatureOff() + launchForExploration(TEST_EXPLORATION_ID_4, shouldSavePartialProgress = false).use { + startPlayingExploration() + mergeDragAndDropItems(position = 0) + dragAndDropItem(fromPosition = 0, toPosition = 2) + rotateToLandscape() + onView( + atPositionOnView( + recyclerViewId = R.id.drag_drop_interaction_recycler_view, + position = 2, + targetViewId = R.id.drag_drop_item_recyclerview + ) + ).check(matches(hasChildCount(2))) + scrollToViewType(DRAG_DROP_SORT_INTERACTION) + onView( + atPositionOnView( + recyclerViewId = R.id.drag_drop_interaction_recycler_view, + position = 2, + targetViewId = R.id.drag_drop_content_text_view + ) + ).check(matches(withText(containsString("a camera at the store")))) + } + } + + @Test + fun testStateFragment_loadDragDropExp_submitTimeError_retainStateOnConfigurationChange() { + setUpTestWithLanguageSwitchingFeatureOff() + launchForExploration(TEST_EXPLORATION_ID_4, shouldSavePartialProgress = false).use { + startPlayingExploration() + onView(withId(R.id.submit_answer_button)).check(matches(isEnabled())) + clickSubmitAnswerButton() + rotateToLandscape() + onView(withId(R.id.drag_drop_interaction_error)).check( + matches(withText(R.string.drag_and_drop_interaction_empty_input)) + ) + } + } + + @Test + fun testStateFragment_loadDragDropExp_mergeFirstTwoItems_retainStateOnConfigurationChange() { + setUpTestWithLanguageSwitchingFeatureOff() + launchForExploration(TEST_EXPLORATION_ID_4, shouldSavePartialProgress = false).use { + startPlayingExploration() + mergeDragAndDropItems(position = 0) + rotateToLandscape() + scrollToViewType(DRAG_DROP_SORT_INTERACTION) + onView( + atPositionOnView( + recyclerViewId = R.id.drag_drop_interaction_recycler_view, + position = 0, + targetViewId = R.id.drag_drop_item_recyclerview + ) + ).check(matches(hasChildCount(2))) + } + } + @Test fun testStateFragment_loadDragDropExp_withoutGrouping_submitWithoutArranging_showsErrorMessage_dragItem_errorMessageIsReset() { // ktlint-disable max-line-length setUpTestWithLanguageSwitchingFeatureOff() @@ -1293,6 +1371,41 @@ class StateFragmentTest { } } + @Test + fun testStateFragment_loadImageRegion_clickRegion6_retainStateOnConfigurationChange() { + setUpTestWithLanguageSwitchingFeatureOff() + launchForExploration(TEST_EXPLORATION_ID_13, shouldSavePartialProgress = false).use { + startPlayingExploration() + waitForImageViewInteractionToFullyLoad() + clickImageRegion(pointX = 0.5f, pointY = 0.5f) + rotateToLandscape() + scrollToViewType(SUBMIT_ANSWER_BUTTON) + onView(withId(R.id.submit_answer_button)).check(matches(isEnabled())) + clickSubmitAnswerButton() + scrollToViewType(FEEDBACK) + onView(withId(R.id.feedback_text_view)).check( + matches( + withText(containsString("Saturn")) + ) + ) + } + } + + @Test + fun testStateFragment_loadImageRegion_submitTimeError_retainStateOnConfigurationChange() { + setUpTestWithLanguageSwitchingFeatureOff() + launchForExploration(TEST_EXPLORATION_ID_13, shouldSavePartialProgress = false).use { + startPlayingExploration() + waitForImageViewInteractionToFullyLoad() + onView(withId(R.id.submit_answer_button)).check(matches(isEnabled())) + clickSubmitAnswerButton() + rotateToLandscape() + onView(withId(R.id.image_input_error)).check( + matches(withText(R.string.image_error_empty_input)) + ) + } + } + @Test @RunOn(TestPlatform.ESPRESSO) // TODO(#1611): Enable for Robolectric. @Ignore("Flaky test") // TODO(#3171): Fix ImageRegion failing test cases. diff --git a/model/src/main/proto/exploration.proto b/model/src/main/proto/exploration.proto index b1ad61b51b4..92b5b40351a 100644 --- a/model/src/main/proto/exploration.proto +++ b/model/src/main/proto/exploration.proto @@ -436,8 +436,12 @@ message UserAnswerState { oneof answer_input_type { // User's selection for selection input interactions. ItemSelectionAnswerState item_selection = 2; - // Raw answer entered by the user in text-based interactions. + // Text answer entered by the user in text-based interactions. string text_input_answer = 3; + // A user's selected list of sets of HTML content IDs in drag-and-drop interactions. + ListOfSetsOfTranslatableHtmlContentIds list_of_sets_of_translatable_html_content_ids = 4; + // Selected image's label. + string image_label = 5; } }