fix: Hidden message command feedback

Command feedback may be sent multiple times, it may also be sent to non
OP players with vanish view permission.

CHANGELOG.md

@@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Fixed
+- Hidden message command feedback
 
 ## [1.4.2] - 2023-06-09
 ### Fixed

src/main/java/me/drex/vanish/api/VanishAPI.java

@@ -50,56 +50,57 @@ static boolean setVanish(@NotNull ServerPlayer player, boolean status) {
     /**
      * Check whether a player can see the action from another player.
      *
-     * @param executive The player who executed the action
-     * @param viewer    Is the player that is viewing the action
+     * @param actor    The player who executed the action
+     * @param observer Is the player that is viewing the action
      * @return the result of the check
      */
-    static boolean canSeePlayer(@NotNull ServerPlayer executive, @NotNull ServerPlayer viewer) {
-        return canSeePlayer(executive.server, executive.getUUID(), viewer);
+    static boolean canSeePlayer(@NotNull ServerPlayer actor, @NotNull ServerPlayer observer) {
+        return canSeePlayer(actor.server, actor.getUUID(), observer);
     }
 
-    static boolean canSeePlayer(@NotNull MinecraftServer server, @NotNull UUID uuid, @NotNull ServerPlayer viewer) {
-        return canSeePlayer(server, uuid, viewer.createCommandSourceStack());
+    static boolean canSeePlayer(@NotNull MinecraftServer server, @NotNull UUID uuid, @NotNull ServerPlayer observer) {
+        return canSeePlayer(server, uuid, observer.createCommandSourceStack());
     }
 
-    static boolean canSeePlayer(@NotNull MinecraftServer server, @NotNull UUID uuid, @NotNull CommandSourceStack viewer) {
-        return VanishManager.canSeePlayer(server, uuid, viewer);
+    static boolean canSeePlayer(@NotNull MinecraftServer server, @NotNull UUID uuid, @NotNull CommandSourceStack observer) {
+        return VanishManager.canSeePlayer(server, uuid, observer);
     }
 
-    static boolean canViewVanished(SharedSuggestionProvider src) {
-        return VanishManager.canViewVanished(src);
+    static boolean canViewVanished(SharedSuggestionProvider observer) {
+        return VanishManager.canViewVanished(observer);
     }
 
     /**
      * Returns a list of players that the given {@link CommandSourceStack source} can see
      *
-     * @param source the viewing source context
+     * @param observer the viewing source context
      * @return an immutable list of players that are visible to the source.
      */
     @NotNull
-    static List<ServerPlayer> getVisiblePlayers(@NotNull CommandSourceStack source) {
-        MinecraftServer server = source.getServer();
+    static List<ServerPlayer> getVisiblePlayers(@NotNull CommandSourceStack observer) {
+        MinecraftServer server = observer.getServer();
         ObjectArrayList<ServerPlayer> list = new ObjectArrayList<>();
         for (ServerPlayer player : server.getPlayerList().getPlayers()) {
-            if (canSeePlayer(server, player.getUUID(), source)) {
+            if (canSeePlayer(server, player.getUUID(), observer)) {
                 list.add(player);
             }
         }
         return list;
     }
 
     /**
-     * Returns a list of players that can view the actions of the given {@link ServerPlayer player}
+     * Returns a list of players that can view the actions of the given {@link ServerPlayer}
      *
-     * @param player the player who is observed
+     * @param actor the player who is observed
      * @return an immutable list of players that can view the player
      */
     @NotNull
-    static List<ServerPlayer> getViewingPlayers(@NotNull ServerPlayer player) {
+    @Deprecated(forRemoval = true)
+    static List<ServerPlayer> getViewingPlayers(@NotNull ServerPlayer actor) {
         ObjectArrayList<ServerPlayer> list = new ObjectArrayList<>();
-        for (ServerPlayer viewer : player.server.getPlayerList().getPlayers()) {
-            if (canSeePlayer(player, viewer)) {
-                list.add(viewer);
+        for (ServerPlayer observer : actor.server.getPlayerList().getPlayers()) {
+            if (canSeePlayer(actor, observer)) {
+                list.add(observer);
             }
         }
         return list;
@@ -109,15 +110,34 @@ static List<ServerPlayer> getViewingPlayers(@NotNull ServerPlayer player) {
      * Broadcasts a message that would reveal players vanish status
      * only to players who can see other vanished players
      *
-     * @param player   the player who caused the message
+     * @param actor   the player who caused the message
      * @param message the message that should be shown
      */
-    static void broadcastHiddenMessage(@NotNull ServerPlayer player, @NotNull Component message) {
+    static void broadcastHiddenMessage(@NotNull ServerPlayer actor, @NotNull Component message) {
         MutableComponent component = message.copy();
         component.append(Component.translatable("text.vanish.chat.hidden")
-                .withStyle(ChatFormatting.GRAY, ChatFormatting.ITALIC));
-        for (ServerPlayer viewingPlayer : getViewingPlayers(player)) {
-            viewingPlayer.sendSystemMessage(component);
+            .withStyle(ChatFormatting.GRAY, ChatFormatting.ITALIC));
+        for (ServerPlayer observer : actor.server.getPlayerList().getPlayers()) {
+            if (canSeePlayer(actor, observer)) {
+                observer.sendSystemMessage(component);
+            }
+        }
+    }
+
+    /**
+     * Conditionally send a message that would reveal players vanish status
+     * only if the observer can see other vanished players
+     *
+     * @param actor    the player who caused the message
+     * @param observer the player who could receive the message
+     * @param message  the message that should be shown
+     */
+    static void sendHiddenMessage(@NotNull ServerPlayer actor, @NotNull ServerPlayer observer, @NotNull Component message) {
+        MutableComponent component = message.copy();
+        component.append(Component.translatable("text.vanish.chat.hidden")
+            .withStyle(ChatFormatting.GRAY, ChatFormatting.ITALIC));
+        if (canSeePlayer(actor, observer)) {
+            observer.sendSystemMessage(component);
         }
     }
 

src/main/java/me/drex/vanish/mixin/CommandSourceStackMixin.java

@@ -29,11 +29,11 @@ public abstract class CommandSourceStackMixin {
                     target = "Lnet/minecraft/server/level/ServerPlayer;sendSystemMessage(Lnet/minecraft/network/chat/Component;)V"
             )
     )
-    public void vanish_hideCommandFeedback(ServerPlayer receiver, Component component, Operation<Void> original) {
-        if (this.entity instanceof ServerPlayer serverPlayer && VanishAPI.isVanished(serverPlayer)) {
-            VanishAPI.broadcastHiddenMessage(serverPlayer, component);
+    public void vanish_hideCommandFeedback(ServerPlayer observer, Component component, Operation<Void> original) {
+        if (this.entity instanceof ServerPlayer actor && VanishAPI.isVanished(actor)) {
+            VanishAPI.sendHiddenMessage(actor, observer, component);
         } else {
-            original.call(receiver, component);
+            original.call(observer, component);
         }
     }
 

src/main/java/me/drex/vanish/mixin/EntityMixin.java

@@ -13,17 +13,17 @@
 public abstract class EntityMixin {
 
     @Inject(method = "broadcastToPlayer", at = @At("HEAD"), cancellable = true)
-    public void vanish_shouldBroadcast(ServerPlayer other, CallbackInfoReturnable<Boolean> cir) {
+    public void vanish_shouldBroadcast(ServerPlayer observer, CallbackInfoReturnable<Boolean> cir) {
         Entity self = (Entity) (Object) this;
-        ServerPlayer executive;
+        ServerPlayer actor;
         if (self instanceof ServerPlayer player) {
-            executive = player;
+            actor = player;
         } else if (self instanceof TraceableEntity traceableEntity && traceableEntity.getOwner() instanceof ServerPlayer owner) {
-            executive = owner;
+            actor = owner;
         } else {
             return;
         }
-        if (!VanishAPI.canSeePlayer(executive, other)) {
+        if (!VanishAPI.canSeePlayer(actor, observer)) {
             cir.setReturnValue(false);
         }
     }

src/main/java/me/drex/vanish/mixin/EntitySelectorMixin.java

@@ -17,9 +17,9 @@ public abstract class EntitySelectorMixin {
     @Inject(method = "findPlayers", at = @At("RETURN"))
     public void vanish_removeVanishedPlayers(CommandSourceStack src, CallbackInfoReturnable<List<ServerPlayer>> cir) {
         List<ServerPlayer> players = cir.getReturnValue();
-        ServerPlayer viewer = src.getPlayer();
-        if (viewer != null) {
-            players.removeIf((executor) -> !VanishAPI.canSeePlayer(executor, viewer));
+        ServerPlayer observer = src.getPlayer();
+        if (observer != null) {
+            players.removeIf((actor) -> !VanishAPI.canSeePlayer(actor, observer));
         }
     }
 

src/main/java/me/drex/vanish/mixin/ListPlayersCommandMixin.java

@@ -21,8 +21,8 @@ public abstract class ListPlayersCommandMixin {
                     target = "Lnet/minecraft/server/players/PlayerList;getPlayers()Ljava/util/List;"
             )
     )
-    private static List<ServerPlayer> vanish_removeVanishedPlayers(PlayerList playerList, CommandSourceStack source) {
-        return VanishAPI.getVisiblePlayers(source);
+    private static List<ServerPlayer> vanish_removeVanishedPlayers(PlayerList playerList, CommandSourceStack observer) {
+        return VanishAPI.getVisiblePlayers(observer);
     }
 
 }

src/main/java/me/drex/vanish/mixin/PlayerListMixin.java

@@ -28,9 +28,9 @@ public abstract class PlayerListMixin {
                     target = "Lnet/minecraft/server/players/PlayerList;broadcastSystemMessage(Lnet/minecraft/network/chat/Component;Z)V"
             )
     )
-    public void vanish_hideJoinMessage(PlayerList playerList, Component component, boolean bl, Operation<Void> original, Connection connection, ServerPlayer player) {
-        if (VanishAPI.isVanished(player)) {
-            VanishAPI.broadcastHiddenMessage(player, component);
+    public void vanish_hideJoinMessage(PlayerList playerList, Component component, boolean bl, Operation<Void> original, Connection connection, ServerPlayer actor) {
+        if (VanishAPI.isVanished(actor)) {
+            VanishAPI.broadcastHiddenMessage(actor, component);
         } else {
             original.call(playerList, component, bl);
         }
@@ -44,8 +44,8 @@ public void vanish_hideJoinMessage(PlayerList playerList, Component component, b
             )
     )
     public boolean vanish_hideGameEvents(ServerGamePacketListenerImpl packetListener, Packet<?> packet, Player player) {
-        if (player instanceof ServerPlayer executor) {
-            return VanishAPI.canSeePlayer(executor, packetListener.player);
+        if (player instanceof ServerPlayer actor) {
+            return VanishAPI.canSeePlayer(actor, packetListener.player);
         } else {
             return true;
         }

src/main/java/me/drex/vanish/mixin/ServerGamePacketListenerImplMixin.java

@@ -44,7 +44,7 @@ public abstract class ServerGamePacketListenerImplMixin {
     public void vanish_modifyPackets(Packet<?> packet, @Nullable PacketSendListener packetSendListener, CallbackInfo ci) {
         if (packet instanceof ClientboundTakeItemEntityPacket takeItemEntityPacket) {
             Entity entity = this.player.level().getEntity(takeItemEntityPacket.getPlayerId());
-            if (entity instanceof ServerPlayer executive && !VanishAPI.canSeePlayer(executive, this.player)) {
+            if (entity instanceof ServerPlayer actor && !VanishAPI.canSeePlayer(actor, this.player)) {
                 this.send(new ClientboundRemoveEntitiesPacket(takeItemEntityPacket.getItemId()));
                 ci.cancel();
             }

src/main/java/me/drex/vanish/mixin/ServerPlayerMixin.java

@@ -22,8 +22,10 @@ public abstract class ServerPlayerMixin {
                     target = "Lnet/minecraft/server/players/PlayerList;broadcastSystemToTeam(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/network/chat/Component;)V"
             )
     )
-    private void vanish_hideDeathMessage(PlayerList playerList, Player player, Component component, Operation<Void> original) {
+    private void vanish_hideTeamDeathMessage(PlayerList playerList, Player player, Component component, Operation<Void> original) {
         if (VanishAPI.isVanished((ServerPlayer) (Object) this)) {
+            // This will send the message to all players, who can view vanished players instead of team only
+            // If this causes any issues for you, make sure to open an issue
             VanishAPI.broadcastHiddenMessage((ServerPlayer) (Object) this, component);
         } else {
             original.call(playerList, player, component);
@@ -37,8 +39,10 @@ private void vanish_hideDeathMessage(PlayerList playerList, Player player, Compo
                     target = "Lnet/minecraft/server/players/PlayerList;broadcastSystemToAllExceptTeam(Lnet/minecraft/world/entity/player/Player;Lnet/minecraft/network/chat/Component;)V"
             )
     )
-    private void vanish_hideDeathMessage2(PlayerList playerList, Player player, Component component, Operation<Void> original) {
+    private void vanish_hideExceptTeamDeathMessage(PlayerList playerList, Player player, Component component, Operation<Void> original) {
         if (VanishAPI.isVanished((ServerPlayer) (Object) this)) {
+            // This will send the message to all players, who can view vanished players instead of all except the same team
+            // If this causes any issues for you, make sure to open an issue
             VanishAPI.broadcastHiddenMessage((ServerPlayer) (Object) this, component);
         } else {
             original.call(playerList, player, component);
@@ -52,7 +56,7 @@ private void vanish_hideDeathMessage2(PlayerList playerList, Player player, Comp
                     target = "Lnet/minecraft/server/players/PlayerList;broadcastSystemMessage(Lnet/minecraft/network/chat/Component;Z)V"
             )
     )
-    private void vanish_hideDeathMessage3(PlayerList playerList, Component component, boolean overlay, Operation<Void> original) {
+    private void vanish_hideDeathMessage(PlayerList playerList, Component component, boolean overlay, Operation<Void> original) {
         if (VanishAPI.isVanished((ServerPlayer) (Object) this)) {
             VanishAPI.broadcastHiddenMessage((ServerPlayer) (Object) this, component);
         } else {

src/main/java/me/drex/vanish/util/VanishManager.java

@@ -64,51 +64,51 @@ public static boolean isVanished(MinecraftServer server, UUID uuid) {
         return data != null && data.vanished;
     }
 
-    public static boolean canViewVanished(SharedSuggestionProvider src) {
-        return Permissions.check(src, "vanish.feature.view", 2);
+    public static boolean canViewVanished(SharedSuggestionProvider observer) {
+        return Permissions.check(observer, "vanish.feature.view", 2);
     }
 
-    public static boolean canSeePlayer(MinecraftServer server, UUID executive, CommandSourceStack viewer) {
-        if (isVanished(server, executive)) {
-            if (viewer.getEntity() != null && executive.equals(viewer.getEntity().getUUID())) {
+    public static boolean canSeePlayer(MinecraftServer server, UUID actor, CommandSourceStack observer) {
+        if (isVanished(server, actor)) {
+            if (observer.getEntity() != null && actor.equals(observer.getEntity().getUUID())) {
                 return true;
             } else {
-                return canViewVanished(viewer);
+                return canViewVanished(observer);
             }
         } else {
             return true;
         }
     }
 
-    public static boolean setVanished(ServerPlayer vanisher, boolean vanish) {
-        if (isVanished(vanisher.server, vanisher.getUUID()) == vanish) return false;
-        if (vanish) vanish(vanisher);
-        VanishData data = PlayerDataApi.getCustomDataFor(vanisher, VANISH_DATA_STORAGE);
+    public static boolean setVanished(ServerPlayer actor, boolean vanish) {
+        if (isVanished(actor.server, actor.getUUID()) == vanish) return false;
+        if (vanish) vanish(actor);
+        VanishData data = PlayerDataApi.getCustomDataFor(actor, VANISH_DATA_STORAGE);
         if (data == null) data = new VanishData();
         data.vanished = vanish;
-        PlayerDataApi.setCustomDataFor(vanisher, VANISH_DATA_STORAGE, data);
-        if (!vanish) unVanish(vanisher);
-        vanisher.server.invalidateStatus();
-        VanishEvents.VANISH_EVENT.invoker().onVanish(vanisher, vanish);
+        PlayerDataApi.setCustomDataFor(actor, VANISH_DATA_STORAGE, data);
+        if (!vanish) unVanish(actor);
+        actor.server.invalidateStatus();
+        VanishEvents.VANISH_EVENT.invoker().onVanish(actor, vanish);
         return true;
     }
 
-    private static void unVanish(ServerPlayer vanisher) {
-        PlayerList list = vanisher.server.getPlayerList();
-        broadcastToOthers(vanisher, ClientboundPlayerInfoUpdatePacket.createPlayerInitializing(Collections.singletonList(vanisher)));
-        list.broadcastSystemMessage(VanishEvents.UN_VANISH_MESSAGE_EVENT.invoker().getUnVanishMessage(vanisher), false);
+    private static void unVanish(ServerPlayer actor) {
+        PlayerList list = actor.server.getPlayerList();
+        broadcastToOthers(actor, ClientboundPlayerInfoUpdatePacket.createPlayerInitializing(Collections.singletonList(actor)));
+        list.broadcastSystemMessage(VanishEvents.UN_VANISH_MESSAGE_EVENT.invoker().getUnVanishMessage(actor), false);
     }
 
-    private static void vanish(ServerPlayer vanisher) {
-        PlayerList list = vanisher.server.getPlayerList();
-        broadcastToOthers(vanisher, new ClientboundPlayerInfoRemovePacket(Collections.singletonList(vanisher.getUUID())));
-        list.broadcastSystemMessage(VanishEvents.VANISH_MESSAGE_EVENT.invoker().getVanishMessage(vanisher), false);
+    private static void vanish(ServerPlayer actor) {
+        PlayerList list = actor.server.getPlayerList();
+        broadcastToOthers(actor, new ClientboundPlayerInfoRemovePacket(Collections.singletonList(actor.getUUID())));
+        list.broadcastSystemMessage(VanishEvents.VANISH_MESSAGE_EVENT.invoker().getVanishMessage(actor), false);
     }
 
-    private static void broadcastToOthers(ServerPlayer vanisher, Packet<?> packet) {
-        for (ServerPlayer viewer : vanisher.server.getPlayerList().getPlayers()) {
-            if (!VanishAPI.canViewVanished(viewer.createCommandSourceStack()) && !viewer.equals(vanisher)) {
-                viewer.connection.send(packet);
+    private static void broadcastToOthers(ServerPlayer actor, Packet<?> packet) {
+        for (ServerPlayer observer : actor.server.getPlayerList().getPlayers()) {
+            if (!VanishAPI.canViewVanished(observer.createCommandSourceStack()) && !observer.equals(actor)) {
+                observer.connection.send(packet);
             }
         }
     }