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

[squeezebox] Fix notification sometimes playing last playlist item first #16368

Merged
merged 1 commit into from
Feb 4, 2024

Conversation

t2000
Copy link
Contributor

@t2000 t2000 commented Feb 4, 2024

Fixes #16367

Fixes openhab#16367

Signed-off-by: Stefan Triller <github@stefantriller.de>
@mhilbush
Copy link
Contributor

mhilbush commented Feb 4, 2024

Thanks for this change.

I'm curious what you're using as a player. I know that some players (such as piCorePlayer) don't need to be powered on. But I wasn't sure about the original SqueezeBox devices, such as the SqueezeBox Receiver.

Do you think the call to isPoweredOn is not needed in restorePlayerState? And, if not needed there, should that method and associated attribute be removed completely from SqueezeBoxPlayerState.java?

@t2000
Copy link
Contributor Author

t2000 commented Feb 4, 2024

I am indeed using software players with piCoreplayer and squeezeboxlite binaries.

Unfortunately I do not have a hardware squeezebox device to test :(

I would probably not remove it from SqueezeBoxPlayerState.java as it IS a state of the player. In the webUI of the Logitech media Server you can click the "on/off" button wich says "Power:XYZ". So I would keep it in the state, as it reflects the real player's state.

However, I would certainly remove it from the start of a notification as this leads to the undesired behavior of playing something that's still in the playlist from the last powered off moment.

@mhilbush
Copy link
Contributor

mhilbush commented Feb 4, 2024

Unfortunately I do not have a hardware squeezebox device to test :(

Neither do I. But i'm fine with the change. We'll just need to see if anyone who uses those devices has a problem.

But what about the call to isPoweredOn in restorePlayerState? Should that also be removed? If we're not sending the power on command, why would we want to send a power off command?

@t2000
Copy link
Contributor Author

t2000 commented Feb 4, 2024

But what about the call to isPoweredOn in restorePlayerState? Should that also be removed? If we're not sending the power on command, why would we want to send a power off command?

Well, the playlist play NUMBER turns the player ON, i.e. the power state changes with that to ON automatically. Stopping it alone does not power it OFF. So I would suggest to keep it in there, especially in cases where real devices have to be powered off. What do you think?

@mhilbush
Copy link
Contributor

mhilbush commented Feb 4, 2024

So I would suggest to keep it in there, especially in cases where real devices have to be powered off. What do you think?

Yes, I think that makes sense.

Copy link
Contributor

@mhilbush mhilbush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jlaur
Copy link
Contributor

jlaur commented Feb 4, 2024

I can give it a test with Radio/Receiver tonight.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! It works perfectly with Squeezebox Receiver and Radio when they are initially off. After playing the notification, they are automatically turned off again.

@jlaur jlaur merged commit 1d3e7c8 into openhab:main Feb 4, 2024
3 checks passed
@jlaur jlaur added this to the 4.2 milestone Feb 4, 2024
@jlaur jlaur added the bug An unexpected problem or unintended behavior of an add-on label Feb 4, 2024
jlaur pushed a commit that referenced this pull request Feb 4, 2024
#16368)

Fixes #16367

Signed-off-by: Stefan Triller <github@stefantriller.de>
@jlaur jlaur added the patch A PR that has been cherry-picked to a patch release branch label Feb 4, 2024
@jlaur jlaur changed the title [squeezebox] Fix notification sometimes plays last playlist item first [squeezebox] Fix notification sometimes playing last playlist item first Feb 11, 2024
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
openhab#16368)

Fixes openhab#16367

Signed-off-by: Stefan Triller <github@stefantriller.de>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[squeezebox] Power on for notification sometimes plays parts of current playlist item first
3 participants