Fix #1845: Replace Language code with icon in AudioBar [Blocked on #1872] #1862

MaskedCarrot · 2020-09-19T06:00:56Z

Explanation

Fixes #1845
Replaced audio language text with an icon

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.

aggarwalpulkit596 · 2020-09-19T06:51:49Z

@MaskedCarrot there's no need to create a new PR everytime you could've tried to fix previous one and in that process you would learn something as well

MaskedCarrot · 2020-09-19T07:08:35Z

I will try to that from the next time :-)

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

anandwana001 · 2020-09-19T18:30:12Z

@MaskedCarrot Congratulations on your first successful Pull Request 🎉

I would like to add few comments which will help you in more successful and smooth contributions in Oppia-andorid.

It seems like you had already completed the onboarding part, in case if you missed something you can check it out or if you find anything difficult, you can reach out to me on our gitter channel - https://github.com/oppia/oppia-android/wiki#onboarding-instructions
I suggest you to setup ktlint successfully at your local machine, this will defiantly help you long way - https://github.com/oppia/oppia-android/wiki#prerequisites
You can check this wiki page here describing about on how you can make code change properly by following good practices - https://github.com/oppia/oppia-android/wiki#instructions-for-making-a-code-change
You will be able to add Reviewers and Assignee once you can had successfully submitted 2 PR. Till then you can mention me in the PR description.

Branch name should be something like replace-language-code-icon-audio-bar.
The PR title should follow this particular format, as per mentioned in the checklist in the PR description - 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: ...".)

These changes you can follow from the next PR. No need to update branch name for this PR.

anandwana001

Thanks @MaskedCarrot
Added a few changes, please let me know if you need any help in this, we can chat over gitter channel.

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

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

app/src/main/res/drawable/ic_audio_lang_24px.xml

anandwana001

PR LGTM, Thanks @MaskedCarrot
Before the merge, could you please comment on my earlier review regarding test.
So, I could resolve it accordingly.

anandwana001

LGTM, Thanks @MaskedCarrot

rt4914

Looks good, just suggested few changes.

Thanks for the PR @MaskedCarrot

app/src/main/res/drawable/ic_audio_lang_24px.xml

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

anandwana001 · 2020-09-24T10:27:13Z

@MaskedCarrot Before we review, could you please resolve the conflicts and correct the failing checks.

MaskedCarrot · 2020-09-24T11:30:07Z

The test in ExplorationActivityTest.kt at testAudioWithWifi_openFractionsExploration_changeLanguage_clickNext_checkLanguageIsHinglish is failing can you please help @anandwana001

anandwana001 · 2020-09-24T11:35:11Z

It's the ignore line, could you please run test on your local machine and paste the error here from the logcat?

anandwana001

Thanks @MaskedCarrot Added few comments.
Also, Why we need to update ExplorationTest here?

app/src/sharedTest/java/org/oppia/app/player/exploration/ExplorationActivityTest.kt

anandwana001 · 2020-09-25T02:32:58Z

@MaskedCarrot You need to update the branch with develop before working more on this PR as we had updated the package names.

anandwana001

Please update your branch with the latest develop branch.
Also, as per test cases in ExplorationActivityTest, could you please add a screenshot which shows that your test cases are passing on both espresso and robolectric?

MaskedCarrot · 2020-09-28T06:12:17Z

Please update your branch with the latest develop branch.

I have updated it with develop branch

Also, as per test cases in ExplorationActivityTest, could you please add a screenshot which shows that your test cases are passing on both espresso and robolectric?

anandwana001 · 2020-09-30T10:45:53Z

This PR is blocked on #1872
After the issue solved, the test in this PR should be updated and tested on both espresso and robolectric and good to merge.

Thanks @MaskedCarrot
I think by the time you can take another issue to solve or if you would like to take #1872 comment on the issue and let me know.

MaskedCarrot · 2020-10-02T07:21:39Z

the test getting ignored are the ones with reason "The ExplorationActivity takes time to finish, needs to fixed in #89." which are not related with language icon

anandwana001 · 2020-10-02T07:36:32Z

@MaskedCarrot To make this pr full pass testAudioFragment_openFragment_showsAudioLanguageIcon() we need your specific test case to pass on both espresso and robolectric.
And, as far as I know all the test cases in this file have only one issue which is regarding the data source when you run test case on robolectric, that's the reason I marked this PR blocked.

anandwana001 · 2020-10-02T14:54:55Z

Could you please add both screenshot showing that your test cases are passing on espresso and robolectric before I review?

