-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat: Flip flop switch for resume/pause events #912
Conversation
lib/EpicPlayer.js
Outdated
* @param {Message} message | ||
*/ | ||
setPausedMessage(message) { | ||
if (this.resumeMessage) this.resumeMessage.delete(); |
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.
Deletion of resume message should happen based on timeout not based on setPausedMessage.
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 don't see a problem about this method having to delete the old message before setting the new one. Is there any misbehavior which is why we need some delay before deleting the old message?
marking as draft cause buggy ;-; and im tired |
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.
Comments after I tested this:
lib/EpicPlayer.js
Outdated
* @param {Message} message | ||
*/ | ||
setResumeMessage(message) { | ||
if (this.pausedMessage) this.pausedMessage.delete(); |
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.
This has 2 issues.
this.pausedMessage
will return true even if the message was already deleted. so probably check using another way.
Secondly, it would be better if the deletion happens before the resume message is sent instead of after. Cuz I tested how it looks and it was looking weird when it has duplicated messages and then the pause get deleted after a second or so. So it would look better if pause is deleted before sending resume embed.
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.
will return true even if the message was already deleted. so probably check using another way.
^^
As discord.js starting version 13.4 had deprecated the Structure#deleted getter and removed tracking functionality of all deleted stuff, we will have to track it ourselves if we're gonna insist to reduce invalid API call in case the original message was deleted but it still in cache.
Closing this PR and making a new one once everything is taken care of and fully implemented |
* feat: Flip flop switch for resume/pause events Continuation of [#912](#912) Requested by a fellow member of the discord server, thinking it was a good idea to reduce the clutter which could be made in chats as one might frequently enter and exit a VC, this feat automatically deletes old messages regarding this matter. * formatting * Update DiscordMusicBot.js
* feat: Flip flop switch for resume/pause events Continuation of [#912](SudhanPlayz/Discord-MusicBot#912) Requested by a fellow member of the discord server, thinking it was a good idea to reduce the clutter which could be made in chats as one might frequently enter and exit a VC, this feat automatically deletes old messages regarding this matter. * formatting * Update DiscordMusicBot.js # Conflicts: # config.js # events/voiceStateUpdate.js
* feat: Flip flop switch for resume/pause events Continuation of [#912](SudhanPlayz/Discord-MusicBot#912) Requested by a fellow member of the discord server, thinking it was a good idea to reduce the clutter which could be made in chats as one might frequently enter and exit a VC, this feat automatically deletes old messages regarding this matter. * formatting * Update DiscordMusicBot.js
Please describe the changes this PR makes and why it should be merged:
Requested by a fellow member of the discord server, thinking it was a good idea to reduce the clutter which could be made in chats as one might frequently enter and exit a VC, this feat automatically deletes old messages regarding this matter.
Status and versioning classification: