Moves media player related functionalities to Media Widget class #2817

shobhitagarwal1612 · 2019-01-23T21:12:32Z

What has been done to verify that this works as intended?

Tested manually on Redmi Note 4 and compared the All-Widgets and Birds forms before and after the changes.

Tests performed:

Swiping next while the audio is running stops the audio
Text color changes to tint color (blue) when playing audio
Minimizing the app while the audio is running stops the audio
Select Widget / Grid Widget / Grid Multi Widget and Audio Widgets are functioning properly

Why is this the best possible solution? Were any other approaches considered?

This PR is aimed at moving all of the media player related functionalities into an abstract class MediaWidget. This also performs minor refactors such as conversion of anonymous inner classes into lambda and minor code cleanup. This reduces the size of QuestionWidget by 100 lines.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This PR changes the way layout which contains the question text and other media (audio, video, image) is initialized and the methods controlling media are moved to another class. So, all of the changes must be closely reviewed in order to prevent any regressions.

Do we need any specific form for testing your changes? If so, please attach one.

I tested using the All-Widgets and Birds form

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No

Before submitting this PR, please make sure you have:

run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

The purpose of this class would be to pull out all functionality related to media player

Audio, grid, grid-multi and select widgets require media player. So the plan is to extend MediaWidget in these classes and in later commits, start moving the media related methods and fields into MediaWidget

The super call is redundant as of now, but it is better than removing the method from QuestionWidget class

Additionally, a check had to be added to ODKView to check if the widget is of type MediaWidget before performing autoplay during loading of the widget

1. Adds autoplay parameter to playAllPromptText() 2. Removes separate methods for playing audio/video and uses the above parameter to distinguish between type of autoplay Since the name of method playAllPromptText() is generic, so it should behave in a generic manner as well. Until now, it was only concerned with autoplaying audio, while giving an impression that it is common method for all kinds of prompts (audio/video)

@nonnull

Remove unused getter. Since the member variable is only being used within the same class, so the getters can be dropped Annotate methods of GuidanceHint as @nonnull to remove nullable warning. Since it is an enum & the default value of GuidanceHint should be 'No' instread of 'null', so it is safe to return 'No' and mark method as @nonnull

grzesiek2010 · 2019-02-05T13:22:06Z

I reviewed and tested your changes since I want to continue work on #2521 and it would be better to merge this pr firs.

I have a problem with the autoplay option... did you test it? If not here is a form:
https://drive.google.com/file/d/14Pcu8hdE7gBqmFXpbPi6KW5779InmiWP/view?usp=sharing

it crashes:

    java.lang.NullPointerException: Attempt to invoke virtual method 'boolean android.media.MediaPlayer.isPlaying()' on a null object reference
        at org.odk.collect.android.views.AudioButton.onClick(AudioButton.java:77)
        at org.odk.collect.android.views.MediaLayout.playAudio(MediaLayout.java:114)
        at org.odk.collect.android.widgets.MediaWidget.playAllPromptText(MediaWidget.java:112)
        at org.odk.collect.android.widgets.SelectWidget.playAllPromptText(SelectWidget.java:98)
        at org.odk.collect.android.views.ODKView.lambda$new$0$ODKView(ODKView.java:226)
        at org.odk.collect.android.views.ODKView$$Lambda$0.run(Unknown Source)
        at android.os.Handler.handleCallback(Handler.java:739)
        at android.os.Handler.dispatchMessage(Handler.java:95)
        at android.os.Looper.loop(Looper.java:145)
        at android.app.ActivityThread.main(ActivityThread.java:5951)
        at java.lang.reflect.Method.invoke(Native Method)
        at java.lang.reflect.Method.invoke(Method.java:372)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1400)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1195)

but it's not your fault since it crashes on master branch as well and on older versions too...
We have no crash reports from Firebase and no reports from real users what indicates it's completely useless. Can we just get rid of that option @yanokwa @shobhitagarwal1612
it doesn't make sense to me...

shobhitagarwal1612 · 2019-02-06T05:23:24Z

@grzesiek2010 No, I didn't. I have listed down all of the tests performed manually in the PR description.

dcbriccetti · 2019-02-06T22:40:35Z

collect_app/src/main/java/org/odk/collect/android/widgets/SelectWidget.java

-        });
-        // plays the question text
-        super.playAllPromptText();
+    public void playAllPromptText(String playOption) {


Hi Shobhit. Did you consider using an enum for playOption?

dcbriccetti · 2019-02-06T22:55:58Z

collect_app/src/main/java/org/odk/collect/android/widgets/MediaWidget.java

    public MediaWidget(Context context, FormEntryPrompt prompt) {
        super(context, prompt);
+
+        String imageURI = this instanceof SelectImageMapWidget ? null : prompt.getImageText();


Not sure, but I think we’re using the convention that initialisms (like URI) are capitalized like uri or imageUri. I have an audio clip of Josh Bloch himself recommending this.

dcbriccetti · 2019-02-06T22:59:22Z

collect_app/src/main/java/org/odk/collect/android/widgets/MediaWidget.java

+            try {
+                playColor = Color.parseColor(playColorString);
+            } catch (IllegalArgumentException e) {
+                Timber.e(e, "Argument %s is incorrect", playColorString);


Argument %s to Color.parseColor is incorrect might be more helpful.

I see you didn’t create, but just moved this code, so disregard the above if you like.

dcbriccetti · 2019-02-06T23:05:10Z

collect_app/src/main/java/org/odk/collect/android/widgets/QuestionWidget.java


        expanded = new AtomicBoolean(false);

        if (getState() != null) {
            if (getState().containsKey(GUIDANCE_EXPANDED_STATE + getFormEntryPrompt().getIndex())) {
-                Boolean result = getState().getBoolean(GUIDANCE_EXPANDED_STATE + getFormEntryPrompt().getIndex());
+                boolean result = getState().getBoolean(GUIDANCE_EXPANDED_STATE + getFormEntryPrompt().getIndex());
                expanded = new AtomicBoolean(result);


I’m suspicious of an AtomicBoolean that’s recreated rather than set. I didn’t study this closely though.

yanokwa · 2019-02-06T23:37:32Z

Autoplay seems to be in use: see https://forum.opendatakit.org/search?q=autoplay%20order%3Alatest). I also think it's useful for ACASI surveys. @grzesiek2010 Can you file an issue so we can discuss what the current behavior is and how we can fix and document it?

mmarciniak90 · 2019-02-08T11:07:52Z

Tested with success

Verified on Android: 4.2, 4.4, 5.1, 6.0, 7.0, 8.1
I did not notice regression.

Verified cases:

all audio widgets in All widgets form
all video widgets in All widgets form
all image widgets in All widgets form
Birds form
widgets on one page
permissions

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

grzesiek2010 · 2019-02-11T12:14:16Z

Ohhhh I feel ashamed that I didn't catch it before but of course, all widgets can have media attached to a question text... @shobhitagarwal1612 your changes brake that option what shows this form:
form.zip

are you able to come up with any reasonable solution because I think reverting all changes might be the best solution here?

Yes since all widgets can have media files the whole issue was wrong...

…ass (#2817)" This reverts commit 8db197e.

…ass (#2817)" (#2862) This reverts commit 8db197e.

yanokwa · 2019-02-12T20:15:29Z

I discussed this a bit with @grzesiek2010 and wanted to document some of the gaps so we can get better.

@dcbriccetti's issue was a little misleading. Perhaps there could have been more details in said issue.
@shobhitagarwal1612 was probably not aware that question text had media. Perhaps a closer look at ODKView would have helped.
Grzegorz might have caught it during review, but Dave approved the changes before and so Grzegorz didn't go back to review.
@mmarciniak90 might have caught it during testing, but didn't think to test those.
@yanokwa commented on the PR, but didn't carefully read the tests performed. There was a big hint there in the Shobhit's list.

I think one thing that we can all do going forward is to let PRs sit a little bit before going into testing. My hope here is that more time will give more people a chance to review. Another thing we can do is to aim for having two people give the go ahead on a merge.

Do these two adjustments sound reasonable for preventing this kind of problem?

shobhitagarwal1612 added 14 commits January 24, 2019 02:14

Adds new abstract class, MediaWidget

5920f83

The purpose of this class would be to pull out all functionality related to media player

Extend widgets with Media Widget

dae5f5c

Audio, grid, grid-multi and select widgets require media player. So the plan is to extend MediaWidget in these classes and in later commits, start moving the media related methods and fields into MediaWidget

Override release() in MediaWidget

7e47345

The super call is redundant as of now, but it is better than removing the method from QuestionWidget class

Move getter for media player to MediaWidget

85d7d43

Move stopAudio() to MediaWidget

199e169

Move playAudio() and playVideo()

ed8f245

Additionally, a check had to be added to ODKView to check if the widget is of type MediaWidget before performing autoplay during loading of the widget

Separate init of textview and media views

064dc34

Init media and playColor in MediaWidget

facfe12

Implement AudioPlayListener in MediaWidget

e5c59b2

Move mediaPlayer init to MediaWidget

f67b2dc

Adds license header

27dba1b

checkstyle fixes

44f5479

yanokwa requested a review from dcbriccetti February 6, 2019 05:15

dcbriccetti reviewed Feb 6, 2019

View reviewed changes

dcbriccetti approved these changes Feb 6, 2019

View reviewed changes

yanokwa added the needs testing label Feb 6, 2019

shobhitagarwal1612 added 2 commits February 8, 2019 08:33

Improve log text message

5d91481

Fix naming convention from varNAME to varName

2dec895

mmarciniak90 mentioned this pull request Feb 8, 2019

Icons next to draw options are not visible after first extend the menu #2858

Closed

getodk-bot added behavior verified and removed needs testing labels Feb 8, 2019

grzesiek2010 merged commit 8db197e into getodk:master Feb 8, 2019

grzesiek2010 added this to the v1.20 milestone Feb 8, 2019

grzesiek2010 mentioned this pull request Feb 8, 2019

Autoplay option doesn't work #2861

Closed

shobhitagarwal1612 deleted the issue-1640 branch February 8, 2019 11:41

shobhitagarwal1612 added a commit that referenced this pull request Feb 11, 2019

Revert "Moves media player related functionalities to Media Widget cl…

962690c

…ass (#2817)" This reverts commit 8db197e.

shobhitagarwal1612 mentioned this pull request Feb 11, 2019

Revert "Moves media player related functionalities to Media Widget class" #2862

Merged

grzesiek2010 pushed a commit that referenced this pull request Feb 11, 2019

Revert "Moves media player related functionalities to Media Widget cl…

c90a8c9

…ass (#2817)" (#2862) This reverts commit 8db197e.

seadowg mentioned this pull request Jul 23, 2019

Audio playback refactor #3229

Merged

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moves media player related functionalities to Media Widget class #2817

Moves media player related functionalities to Media Widget class #2817

shobhitagarwal1612 commented Jan 23, 2019 •

edited

Loading

grzesiek2010 commented Feb 5, 2019 •

edited

Loading

shobhitagarwal1612 commented Feb 6, 2019

dcbriccetti Feb 6, 2019

dcbriccetti Feb 6, 2019

dcbriccetti Feb 6, 2019 •

edited

Loading

dcbriccetti Feb 6, 2019

dcbriccetti Feb 6, 2019

yanokwa commented Feb 6, 2019 •

edited

Loading

mmarciniak90 commented Feb 8, 2019

grzesiek2010 commented Feb 11, 2019 •

edited

Loading

yanokwa commented Feb 12, 2019 •

edited

Loading

Moves media player related functionalities to Media Widget class #2817

Moves media player related functionalities to Media Widget class #2817

Conversation

shobhitagarwal1612 commented Jan 23, 2019 • edited Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

grzesiek2010 commented Feb 5, 2019 • edited Loading

shobhitagarwal1612 commented Feb 6, 2019

dcbriccetti Feb 6, 2019

Choose a reason for hiding this comment

dcbriccetti Feb 6, 2019

Choose a reason for hiding this comment

dcbriccetti Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

dcbriccetti Feb 6, 2019

Choose a reason for hiding this comment

dcbriccetti Feb 6, 2019

Choose a reason for hiding this comment

yanokwa commented Feb 6, 2019 • edited Loading

mmarciniak90 commented Feb 8, 2019

grzesiek2010 commented Feb 11, 2019 • edited Loading

yanokwa commented Feb 12, 2019 • edited Loading

shobhitagarwal1612 commented Jan 23, 2019 •

edited

Loading

grzesiek2010 commented Feb 5, 2019 •

edited

Loading

dcbriccetti Feb 6, 2019 •

edited

Loading

yanokwa commented Feb 6, 2019 •

edited

Loading

grzesiek2010 commented Feb 11, 2019 •

edited

Loading

yanokwa commented Feb 12, 2019 •

edited

Loading