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.

Sign up for GitHub

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 #1845: Replace Language code with icon in AudioBar #2499

Merged
merged 8 commits into from
Jan 30, 2021
Merged

Fix #1845: Replace Language code with icon in AudioBar #2499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
de7bb1e
replace icon
MaskedCarrot Jan 17, 2021
1a91a36
removed comment
MaskedCarrot Jan 17, 2021
9db0a2b
removed wild card import
MaskedCarrot Jan 17, 2021
6513ca4
fixed ids of audio_fragment
MaskedCarrot Jan 26, 2021
240c347
fixed failing tests
MaskedCarrot Jan 26, 2021
f9e7ae0
lint fix
MaskedCarrot Jan 26, 2021
572d0b4
fixed failing build
MaskedCarrot Jan 27, 2021
09ca138
added TODO for ignored robolectric test
MaskedCarrot Jan 27, 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
29 changes: 15 additions & 14 deletions app/src/main/java/org/oppia/android/app/player/audio/AudioFragmentPresenter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,27 +78,28 @@ class AudioFragmentPresenter @Inject constructor(
)

val binding = AudioFragmentBinding.inflate(inflater, container, /* attachToRoot= */ false)
binding.sbAudioProgress.setOnSeekBarChangeListener(object : SeekBar.OnSeekBarChangeListener {
override fun onProgressChanged(seekBar: SeekBar?, progress: Int, fromUser: Boolean) {
if (fromUser) {
userProgress = progress
binding.audioProgressSeekBar.setOnSeekBarChangeListener(
object : SeekBar.OnSeekBarChangeListener {
override fun onProgressChanged(seekBar: SeekBar?, progress: Int, fromUser: Boolean) {
if (fromUser) {
userProgress = progress
}
}
}

override fun onStartTrackingTouch(seekBar: SeekBar?) {
userIsSeeking = true
}
override fun onStartTrackingTouch(seekBar: SeekBar?) {
userIsSeeking = true
}

override fun onStopTrackingTouch(seekBar: SeekBar?) {
viewModel.handleSeekTo(userProgress)
userIsSeeking = false
}
})
override fun onStopTrackingTouch(seekBar: SeekBar?) {
viewModel.handleSeekTo(userProgress)
userIsSeeking = false
}
})
viewModel.playStatusLiveData.observe(
fragment,
Observer {
prepared = it != UiAudioPlayStatus.LOADING && it != UiAudioPlayStatus.FAILED
binding.sbAudioProgress.isEnabled = prepared
binding.audioProgressSeekBar.isEnabled = prepared
}
)

Expand Down
12 changes: 12 additions & 0 deletions app/src/main/res/drawable/ic_audio_lang_24px.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<vector xmlns:android="http://schemas.android.com/apk/res/android"
android:width="24dp"
android:height="24dp"
android:viewportWidth="24"
android:viewportHeight="24">
<path
android:fillColor="#fff"
android:pathData="M21.04,5.93C19.04,1.92 15.45,0.21 11.38,0.05A12.21,12.21 0,0 0,5.04 1.63,9.77 9.77,0 0,0 0.04,9.45a8.84,8.84 0,0 0,2 6.44,1.75 1.75,0 0,1 0.3,2.06c-0.47,0.89 -1,1.74 -1.56,2.59 -0.21,0.33 -0.41,0.64 -0.21,1a0.9,0.9 0,0 0,1 0.51,12.16 12.16,0 0,0 2.53,-0.7c1,-0.44 1.87,-1.09 2.81,-1.62a1.06,1.06 0,0 1,0.72 -0.09,11.52 11.52,0 0,0 9.43,-1.17A9.5,9.5 0,0 0,21.04 5.93ZM9.25,12.05a0.62,0.62 0,0 1,-0.51 -0.25,6.73 6.73,0 0,1 -0.43,-1.19c-0.11,-0.39 -0.29,-0.6 -0.74,-0.54a3.43,3.43 0,0 1,-1.06 0c-0.59,-0.1 -0.83,0.19 -1,0.7 -0.07,0.29 -0.2,0.56 -0.28,0.85s-0.29,0.56 -0.65,0.43 -0.34,-0.44 -0.23,-0.76c0.65,-1.87 1.29,-3.74 2,-5.6 0.12,-0.33 0.1,-0.83 0.59,-0.84s0.53,0.5 0.65,0.84c0.64,1.84 1.28,3.67 1.91,5.51 0.05,0.13 0.08,0.27 0.16,0.5C9.53,11.79 9.4,12.05 9.25,12.05ZM17.5,10.6a0.54,0.54 0,0 0,-0.4 0.23,7.94 7.94,0 0,1 -1.86,3.08l1.6,1c0.28,0.16 0.57,0.32 0.38,0.69s-0.51,0.33 -0.82,0.15c-0.62,-0.37 -1.23,-0.76 -1.8,-1.11l-2.31,1.17c-0.28,0.14 -0.61,0.2 -0.77,-0.18s0.1,-0.55 0.43,-0.7c0.65,-0.29 1.29,-0.63 1.92,-0.94l-1.2,-1.57c-0.2,-0.28 -0.26,-0.59 0.07,-0.78s0.54,0 0.74,0.28c0.32,0.47 0.68,0.9 1.06,1.4a6.39,6.39 0,0 0,1.66 -2.7L12.04,10.62c-0.32,0 -0.56,-0.11 -0.56,-0.48s0.24,-0.48 0.57,-0.48h2.32a3.16,3.16 0,0 1,0 -0.88c0.06,-0.21 0.27,-0.5 0.45,-0.53s0.49,0.2 0.49,0.54 0,0.53 0,0.87L17.44,9.66c0.36,0 0.77,0 0.78,0.48S17.86,10.56 17.5,10.58Z" />
<path
android:fillColor="#fff"
android:pathData="M6.14,9.11L7.79,9.11L6.96,6.73C6.65,7.61 6.4,8.35 6.14,9.11Z" />
</vector>
25 changes: 13 additions & 12 deletions app/src/main/res/layout/audio_fragment.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@
xmlns:app="http://schemas.android.com/apk/res-auto">

<data>

<import type="org.oppia.android.app.player.audio.AudioViewModel.UiAudioPlayStatus" />

<import type="android.view.View" />

<variable
name="audioFragment"
type="org.oppia.android.app.player.audio.LanguageInterface" />

<variable
name="viewModel"
type="org.oppia.android.app.player.audio.AudioViewModel" />
Expand All @@ -25,7 +29,7 @@
android:padding="4dp">

<ImageView
android:id="@+id/ivPlayPauseAudio"
android:id="@+id/play_pause_audio_icon"
android:layout_width="48dp"
android:layout_height="48dp"
android:layout_marginStart="8dp"
Expand All @@ -39,10 +43,11 @@
app:layout_constraintTop_toTopOf="parent" />

<SeekBar
android:id="@+id/sbAudioProgress"
android:id="@+id/audio_progress_seek_bar"
style="@style/AudioSeekBar"
android:layout_width="0dp"
android:layout_height="26dp"
android:layout_marginStart="16dp"
android:max="@{viewModel.durationLiveData}"
android:maxHeight="4dp"
android:minWidth="180dp"
Expand All @@ -51,18 +56,17 @@
android:progressDrawable="@drawable/audio_seekbar_background"
android:splitTrack="false"
android:thumb="@drawable/seekbar_thumb"
android:layout_marginStart="16dp"
android:thumbOffset="16dp"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintEnd_toStartOf="@+id/tvAudioLanguage"
app:layout_constraintStart_toEndOf="@+id/ivPlayPauseAudio"
app:layout_constraintEnd_toStartOf="@+id/audio_language_icon"
app:layout_constraintStart_toEndOf="@+id/play_pause_audio_icon"
app:layout_constraintTop_toTopOf="parent" />

<TextView
android:id="@+id/tvAudioLanguage"
<ImageView
android:id="@+id/audio_language_icon"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:fontFamily="sans-serif-medium"
android:contentDescription="@{viewModel.currentLanguageCode}"
android:gravity="center"
android:minWidth="48dp"
android:minHeight="48dp"
Expand All @@ -71,10 +75,7 @@
android:paddingTop="16dp"
android:paddingEnd="16dp"
android:paddingBottom="16dp"
android:text="@{viewModel.currentLanguageCode}"
android:textAllCaps="true"
android:textColor="@color/white"
android:textSize="14sp"
android:src="@drawable/ic_audio_lang_24px"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintTop_toTopOf="parent" />
Expand Down
43 changes: 21 additions & 22 deletions app/src/sharedTest/java/org/oppia/android/app/player/audio/AudioFragmentTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@ class AudioFragmentTest {
)
}