MaskedCarrot · 2020-10-02T15:01:29Z

I am facing a problem with that. although the test passes on GitHub. On my local machine, the test on ExplorationActivityTest.kt is working fine. But when I run AudioFragmentTest.kt i get an error saying Didn't find class "org.robolectric.shadows.util.DataSource and I can't figure out a way to fix this

anandwana001 · 2020-10-02T15:05:40Z

I am facing a problem with that. although the test passes on GitHub. On my local machine, the test on ExplorationActivityTest.kt is working fine. But when I run AudioFragmentTest.kt i get an error saying Didn't find class "org.robolectric.shadows.util.DataSource and I can't figure out a way to fix this

Yes, that is the issue I am mentioning earlier and why I want this PR to be blocked till that issue gets fixed #1872 .

I suggest by the time feel free to pick other issues.

MaskedCarrot · 2020-10-02T15:07:10Z

can I take up a different issue then?

anandwana001 · 2020-10-02T15:19:22Z

can I take up a different issue then?

Yes you can, comment on any issue you want to work and i will assign you.

anandwana001 · 2020-10-05T05:57:28Z

I am facing a problem with that. although the test passes on GitHub. On my local machine, the test on ExplorationActivityTest.kt is working fine. But when I run AudioFragmentTest.kt i get an error saying Didn't find class "org.robolectric.shadows.util.DataSource and I can't figure out a way to fix this

Yes, that is the issue I am mentioning earlier and why I want this PR to be blocked till that issue gets fixed #1872 .

I suggest by the time feel free to pick other issues.

I am able to run test cases on Robolectric as I fixed it, but didn't send the PR as it is not working on Espresso as per the #59 so @rt4914 Could you suggest what should we do in this case?

rt4914 · 2020-10-05T06:04:46Z

I am facing a problem with that. although the test passes on GitHub. On my local machine, the test on ExplorationActivityTest.kt is working fine. But when I run AudioFragmentTest.kt i get an error saying Didn't find class "org.robolectric.shadows.util.DataSource and I can't figure out a way to fix this

Yes, that is the issue I am mentioning earlier and why I want this PR to be blocked till that issue gets fixed #1872 .
I suggest by the time feel free to pick other issues.

I am able to run test cases on Robolectric as I fixed it, but didn't send the PR as it is not working on Espresso as per the #59 so @rt4914 Could you suggest what should we do in this case?

@anandwana001 Does this mean that you have fixed the tests (but it is not on develop) and once you merge those tests to develop, after which this PR can be updated and will be ready to merge directly?

anandwana001 · 2020-10-05T06:07:53Z

I am facing a problem with that. although the test passes on GitHub. On my local machine, the test on ExplorationActivityTest.kt is working fine. But when I run AudioFragmentTest.kt i get an error saying Didn't find class "org.robolectric.shadows.util.DataSource and I can't figure out a way to fix this

Yes, that is the issue I am mentioning earlier and why I want this PR to be blocked till that issue gets fixed #1872 .
I suggest by the time feel free to pick other issues.

I am able to run test cases on Robolectric as I fixed it, but didn't send the PR as it is not working on Espresso as per the #59 so @rt4914 Could you suggest what should we do in this case?

I am facing a problem with that. although the test passes on GitHub. On my local machine, the test on ExplorationActivityTest.kt is working fine. But when I run AudioFragmentTest.kt i get an error saying Didn't find class "org.robolectric.shadows.util.DataSource and I can't figure out a way to fix this

Yes, that is the issue I am mentioning earlier and why I want this PR to be blocked till that issue gets fixed #1872 .
I suggest by the time feel free to pick other issues.

I am able to run test cases on Robolectric as I fixed it, but didn't send the PR as it is not working on Espresso as per the #59 so @rt4914 Could you suggest what should we do in this case?

@anandwana001 Does this mean that you have fixed the tests (but it is not on develop) and once you merge those tests to develop, after which this PR can be updated and will be ready to merge directly?

I can send a PR but it will not work on espresso as it is blocked on #59. I had to confirm with ben once. Though, I am trying to fix it by then.

rt4914 · 2020-10-05T06:17:45Z

I am facing a problem with that. although the test passes on GitHub. On my local machine, the test on ExplorationActivityTest.kt is working fine. But when I run AudioFragmentTest.kt i get an error saying Didn't find class "org.robolectric.shadows.util.DataSource and I can't figure out a way to fix this

Yes, that is the issue I am mentioning earlier and why I want this PR to be blocked till that issue gets fixed #1872 .
I suggest by the time feel free to pick other issues.

I am able to run test cases on Robolectric as I fixed it, but didn't send the PR as it is not working on Espresso as per the #59 so @rt4914 Could you suggest what should we do in this case?

I am facing a problem with that. although the test passes on GitHub. On my local machine, the test on ExplorationActivityTest.kt is working fine. But when I run AudioFragmentTest.kt i get an error saying Didn't find class "org.robolectric.shadows.util.DataSource and I can't figure out a way to fix this

Yes, that is the issue I am mentioning earlier and why I want this PR to be blocked till that issue gets fixed #1872 .
I suggest by the time feel free to pick other issues.

I am able to run test cases on Robolectric as I fixed it, but didn't send the PR as it is not working on Espresso as per the #59 so @rt4914 Could you suggest what should we do in this case?

@anandwana001 Does this mean that you have fixed the tests (but it is not on develop) and once you merge those tests to develop, after which this PR can be updated and will be ready to merge directly?

I can send a PR but it will not work on espresso as it is blocked on #59. I had to confirm with ben once. Though, I am trying to fix it by then.

Okay, makes sense. Can you file an issues related to fixing the test cases and it is blocked on 59 and mention it here so that we can close this PR. How does this sound?

anandwana001 · 2020-10-05T06:19:57Z

@rt4914

I did mention it. Let's not close the PR. We can fix it and then this PR can continue, if it took a bit longer, then we can close this PR.

This PR is blocked on #1872
After the issue solved, the test in this PR should be updated and tested on both espresso and robolectric and good to merge.

rt4914 · 2020-10-22T14:12:15Z

@MaskedCarrot I am not sure what should be the next step here? You have closed this PR and re-opened so should be review it again or anything else?

MaskedCarrot · 2020-10-22T14:17:34Z

I accidentally closed the pr and so I reopened it. The roboelectric test needs to be fixed for AudioFragmentTest then this pr will be good to merge. Sorry for the inconvenience :-)

rt4914 · 2020-10-22T14:30:33Z

I accidentally closed the pr and so I reopened it. The roboelectric test needs to be fixed for AudioFragmentTest then this pr will be good to merge. Sorry for the inconvenience :-)

@MaskedCarrot

Sounds good. I have updated the title accordingly.

anandwana001 · 2020-12-08T05:49:31Z

@MaskedCarrot I had fixed the test file, please take the latest pull and you can continue your work.

MaskedCarrot · 2020-12-08T12:03:17Z

@anandwana001 sorry for the delay but can I start working on this by Friday?

anandwana001 · 2020-12-08T14:22:14Z

@anandwana001 sorry for the delay but can I start working on this by Friday?

Sure

anandwana001 · 2020-12-22T03:21:48Z

@MaskedCarrot any update on this?

MaskedCarrot · 2020-12-22T04:23:58Z

I am sorry for the delay. I tried running audioFragmentTests and its tests are failing.

anandwana001 · 2020-12-22T05:12:37Z

Did you update from the latest develop? @MaskedCarrot
Some of test cases are ignored which checks pause icon, due to unavailability of fake audio library which is a separate issue.

MaskedCarrot · 2020-12-22T05:15:38Z

Yes my repo is up-to-date with the latest develop

MaskedCarrot · 2020-12-22T17:11:09Z

@anandwana001 can you please help me in fixing these test cases.

anandwana001 · 2020-12-23T02:32:48Z

@MaskedCarrot Connecting with you on Gitter.

MaskedCarrot · 2020-12-24T13:32:22Z

made another pr for this issue #2308

anandwana001 reviewed Sep 19, 2020

View reviewed changes

app/src/sharedTest/java/org/oppia/app/player/audio/AudioFragmentTest.kt Outdated Show resolved Hide resolved

anandwana001 assigned MaskedCarrot Sep 19, 2020

anandwana001 changed the title ~~Develop replace icon~~ Fix #1845: Replace Language code with icon in AudioBar Sep 21, 2020

anandwana001 suggested changes Sep 21, 2020

View reviewed changes

MaskedCarrot requested a review from anandwana001 September 21, 2020 12:06

anandwana001 suggested changes Sep 21, 2020

View reviewed changes

anandwana001 approved these changes Sep 22, 2020

View reviewed changes

anandwana001 assigned rt4914 and unassigned MaskedCarrot Sep 22, 2020

rt4914 suggested changes Sep 23, 2020

View reviewed changes

app/src/main/res/drawable/ic_audio_lang_24px.xml Outdated Show resolved Hide resolved

