You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by fafaman <gi...@git.apache.org> on 2016/04/14 17:20:11 UTC
[GitHub] cordova-plugin-media pull request: Protect setVolume call in case ...
GitHub user fafaman opened a pull request:
https://github.com/apache/cordova-plugin-media/pull/93
Protect setVolume call in case Player object is referenced but not cr…
Hi, this is a fix for https://issues.apache.org/jira/browse/CB-11086. thanks.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/fafaman/cordova-plugin-media CB-11086cordova-plugin-media
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-media/pull/93.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #93
----
commit 930fe187e0a2385ce9f693f8fc92b465c04e13ac
Author: Fabrice Lebas <fa...@gmail.com>
Date: 2016-04-14T15:14:00Z
Protect setVolume call in case Player object is referenced but not created yet
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-media pull request #93: Protect setVolume call in case Player...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-media/pull/93
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-media pull request: Protect setVolume call in case ...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-plugin-media/pull/93#issuecomment-211529912
Should you instead use the ```readyPlayer(...)``` followed by seeking to the end of the file in this case ? It seems like a pattern used by ```startPlaying(...)``` and ```seekToPlaying(...)```.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-media pull request: Protect setVolume call in case ...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-plugin-media/pull/93#issuecomment-212122345
@riknoll to recommend on how to handle Android activity restart when ['configchanges' ](http://developer.android.com/guide/topics/resources/runtime-changes.html#HandlingTheChange)happen.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-media pull request: Protect setVolume call in case ...
Posted by fafaman <gi...@git.apache.org>.
Github user fafaman commented on the pull request:
https://github.com/apache/cordova-plugin-media/pull/93#issuecomment-211824714
@nikhilkh I do not have a great Java knowledge, so I don't know if it better performance-wise to use the if(). I am used to the python approach of using exceptions. I can change if this is required for upstreaming.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-media pull request: Protect setVolume call in case ...
Posted by fafaman <gi...@git.apache.org>.
Github user fafaman commented on the pull request:
https://github.com/apache/cordova-plugin-media/pull/93#issuecomment-211828264
@omefire I am not too keen on seeking to the end of file: Is this module not supposed to be able to play a stream? To be clear, this patch was done while looking after a problem of codrova app restarting when the phone is plugged or unplugged from a car audio stand. I seams that an event telling the app the audio system has changed provoques that app restart. I first thought the app restart was due to that null pointer exception, but once fixed, the app still restarts... I decided to propose the patch (as well as CB-11085) so this work is not lost, and code cleaner for going forward. In my opinion, the real problem comes from before during the player creation: we should not have this.players.get(id) returning a non null while the player is null.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-media pull request: Protect setVolume call in case ...
Posted by cordova-qa <gi...@git.apache.org>.
Github user cordova-qa commented on the pull request:
https://github.com/apache/cordova-plugin-media/pull/93#issuecomment-219890980
Cordova CI Build has completed successfully.
**Commit** - [Link](https://github.com/apache/cordova-plugin-media/pull/93/commits/930fe187e0a2385ce9f693f8fc92b465c04e13ac)
**Dashboard** - [Link](http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4/)
| Builder Name | Console Output | Test Report | Device Logs |
| :---: | :---: | :---: | :---: |
| [Windows 8.1 Store]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-8.1-store/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-8.1-store/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-8.1-store/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-8.1-store/artifact/) |
| [Windows 10 Store]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-10-store/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-10-store/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-10-store/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-10-store/artifact/) |
| [Windows 8.1 Phone]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-8.1-phone/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-8.1-phone/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-8.1-phone/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=windows-slave,platformName=windows-8.1-phone/artifact/) |
| [iOS]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=mac-slave,platformName=ios/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=mac-slave,platformName=ios/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=mac-slave,platformName=ios/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=mac-slave,platformName=ios/artifact/) |
| [Android Mac]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=mac-slave,platformName=android/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=mac-slave,platformName=android/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=mac-slave,platformName=android/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/4//label=mac-slave,platformName=android/artifact/) |
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-media pull request: Protect setVolume call in case ...
Posted by nikhilkh <gi...@git.apache.org>.
Github user nikhilkh commented on the pull request:
https://github.com/apache/cordova-plugin-media/pull/93#issuecomment-211453937
Instead of catching the `NullPointerException`, should you test for `if(this.player)` - I'm assuming that's what is null in this scenario.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-media pull request: Protect setVolume call in case ...
Posted by riknoll <gi...@git.apache.org>.
Github user riknoll commented on the pull request:
https://github.com/apache/cordova-plugin-media/pull/93#issuecomment-212146985
@fafaman that does sound like a `configChanges` issue as Nikhil mentioned. To prevent the restart, you need to add the `uiMode` value in the `configChanges` attribute of the Activity declaration in AndroidManifest.xml where we declare [the rest of them](https://github.com/apache/cordova-android/blob/master/bin/templates/project/AndroidManifest.xml#L40). You will find that file in the platforms folder of your project:
```
<project-root>/platforms/android/AndroidManifest.xml
```
The best way to make that change is by using a hook or a plugin (we recommend that you don't edit any files in the platform directory directly).
If you want to actually respond to that event (e.g. to change your UI to a car-friendly UI), you'll need to make a plugin that surfaces it to the javascript. We provide a [callback](https://github.com/apache/cordova-android/blob/master/framework/src/org/apache/cordova/CordovaPlugin.java#L386) to plugins for configuration changes so that would be pretty simple but would require Java code.
A note about the implications of making that change:
By default, Android just restarts the activity when a configuration changes because that is the easiest way to ensure that all of the resources and other things that need to be swapped out will automatically be taken care of by the OS. Cordova already handles some configurations, like orientation changes. Adding that `uiMode` value will prevent the restart in this scenario, but means that any changes that need to happen because of the configuration change won't happen automatically. I don't really have a way of testing the implications of that so I honestly can't say if there is any issue with adding it. You'll need to test it for yourself to make sure nothing weird happens in that scenario.
Read more about configuration changes [here](http://developer.android.com/guide/topics/resources/runtime-changes.html)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org