Merge lp:~widelands-dev/widelands/bug-1827182-sort-client-list into lp:widelands

[Toni Förster]

sort the lobby client list by client type admin->irc

The admin users will be on top of the list, followed by registered
and unregistered. IRC user are at the bottom as they are already.

IRC user's join/part messages will also be displayed

Set focus to chat input in lobby & mp by default.

Clicking on the chatbox gives focus to input field. Clicking on any
other UI element removes focus.

[Toni Förster (stonerl)]

I do need some help here. This is good as it is now, but I'd like to sort the irc/superuser/registered list alphabetically before inserting them into clientlist_. Can some tell me how to do this.

[GunChleoc (gunchleoc)]

Define a compare functions for the table column in the UI, using the set_column_compare function. See campaign_select.cc for an example.

Then you will only need 1 vector for the clients without copying anything.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4854. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/527360011.
Appveyor build 4635. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1827182_sort_client_list-4635.

[Toni Förster (stonerl)]

I found a way to sort clientlist_ alphabetically.

But how do I tell the clientsonline_list_ in internet_lobby.cc that it should sort by client type as default. Currently, it just takes the list and I have to manually click on the "*"-header to sort it.

[Toni Förster (stonerl)]

okay found it :D

[Toni Förster (stonerl)]

Addressed your review, and one diff comment.

[GunChleoc (gunchleoc)]

LGTM :)

This branch can go in once codecheck has been fixed. If you don't want to trigger a compile for this, you can use

cmake/codecheck/CodeCheck.py src/* | grep -v "src/third_party"

review: Approve

[Toni Förster (stonerl)]

Done. :)

[GunChleoc (gunchleoc)]

@bunnybot merge

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4866. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/527644741.
Appveyor build 4647. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1827182_sort_client_list-4647.

[bunnybot (widelandsofficial)]

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4866. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/527644741.

[Toni Förster (stonerl)]

Sorry I turned this into a lobby/chat cleanup merge request. Gun would you be so kind and have another look, please?

[GunChleoc (gunchleoc)]

I an crash this.

1. Host a new LAN game
2. Type something
3. Click on the icon next to your username

widelands: ../src/ui_basic/panel.h:267: bool UI::Panel::has_focus() const: Assertion `get_can_focus()' failed.

Thread 1 "widelands" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) backtrace
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1 0x00007ffff434c801 in __GI_abort () at abort.c:79
#2 0x00007ffff433c39a in __assert_fail_base (fmt=0x7ffff44c37d8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x5555576043e0 "get_can_focus()",
    file=file@entry=0x5555576043a0 "../src/ui_basic/panel.h", line=line@entry=267,
    function=function@entry=0x55555760cd20 <UI::Panel::has_focus() const::__PRETTY_FUNCTION__> "bool UI::Panel::has_focus() const") at assert.c:92
#3 0x00007ffff433c412 in __GI___assert_fail (assertion=0x5555576043e0 "get_can_focus()", file=0x5555576043a0 "../src/ui_basic/panel.h", line=267,
    function=0x55555760cd20 <UI::Panel::has_focus() const::__PRETTY_FUNCTION__> "bool UI::Panel::has_focus() const") at assert.c:101
#4 0x0000555556af02c4 in UI::Panel::has_focus (this=0x61a00023e590) at ../src/ui_basic/panel.h:267
#5 0x0000555556afc1d0 in UI::EditBox::draw (this=0x61a00023e590, dst=...) at ../src/ui_basic/editbox.cc:364
#6 0x0000555556b2e32d in UI::Panel::do_draw_inner (this=0x61a00023e590, dst=...) at ../src/ui_basic/panel.cc:755
#7 0x0000555556b2ea07 in UI::Panel::do_draw (this=0x61a00023e590, dst=...) at ../src/ui_basic/panel.cc:788
#8 0x0000555556b2e378 in UI::Panel::do_draw_inner (this=0x61a00023e280, dst=...) at ../src/ui_basic/panel.cc:759
#9 0x0000555556b2ea07 in UI::Panel::do_draw (this=0x61a00023e280, dst=...) at ../src/ui_basic/panel.cc:788
#10 0x0000555556b2e378 in UI::Panel::do_draw_inner (this=0x7fffffff5af0, dst=...) at ../src/ui_basic/panel.cc:759
#11 0x0000555556b2ea07 in UI::Panel::do_draw (this=0x7fffffff5af0, dst=...) at ../src/ui_basic/panel.cc:788
#12 0x0000555556b2a015 in UI::Panel::do_run (this=0x7fffffff5af0) at ../src/ui_basic/panel.cc:195
#13 0x00005555563898fe in UI::Panel::run<FullscreenMenuBase::MenuTarget> (this=0x7fffffff5af0) at ../src/ui_basic/panel.h:104
#14 0x0000555556a057ad in GameHost::run (this=0x7fffffff7470) at ../src/network/gamehost.cc:608
#15 0x0000555556376289 in WLApplication::mainmenu_multiplayer (this=0x611000000180) at ../src/wlapplication.cc:1275
#16 0x0000555556374a40 in WLApplication::mainmenu (this=0x611000000180) at ../src/wlapplication.cc:1113
#17 0x000055555636ba12 in WLApplication::run (this=0x611000000180) at ../src/wlapplication.cc:466
#18 0x00005555563677af in main (argc=1, argv=0x7fffffffde88) at ../src/main.cc:44

[Toni Förster (stonerl)]

Hmm, you mean the icon in the client list, where you can choose your colour? Tried this, but I'm unable to crash it...

[Toni Förster (stonerl)]

Okay it only crashes in when compiled in debug mode. Release builds don't crash. Weird.

[Klaus Halfmann (klaus-halfmann)]

Thats the way how Asserts work ... only in debug bilds.
I dont have that much time today, Ill try to throw it at the debugger.

[Toni Förster (stonerl)]

Is this assert even correct? When calling has_focus() I expect either true or false. And it crashes here because it receives a false as answer but expects a true, which doesn't make much sense to me.

[Toni Förster (stonerl)]

I'm pretty sure this assert is wrong here. Removed it in r9101.

[Klaus Halfmann (klaus-halfmann)]

Mhh, that assert looks valid fro me. Question is why this particular inputfield
does not have the corresponding flag set?

[Toni Förster (stonerl)]

If you have any idea let me know. I'm running out of ideas.

[Klaus Halfmann (klaus-halfmann)]

I think this is a better fix:

 bool has_focus() const {
  return (get_can_focus() && parent_->focus_ == this);
 }

That can_ / has_focus handling is a bit different then I had expected, well

Works find for me now

[Toni Förster (stonerl)]

Can you push your fix directly to this branch? Thanks allot.

[Klaus Halfmann (klaus-halfmann)]

Done

[Klaus Halfmann (klaus-halfmann)]

Some regression tais fail, but I dont think they are related:

./regression_test.py -b ./widelands

FAIL: test/maps/ship_transportation.wmf/scripting/test_rip_ship_before_picking_up_transporting_ware.lua

FAIL: test/maps/ship_transportation.wmf/scripting/test_rip_portdock_with_worker_and_ware_in_transit.lua

FAIL: test/maps/ship_transportation.wmf/scripting/test_rip_ports_with_worker_in_transit.lua

Ran 44 tests in 1493.063s

FAILED (failures=3)

[Toni Förster (stonerl)]

> Some regression tais fail, but I dont think they are related:
>

Neither do I. Lets wait for travis.

[GunChleoc (gunchleoc)]

Tested and working :)

review: Approve

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4895. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/528399156.
Appveyor build 4676. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1827182_sort_client_list-4676.

[Toni Förster (stonerl)]

well :)

@bunnybot merge

=== modified file 'src/network/internet_gaming.cc'
--- src/network/internet_gaming.cc	2019-05-01 20:17:37 +0000
+++ src/network/internet_gaming.cc	2019-05-05 12:59:18 +0000
@@ -19,6 +19,7 @@
 
 #include "network/internet_gaming.h"
 
+#include <algorithm>
 #include <memory>
 #include <thread>
 
@@ -527,8 +528,7 @@
 			// Client received the new list of clients
 			uint8_t number = boost::lexical_cast<int>(packet.string()) & 0xff;
 			std::vector<InternetClient> old = clientlist_;
-			// Push IRC users to a second list and add them to the back
-			std::vector<InternetClient> irc;
+			// Push admins/registred/IRC users to a temporary list and add them back later
 			clientlist_.clear();
 			log("InternetGaming: Received a client list update with %u items.\n", number);
 			InternetClient inc;
@@ -537,12 +537,7 @@
 				inc.build_id = packet.string();
 				inc.game = packet.string();
 				inc.type = packet.string();
-				if (inc.type == INTERNET_CLIENT_IRC) {
-					irc.push_back(inc);
-					// No "join" or "left" messages for IRC users
-					continue;
-				}
-				// No IRC client
+
 				clientlist_.push_back(inc);
 
 				bool found =
@@ -554,14 +549,18 @@
 						break;
 					}
 				}
-				if (!found)
+				if (!found) {
 					format_and_add_chat(
 					   "", "", true, (boost::format(_("%s joined the lobby")) % inc.name).str());
+				}
 			}
-			clientlist_.insert(clientlist_.end(), irc.begin(), irc.end());
+
+			std::sort(clientlist_.begin(), clientlist_.end(),
+						[](const InternetClient &left, const InternetClient &right)
+						{return (left.name < right.name);});
 
 			for (InternetClient& client : old) {
-				if (client.name.size() && client.type != INTERNET_CLIENT_IRC) {
+				if (client.name.size()) {
 					format_and_add_chat(
 					   "", "", true, (boost::format(_("%s left the lobby")) % client.name).str());
 				}

=== modified file 'src/ui_basic/panel.h'
--- src/ui_basic/panel.h	2019-03-17 11:44:04 +0000
+++ src/ui_basic/panel.h	2019-05-05 12:59:18 +0000
@@ -264,8 +264,7 @@
 		return flags_ & pf_can_focus;
 	}
 	bool has_focus() const {
-		assert(get_can_focus());
-		return (parent_->focus_ == this);
+		return (get_can_focus() && parent_->focus_ == this);
 	}
 	virtual void focus(bool topcaller = true);
 

=== modified file 'src/ui_fsmenu/internet_lobby.cc'
--- src/ui_fsmenu/internet_lobby.cc	2019-04-24 06:01:37 +0000
+++ src/ui_fsmenu/internet_lobby.cc	2019-05-05 12:59:18 +0000
@@ -39,9 +39,9 @@
 namespace {
 
 // Constants for convert_clienttype() / compare_clienttype()
-const uint8_t kClientUnregistered = 0;
+const uint8_t kClientSuperuser = 0;
 const uint8_t kClientRegistered = 1;
-const uint8_t kClientSuperuser = 2;
+const uint8_t kClientUnregistered = 2;
 // 3 was INTERNET_CLIENT_BOT which is not used
 const uint8_t kClientIRC = 4;
 }  // namespace
@@ -162,6 +162,9 @@
 	// try to connect to the metaserver
 	if (!InternetGaming::ref().error() && !InternetGaming::ref().logged_in())
 		connect_to_metaserver();
+
+	// set focus to chat input
+	chat.focus_edit();
 }
 
 void FullscreenMenuInternetLobby::layout() {
@@ -191,6 +194,10 @@
 	if (InternetGaming::ref().update_for_games()) {
 		fill_games_list(InternetGaming::ref().games());
 	}
+	// unfocus chat window when other UI element has focus
+	if (!chat.has_focus()) {
+		chat.unfocus_edit();
+	}
 }
 
 void FullscreenMenuInternetLobby::clicked_ok() {
@@ -302,6 +309,7 @@
 		}
 		prev_clientlist_len_ = clients->size();
 	}
+	clientsonline_list_.sort();
 }
 
 /// called when an entry of the client list was doubleclicked
@@ -324,12 +332,13 @@
 
 		temp += text;
 		chat.set_edit_text(temp);
-		chat.focus();
+		chat.focus_edit();
 	}
 }
 
 /// called when an entry of the server list was selected
 void FullscreenMenuInternetLobby::server_selected() {
+	// remove focus from chat
 	if (opengames_list_.has_selection()) {
 		const InternetGame* game = &opengames_list_.get_selected();
 		if (game->connectable)

=== modified file 'src/ui_fsmenu/launch_mpg.cc'
--- src/ui_fsmenu/launch_mpg.cc	2019-04-26 11:33:10 +0000
+++ src/ui_fsmenu/launch_mpg.cc	2019-05-05 12:59:18 +0000
@@ -228,6 +228,7 @@
 	delete chat_;
 	chat_ = new GameChatPanel(this, get_w() * 3 / 80, get_h() * 17 / 30 + 0.5 * label_height_,
 	                          get_w() * 53 / 80, get_h() * 11 / 30, chat, UI::PanelStyle::kFsMenu);
+	chat_->focus_edit();
 }
 
 /**
@@ -372,6 +373,11 @@
 		ctrl_->think();
 	}
 	refresh();
+
+	// unfocus chat window when other UI element has focus
+	if (!chat_->has_focus()) {
+		chat_->unfocus_edit();
+	}
 }
 
 /**

=== modified file 'src/wui/game_chat_panel.cc'
--- src/wui/game_chat_panel.cc	2019-04-24 06:01:37 +0000
+++ src/wui/game_chat_panel.cc	2019-05-05 12:59:18 +0000
@@ -119,3 +119,15 @@
 	editbox.set_text("");
 	aborted();
 }
+
+/**
+ * The mouse was clicked on this chatbox
+ */
+bool GameChatPanel::handle_mousepress(const uint8_t btn, int32_t, int32_t) {
+	if (btn == SDL_BUTTON_LEFT && get_can_focus()) {
+		focus_edit();
+		return true;
+	}
+
+	return false;
+}

=== modified file 'src/wui/game_chat_panel.h'
--- src/wui/game_chat_panel.h	2019-03-17 11:44:04 +0000
+++ src/wui/game_chat_panel.h	2019-05-05 12:59:18 +0000
@@ -54,6 +54,7 @@
 		editbox.set_text(text);
 	}
 
+	bool handle_mousepress(uint8_t btn, int32_t x, int32_t y) override;
 	void focus_edit();
 	void unfocus_edit();