core icon indicating copy to clipboard operation
core copied to clipboard

Reduce complexity in the homekit config flow filters

Open bdraco opened this issue 5 months ago • 1 comments

Proposed change

Reduce complexity in the homekit config flow filters

It seems about every 2 weeks we get a new issue like https://github.com/home-assistant/core/issues/93430 and https://github.com/home-assistant/core/issues/109046 with a whole lot of me too comments. Each time I ask for the diagnostics and find nothing is wrong, so I've given up asking for diagnostics as I usually find the user missed reading The include will cover the entire domain if you do not select any entities for a given domain..

The code was a bit complex so I figured I would clean it up a bit. In doing so I had hoped to find a bug, but I didn't so I'm moving forward the cleanup.

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)
  • [x] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • [x] 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)
  • [x] 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:

bdraco avatar Feb 07 '24 00:02 bdraco

I tried converting to selectors but it was not better since the user can select up to 150 entities

Screenshot 2024-02-06 at 7 04 28 PM

bdraco avatar Feb 07 '24 01:02 bdraco

thanks

bdraco avatar Feb 09 '24 14:02 bdraco

You have misunderstood the issue being reported here - it's not user error at all. The documented behaviour vs. how it actually works is rather confusing and error prone. I provided you several different repro steps in that thread but you did not respond.

To reproduce:

  1. Garage Door (think ESPHome with relay)

    1. You select Include option and Sensor domain only. Select the Garage Door Battery sensor only. Only the Battery sensor is added to HomeKit - this is expected.
    2. You select Include option and both Cover & Sensor domains. Select the Garage Door Battery sensor only. Both the Garage Door, and the Battery sensor, are added to HomeKit - this is unexpected because I never ticked the Garage Door.
    3. 2 is a problem because, particularly with ESPHome devices, you can have multiple entities (say switches) on the same ESPHome device but selecting one entity from a given domain can cause all of them to be exposed to HomeKit.
  2. Heater / Cooler (think Dyson)

    1. You select Include option and both Fan & Climate domains. Select the Fan entity only. Both the Fan & Climate entities become exposed to HomeKit - this is unexpected.
    2. You select Include option and Fan domain. Select the Fan entity. Only the Fan becomes exposed to HomeKit - this is expected.
  3. Valve (think Eve Aqua) - Valve is actually Switch but I digress

    1. Something to think about too - this is hypothetical.
    2. You select Include option and Valve domain only. Select the Valve entity only. By the HAP spec, we should expose the Lock Physical Controls switch & Battery Sensor too, despite even if the user did not intentionally select it. This is hard though because we don't always know what those will be, i.e. if it's a home-built ESPHome device.

There is also another issue with the HomeKit config flow selectors. Media Player and one or two other entities cannot be added as bridge but the config flow will allow you to do this. The very first prompt should be do you want to create a bridge or an accessory and we should recommend bridge for anything that is not a Media Player, etc.

codyc1515 avatar Feb 10 '24 07:02 codyc1515