Fix #3487: Make changes to UI to indicate partial progress of explorations. #3500

MaskedCarrot · 2021-07-17T19:17:14Z

Explanation

Fixes #3487

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

Espresso Tests

RecentlyPlayedFragmentTest

TopicLessonsFragmentTest

Steps to see these changes or a real/emulator device

Change the value of the variables shouldSavePartialProgress and isCheckpointingEnabled to true in TopicLessonsFragmentPresenter or replace topicLessonsFragmentPresneter with this file.
Run the app
start any exploration from the topic lessons tab. Exit it partially complete.
see changes.

MaskedCarrot · 2021-07-17T19:29:09Z

@rt4914 where can I get this icon to represent partial progress?

MaskedCarrot · 2021-07-17T19:30:05Z

@aggarwalpulkit596 Can you please review these changes.

MaskedCarrot · 2021-07-18T05:10:36Z

Now the recently played chapter means chapters that are marked started_not_completed, in_progress_saved and in_progress_not_saved instead of just started_not_completed.

There were a lot of functions in StoryProgressTestHelper that marked topic/stories/explorations as started_not_completed. These functions were named like testMarkChapterAsRecentlyPlayed......... In this PR, every functions that marked a chapter/story/topic as recently played is now expanded into three functions:

testMarkChapterAsStartedNotCompleted......
testMarkChapterAsInProgressSaved.......
testMarkChapterAsInProgressNotSaved......

The functions and their tests (present in StoryProgressTestHelperTest) have remained almost the same, the only change that is made is in the function name and what chapterPlayState they are marking the topic/story/exploration in.

MaskedCarrot · 2021-07-18T08:06:08Z

app/src/sharedTest/java/org/oppia/android/app/home/RecentlyPlayedFragmentTest.kt

@@ -173,12 +173,50 @@ class RecentlyPlayedFragmentTest {
  }

  @Test
-  fun testRecentlyPlayedTestActivity_defaultRecentlyPlayedToolbarTitleIsDisplayed() {


This test was incorrectly passing on robolectic and was failing on espresso.

Title on RecentlyPlayedActivtiy changes depending upon the chapters played. But it takes time to update the title. Adding delay after launching the activity makes this test fail on robolectric too.

This test is now passing.

MaskedCarrot

@aggarwalpulkit596 @anandwana001 PTAL.

rt4914 · 2021-07-22T02:47:12Z

@rt4914 where can I get this icon to represent partial progress?

I will email @mschanteltc for this.

aggarwalpulkit596 · 2021-07-23T09:39:03Z

app/src/main/java/org/oppia/android/app/customview/SegmentedCircularProgressView.kt

+    chapterInProgressArcPaint.color =
+      ContextCompat.getColor(context, R.color.oppiaProgressChapterInProgress)
+
+    chapterNotStartedArcPaint = Paint(Paint.ANTI_ALIAS_FLAG)


Can we move it to helper function since this is being written here twice?

This makes it consistent with chapterFinishedArcPaint. Doing these two with a helper function and chapterFinishedArcPaint without a helper function might be slightly misleading.

Since that is also using a similar process i would suggest moving both of them to a helper function.

aggarwalpulkit596 · 2021-07-23T09:39:51Z

app/src/main/java/org/oppia/android/app/customview/SegmentedCircularProgressView.kt

@@ -109,12 +124,20 @@ class SegmentedCircularProgressView : View {
        canvas.drawArc(baseRect!!, startAngle, sweepAngle, false, chapterFinishedArcPaint)


can we use lateinit instead of making it nullable?

aggarwalpulkit596 · 2021-07-23T09:40:17Z

app/src/main/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentPresenter.kt

+    val inProgressChapterCount =
+      chapterSummaries.map(ChapterSummary::getChapterPlayState)
+        .filter {
+          Log.d("12345", "bindTopicLessonStorySummary: chapter is marked $it")


Cleanup logs

MaskedCarrot · 2021-07-27T09:18:38Z

For now, this PR uses the ic_arrow_right_24dp icon as the icon for showing partial progress. I will update this temporary icon with the actual partial progress icon once it is available. All the tests check the arrow icon for now.

anandwana001 · 2021-07-27T16:17:44Z

I will take a look at this tomorrow.

BenHenning · 2021-07-28T04:56:25Z

Deferring to @aggarwalpulkit596 for approval before I take a look at codeowners.

anandwana001

Thanks @MaskedCarrot
Left nit thoughts.

app/src/sharedTest/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentTest.kt

testing/src/main/java/org/oppia/android/testing/story/StoryProgressTestHelper.kt

testing/src/test/java/org/oppia/android/testing/story/StoryProgressTestHelperTest.kt

rt4914 · 2021-08-08T03:42:21Z

@MaskedCarrot I have done following steps and still I am unable to check this implementation.

Uninstalled the app.
Change isCheckpointingEnabled in ExplorationActivityPresenter to true
Change isCheckpointingEnabled in ExplorationActivity to true
Change shouldSavePartialProgress in ExplorationProgress to true

After these steps I ran the app on emulator Pixel 3A API 28

Open Fractions topic
Open Matthew Goes to Bakery
Complete couple of states and then leave exploration

Unable to see any partial-progress icon. Also when I open the exploration again it starts from first state.

MaskedCarrot · 2021-08-08T06:09:47Z

@rt4914 I missed specifying that isCheckpointingEnabled and shouldSavePartialProgress will only have to be changed in topicLessonsFragmentPresenter. I have updated the PR description now.

rt4914

LGTM, thanks.

anandwana001

Code owner file LGTM.

Could you please add the Espresso test result screenshot of all the test file in this PR? @MaskedCarrot

BenHenning

Thanks @MaskedCarrot. Just a couple of follow-ups, otherwise this PR LGTM.

testing/src/main/java/org/oppia/android/testing/story/StoryProgressTestHelper.kt

.idea/runConfigurations.xml

domain/src/test/java/org/oppia/android/domain/topic/TopicListControllerTest.kt

MaskedCarrot · 2021-08-09T21:57:59Z

@BenHenning PTAL

BenHenning

Thanks @MaskedCarrot. This LGTM!

MaskedCarrot added 2 commits July 18, 2021 00:40

show partial progress in progress bar

3c316cb

fixed and added tests

521490a

MaskedCarrot requested a review from aggarwalpulkit596 July 17, 2021 19:17

MaskedCarrot self-assigned this Jul 17, 2021

removed log.d added for debugging

fea9a96

MaskedCarrot marked this pull request as ready for review July 17, 2021 19:22

MaskedCarrot requested review from anandwana001, BenHenning and rt4914 as code owners July 17, 2021 19:22

MaskedCarrot marked this pull request as draft July 17, 2021 19:22

fixed failing test

8398cdf

MaskedCarrot assigned aggarwalpulkit596 Jul 17, 2021

MaskedCarrot added 2 commits July 18, 2021 12:13

ignore failing tests

c059b77

fixed tests

d1eb649

MaskedCarrot commented Jul 18, 2021

View reviewed changes

MaskedCarrot assigned rt4914 and unassigned MaskedCarrot Jul 19, 2021

MaskedCarrot marked this pull request as ready for review July 21, 2021 14:15

aggarwalpulkit596 reviewed Jul 23, 2021

View reviewed changes

added tests and nit changes

6bc3d36

MaskedCarrot assigned BenHenning and anandwana001 Jul 27, 2021

MaskedCarrot assigned BenHenning Aug 5, 2021

anandwana001 suggested changes Aug 6, 2021

View reviewed changes

anandwana001 assigned MaskedCarrot and unassigned anandwana001 Aug 6, 2021

MaskedCarrot added 3 commits August 7, 2021 00:56

nit

f6ca099

revert nit

15241a5

nit

e31c649

MaskedCarrot assigned anandwana001 and unassigned MaskedCarrot Aug 6, 2021

MaskedCarrot requested a review from anandwana001 August 6, 2021 19:35

rt4914 approved these changes Aug 8, 2021

View reviewed changes

rt4914 removed their assignment Aug 8, 2021

anandwana001 approved these changes Aug 8, 2021

View reviewed changes

anandwana001 assigned MaskedCarrot and unassigned anandwana001 Aug 8, 2021

aggarwalpulkit596 enabled auto-merge (squash) August 9, 2021 14:02

BenHenning reviewed Aug 9, 2021

View reviewed changes

BenHenning removed their assignment Aug 9, 2021

MaskedCarrot and others added 2 commits August 10, 2021 03:23

nits

2c16793

Merge branch 'develop' into show-partial-progress-in-ui

3dbf1e1

MaskedCarrot requested a review from BenHenning August 9, 2021 21:58

MaskedCarrot assigned BenHenning and unassigned MaskedCarrot Aug 9, 2021

lint fix

8ffb3fa

BenHenning approved these changes Aug 10, 2021

View reviewed changes

aggarwalpulkit596 merged commit 630a433 into develop Aug 10, 2021

aggarwalpulkit596 deleted the show-partial-progress-in-ui branch August 10, 2021 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3487: Make changes to UI to indicate partial progress of explorations. #3500

Fix #3487: Make changes to UI to indicate partial progress of explorations. #3500

MaskedCarrot commented Jul 17, 2021 •

edited

MaskedCarrot commented Jul 17, 2021

MaskedCarrot commented Jul 17, 2021

MaskedCarrot commented Jul 18, 2021

MaskedCarrot Jul 18, 2021

MaskedCarrot left a comment

rt4914 commented Jul 22, 2021

aggarwalpulkit596 Jul 23, 2021

MaskedCarrot Jul 27, 2021

MaskedCarrot Jul 27, 2021

aggarwalpulkit596 Jul 30, 2021

MaskedCarrot Jul 30, 2021

aggarwalpulkit596 Jul 23, 2021

MaskedCarrot Jul 27, 2021

aggarwalpulkit596 Jul 23, 2021

MaskedCarrot Jul 27, 2021

MaskedCarrot commented Jul 27, 2021

anandwana001 commented Jul 27, 2021

BenHenning commented Jul 28, 2021

anandwana001 left a comment

rt4914 commented Aug 8, 2021

MaskedCarrot commented Aug 8, 2021

rt4914 left a comment

anandwana001 left a comment •

edited

BenHenning left a comment

MaskedCarrot commented Aug 9, 2021

BenHenning left a comment

		@@ -109,12 +124,20 @@ class SegmentedCircularProgressView : View {
		canvas.drawArc(baseRect!!, startAngle, sweepAngle, false, chapterFinishedArcPaint)

Fix #3487: Make changes to UI to indicate partial progress of explorations. #3500

Fix #3487: Make changes to UI to indicate partial progress of explorations. #3500

Conversation

MaskedCarrot commented Jul 17, 2021 • edited

Explanation

Checklist

Espresso Tests

RecentlyPlayedFragmentTest

TopicLessonsFragmentTest

Steps to see these changes or a real/emulator device

MaskedCarrot commented Jul 17, 2021

MaskedCarrot commented Jul 17, 2021

MaskedCarrot commented Jul 18, 2021

Choose a reason for hiding this comment

MaskedCarrot left a comment

Choose a reason for hiding this comment

rt4914 commented Jul 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskedCarrot commented Jul 27, 2021

anandwana001 commented Jul 27, 2021

BenHenning commented Jul 28, 2021

anandwana001 left a comment

Choose a reason for hiding this comment

rt4914 commented Aug 8, 2021

MaskedCarrot commented Aug 8, 2021

rt4914 left a comment

Choose a reason for hiding this comment

anandwana001 left a comment • edited

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

MaskedCarrot commented Aug 9, 2021

BenHenning left a comment

Choose a reason for hiding this comment

MaskedCarrot commented Jul 17, 2021 •

edited

anandwana001 left a comment •

edited