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

Queue command format and Auto-Announce feature #71

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Nyhilo
Copy link

@Nyhilo Nyhilo commented Nov 23, 2021

I did a reformat on the queue command to make it more compact and to give a bit more info (such as playtimes).
Looks like this:
image

I also added a configurable feature that auto-announces when a new song starts playing. Useful for people to see what's on deck at a glance without having to do $np
image

@Nyhilo
Copy link
Author

Nyhilo commented Nov 23, 2021

This PR also resolves issue #40.

I'm not well practiced in contributing the code I write, so let me know if you need anything from me @Raptor123471.

@Raptor123471 Raptor123471 self-assigned this Nov 23, 2021
@Raptor123471
Copy link
Owner

Raptor123471 commented Nov 23, 2021

I have not had a chance to run this yet but it looks great. My only suggestion right now is that the auto announce configuration in config.py becomes a setting in settings.py so that it can become configurable per guild. I will test your PR out when I get the chance.

@Raptor123471 Raptor123471 linked an issue Nov 24, 2021 that may be closed by this pull request
@Nyhilo
Copy link
Author

Nyhilo commented Nov 28, 2021

Yeah I can do that. I was little unclear about how settings.py is organized, but I've got some time this afternoon to work on it.

@Nyhilo
Copy link
Author

Nyhilo commented Nov 28, 2021

I did the move from the config to settings.py, and I resolved the merge conflicts.

However, it seems like an existing settings.json value doesn't actually take the new setting until you run set announce_tracks true once, even though upgrade() runs. Not sure why

@Raptor123471
Copy link
Owner

Raptor123471 commented Nov 29, 2021

That's a mistake on my part where the missing keys are found and set in memory but never written to disk in the 'upgrade' function. I will push the fix and then we can get this PR merged. Check out #72 . I'm going to test it some more when I am home.

musicbot/commands/music.py Outdated Show resolved Hide resolved
@@ -327,6 +334,7 @@ async def timeout_handler(self):
await self.udisconnect()

async def uconnect(self, ctx):
self.ctx = ctx
Copy link
Owner

Choose a reason for hiding this comment

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

When the setting start_voice_channel is set it bypasses uconnect() which will have ctx = none. See line 71 and 79 in run.py. For this feature to work when using these settings uconnect() would need to be reworked to accept an optional voice_channel argument for use in run.py and for line 71 & 79 to use uconnect().

Co-authored-by: Raptor123471 <raptor123471@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show newest playing song on play
2 participants