Change light.toggle service call to invoke LightEntity.async_toggle#156196
Change light.toggle service call to invoke LightEntity.async_toggle#156196emontnemery merged 14 commits intohome-assistant:devfrom
Conversation
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
I am trying to understand why you can't override this method ( |
Real answer: I wasn't aware that you could override a service in a custom integration -- I'm a relative novice to Home Assistant. That said, it seems unnecessary to do so? LightEntity already nominally provides the correct extension point ( |
It can't be overridden because it's not a class method, it's a regular function |
|
@ams2990 I don't see a motivation for the change, what value does it add? If there's no motivation, I'd suggest to instead add |
I have a custom integration for dealing with my lights, and I wanted to be able to handle toggle specially. My integration is working around the fact that HA (natively) has no way to adjust the brightness of a light that's off without also issuing a "turn on" command to the light. Therefore, the way I wanted to control on/off was the on/off button on the light entity itself, which invokes toggle. To work around the fact that async_toggle is never invoked, I've changed async_turn_on and async_turn_off to detect if it should actually do the opposite behavior.
Beyond "it let's me do what I want to more easily", which I realize is not the most compelling argument, it makes LightEntity behave as a proper ToggleEntity.
This doesn't make sense to me -- we have a ToggleEntity subclass that can't be interacted with as a ToggleEntity. Maybe we should instead make LightEntity not subclass ToggleEntity, if it isn't supposed to support the methods of the base class? |
|
@ams2990 It sounds like what you really want is a way to set light parameters without changing the state, i.e. a new service separate from turn_on / turn_off or toggle? |
|
Correct, but that's been discussed to death in various other places and maintainers have shown very little inclination to accept such a proposal. Such an API would eliminate a lot of complexity that I've manually implemented. I could probably delete something around a thousand lines of custom Python and a few hundred lines of YAML if I had such an API. I would love it if such an API were made available, but I'm not holding my breath -- which is why I'm trying to fix a small QoI issue rather than go through the architectural review process for large HA changes. |
emontnemery
left a comment
There was a problem hiding this comment.
I discussed the proposal with some others and we agree there's no need to block integrations from implementing their own toggle handlers
|
@ams2990 wouldn't a custom service for your integration be easier for users to understand than hacking the toggle service? |
|
(Test are failing, I updated the branch to make sure it's not somehow related to this PR) |
There was a problem hiding this comment.
Pull request overview
This PR updates the Light domain so light.toggle invokes LightEntity.async_toggle (instead of delegating to light.turn_on / light.turn_off), enabling custom light integrations to implement specialized toggle behavior.
Changes:
- Refactors light service parameter handling into shared helpers (
process_turn_on_params/process_turn_off_params). - Updates the
light.togglehandler to calllight.async_toggle(...)directly. - Adds a
LightEntity.async_toggleoverride to preserve light-specific toggle semantics (including param processing/filtering).
|
To speed things up a little, I fixed the last few details. |
There was a problem hiding this comment.
Please adjust the description so it correctly reflects the current PR state, it should no longer be listed as breaking.
Also please update the developer docs and link it.
|
Documentation added in home-assistant/developers.home-assistant#2986. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
homeassistant/components/light/init.py:1069
- The PR description states "LightEntity does not define an overridden
async_toggle, so it actually invokesToggleEntity.async_toggle, which has the expected behavior." However, the implementation does add anasync_toggleoverride toLightEntity. The PR description is inaccurate —LightEntity.async_toggleis not simply delegating toToggleEntity.async_toggle; it contains light-specific parameter processing logic (callingprocess_turn_on_params,process_turn_off_params, and the brightness/white zero-check). The description should be updated to reflect the actual implementation.
@override
async def async_toggle(self, **kwargs: Any) -> None:
"""Toggle the entity."""
if not self.is_on:
params = process_turn_on_params(self.hass, self, kwargs)
if params.get(ATTR_BRIGHTNESS) != 0 and params.get(ATTR_WHITE) != 0:
await self.async_turn_on(**filter_turn_on_params(self, params))
return
params = process_turn_off_params(self.hass, self, kwargs)
await self.async_turn_off(**filter_turn_off_params(self, params))
Proposed change
LightEntityoverridesToggleEntity, but thelight.toggleservice call never invokesLightEntity.async_toggle. This makes it impossible to write an integration that wants to handle toggle differently than a simple "if on, turn off; otherwise, turn on".This PR changes that, so that
light.togglenow invokesLightEntity.async_toggle.Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: