Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-13150] Added support for prepareAsync #4220

Closed
wants to merge 5 commits into from

Conversation

salachi
Copy link
Contributor

@salachi salachi commented Apr 29, 2013

@@ -181,19 +187,7 @@ public void play()
initialize();
}

if (mp != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this part is deleted, the sound won't play again after the sound is paused or stopped. Please test with KS->Phone->Sound.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch, I missed testing the 'pause'. I will fix this

@salachi
Copy link
Contributor Author

salachi commented May 2, 2013

Based on the review feedback, fixed the issue with 'pause'


private void startPlaying()
{
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the play() method, initialize() and startPlaying() are already in a try-catch block, so the try-catch here is not necessary. Just need to put startPlaying() in a try-catch block in the onPrepared() method.

@ghost ghost assigned pingwang2011 May 6, 2013
@pingwang2011
Copy link
Contributor

Since you add an Async call to prepare the media player, the states of the player have to be set in the right place. Otherwise, it will mess up the other sync methods, like stop and pause.
For example, setState(STATE_INITIALIZED) should be called in different places for sync and async preparation. If the users stop or pause the audio before the callback of the async method, the audio should not play at all. But that's not the case in this implementation. I ran the test case in the ticket, the audio still plays although it calls audioPlayer.stop() in the app.js.
BTW, after your change, it is more appropriate to rename the method initialize() to something like initializeAndPlay().

@billdawson
Copy link
Contributor

Ping's comments have been addressed. However the functional test fails. I'm using this test:

https://jira.appcelerator.org/browse/TIMOB-13150?focusedCommentId=265017&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-265017

After calling stop(), there is a pause and then the stream starts again. It's because of the prepare listener (stop() calls .prepare).

@billdawson
Copy link
Contributor

The re-start problem is fixed. However, stop() is holding up the thread (like start() was in the original complaint.) After stopping, it calls .prepare rather than .prepareAsync even when the source is remote. The remote variable is set and available at that point, so I think you can use it to determine whether to call .prepareAsync.

@salachi
Copy link
Contributor Author

salachi commented Aug 21, 2013

Added new pull request #4593

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants