-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Set load and clear chat command. #6089
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor nits, but otherwise lgtm
src/message-dispatch.js
Outdated
case "clear": | ||
{ | ||
if (APP.hubChannel.can("pin_objects") && APP.hubChannel.signIn) { | ||
clearState(this.hubChannel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange to see APP.hubChannel
on the line above and this.hubChannel
on this line. They refer to the same object. Use this.hubChannel
on both lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed this change.
src/utils/entity-state-utils.ts
Outdated
export function clearState(hubChannel: HubChannel) { | ||
networkedQuery(APP.world).forEach(eid => { | ||
if (isNetworkInstantiated(eid) && isPinned(eid)) { | ||
deleteTheDeletableAncestor(APP.world, eid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass world: HubsWorld
into clearState
and loadState
as an argument, rather than pulling it off the APP
global. We are inconsistent at doing this, so you'll see it done both ways, but generally speaking, direct use of the global is to be avoided.
In this case, it doesn't make much of a difference -- we simply pass the access up one layer (to message-dispatch), but it's happening there already so it's not another new place we're going against the grain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to make this change as well.
Why:
We wanted to create a chat command to enable the QA team to load bulk assets for testing.
I have wrapped both with a condition where sign-in and admin permissions are needed.