search

forslund / spotify-skill

Mycroft Skill to control spotify using the Spotify Connect API
Apache License 2.0
71 stars 38 forks source link

fix unterminated regex in file saved_songs for local nl-nl #183

Closed Aduen closed 1 year ago

Aduen commented 1 year ago

Bug: spotify skill crashes in nl-nl locale with a "Unterminated Regex" error on any utterances involving playback. Resolve: The file ./nl-nl/saved_songs.regex contained the unterminated regex. Regex now terminated. Test: Dutch query for playing a song/band works without crashing ("Start de band Metallica op Spotify")

forslund commented 1 year ago

Looks like a good fix. Do you think there should be a space after spotify as well?

Testing in pythex I get that mijn opgeslagen nummers is matched while spotify opgeslagen nummers fails. It instead matches with spotifyopgeslagen nummers

Is that correct? If you think it should be merged as is I can do that as well!

Aduen commented 1 year ago

Hi, if I would suggest an update for the regex as a whole I might go for something like this (mijn|)( spotify)? (vind ik leuk|favorieten|opgeslagen) (nummers|liedjes)?

Translations: mijn = my vind ik leuk = liked (uncommon in dutch) favorieten = favorites opgeslagen = saved nummers = tracks/songs liedjes = songs

This would select the following bits in the Dutch sentences (and indeed handle spacing properly): speel mijn favorieten op spotify speel mijn favorieten nummers op spotify speel mijn favorieten liedjes op spotify speel mijn spotify favorieten op spotify (these ones are a bit redundant and weird but might happen in digital assistant context)

speel mijn vind ik leuk op spotify speel mijn spotify vind ik leuk op spotify speel mijn vind ik leuk nummers op spotify speel mijn vind ik leuk liedjes op spotify

speel mijn spotify opgeslagen nummers op spotify speel mijn opgeslagen nummers op spotify speel mijn opgeslagen liedjes op spotify

I am unsure how greedy (or not) the regex need to be, this I will leave to you. Let me know if you want me to send a second pull request for this.

Cheers, Aduen

Aduen commented 1 year ago

I have to come back on this last suggestion. I had to check on Spotify and noticed they already butchered most of the English to Dutch translation. I only found one phrase to work as desired. The rest moves me to public playlists. (mijn|)( spotify)? (opgeslagen) (nummers|liedjes)?

Something lexical to think about, if a command starts with 'my' it feels like it should -only- look in the user related playlists and favorites. Right now the command is being directed to publicly available playlists called "My liked songs". This could just be a nl-nl locale thing caused by poor translations on the Spotify side...

forslund commented 1 year ago

I think it's based on my original english phrase. (back then it was only looking at the users own playlists and not the public ones)

It would likely be better if we separate them as you suggested. spotify including both personal and public playlists and my including only the users personal. I'll look into it some more this weekend.

I'll merge this now, I'll be happy to merge more fixes to nl-nl but you might want to push them to https://translate.mycroft.ai/ aswell. They periodically push out translations and your change here might be overwritten.