rickyphewitt / emby-skill

This skill allows audio playback from an emby server
11 stars 19 forks source link

Update emby.intent #46

Closed mexican1973 closed 4 years ago

mexican1973 commented 5 years ago

added different variations I have seen for emby, will do changes also in German and Spanish

rickyphewitt commented 5 years ago

This is awesome! I've been wanting to provide a more natural and dynamic way to talk to mycroft and request music. I've pulled it down but was not able to use the new patterns. For example when on master I can ask mycroft: "play {artist/album/song} from emby" and an instant mix will start playing. When I do the same after I incorporate the changes the skill is unable to find music to play.

Example logs for master: 06:53:02.965 | INFO | 7386 | Emby | {'media': 'wage war', 'utterance': 'play wage war from emby'} 06:53:03.996 | INFO | 7386 | emby-skill.emby_client | ?SearchTerm=wage war 06:53:04.011 | INFO | 7386 | emby-skill.emby_croft | Instant Mix potential match: Wage War

Example logs for the proposed changes: 06:46:03.334 | INFO | 6432 | Emby | Play wage war from emby 06:46:03.335 | INFO | 6432 | root | phrase: Play wage war from emby 06:46:03.335 | INFO | 6432 | emby-skill.emby_client | ?SearchTerm=play wage war from emby Removing event mycroft-playback-control.mycroftai:PlayQueryTimeout Removing event mycroft-playback-control.mycroftai:PlayQueryTimeout 06:46:03.644 | INFO | 6432 | Playback Control Skill | No matches

It looks like the key difference is that the search term includes the additional 'from emby' which fails to be found. The skill code extracts the item within the {media} part of the intent and it appears that part now includes more than just the 'media' (for lack of a better term).

Another unfortunate thing is that all the 'live' unit tests pass. Which means there is not adequate coverage around this part of the skill. The good thing is that now that we know there is a hole we can add tests to verify this usecase (and hopefully add mocked ones that run during ci)

I think we need to explore more about the intent syntax as from what I see (and my limited knowledge of how it work) I would expect it to still work.

ChanceNCounter commented 5 years ago

I'm very much just taking a shot in the dark, but I'm guessing that the patterns expand into too many options, causing some parser or another to give up.

I don't have an Emby server, so I can't test, but I'd try commenting out all but one line in the intent and see if it starts working again.

mexican1973 commented 5 years ago

I just tested again and commented out the lines for song and artist. The album is found quiet nicely. Need to test some more. Its finding my albums form my emby server. Thats already really great. Only thing is i will have to rename the names a bit because sometimes they are difficult to interpret for the STT engine.

rickyphewitt commented 5 years ago

@ChanceNCounter,

I was thinking something similar.

@mexican1973,

Great! Keep me updated on the progress and let me know if you have any questions or if I can help. I'm really looking forward to the new intents :+1: