Fix part of #134: Home fragment low fi with TopicList #289

rt4914 · 2019-10-31T16:25:22Z

Explanation

This PR includes implementation of HomeFragment and next PR on this will include ContinuePlayingActivity

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 assigned to an appropriate reviewer.

…w-fi

BenHenning

Thanks @rt4914! This generally LGTM (& really nice tests for the topic list). Feel free to re-assign to me if you want me to do one more pass, but once all my comments are resolved I'm completely fine with this being merged.

app/src/main/java/org/oppia/app/home/RouteToTopicPlayStoryListener.kt

app/src/main/java/org/oppia/app/home/HomeFragmentPresenter.kt

app/src/main/java/org/oppia/app/home/topiclist/PromotedStoryViewModel.kt

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

app/src/sharedTest/java/org/oppia/app/home/ContinuePlayingActivityTest.kt

app/src/sharedTest/java/org/oppia/app/home/HomeActivityTest.kt

nikitamarysolomanpvt

PTAL

step to follow while testing

Turn on the auto-rotate in the device
Keep your device in LANDSCAPE mode.
Now start playing test cases.

Issue
Seven test cases failed
Expected result
All test cases should be successful

It seems the configuration changes in test cases are not working.
PTAL on other Nit changes.

nikitamarysolomanpvt · 2019-11-04T11:40:35Z

app/src/sharedTest/java/org/oppia/app/home/HomeActivityTest.kt

+
+ @Test
+ fun testHomeActivity_recyclerViewIndex0_configurationChange_displaysWelcomeMessageCorrectly() {
+ launch(HomeActivity::class.java).use {


This code seems not working. Even while debugging not changing (not showing the change in configuration)the configuration.

Introduced new file OrientationChangeAction which helps to work with orientationchange along with visibility.
onView(isRoot()).perform(orientationLandscape())

@BenHenning Please check this new implementation for orientation change.

Is this just for convenience? I'm trying to understand why it's needed versus scenario.onActivity { activity -> activity.requestedOrientation = ... }

@BenHenning When we use
scenario.onActivity { activity -> activity.requestedOrientation = ... }
the test cases pass but the orientation change is not visible on screen.
The OrientationChangeAction file makes sure that the orientation change is actually visible on device when we run these test cases.

app/src/sharedTest/java/org/oppia/app/home/HomeActivityTest.kt

nikitamarysolomanpvt · 2019-11-04T11:57:43Z

app/src/main/java/org/oppia/app/databinding/DrawableBindingAdapters.kt

+fun setBackgroundDrawable(view: View, @ColorInt colorRgb: Int) {
+ view.setBackgroundResource(R.drawable.rounded_rect_background)
+ // The input color needs to have alpha channel prepended to it.
+ (view.background as GradientDrawable).setColor((0xff000000 or colorRgb.toLong()).toInt())


Is't supposed to be added in resource file.

@BenHenning Can you address this given the case that I have used this from your code.

What specifically should be added to a resource file? There's nothing here that's really shareable. The 0xff000000 isn't a color, it's an alpha filter so it wouldn't belong in a resource file. Is there another aspect of this that you think should be in a resource file?

I mentioned about 0xff000000 only.
We may add color with alpha filter in resource files is't?.(Just to clarify)
Just for example like the below code.
https://android.googlesource.com/platform/frameworks/base/+/master/core/res/res/values/colors.xml

rt4914 · 2019-11-04T12:54:51Z

PTAL

step to follow while testing

Turn on the auto-rotate in the device

Keep your device in LANDSCAPE mode.

Now start playing test cases.

Issue
Seven test cases failed

Expected result
All test cases should be successful

It seems the configuration changes in test cases are not working.

PTAL on other Nit changes.

All tests were written assuming that device will be in portrait mode. Thanks for pointing this out.

Added onView(withId(R.id.home_recycler_view)).perform(scrollToPosition<RecyclerView.ViewHolder>(3)) this and similar index based scrolling to resolve this issue.

BenHenning · 2019-11-05T02:20:22Z

app/src/sharedTest/java/org/oppia/app/utility/OrientationChangeAction.kt

+class OrientationChangeAction(private val orientation: Int) : ViewAction {
+
+ companion object {
+ fun orientationLandscape(): ViewAction = OrientationChangeAction(ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE)


Let's make these file-level functions instead of companion functions. Then we can make this action private to the file to make things a bit cleaner.

I am actually in favour of keeping this implementation because this makes the usage of these functions quite easy in test cases.

sample:

onView(isRoot()).perform(orientationLandscape())

app/src/sharedTest/java/org/oppia/app/utility/OrientationChangeAction.kt

BenHenning · 2019-11-05T02:22:01Z

app/src/sharedTest/java/org/oppia/app/utility/OrientationChangeAction.kt

+ val c = view.childCount
+ var i = 0
+ while (i < c && activity == null) {
+ activity = getActivity(view.getChildAt(i).context)


Why can't we just use the ViewGroup's context? This extra iteration is a bit confusing.

I did try to use the viewgroup's context but that was leading to NullPointerException error and therefore keeping it as it is for now.

BenHenning · 2019-11-05T02:25:06Z

@rt4914 I did not do another pass on this. Let me know if you'd like me to, or want to discuss the specific aspects that you mentioned me in above.

app/src/main/res/layout/home_fragment.xml

nikitamarysolomanpvt

LGTM .Please fix the comment mentioned

…w-fi

rt4914 · 2019-11-05T07:52:05Z

Merging this PR for now, there are some open comments which will be discussed once low-fi is finished but those comments are non-blocking.

Rajat Talesra added 14 commits October 30, 2019 12:21

HomeFragment xml introduction

e322c28

Promoted Story ViewModel

2bd1339

PromotedStoryViewModel

eaef41d

Basic promoted story

68d9ae5

Promoted story data display

f1312bd

Promoted card display

f1c577d

GridLayoutManager

fd9e5d8

Functional HomeFragment

5cc1baf

Click listeners

f591ae2

Click listeners final

e4c3bb0

Merge remote-tracking branch 'upstream/develop' into home-fragment-lo…

fe9ba4b

…w-fi

Chapter name issue fixed

973b8f2

Fully functional with click listeners

2ad28fd

Merge remote-tracking branch 'upstream/develop' into home-fragment-lo…

9e146aa

…w-fi

rt4914 changed the title ~~Fix #134: Home fragment low fi with TopicList~~ Fix part of #134: Home fragment low fi with TopicList Oct 31, 2019

Rajat Talesra added 5 commits November 1, 2019 01:18

Introduce ContinuePlayingActivity

78e5f0c

Click listener integration

54403f5

Test for ContinuePlayingActivity

4cb6061

HomeTest cases

c2bf320

Nit changes

04fdc35

rt4914 requested review from BenHenning, veena14cs, jamesxu0, nikitamarysolomanpvt and vinitamurthi October 31, 2019 21:00

rt4914 assigned vinitamurthi, BenHenning, veena14cs, jamesxu0 and nikitamarysolomanpvt Oct 31, 2019

veena14cs assigned rt4914 and unassigned veena14cs Nov 4, 2019

BenHenning approved these changes Nov 4, 2019

View reviewed changes

BenHenning removed their assignment Nov 4, 2019

Rajat Talesra added 4 commits November 4, 2019 16:44

Nit suggested changes

db8c117

Code updated as per suggestions

d9af742

Added HomeItemViewModel

f40393a

Nit changes

0795666

nikitamarysolomanpvt suggested changes Nov 4, 2019

View reviewed changes

rt4914 assigned BenHenning and unassigned rt4914 Nov 4, 2019

Rajat Talesra added 2 commits November 4, 2019 18:51

Orientation Change

7933c5a

EOF

dcbbc9e

BenHenning reviewed Nov 5, 2019

View reviewed changes

app/src/sharedTest/java/org/oppia/app/utility/OrientationChangeAction.kt Show resolved Hide resolved

BenHenning reviewed Nov 5, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Nov 5, 2019

Merge conflicts

1a63bd0

nikitamarysolomanpvt reviewed Nov 5, 2019

View reviewed changes

app/src/main/res/layout/home_fragment.xml Show resolved Hide resolved

nikitamarysolomanpvt approved these changes Nov 5, 2019

View reviewed changes

nikitamarysolomanpvt removed their assignment Nov 5, 2019

Rajat Talesra added 2 commits November 5, 2019 13:06

Text fix

d5f4366

Merge remote-tracking branch 'upstream/develop' into home-fragment-lo…

8396b9c

…w-fi

rt4914 merged commit 8bfbbec into develop Nov 5, 2019

rt4914 deleted the home-fragment-low-fi branch November 5, 2019 07:52

rt4914 mentioned this pull request Nov 6, 2019

Fix #134: HomeFragment- Continue playing Low-fi #304

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #134: Home fragment low fi with TopicList #289

Fix part of #134: Home fragment low fi with TopicList #289

rt4914 commented Oct 31, 2019 •

edited

Loading

BenHenning left a comment •

edited

Loading

nikitamarysolomanpvt left a comment •

edited

Loading

nikitamarysolomanpvt Nov 4, 2019

rt4914 Nov 4, 2019

BenHenning Nov 5, 2019

rt4914 Nov 5, 2019

nikitamarysolomanpvt Nov 4, 2019

rt4914 Nov 4, 2019

BenHenning Nov 5, 2019

nikitamarysolomanpvt Nov 5, 2019

rt4914 commented Nov 4, 2019

BenHenning Nov 5, 2019

rt4914 Nov 5, 2019

BenHenning Nov 5, 2019

rt4914 Nov 5, 2019 •

edited

Loading

BenHenning commented Nov 5, 2019

nikitamarysolomanpvt left a comment

rt4914 commented Nov 5, 2019

Fix part of #134: Home fragment low fi with TopicList #289

Fix part of #134: Home fragment low fi with TopicList #289

Conversation

rt4914 commented Oct 31, 2019 • edited Loading

Explanation

Checklist

BenHenning left a comment • edited Loading

Choose a reason for hiding this comment

nikitamarysolomanpvt left a comment • edited Loading

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

rt4914 commented Nov 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rt4914 Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

BenHenning commented Nov 5, 2019

nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

rt4914 commented Nov 5, 2019

rt4914 commented Oct 31, 2019 •

edited

Loading

BenHenning left a comment •

edited

Loading

nikitamarysolomanpvt left a comment •

edited

Loading

rt4914 Nov 5, 2019 •

edited

Loading