Feature/allow master control

pmulh commented 2 years ago

Description

Two separate changes:

Enabling "what song is this?"/"What are we listening to?" functionality, that was mostly already implemented, but commented out...? It seems to all work well (#7 )
Adding option to allow mycroft to control playback on other spotify devices, if allow_master_control is set to true in settings.json (e.g. 'play the artist the beatles' will start playback on the primary device; 'transfer spotify to ' will transfer playback, then 'pause'/'next track' etc will control playback on the device that playback was transferred to).

Type of PR

If your PR fits more than one category, there is a high chance you should submit more than one PR. Please consider this carefully before opening the PR. Either delete those that do not apply, or add an x between the square brackets like so: - [x]

[ ] Bugfix
[X] Feature implementation
[ ] Refactor of code (without functional changes)
[ ] Documentation improvements
[ ] Test improvements

Testing

These changes have only been tested on picroft; if anyone can test them on a Mark1 or Mark2, that would be good.

Documentation

Does documentation for this already exist? Are there docstrings on all the public methods you added or modified? Have these been updated?

forslund commented 2 years ago

Hi, sorry for the late reply.

I need to dig into and try it out properly but in general I think it looks good.

The thing I'm wondering about is the queries for asking about what's playing. It was my intention that the queries would only be available if music was actually playing...I need to check if that was working as intended in the first place.

forslund commented 2 years ago

I've just refreshed my memory and the "what song is this / what are we listening to" intents are currently enabled when the spotify skill is playing. Maybe you can change it to be default on when "master control" is enabled, and keep it as it is (enabling on play, disabling on stop)

pmulh commented 2 years ago

Sorry, I forgot to reply to your previous comment. Thanks for the clarification of how the intents are currently enabled/disabled, I hadn't appreciated that. I'll have a look at changing the default when "master control" is enabled like you suggested

pmulh commented 2 years ago

I pushed another commit there to address the issue you pointed out about how the "what song is this" etc intents are enabled/disabled. Now it should be that when allow_master_control is true, they aren't disabled when music stops playing, but are when allow_master_control is false and music stops (i.e. no change).

When I was testing it I did notice that currently (so with allow_master_control == False), those intents are disabled if the music is paused, but aren't re-enabled when the music is resumed. So I added a call to enable_playing_intents() in the resume() function to address that (assuming that these intents should always be enabled when music is playing).

The changes in song_info(), artist_info(), album_info() are to prevent mycroft saying something like "we're listening to None by None" if allow_master_control is True but nothing is currently playing (it will say the NothingPlaying dialog instead).

pmulh commented 2 years ago

Sorry that it has taken me a long time to get back to you on this. I still need to do more trials but I'll post the few comments / questions I had right now.

No worries, thanks for those comments. I've addressed those in the latest commits.

Minor additional point: one thing I noticed when doing some testing was that currently (in this branch and in 21.02 I believe) if you do something like "transfer spotify to Device2", while the playback control intents (pause, next track etc) will no longer work, the "what song is this" intents will still work, since the self.spotify.is_playing() check on line 383 checks for playback on any device. Personally I don't think this is an issue, but just mentioning it in case this isn't the intended functionality.

pmulh commented 2 years ago

Sorry to completely rework the changes mid-pull request, but I was having another look at the code and doing some more testing after your comments last week, and noticed a couple of areas that could be improved. I think the most recent commit is a much more robust set of changes than the previous commit.

The new changes use the current_playback() spotipy function to check what the current playing or most recently played (with a cutoff of about 10 minutes after playback is paused) device is, which avoids weird issues where the skill is trying to pause/resume/skip track etc on the wrong device. This only applies when that allow_master_control config is set to True.

The changes when allow_master_control are False should be very minimal - a minor bugfix or two, and I added return statements to prev_track() so that it matches the structure of next_track().

Also I realise that this is probably not a particularly high priority enhancement for the skill, so I understand if you don't want to incorporate these changes right now. They're very useful for my particular setup, but may not be as useful to others.

forslund commented 2 years ago

Hey I think this is a valid use case and I'd love to merge it. I think your changes looks reasonable and will do quick verification to make sure the default case is functional. After that I will merge since this looks fairly good. There may be issues like the comment I made on the stop signal. Fixing it can wait since it shouldn't affect the normal control.

forslund commented 2 years ago

Seems to be working fine, will be merging it in it's current state. It would be great if you can do a follow up PR with some sort of addition to the README.md to explain the feature.

pmulh commented 2 years ago

Seems to be working fine, will be merging it in it's current state. It would be great if you can do a follow up PR with some sort of addition to the README.md to explain the feature.

Great, thanks for merging. Yes, I'll do a follow-up with an explanation and a fix for that issue on the stop signal you mentioned.

forslund / spotify-skill