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

MaskedCarrot · 2021-01-17T06:22:28Z

Explanation

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.

Tests

class	Robolectric	Espresso
AudioFragmentTests
ExplorationActivityTest

Screenshots

Before	After

MaskedCarrot · 2021-01-17T06:28:09Z

app/src/sharedTest/java/org/oppia/android/app/player/audio/AudioFragmentTest.kt

    }
  }

-  // TODO(#1845): As updating tvAudioLanguage to image, we need to remove this test
+  @RunOn(TestPlatform.ESPRESSO)


@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.

please add stacktrace.

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}"

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

@anandwana001 PTAL

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

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)?

added Todo for the failing test

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

@BenHenning I have mentioned this issue here #2417

anandwana001 · 2021-01-18T05:23:28Z

@MaskedCarrot Please add a screenshot which shows test result of both espresso and robolectric.

Luffy18346 · 2021-01-21T17:49:53Z

@MaskedCarrot Any updates on this?

MaskedCarrot · 2021-01-22T03:57:32Z

@Luffy18346 I am working on this

anandwana001

LGTM

rt4914 · 2021-01-23T18:58:21Z

Will be reviewing this on Monday. Thanks.

rt4914

LGTM, thanks.

BenHenning

Thanks @MaskedCarrot! I think the changes per my codeowners look generally fine, though I had one comment regarding the ID naming.

Also, could you upload a before/after screenshot & add them to the PR opening comment to clearly demonstrate the visual changes introduced here?

BenHenning · 2021-01-26T01:53:58Z

app/src/sharedTest/java/org/oppia/android/app/player/audio/AudioFragmentTest.kt

    }
  }

-  // TODO(#1845): As updating tvAudioLanguage to image, we need to remove this test
+  @RunOn(TestPlatform.ESPRESSO)


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)?

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

rt4914 · 2021-01-27T07:12:53Z

@MaskedCarrot Please keep the Assignee section up-to-date: https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section

MaskedCarrot · 2021-01-27T07:22:41Z

@MaskedCarrot Please keep the Assignee section up-to-date: https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section

thanks. Updated the assignee section appropriately

BenHenning

Thanks @MaskedCarrot! Left one comment on the existing thread, but otherwise this LGTM for codeowners.

BenHenning · 2021-01-29T01:52:02Z

Ah, forgot to reassign yesterday. @MaskedCarrot PTAL.

MaskedCarrot · 2021-01-29T05:39:31Z

I have filed a new issue regarding the failing test in #2568 and also I have mentioned it here #2417 and mentioned the same in todo @BenHenning PTAL

BenHenning

Thanks @MaskedCarrot. LGTM.

replace icon

de7bb1e

MaskedCarrot requested review from BenHenning and rt4914 as code owners January 17, 2021 06:22

MaskedCarrot changed the title ~~replace icon~~ Fix #1845: Replace Language code with icon in AudioBar Jan 17, 2021

removed comment

1a91a36

MaskedCarrot commented Jan 17, 2021

View reviewed changes

removed wild card import

9db0a2b

MaskedCarrot requested a review from anandwana001 January 17, 2021 06:31

MaskedCarrot assigned anandwana001 Jan 17, 2021

anandwana001 assigned MaskedCarrot and unassigned anandwana001 Jan 18, 2021

rt4914 assigned rt4914 and anandwana001 and unassigned rt4914 Jan 18, 2021

anandwana001 approved these changes Jan 22, 2021

View reviewed changes

anandwana001 removed their assignment Jan 22, 2021

MaskedCarrot assigned rt4914 and unassigned MaskedCarrot Jan 22, 2021

rt4914 approved these changes Jan 25, 2021

View reviewed changes

rt4914 assigned BenHenning and unassigned rt4914 Jan 25, 2021

BenHenning requested changes Jan 26, 2021

View reviewed changes

BenHenning assigned MaskedCarrot and unassigned BenHenning Jan 26, 2021

MaskedCarrot added 2 commits January 26, 2021 19:10

fixed ids of audio_fragment

6513ca4

fixed failing tests

240c347

lint fix

f9e7ae0

MaskedCarrot requested a review from BenHenning January 26, 2021 14:06

fixed failing build

572d0b4

MaskedCarrot assigned BenHenning and unassigned MaskedCarrot Jan 27, 2021

added TODO for ignored robolectric test

09ca138

MaskedCarrot assigned MaskedCarrot and BenHenning and unassigned BenHenning and MaskedCarrot Jan 27, 2021

MaskedCarrot mentioned this pull request Jan 27, 2021

Fix all the ignored test cases #2417

Open

MaskedCarrot assigned MaskedCarrot and BenHenning and unassigned BenHenning and MaskedCarrot Jan 27, 2021

BenHenning approved these changes Jan 28, 2021

View reviewed changes

BenHenning assigned MaskedCarrot and unassigned BenHenning Jan 29, 2021

MaskedCarrot assigned BenHenning and unassigned MaskedCarrot Jan 29, 2021

BenHenning approved these changes Jan 30, 2021

View reviewed changes

BenHenning merged commit 46870e8 into oppia:develop Jan 30, 2021

MaskedCarrot deleted the ReplaceAudioLang branch February 1, 2021 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MaskedCarrot commented Jan 17, 2021 •

edited

Loading

MaskedCarrot Jan 17, 2021

anandwana001 Jan 18, 2021

MaskedCarrot Jan 18, 2021

MaskedCarrot Jan 18, 2021

rt4914 Jan 19, 2021

anandwana001 Jan 22, 2021

BenHenning Jan 26, 2021

MaskedCarrot Jan 27, 2021

BenHenning Jan 28, 2021

MaskedCarrot Jan 28, 2021 •

edited

Loading

anandwana001 commented Jan 18, 2021

Luffy18346 commented Jan 21, 2021

MaskedCarrot commented Jan 22, 2021

anandwana001 left a comment

rt4914 commented Jan 23, 2021

rt4914 left a comment

BenHenning left a comment

BenHenning Jan 26, 2021

rt4914 commented Jan 27, 2021

MaskedCarrot commented Jan 27, 2021

BenHenning left a comment

BenHenning commented Jan 29, 2021

MaskedCarrot commented Jan 29, 2021 •

edited

Loading

BenHenning left a comment

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

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

Conversation

MaskedCarrot commented Jan 17, 2021 • edited Loading

Explanation

Checklist

Tests

Screenshots

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

MaskedCarrot Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

anandwana001 commented Jan 18, 2021

Luffy18346 commented Jan 21, 2021

MaskedCarrot commented Jan 22, 2021

anandwana001 left a comment

Choose a reason for hiding this comment

rt4914 commented Jan 23, 2021

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rt4914 commented Jan 27, 2021

MaskedCarrot commented Jan 27, 2021

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Jan 29, 2021

MaskedCarrot commented Jan 29, 2021 • edited Loading

BenHenning left a comment

Choose a reason for hiding this comment

MaskedCarrot commented Jan 17, 2021 •

edited

Loading

MaskedCarrot Jan 28, 2021 •

edited

Loading

MaskedCarrot commented Jan 29, 2021 •

edited

Loading