Merge lp:~widelands-dev/widelands/net-user-type into lp:widelands

[Notabilis]

Cleaner transmission of client status

Some small changes regarding transmitted client status and IRC clients.

- That a metaserver client is an IRC user is now transmitted as permission instead of as build-id. Avoids the theoretical problem that a Widelands client could have a build-id of "IRC".

- No longer block private messages to IRC users client side. This is now blocked on the metaserver.

- No longer sending (not existing) "points" of a user when transmitting the client list. If someone has an objection against removing these "points" (e.g., there are plans to use them), please say so.

- cleaning up code for "messages beeps" when receiving a non-system message.

Note: Don't merge/deploy this, I should do that myself since the protocol version has not been changed yet.

[Notabilis (notabilis27)]

Oh, and does someone know something about the "bots"?

  static const std::string INTERNET_CLIENT_BOT = "BOT";

I removed it now since it does not seem to be used, neither in Widelands nor on the metaserver.

[GunChleoc (gunchleoc)]

I don't know anything about bots, sorry.

I think the points can go - they were never displayed anyway, and the screen space is now used for displaying the client version.

Code LGTM :)

review: Approve

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3393. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/369523180.
Appveyor build 3199. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_net_user_type-3199.

[Klaus Halfmann (klaus-halfmann)]

* I assume that "BOT" was intended for chat or game Bots (we have/had some on irc, do we?)

* A first review of the diffs shows me no obvious Issues, some nits in the comments.

* I will now compile the code an do some review on the complete internet_gaming* files.
  (More for me to get familiar with that code)

* Notabalis: I will try to be online with that version starting at perhaps 16:00 CEST,
  so we can try you changes "live" in chat and on IRC.
  Maybe we can hunt down these IRC Bots, too?

* GunCheloc: FYI with latest XCode updates Apple has pushed clang to a newer version,
  so a lot of newer options finally work here and some warnigns are finally gone:
  Apple LLVM version 9.1.0 (clang-902.0.39.1)
  Target: x86_64-apple-darwin17.5.0
  Thread model: posix
  InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

[Klaus Halfmann (klaus-halfmann)]

I checekd the Multiplayer lobby and it looks garbled now,
will you need a screenshot?

I did a small improvemenet in gamechatpanel.cc, not sure if I should commit it?

review: Needs Fixing (compile, testplay)

[GunChleoc (gunchleoc)]

@Klaus: Glad it's working better now. I have opened a bug to update Travis with the new version too: https://bugs.launchpad.net/widelands/+bug/1766069

[Notabilis (notabilis27)]

Sorry, should have mentioned it: This branch is not compatible with the currently running metaserver. So its "fine" that it looks strange.

Feel free to commit your change. I will look at your suggestions later on.

[Notabilis (notabilis27)]

It looks indeed as it might have been a game or chat bot. But I don't know whether there have been any at some time. I am not aware of any that are currently active.

Thanks for the review and for fixing the one nit. I replaced the numeric constants with const variables, an enum wasn't possible since we have to compare the values. But it looks much better now.

Regarding testing this branch: If you want to test a branch which changes something on the online gaming protocol (or the relay protocol), you have to run your own metaserver based on the matching branch of it (https://github.com/widelands/widelands_metaserver/pull/29 is the merge request on the metaserver code).
To make widelands use your local metaserver, start it with "--metaserver=localhost" as parameter. Later on you have to either clean the line from the configuration file or start with "--metaserver=widelands.org".

[GunChleoc (gunchleoc)]

Can you please document the metaserver option in a separate branch? This is where it should go:

https://bazaar.launchpad.net/~widelands-dev/widelands/trunk/view/head:/src/wlapplication_messages.cc

[Notabilis (notabilis27)]

Oh! I wasn't aware that this isn't documented. Thanks for the pointer, I will do so.

[kaputtnik (franku)]

The bot is maybe a holdover from the time ggz was used?

http://www.ggzgamingzone.org

[Klaus Halfmann (klaus-halfmann)]

Sorry I was unable to the get wlms server going:

klaus$ ./go/bin/wlms
2018/05/01 10:07:50 No configuration found, using in-memory database
2018/05/01 10:07:50 Using 127.0.0.1 and fe80::1 as IP addresses of the relay
2018/05/01 10:07:50 Unable to connect to relay server at localhost: dial tcp [::1]:7398: connect: connection refused
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x13152c1]

