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 support for EFFECT_OFF #99

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

Add support for EFFECT_OFF #99

wants to merge 12 commits into from

Conversation

dmulcahey
Copy link
Contributor

Fixes: #98

Copy link

codecov bot commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.49%. Comparing base (c394a7c) to head (81b7f70).
Report is 8 commits behind head on dev.

Files with missing lines Patch % Lines
zha/application/platforms/light/__init__.py 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev      #99      +/-   ##
==========================================
+ Coverage   96.45%   96.49%   +0.04%     
==========================================
  Files          61       61              
  Lines        9336     9337       +1     
==========================================
+ Hits         9005     9010       +5     
+ Misses        331      327       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheJulianJES
Copy link
Contributor

Can't comment on unchanged lines, but the assume option for effects should be changed to be like the others.
So, adding the is not None check. Line 1383 and following currently.
image


do we want to do this color mode part

We could do that, yeah. (would prefer a separate PR though)
IIRC, changing the color during a colorloop effect doesn't impact the bulb, so we could set the color modes to on_off and brightness during that. Brightness changes still affect it, I think.

(On a semi-related note, I'd also really like to support Hue effects in the future. Will require changes to both quirks and zha.)

@dmulcahey dmulcahey changed the title Add support for EFFECT_NONE Add support for EFFECT_OFF Jul 26, 2024
@dmulcahey dmulcahey marked this pull request as ready for review July 29, 2024 17:38
@puddly puddly added this to the 2024.10 milestone Sep 10, 2024
@TheJulianJES
Copy link
Contributor

I've rebased the PR to fix the conflicts and addressed the comments I just made above. There's some more context in those comments on why the changes are needed IMO.

@TheJulianJES
Copy link
Contributor

I think this PR should be ready now. The missing coverage is on existing lines, something to improve in the future.


Also discovered an existing (long-standing) issue when turning off effects via changing the color, but it's unrelated to this PR. Filed #210. Also something to be fixed in the future. The effect code on/off should be cleaned up a bit with it. Looks like nobody reported this before.

@TheJulianJES TheJulianJES removed this from the 2024.10 milestone Oct 10, 2024
@TheJulianJES TheJulianJES added this to the 2024.11 milestone Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add EFFECT_OFF for Home Assistant
3 participants