Skip to content
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

Feature #7756: Allow server to supply a reason to kicked/banned clients #7859

Merged
merged 1 commit into from Feb 4, 2020

Conversation

bjarnithor99
Copy link
Contributor

@bjarnithor99 bjarnithor99 commented Dec 12, 2019

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

@ldpl
Copy link
Contributor

ldpl commented Dec 12, 2019

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.

@FLHerne
Copy link
Contributor

FLHerne commented Dec 13, 2019

@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.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

I like it

src/network/network_client.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/network/network_client.cpp Outdated Show resolved Hide resolved
src/network/network_server.cpp Show resolved Hide resolved
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.
@duckfullstop
Copy link
Contributor

duckfullstop commented Jan 24, 2020

Closes #7756 when merged (thanks for implementing this!)

Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@LordAro LordAro merged commit 5880f14 into OpenTTD:master Feb 4, 2020
@YorVeX
Copy link

YorVeX commented Apr 10, 2020

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/

@FLHerne
Copy link
Contributor

FLHerne commented Apr 10, 2020

@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.

See #8060 and #8061.

@YorVeX
Copy link

YorVeX commented Apr 10, 2020

Ah, sorry, my bad! Good decision there, too. Thanks for the heads-up regarding 1.10.1 ;-)

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.

Allow server to supply a reason to kicked / banned clients
7 participants