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

Restore Bluetooth configuration panel #23877

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

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Jan 24, 2025

Proposed change

Restores the panel that was disabled in #23830 and add router so the options flow can still be accessed.

Screenshot 2025-01-24 at 10 52 03 AM

The next iteration (future PR) will add information about the connection slots once home-assistant/core#136215 is finished

Type of change

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

Example configuration

Additional information

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

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Comment on lines +25 to +37
<ha-card
.header=${this.hass.localize(
"ui.panel.config.bluetooth.settings_title"
)}
>
<div class="card-actions">
<mwc-button @click=${this._openOptionFlow}
>${this.hass.localize(
"ui.panel.config.bluetooth.option_flow"
)}</mwc-button
>
</div>
</ha-card>
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a bit up to you, but if you want people to end up here during troubleshooting, which was a use-case I had in mind, then we don't need to show this card. (You can visit /config/bluetooth without providing a config flow, where /config/bluetooth will act as a bluetooth hub.)

Copy link
Member Author

@bdraco bdraco Jan 25, 2025

Choose a reason for hiding this comment

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

Is there a way to reach it without manually typing in the URL? If they have to type it in it's much more likely we will never get them to do it and the issue report will never get solved. We have such a hard time getting debug logs and diagnostics, I want it to be as frictionless as possible to get here.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://my.home-assistant.io/ links can be added to the documentation page.

Copy link
Member Author

@bdraco bdraco Jan 25, 2025

Choose a reason for hiding this comment

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

I think that would limit discoverability as it would make it a lot harder to find. I have limited success getting people to click my links when working issues (reading the whole Bluetooth docs as well)

Copy link
Contributor

Choose a reason for hiding this comment

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

The only place where they are being taken to the integration panel, is through the configure button on the config flow. What would be the ultimate solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

KNX is the best example that comes to mind first.
Quick search brings Insteon, Dynalite and lcn as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a side note, since it's a built-in panel, you may need to use frontend.async_register_built_in_panel instead of panel_custom.async_register_panel.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a bit on the fence since there's 100s of thousands of people who are suddenly gonna get a Bluetooth panel. It's a really tough call because we wanna make it easy for them to find their devices, but we also don't wanna overwhelm them with information.

Copy link
Member Author

Choose a reason for hiding this comment

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

The other API's I'm planning on adding our config entry/adapter specific so we would still need a screen for each adapter.

Copy link
Member

Choose a reason for hiding this comment

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

having just a URL will make it too hard to be discoverable. I suggest we do use the "configure" from the config entry to go to the bluetooth panel, as we do for Zigbee, Z-Wave, Matter and KNX. And then on the bluetooth panel, have a button to open the options flow for that config entry, that way no options are lost.

Comment on lines 41 to 48
const searchParams = new URLSearchParams(window.location.search);
if (this._configEntry && !searchParams.has("config_entry")) {
searchParams.append("config_entry", this._configEntry);
navigate(
`${this.routeTail.prefix}${this.routeTail.path}?${searchParams.toString()}`,
{ replace: true }
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for this code to still exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this can be trimmed down more. Let me check

Copy link
Member Author

Choose a reason for hiding this comment

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

trimmed it back down in 1bd0a2b

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

Successfully merging this pull request may close these issues.

3 participants