Fix #4492 & 3537: Improved hints and solution UI UX #4500

rt4914 · 2022-08-15T18:14:38Z

Explanation

Fixes #4492 by introducing a complete revamp of the hint & solution management flow in the app (see designs below).

Fixes #3537 by merging the hint summary layouts.

Note that one part of #4492 isn't implemented: pausing the hint timer while hints are being viewed. Ensuring this is properly implemented is being tracked as part of #4510.

Two new test exemptions were added: one for a new view model file (which seems sensible since most of the changes in this PR are aesthetic and not functional), and a second for a new hints configuration module that's specific to the alpha user study build of the app (which is temporary and we generally don't test thoroughly, anyway).

Designs
https://xd.adobe.com/view/3dca36c2-5115-419c-b25e-0f10526b077c-6899/screen/619724cc-0ecf-4c32-a452-432fded803f5/specs/
https://xd.adobe.com/view/3dca36c2-5115-419c-b25e-0f10526b077c-6899/screen/5311124a-b143-4916-aa46-6e318aca5650/specs/
https://xd.adobe.com/view/3dca36c2-5115-419c-b25e-0f10526b077c-6899/screen/545c6f32-2fec-4a8b-bb53-cecfb8e52f31/specs/

Essential Checklist

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: ...".)
Any changes to scripts/assets files have their rationale included in the PR explanation.
The PR follows the style guide.
The PR does not contain any unnecessary code changes from Android Studio (reference).
The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
The PR is assigned to the appropriate reviewers (reference).

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)
Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

Mobile Portrait

Mobile Landscape

Tablet

Return to Lesson button

NOTE: This button was created later on that's why not all screenshots contain this button but the below screenshot should suffice. It is fully functional too.

Dark Mode (Not Implemented)

Do not have designs for dark mode

RTL Support

Accessibility Output

There are few issues but not solving them right now.

Video Output

20220816_000912.mp4

BenHenning

Thanks for much @rt4914! Really well done implementation. Just had a few comments that I plan to follow up on directly.

BenHenning · 2022-08-16T23:51:05Z

app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragment.kt

-      if (currentExpandedHintListIndex == -1) {
-        currentExpandedHintListIndex = null
-      }
+      expandedItemsList =


Noting that it would be preferable to use a proto here to avoid passing around mutable state.

.../main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragmentPresenter.kt

BenHenning · 2022-08-16T23:57:57Z

.../main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragmentPresenter.kt

+  ) {
+    binding.buttonViewModel = returnToLessonViewModel
+
+    binding.returnToLessonButton.setOnClickListener {


Could be moved to the view model for slightly simpler binding.

app/src/main/java/org/oppia/android/app/hintsandsolution/HintsViewModel.kt

app/src/main/res/values/strings.xml

app/src/test/java/org/oppia/android/app/player/state/StateFragmentLocalTest.kt

BenHenning · 2022-08-17T00:15:00Z

app/src/test/java/org/oppia/android/app/player/state/StateFragmentLocalTest.kt

      closeHintsAndSolutionsDialog()

      onView(withId(R.id.hint_bulb))
        .check(matches(withDrawable(R.drawable.ic_keyboard_arrow_right_white_48)))
    }
  }

+  @Test
+  fun testStateFragment_openHintsAndSolution_checkReturnToLessonButtonIsVisible() {


What about a test verifying clicking on the return to lesson button?

BenHenning · 2022-08-17T00:15:24Z

app/src/test/java/org/oppia/android/app/player/state/StateFragmentLocalTest.kt

      closeHintsAndSolutionsDialog()

      onView(withId(R.id.hint_bulb))
        .check(matches(withDrawable(R.drawable.ic_keyboard_arrow_right_white_48)))
    }
  }

+  @Test
+  fun testStateFragment_openHintsAndSolution_checkReturnToLessonButtonIsVisible() {


Might be worth adding these new tests to questions' local tests, too.

app/src/main/res/values/strings.xml

BenHenning · 2022-08-19T07:45:08Z

FWIW this seems like a complete fix of #4492 as far as I can tell, though some investigation may be needed with regards to the hint timing logic.

BenHenning · 2022-08-19T07:47:50Z

Confirming that the current logic will NOT pause the timer while the hints are being viewed (I think this is a change in behavior from the old hints implementation). I think fixing it will be straightforward, but I'll follow up soon.

BenHenning · 2022-08-19T07:53:06Z

Okay, this is very much not going to be trivial, so I think we may need to skip it for this release. Will follow up with Sean on product priority here.

The main issue is that hints are handled in the domain layer entirely separately from the frontend lifecycle. We almost certainly would need to either somehow provide signal from the frontend when hints are being shown (and not mess this up since it'll break the hints timer), or manage hint state based on the observation of its results (similar to how ExplorationProgressController used to work), but this is difficult and not at all straightforward since hint behaviors are observed through ExplorationProgressController. I suspect we may need the DataProvider dynamic transformation that I'm working on for the beta release in order to even start tackling this issue.

seanlip · 2022-08-19T08:00:18Z

I would say, just make the timer for showing the next hint long enough so that it's unlikely that a student will get another hint while the current hint is showing. 2 or 2.5 minutes should be reasonable.

(Note: this is still a stopgap and we should fix it properly in the long term.)

BenHenning · 2022-08-19T08:07:05Z

I would say, just make the timer for showing the next hint long enough so that it's unlikely that a student will get another hint while the current hint is showing. 2 or 2.5 minutes should be reasonable.

(Note: this is still a stopgap and we should fix it properly in the long term.)

@seanlip fortunately we can very easily tweak this specifically for the study build. We currently have three hint delays which I think should be adjusted as such:

Delay for initial hint: 60 seconds (this should be kept the same since the issue won't affect it)
Delay for subsequent hints if the user doesn't try to answer: 30 seconds (bump to 2.5 minutes)
Delay for subsequent hints if the user submits a wrong answer: 10 seconds (keep since the user is more likely on the card at this point, and we want them to see the hint quickly)

Does this seem sensible?

seanlip · 2022-08-19T08:11:48Z

Yes, seems reasonable. I slightly lean towards making the third one 15 seconds instead of 10, to give the learner some time to read the wrong answer feedback.

Update hint delay times for the Kenya-specific alpha build of the app.

BenHenning

Approving this with some open comments. #4510 is tracking addressing those comments post-merge. Plus, an additional review should take place for the changes I contributed directly to the PR (since they haven't yet been reviewed).

rt4914 added 4 commits August 15, 2022 21:25

UI for xml files

b51239b

Removed divider

c7e0c1b

Removed const value

5d6918d

Multie expand issue fixed

b4d8ec4

rt4914 requested a review from BenHenning as a code owner August 15, 2022 18:14

rt4914 added 8 commits August 16, 2022 00:02

Tablet UI changes

c1372d3

Added return to topic button

c2bbf28

Removed index multiplier from tests

5eced27

Updated tests

6c0cb84

nit fixes

8c4ae65

nit fixes

572746e

Fixed implementation

1c91573

Updated tests

5575902

rt4914 changed the title ~~Fix #4492 & 3537: Improved hints and solution UI UX~~ Fix part of #4492 & 3537: Improved hints and solution UI UX Aug 15, 2022

rt4914 added 2 commits August 16, 2022 04:10

Fixed test case

653367d

Bazel fix

3904726

rt4914 assigned BenHenning Aug 16, 2022

BenHenning reviewed Aug 17, 2022

View reviewed changes

rt4914 added 2 commits August 17, 2022 12:50

Minor suggested changes

59cabb6

Nit fixes

de2f80f

BenHenning reviewed Aug 18, 2022

View reviewed changes

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved

BenHenning changed the title ~~Fix part of #4492 & 3537: Improved hints and solution UI UX~~ Fix #4492 & 3537: Improved hints and solution UI UX Aug 19, 2022

Revert back string change for hint button.

4d88cbd

BenHenning mentioned this pull request Aug 19, 2022

Fast-follow work items from MR5 (mainly reviews and open comments) #4510

Open

Merge branch 'develop' into improved-hints-and-solution-ui-ux

3c93348

Fix broken hints.

492abaa

Update hint delay times for the Kenya-specific alpha build of the app.

BenHenning added the TODO: Review (post-merge) label Aug 19, 2022

BenHenning approved these changes Aug 19, 2022

View reviewed changes

BenHenning merged commit 6bab482 into develop Aug 19, 2022

BenHenning deleted the improved-hints-and-solution-ui-ux branch August 19, 2022 10:15

seanlip mentioned this pull request Jun 9, 2023

[BUG]: Fix hint timers continuing to count while hints are being viewed #5037

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4492 & 3537: Improved hints and solution UI UX #4500

Fix #4492 & 3537: Improved hints and solution UI UX #4500

rt4914 commented Aug 15, 2022 •

edited by BenHenning

BenHenning left a comment

BenHenning Aug 16, 2022

BenHenning Aug 16, 2022

BenHenning Aug 17, 2022

BenHenning Aug 17, 2022

BenHenning commented Aug 19, 2022

BenHenning commented Aug 19, 2022

BenHenning commented Aug 19, 2022

seanlip commented Aug 19, 2022 •

edited

BenHenning commented Aug 19, 2022

seanlip commented Aug 19, 2022

BenHenning left a comment

Fix #4492 & 3537: Improved hints and solution UI UX #4500

Fix #4492 & 3537: Improved hints and solution UI UX #4500

Conversation

rt4914 commented Aug 15, 2022 • edited by BenHenning

Explanation

Essential Checklist

For UI-specific PRs only

Mobile Portrait

Mobile Landscape

Tablet

Return to Lesson button

Dark Mode (Not Implemented)

RTL Support

Accessibility Output

Video Output

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Aug 16, 2022

Choose a reason for hiding this comment

BenHenning Aug 16, 2022

Choose a reason for hiding this comment

BenHenning Aug 17, 2022

Choose a reason for hiding this comment

BenHenning Aug 17, 2022

Choose a reason for hiding this comment

BenHenning commented Aug 19, 2022

BenHenning commented Aug 19, 2022

BenHenning commented Aug 19, 2022

seanlip commented Aug 19, 2022 • edited

BenHenning commented Aug 19, 2022

seanlip commented Aug 19, 2022

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 commented Aug 15, 2022 •

edited by BenHenning

seanlip commented Aug 19, 2022 •

edited