Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1873: Fix OnboardingFragmentTest test cases on Robolectric #2482

Merged
merged 60 commits into from
Feb 3, 2021
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
df75643
fixed one test
prayutsu Oct 1, 2020
af2fb52
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Oct 4, 2020
a87d960
fix 4 remaining tests
prayutsu Oct 5, 2020
a23aea0
try fixing tests bu introducing scrollview
prayutsu Oct 8, 2020
b498b4d
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Oct 8, 2020
4d46313
tried adding scrollview
prayutsu Oct 8, 2020
be8405b
fix AndroidManifest.xml
prayutsu Oct 8, 2020
564d1bd
fix nit
prayutsu Oct 8, 2020
1e34fa2
fix basic set of tests
prayutsu Oct 9, 2020
7e2fbb6
fixed 3 failing tests
prayutsu Oct 10, 2020
eabd915
removed extra changes
prayutsu Oct 10, 2020
11a9b61
removed extra changes
prayutsu Oct 10, 2020
543d34c
try fixing tests with testCoroutineDispatchers
prayutsu Oct 13, 2020
da4f678
try advancedUntilIdle()
prayutsu Oct 24, 2020
6b7b059
tried migrating to viewpager2
prayutsu Oct 30, 2020
e028aa9
tried implementing new methods
prayutsu Nov 3, 2020
12540d3
Replace ViewPager with ViewPager2
prayutsu Nov 18, 2020
db59608
Add ignore annotations
prayutsu Nov 18, 2020
708a488
fix lint
prayutsu Nov 18, 2020
75eac88
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Nov 18, 2020
250530b
add dependencies to BUILD.bazel
prayutsu Nov 18, 2020
8929252
remove unnecessary ignore annotations
prayutsu Nov 18, 2020
176dec4
fix tests by using swipeLeft and swiperight
prayutsu Dec 2, 2020
8cc353f
fix lint
prayutsu Dec 3, 2020
6555129
fix lint
prayutsu Dec 3, 2020
811e772
fix lint
prayutsu Dec 3, 2020
5d92e0c
fix lint error
prayutsu Dec 3, 2020
f8ce120
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Dec 7, 2020
06a7a71
remove redundant lines
prayutsu Dec 9, 2020
4a39e40
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Jan 15, 2021
bf0b6b7
Fix Viewpager2 regressions
prayutsu Jan 15, 2021
3f5fff8
Fix lint errors.
prayutsu Jan 15, 2021
9a3d39c
Remove comments
prayutsu Jan 15, 2021
940137c
Fix all test cases on Robolectric
prayutsu Jan 15, 2021
2151c45
Try fixing flakiness in espresso
prayutsu Jan 15, 2021
709c7c8
Revert "Try fixing flakiness in espresso"
prayutsu Jan 15, 2021
6976d7d
Replace isDisplayed() with isCompletelyDisplayed()
prayutsu Jan 18, 2021
54a4659
Fix suggested changes
prayutsu Jan 20, 2021
9840c22
Try fixing test cases by using isADescendantOf()
prayutsu Jan 20, 2021
950401c
Revert changes
prayutsu Jan 20, 2021
2db7622
Try stabilizing ttion est cases by changing configuration
prayutsu Jan 21, 2021
5a8a8cd
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Jan 21, 2021
3b8e1fb
Revert last commit to remove unwanted configuration changes
prayutsu Jan 26, 2021
7f02b77
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Jan 26, 2021
8903b00
Fix ktlint error
prayutsu Jan 26, 2021
4c33d03
Resolve merge conflicts
prayutsu Jan 28, 2021
f9cd0e5
Try adding custom ViewMatcher
prayutsu Jan 29, 2021
5413965
Add custom ViewMatcher
prayutsu Jan 29, 2021
4f2fff8
Add custom ViewAction for scrolling ViewPager pages
prayutsu Jan 29, 2021
c1786e5
Disable smooth scrolling to fix test cases
prayutsu Jan 29, 2021
dd21e64
Remove holder.setIsRecycleable(false) and optimise perform()
prayutsu Jan 29, 2021
83f14db
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Jan 29, 2021
b7a671f
Remove unnecessary id of constraintLayout in onboarding_slide.xml
prayutsu Jan 29, 2021
ca84aea
Replace advancedUntilIdle bu runCurrent wherever applicable
prayutsu Jan 29, 2021
3555545
Remove unnecessary delay
prayutsu Jan 29, 2021
98a4db2
Add argument comment for smoothScroll
prayutsu Jan 30, 2021
26fb177
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Feb 2, 2021
3023508
Add a space after argument comment
prayutsu Feb 2, 2021
33a48da
Fix suggested changes.
prayutsu Feb 3, 2021
1ee0cf5
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
prayutsu Feb 3, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,14 @@ import android.view.ViewGroup
import android.widget.ImageView
import android.widget.LinearLayout
import androidx.appcompat.app.AppCompatActivity
import androidx.core.text.TextUtilsCompat
import androidx.core.view.ViewCompat
import androidx.fragment.app.Fragment
import androidx.viewpager.widget.ViewPager
import androidx.viewpager2.widget.ViewPager2
import org.oppia.android.R
import org.oppia.android.app.fragment.FragmentScope
import org.oppia.android.app.viewmodel.ViewModelProvider
import org.oppia.android.databinding.OnboardingFragmentBinding
import org.oppia.android.util.statusbar.StatusBarColor
import java.util.Locale
import javax.inject.Inject
import kotlin.collections.ArrayList

/** The presenter for [OnboardingFragment]. */
@FragmentScope
Expand All @@ -28,7 +24,6 @@ class OnboardingFragmentPresenter @Inject constructor(
private val viewModelProviderFinalSlide: ViewModelProvider<OnboardingSlideFinalViewModel>
) : OnboardingNavigationListener {
private val dotsList = ArrayList<ImageView>()
private lateinit var onboardingPagerAdapter: OnboardingPagerAdapter
private lateinit var binding: OnboardingFragmentBinding

fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? {
Expand All @@ -50,18 +45,11 @@ class OnboardingFragmentPresenter @Inject constructor(
}

private fun setUpViewPager() {
onboardingPagerAdapter =
val adapter =
OnboardingPagerAdapter(fragment.requireContext(), getOnboardingSlideFinalViewModel())
binding.onboardingSlideViewPager.adapter = onboardingPagerAdapter
// TODO (#2194 ): Remove this once the implementation is updated to ViewPager2
val isRTL = TextUtilsCompat
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it working correctly for RTL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there's some problem with the back button -

Screenrecorder-2021-01-18-14-26-15-85.mp4

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FareesHussain mentioning you here to help with RTL stuff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

video looks fine
@prayutsu have a look at the todos in the OnboardingFragmentPresenter and OnboardingPagerAdapter and revert them
and then check if RTL works fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked it on the develop branch, and it looks ditto same as in this video.
Is it correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

.getLayoutDirectionFromLocale(Locale.getDefault()) == ViewCompat.LAYOUT_DIRECTION_RTL
if (isRTL) {
binding.onboardingSlideViewPager.rotationY = 180f
}
binding.onboardingSlideViewPager.addOnPageChangeListener(
object :
ViewPager.OnPageChangeListener {
binding.onboardingSlideViewPager.adapter = adapter
binding.onboardingSlideViewPager.registerOnPageChangeCallback(
object : ViewPager2.OnPageChangeCallback() {
override fun onPageScrollStateChanged(state: Int) {
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,65 +2,81 @@ package org.oppia.android.app.onboarding

import android.content.Context
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import android.widget.ScrollView
import androidx.core.text.TextUtilsCompat
import androidx.core.view.ViewCompat
import androidx.viewpager.widget.PagerAdapter
import androidx.recyclerview.widget.RecyclerView
import org.oppia.android.databinding.OnboardingSlideBinding
import org.oppia.android.databinding.OnboardingSlideFinalBinding
import java.util.Locale

/** Adapter to control the slide details in onboarding flow. */
private const val VIEW_TYPE_TITLE_TEXT = 1
private const val VIEW_TYPE_STORY_ITEM = 2

class OnboardingPagerAdapter(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to use the Bindale Adapter and remove this Adapter in this PR only or we are keeping this as a separate issue? @rt4914

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a larger & unrelated change--I suggest splitting it off of this task.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, given that so much of the adapter has changed anyway, maybe it is worth trying to migrate to BindableAdapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenHenning So, should I replace it with a bindable Adapter in this PR only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I guess you are suggesting to track it via a new issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the case that @prayutsu has already worked a lot on this issue, let him decide what he wants to do:
(a.) File an issue, mention it this PR and later on either @prayutsu solves it or the community.
(b.) Use BindableAdapter in this PR itself.

val context: Context,
val onboardingSlideFinalViewModel: OnboardingSlideFinalViewModel
) : PagerAdapter() {
// TODO (#2194 ): Remove this once the implementation is updated to ViewPager2
val isRTL = TextUtilsCompat
.getLayoutDirectionFromLocale(Locale.getDefault()) == ViewCompat.LAYOUT_DIRECTION_RTL
override fun instantiateItem(container: ViewGroup, position: Int): Any {
if (position == TOTAL_NUMBER_OF_SLIDES - 1) {
val binding =
OnboardingSlideFinalBinding.inflate(
) :
RecyclerView.Adapter<RecyclerView.ViewHolder>() {

override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): RecyclerView.ViewHolder {
return when (viewType) {
VIEW_TYPE_TITLE_TEXT -> {
val binding = OnboardingSlideBinding.inflate(
LayoutInflater.from(context),
container,
parent,
false
)
binding.viewModel = onboardingSlideFinalViewModel
if (isRTL) {
binding.finalLayout!!.rotationY = 180f
OnboardingSlideViewHolder(binding)
}

container.addView(binding.root)
return binding.root
VIEW_TYPE_STORY_ITEM -> {
val binding =
OnboardingSlideFinalBinding.inflate(
LayoutInflater.from(context),
parent,
false
)
OnboardingSlideFinalViewHolder(binding)
}
else -> throw IllegalArgumentException("Invalid view type: $viewType")
}
}

val binding = OnboardingSlideBinding.inflate(
LayoutInflater.from(context),
container,
false
)
if (isRTL) {
binding.root.rotationY = 180f
override fun getItemViewType(position: Int): Int {
return if (position == TOTAL_NUMBER_OF_SLIDES - 1) {
VIEW_TYPE_STORY_ITEM
} else {
VIEW_TYPE_TITLE_TEXT
}
val onboardingSlideViewModel =
OnboardingSlideViewModel(context, ViewPagerSlide.getSlideForPosition(position))
binding.viewModel = onboardingSlideViewModel
container.addView(binding.root)
return binding.root
}

override fun getCount(): Int {
override fun getItemCount(): Int {
return TOTAL_NUMBER_OF_SLIDES
}

override fun isViewFromObject(view: View, `object`: Any): Boolean {
return view == `object`
override fun onBindViewHolder(holder: RecyclerView.ViewHolder, position: Int) {
when (holder.itemViewType) {
VIEW_TYPE_TITLE_TEXT -> {
val onboardingSlideViewModel =
OnboardingSlideViewModel(context, ViewPagerSlide.getSlideForPosition(position))
(holder as OnboardingSlideViewHolder).bind(onboardingSlideViewModel)
}
VIEW_TYPE_STORY_ITEM -> {
(holder as OnboardingSlideFinalViewHolder).bind(onboardingSlideFinalViewModel)
}
}
}

inner class OnboardingSlideViewHolder(
private val binding: OnboardingSlideBinding
) : RecyclerView.ViewHolder(binding.root) {
fun bind(onboardingSlideViewModel: OnboardingSlideViewModel) {
binding.viewModel = onboardingSlideViewModel
}
}

override fun destroyItem(container: ViewGroup, position: Int, `object`: Any) {
container.removeView(`object` as ScrollView)
inner class OnboardingSlideFinalViewHolder(
private val binding: OnboardingSlideFinalBinding
) : RecyclerView.ViewHolder(binding.root) {
fun bind(onboardingSlideFinalViewModel: OnboardingSlideFinalViewModel) {
binding.viewModel = onboardingSlideFinalViewModel
}
}
}
4 changes: 2 additions & 2 deletions app/src/main/res/layout-land/onboarding_fragment.xml
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@
android:layout_height="match_parent"
android:background="@color/oppiaBackgroundYellowIvory">

<androidx.viewpager.widget.ViewPager
<androidx.viewpager2.widget.ViewPager2
android:id="@+id/onboarding_slide_view_pager"
android:layout_width="match_parent"
android:layout_height="0dp"
android:layout_height="match_parent"
android:layout_marginBottom="0dp"
android:overScrollMode="never"
android:scrollbars="none"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@
android:layout_height="match_parent"
android:background="@color/oppiaBackgroundYellowIvory">

<androidx.viewpager.widget.ViewPager
<androidx.viewpager2.widget.ViewPager2
android:id="@+id/onboarding_slide_view_pager"
android:layout_width="match_parent"
android:layout_height="0dp"
android:layout_height="match_parent"
android:overScrollMode="never"
android:scrollbars="none"
app:layout_constraintBottom_toTopOf="@id/get_started_container"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
android:layout_height="match_parent"
android:background="@color/oppiaBackgroundYellowIvory">

<androidx.viewpager.widget.ViewPager
<androidx.viewpager2.widget.ViewPager2
android:id="@+id/onboarding_slide_view_pager"
android:layout_width="match_parent"
android:layout_height="0dp"
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/res/layout/onboarding_fragment.xml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
android:layout_height="match_parent"
android:background="@color/oppiaBackgroundYellowIvory">

<androidx.viewpager.widget.ViewPager
<androidx.viewpager2.widget.ViewPager2
android:id="@+id/onboarding_slide_view_pager"
android:layout_width="match_parent"
android:layout_height="0dp"
Expand Down
Loading