Kickall#943
Conversation
R00tB33rMan
left a comment
There was a problem hiding this comment.
Only suggestion from me (for now): revert this back due to checkstyle to individual imports
|
The imports in VelocityServer should be updated now. |
(Refer to our /send, /glist, & /plist commands). Can you implement something a bit more like this? I feel like it'd match up better with our current conventions and would allow the |
There was a problem hiding this comment.
I would rather make the /gkick command support PlayerIdentifier args like all the other CTD commands instead of introducing a new command for this. That would keep it aligned with all orher commands.
That would allow you to execute /gkick all, /gkick +server and /gkick -proxy for free, as PlayerIdentifier already implements this functionality for commands. Of course check this newly introduced permission before actually kicking each player.
|
Also make sure to document the permission in README |
WouterGritter
left a comment
There was a problem hiding this comment.
Still requesting changes, as per my previous comment, this functionality is better suited to be a part of /gkick. Allowing this existing command to support player identifiers aligns it with all other CTD commands that can perform actions on one player or a group of players, unless you have a good reason why you want this to be a separate command instead?
|
I'm working on the changes to the "/gkick" command. I've been having seizures again, so there may be a delay. I simply added the permission nodes to the README in the meantime until the updated code for "/gkick" is ready. |
Not an issue at all of course! Wishing you good health, and take your time. |
Removed GkickAllCommand registration from VelocityServer.
Removed gkickall command functionality and related checks.
|
I've removed the original "/kickall" command and updated the code for "/gkick". Is this what you're looking for, or are more changes needed? |
WouterGritter
left a comment
There was a problem hiding this comment.
The current approach deviates from other commands that accept "all", "specific server", "specific proxy" and "specific player". /alert and /transfer to name a few make use of our helper class PlayerIdentifier that computes the target(s) from a string like all, +<server> -<proxy> or <player>. This avoids having the second argument (e.g. /gkick server cool_server would become /gkick +cool_server), and works elegantly with the suggestionsprovider PlayerIdentifier already exposes:

To get his feature merged it really needs to be aligned with the other commands and make use of PlayerIdentifier internally, not only to make the command arguments work the same as the other commands, but also to avoid duplicate code.
Currently /gkick Notch would break here, you would need to type /gkick player Notch. messages.properties on existing Velocity-CTD setups don't migrate, meaning they will see the old usage message (/gkick <player> [reason]). This could be misleading if this stops working, though luckily refactoring to use PlayerIdentifier will make this work again.
| velocity.command.find.usage=<red>/<arg:0> <player> | ||
| velocity.command.find.message=<red><arg:0> <white>is currently connected to <red><arg:1><white>. | ||
| velocity.command.gkick.usage=<red>/<arg:0> <player> [reason] | ||
| velocity.command.gkick.usage=<red>Usage: /<arg:0> <all|server|player> <target> [reason] |
There was a problem hiding this comment.
Adding Usage: makes this usage string unique to all other usage strings. Let's keep this aligned with all others for now (drop Usage: ), if we want to add this prefix this is something for another PR and should be done for all similar strings.
| return SINGLE_SUCCESS; | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Formatting (re-introduce newline)
| .arguments(Argument.string("0", player.getUsername())) | ||
| source.sendMessage( | ||
| Component.translatable("velocity.command.gkick.message") | ||
| .arguments(Argument.string("0", player.getUsername())) |
There was a problem hiding this comment.
Please reformat the entire file with a continuation indent of 4 instead of 8. Here it's most visible in the diff but the continuation indentation is wrong in the entire class.
|
|
||
| /** | ||
| * Implements Velocity-CTD's {@code /gkick} command. | ||
| * Implements Velocity-CTD's flexible {@code /gkick} command. |
There was a problem hiding this comment.
I'd drop the flexible wording, we don't have this anywhere else.
| for (VelocityClusterPlayer player : targets) { | ||
| if (bypassPermission != null) { | ||
| boolean bypass = server.getPlayer(player.getUniqueId()) | ||
| .map(nativePlayer -> nativePlayer.hasPermission(bypassPermission)) |
There was a problem hiding this comment.
This permission check won't work on remote players in a proxy cluster setup. This will require ClusterPlayer to expose a new flag (isBypassGkick), and to wire the redis system to store this in PlayerEntry
| RequiredArgumentBuilder<CommandSource, String> serverNameNode = | ||
| BrigadierCommand.requiredArgumentBuilder("name", StringArgumentType.word()) | ||
| .suggests((ctx, builder) -> { | ||
| for (com.velocitypowered.api.proxy.server.RegisteredServer registeredServer : server.getAllServers()) { |
There was a problem hiding this comment.
Formatting (use of FQDN while its imported)
| Optional<RegisteredServer> registeredServer = server.getServer(serverName) | ||
| .map(RegisteredServer.class::cast); | ||
|
|
||
| if (registeredServer.isEmpty()) { |
There was a problem hiding this comment.
No need to cast the result of server.getServer at line 143 just to check isEmpty(), these 3 lines can be condensed to a single if (server.getServer(serverName).isEmpty())
|
Is this code for "/gkick" better? |
|
This has been cleanly resolved in: #971 - I appreciate the effort; however, there are a lot of oddities that weren't properly addressed here. I appreciate the contribution but this has been resolved! |
I would like to suggest the addition of a new command for Velocity-CTD: "/gkickall".
The "/gkickall" command would provide a way to easily kick all players from anywhere on the network. I've also included a bypass permission node: "velocity.command.gkickall.bypass"