Fix part of #17: Exploration Player Audio Component (Part 4) #105

rt4914 · 2019-09-16T05:26:47Z

Explanation

This PR contains the basic structure of AudioComponent mainly introduction of activities and fragments.

Things which needs to be implemented:

AudioControllerFragment and ViewModel.
Everything related to interaction is pending.

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.
x ] 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-16T05:28:03Z

@BenHenning PTAL. This PR mainly misses the functional part of audio component. Also, note that PR base branch is exploration-player-1-base.

…n code

BenHenning

Thanks @rt4914! I'd like to do one more pass on this.

@jamesxu0 I suggest you also do a review pass on this.

app/src/main/java/org/oppia/app/FakeDataProvider.kt

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

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

app/src/main/res/values/dimens.xml

app/src/main/res/values/strings.xml

app/src/main/java/org/oppia/app/player/state/audio/LanguageInterface.kt

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

app/src/main/res/values/dimens.xml

rt4914 · 2019-09-17T07:36:20Z

@BenHenning @jamesxu0 This PR cannot be merged and is still pending. Refer to email that I have sent you. But this PR is the base and can be taken ahead from here.

BenHenning · 2019-09-18T06:43:10Z

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

+    if (prev != null) {
+      fragmentTransaction.remove(prev)
+    }
+    fragmentTransaction.addToBackStack(null)


Rather than relying on the backstack here, could we just retrieve the existing dialog via findFragmentByTag() and dismiss it, then show a new dialog?

I have updated this code as per this resource but it almost looks like previous code, so not sure if it is correct.

See my later thread. I was hoping we could use dismiss() on the dialog fragment, but your solution is more correct so we'll keep this.

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

BenHenning · 2019-09-18T06:48:31Z

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

+      android:textSize="14sp"
+      android:textStyle="bold"
+      android:text="@{viewModel.audioLanguageCode}"
+      android:onClick="@{(v) -> viewModel.languageSelectionClicked(fragmentManager)}"


Hmm, this feels a bit odd to me. Can we call a method on the fragment, instead? It feels safer to keep fragment transactions isolated to fragment classes, and view models probably shouldn't really do much logic if possible. It's easier to verify fragments as correct, so we should push logic to those as much as possible.

Per https://stackoverflow.com/a/54312819/3689782 this seems possible. I suppose we can bind just about anything in the binding object.

Made changes as per link mentioned shifted logic part to AudioFragment

Thanks--this looks good!

Actually sorry I misunderstood--let's only perform the transaction in AudioFragment and not ViewModel. The current code is still calling languageSelectionClicked() in ViewModel, and this code should instead be in AudioFragment.

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

BenHenning · 2019-09-18T06:55:16Z

Also @rt4914 can you resolve the conflicts that are pending for this branch?

rt4914 · 2019-09-18T19:20:40Z

@BenHenning PTAL

BenHenning

Thanks @rt4914. Just one thing needs to be removed and this looks good to submit.

app/src/main/java/org/oppia/app/home/HomeActivityController.kt

BenHenning · 2019-09-18T19:40:02Z

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

+  fun languageSelectionClicked(fragmentManager: FragmentManager) {
+    val previousFragment = fragmentManager.findFragmentByTag(TAG_DIALOG)
+    if (previousFragment != null) {
+      fragmentManager.beginTransaction().remove(previousFragment).commitNow()


Ah. I really think it would be nicer to use dismiss(), but I just looked through the AndroidX code and it appears dismiss() is not synchronous (it uses commit() internally), so this is unfortunately necessary.

That means this is correct right?

BenHenning

Sorry, I missed one thing that we should fix before submission.

BenHenning · 2019-09-18T19:43:02Z

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

+      android:textSize="14sp"
+      android:textStyle="bold"
+      android:text="@{viewModel.audioLanguageCode}"
+      android:onClick="@{(v) -> viewModel.languageSelectionClicked(fragmentManager)}"


Actually sorry I misunderstood--let's only perform the transaction in AudioFragment and not ViewModel. The current code is still calling languageSelectionClicked() in ViewModel, and this code should instead be in AudioFragment.

rt4914 · 2019-09-19T03:57:05Z

Sorry, I missed one thing that we should fix before submission.

Hi @BenHenning earlier one of commits related to ViewModel code missed, can you have a fresh look on this PR.

BenHenning

LGTM, though I think you should make the language code field observable so it automatically updates in the UI.

app/src/main/java/org/oppia/app/home/HomeActivityController.kt

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

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

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

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

rt4914 · 2019-09-19T09:31:50Z

Merging this code to unlock James. This code does need improvements but that will be managed in another PR.

Rajat Talesra added 6 commits September 13, 2019 14:03

Exploration Player base code for activity and fragment

4908754

Fake Data Provider

8e45390

Base code for audio player

4b0ee2f

XML code and resources for audio component

27ef74e

LanguageDialogFragment added

86633c4

Language Interface code

e4e8f7c

rt4914 requested a review from BenHenning September 16, 2019 05:26

rt4914 assigned BenHenning and jamesxu0 Sep 16, 2019

rt4914 requested a review from jamesxu0 September 16, 2019 05:27

Rajat Talesra added 6 commits September 16, 2019 11:15

Merge remote-tracking branch 'upstream/develop' into HEAD

451cb3f

Nit changes mainly in documentation

10768a3

Nit changes mainly in documentation for State too

e257140

Changed Controller to Presenter and optimised the fragment transactio…

2d7e210

…n code

Basic code for ExporationActivityTest

fb9e952

ExplorationActivity dummy test

84c42b8

BenHenning requested changes Sep 17, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Sep 17, 2019

BenHenning mentioned this pull request Sep 17, 2019

Fix #17: adds basic audio player functionality #108

Closed

6 tasks

Rajat Talesra added 4 commits September 17, 2019 09:18

Resolve merge conflicts

32a289b

Nit changes like naming and spacing and comments

003384d

Nit changes to audio fragment in xml files and java

fe881ad

Audio language selection

372a029

rt4914 assigned BenHenning and unassigned rt4914 Sep 17, 2019

BenHenning requested changes Sep 18, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Sep 18, 2019

BenHenning changed the title ~~Fix part of #17: Exploration Player Audio Component (Part 4) [Blocked: #5]~~ Fix #130: Exploration Player Audio Component (Part 4) [Blocked: #5] Sep 18, 2019

BenHenning changed the title ~~Fix #130: Exploration Player Audio Component (Part 4) [Blocked: #5]~~ Fix part of #17: Exploration Player Audio Component (Part 4) Sep 18, 2019

Rajat Talesra added 3 commits September 18, 2019 16:20

Removed logs, and improvised fragment code

c9a1b5d

Removed unused imports

2dfe2b9

Changes to LanguageFragment but still not perfect

8aa315d

rt4914 assigned BenHenning and unassigned rt4914 Sep 18, 2019

BenHenning approved these changes Sep 18, 2019

View reviewed changes

BenHenning reviewed Sep 18, 2019

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Sep 18, 2019

Rajat Talesra added 2 commits September 19, 2019 09:16

ViewModel and Fragment changes

4516c61

Resolve merge conflicts with develop

6db5c62

rt4914 changed the base branch from exploration-player-1-base to develop September 19, 2019 03:53

Nit changes

0685908

BenHenning approved these changes Sep 19, 2019

View reviewed changes

Rajat Talesra added 3 commits September 19, 2019 11:44

Nit changes in javadoc and unused imports

8f841c7

LanguageInterface along with its usage

29e71b4

Nit changes

455d2ee

rt4914 merged commit 4711fd5 into develop Sep 19, 2019

rt4914 deleted the exploration-player-4-audio branch September 25, 2019 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #17: Exploration Player Audio Component (Part 4) #105

Fix part of #17: Exploration Player Audio Component (Part 4) #105

rt4914 commented Sep 16, 2019

rt4914 commented Sep 16, 2019

BenHenning left a comment

rt4914 commented Sep 17, 2019

BenHenning Sep 18, 2019

rt4914 Sep 18, 2019

BenHenning Sep 18, 2019

BenHenning Sep 18, 2019

rt4914 Sep 18, 2019

BenHenning Sep 18, 2019

BenHenning Sep 18, 2019

BenHenning commented Sep 18, 2019

rt4914 commented Sep 18, 2019

BenHenning left a comment

BenHenning Sep 18, 2019

rt4914 Sep 19, 2019

BenHenning left a comment

BenHenning Sep 18, 2019

rt4914 commented Sep 19, 2019

BenHenning left a comment

rt4914 commented Sep 19, 2019

Fix part of #17: Exploration Player Audio Component (Part 4) #105

Fix part of #17: Exploration Player Audio Component (Part 4) #105

Conversation

rt4914 commented Sep 16, 2019

Explanation

Checklist

rt4914 commented Sep 16, 2019

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 commented Sep 17, 2019

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

BenHenning commented Sep 18, 2019

rt4914 commented Sep 18, 2019

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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 Sep 19, 2019

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 commented Sep 19, 2019