goroutine 53 [running]:
main.(*Server).mainLoop(0xc4201480c0)
 /Users/klaus/go/src/github.com/widelands/widelands_metaserver/wlms/server.go:450 +0x41
created by main.CreateServerUsing
 /Users/klaus/go/src/github.com/widelands/widelands_metaserver/wlms/server.go:445 +0x5b0

And I was unbale to git checkout that pullrequerst, either. No time to really deep dive into that code yet.

Still the code change is ok for me. So how to we get this into the offical server now?

review: Approve (review, compile)

[GunChleoc (gunchleoc)]

> So how to we get this into the offical server now?

We leave that up to Notabilis. He knows how to do it :)

Do not merge this branch, let him handle it, because it needs to be coordinated.

[Notabilis (notabilis27)]

Hm, well... Unfortunately its not easy to get it running, actually I am surprised you got that far. I had quite some trouble getting it compiled when I first tried to.
What is missing in your case is a running relay server. Seems that the "documentation" is incomplete, sorry for that. You have to compile and run the wlnr executable, afterwards the wlms can connect to it and it should start correctly.
Currently I am working on some semi-automated test scripts for the metaserver but they are far from useful yet.

I will do some local testing with my three branches now and if they work together, I will merge and deploy them.

[Notabilis (notabilis27)]

@bunnybot merge

[Notabilis (notabilis27)]

Thanks for the reviews! The new version is online and seems to be working. If someone notices a bug, please say so.

[GunChleoc (gunchleoc)]

Will do. Thanks!

=== modified file 'src/network/internet_gaming.cc'
--- src/network/internet_gaming.cc	2018-04-07 16:59:00 +0000
+++ src/network/internet_gaming.cc	2018-04-22 16:47:03 +0000
@@ -606,8 +606,7 @@
 				inc.build_id = packet.string();
 				inc.game = packet.string();
 				inc.type = packet.string();
-				inc.points = packet.string();
-				if (inc.build_id == "IRC") {
+				if (inc.type == INTERNET_CLIENT_IRC) {
 					irc.push_back(inc);
 					// No "join" or "left" messages for IRC users
 					continue;
@@ -631,7 +630,7 @@
 			clientlist_.insert(clientlist_.end(), irc.begin(), irc.end());
 
 			for (InternetClient& client : old) {
-				if (client.name.size() && client.build_id != "IRC") {
+				if (client.name.size() && client.type != INTERNET_CLIENT_IRC) {
 					format_and_add_chat(
 					   "", "", true, (boost::format(_("%s left the lobby")) % client.name).str());
 				}
@@ -867,17 +866,9 @@
 			   _("Message could not be sent: Was this supposed to be a private message?"));
 			return;
 		}
-		std::string recipient = msg.substr(1, space - 1);
-		for (const InternetClient& client : clientlist_) {
-			if (recipient == client.name && client.build_id == "IRC") {
-				format_and_add_chat(
-				   "", "", true, _("Private messages to IRC users are not supported."));
-				return;
-			}
-		}
 
 		s.string(trimmed);    // message
-		s.string(recipient);  // recipient
+		s.string(msg.substr(1, space - 1));  // recipient
 
 		format_and_add_chat(clientname_, msg.substr(1, space - 1), false, msg.substr(space + 1));
 

=== modified file 'src/network/internet_gaming.h'
--- src/network/internet_gaming.h	2018-04-07 16:59:00 +0000
+++ src/network/internet_gaming.h	2018-04-22 16:47:03 +0000
@@ -37,7 +37,6 @@
 	std::string build_id;
 	std::string game;
 	std::string type;
-	std::string points;  // Currently unused
 };
 
 /// A simple network game struct

=== modified file 'src/network/internet_gaming_protocol.h'
--- src/network/internet_gaming_protocol.h	2018-04-07 16:59:00 +0000
+++ src/network/internet_gaming_protocol.h	2018-04-22 16:47:03 +0000
@@ -71,7 +71,7 @@
 static const std::string INTERNET_CLIENT_UNREGISTERED = "UNREGISTERED";
 static const std::string INTERNET_CLIENT_REGISTERED = "REGISTERED";
 static const std::string INTERNET_CLIENT_SUPERUSER = "SUPERUSER";
-static const std::string INTERNET_CLIENT_BOT = "BOT";
+static const std::string INTERNET_CLIENT_IRC = "IRC";
 
 /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
  * COMMUNICATION PROTOCOL BETWEEN CLIENT AND METASERVER                    *
@@ -322,10 +322,8 @@
 
 /**
  * Sent by the metaserver to inform the client, that the list of clients was changed. No payload is
- * sent,
- * as e.g. clients in a game are not really interested about other clients and we want to keep
- * traffic
- * as low as possible.
+ * sent, as e.g. clients in a game are not really interested about other clients and we want to
+ * keep traffic as low as possible.
  *
  * To get the new list of clients, the client must send \ref IGPCMD_CLIENT
  */
@@ -338,9 +336,8 @@
  * \li string:    Number of client packages and for uint8_t i = 0; i < num; ++i {:
  * \li string:    Name of the client
  * \li string:    Widelands version
- * \li string:    Game the player is connected to, else empty.
+ * \li string:    Game the player is connected to, else empty
  * \li string:    Clients rights (see client rights section above)
- * \li string:    Points of the client
  * }
  */
 static const std::string IGPCMD_CLIENTS = "CLIENTS";

=== modified file 'src/ui_fsmenu/internet_lobby.cc'
--- src/ui_fsmenu/internet_lobby.cc	2018-04-07 16:59:00 +0000
+++ src/ui_fsmenu/internet_lobby.cc	2018-04-22 16:47:03 +0000
@@ -35,6 +35,16 @@
 #include "random/random.h"
 #include "ui_basic/messagebox.h"
 
+namespace {
+
+	// Constants for convert_clienttype() / compare_clienttype()
+	const uint8_t kClientUnregistered = 0;
+	const uint8_t kClientRegistered = 1;
+	const uint8_t kClientSuperuser = 2;
+	// 3 was INTERNET_CLIENT_BOT which is not used
+	const uint8_t kClientIRC = 4;
+}
+
 FullscreenMenuInternetLobby::FullscreenMenuInternetLobby(char const* const nick,
                                                          char const* const pwd,
                                                          bool registered)
@@ -232,13 +242,13 @@
 
 uint8_t FullscreenMenuInternetLobby::convert_clienttype(const std::string& type) {
 	if (type == INTERNET_CLIENT_REGISTERED)
-		return 1;
+		return kClientRegistered;
 	if (type == INTERNET_CLIENT_SUPERUSER)
-		return 2;
-	if (type == INTERNET_CLIENT_BOT)
-		return 3;
+		return kClientSuperuser;
+	if (type == INTERNET_CLIENT_IRC)
+		return kClientIRC;
 	// if (type == INTERNET_CLIENT_UNREGISTERED)
-	return 0;
+	return kClientUnregistered;
 }
 
 /**
@@ -262,27 +272,24 @@
 			er.set_string(2, client.build_id);
 			er.set_string(3, client.game);
 
-			if (client.build_id == "IRC") {
-				// No icon for IRC users
-				continue;
-			}
-
 			const Image* pic;
 			switch (convert_clienttype(client.type)) {
-			case 0:  // UNREGISTERED
+			case kClientUnregistered:
 				pic = g_gr->images().get("images/wui/overlays/roadb_red.png");
 				er.set_picture(0, pic);
 				break;
-			case 1:  // REGISTERED
+			case kClientRegistered:
 				pic = g_gr->images().get("images/wui/overlays/roadb_yellow.png");
 				er.set_picture(0, pic);
 				break;
-			case 2:  // SUPERUSER
-			case 3:  // BOT
+			case kClientSuperuser:
 				pic = g_gr->images().get("images/wui/overlays/roadb_green.png");
 				er.set_color(RGBColor(0, 255, 0));
 				er.set_picture(0, pic);
 				break;
+			case kClientIRC:
+				// No icon for IRC users
+				continue;
 			default:
 				continue;
 			}
@@ -301,11 +308,6 @@
 	if (clientsonline_list_.has_selection()) {
 		UI::Table<const InternetClient* const>::EntryRecord& er = clientsonline_list_.get_record(i);
 
-		if (er.get_string(2) == "IRC") {
-			// No PM to IRC users
-			return;
-		}
-
 		std::string temp("@");
 		temp += er.get_string(1);
 		std::string text(chat.get_edit_text());

=== modified file 'src/wui/gamechatpanel.cc'
--- src/wui/gamechatpanel.cc	2018-04-07 16:59:00 +0000
+++ src/wui/gamechatpanel.cc	2018-04-22 16:47:03 +0000
@@ -45,7 +45,7 @@
              g_gr->images().get("images/ui_basic/but1.png"),
              UI::MultilineTextarea::ScrollMode::kScrollLogForced),
      editbox(this, 0, h - 20, w, 20, 2),
-     chat_message_counter(std::numeric_limits<uint32_t>::max()) {
+     chat_message_counter(0) {
 	editbox.ok.connect(boost::bind(&GameChatPanel::key_enter, this));
 	editbox.cancel.connect(boost::bind(&GameChatPanel::key_escape, this));
 	editbox.activate_history(true);
@@ -65,31 +65,27 @@
 void GameChatPanel::recalculate() {
 	const std::vector<ChatMessage> msgs = chat_.get_messages();
 
+	size_t msgs_size = msgs.size();
 	std::string str = "<rt>";
-	for (uint32_t i = 0; i < msgs.size(); ++i) {
+	for (uint32_t i = 0; i < msgs_size; ++i) {
 		str += format_as_richtext(msgs[i]);
 	}
 	str += "</rt>";
 
 	chatbox.set_text(str);
 
-	// If there are new messages, play a sound
-	if (0 < msgs.size() && msgs.size() != chat_message_counter) {
-		// computer generated ones are ignored
-		// Note: if many messages arrive simultaneously,
-		// the latest is a system message and some others
-		// are not, then this act wrong!
-		if (!msgs.back().sender.empty() && !chat_.sound_off()) {
-			// The latest message is not a system message
-			if (std::string::npos == msgs.back().sender.find("(IRC)") &&
-			    chat_message_counter < msgs.size()) {
-				// The latest message was not relayed from IRC.
-				// The above built-in string constant should match
-				// that of the IRC bridge.
+	if (chat_message_counter < msgs_size) { // are there new messages?
+		if (!chat_.sound_off()) { // play a sound, if needed
+			for (size_t i = chat_message_counter; i < msgs_size; ++i) {
+				if (msgs[i].sender.empty()) {
+					continue; // System message. Don't play a sound
+				}
+				// Got a message that is no system message. Beep
 				play_new_chat_message();
+				break;
 			}
 		}
-		chat_message_counter = msgs.size();
+		chat_message_counter = msgs_size;
 	}
 }
 

=== modified file 'src/wui/gamechatpanel.h'
--- src/wui/gamechatpanel.h	2018-04-07 16:59:00 +0000
+++ src/wui/gamechatpanel.h	2018-04-22 16:47:03 +0000
@@ -58,7 +58,7 @@
 	ChatProvider& chat_;
 	UI::MultilineTextarea chatbox;
 	UI::EditBox editbox;
-	uint32_t chat_message_counter;
+	size_t chat_message_counter;
 	std::unique_ptr<Notifications::Subscriber<ChatMessage>> chat_message_subscriber_;
 };