app/src/main/res/layout/audio_fragment.xml Outdated Show resolved Hide resolved

rt4914 assigned anandwana001 and MaskedCarrot and unassigned rt4914 and anandwana001 Sep 23, 2020

MaskedCarrot requested review from anandwana001 and rt4914 September 24, 2020 09:59

anandwana001 suggested changes Sep 24, 2020

View reviewed changes

MaskedCarrot requested a review from anandwana001 September 26, 2020 13:29

anandwana001 suggested changes Sep 28, 2020

View reviewed changes

MaskedCarrot requested a review from anandwana001 October 2, 2020 14:32

MaskedCarrot closed this Oct 7, 2020

MaskedCarrot deleted the developReplaceIcon branch October 7, 2020 01:45

MaskedCarrot restored the developReplaceIcon branch October 7, 2020 01:45

MaskedCarrot reopened this Oct 7, 2020

rt4914 changed the title ~~Fix #1845: Replace Language code with icon in AudioBar~~ Fix #1845: Replace Language code with icon in AudioBar [Blocked on #1872] Oct 22, 2020

MaskedCarrot closed this Dec 24, 2020

Fix #1845: Replace Language code with icon in AudioBar [Blocked on #1872] #1862

Fix #1845: Replace Language code with icon in AudioBar [Blocked on #1872] #1862

Conversation

MaskedCarrot commented Sep 19, 2020 • edited Loading

Explanation

Checklist

aggarwalpulkit596 commented Sep 19, 2020

MaskedCarrot commented Sep 19, 2020

anandwana001 commented Sep 19, 2020 • edited Loading

anandwana001 left a comment

Choose a reason for hiding this comment

anandwana001 left a comment

Choose a reason for hiding this comment

anandwana001 left a comment

Choose a reason for hiding this comment

rt4914 left a comment • edited Loading

Choose a reason for hiding this comment

anandwana001 commented Sep 24, 2020

MaskedCarrot commented Sep 24, 2020 • edited Loading

anandwana001 commented Sep 24, 2020

anandwana001 left a comment • edited Loading

Choose a reason for hiding this comment

anandwana001 commented Sep 25, 2020

anandwana001 left a comment • edited Loading

Choose a reason for hiding this comment

MaskedCarrot commented Sep 28, 2020

anandwana001 commented Sep 30, 2020

MaskedCarrot commented Oct 2, 2020

anandwana001 commented Oct 2, 2020 • edited Loading

anandwana001 commented Oct 2, 2020

MaskedCarrot commented Oct 2, 2020

anandwana001 commented Oct 2, 2020 • edited Loading

MaskedCarrot commented Oct 2, 2020

anandwana001 commented Oct 2, 2020 • edited Loading

anandwana001 commented Oct 5, 2020 • edited Loading

rt4914 commented Oct 5, 2020

anandwana001 commented Oct 5, 2020 • edited Loading

rt4914 commented Oct 5, 2020

anandwana001 commented Oct 5, 2020

rt4914 commented Oct 22, 2020

MaskedCarrot commented Oct 22, 2020

rt4914 commented Oct 22, 2020

anandwana001 commented Dec 8, 2020

MaskedCarrot commented Dec 8, 2020 • edited Loading

anandwana001 commented Dec 8, 2020

anandwana001 commented Dec 22, 2020

MaskedCarrot commented Dec 22, 2020 • edited Loading

anandwana001 commented Dec 22, 2020 • edited Loading

MaskedCarrot commented Dec 22, 2020

MaskedCarrot commented Dec 22, 2020

anandwana001 commented Dec 23, 2020

MaskedCarrot commented Dec 24, 2020

MaskedCarrot commented Sep 19, 2020 •

edited

Loading

anandwana001 commented Sep 19, 2020 •

edited

Loading

rt4914 left a comment •

edited

Loading

MaskedCarrot commented Sep 24, 2020 •

edited

Loading

anandwana001 left a comment •

edited

Loading

anandwana001 left a comment •

edited

Loading

anandwana001 commented Oct 2, 2020 •

edited

Loading

anandwana001 commented Oct 2, 2020 •

edited

Loading

anandwana001 commented Oct 2, 2020 •

edited

Loading

anandwana001 commented Oct 5, 2020 •

edited

Loading

anandwana001 commented Oct 5, 2020 •

edited

Loading

MaskedCarrot commented Dec 8, 2020 •

edited

Loading

MaskedCarrot commented Dec 22, 2020 •

edited

Loading

anandwana001 commented Dec 22, 2020 •

edited

Loading