-
Notifications
You must be signed in to change notification settings - Fork 2
Implemented notification sound for incoming messages #71
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
base: main
Are you sure you want to change the base?
Conversation
|
For a873dd3 please open another PR |
6716aee to
d29225e
Compare
| if (!CloudnodeMSG.getInstance().getConfig().getBoolean("pm-sound.enabled", true)) return; | ||
|
|
||
| final String soundName = CloudnodeMSG.getInstance().getConfig().getString("pm-sound.sound", "BLOCK_NOTE_BLOCK_PLING"); | ||
| final float volume = (float) CloudnodeMSG.getInstance().getConfig().getDouble("pm-sound.volume", 1.0); | ||
| final float pitch = (float) CloudnodeMSG.getInstance().getConfig().getDouble("pm-sound.pitch", 1.0); |
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.
Please use the pro.cloudnode.smp.cloudnodemsg.PluginConfig class for accessing the config
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.
oh sure, my bad didn't noticed it
| } catch (IllegalArgumentException | NoSuchFieldError | NullPointerException e) { | ||
| // fallback to a safe default if configured sound isn't available on this server version | ||
| try { | ||
| recPlayer.playSound(recPlayer.getLocation(), Sound.BLOCK_NOTE_BLOCK_PLING, volume, pitch); | ||
| } catch (Throwable ignored) { | ||
| // silently ignore if no sound is available | ||
| } | ||
| } |
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 think it would be better to log and do nothing, so e.g. something like
| } catch (IllegalArgumentException | NoSuchFieldError | NullPointerException e) { | |
| // fallback to a safe default if configured sound isn't available on this server version | |
| try { | |
| recPlayer.playSound(recPlayer.getLocation(), Sound.BLOCK_NOTE_BLOCK_PLING, volume, pitch); | |
| } catch (Throwable ignored) { | |
| // silently ignore if no sound is available | |
| } | |
| } | |
| } catch (Throwable e) { | |
| CloudnodeMSG.getInstance().getLogger().log(Level.SEVERE, "Failed to play sound", e); | |
| } |
| } catch (Throwable ignored) { | ||
| // Ensure message sending never fails because of sound errors | ||
| } |
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 try/catch should not be needed as it would only cover the config access
| } | ||
| } |
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.
The empty line at the end of the file should not be removed
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.
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.
|
|
||
|
|
||
| pm-sound: |
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 think there should also be a way to do this for team messages, and make the config like.
sound:
private:
# …
team:
# …Also I think it would be best to move this above the errors section so it’s not buried by all the errors.
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.
yep, sounds reasonable, my personal intention was just to notify players about pm notification only since team message can be more spammy sometimes, but still yeah config might be a better choice for server owners to pick
| # Enable or disable playing a sound when a private message is received | ||
| enabled: true | ||
| # Sound to play on private message (Minecraft sound identifier) | ||
| # Examples: ENTITY_ALLAY_AMBIENT_WITH_ITEM, ENTITY_PLAYER_LEVELUP, etc. | ||
| sound: BLOCK_NOTE_BLOCK_PLINGt | ||
| # Volume of the sound (float). 1.0 is normal volume, increase to be louder. | ||
| volume: 1.0 | ||
| # Pitch of the sound (float). 1.0 is normal pitch, lower for deeper, higher for sharper. | ||
| pitch: 1.0 |
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.
Some rewording of the explanations to be more consistent with the rest of the config.
| # Enable or disable playing a sound when a private message is received | |
| enabled: true | |
| # Sound to play on private message (Minecraft sound identifier) | |
| # Examples: ENTITY_ALLAY_AMBIENT_WITH_ITEM, ENTITY_PLAYER_LEVELUP, etc. | |
| sound: BLOCK_NOTE_BLOCK_PLINGt | |
| # Volume of the sound (float). 1.0 is normal volume, increase to be louder. | |
| volume: 1.0 | |
| # Pitch of the sound (float). 1.0 is normal pitch, lower for deeper, higher for sharper. | |
| pitch: 1.0 | |
| # Whether to play a sound when receiving a private message | |
| enabled: false | |
| # Minecraft sound identifier from https://jd.papermc.io/paper/1.20.4/org/bukkit/Sound.html | |
| sound: BLOCK_NOTE_BLOCK_PLING | |
| # Volume of the sound from 0.0 to 1.0. Decrease to be quieter. | |
| volume: 1.0 | |
| # Pitch of the sound from 0.5 to 2.0. Values below 1.0 lower the pitch and increase the duration; values above 1.0 raise the pitch and reduce the duration. | |
| pitch: 1.0 |
|
I was also thinking about a command to let players toggle the sound for themselves. Feel free to add that if you want. Or I can just add it myself in another PR. |

No description provided.