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

Implement volume control #830 #994

Merged
merged 9 commits into from
Mar 13, 2024
Merged

Conversation

FoxxMD
Copy link
Contributor

@FoxxMD FoxxMD commented Feb 2, 2024

  • Enabled using env VOLUME_CONTROL=true
  • Disabled by default

Closes #830

Copy link

github-actions bot commented Feb 2, 2024

📦 A new release has been made for this pull request.

To play around with this PR, pull codetheweb/muse:pr-994 or codetheweb/muse:52d2ccba1f2bd02253e29a305b72cf95681d5b65.

Images are available for x86_64 and ARM64.

Latest commit: 52d2ccb

@FoxxMD
Copy link
Contributor Author

FoxxMD commented Feb 2, 2024

Er...need to make volume persistent first. Please hold.

Copy link
Collaborator

@codetheweb codetheweb left a comment

Choose a reason for hiding this comment

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

thanks for the contribution!

  1. let's remove the VOLUME_CONTROL env var, I think this command can just always be available
  2. we should persist this in the sqlite database so volume changes survive across Muse restarts

@FoxxMD
Copy link
Contributor Author

FoxxMD commented Feb 7, 2024

VOLUME_CONTROL was implemented because in the original issue you cited volume control as having the potential to be abused, which I agree with. Right now, even w/o the env, only guild admins can use the control.

Would you prefer to keep it this way? I'd prefer to make it accessible to admin and role-based so admins can dole out control to trusted users. I have no problem implementing that if that is ok for you. Nevermind, I forgot guilds can control command access via roles in the discord server settings ui now. I'll make the command available to everyone. 👍

Instead of using the same /volume command for the persisted volume level I'm implementing a default volume config setting via /config set-default-volume so that the persisted value is a default for new players but the player volume itself is independent for the "listening session"

For settings in sqlite -- I see that currently each setting is a different column. I usually prefer to do a KV approach so that new settings can be added without requiring a migration. Would that be acceptable? I'll write the migrations for that as well. EX

Current Table

image

New

image

@FoxxMD FoxxMD changed the title Implement an optional "master volume control" command #830 Implement volume control #830 Feb 7, 2024
Copy link
Collaborator

@codetheweb codetheweb left a comment

Choose a reason for hiding this comment

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

sorry for my late reply, let's keep the existing structure of the config table and add a new column (I prefer the existing approach because it's more conventional and allows for correct data types instead of each setting being a string)

otherwise looks good!

Copy link
Collaborator

@codetheweb codetheweb left a comment

Choose a reason for hiding this comment

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

made a few minor changes and it's working great, only comment is that changes to the setting don't take effect until after Muse is started; is it possible to reload the config on the fly?

Copy link
Contributor Author

@FoxxMD FoxxMD left a comment

Choose a reason for hiding this comment

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

I believe this change should address using newest default volume. Sets default vol on new class field and uses that if volume is not explicitly set for the player. This way the player will always get most up-to-date default volume and use it if no user interacts with /volume command.

src/services/player.ts Outdated Show resolved Hide resolved
@FoxxMD
Copy link
Contributor Author

FoxxMD commented Mar 5, 2024

I've been using this on my own server since the last change and it has been working perfectly 👍

@ghost
Copy link

ghost commented Mar 11, 2024

I would love to see this so people stop complaining about it being way to loud when they first experience the music bot.

@codetheweb
Copy link
Collaborator

looks like there's some merge conflicts, happy to take a final look and merge after they're resolved

@GitGurky in the meantime, if you're using the Docker image, feel free to switch to the image tag that the bot posted above to start using this

FoxxMD added 2 commits March 12, 2024 09:47
# Conflicts:
#	CHANGELOG.md
#	schema.prisma
#	src/commands/config.ts
ccd8793 introduced a new migration so we need to regenerate since the new migration does not include defaultVolume
@FoxxMD
Copy link
Contributor Author

FoxxMD commented Mar 12, 2024

This is ready to merge, again.

@GitGurky you should use foxxmd/muse:latest because its the latest code IE this PR merged with latest muse. codetheweb/muse:pr-994 is the PR as of feb 2 which is missing features and not up to date with db migrations.

@codetheweb
Copy link
Collaborator

codetheweb/muse:pr-994 is the PR as of feb 2 which is missing features and not up to date with db migrations.

hmm, why do you say that? going by the commit hash it seems to have the latest commit

Copy link
Collaborator

@codetheweb codetheweb left a comment

Choose a reason for hiding this comment

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

seems to be working great!

@codetheweb codetheweb merged commit 6baaffb into museofficial:master Mar 13, 2024
5 checks passed
Copy link

🚀 Released in Release v2.7.0.

@FoxxMD
Copy link
Contributor Author

FoxxMD commented Mar 13, 2024

When I looked at the tag on dockerhub the last push was ~20 days ago. It now shows it as pushed 13 hours ago.

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.

Enhancement: Volume control
2 participants