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

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 left a comment

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
@nielsmh nielsmh added this to the 1.10.0 milestone Jan 12, 2020
@bjarnithor99 bjarnithor99 force-pushed the bjarnithor99:kick-message branch from 8bd77a6 to 0853edd Jan 13, 2020
@@ -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)

This comment has been minimized.

Copy link
@LordAro

LordAro Jan 18, 2020

Member

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

This comment has been minimized.

Copy link
@bjarnithor99

bjarnithor99 Jan 21, 2020

Author Contributor

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?

This comment has been minimized.

Copy link
@LordAro

LordAro Jan 21, 2020

Member

Yeah, that makes sense. Didn't notice that that also applied to SendError

    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 bjarnithor99 force-pushed the bjarnithor99:kick-message branch from 0853edd to 7dac71b Jan 21, 2020
@duckfullstop
Copy link
Contributor

duckfullstop commented Jan 24, 2020

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

Copy link
Contributor

nielsmh left a comment

Looks fine to me.

@bjarnithor99 bjarnithor99 requested a review from LordAro Feb 3, 2020
@LordAro
LordAro approved these changes Feb 4, 2020
@LordAro LordAro merged commit 5880f14 into OpenTTD:master Feb 4, 2020
8 checks passed
8 checks passed
Commit checker
Details
OpenTTD CI Build #20200121.2 succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.9) Linux linux-amd64-clang-3.9 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@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

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.