AudioPlayer functionality (Continue PR #88) #92

Merged
merged 12 commits into from Dec 26, 2016

Conversation

Projects
None yet
4 participants
Contributor

fremail commented Dec 23, 2016

Continuing PR #88 I added app.audioPlayer(event, func)
It allows to add an event handler to these AudioPlayer events:

  • PlaybackStarted
  • PlaybackFinished
  • PlaybackStopped
  • PlaybackNearlyFinished
  • PlaybackFailed

Also this function supports async responses (similar to intent()).

wschaeferiii and others added some commits Nov 3, 2016

@wschaeferiii wschaeferiii wip, have context object appending with most props 6bdbc64
@wschaeferiii wschaeferiii added audioplayer directives and context object to enable long form a…
…udio using response object
82ec687
@wschaeferiii wschaeferiii eslint
0ace347
@wschaeferiii wschaeferiii added unit tests
d3a2488
@wschaeferiii wschaeferiii eslint
d83dc1b
@wschaeferiii wschaeferiii added more tests
fdc55c1
@fremail fremail Merge branch 'master' of git://github.com/wschaeferiii/alexa-app into…
… continue-88
e24cf01
@fremail fremail Add audioPlayer() function to define AudioPlayer event handlers
d624ee2

Coverage Status

Coverage increased (+1.9%) to 87.153% when pulling d624ee2 on fremail:continue-88 into e2b8c6c on matt-kruse:master.

@fremail fremail Update CHANGELOG.md
5faa7ad

Coverage Status

Coverage increased (+1.9%) to 87.153% when pulling 5faa7ad on fremail:continue-88 into e2b8c6c on matt-kruse:master.

@dblock

I think this is almost there, nice work. Lets finish it up, see my comments.

@@ -167,6 +167,33 @@ app.sessionEnded(function(request, response) {
});
```
+## AudioPlayer Event Request
+
+Define the handler for multiple events using multiple calls to `audioPlayer()`. You can define only one handler per event. Event handlers that don't return an immediate response (because they do some asynchronous operation) must return false.
@dblock

dblock Dec 23, 2016

Collaborator

Maybe add a link to audio player support on some amazon site to explain what this is somewhere?

@wschaeferiii

wschaeferiii Jan 17, 2017

Hey @fremail , thank you for fixing my sloppy code and adding the AudioPlayer Event Request feature!

I would love to use the feature, however having issues issues with my app listening to AudioPlayer events. When the Alexa sends a audioplayer request (I can log the request_json, so I know its sent), the try / catch block with the var requestType = request.type() isn't even firing in my code.

Am I missing something here?

@dblock

dblock Jan 17, 2017

Collaborator

@wschaeferiii I know it's a continuation of this code, but just open a new issue for any bug you encounter against 2.4.0, hopefully with a test that reproduces the problem.

@fremail

fremail Jan 19, 2017

Contributor

@wschaeferiii it's hard to suspect why it doesn't work for you. Please create an issue with a code example. Or point me to the issue please.

README.md
+## AudioPlayer Event Request
+
+Define the handler for multiple events using multiple calls to `audioPlayer()`. You can define only one handler per event. Event handlers that don't return an immediate response (because they do some asynchronous operation) must return false.
+You can define handlers for these events:
@dblock

dblock Dec 23, 2016

Collaborator

for the following events ...

And space things out a bit, add an empty line above and below.

README.md
+* PlaybackNearlyFinished
+* PlaybackFailed
+
+See example further below.
@dblock

dblock Dec 23, 2016

Collaborator

I would say "The following example [explain what this does]."

README.md
+});
+app.audioPlayer("PlaybackFinished", function(request, response) {
+ // async response
+ getNextSongFromDB(function(url, token) {
@dblock

dblock Dec 23, 2016

Collaborator

Maybe add something that actually works as long as it's a 1-liner, like fetch some audio clip from some public website?

@fremail

fremail Dec 23, 2016

Contributor

I don't have ideas what it could be

index.js
@@ -131,7 +133,67 @@ alexa.response = function(session) {
this.sessionObject.clear(key);
return this;
};
-
+ this.audioPlayerPlay = function (url, token, playBehavior, offsetInMilliseconds, expectedPreviousToken) {
@dblock

dblock Dec 23, 2016

Collaborator

Which parts are required? Sounds like it's url and playBehavior. So maybe it shouldn't be method that takes 5 parameters but only those 2 and additional options that just get merged? This way you don't need the switch(arguments.length) at all.

@fremail

fremail Dec 23, 2016

Contributor

expectedPreviousToken is required only in case of playBehavior is ENQUEUE. Otherwise we don't need it.
Other params are required in all responses: look at the table there

We can try to assign default values to the arguments and make as you wrote, but I don't sure it'd be convenient.

@dblock

dblock Dec 24, 2016

Collaborator

I still think this is not future-proof. Every time Amazon adds something to audioplay we'll be throwing more parameters in here. What do you think about leaving this to the user and not being too smart about it?

this.audioPlayerPlay = function (playBehavior, audioItem) {

}

this.audioPlayerPlayStream = function (playBehavior, stream) {
  
}
index.js
+ };
+ this.audioPlayerClearQueue = function (clearBehavior) {
+ var audioPlayerDirective;
+ if (arguments.length === 0) {
@dblock

dblock Dec 23, 2016

Collaborator

There should be a cleaner way to do this one without the big if.

audioPlayerDirective = {}
if () { ... }
self.response....
@fremail

fremail Dec 23, 2016

Contributor

Agree. I'll rewrite this function

index.js
var requestType = request.type();
if (typeof self.pre == "function") {
- self.pre(request, response, requestType);
+ self.pre(request, response, requestType, context);
@dblock

dblock Dec 23, 2016

Collaborator

I think pre shouldn't take the 4th parameter since request.context is a thing. You can access that inside pre.

@fremail

fremail Dec 23, 2016

Contributor

It was added by @wschaeferiii. Really I don't know reasons to add it.

@dblock

dblock Dec 23, 2016

Collaborator

Lets remove code that we don't know why it's there :)

@fremail fremail Simplify app.audioPlayerClearQueue() function code
Remove fourth argument of app.pre() function
Update README
a42d46c

Coverage Status

Coverage increased (+0.3%) to 85.512% when pulling a42d46c on fremail:continue-88 into e2b8c6c on matt-kruse:master.

fremail added some commits Dec 26, 2016

@fremail fremail Move legacy code in alexa.response at the end b0fb1c1
@fremail fremail Update response.audioPlayerPlay function to get only 2 arguments
Add response.audioPlayerPlayStream function as a helper to response.audioPlayerPlay function
Update README
acd32a8

Coverage Status

Coverage decreased (-0.2%) to 85.091% when pulling acd32a8 on fremail:continue-88 into e2b8c6c on matt-kruse:master.

Contributor

fremail commented Dec 26, 2016

Interesting.. when you expand readme file, test coverage decreases.

dblock merged commit cb4023f into alexa-js:master Dec 26, 2016

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 85.091%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Collaborator

dblock commented Dec 26, 2016

Merge, excellent work @fremail and @wschaeferiii.

Collaborator

dblock commented Feb 7, 2017

Btw, just used this in artsy/elderfield#56 and it worked out beautifully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment