You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by ghenry22 <gi...@git.apache.org> on 2017/02/22 04:30:42 UTC
[GitHub] cordova-plugin-media pull request #130: CB-8098 & CB-7810:(android) Added me...
GitHub user ghenry22 opened a pull request:
https://github.com/apache/cordova-plugin-media/pull/130
CB-8098 & CB-7810:(android) Added media rate for android
### Platforms affected
Android
### What does this PR do?
adds set rate function for android
this already exists on iOS
there are long running issues in jira for this functionality
### What testing has been done on this change?
tested in my own project, playback rate audibly changes. There are already automated tests for playback rate, this just adds another platform.
### Checklist
- [X ] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
- [X ] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
- [X ] Added automated test coverage as appropriate for this change.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/altaf933/cordova-plugin-media master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-media/pull/130.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 #130
----
commit e4e8a714bceaa58ed93b96bd69153adf5bd0975a
Author: altaf <al...@gmail.com>
Date: 2017-02-10T09:45:26Z
Added media rate for android verion 6.0
----
---
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 #130: CB-8098 & CB-7810:(android) Added me...
Posted by dellagustin <gi...@git.apache.org>.
Github user dellagustin commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-media/pull/130#discussion_r120745011
--- Diff: src/android/AudioPlayer.java ---
@@ -532,6 +532,27 @@ public void setVolume(float volume) {
}
}
+ /**
+ * Set the rate for audio player
+ *
+ * @param rate
+ */
+
+ public void setRate(float rate) {
+ if (this.player != null) {
+
+ if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
--- End diff --
I'm not sure if there is a guideline here, or what is the standard behavior in this cases, but should whe not send an error stats or something when the version does not meet the minimum requirement?
---
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 issue #130: CB-8098 & CB-7810:(android) Added media rat...
Posted by dellagustin <gi...@git.apache.org>.
Github user dellagustin commented on the issue:
https://github.com/apache/cordova-plugin-media/pull/130
@ghenry22, I'm interested on using this feature as well in android, but I think some adaptations on the code are needed, can you take a look a my comments?
---
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 issue #130: CB-8098 & CB-7810:(android) Added media rat...
Posted by ghenry22 <gi...@git.apache.org>.
Github user ghenry22 commented on the issue:
https://github.com/apache/cordova-plugin-media/pull/130
I haven't tested this yet, just found the PR while looking through various forks.
I plan to test to mAke sure this is consistent with iOS and then will update here with suggestion to merge or not.
---
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 issue #130: CB-8098 & CB-7810:(android) Added media rat...
Posted by cordova-qa <gi...@git.apache.org>.
Github user cordova-qa commented on the issue:
https://github.com/apache/cordova-plugin-media/pull/130
Cordova CI Build has completed successfully.
**Commit** - [Link](https://github.com/apache/cordova-plugin-media/pull/130/commits/e4e8a714bceaa58ed93b96bd69153adf5bd0975a)
**Dashboard** - [Link](http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100/)
| Builder Name | Console Output | Test Report | Device Logs |
| :---: | :---: | :---: | :---: |
| [Windows 10 Store]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=windows-10-store/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=windows-10-store/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=windows-10-store/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=windows-10-store/artifact/) |
| [iOS 9.3]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=ios-9.3/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=ios-9.3/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=ios-9.3/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=ios-9.3/artifact/) |
| [iOS 10.0]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=ios-10.0/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=ios-10.0/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=ios-10.0/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=ios-10.0/artifact/) |
| [Android 4.4]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=android-4.4/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=android-4.4/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=android-4.4/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=android-4.4/artifact/) |
| [Android 5.1]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=android-5.1/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=android-5.1/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=android-5.1/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-media-pr/100//PLATFORM=android-5.1/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 issue #130: CB-8098 & CB-7810:(android) Added media rat...
Posted by dellagustin <gi...@git.apache.org>.
Github user dellagustin commented on the issue:
https://github.com/apache/cordova-plugin-media/pull/130
@filmaj @AshishMehra I created pull request https://github.com/apache/cordova-plugin-media/pull/142 to document the existing setRate method for iOS platform.
---
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 #130: CB-8098 & CB-7810:(android) Added me...
Posted by dellagustin <gi...@git.apache.org>.
Github user dellagustin commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-media/pull/130#discussion_r119812195
--- Diff: www/Media.js ---
@@ -173,7 +173,9 @@ Media.prototype.setRate = function(rate) {
if (cordova.platformId === 'ios'){
--- End diff --
I think some polishing would be good here:
1. As the current code is, the if/else is not necessary, as both blocks execute the exact same instruction
2. This pull request says it enables the set rate for Android, but the else block is now executed for any other platform then iOS, this looks wrong. Am I missing something?
It looks to me the result should look like:
```javascript
if (cordova.platformId === 'ios' || cordova.platformId === 'android'){
exec(null, null, "Media", "setRate", [this.id, rate]);
} else {
console.warn('media.setRate method is currently not supported for', cordova.platformId, 'platform.');
}
```
---
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 #130: CB-8098 & CB-7810:(android) Added me...
Posted by dellagustin <gi...@git.apache.org>.
Github user dellagustin commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-media/pull/130#discussion_r120737624
--- Diff: src/android/AudioPlayer.java ---
@@ -271,7 +272,6 @@ public void stopRecording(boolean stop) {
this.moveFile(this.audioFile);
} else {
LOG.d(LOG_TAG, "pause recording");
- this.setState(STATE.MEDIA_PAUSED);
--- End diff --
This does not look related to changing the playback rate, was possibly removed by mistake, or aimed as part of solving a different issue?
---
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 issue #130: CB-8098 & CB-7810:(android) Added media rat...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-plugin-media/pull/130
@ghenry22 any updates on your testing?
On the one side, I am not super keen on adding undocumented functionality, on the other, this already exists in iOS....
---
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 issue #130: CB-8098 & CB-7810:(android) Added media rat...
Posted by AshishMehra <gi...@git.apache.org>.
Github user AshishMehra commented on the issue:
https://github.com/apache/cordova-plugin-media/pull/130
hey there @ghenry22, how do i increase/decrease the rate for both iOS and Android? There is no documentation on this feature. Thanks!
---
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