// TODO(#1845): As updating tvAudioLanguage to image, we need to remove this test
@Test
fun testAudioFragment_withDefaultProfile_showsAudioLanguageAsEnglish() {
addMediaInfo()
Expand All @@ -160,11 +159,12 @@ class AudioFragmentTest {
internalProfileId
)
).use {
onView(withId(R.id.tvAudioLanguage)).check(matches(withText("EN")))
onView(withId(R.id.audio_language_icon)).check(matches(withContentDescription("en")))
}
}

// TODO(#1845): As updating tvAudioLanguage to image, we need to remove this test
// TODO(#2417): Re-enable once this test passes on robolectric
@RunOn(TestPlatform.ESPRESSO)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anandwana001 PTAL this test fails on roboelectric because the player is created with AUDIO_LANGUAGE_UNSPECIFIED. Can you give me a few pointers on how to fix this.

Copy link
Contributor

Choose a reason for hiding this comment

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

please add stacktrace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

androidx.test.espresso.base.DefaultFailureHandler$AssertionFailedWithCauseError: 'with content description text: is "hi"' doesn't match the selected view. Expected: with content description text: is "hi" Got: "AppCompatImageView{id=2131296629, res-name=ivAudioLanguage, desc=en, visibility=VISIBLE, width=144, height=168, has-focus=false, has-focusable=true, has-window-focus=true, is-clickable=true, is-enabled=true, is-focused=false, is-focusable=true, is-layout-requested=false, is-selected=false, layout-params=androidx.constraintlayout.widget.ConstraintLayout$LayoutParams@1e51db05, tag=null, root-is-layout-requested=false, has-input-connection=false, x=804.0, y=12.0}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

