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

Media player send key service #27587

Closed
wants to merge 2 commits into from

Conversation

tulindo
Copy link
Contributor

@tulindo tulindo commented Oct 13, 2019

Breaking Change:

None

Description:

Creation of the new media_send_key service. This will allow to send to the media player a control key (just like an IR remote control).
This can be useful to implement at frontend level a generic remote control to deal with television.

The service is also been implemented for samsungtv component.

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][dev-checklist]

@tulindo
Copy link
Contributor Author

tulindo commented Oct 13, 2019

Can someone tell me what is the error behind the codecov/patch test. How can I address it?
Thanks
Paolo

@frenck
Copy link
Member

frenck commented Oct 13, 2019

Hi there, @tulindo!

That is because you've adjusted on of our internal/core components: media_player. Those are equipped with unit tests. Since you've added new features to it, it will need to be unit-tested. So please add those.

Besides all that, it might need to be discussed in our architectural repository first: https://github.com/home-assistant/architecture

@tulindo
Copy link
Contributor Author

tulindo commented Oct 13, 2019

Thanks @frenck you've been very clear.
I'm going to add that.
I'm only surprised that the codecov complains about the samungtv component and not the core media player. @MartinHjelmare what do you think?

It sounds logical that being media player a core component it has to be discussed somewhere... But I didn't know how to start a discussion.
Now I know! Thanks again

Copy link
Contributor

@dgomes dgomes left a comment

Choose a reason for hiding this comment

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

This will need to be first reviewed in home-assistant/architecture#299

@@ -63,3 +64,40 @@
SUPPORT_PLAY = 16384
SUPPORT_SHUFFLE_SET = 32768
SUPPORT_SELECT_SOUND_MODE = 65536

MEDIA_KEYS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

these must be reduced to the common denominator (probably numbers + prog + volume + power)

@MartinHjelmare
Copy link
Member

The samsungtv integration has tests. They need to be extended when new code is added to keep coverage in line.

https://github.com/home-assistant/home-assistant/blob/da29c1125fac4fd9059ffec82899d65e751ef6be/tests/components/samsungtv/test_media_player.py#L1

@MartinHjelmare
Copy link
Member

Let's close this since the architecture issue discussion points in another direction.

Dev automation moved this from Needs review to Cancelled Oct 23, 2019
@tulindo tulindo deleted the MediaPlayerSendKey branch October 23, 2019 17:45
@lock lock bot locked and limited conversation to collaborators Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

5 participants