-
Notifications
You must be signed in to change notification settings - Fork 503
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 part of #140: Topic play animation #486
Changes from 1 commit
ddc950a
9d560a2
2632149
ae1fb63
16c18aa
faf8229
f8b35e6
f288b1b
bff5d30
0a856e0
6b7cf74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,17 @@ | ||
package org.oppia.app.topic.play | ||
|
||
import android.content.Context | ||
import android.util.Log | ||
import android.view.LayoutInflater | ||
import android.view.View | ||
import android.view.ViewGroup | ||
import android.view.animation.Animation | ||
import android.view.animation.AnimationUtils | ||
import android.view.animation.Transformation | ||
import android.widget.LinearLayout | ||
import androidx.core.view.isVisible | ||
import androidx.recyclerview.widget.RecyclerView | ||
import org.oppia.app.R | ||
import org.oppia.app.databinding.TopicPlayStorySummaryBinding | ||
import org.oppia.app.databinding.TopicPlayTitleBinding | ||
import org.oppia.app.model.ChapterPlayState | ||
|
@@ -21,6 +25,7 @@ private const val ANIMATION_DURATION: Long = 400 | |
|
||
/** Adapter to bind StorySummary to [RecyclerView] inside [TopicPlayFragment]. */ | ||
class StorySummaryAdapter( | ||
private val context: Context, | ||
private val itemList: MutableList<TopicPlayItemViewModel>, | ||
private val chapterSummarySelector: ChapterSummarySelector, | ||
private val expandedChapterListIndexListener: ExpandedChapterListIndexListener, | ||
|
@@ -92,19 +97,6 @@ class StorySummaryAdapter( | |
inner class StorySummaryViewHolder(private val binding: TopicPlayStorySummaryBinding) : | ||
RecyclerView.ViewHolder(binding.root) { | ||
internal fun bind(storySummaryViewModel: StorySummaryViewModel, position: Int) { | ||
if (currentExpandedChapterListIndex != null && currentExpandedChapterListIndex == position) { | ||
binding.isListExpanded = true | ||
|
||
if (currentExpandedChapterListIndex == position) { | ||
binding.chapterListDropDownIcon.animate().rotation(180F).setDuration(400).start() | ||
expand(binding.chapterListContainer) | ||
} else { | ||
binding.chapterListDropDownIcon.animate().rotation(0F).setDuration(400).start() | ||
collapse(binding.chapterListContainer) | ||
} | ||
} else { | ||
binding.isListExpanded = false | ||
} | ||
binding.viewModel = storySummaryViewModel | ||
|
||
val chapterSummaries = storySummaryViewModel.storySummary.chapterList | ||
|
@@ -124,27 +116,35 @@ class StorySummaryAdapter( | |
val chapterList = storySummaryViewModel.storySummary.chapterList | ||
binding.chapterRecyclerView.adapter = ChapterSummaryAdapter(chapterList, chapterSummarySelector) | ||
|
||
if (currentExpandedChapterListIndex != null && currentExpandedChapterListIndex == position) { | ||
val aniRotate = AnimationUtils.loadAnimation(context, R.anim.rotation_clockwise_180) | ||
binding.chapterListDropDownIcon.startAnimation(aniRotate) | ||
expand(binding.chapterListContainer) | ||
} else { | ||
val aniRotate = AnimationUtils.loadAnimation(context, R.anim.rotation_anti_clockwise_180) | ||
binding.chapterListDropDownIcon.startAnimation(aniRotate) | ||
collapse(binding.chapterListContainer) | ||
} | ||
|
||
binding.root.setOnClickListener { | ||
binding.isListExpanded = !binding.chapterListContainer.isVisible | ||
val previousItem = currentExpandedChapterListIndex | ||
|
||
if (!binding.chapterListContainer.isVisible) { | ||
binding.chapterListDropDownIcon.animate().rotation(180F).setDuration(400).start() | ||
expand(binding.chapterListContainer) | ||
} else { | ||
binding.chapterListDropDownIcon.animate().rotation(0F).setDuration(400).start() | ||
collapse(binding.chapterListContainer) | ||
} | ||
|
||
if (binding.chapterListContainer.isVisible) { | ||
currentExpandedChapterListIndex = position | ||
} else { | ||
currentExpandedChapterListIndex = null | ||
} else { | ||
currentExpandedChapterListIndex = position | ||
} | ||
expandedChapterListIndexListener.onExpandListIconClicked(currentExpandedChapterListIndex) | ||
if (previousItem != null && previousItem != position) { | ||
notifyItemChanged(previousItem) | ||
} | ||
notifyItemChanged(position) | ||
} | ||
} | ||
|
||
private fun expand(chapterListContainer: View) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit confused by this. Why can't we rely on RecyclerView's own item animator for this? These animations seem a bit complex, and should really be tested directly since if they regress it will be hard for us to detect. I'd rather we rely on existing animation systems rather than creating our own Animation classes, if possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am actually not able to understand how can we do this using RecyclerView's animator?
I don't have much experience of automated animation, can you just give information regarding RecyclerView's item animator in more detail? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. I was thinking specifically DefaultItemAnimator, but I'm guessing that might not do the right thing. In which case, maybe something like https://stackoverflow.com/a/37453839 or another animator library might make this a bit easier if it's possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This animation is applicable on recyclerview item. But here we want to expand and contract a linear layout, which is again within a recyclerview item. |
||
Log.d("TAG", "expand") | ||
chapterListContainer.clearAnimation() | ||
val matchParentMeasureSpec = | ||
View.MeasureSpec.makeMeasureSpec((chapterListContainer.parent as View).width, View.MeasureSpec.EXACTLY) | ||
|
@@ -174,7 +174,9 @@ class StorySummaryAdapter( | |
} | ||
|
||
private fun collapse(chapterListContainer: View) { | ||
Log.d("TAG", "collapse") | ||
chapterListContainer.clearAnimation() | ||
|
||
val initialHeight = chapterListContainer.measuredHeight | ||
|
||
val collapseAnimation = object : Animation() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<set xmlns:android="http:https://schemas.android.com/apk/res/android" android:interpolator="@android:anim/cycle_interpolator"> | ||
<rotate android:fromDegrees="180" | ||
android:toDegrees="0" | ||
android:pivotX="50%" | ||
android:pivotY="50%" | ||
android:duration="400" /> | ||
</set> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<set xmlns:android="http:https://schemas.android.com/apk/res/android" android:interpolator="@android:anim/cycle_interpolator"> | ||
<rotate android:fromDegrees="0" | ||
android:toDegrees="180" | ||
android:pivotX="50%" | ||
android:pivotY="50%" | ||
android:duration="400" /> | ||
</set> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic isn't trivial to follow. Can we add tests to make sure that the expand/collapse works as expected & stays working? We shouldn't test the animations, just that the state changes are being reflected correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated all test cases:
Check the following tests which checks this functionality:
testTopicPlayFragment_loadFragmentWithTopicTestId0_clickExpandListIconIndex0_clickExpandListIconIndex1_chapterListForIndex0IsNotDisplayed
testTopicPlayFragment_loadFragmentWithTopicTestId0_clickExpandListIconIndex1_clickExpandListIconIndex0_chapterListForIndex0IsNotDisplayed