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

Update entities to respect new enable/visibility principles #273

Merged

Conversation

dermotduffy
Copy link
Collaborator

@dermotduffy dermotduffy added the removal Removals and Deprecations label Jun 4, 2022
@codecov
Copy link

codecov bot commented Jun 4, 2022

Codecov Report

Merging #273 (dfceed5) into master (c68756d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #273   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines         1452      1447    -5     
=========================================
- Hits          1452      1447    -5     
Impacted Files Coverage Δ
custom_components/frigate/binary_sensor.py 100.00% <ø> (ø)
custom_components/frigate/switch.py 100.00% <ø> (ø)
custom_components/frigate/__init__.py 100.00% <100.00%> (ø)
custom_components/frigate/sensor.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c68756d...dfceed5. Read the comment docs.

@@ -71,6 +71,7 @@ def __init__(
self._obj_name = obj_name
self._is_on = False
self._frigate_config = frigate_config
self._attr_entity_registry_visible_default = obj_name != "all"
Copy link
Collaborator

Choose a reason for hiding this comment

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

So if this is false then it is hidden or disabled by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will be default enabled, and default visible (unless it's the all, and then it'll be enabled and hidden). This should be as described here, but let me know if you spot anything to be contrary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I was mostly unsure what _attr_entity_registry_visible_default = false would do but that makes sense 👍

@NickM-27
Copy link
Collaborator

NickM-27 commented Jun 5, 2022

Awesome, definitely clean and glad to see this coming together. Do you think this should be a pre-release until frigate 0.11 is fully released?

@dermotduffy
Copy link
Collaborator Author

Awesome, definitely clean and glad to see this coming together. Do you think this should be a pre-release until frigate 0.11 is fully released?

Since there'll be behavior in the next new build that depends on 0.11 (e.g. motion), I think what you describe makes sense -- i.e. we do a new major build, but keep it pre-release until 0.11 is out.

@NickM-27
Copy link
Collaborator

NickM-27 commented Jun 5, 2022

Yeah definitely, sounds good

@dermotduffy
Copy link
Collaborator Author

Experimenting with this on my home instance and running into a few small issues (one was a bug with the rename logic -- now fixed).

One unexpected behavior is that when an entity is marked as hidden by the integration, the user doesn't seem to be able to disable it or mark is visible at all (the radio buttons are just disabled intentionally, source). This is problematic, as it means every entity we mark hidden cannot be later disabled.

@NickM-27
Copy link
Collaborator

NickM-27 commented Jun 5, 2022

Oh wow that's really weird, might want to report to HA devs, I can't think of why that would be forced. Unless there's a separate attr that can be changed 🤔

@dermotduffy
Copy link
Collaborator Author

Bug filed. This will go one of three ways:

  • If the behavior is WAI, then I'm less sure we should use hidden entities at all, as it effectively means we're making a permanent choice for the user.
  • If the behavior is not WAI and gets fixed, then this PR as-is would work once we get to the next HA release (first Wednesday in July).
  • I'll learn some workaround / other option.

As I do want to get these changes in for the next major Frigate release (which is probably before that), it may make sense to avoid the use of hidden for now. By default all hidden would move to visible/enabled, as all of them are probably too widely used to disable.

@NickM-27
Copy link
Collaborator

NickM-27 commented Jun 5, 2022

I am curious if properties act different, seems interesting that the disabled property worked smoothly but this attr doesn't allow any changes.

Do you have the link to the bug? Curious to follow along

@dermotduffy
Copy link
Collaborator Author

I am curious if properties act different, seems interesting that the disabled property worked smoothly but this attr doesn't allow any changes.

It's a frontend issue, so the use of property or not won't matter. You can see the code here specifically disables the checkbox if the entity is hidden by the integration, but yet leaves the checkbox enabled if the entity is disabled by the integration, i.e. disabling works as we expected, but hiding doesn't.

Do you have the link to the bug? Curious to follow along

home-assistant/frontend#12887

@NickM-27
Copy link
Collaborator

NickM-27 commented Jun 5, 2022

Very interesting 🤔

@dermotduffy dermotduffy merged commit 52dae45 into blakeblackshear:master Jun 5, 2022
@dermotduffy dermotduffy deleted the entity-visibility-changes branch June 5, 2022 22:21
@dermotduffy
Copy link
Collaborator Author

I'll revert 52dae45d557461b717731c3114798de4b5391d3e if we choose to bring back the hidden entities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
removal Removals and Deprecations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide some entities by default
2 participants