Skip to content

Make entities and devices independent in the scene editor#11046

Merged
balloob merged 9 commits intohome-assistant:devfrom
dmulcahey:dm/scene-editor-changes
May 23, 2022
Merged

Make entities and devices independent in the scene editor#11046
balloob merged 9 commits intohome-assistant:devfrom
dmulcahey:dm/scene-editor-changes

Conversation

@dmulcahey
Copy link
Copy Markdown
Contributor

Breaking change

Proposed change

The scene editor currently only works with devices. You can pick an entity but if it has an associated device the device is what is added to the scene. This "works" but falls down with devices such as multi-gang smart switches where the individual states for each gang should not be forced to be used together in a scene. It is very common for 1 gang to control the lighting for 1 room / area and another gang to control the lighting in another room / area.

This change adds a key entity_only to the stored configuration in scenes.yaml so that it can be used by the editor as a hint when saving and loading the scene. This allows individual entities to be used in scenes regardless of whether or not they are associated to a device.

I have done some light testing and this appears to work. It needs a typing issue corrected but I wanted to open this to get some feedback on the change before trying to finish it.

If this change is welcome it may also be worth discussing making the entity picker always available. I get the intention to make this as easy to use as possible but IMO it isn't intuitive to have this picker hidden behind advanced mode.

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

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:

@frenck
Copy link
Copy Markdown
Member

frenck commented Dec 29, 2021

The scene editor currently only works with devices.

No?

image

@dmulcahey
Copy link
Copy Markdown
Contributor Author

The scene editor currently only works with devices.

No?

image

🤔 is the rest of what I wrote after the opening sentence unclear? I can try to explain it differently but I’m unsure of how to start with just No?

@frenck
Copy link
Copy Markdown
Member

frenck commented Dec 29, 2021

is the rest of what I wrote after the opening sentence unclear?

It wasn't unclear, I just don't recognize this myself.

But I think I've now learned what is happening. Apparently, most of my scenes use devices with a single entity. Wonder why I never ran into this myself before :S

@dmulcahey
Copy link
Copy Markdown
Contributor Author

Another question: should entities (either individual or entities belonging to devices) that have entity_category set to config / diagnostic / system be excluded from the editor / scenes?

@frenck
Copy link
Copy Markdown
Member

frenck commented Dec 29, 2021

That is a very good question, which I don't have the answer to myself, to be honest. Both cases sound interesting. As in, scenes can be used to restore the state of a device (e.g., a specific camera configuration when you are away for example, or child locks enabled when kids are home... those things).

I think the most common use cases won't be needing the non-primary entities; but ruling them out sounds kinda odd as well 🤷

@dmulcahey
Copy link
Copy Markdown
Contributor Author

As in, scenes can be used to restore the state of a device (e.g., a specific camera configuration when you are away for example, or child locks enabled when kids are home... those things).

Good point. I think leaving them as is makes sense if we can solve the issue this RFC attempts to solve. Then the user can explicitly include or exclude them depending on what they want to achieve.

@emontnemery
Copy link
Copy Markdown
Collaborator

emontnemery commented Jan 4, 2022

@dmulcahey, @frenck let's leave out entities with entity_category set when a device is picked, that should maybe be done in a separate PR though.

@Ragingfire105
Copy link
Copy Markdown

Ragingfire105 commented Feb 19, 2022

When I ran into this issue on my Shelly 2.5 this is the simple interface that came to mind as a solution.
Pickable Entity

To implement this today you would have to go into scenes.yaml and delete the entities you wish to exclude.
So, the expected behavior would be, on save of the scene unchecked entities would not be written to file. On load of the scene in the editor as each entity is read in it would check if the device has already been loaded. If so, enable that entity else, add the device and enable only the loading entity.
If implemented how outlined, I would expect this would no longer be considered a breaking change. Loading of scenes that both have and have not been modified to exclude entities would load and save as expected.

@SuPeRMiNoR2
Copy link
Copy Markdown

Something like this would be super helpful for me. The main devices I have trouble with are WLED devices and my Inovelli Fan + Light Switches.
It is pretty annoying having to manually edit the scenes just so that the scenes don't control my fan AND my light.

@emontnemery emontnemery force-pushed the dm/scene-editor-changes branch from dbbc0e8 to bd3a546 Compare April 21, 2022 12:40
[entityId: string]: string | { state: string; [key: string]: any };
[entityId: string]:
| string
| { state: string; [key: string]: any; entity_only?: boolean | undefined };
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we tuck the entity_only flag away in a metadata object?

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.

Do we even need entity_only? If we allow this, we would just make it so that picking an entity in the entity list doesn't automatically upgrade to the device.

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.

So you pick a device for devices, you pick an entity for entities.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The problem is that the scene config does not have a concept of devices which are included, frontend derives the included devices by looking up device ids for entities included in the scene.

It means we need some way for frontend to keep track of which devices should be included by storing additional data in the scene config. The entity_only is an opt-out flag; it would maybe be more logical to make it an opt-in flag instead?

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.

If it's for the frontend only, we generally use metadata.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fixed, the frontend settings are now moved to a seperata metadata item in the scene

@balloob
Copy link
Copy Markdown
Member

balloob commented Apr 21, 2022

I am not sure if we currently already do this, but selecting a device for a scene should only include entities that have no entity categories.

@emontnemery
Copy link
Copy Markdown
Collaborator

I am not sure if we currently already do this, but selecting a device for a scene should only include entities that have no entity categories.

We don't do that, and we should fix that too, in a separate PR

@emontnemery emontnemery force-pushed the dm/scene-editor-changes branch from 3f022e7 to cf15bf7 Compare May 19, 2022 15:09
@balloob
Copy link
Copy Markdown
Member

balloob commented May 20, 2022

Erik has updated the backend to allow metadata to be stored. Use that to store if an entity was added without a device.

Comment on lines +643 to +645
entityMetaData
? !entityMetaData.entity_only
: false
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.

It took me a while staring at this code to figure out what it does. This seems to simplify it a bit.

Suggested change
entityMetaData
? !entityMetaData.entity_only
: false
entityMetaData &&
!entityMetaData.entity_only

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To not change the behavior in a surprising way, the logic should be reversed so we assume entities in scenes without metadata are added as devices.

const entityMetaData = config.metadata?.[entityReg.entity_id];
if (
!this._devices.includes(entityReg.device_id) &&
!(typeof entity === "string") &&
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.

Why check this?

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.

Suggested change
!(typeof entity === "string") &&
typeof entity !== "string" &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This was added when the entity_only flag was set in the entities map and is indeed no longer needed 👍

@balloob balloob changed the title RFC: Make entities and devices independent in the scene editor Make entities and devices independent in the scene editor May 20, 2022
zsarnett
zsarnett previously approved these changes May 23, 2022
balloob and others added 2 commits May 23, 2022 15:40
Co-authored-by: Zack Barett <zackbarett@hey.com>
@balloob balloob enabled auto-merge (squash) May 23, 2022 22:59
@balloob balloob merged commit b71b230 into home-assistant:dev May 23, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 25, 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.

8 participants