svrooij / node-sonos-ts

:speaker: Sonos control library, use this library in your own appliction.

https://sonos-ts.svrooij.io/

MIT License

80 stars 18 forks source link

Adding Compatibility For Home Theater Channel Map Sets #166

Closed jkossis closed 2 years ago

jkossis commented 2 years ago

Adding Compatibility For Home Theater Channel Map Sets

Description

Only stereo ChannelMapSets are parsed when pulling information from the zone group topology. This change enhances the parsing behavior to account for home theater setups as well.

Your checklist for this pull request

🚨 Please review the guidelines for contributing to this repository.

[x] Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
[x] Make sure you are making a pull request against the beta branch (left side). Also you should start your branch off svrooij/node-sonos-ts/beta.
[x] Check the commit's or even all commits' message styles matches our requested structure.
[x] Check your code additions will fail neither code linting checks nor unit test.
[x] Check you add tests for added code.

💔 Thank you!

jkossis commented 2 years ago

Deepsource doesn't seem to like my use of any, though I was just following the established pattern of its usage. I can update this if needed.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 2720348942

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.06%) to 81.363%

Totals
Change from base Build 2628396018:	0.06%
Covered Lines:	1654
Relevant Lines:	1989

💛 - Coveralls

svrooij commented 2 years ago

Deepsource doesn't seem to like my use of any, though I was just following the established pattern of its usage. I can update this if needed.

I was just trying out deepsource. Not sure if I want it turned on or if those issues matter. A lot of deepsource issues are on ignore anyway. I’ll be checking your PR tomorrow, but at first glance everything looks alright to me.

svrooij commented 2 years ago

You only forgot this:

@all-contributors add @jkossis for code

allcontributors[bot] commented 2 years ago

@svrooij

I've put up a pull request to add @jkossis! :tada:

svrooij commented 2 years ago

@jkossis do the satellite players also need some controls? Or are they only controlled by the main player? Can you change the name of such a player through the original app or is that not possible and are they seen as one player in the system?

Do you think changing sets in this library would be a feature you would use? Or is that something where you just have to get the official app for?

jkossis commented 2 years ago

@svrooij to answer your first question, satellites are indeed controlled by the main player(coordinator). They are seen as one player when in a home theater setup.

I'm not sure I understand your second question ... what do you mean by changing sets?

svrooij commented 2 years ago

I mean setting up the home theater configuration. Start with a playbar, speaker x should be back left, this should be the subwoofer.

Or would you always do those configs in the official app, since you only have to do it once?

jkossis commented 2 years ago

@svrooij ah, gotcha. Yeah, my gut feeling is that functionality is probably best left up to the official app. I am not aware of the protocol/handshaking required to setup/maintain those relationships.

jkossis commented 2 years ago

I think this change was more to enhance/fix the existing functionality that provides the channel set info. In my use case, I only want to show sub configuration in my app if the coordinator has a sub connected.

svrooij commented 2 years ago

All looking good to me!

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 2.5.1-beta.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket: