Skip to content

Commit

Permalink
Fixes #3169: Item selection interaction text change and checkboxes sh…
Browse files Browse the repository at this point in the history
…ould be disabled on selection (#4777)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
Fixes #3169
Fixes #3624

This PR is doing 2 things - 

1. It's updating the textView based on the selection of checkboxes by
the user, If the user has not selected any checkbox then the Text will
be `Please select all correct choices.`, If the user has selected less
than maxSelection then the text will be `You may select more choices.`
and if the user has selected maxSelection then the text will be `No more
than $maxSelection choices may be selected.`

2. If the user has selected maximum checkboxes that can be selected,
then the non selected checkboxes will be disbaled, and if the user
unselects the checkbox or checkboxes, then all checkboxes will get
enabled.

This is the link to the repository for the comparison of the
StateFragmentTest.kt, to show that the same tests are failing on both
branches(i.e. my feature branch and my develop branch)

https://github.com/Akshatkamboj14/Test-Comparison-For-StateFragmentTest.kt-oppia-android

<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [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
<!-- Delete these section if this PR does not include UI-related
changes. -->
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))
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing


[issue.webm](https://user-images.githubusercontent.com/92685493/221786503-45d2c027-bedd-4cfb-92ce-3e351e81c34b.webm)

---------

Co-authored-by: Ben Henning <[email protected]>
  • Loading branch information
Akshatkamboj14 and BenHenning authored Mar 2, 2023
1 parent 2eed01e commit c1ce5f8
Show file tree
Hide file tree
Showing 17 changed files with 610 additions and 14 deletions.
3 changes: 2 additions & 1 deletion app/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ VIEW_MODELS_WITH_RESOURCE_IMPORTS = [
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/MathExpressionInteractionsViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/PreviousResponsesHeaderViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/RatioExpressionInputInteractionViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/SubmittedAnswerViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/TextInputViewModel.kt",
"src/main/java/org/oppia/android/app/profile/AddProfileViewModel.kt",
Expand Down Expand Up @@ -316,7 +317,6 @@ VIEW_MODELS = [
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/ReplayButtonViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/ReturnToTopicButtonViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionContentViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/StateItemViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/itemviewmodel/SubmitButtonViewModel.kt",
"src/main/java/org/oppia/android/app/player/state/StateViewModel.kt",
Expand Down Expand Up @@ -469,6 +469,7 @@ BINDING_ADAPTERS_WITH_RESOURCE_IMPORTS = [

# keep sorted
BINDING_ADAPTERS = [
"src/main/java/org/oppia/android/app/databinding/AppCompatCheckBoxBindingAdapters.java",
"src/main/java/org/oppia/android/app/databinding/ConstraintLayoutAdapters.java",
"src/main/java/org/oppia/android/app/databinding/EditTextBindingAdapters.java",
"src/main/java/org/oppia/android/app/databinding/GuidelineBindingAdapters.java",
Expand Down
1 change: 1 addition & 0 deletions app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@
android:name=".app.story.StoryActivity"
android:label="@string/story_activity_title"
android:theme="@style/OppiaThemeWithoutActionBar" />
<activity android:name=".app.testing.AppCompatCheckBoxBindingAdaptersTestActivity" />
<activity android:name=".app.testing.AudioFragmentTestActivity" />
<activity android:name=".app.testing.BindableAdapterTestActivity" />
<activity android:name=".app.testing.ConceptCardFragmentTestActivity" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import org.oppia.android.app.settings.profile.ProfileResetPinActivity
import org.oppia.android.app.splash.SplashActivity
import org.oppia.android.app.story.StoryActivity
import org.oppia.android.app.testing.AdministratorControlsFragmentTestActivity
import org.oppia.android.app.testing.AppCompatCheckBoxBindingAdaptersTestActivity
import org.oppia.android.app.testing.AudioFragmentTestActivity
import org.oppia.android.app.testing.ConceptCardFragmentTestActivity
import org.oppia.android.app.testing.DragDropTestActivity
Expand Down Expand Up @@ -114,6 +115,10 @@ interface ActivityComponentImpl :
fun inject(administratorControlsActivity: AdministratorControlsActivity)
fun inject(administratorControlsFragmentTestActivity: AdministratorControlsFragmentTestActivity)
fun inject(adminPinActivity: AdminPinActivity)
fun inject(
appCompatCheckBoxBindingAdaptersTestActivity:
AppCompatCheckBoxBindingAdaptersTestActivity
)
fun inject(appLanguageActivity: AppLanguageActivity)
fun inject(appVersionActivity: AppVersionActivity)
fun inject(audioFragmentTestActivity: AudioFragmentTestActivity)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package org.oppia.android.app.databinding;

import android.content.res.ColorStateList;
import androidx.annotation.ColorInt;
import androidx.annotation.NonNull;
import androidx.appcompat.widget.AppCompatCheckBox;
import androidx.databinding.BindingAdapter;

/**
* Custom data-binding adapters for {@link AppCompatCheckBox}s.
*/
public final class AppCompatCheckBoxBindingAdapters {
/** Sets the button tint for the specified checkbox, via data-binding. */
@BindingAdapter("app:buttonTint")
public static void setButtonTint(@NonNull AppCompatCheckBox checkBox, @ColorInt int colorRgb) {
checkBox.setSupportButtonTintList(ColorStateList.valueOf(colorRgb));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ class SelectionInteractionContentViewModel(
val htmlContent: SubtitledHtml,
val hasConversationView: Boolean,
private val itemIndex: Int,
private val selectionInteractionViewModel: SelectionInteractionViewModel
private val selectionInteractionViewModel: SelectionInteractionViewModel,
val isEnabled: ObservableBoolean
) : ObservableViewModel() {
var isAnswerSelected = ObservableBoolean()

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package org.oppia.android.app.player.state.itemviewmodel

import androidx.databinding.Observable
import androidx.databinding.ObservableBoolean
import androidx.databinding.ObservableField
import androidx.databinding.ObservableList
import org.oppia.android.R
import org.oppia.android.app.model.Interaction
import org.oppia.android.app.model.InteractionObject
import org.oppia.android.app.model.SetOfTranslatableHtmlContentIds
Expand All @@ -13,6 +15,7 @@ import org.oppia.android.app.model.WrittenTranslationContext
import org.oppia.android.app.player.state.answerhandling.InteractionAnswerErrorOrAvailabilityCheckReceiver
import org.oppia.android.app.player.state.answerhandling.InteractionAnswerHandler
import org.oppia.android.app.player.state.answerhandling.InteractionAnswerReceiver
import org.oppia.android.app.translation.AppLanguageResourceHandler
import org.oppia.android.app.viewmodel.ObservableArrayList
import org.oppia.android.domain.translation.TranslationController
import javax.inject.Inject
Expand All @@ -31,7 +34,8 @@ class SelectionInteractionViewModel private constructor(
private val interactionAnswerErrorOrAvailabilityCheckReceiver: InteractionAnswerErrorOrAvailabilityCheckReceiver, // ktlint-disable max-line-length
val isSplitView: Boolean,
val writtenTranslationContext: WrittenTranslationContext,
private val translationController: TranslationController
private val translationController: TranslationController,
private val resourceHandler: AppLanguageResourceHandler
) : StateItemViewModel(ViewType.SELECTION_INTERACTION), InteractionAnswerHandler {
private val interactionId: String = interaction.id

Expand All @@ -52,10 +56,21 @@ class SelectionInteractionViewModel private constructor(
?: minAllowableSelectionCount
}
private val selectedItems: MutableList<Int> = mutableListOf()
private val enabledItemsList by lazy {
List(choiceSubtitledHtmls.size) {
ObservableBoolean(true)
}
}
val choiceItems: ObservableList<SelectionInteractionContentViewModel> =
computeChoiceItems(choiceSubtitledHtmls, hasConversationView, this)
computeChoiceItems(choiceSubtitledHtmls, hasConversationView, this, enabledItemsList)

private val isAnswerAvailable = ObservableField(false)
val selectedItemText =
ObservableField(
resourceHandler.getStringInLocale(
R.string.state_fragment_item_selection_no_items_selected_hint_text
)
)

init {
val callback: Observable.OnPropertyChangedCallback =
Expand Down Expand Up @@ -124,21 +139,23 @@ class SelectionInteractionViewModel private constructor(
isCurrentlySelected -> {
selectedItems -= itemIndex
updateIsAnswerAvailable()
updateSelectionText()
updateItemSelectability()
false
}
!areCheckboxesBound() -> {
// Disable all items to simulate a radio button group.
// De-select all other items to simulate a radio button group.
choiceItems.forEach { item -> item.isAnswerSelected.set(false) }
selectedItems.clear()
selectedItems += itemIndex
updateIsAnswerAvailable()
true
}
selectedItems.size < maxAllowableSelectionCount -> {
// TODO(#3624): Add warning to user when they exceed the number of allowable selections or are under the minimum
// number required.
selectedItems += itemIndex
updateIsAnswerAvailable()
updateSelectionText()
updateItemSelectability()
true
}
else -> {
Expand All @@ -148,6 +165,38 @@ class SelectionInteractionViewModel private constructor(
}
}

private fun updateSelectionText() {
if (selectedItems.size < maxAllowableSelectionCount) {
selectedItemText.set(
resourceHandler.getStringInLocale(
R.string.state_fragment_item_selection_some_items_selected_hint_text
)
)
}
if (selectedItems.size == 0) {
selectedItemText.set(
resourceHandler.getStringInLocale(
R.string.state_fragment_item_selection_no_items_selected_hint_text
)
)
}
if (selectedItems.size == maxAllowableSelectionCount) {
selectedItemText.set(
resourceHandler.getStringInLocaleWithWrapping(
R.string.state_fragment_item_selection_max_items_selected_hint_text,
maxAllowableSelectionCount.toString()
)
)
}
}

private fun updateItemSelectability() {
if (selectedItems.size == maxAllowableSelectionCount) {
// All non-selected items should be disabled when the limit is reached.
enabledItemsList.filterIndexed { idx, _ -> idx !in selectedItems }.forEach { it.set(false) }
} else enabledItemsList.forEach { it.set(true) } // Otherwise, all items are available.
}

private fun areCheckboxesBound(): Boolean {
return interactionId == "ItemSelectionInput" && maxAllowableSelectionCount > 1
}
Expand All @@ -161,7 +210,8 @@ class SelectionInteractionViewModel private constructor(

/** Implementation of [StateItemViewModel.InteractionItemFactory] for this view model. */
class FactoryImpl @Inject constructor(
private val translationController: TranslationController
private val translationController: TranslationController,
private val resourceHandler: AppLanguageResourceHandler
) : InteractionItemFactory {
override fun create(
entityId: String,
Expand All @@ -181,7 +231,8 @@ class SelectionInteractionViewModel private constructor(
answerErrorReceiver,
isSplitView,
writtenTranslationContext,
translationController
translationController,
resourceHandler
)
}
}
Expand All @@ -190,15 +241,17 @@ class SelectionInteractionViewModel private constructor(
private fun computeChoiceItems(
choiceSubtitledHtmls: List<SubtitledHtml>,
hasConversationView: Boolean,
selectionInteractionViewModel: SelectionInteractionViewModel
selectionInteractionViewModel: SelectionInteractionViewModel,
enabledItemsList: List<ObservableBoolean>
): ObservableArrayList<SelectionInteractionContentViewModel> {
val observableList = ObservableArrayList<SelectionInteractionContentViewModel>()
observableList += choiceSubtitledHtmls.mapIndexed { index, subtitledHtml ->
SelectionInteractionContentViewModel(
htmlContent = subtitledHtml,
hasConversationView = hasConversationView,
itemIndex = index,
selectionInteractionViewModel = selectionInteractionViewModel
selectionInteractionViewModel = selectionInteractionViewModel,
isEnabled = enabledItemsList[index]
)
}
return observableList
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package org.oppia.android.app.testing

import android.os.Bundle
import org.oppia.android.R
import org.oppia.android.app.activity.ActivityComponentImpl
import org.oppia.android.app.activity.InjectableAppCompatActivity

/** Test activity for [org.oppia.android.app.databinding.AppCompatCheckBoxBindingAdapters]. */
class AppCompatCheckBoxBindingAdaptersTestActivity : InjectableAppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
(activityComponent as ActivityComponentImpl).inject(this)
setContentView(R.layout.test_app_compat_check_box_bindable_adapter_activity)
}
}
5 changes: 3 additions & 2 deletions app/src/main/res/layout/item_selection_interaction_items.xml
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,10 @@
android:layout_height="wrap_content"
android:checked="@{viewModel.answerSelected}"
android:clickable="false"
android:enabled="@{viewModel.isEnabled}"
android:focusable="false"
android:labelFor="@id/item_selection_contents_text_view"
app:buttonTint="@color/shared_interaction_selector" />
app:buttonTint="@{viewModel.isEnabled ? @color/component_color_shared_item_selection_interaction_enabled_color : @color/component_color_shared_item_selection_interaction_disabled_color}" />

<TextView
android:id="@+id/item_selection_contents_text_view"
Expand All @@ -43,7 +44,7 @@
android:layout_toEndOf="@+id/item_selection_checkbox"
android:fontFamily="sans-serif"
android:text="@{htmlContent}"
android:textColor="@{viewModel.isAnswerSelected() ? @color/component_color_shared_selection_interaction_selected_text_color : @color/component_color_shared_selection_interaction_unselected_text_color}"
android:textColor="@{viewModel.isEnabled ? @color/component_color_shared_item_selection_interaction_enabled_color : @color/component_color_shared_item_selection_interaction_disabled_color}"
android:textSize="16sp" />
</RelativeLayout>
</layout>
2 changes: 1 addition & 1 deletion app/src/main/res/layout/selection_interaction_item.xml
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
android:layout_marginTop="4dp"
android:layout_marginBottom="4dp"
android:fontFamily="sans-serif-light"
android:text="@string/item_selection_text"
android:text="@{viewModel.selectedItemText}"
android:textColor="@color/component_color_shared_primary_text_color"
android:textSize="14sp"
android:textStyle="italic"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="utf-8"?>
<layout xmlns:android="http://schemas.android.com/apk/res/android">

<androidx.appcompat.widget.AppCompatCheckBox
android:id="@+id/test_check_box"
android:layout_width="wrap_content"
android:layout_height="wrap_content" />
</layout>
2 changes: 2 additions & 0 deletions app/src/main/res/values/component_colors.xml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
<color name="component_color_shared_item_selection_interaction_unselected_color">@color/color_palette_item_selection_unselected_color</color>
<color name="component_color_shared_selection_interaction_selected_text_color">@color/color_palette_item_selection_unselected_text_color</color>
<color name="component_color_shared_selection_interaction_unselected_text_color">@color/color_palette_item_selection_selected_text_color</color>
<color name="component_color_shared_item_selection_interaction_enabled_color">@color/color_palette_item_selection_selected_text_color</color>
<color name="component_color_shared_item_selection_interaction_disabled_color">@color/color_palette_disabled_button_background_color</color>
<color name="component_color_shared_back_forward_arrow_button_color">@color/color_palette_back_forward_arrow_button_color</color>
<color name="component_color_shared_replay_button_color">@color/color_palette_replay_button_color</color>
<color name="component_color_shared_progress_bar_solid_color">@color/color_palette_progress_bar_solid_color</color>
Expand Down
3 changes: 3 additions & 0 deletions app/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -584,4 +584,7 @@
<string name="administrator_controls_fragment_test_activity_label">Administrator Controls Fragment Test Activity</string>
<string name="revision_navigation_cards_header">Continue Studying</string>
<string name="state_fragment_hint_bar_forced_announcement_text">Go to the bottom of the screen for a hint.</string>
<string name="state_fragment_item_selection_no_items_selected_hint_text">Please select all correct choices.</string>
<string name="state_fragment_item_selection_some_items_selected_hint_text">You may select more choices.</string>
<string name="state_fragment_item_selection_max_items_selected_hint_text">No more than %s choices may be selected.</string>
</resources>
Loading

0 comments on commit c1ce5f8

Please sign in to comment.