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

Change keys for samsung TV Next and Prev Track #28167

Closed
wants to merge 1 commit into from

Conversation

tulindo
Copy link
Contributor

@tulindo tulindo commented Oct 24, 2019

Breaking Change:

Changed behaviour for next and previous track command for samsung TV's

Description:

Currently the samsung tv component react to the two above command sending the KEY_FF and KEY_REWIND command. While watching TV programs those two command do nothing.
This patch sends the KEY_CHUP and KEY_CHDOWN commands instead.
The best solution would be to send the right command based on the status of the TV (just like in the LG webos component) but unluckily looks like there is no way to detect what the TV currently showing.
Since there is no next/prev command in the media player but only next/prev track one has to deal with this.
Another option can be the introduction of a new configuration parameter (for samsungtv component) that allows the user to choose between the two alternative behaviour.

Related issue (if applicable): fixes #

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@tulindo
Copy link
Contributor Author

tulindo commented Oct 24, 2019

@MartinHjelmare no idea why black tests fails.
In my local development tox says there are no errors.
I ran also
black --fast homeassistant tests
and no complains
Development and test environment is created following https://developers.home-assistant.io/docs/en/development_environment.html and https://developers.home-assistant.io/docs/en/development_testing.html

Do I miss something in my setup? Something to update?
Thanks,
Paolo
PS:
tox version: 3.14.0
black version: 19.3b0

@tulindo
Copy link
Contributor Author

tulindo commented Oct 24, 2019

Looks like PR #28171 fixed black issues.
@MartinHjelmare once that PR will be merged will this PR be rechecked again?

@florisvdk
Copy link
Contributor

No, the checks will have to be manually started again. And from the looks of it not everyone can do that.

@MartinHjelmare
Copy link
Member

The PR to fix formatting on dev branch is merged. Try rebasing on latest dev branch to let the build pass.

@kennedyshead
Copy link
Contributor

This is kind of fun... I just looked into this :D

@tulindo tulindo force-pushed the SamsungTVNextPrevTrack branch from 7964578 to 95c305d Compare October 24, 2019 18:02
@dgomes
Copy link
Contributor

dgomes commented Oct 24, 2019

Since this effectively changes the current behaviour, shouldn't this be labeled as a breaking-change ?

@tulindo
Copy link
Contributor Author

tulindo commented Oct 25, 2019

@dgomes label it as breaking changes is up to me or to the reviewers?

@dgomes
Copy link
Contributor

dgomes commented Oct 25, 2019

It's up to reviewers, it's just a matter of making sure that on the blog post of the release users get notified of the change in behaviour.

@tulindo
Copy link
Contributor Author

tulindo commented Oct 25, 2019

Made mistakes trying to resolve conflicts. open a clean new one.

@tulindo tulindo closed this Oct 25, 2019
@tulindo tulindo deleted the SamsungTVNextPrevTrack branch October 25, 2019 19:47
@lock lock bot locked and limited conversation to collaborators Oct 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants