widelands

Merge lp://staging/~widelands-dev/widelands/bug-1826744-lobby-commands into lp://staging/widelands

  1. bug-1826744-lobby-commands
  2. Merge into trunk
Proposed by Notabilis
Status: Merged
Merged at revision: 9148
Proposed branch: lp://staging/~widelands-dev/widelands/bug-1826744-lobby-commands
Merge into: lp://staging/widelands
Diff against target: 93 lines (+40/-2)
3 files modified
src/network/internet_gaming.cc (+30/-2)
src/network/internet_gaming_messages.cc (+1/-0)
src/network/internet_gaming_protocol.h (+9/-0)
To merge this branch: bzr merge lp://staging/~widelands-dev/widelands/bug-1826744-lobby-commands
Reviewer Review Type Date Requested Status
GunChleoc Approve
Toni Förster Approve
Review via email: mp+368285@code.staging.launchpad.net

Commit message

Adding support for /warn and /kick commands of superusers in the internet gaming lobby.

Description of the change

/warn is sending a system message to a single user.
/kick is banning the IP of the user for 24 hours to avoid immediate reconnects.

To post a comment you must log in.
Revision history for this message
Toni Förster (stonerl) wrote :

Could you change "/announcement" to "/announce"? That's the command for the multiplayer lobby.

Also, could you implement /help for the lobby (for users and superusers)?

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 5145. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/540930580.
Appveyor build 4927. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1826744_lobby_commands-4927.

Revision history for this message
Notabilis (notabilis27) wrote :

I can rename /announce, no problem. But what is /help supposed to do for normal users? They only have /me as a "command". And the /help command for superusers is already part of this branch, or do you want something different there?

Revision history for this message
Toni Förster (stonerl) wrote :

Sorry, I was under the impression that users could use the help command as well.

The it is just /announcement command that needs to be changed :)

Revision history for this message
Notabilis (notabilis27) wrote :

Ah, okay. Now it makes sense. :)
The /help command visible in the diff is within an if-block that only triggers for superusers.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 5151. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/541357325.
Appveyor build 4933. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1826744_lobby_commands-4933.

Revision history for this message
GunChleoc (gunchleoc) wrote :

1 small string nit.

Revision history for this message
Notabilis (notabilis27) wrote :

Thanks for the reviews. The strings are fixed now. I also "added" the requested /ban command (actually, it will simply be forwarded to the metaserver).

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 5159. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/541867359.
Appveyor build 4941. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1826744_lobby_commands-4941.

Revision history for this message
Toni Förster (stonerl) wrote :

LGTM :)

But we need the server to be in place to test this better.

review: Approve
Revision history for this message
Notabilis (notabilis27) wrote :

The related metaserver branch is now deployed.

Revision history for this message
Toni Förster (stonerl) wrote :

Admins shouldn't be able to kick or ban themselves or other admins.

review: Needs Fixing
Revision history for this message
Toni Förster (stonerl) wrote :

But I guess this should be solved on the server side...

Revision history for this message
GunChleoc (gunchleoc) wrote :

Yes it should. Are you happy with your testing otherwise?

If so, we should merge this so it will be easier for Notabilis to continue with the needed changes on the server.

Revision history for this message
Toni Förster (stonerl) wrote :

I'm happy :)

@bunnybot merge

review: Approve
Revision history for this message
Notabilis (notabilis27) wrote :

Thanks for the reviews. A metaserver branch with the requested change is open at https://github.com/widelands/widelands_metaserver/pull/60 and already deployed for testing.

Revision history for this message
GunChleoc (gunchleoc) wrote :

I have found some things that still need fixing/improving - most of them on the server side. So, I have created a new Metaserver issue:

https://github.com/widelands/widelands_metaserver/issues/63

This branch can go in though :)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: