Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Echo device dependent default players #121

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

stridger
Copy link

Hi!

I'm not sure if you are interested in my patches here, but basically they do the following:

  • Close off Play command not working #81 by introducing a PlayIntent
  • Add some more utterances such as "power on" and "power off"
  • Allow the default player to be dependent on the Echo device that is used to send the command:
    -- The way this is done, since we do need to store a bit of data, but I didn't want to open up the whole can of worms with DynamoDB etc, it creates a container called "squeeze-alexa" in the Squeezebox favourites, and saves the mappings in there whenever one says "Tell squeezebox I am in the Living Room" or "Tell squeezebox I am using the Living Room player" etc.
    -- One thing to bear in mind is that it is using the Echo deviceId which will change every time the skill is disabled and enabled or the Echo device is removed and re-paired with the account. When this happens one would need to walk around the rooms again and tell Alexa which player corresponds to which room.
    -- Once the definition has happened, the default player is now always pre-set to the player in the room, however in rooms where no default has been set, it will continue to function as before (ie either using the last one in an active session or revert to the global default)
    -- I am neither a Python or AWS expert, so I am sure there are bugs and ugly code etc. I would be very grateful for any input etc going forwards.

Thank you for this awesome project!

* Allow voice deletions of default echo/player maps
* Code simplification
@coveralls
Copy link

Pull Request Test Coverage Report for Build 224

  • 66 of 85 (77.65%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.3%) to 93.755%

Changes Missing Coverage Covered Lines Changed/Added Lines %
squeezealexa/main.py 35 37 94.59%
squeezealexa/squeezebox/server.py 30 47 63.83%
Totals Coverage Status
Change from base Build 211: -1.3%
Covered Lines: 1066
Relevant Lines: 1137

💛 - Coveralls

@declension
Copy link
Owner

Thanks for your interest in squeeze-alexa!

In general, yes, submissions are welcome, though please note the documented guidelines - especially around test coverage, and linking to pre-existing issues (or creating them first).

In terms of this PR, I'd strongly suggest splitting it up into its logical, independent PRs. Your bulleted list seems just right - one to fix #81, one to add utterances, and the slightly more controversial / involved one about stateful location and device-stickiness (which perhaps needs some more thought).

Logistically, I'd suggest creating the smallest PRs (and therefore easiest to get merged) first, and if / once merged, you can rebase / merge master back into this (which would become ) to reduce its size (Github will handle all this). Let me know if you run into trouble with this or would like some help.

@stridger
Copy link
Author

I have already cleaned up all the tests earlier and that is now all passing by the looks of it. I'll look at increasing the coverage and doing more atomic PR later, I don't have much time to work on this, I'm afraid.

As you say, it's unclear if many people would be interested in sticky default players, but for me it's very important as I have an Echo Dot and a Squeezebox in every room.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants