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

Allow 'Discovered' flows to have title placeholders #3106

Merged
merged 2 commits into from Apr 21, 2019
Merged

Allow 'Discovered' flows to have title placeholders #3106

merged 2 commits into from Apr 21, 2019

Conversation

Jc2k
Copy link
Member

@Jc2k Jc2k commented Apr 20, 2019

This PR is a bit of a proof of concept and more about starting a conversation than anything else.

When I switch homekit_controller over to Config Entries (hopefully in 0.94) even though i know the name and model of the device there doesn't seem to be a mechanism to surface that in the 'Discovered' part of the Integrations panel. Instead I just see the localisation for component.homekit_controller.config.title.

This PR allows me to push a placeholders dict via a flows context which I can then fill into a new flow_title localisation. If there are no placeholders it continues to use the existing title localisation so existing integrations are not impacted.

Screenshot 2019-04-20 at 15 30 21

This works, and doesn't require any changes on the server side for existing integrations. For integrations that want to do it, they can just poke a placeholders dict into self.context from their async_step_discovery and add a flow_title to their strings.json.

However it does feel like i'm breaking an abstraction or violating some layering by doing it this way so I thought it best to start a conversation about the "right way". Should I added a new key at the context level? Should there (is there?) a nicer way to set context during async_step_discovery? Should a flow have an optional title that can be concatenated together with the integration name (I think thats what happens with a config entry's title, for example).

@balloob what do you think? should i be doing this a different way?

@balloob
Copy link
Member

balloob commented Apr 21, 2019

The code starting a config flow should not be aware of what it is discovering, nor be aware what keys to set in the context. I think that indeed it should be dealt with by setting something during async_step_discovery. I think that allowing a config flow write to it's own context is not such a bad idea.

@balloob
Copy link
Member

balloob commented Apr 21, 2019

And it's actually a great idea for a lot of other discoveries too.

@@ -190,6 +190,23 @@ class HaConfigManagerDashboard extends LocalizeMixin(
return localize(`component.${integration}.config.title`);
}

_computeActiveFlowTitle(localize, integration) {
const placeholders = integration.context.placeholders || {};
Copy link
Member

Choose a reason for hiding this comment

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

Can we call it title_placeholders

@Jc2k
Copy link
Member Author

Jc2k commented Apr 21, 2019

Requested change made.

On the HA server side i'm currently doing something like this inside async_step_discovery:

self.context['title_placeholders'] = {
    'name': discovery_info['name'],
}

And in my strings:

{
    "config": {
        "title": "HomeKit Accessory",
        "flow_title": "HomeKit Accessory: {name}",
<snip>

This works but it's throwing pylint off:

E1137: 'self.context' does not support item assignment (unsupported-assignment-operation)

I could suppress it but I don't really know why it thinks this is a problem.

@balloob
Copy link
Member

balloob commented Apr 21, 2019

I think it's because of the type of context. You can disable pylint check

@balloob balloob merged commit b3c1bea into home-assistant:dev Apr 21, 2019
@Jc2k Jc2k deleted the config_flow_titles branch April 21, 2019 18:00
@balloob balloob mentioned this pull request Apr 24, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants