-
Notifications
You must be signed in to change notification settings - Fork 3k
Refactor minor activities #7766
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
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.
I like this change.
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 like this! Other than the minor readability adjustment.
(Haha get it "minor"????) |
Reminder to add conversion functions for pre-existing roomsettings. |
yeah, just trying to fix this crash first then i will |
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.
Looking good otherwise!
Good change.
if (this.minorActivity && 'voters' in this.minorActivity) { | ||
if (user.id in this.minorActivity.voters) this.minorActivity.updateFor(user); | ||
} | ||
this.minorActivity?.onRename?.(user, oldid, joining); |
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.
Is this actually necessary? onRename
only applies if IP stays the same, and the IP should already be in the "already voted" table.
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 do not know
@@ -889,8 +928,8 @@ export abstract class BasicRoom { | |||
this.users[user.id] = user; | |||
this.userCount++; | |||
|
|||
if (this.minorActivity) this.minorActivity.onConnect(user, connection); | |||
if (this.game && this.game.onJoin) this.game.onJoin(user, connection); | |||
this.minorActivity?.onConnect?.(user, connection); |
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.
Is this necessary? This will call onConnect
twice when a user joins.
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 do not know
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.
Except for the onConnect thing, looks good to me!
Good changes.
I guess I'll figure out the |
No description provided.