Skip to content

include entities not in entity registry in config entities#4867

Merged
bramkragten merged 5 commits into
devfrom
include-entities-not-in-entity-registry
Feb 17, 2020
Merged

include entities not in entity registry in config entities#4867
bramkragten merged 5 commits into
devfrom
include-entities-not-in-entity-registry

Conversation

@bramkragten
Copy link
Copy Markdown
Member

@bramkragten bramkragten commented Feb 13, 2020

Can use some optimizations, states refreshes too much.
Closes #4708

image
Open for suggestions for this text:
image

div {
overflow: auto;
max-height: 600px;
max-height: calc(100vh - 300px);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this very limiting on mobile ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This shouldn't have been in this PR 🙈 but, no It just makes sure the scroll container fits on the screen, will fine-tune it a bit.

image

: "hass:cancel"}
: entity.disabled_by
? "hass:cancel"
: "hass:pencil-off"}
Copy link
Copy Markdown
Member

@balloob balloob Feb 13, 2020

Choose a reason for hiding this comment

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

I like the pencil-off icon, good choice.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is the icon truly needed? The entity's check box is grayed-out, signalling read-only status. A quick glance on the left already indicates which entities can/cannot be edited. The no-edit icon repeats on the right what is already communicated on the left.

Whereas the exclamation icon draws one's attention to an anomaly (entity is unavailable), lacking a unique_id isn't an anomalous condition but shares the same column.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A user needs to know why the checkbox is disabled, there is no explanation now.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I agree. However, I don't believe the pencil-off icon provides that missing explanation. It simply reiterates the meaning of the dimmed checkbox: this table-row cannot be edited.

Suggestion:
Perhaps by hovering over the checkbox or table-row (and/or if you click the table-row) it reveals an explanation like:

Cannot edit this entity because it lacks a unique_id. Refer to configuration.yaml file.

Anyway, I don't mean to belabor the point but, in its current form, it tells me twice that I can't edit the table-row (and without a hint as to why).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It does show that it is not editable on click in the settings dialog.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Check the screenshot, it does tell you why.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your original post's edit-history shows the screenshot was added 3 days ago, same time when I made my previous post. My mistake; I didn't see you had amended your original post.

I suggest the message also mention configuration.yaml to answer the reader's question "If not from the UI then where?" However, even without mentioning it, the message achieves the goal of clarifying why the entity cannot be edited in the UI.

BTW, I believe the message's text should use the conjunctive adverb therefore as opposed to the adverb therefor.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It doesn't necessarily have to be in configuration.yaml for example the group entities from hue don't have unique ID's but are set up with an integration flow.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

True, but all entities defined in configuration.yaml are not obligated to have a unique_id. Traditionally, it's a prime source of entities than do not appear in the Entities list and why I suggested it be mentioned. Nevertheless, even without referring to the YAML file, the message explains why you can't edit the entity and that's very helpful. It's all good.

FWIW, the few flow-based integrations I've used (Homekit_controller, LIFX, and the currently in-beta UPB) all generate entities with unique_id's. This gave me the impression that it's the norm for flow-based integrations.

Off-topic: As for the Hue integration, without knowing why it fails to generate unique id's (a design constraint?), I'd say it's anomalous (and probably should create them).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well every integration should try to do it, also a lot of integrations that are set up with configuration.yaml have them. In the case of hue, it is a technical limitation just for groups, lights and sensors do have unique id's.

Comment thread src/panels/config/entities/ha-config-entities.ts
"ui.panel.config.entities.picker.filter.show_unavailable"
)}
</paper-icon-item>
<paper-icon-item @tap="${this._showReadOnlyChanged}">
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know if this should be an option. They are active, they have states etc otherwise they wouldn't exist. I think that we should always show them.

Comment thread src/panels/config/entities/ha-config-entities.ts Outdated
Comment thread src/panels/config/entities/ha-config-entities.ts Outdated
Comment thread src/panels/config/entities/ha-config-entities.ts Outdated
Comment thread src/panels/config/entities/ha-config-entities.ts Outdated
Comment thread src/panels/config/entities/ha-config-entities.ts Outdated
@bramkragten bramkragten merged commit 49b0c8d into dev Feb 17, 2020
@bramkragten bramkragten mentioned this pull request Feb 17, 2020
@balloob balloob deleted the include-entities-not-in-entity-registry branch February 17, 2020 16:46
@lock lock Bot locked and limited conversation to collaborators Feb 18, 2020
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.

Entities tab should include entities not in entity registry

4 participants