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

Add select hold to AppleTVs remote entity as possible command #105764

Merged
merged 20 commits into from
Apr 16, 2024

Conversation

myMartek
Copy link
Contributor

@myMartek myMartek commented Dec 14, 2023

Breaking change

The home_hold command has been removed due to issues encountered since Home Assistant version 2023.11.1. Instead, users can now utilize the standard service data attribute hold_secs in conjunction with the home action. This combination allows for button holding, enabling the display of the control center.

Proposed change

This PR adds the select_hold command for AppleTV remotes. It is used in some programs in order to open a context menu which is otherwise not available.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

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.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @postlund, mind taking a look at this pull request as it has been labeled with an integration (apple_tv) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of apple_tv can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign apple_tv Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@myMartek myMartek marked this pull request as draft December 14, 2023 20:19
@myMartek myMartek changed the title Feature/apple tv select hold Add select hold to AppleTVs remote entity as possible command Dec 14, 2023
@myMartek myMartek mentioned this pull request Dec 14, 2023
20 tasks
@myMartek
Copy link
Contributor Author

myMartek commented Dec 14, 2023

Sorry for not attending this matter earlier, but adding new _hold buttons is not the correct way to go here. There is an argument to send_command called hold_secs that should be used instead:

My suggestion is to pass InputAction.Hold to any method supporting it in case hold_secs >= 1. Currently, pyatv does not support arbitrary hold times and the default is one second, which I believe makes sense (that's what iOS uses too I believe).

The same kind of reasoning can be used with num_repeats to implement InputAction.DoubleTap later on.

Commands supporting InputAction can be seen in the API reference here:

https://pyatv.dev/api/interface/#pyatv.interface.RemoteControl

In general you are right. I would agree for the _hold commands to do so.
For the repeat option I would stick to the current for _ in range(num_repeats): implementation since this one even supports more then a double tap. I am not even sure if double tap is used anywhere in tvOS right now. I don't think it will do anything so I would not recommend to implement the pyatv doubletap InputAction right now.

Another question: When adding the hold_secs parameter it maybe makes sense to make a breaking change and also remove the home_hold command completely. If you agree on all of this I can change the PR to add hold_secs and remove home_hold

@myMartek
Copy link
Contributor Author

I already added hold_secs to the implementation

@myMartek myMartek marked this pull request as ready for review December 15, 2023 18:38
@kazango
Copy link

kazango commented Dec 22, 2023

In general you are right. I would agree for the _hold commands to do so. For the repeat option I would stick to the current for _ in range(num_repeats): implementation since this one even supports more then a double tap. I am not even sure if double tap is used anywhere in tvOS right now. I don't think it will do anything so I would not recommend to implement the pyatv doubletap InputAction right now.

Not sure how much help I can be to you here but I've been following along this issue for a few weeks. As an AppleTV User, a double tap is used on the menu button on the remote to pull up the "Open apps" menu to close apps or switch between them. I'm not sure of any other mechanism to do this action other than a double-tap on the menu button.

Also eagerly awaiting a fix for the broken home_hold.

@myMartek
Copy link
Contributor Author

In general you are right. I would agree for the _hold commands to do so. For the repeat option I would stick to the current for _ in range(num_repeats): implementation since this one even supports more then a double tap. I am not even sure if double tap is used anywhere in tvOS right now. I don't think it will do anything so I would not recommend to implement the pyatv doubletap InputAction right now.

Not sure how much help I can be to you here but I've been following along this issue for a few weeks. As an AppleTV User, a double tap is used on the menu button on the remote to pull up the "Open apps" menu to close apps or switch between them. I'm not sure of any other mechanism to do this action other than a double-tap on the menu button.

Also eagerly awaiting a fix for the broken home_hold.

That’s true. But actually the double Tap works with the current implementation and I only see drawbacks and issues when implementing the doubleTap method of pyatv. In my opinion the current implementation is superior then implementing doubleTap.

You can issue a doubleTab by setting repeat to 2 and delay to eg 100ms

In my opinion this pull request should be merged as soon as possible. Yes it removes the current home_hold function and replaces it with the hold delay of HA. This is a breaking change which should normally have a deprecation beforehand. But in my opinion it’s not worth to deprecate a feature that is currently broken.

I guess it’s best to merge this bevor 23.1 release.

@myMartek
Copy link
Contributor Author

Any news on merging this PR?

@mirceadb7
Copy link

On Apple tvOS 17.2 when you press de power button short the upper menu will appear, so it doesn’t have to be home_hold.

@kazango
Copy link

kazango commented Jan 8, 2024

On Apple tvOS 17.2 when you press de power button short the upper menu will appear, so it doesn’t have to be home_hold.

This might be true for your Apple TV, but for my model I don’t have a power button to press so after updating I do not have this option with the remote I have. The remote option controls homeassistant provide also don’t currently solve this issue from what I’ve seen so I’d argue the new home_hold function is still desperately needed to fix the issue

@myMartek
Copy link
Contributor Author

When can this change be merged? All in all I don't believe its too big. But it fixes the hold actions which are currently not working at all.

@AndyReed36
Copy link

Hi! Any luck merging this? Anxiously awaiting this fix! 🤞🏻

@LOTRtelcontar
Copy link

On Apple tvOS 17.2 when you press de power button short the upper menu will appear, so it doesn’t have to be home_hold.

There also doesn't appear to be a way to "press the power button" from home assistant. I've found no way to trigger the upper menu since home_hold stopped working.

@emontnemery
Copy link
Contributor

@myMartek It's mentioned in the comments this is a breaking change because the home_hold action is removed.
Can you please fill in the breaking change section of the PR description?

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Looks good @myMartek 👍

However, please fill in the breaking change section of the PR description.

@home-assistant home-assistant bot marked this pull request as draft April 16, 2024 07:40
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@myMartek myMartek marked this pull request as ready for review April 16, 2024 08:17
@myMartek
Copy link
Contributor Author

Looks good @myMartek 👍

However, please fill in the breaking change section of the PR description.

I now filled the breaking change section

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Thanks, @myMartek 👍

@emontnemery emontnemery merged commit 18ac9a7 into home-assistant:dev Apr 16, 2024
20 checks passed
)
assert len(remote.atv.method_calls) == 1
assert str(remote.atv.method_calls[0]) == f"call.{method}()"
if hold_secs >= 1:
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use conditions in tests. Either parametrize the test further or make a separate test to test this case.

@NateEaton
Copy link

Not sure this is really a fix. I changed my use of send_command home_hold to using home with various hold_seconds values (1, 2, 3, 5, etc) but nothing pops up the control panel.

@myMartek
Copy link
Contributor Author

This has not yet been released. The fix is not yet live. Just wait for 2024.4.4 which will include the fix.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 19, 2024
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.

Apple TV integration
8 participants