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

Entity stays invisible when deleting Zone #795

Open
JulianGro opened this issue Jan 23, 2024 · 14 comments
Open

Entity stays invisible when deleting Zone #795

JulianGro opened this issue Jan 23, 2024 · 14 comments
Labels
enhancement New feature or request To Be Closed Suggested to be closed.

Comments

@JulianGro
Copy link
Member

When you delete the zone that an entity is using as part of "Render With Zones", the entity will stay hidden and display an "ERROR: NOT FOUND".
The error seems nice, but I would argue that an entity should not be culled if the zone that culls it does not exist.
IMG_20240123_110519

@JulianGro JulianGro added the bug Something isn't working label Jan 23, 2024
@AleziaKurdis
Copy link
Contributor

This is not a bug, it is the expected behavior.
If we change this, it would make the system glitchy.
A zone can be small and distant and because of that might not be loaded. this would start to show content that should not be, wasting the optimization that was aimed.

You need to manage the parenting if you delete the zone.
The create app support multiple entity selection. so it is easy to address rapidly.

Also revealing imply to know what zone is a broken link. Doing this would have a performance cost.
(@HifiExperiments could confirm or infirm this.)

@gydence
Copy link

gydence commented Jan 23, 2024

yeah I agree that this is the correct behavior. maybe when you delete a zone, its ID should be removed from all renderWithZones lists? but we don’t currently keep track of that mapping anywhere and it would be a bit expensive to loop over all entities. we’d have to figure out where to put that logic…create or interface could not have loaded entities yet or might not have edit permission, so I guess it would be on the entity server

@AleziaKurdis
Copy link
Contributor

There are other reasons to obtain a broken link by the way. (Copy of the entity in a different domain, import a json that has an id but with no zone )

Most of the time I prefer to have a clue that there was a broken link (in order to manage it)
If you delete a zone by accident you still have at least a clue of which entities were depending on it.

I would say: Caution with changing this behavior for something destructive.
It is probably not top priority to address anyway. Maybe we could wait a 10th user to complain, since we are like 3 people having use this feature. (yes I'm certainly exegerating... but not that much )

@gydence
Copy link

gydence commented Jan 23, 2024

fair 😁

I think the entity list can show you when URLs are broken, right? maybe it could also show a warning for this

@AleziaKurdis AleziaKurdis removed the bug Something isn't working label Jan 23, 2024
@AleziaKurdis
Copy link
Contributor

We could have this kind of thing yes.
it would be useful.
So more an enhancement anyway

@AleziaKurdis AleziaKurdis added the enhancement New feature or request label Jan 23, 2024
@AleziaKurdis
Copy link
Contributor

Maybe a tool specifically to manage the broken links.
It would list the id that are not resovled among the renderWithZones properties and offer the choice to: Remove or Replace them.
It would work better than the multiple selection. and control the list of the entities that you want to fix,

@AleziaKurdis
Copy link
Contributor

I suggest we close this now.

@AleziaKurdis AleziaKurdis added the To Be Closed Suggested to be closed. label Feb 26, 2024
@JulianGro
Copy link
Member Author

This doesn't fix the original issue as far as I can tell.
I don't see why we cannot just run a check when deleting a zone via the Create App to check if something gets culled by the non-existent zone.

@AleziaKurdis
Copy link
Contributor

The find entity can’t be trusted for that, it might not not return entities that are far and small and never been visited in the current session. So such warning might fail to advise you and this entity will stay hidden anyway.

Also there is the case where you paste entities that come from another domain and have a value in renderWithZones. Same issue here. We can’t trust the Find entities, so zone zone might be wrongly considered as missing. So you might get the no warning and still get the pasted entities not visible.

@JulianGro
Copy link
Member Author

If Create App itself cannot do it, maybe we should have the server deal with it. My understanding is that we just need to have the server run the search for entities. Since people who can add and remove zones already have Create permissions, their client should be able to send commands to the server.

If our permission system becomes more fine grained in the future, we could add a special function for that; I think @74hc595 has done something similar here: #664

@AleziaKurdis
Copy link
Contributor

yeah... but create app should also work for serverless.
It sound like complicated for a usecase that seem questionable.
Can you explain what was the context when you experienced that?
Was that a test of a real case ?

@HifiExperiments
Copy link
Member

I would vote for doing this one the create side rather than the server so that we don't have to worry about the permissions issues (and so that this will work in serverless, great point). I agree that findEntities might miss an entity, or that the client might not have permissions to modify the other entities, but those cases should be rare and the new RenderWithZones manager makes it easy to resolve.

separately, for the pasting issue, renderWithZones IDs should get re-written when pasting entities so that they target the new ID. if that's not working as intended we should fix it

@AleziaKurdis
Copy link
Contributor

separately, for the pasting issue, renderWithZones IDs should get re-written when pasting entities so that they target the new ID. if that's not working as intended we should fix it

I undertsand that you will continue to set the id in renderwithzone only if the zone is part of the copy?
and of course discard it when it is not, right?

@HifiExperiments
Copy link
Member

I think that's how it's supposed to work 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request To Be Closed Suggested to be closed.
Projects
None yet
Development

No branches or pull requests

4 participants