Skip to content

Conversation

Pawelusze
Copy link

Moved sound sending functions e.g. (PlayerQuitSoundController -> PlayerQuitMessageController)

@Pawelusze Pawelusze requested a review from a team as a code owner October 6, 2025 18:56
@Pawelusze Pawelusze changed the title Refactor sound sending system GH-1196 Refactor sound sending system Oct 6, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the sound sending system by moving the logic from dedicated listener classes into the relevant controllers, such as PlayerJoinMessageController and ChatManagerController. The package structure for session-related controllers has also been improved. The changes are logical, but I've identified a couple of areas for improvement. There's a potential logic issue in the chat sound handler that might prevent sounds from playing for users with bypass permissions. Additionally, the new sound playing logic is duplicated across three different controllers, and I've suggested creating a dedicated SoundService to centralize this functionality and improve maintainability.

Comment on lines +54 to +56
for (Player online : this.server.getOnlinePlayers()) {
online.playSound(online.getLocation(), sound.afterJoin, sound.afterJoinVolume, sound.afterJoinPitch);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This logic for iterating over all online players to play a sound is duplicated in PlayerQuitMessageController.java (lines 46-48) and ChatManagerController.java (lines 115-117). To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, I suggest extracting this into a shared service, for example, a SoundService.

This would centralize the sound playing logic, making it easier to manage and modify in the future. Here's an example of how such a service could look:

@Service
public class SoundService {
    private final Server server;

    @Inject
    public SoundService(Server server) {
        this.server = server;
    }

    public void playSoundToAll(Sound sound, float volume, float pitch) {
        if (sound == null) {
            return;
        }
        for (Player online : this.server.getOnlinePlayers()) {
            online.playSound(online.getLocation(), sound, volume, pitch);
        }
    }
}

You could then inject SoundService into the controllers and simplify the code to a single method call, like this.soundService.playSoundToAll(sound.afterJoin, sound.afterJoinVolume, sound.afterJoinPitch);.

Comment on lines +111 to +113
if (!this.chatSettings.chatEnabled()) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This check if (!this.chatSettings.chatEnabled()) appears to be redundant and might cause unintended behavior. The event handler is already set to ignoreCancelled = true, so it won't run if the chat event is cancelled (for example, when chat is disabled for a user without bypass permissions).

However, for a user with bypass permissions, their message will go through, but this check will prevent the sound from playing. It seems more consistent to play the sound if the message is successfully sent. I recommend removing this conditional block to fix this.

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.

2 participants