val data = profileManagementController.updateAudioLanguage( profileId, AudioLanguage.HINDI_AUDIO_LANGUAGE ).toLiveData()
profileTestHelper.waitForOperationToComplete(data) i think the problem lies here. @anandwana001 WDYT.

Copy link
Contributor

Choose a reason for hiding this comment

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

@anandwana001 PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit complex as it involves media part. As for now, let keep this test espresso only.

Copy link
Member

Choose a reason for hiding this comment

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

Should we add a TODO to our audio fake library tracking issue to make sure this doesn't get forgotten ('this' being adding Robolectric support for this test)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added Todo for the failing test

Copy link
Member

Choose a reason for hiding this comment

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

Please point to the direct issue tracking audio fakes: #2255.

Copy link
Contributor Author

@MaskedCarrot MaskedCarrot Jan 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@BenHenning I have mentioned this issue here #2417

@Test
fun testAudioFragment_withHindiAudioLanguageProfile_showsHindiAudioLanguage() {
addMediaInfo()
Expand All @@ -181,7 +181,7 @@ class AudioFragmentTest {
it.onActivity {
profileTestHelper.waitForOperationToComplete(data)
}
onView(withId(R.id.tvAudioLanguage)).check(matches(withText("EN")))
onView(withId(R.id.audio_language_icon)).check(matches(withContentDescription("hi")))
}
}

Expand All @@ -194,9 +194,9 @@ class AudioFragmentTest {
)
).use {
testCoroutineDispatchers.runCurrent()
onView(withId(R.id.ivPlayPauseAudio))
onView(withId(R.id.play_pause_audio_icon))
.check(matches(isDisplayed()))
onView(withId(R.id.ivPlayPauseAudio))
onView(withId(R.id.play_pause_audio_icon))
.check(matches(withContentDescription(context.getString(R.string.audio_play_description))))
}
}
Expand All @@ -213,10 +213,10 @@ class AudioFragmentTest {
).use {
testCoroutineDispatchers.runCurrent()

onView(withId(R.id.ivPlayPauseAudio)).perform(click())
onView(withId(R.id.play_pause_audio_icon)).perform(click())

testCoroutineDispatchers.runCurrent()
onView(withId(R.id.ivPlayPauseAudio))
onView(withId(R.id.play_pause_audio_icon))
.check(matches(withContentDescription(context.getString(R.string.audio_pause_description))))
}
}
Expand All @@ -231,10 +231,10 @@ class AudioFragmentTest {
).use {
testCoroutineDispatchers.runCurrent()

onView(withId(R.id.sbAudioProgress)).perform(setProgress(100))
onView(withId(R.id.audio_progress_seek_bar)).perform(setProgress(100))

testCoroutineDispatchers.runCurrent()
onView(withId(R.id.ivPlayPauseAudio))
onView(withId(R.id.play_pause_audio_icon))
.check(matches(withContentDescription(context.getString(R.string.audio_play_description))))
}
}
Expand All @@ -251,12 +251,12 @@ class AudioFragmentTest {
).use {
testCoroutineDispatchers.runCurrent()

onView(withId(R.id.ivPlayPauseAudio)).perform(click())
onView(withId(R.id.play_pause_audio_icon)).perform(click())
testCoroutineDispatchers.runCurrent()
onView(withId(R.id.sbAudioProgress)).perform(setProgress(100))
onView(withId(R.id.audio_progress_seek_bar)).perform(setProgress(100))

testCoroutineDispatchers.runCurrent()
onView(withId(R.id.ivPlayPauseAudio))
onView(withId(R.id.play_pause_audio_icon))
.check(matches(withContentDescription(context.getString(R.string.audio_pause_description))))
}
}
Expand All @@ -270,15 +270,14 @@ class AudioFragmentTest {
)
).use {
invokePreparedListener(shadowMediaPlayer)
onView(withId(R.id.ivPlayPauseAudio)).perform(click())
onView(withId(R.id.sbAudioProgress)).perform(setProgress(100))
onView(withId(R.id.play_pause_audio_icon)).perform(click())
onView(withId(R.id.audio_progress_seek_bar)).perform(setProgress(100))
onView(isRoot()).perform(orientationLandscape())
onView(withId(R.id.ivPlayPauseAudio))
onView(withId(R.id.play_pause_audio_icon))
.check(matches(withContentDescription(context.getString(R.string.audio_pause_description))))
}
}

