-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[squeezebox] Implement like/unlike for remote streaming services #7396
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't really clear to me from the code, but do you update the like/unlike channels whenever a new song or track is played? Or are the like/unlike channels write-only?
...x/src/main/java/org/openhab/binding/squeezebox/internal/handler/SqueezeBoxPlayerHandler.java
Show resolved
Hide resolved
...x/src/main/java/org/openhab/binding/squeezebox/internal/handler/SqueezeBoxPlayerHandler.java
Outdated
Show resolved
Hide resolved
...x/src/main/java/org/openhab/binding/squeezebox/internal/handler/SqueezeBoxServerHandler.java
Outdated
Show resolved
Hide resolved
...x/src/main/java/org/openhab/binding/squeezebox/internal/handler/SqueezeBoxServerHandler.java
Outdated
Show resolved
Hide resolved
...eezebox/src/main/java/org/openhab/binding/squeezebox/internal/dto/ButtonDTODeserializer.java
Outdated
Show resolved
Hide resolved
...eezebox/src/main/java/org/openhab/binding/squeezebox/internal/dto/ButtonDTODeserializer.java
Outdated
Show resolved
Hide resolved
...eezebox/src/main/java/org/openhab/binding/squeezebox/internal/dto/ButtonDTODeserializer.java
Outdated
Show resolved
Hide resolved
Whenever a new song/track is played, the binding queries the LMS in order to update the like/unlike commands. These are the commands that need to be sent to the LMS in order to like/unlike a song. The commands depend on the streaming service being used (e.g. Pandora is "pandora rate 1" and slacker is "slacker rate F"), which is why they need to be updated on each new song. It does this in |
@cpmeister Thanks for your review! I've responded to all your comments, and pushed a commit with the changes. Just one question above for which I'm looking for guidance. |
In that case - when these channels do not have a state - you might want to use a DynamicCommandDescriptionProvider / |
I could do that. But, as there are a couple other existing Switch channels that also accept just ON (i.e. Example of existing channel |
Travis tests were successfulHey @mhilbush, |
I would not touch existing channels. Only new ones. Additionally I am not convinced if it makes sense to use a dynamic command description for other item types than |
So, then I'm not sure what's to be gained by changing this. I'd need to change the channel type (breaking existing people using the new feature), and rework the code. I'm just not sure of the value in doing that in this case. |
I don't want to push you to use it. I want to promote the feature. It was a suggestion. Up to you. But let me quote from the JavaDoc of
Isn't that exactly what you want to achieve in the end. Let me correct my previous statement: I am personally not convinced... You can use it with every item type. There're no restrictions inside the framework.
I don't think we have to discuss this right now. They're using it on their own risk as long as this PR remains unmerged. Don't get me wrong. I neither want to tease you nor them. |
I understand completely. Just trying to sort out the value in this case.
Yes, it is exactly what I want to acheive.
That's interesting. So I might be able to leave the channel as a Switch. As an aside, the documentation says it only works with a String.
C'mon, man. Sure you do. 😉 I'll prototype it to get an idea how it works. Now that the PR missed the 2.5.5 release, I have plenty of time. 😃 |
@cweitkamp Here's what I've done so far. I've created a new channel called I defined the
In the sitemap I put this.
All good so far. But I've run into some issues.
|
Very much appreciated that you gave it a try.
Hm, I have the slight feeling we have the same problem with dynamic state descriptions too. Time to open an issue in OHC. A possible solution could be a new event emitted via OH event bus. UIs have to refresh their elements on state description or command option changes. This will not make it into OH2.5.x anymore.
For this there maybe several possibilities.
So not ask me something about HABpanel. I am not familiar with it 😉. Rather ping @ghys and find a solution for the new UI. |
I really like the idea of a single channel with command options that can be adjusted as necessary. That model really does fit what I'm trying to do. However, given the issues I mention above (especially the HABpanel issue), I think I'd prefer to stick with the current implementation.
I think you're right.
In Basic UI, it appears to use switch if there are no command options. I'm not sure I understand the rationale for doing this when we're talking about an item of type String.
Currently,
The command options list argument to
I think that will result in the same behavior given the above implementation in Basic UI. |
@openhab/add-ons-maintainers I'd like to get this PR back onto someone's radar screen. It seems to have fallen through the cracks. Thanks! |
@mhilbush |
Sorry if I left the impression this was for 3.0. I would like it included in 2.5.x |
I am afraid this confusion results due to my questioning. Sry. FTR: I am already working on a scratch for a |
bundles/org.openhab.binding.squeezebox/src/main/resources/ESH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
Travis tests were successfulHey @mhilbush, |
@cpmeister I've pushed an update the addresses your remaining comments. Please let me know if there's anything further that needs to be changed. |
Signed-off-by: Mark Hilbush <mark@hilbush.com>
Signed-off-by: Mark Hilbush <mark@hilbush.com>
Travis tests were successfulHey @mhilbush, |
I've resolved the merge conflicts. Is there anything else I need to do before this can be merged? |
Ugh. Missed another release cycle. Some day, I suppose... |
Anyone? Bueller? Bueller? |
Must be Ferris's day off... 🙁 😉 @openhab/add-ons-maintainers Coming up on 2.5 months for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @Hilbrand |
...x/src/main/java/org/openhab/binding/squeezebox/internal/handler/SqueezeBoxServerHandler.java
Outdated
Show resolved
Hide resolved
...x/src/main/java/org/openhab/binding/squeezebox/internal/handler/SqueezeBoxServerHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Mark Hilbush <mark@hilbush.com>
@cpmeister Thanks for the additional review. I've addressed your feedback. Let me know if ok. |
Travis tests were successfulHey @mhilbush, |
…nhab#7396) * Implement like/unlike for remote streaming services * Fix formatting * Address review feedback * Combine like/unlike to single rate channel * Address review comments Signed-off-by: Mark Hilbush <mark@hilbush.com>
…nhab#7396) * Implement like/unlike for remote streaming services * Fix formatting * Address review feedback * Combine like/unlike to single rate channel * Address review comments Signed-off-by: Mark Hilbush <mark@hilbush.com> Signed-off-by: CSchlipp <christian@schlipp.de>
…nhab#7396) * Implement like/unlike for remote streaming services * Fix formatting * Address review feedback * Combine like/unlike to single rate channel * Address review comments Signed-off-by: Mark Hilbush <mark@hilbush.com> Signed-off-by: MPH80 <michael@hazelden.me>
…nhab#7396) * Implement like/unlike for remote streaming services * Fix formatting * Address review feedback * Combine like/unlike to single rate channel * Address review comments Signed-off-by: Mark Hilbush <mark@hilbush.com>
…nhab#7396) * Implement like/unlike for remote streaming services * Fix formatting * Address review feedback * Combine like/unlike to single rate channel * Address review comments Signed-off-by: Mark Hilbush <mark@hilbush.com>
…nhab#7396) * Implement like/unlike for remote streaming services * Fix formatting * Address review feedback * Combine like/unlike to single rate channel * Address review comments Signed-off-by: Mark Hilbush <mark@hilbush.com>
…nhab#7396) * Implement like/unlike for remote streaming services * Fix formatting * Address review feedback * Combine like/unlike to single rate channel * Address review comments Signed-off-by: Mark Hilbush <mark@hilbush.com>
…nhab#7396) * Implement like/unlike for remote streaming services * Fix formatting * Address review feedback * Combine like/unlike to single rate channel * Address review comments Signed-off-by: Mark Hilbush <mark@hilbush.com> Signed-off-by: Daan Meijer <daan@studioseptember.nl>
…nhab#7396) * Implement like/unlike for remote streaming services * Fix formatting * Address review feedback * Combine like/unlike to single rate channel * Address review comments Signed-off-by: Mark Hilbush <mark@hilbush.com>
Some streaming services, such as Pandora and Slacker, support "liking" or "unliking" the currently playing song. Liking a song causes the streaming service to play more songs like the currently playing song. Unliking the song skips the currently playing song, and won't play it again. As each streaming service has a different way to accomplish this, the LMS jsonrpc service supplies the specific like/unlike commands based on the service that's streaming the song. It does this by overriding the functionality of the
repeat
andshuffle
buttonsThis change adds two new channels to the player thing -
like
andunlike
. If a streaming service doesn't support the like/unlike functionality, sending a command to thelike
orunlike
channel does nothing.Unfortunately, the current implementation of the squeezebox binding does not use the Logitech Media Server (LMS) jsonrpc service; it uses the older command line interface (CLI). Therefore, I needed to extend the binding to support the jsonrpc service in order to obtain the like/unlike commands.
I considered changing the binding to use only the jsonrpc service; however, that would require very significant modifications to the binding, which I chose to not undertake at this time.
This PR also introduces a slight change to the process to retrieve favorites from the LMS. Currently, there's a slight race condition where it's possible for the bridge to send the favorites to the player handlers before the player handlers have completed initialization. This change simply adds a slight delay to the retrieval of the favorites from the LMS.
This PR also changes the binding to use constructor injection.
I considered adding null annotations, but decided to save that for another PR as the changes looked pretty significant.
Signed-off-by: Mark Hilbush mark@hilbush.com