Feature #7756: Allow server to supply a reason to kicked/banned clients #7859
Conversation
|
Mb I missed something but without actual GUI this looks pretty pointless as server already can send player a message before kicking and this patch doesn't seem to make it any more noticeable. |
|
@ldpl It looks like the patch shows the message in a popup error window (like a network disconnect). That makes it more noticeable, but more importantly the message remains visible after being kicked; an in-game chat message sent at the same time would never be seen. |
bjarnithor99
force-pushed
the
kick-message
branch
from
8bd77a6 to
0853edd
Compare
src/console_cmds.cpp
Outdated
| @@ -469,7 +469,7 @@ DEF_CONSOLE_CMD(ConClearBuffer) | |||
| * Network Core Console Commands | |||
| **********************************/ | |||
|
|
|||
| static bool ConKickOrBan(const char *argv, bool ban) | |||
| static bool ConKickOrBan(const char *argv, bool ban, const char *reason = nullptr) | |||
There was a problem hiding this comment.
I wonder whether this function (and the others) need to have the default parameters. Would be more consistent to explicitly pass nullptr if no reason, imo
There was a problem hiding this comment.
I agree that the functions related to kicking/banning a client should explicitly pass nullptr if no kick reason is supplied. However, the function SendError is used in more cases than just kicking a client, all of which don't support printing a message at the moment. So I'm not sure that removing the default parameter and explicitly passing nullptr every time that SendError is called would increase the readability of the code. What do you think?
There was a problem hiding this comment.
Yeah, that makes sense. Didn't notice that that also applied to SendError
…d clients
This commit adds the missing feature of allowing the server owner to
provide a reason for kicking/banning a client, which the client sees in
a pop-up window after being kicked. The implementation extends the
network protocol by adding a new network action called
NETWORK_ACTION_KICKED that is capable of having an error string, unlike
the other network error packages. Additionally, the kick function
broadcasts a message to all clients about the kicked client and the
reason for the kick.
bjarnithor99
force-pushed
the
kick-message
branch
from
0853edd to
7dac71b
Compare
|
Closes #7756 when merged (thanks for implementing this!) |
|
Nice change, appreciated! But for the love of of God, why would you put the new NetworkAction enum value somewhere at the beginning of the enum in src/network/network_type.h - shifting all following packet numbers around for the admin interface, effectively killing all existing implementations (that otherwise would still work and only stumble about the new KICK packet if not updated)? Good thing I found this so I knew what to look for: https://www.reddit.com/r/openttd/comments/fukwjq/server_1_and_3_version_1100_update_information_or/fmfhmey/ |
|
@YorVeX This was noted already (#8060), and the new value has been moved to the end for 1.10.1. Of course that means anything updated for 1.10.0 will need changing again; decision was that re-breaking some clients that are currently maintained and likely to be fixed was better than a permanent incompatibility. |
|
Ah, sorry, my bad! Good decision there, too. Thanks for the heads-up regarding 1.10.1 ;-) |
This commit adds the missing feature of allowing the server owner to provide a reason for kicking/banning a client. The implementation extends the network protocol by adding a new network action called NETWORK_ACTION_KICKED that is capable of having an error string, unlike the other network error packages. Additionally, the kick function broadcasts a message to all clients about the kicked client and the reason for the kick.
However, for now, when banning a client, the message broadcasted says "kicked" instead of "banned".
Closes #7756