Fix stop vocab only working for common play usage #109
Conversation
…requires a change to the common play framework
Message for converse debugging was logged every time the News Skill was active and an utterance was made. Hence converse wasn't confused, it is working as intended.
This isn't explicitly necessary to stop the audio playback however it is how the current VK test steps verify that the audio has stopped, and it shouldn't hurt. Remaining @xfail tests are caused by the Timer Skill over-matching
There was a problem hiding this comment.
This is where the next PR I submit is heading. In many simple cases the converse() method is unnecessary and state transitions should be handled by the skill base class driven off a state/intent data structure. BTW, are you sure this works as expected? If you set an alarm for 2 minutes in the future and then play the news, when the alarm goes off while the news is playing and you say stop, does the news continue to play or does it stop also?
|
Nope it does the current Mycroft standard behaviour of stopping everything. But if we're trying to stop the Alarm/Timer and not the news - shouldn't we use converse on those rather than this one? |
|
Yea, that's why I figured we should hold off on this. I have a PR going in today which addresses the issue. |
Description
An alternative to PR #108 - this adds Skill specific "stop" vocab that is only registered when the Skill is performing it's own local playback ie not through the Common Play Framework.
As the intent is only active while the News Skill is playing, it is quite liberal with it's matching at the moment.
This also calls
audioservice.stop()to allow the VK tests to pass. Remaining @xfail's are caused by the Timer Skill matching on Stop utterances when it has no alarms set or expired.I don't believe this local vocab is a long term solution but we first need better contextual awareness and a unified playback system.
Type of PR
Testing
VK tests