Fix #2132: Chapter List Page Issues (#2232) - Blur and lock thumbnails of inactive cards. #2422

viktoriias · 2021-01-07T05:02:16Z

Explanation

Fixes #2132. Here I blur and lock thumbnails of inactive cards.

LessonThumbnailImageView loads images in fun loadLessonThumbnail():

if (lessonThumbnail.thumbnailFilename.isNotEmpty()) {
  // This branch will be used in production. I'm not able to test blurring here.
  loadImage(lessonThumbnail.thumbnailFilename)
} else {
  // This branch is executed in develop. I can test blurring this.
  imageView.setImageResource(getLessonDrawableResource(lessonThumbnail))
}

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.

Test runs relevant to this PR:

viktoriias · 2021-01-07T05:24:40Z

Result:

viktoriias · 2021-01-07T16:52:08Z

@anandwana001 @BenHenning Please review first draft.

BenHenning

Thanks @viktoriias! Took a high-level pass & focused generally less on nits/readability.

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

app/src/main/res/layout-land/story_chapter_view.xml

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

model/src/main/proto/thumbnail.proto

utility/src/main/java/org/oppia/android/util/parser/BlurTransformation.kt

utility/src/main/java/org/oppia/android/util/parser/ImageLoader.kt

viktoriias · 2021-01-09T03:33:55Z

Thanks @viktoriias! Took a high-level pass & focused generally less on nits/readability.

@BenHenning PTAL

BenHenning

Thanks @viktoriias! Other than the open questions around the blur transformation itself, I think this is looking good. Had some additional minor comments. Also, we should add tests for the new blurring behavior. To do this, I think you can provide a mock ImageLoader (e.g. using Mockito) by creating a test module rather than including GlideImageLoaderModule. By doing this, you can verify that the system being tested is properly blurring the image by verifying that it correctly passes the BLUR image transformation in the situations we're interested in.

I think just having tests for story_chapter_view is sufficient, but we should make sure to cover the portrait/landscape and handset/tablet scenarios (e.g. if any of the four XML files change, the tests should fail).

utility/src/main/java/org/oppia/android/util/parser/ImageLoader.kt

utility/src/main/java/org/oppia/android/util/parser/GlideImageLoader.kt

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

utility/src/main/java/org/oppia/android/util/parser/BlurTransformation.kt

viktoriias · 2021-01-10T05:09:37Z

We should add tests for the new blurring behavior. To do this, I think you can provide a mock ImageLoader (e.g. using Mockito) by creating a test module rather than including GlideImageLoaderModule. By doing this, you can verify that the system being tested is properly blurring the image by verifying that it correctly passes the BLUR image transformation in the situations we're interested in.

I think just having tests for story_chapter_view is sufficient, but we should make sure to cover the portrait/landscape and handset/tablet scenarios (e.g. if any of the four XML files change, the tests should fail).

@BenHenning @anandwana001 I have some questions regarding testing.

This PR is causing testRecentlyPlayedTestActivity_recyclerViewItem1_lessonThumbnailIsCorrect to fail. I don't think it's possible to test Glige loading image using withDrawable(R.drawable.lesson_thumbnail_name), but I can't find an official proof of that. Can I remove two lessonThumbnailIsCorrect tests from RecentlyPlayedFragmentTest?

Another question is about StoryFragmentTest, where I believe I would add tests for new blurring behavior. I didn't realize StoryFragmentTest wasn't included in CI Lint and Tests / Robolectric Tests. When I ran it in Android Studio, all tests in this class failed with the following error: android.renderscript.RSDriverException: Failed to create RS context.
The I found out that Renderscript does not work with Robolectric: robolectric/robolectric#1034
Will mock ImageLoader solve this problem? (I'm still working on figuring out the mocking part)

anandwana001 · 2021-01-11T04:43:19Z

@viktoriias For Glide in testing, please have a look at this

oppia-android/app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt

Line 175 in 70c486f

 // Initialize Glide such that all of its executors use the same shared dispatcher pool as the 

Also need to check if ourDrawableMatcher can handle this update or not
reference might help,not sure - https://gist.github.com/frankiesardo/7490059

anandwana001 · 2021-01-11T04:47:33Z

StoryFragmentTest wasn't included in CI Lint and Tests / Robolectric Tests

It's included

oppia-android/.github/workflows/main.yml

Line 106 in 70c486f

name: Robolectric Tests - App Module (Non-Flaky)

viktoriias · 2021-01-11T04:52:26Z

StoryFragmentTest wasn't included in CI Lint and Tests / Robolectric Tests

It's included

oppia-android/.github/workflows/main.yml

Line 106 in 70c486f

name: Robolectric Tests - App Module (Non-Flaky)

Oh, you're right. Found it and it failed with the same error: android.renderscript.RSDriverException: Failed to create RS context.

anandwana001 · 2021-01-11T05:05:36Z

Can I remove two lessonThumbnailIsCorrect tests from RecentlyPlayedFragmentTest?

Both the tests are different. One is for port and one is for landscape.

fun testRecentlyPlayedTestActivity_changeConfiguration_recyclerViewItem1_lessonThumbnailIsCorrect()
fun testRecentlyPlayedTestActivity_recyclerViewItem1_lessonThumbnailIsCorrect()

viktoriias · 2021-01-25T05:24:52Z

Also, could you please add a screenshot in the PR Description showing test results for the modified test files in this PR? You can simply run the test files on both Espresso and Robolectric.

Done.

Also, could you please confirm by running any test file which does not ave TestImageLoaderModule is it passing on both Espresso and Robolectric or not, I am bit confused on using all the test modules in all the files or not.

Done, I included ProfilePictureActivityTest.

BenHenning

Thanks @viktoriias! This seems pretty close to being wrapped up. Just had a few nits.

Also, please note the failing Robolectric tests in CI.

utility/src/main/java/org/oppia/android/util/parser/GlideImageLoader.kt

utility/src/main/java/org/oppia/android/util/parser/TestGlideImageLoader.kt

app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt

…entTest.kt Co-authored-by: Ben Henning <[email protected]>

BenHenning

Thanks @viktoriias! Just nits left, then I think this will LGTM.

utility/src/main/java/org/oppia/android/util/parser/TestGlideImageLoader.kt

BenHenning

Thanks @viktoriias! This looks great!

BenHenning · 2021-01-30T01:27:36Z

Adding @rt4914 for codeowners.

anandwana001

LGTM, Thanks @viktoriias

rt4914

LGTM, thanks.

Lock and blur

6463741

viktoriias requested a review from rt4914 as a code owner January 7, 2021 05:02

viktoriias added 2 commits January 6, 2021 21:19

Replace color/white with #FFFFFF

6475828

Add \n

14af019

viktoriias assigned anandwana001 and BenHenning Jan 7, 2021

BenHenning reviewed Jan 8, 2021

View reviewed changes

BenHenning assigned viktoriias and unassigned BenHenning Jan 8, 2021

viktoriias added 4 commits January 8, 2021 13:45

Move isBlurred to LessonThumbnailImageView.

0ea6682

Remove unused import.

45e1eed

Move dependency on Glide away from from ImageLoader.

3b68b59

Remove unneeded line.

865c199

viktoriias assigned BenHenning and unassigned viktoriias Jan 9, 2021

BenHenning requested changes Jan 9, 2021

View reviewed changes

BenHenning assigned viktoriias and unassigned BenHenning Jan 9, 2021

viktoriias added 4 commits January 9, 2021 00:11

Refactor Transformation-related code, add KDocs.

3926ff3

Renaming.

ba7d34b

Remove unused import.

c31e867

Reformat.

e03c592

anandwana001 removed their assignment Jan 25, 2021

Nits and comments.

c45afb4

viktoriias requested a review from anandwana001 January 25, 2021 05:54

Revert changes in landscape design.

7daa0b3

viktoriias requested a review from BenHenning January 26, 2021 21:08

viktoriias assigned BenHenning Jan 27, 2021

Change BlurTransformation to return a copy instead of 'toTransform'.

8030726

BenHenning reviewed Jan 28, 2021

View reviewed changes

BenHenning removed their assignment Jan 28, 2021

viktoriias and others added 2 commits January 27, 2021 21:33

Update app/src/sharedTest/java/org/oppia/android/app/story/StoryFragm…

757c7c0

…entTest.kt Co-authored-by: Ben Henning <[email protected]>

Refactor TestGlideImageLoader.

11f55b0

viktoriias requested a review from BenHenning January 28, 2021 18:36

viktoriias assigned BenHenning and anandwana001 Jan 28, 2021

BenHenning reviewed Jan 29, 2021

View reviewed changes

BenHenning removed their assignment Jan 29, 2021

Nits

a400eb7

viktoriias requested a review from BenHenning January 29, 2021 03:45

viktoriias assigned BenHenning Jan 29, 2021

BenHenning approved these changes Jan 30, 2021

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Jan 30, 2021

viktoriias removed their assignment Jan 31, 2021

anandwana001 approved these changes Feb 1, 2021

View reviewed changes

anandwana001 removed their assignment Feb 1, 2021

rt4914 approved these changes Feb 3, 2021

View reviewed changes

rt4914 merged commit 2bcd511 into oppia:develop Feb 3, 2021

utzcoz mentioned this pull request Dec 23, 2021

Failed to create RenderScript Context robolectric/robolectric#1034

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2132: Chapter List Page Issues (#2232) - Blur and lock thumbnails of inactive cards. #2422

Fix #2132: Chapter List Page Issues (#2232) - Blur and lock thumbnails of inactive cards. #2422

viktoriias commented Jan 7, 2021 •

edited

Loading

viktoriias commented Jan 7, 2021

viktoriias commented Jan 7, 2021

BenHenning left a comment

viktoriias commented Jan 9, 2021

BenHenning left a comment •

edited

Loading

viktoriias commented Jan 10, 2021 •

edited

Loading

anandwana001 commented Jan 11, 2021 •

edited

Loading

anandwana001 commented Jan 11, 2021

viktoriias commented Jan 11, 2021

anandwana001 commented Jan 11, 2021

viktoriias commented Jan 25, 2021

BenHenning left a comment •

edited

Loading

BenHenning left a comment

BenHenning left a comment

BenHenning commented Jan 30, 2021

anandwana001 left a comment

rt4914 left a comment

Fix #2132: Chapter List Page Issues (#2232) - Blur and lock thumbnails of inactive cards. #2422

Fix #2132: Chapter List Page Issues (#2232) - Blur and lock thumbnails of inactive cards. #2422

Conversation

viktoriias commented Jan 7, 2021 • edited Loading

Explanation

Checklist

viktoriias commented Jan 7, 2021

viktoriias commented Jan 7, 2021

BenHenning left a comment

Choose a reason for hiding this comment

viktoriias commented Jan 9, 2021

BenHenning left a comment • edited Loading

Choose a reason for hiding this comment

viktoriias commented Jan 10, 2021 • edited Loading

anandwana001 commented Jan 11, 2021 • edited Loading

anandwana001 commented Jan 11, 2021

viktoriias commented Jan 11, 2021

anandwana001 commented Jan 11, 2021

viktoriias commented Jan 25, 2021

BenHenning left a comment • edited Loading

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Jan 30, 2021

anandwana001 left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

viktoriias commented Jan 7, 2021 •

edited

Loading

BenHenning left a comment •

edited

Loading

viktoriias commented Jan 10, 2021 •

edited

Loading

anandwana001 commented Jan 11, 2021 •

edited

Loading

BenHenning left a comment •

edited

Loading