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

[androidTV] fix version parsing for newer Philips TV models #17373

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

marcelrv
Copy link
Contributor

@marcelrv marcelrv commented Sep 5, 2024

closing #17370

This fixes the issue for me. But needs to be tested for older TVs which provides the int as a version.

(note, the alternative would be to completely remove the version parsing, as the getVersion & setVersions don't seem to be called anywhere)

Signed-off-by: Marcel Verpaalen <marcel@verpaalen.com>
@jlaur jlaur added the bug An unexpected problem or unintended behavior of an add-on label Sep 5, 2024
@lsiepel
Copy link
Contributor

lsiepel commented Sep 5, 2024

You could add a unit test to simulate an older tv version string.

Edit: If you provide a JAR, it would be easier to invite people to test. Or maybe @morph166955 can confirm ther eis no regression to older AndroidTV's.

@lsiepel lsiepel added the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Sep 8, 2024
@marcelrv
Copy link
Contributor Author

marcelrv commented Sep 8, 2024

You could add a unit test to simulate an older tv version string.
I recommended the testing indeed as I don't know the format for older TVs. Hence was hoping @morph166955 would be able to test it against an older TV.

@morph166955 Here is the jar if that makes testing more easy.
https://www.verpaalen.com/openhab/org.openhab.binding.androidtv-4.3.0-SNAPSHOT.jar

@morph166955
Copy link
Contributor

I don't have a Philips TV personally to test this against. We pulled Philips in a while ago because it runs the ATV protocols under the hood. Philips is much like the Nvidia Shield for this binding where it provides additional hooks beyond what ATV provides. ATV is a very broad userbase because it's not tied to a single vendor. This shouldn't do anything to cause issues with the ATV stack. You may want to put a post on the forum and see if other PTV users can regression test this on older models to make sure it doesn't have issues.

As test for version as string and as int for PhilipsTV

Signed-off-by: Marcel Verpaalen <marcel@verpaalen.com>
@marcelrv
Copy link
Contributor Author

marcelrv commented Sep 9, 2024

So, I added a test for both the version as a string as well version as an int.
With this I'm fairly confident the parsing will happen without any error.
This version variable is nowhere used, so the main question was if any exception was triggered if the version would still be provided as int.

@lsiepel lsiepel removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Sep 9, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@lsiepel lsiepel merged commit 65fce20 into openhab:main Sep 9, 2024
5 checks passed
@lsiepel lsiepel added this to the 4.3 milestone Sep 9, 2024
@marcelrv marcelrv deleted the androidtv-version branch September 10, 2024 11:04
digitaldan pushed a commit to digitaldan/openhab-addons that referenced this pull request Sep 24, 2024
…17373)

* [androidTV] fix version parsing for newer Philips TV models

Signed-off-by: Marcel Verpaalen <marcel@verpaalen.com>
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
…17373)

* [androidTV] fix version parsing for newer Philips TV models

Signed-off-by: Marcel Verpaalen <marcel@verpaalen.com>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
…17373)

* [androidTV] fix version parsing for newer Philips TV models

Signed-off-by: Marcel Verpaalen <marcel@verpaalen.com>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
…17373)

* [androidTV] fix version parsing for newer Philips TV models

Signed-off-by: Marcel Verpaalen <marcel@verpaalen.com>
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
Projects
None yet
4 participants