// TODO(#1845): As updating tvAudioLanguage to image, we need to update this test
@Test
fun testAudioFragment_invokePrepared_changeDifferentLanguage_checkResetSeekBarAndPaused() {
addMediaInfo()
Expand All @@ -289,12 +288,12 @@ class AudioFragmentTest {
).use {
testCoroutineDispatchers.runCurrent()

onView(withId(R.id.ivPlayPauseAudio)).perform(click())
onView(withId(R.id.play_pause_audio_icon)).perform(click())
testCoroutineDispatchers.runCurrent()
onView(withId(R.id.sbAudioProgress)).perform(setProgress(100))
onView(withId(R.id.audio_progress_seek_bar)).perform(setProgress(100))

testCoroutineDispatchers.runCurrent()
onView(withId(R.id.tvAudioLanguage)).perform(click())
onView(withId(R.id.audio_language_icon)).perform(click())

val locale = Locale("es")

Expand All @@ -305,9 +304,9 @@ class AudioFragmentTest {
onView(withText("OK")).inRoot(isDialog()).perform(click())

testCoroutineDispatchers.runCurrent()
onView(withId(R.id.ivPlayPauseAudio))
onView(withId(R.id.play_pause_audio_icon))
.check(matches(withContentDescription(context.getString(R.string.audio_play_description))))
onView(withId(R.id.sbAudioProgress)).check(matches(withSeekBarPosition(0)))
onView(withId(R.id.audio_progress_seek_bar)).check(matches(withSeekBarPosition(0)))
}
}

Expand Down
20 changes: 10 additions & 10 deletions app/src/sharedTest/java/org/oppia/android/app/player/exploration/ExplorationActivityTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ class ExplorationActivityTest {
withEffectiveVisibility(Visibility.VISIBLE)
)
).inRoot(isDialog()).perform(click())
onView(withId(R.id.ivPlayPauseAudio)).check(matches(not(isDisplayed())))
onView(withId(R.id.play_pause_audio_icon)).check(matches(not(isDisplayed())))
}
explorationDataController.stopPlayingExploration()
}
Expand Down Expand Up @@ -438,7 +438,7 @@ class ExplorationActivityTest {

onView(
allOf(
withId(R.id.ivPlayPauseAudio),
withId(R.id.play_pause_audio_icon),
withEffectiveVisibility(Visibility.VISIBLE)
)
)
Expand Down Expand Up @@ -476,7 +476,7 @@ class ExplorationActivityTest {
onView(withId(R.id.action_audio_player)).perform(click())

testCoroutineDispatchers.runCurrent()
onView(withId(R.id.ivPlayPauseAudio)).check(matches(not(isDisplayed())))
onView(withId(R.id.play_pause_audio_icon)).check(matches(not(isDisplayed())))
onView(withText(context.getString(R.string.cellular_data_alert_dialog_title)))
.check(doesNotExist())
}
Expand Down Expand Up @@ -514,7 +514,7 @@ class ExplorationActivityTest {
onView(withId(R.id.action_audio_player)).perform(click())

testCoroutineDispatchers.runCurrent()
onView(withId(R.id.ivPlayPauseAudio)).check(matches(isDisplayed()))
onView(withId(R.id.play_pause_audio_icon)).check(matches(isDisplayed()))
onView(withText(context.getString(R.string.cellular_data_alert_dialog_title)))
.check(doesNotExist())
}
Expand All @@ -540,13 +540,13 @@ class ExplorationActivityTest {
onView(withId(R.id.action_audio_player)).perform(click())
onView(
allOf(
withId(R.id.ivPlayPauseAudio),
withId(R.id.play_pause_audio_icon),
withEffectiveVisibility(Visibility.VISIBLE)
)
)
onView(allOf(withText("EN"), withEffectiveVisibility(Visibility.VISIBLE)))
onView(allOf(withId(R.id.audio_language_icon), withEffectiveVisibility(Visibility.VISIBLE)))
waitForTheView(withDrawable(R.drawable.ic_pause_circle_filled_white_24dp))
onView(withId(R.id.ivPlayPauseAudio)).check(
onView(withId(R.id.play_pause_audio_icon)).check(
matches(
withDrawable(
R.drawable.ic_pause_circle_filled_white_24dp
Expand Down Expand Up @@ -581,7 +581,7 @@ class ExplorationActivityTest {
onView(withId(R.id.action_audio_player)).perform(click())
onView(
allOf(
withText("EN"),
withId(R.id.audio_language_icon),
withEffectiveVisibility(Visibility.VISIBLE)
)
).perform(click())
Expand All @@ -599,7 +599,7 @@ class ExplorationActivityTest {
)
)
onView(withId(R.id.continue_button)).perform(click())
onView(withText("HI-EN")).check(matches(isDisplayed()))
onView(withId(R.id.audio_language_icon)).check(matches(withContentDescription("hi-en")))
}
explorationDataController.stopPlayingExploration()
}
Expand Down Expand Up @@ -633,7 +633,7 @@ class ExplorationActivityTest {
onView(withId(R.id.submit_answer_button)).perform(click())
Thread.sleep(1000)

onView(withId(R.id.ivPlayPauseAudio))
onView(withId(R.id.play_pause_audio_icon))
.check(matches(withContentDescription(context.getString(R.string.audio_pause_description))))
}
explorationDataController.stopPlayingExploration()
Expand Down