Fix part of #130: Cellular data alert in audio player #146

rt4914 · 2019-09-19T16:49:48Z

Explanation

This PR contains the code for cellular data alert. Currently the code does not save user preference to storage.

Output (low-fi)

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 assigned to an appropriate reviewer.

rt4914 · 2019-09-19T16:50:28Z

@BenHenning @jamesxu0 PTAL

BenHenning

Generally LGTM @rt4914--can you also add tests to verify that the dialogs at least appear? I know we can't fully flesh out the behavioral aspects of when they should show up, but ideally if they are always showing then these tests will fail once the real domain implementations are added, and then we'll need to update them accordingly.

app/src/main/java/org/oppia/app/player/audio/AudioFragment.kt

app/src/main/java/org/oppia/app/player/audio/CellularDataDialogFragment.kt

app/src/main/java/org/oppia/app/player/audio/CellularDataInterface.kt

app/src/main/java/org/oppia/app/player/audio/CellularDataDialogFragment.kt

app/src/main/java/org/oppia/app/player/audio/AudioFragment.kt

rt4914 · 2019-09-20T10:50:43Z

Generally LGTM @rt4914--can you also add tests to verify that the dialogs at least appear? I know we can't fully flesh out the behavioral aspects of when they should show up, but ideally if they are always showing then these tests will fail once the real domain implementations are added, and then we'll need to update them accordingly.

Test case done. This test case checks the entire flow of the audio component from showing CelluarDataDialogFragment to AudioFragment to LanguageDialogFragment

rt4914 · 2019-09-20T10:56:59Z

@BenHenning PTAL
I have made following changes as per our discussion:

Shifted code of CellularDataDialogFragment to StateFragment
Changed the usage implementation of CellularDataInterfaceand LanguageInterface as per your suggestions from this link: https://stackoverflow.com/a/23144683/3689782
Added test-case which checks the entire flow of CellularDataDialogFragment, AudioFragment and LanguageDialogFragment
Based on user selection in CellularDataDialogFragment, the code controls whether to show AudioFragment or not.

BenHenning

Just did a really quick glance, but didn't deep-dive yet. Got a bit tired all of a sudden, so I'll take a closer look at this later.

@jamesxu0 could you please give this PR a pass tomorrow?

app/src/main/java/org/oppia/app/player/audio/AudioFragment.kt

app/src/main/java/org/oppia/app/player/audio/CellularDataDialogFragment.kt

app/src/main/java/org/oppia/app/player/state/StateFragment.kt

BenHenning

Thanks @rt4914! Did a more thorough pass on this. PTAL at comments.

app/src/main/java/org/oppia/app/player/audio/CellularDataDialogFragment.kt

app/src/main/java/org/oppia/app/player/audio/CellularDataInterface.kt

app/src/main/java/org/oppia/app/player/audio/LanguageDialogFragment.kt

app/src/main/java/org/oppia/app/player/state/StateFragment.kt

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

app/src/main/java/org/oppia/app/player/state/StateFragment.kt

…er-audio-low-fi

app/src/main/java/org/oppia/app/player/audio/AudioFragment.kt

BenHenning · 2019-09-25T05:27:55Z

app/src/main/java/org/oppia/app/player/audio/LanguageDialogFragment.kt

-      languageList.add("hi-en")
-    }
+    var selectedIndex = arguments!!.getInt(KEY_SELECTED_INDEX, 0)
+    val languageArrayList: ArrayList<String> = arguments?.getStringArrayList(KEY_LANGUAGE_LIST) as ArrayList<String>


Why is this cast needed? It seems superfluous since getStringArrayList() returns ArrayList.

Also, the ?. operator needed here? I think you can avoid it by saving arguments locally:

val args = checkNotNull(arguments) { "Expected arguments to be pass to LanguageDialogFragment" } // args can now be used without the nullability considerations

Regarding extra cast:

val languageArrayList: ArrayList<String> = args.getStringArrayList(KEY_LANGUAGE_LIST) as ArrayList<String>

If we do not add this extra cast then there is type mismatch warning

Added this line:

val args = checkNotNull(arguments) { "Expected arguments to be pass to LanguageDialogFragment" }

app/src/main/java/org/oppia/app/player/audio/CellularDataDialogFragment.kt

BenHenning

Thanks @rt4914! Generally LGTM, but I think there are still some earlier unresolved comments. Can you please take a look at those & address them? I'm specifically interested in the request for additional tests.

BenHenning

Given that the remaining changes are trivial, feel free to submit once they're resolved (changing the fragment manager & adding the remaining 2 test cases).

rt4914 · 2019-09-25T06:03:45Z

Given that the remaining changes are trivial, feel free to submit once they're resolved (changing the fragment manager & adding the remaining 2 test cases).

Thanks @BenHenning for regular and prompt reviews.

I have replaced fragmentManager with childFragmentManager to establish parent/child relation within fragments.

Also, working on test-cases now.

…er-audio-low-fi

rt4914 · 2019-09-25T06:29:22Z

Final task of adding test-cases has been done, therefore, merging this PR now.

Celluar data alert in audio player

5f4f79a

rt4914 requested review from BenHenning and jamesxu0 September 19, 2019 16:49

rt4914 assigned BenHenning, veena14cs, jamesxu0 and nikitamarysolomanpvt Sep 19, 2019

Javadoc updates

a2d9dbd

BenHenning requested changes Sep 20, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Sep 20, 2019

Rajat Talesra added 6 commits September 20, 2019 10:15

Changed CellularDataInterface with simpler methods

a70d719

CellularDataFragment shifted to StateFragment

f2854d4

Imrpovised Language and Celluar Fragments

3a48450

Implementation change in CellularDataINterface

f0d1fc2

Implementation change in LanguageInterface

09f7ca9

Complete cellular, audio, language screen flow test

e4b3395

Nit changes

7c84764

rt4914 assigned BenHenning and unassigned rt4914 Sep 20, 2019

BenHenning requested changes Sep 23, 2019

View reviewed changes

app/src/main/java/org/oppia/app/player/audio/AudioFragment.kt Outdated Show resolved Hide resolved

app/src/main/java/org/oppia/app/player/audio/CellularDataDialogFragment.kt Outdated Show resolved Hide resolved

Nit changes and removed try-catch-throw code

3829297

jamesxu0 reviewed Sep 23, 2019

View reviewed changes

app/src/main/java/org/oppia/app/player/state/StateFragment.kt Outdated Show resolved Hide resolved

rt4914 unassigned jamesxu0 Sep 24, 2019

Rajat Talesra added 3 commits September 24, 2019 08:55

Removed unnecessary interface code

71c2a12

Remove StateFragment usgae from HomeActivityController

04e2c6f

Nit javadoc changes

c82f0ef

BenHenning requested changes Sep 24, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Sep 24, 2019

Rajat Talesra added 4 commits September 24, 2019 13:00

Optimise DialogFragment code and test case

57841ed

Code optimization

69ffe6f

Updated isAudioFragmemntVisible usage

e1bdb0d

Data share via Bundle in DialogFragment

1b281c3

rt4914 assigned BenHenning and jamesxu0 and unassigned rt4914, veena14cs and nikitamarysolomanpvt Sep 24, 2019

Rajat Talesra added 2 commits September 24, 2019 21:34

Merge remote-tracking branch 'upstream/develop' into exploration-play…

e4e5b81

…er-audio-low-fi

Using targetFragment for interfaces

8b4009b

BenHenning reviewed Sep 25, 2019

View reviewed changes

BenHenning requested changes Sep 25, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Sep 25, 2019

Using childFragmentManager

f7f743a

BenHenning approved these changes Sep 25, 2019

View reviewed changes

Rajat Talesra added 2 commits September 25, 2019 11:57

Test case for CellularDataDialogFragment

55597f4

Merge remote-tracking branch 'upstream/develop' into exploration-play…

b78d834

…er-audio-low-fi

rt4914 merged commit 4404f71 into develop Sep 25, 2019

rt4914 unassigned jamesxu0 Sep 25, 2019

rt4914 deleted the exploration-player-audio-low-fi branch September 25, 2019 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #130: Cellular data alert in audio player #146

Fix part of #130: Cellular data alert in audio player #146

rt4914 commented Sep 19, 2019

rt4914 commented Sep 19, 2019

BenHenning left a comment

rt4914 commented Sep 20, 2019

rt4914 commented Sep 20, 2019

BenHenning left a comment

BenHenning left a comment

BenHenning Sep 25, 2019

rt4914 Sep 25, 2019 •

edited

Loading

BenHenning left a comment

BenHenning left a comment

rt4914 commented Sep 25, 2019

rt4914 commented Sep 25, 2019

Fix part of #130: Cellular data alert in audio player #146

Fix part of #130: Cellular data alert in audio player #146

Conversation

rt4914 commented Sep 19, 2019

Explanation

Output (low-fi)

Checklist

rt4914 commented Sep 19, 2019

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 commented Sep 20, 2019

rt4914 commented Sep 20, 2019

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Sep 25, 2019

Choose a reason for hiding this comment

rt4914 Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 commented Sep 25, 2019

rt4914 commented Sep 25, 2019

rt4914 Sep 25, 2019 •

edited

Loading