Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1842396-no-tribe into lp:widelands

[Notabilis]

Sounds got to me, the changes are online.
Thanks for the reviews!
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1842396-no-tribe/+merge/372281
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1842396-no-tribe.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1842396-no-tribe into lp:widelands

[Notabilis]

We could use
  if (UserSettings::highest_playernum() >= settings.playernum)
which is the check used inside of the throwing get_players_tribe() function to 
decide whether to throw.
I decided to catch the exception since that is what is done in the other two 
cases the function is used, but of course we could change those calls as well. 
The current fix is a copy of those other calls.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1842396-no-tribe/+merge/372281
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1842396-no-tribe.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1842396-no-tribe into lp:widelands

[Notabilis]

Notabilis has proposed merging lp:~widelands-dev/widelands/bug-1842396-no-tribe 
into lp:widelands.

Commit message:
Fixing crash when no tribe is selected for a multiplayer client.


Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1842396 in widelands: "Random tribes crash multiplayer client"
  https://bugs.launchpad.net/widelands/+bug/1842396

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1842396-no-tribe/+merge/372281

Fixing a crash with showing loading screen tips when no tribe is selected for a 
multiplayer client.

Seems to be a copy or refactoring bug: When creating the tips as a single 
player or multiplayer host, the check for the thrown exception is done.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1842396-no-tribe into lp:widelands.
=== modified file 'src/network/gameclient.cc'
--- src/network/gameclient.cc	2019-08-28 06:12:07 +
+++ src/network/gameclient.cc	2019-09-04 17:13:59 +
@@ -149,10 +149,14 @@
  * Show progress dialog and load map or saved game.
  */
 InteractiveGameBase* GameClientImpl::init_game(GameClient* parent, UI::ProgressWindow* loader) {
-
-	const std::string& tribename = parent->get_players_tribe();
-	assert(Widelands::tribe_exists(tribename));
-	GameTips tips(*loader, {"general_game", "multiplayer", tribename});
+	std::vector tipstext;
+	tipstext.push_back("general_game");
+	tipstext.push_back("multiplayer");
+	try {
+		tipstext.push_back(parent->get_players_tribe());
+	} catch (GameSettingsProvider::NoTribe) {
+	}
+	GameTips tips(*loader, tipstext);
 
 	modal = loader;
 

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ignore-me__net-debug into lp:widelands

[Notabilis]

Notabilis has proposed merging lp:~widelands-dev/widelands/ignore-me__net-debug 
into lp:widelands.

Commit message:
do not merge

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/ignore-me__net-debug/+merge/369105

Do not review or merge this branch, I am only proposing it for the Windows 
builds.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/ignore-me__net-debug into lp:widelands.
=== modified file 'src/network/internet_gaming.cc'
--- src/network/internet_gaming.cc	2019-06-14 16:50:41 +
+++ src/network/internet_gaming.cc	2019-06-20 16:46:28 +
@@ -65,6 +65,7 @@
 
 /// resets all stored variables without the chat messages for a clean new login (not relogin)
 void InternetGaming::reset() {
+	log("InternetGaming::reset\n");
 	net.reset();
 	state_ = OFFLINE;
 	authenticator_ = "";
@@ -102,14 +103,21 @@
 
 void InternetGaming::initialize_connection() {
 	// First of all try to connect to the metaserver
-	log("InternetGaming: Connecting to the metaserver.\n");
+	const time_t now = time(nullptr);
+	log("InternetGaming: Connecting to the metaserver at UTC %s",
+		asctime(gmtime()));
 	NetAddress addr;
+	log("InternetGaming::initialize_connection: Trying to resolve to v6\n");
 	if (NetAddress::resolve_to_v6(, meta_, port_)) {
+	log("InternetGaming::initialize_connection: Trying to connect to v6\n");
 		net = NetClient::connect(addr);
 	}
+	log("InternetGaming::initialize_connection: Trying to resolve to v4\n");
 	if ((!net || !net->is_connected()) && NetAddress::resolve_to_v4(, meta_, port_)) {
+	log("InternetGaming::initialize_connection: Trying to connect to v4\n");
 		net = NetClient::connect(addr);
 	}
+	log("InternetGaming::initialize_connection: Done with trying\n");
 	if (!net || !net->is_connected()) {
 		throw WLWarning(_("Could not establish connection to host"),
 		_("Widelands could not establish a connection to the given address.\n"
@@ -127,10 +135,12 @@
bool registered,
const std::string& meta,
uint32_t port) {
+	log("InternetGaming::login\n");
 
 	// Reset local state. Only resetting on logout() or error isn't enough since
 	// the game might jump to the main menu from other places, too
 	reset();
+	log("InternetGaming::resetted\n");
 
 	clientname_ = nick;
 	reg_ = registered;
@@ -149,6 +159,7 @@
 }
 
 bool InternetGaming::do_login(bool should_relogin) {
+	log("InternetGaming::do_login\n");
 
 	initialize_connection();
 
@@ -163,6 +174,7 @@
 	s.string(bool2str(reg_));
 	s.string(reg_ ? "" : authenticator_);
 	net->send(s);
+	log("send\n");
 
 	// Now let's see, whether the metaserver is answering
 	uint32_t const secs = time(nullptr);
@@ -172,7 +184,9 @@
 		// Check if we are a step further... if yes handle_packet has taken care about all the
 		// paperwork, so we put our feet up and just return. ;)
 		if (state_ != CONNECTING) {
+	log("Connected!\n");
 			if (state_ == LOBBY) {
+	log("in lobby\n");
 if (!should_relogin) {
 	format_and_add_chat(
 	   "", "", true, _("Users marked with IRC will possibly not react to messages."));
@@ -319,12 +333,14 @@
 		while (net != nullptr) {
 			// Check if the connection is still open
 			if (!net->is_connected()) {
+	log("InternetGaming::handle_metaserver_communication: not connected\n");
 handle_failed_read();
 return;
 			}
 			// Process all available packets
 			std::unique_ptr packet = net->try_receive();
 			if (packet) {
+	log("InternetGaming::handle_metaserver_communication: got packet\n");
 handle_packet(*packet, relogin_on_error);
 			} else {
 // Nothing more to receive
@@ -337,6 +353,7 @@
 	}
 
 	if (state_ == LOBBY) {
+	// log("InternetGaming::handle_metaserver_communication: in lobby\n");
 		// client is in the lobby and therefore we want realtime information updates
 		if (clientupdateonmetaserver_) {
 			SendPacket s;
@@ -356,6 +373,7 @@
 	}
 
 	if (!waitcmd_.empty()) {
+	log("InternetGaming::handle_metaserver_communication: waiting\n");
 		// Check if timeout is reached
 		time_t now = time(nullptr);
 		if (now > waittimeout_) {
@@ -373,9 +391,11 @@
 	// Check connection to the metaserver
 	// Was a ping received in the last 4 minutes?
 	if (time(nullptr) - lastping_ > 240) {
+	log("InternetGaming::handle_metaserver_communication: ping timeout\n");
 		// Try to relogin
 		set_error();
 		if (relogin_on_error && !relogin()) {
+	log("InternetGaming::handle_metaserver_communication: ping timeout - resetting\n");
 			// Do not try to relogin again automatical

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-more-resolve-logging into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/net-more-resolve-logging into lp:widelands.

Commit message:
More log output if trying to resolve network addresses

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/net-more-resolve-logging/+merge/368955

Some more log output to help debug network problems.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/net-more-resolve-logging into lp:widelands.
=== modified file 'src/network/network.cc'
--- src/network/network.cc	2019-02-23 11:00:49 +
+++ src/network/network.cc	2019-06-18 06:13:12 +
@@ -38,14 +38,19 @@
 		boost::asio::ip::tcp::resolver::iterator iter = resolver.resolve(query);
 		if (iter == boost::asio::ip::tcp::resolver::iterator()) {
 			// Resolution failed
+			log("Could not resolve network name '%s:%u' to %s-address\n", hostname.c_str(),
+port, ((protocol == boost::asio::ip::tcp::v4()) ? "IPv4" : "IPv6"));
 			return false;
 		}
 		addr->ip = iter->endpoint().address();
 		addr->port = port;
+		log("Resolved network name '%s:%u' to %s\n",
+			hostname.c_str(), port, addr->ip.to_string().c_str());
 		return true;
 	} catch (const boost::system::system_error& ec) {
 		// Resolution failed
-		log("Could not resolve network name: %s\n", ec.what());
+		log("Could not resolve network name '%s:%u' to %s-address: %s\n", hostname.c_str(),
+			port, ((protocol == boost::asio::ip::tcp::v4()) ? "IPv4" : "IPv6"), ec.what());
 		return false;
 	}
 }

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1830868-skip-tests into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1830868-skip-tests into lp:widelands.

Commit message:
Adding compile.sh switch -s to skip building the tests.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1830868 in widelands: "Add switch to compile.sh for skipping boost unit 
tests"
  https://bugs.launchpad.net/widelands/+bug/1830868

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1830868-skip-tests/+merge/368858
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1830868-skip-tests into lp:widelands.
=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2019-05-09 06:43:00 +
+++ CMakeLists.txt	2019-06-15 09:39:05 +
@@ -48,6 +48,7 @@
 option(OPTION_GLEW_STATIC "Use static GLEW Library" OFF)
 option(OPTION_BUILD_WEBSITE_TOOLS "Build website-related tools" ON)
 option(OPTION_BUILD_TRANSLATIONS "Build translations" ON)
+option(OPTION_BUILD_TESTS "Build tests" ON)
 
 if (CMAKE_SOURCE_DIR STREQUAL CMAKE_BINARY_DIR)
   message(FATAL_ERROR "Build directory and source directory must not be the same.")
@@ -298,17 +299,19 @@
 endif (NOT DEFINED WL_VERSION)
 
 # Enable testing.
-include(CTest)
-enable_testing()
+if (OPTION_BUILD_TESTS)
+  include(CTest)
+  enable_testing()
 
-# Run a test after a normal compile. This magic is needed as 'make test' will
-# not rebuild tests:
-# http://stackoverflow.com/questions/733475/cmake-ctest-make-test-doesnt-build-tests
-add_custom_target(_run_all_tests ALL
-  COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure
-  WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
-  DEPENDS wl_tests
-)
+  # Run a test after a normal compile. This magic is needed as 'make test' will
+  # not rebuild tests:
+  # http://stackoverflow.com/questions/733475/cmake-ctest-make-test-doesnt-build-tests
+  add_custom_target(_run_all_tests ALL
+COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure
+WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
+DEPENDS wl_tests
+  )
+endif (OPTION_BUILD_TESTS)
 
 install (
   FILES ${CMAKE_CURRENT_BINARY_DIR}/VERSION

=== modified file 'cmake/WlFunctions.cmake'
--- cmake/WlFunctions.cmake	2019-04-18 12:46:07 +
+++ cmake/WlFunctions.cmake	2019-06-15 09:39:05 +
@@ -178,6 +178,12 @@
 
 # Common test target definition.
 function(wl_test NAME)
+
+  if (NOT OPTION_BUILD_TESTS)
+return()
+  endif()
+
+
   _parse_common_args("${ARGN}")
 
   add_executable(${NAME} ${ARG_SRCS})

=== modified file 'compile.sh'
--- compile.sh	2018-12-06 21:59:09 +
+++ compile.sh	2019-06-15 09:39:05 +
@@ -38,6 +38,9 @@
 echo "-t or --no-translations"
 echo "  Omit building translations."
 echo " "
+echo "-s or --skip-tests"
+echo "  Skip linking and executing the tests."
+echo " "
 echo "-a or --no-asan   If in debug mode, switch off the AddressSanitizer."
 echo "  Release builds are created without AddressSanitizer"
 echo "  by default."
@@ -81,6 +84,7 @@
 ## Options to control the build.
 BUILD_WEBSITE="ON"
 BUILD_TRANSLATIONS="ON"
+BUILD_TESTS="ON"
 BUILD_TYPE="Debug"
 USE_ASAN="ON"
 COMPILER="default"
@@ -128,6 +132,10 @@
   BUILD_TRANSLATIONS="OFF"
 shift
 ;;
+-s|--skip-tests)
+  BUILD_TESTS="OFF"
+shift
+;;
 -w|--no-website)
   BUILD_WEBSITE="OFF"
 shift
@@ -169,7 +177,14 @@
   echo "Translations will be built."
   echo "You can use -t or --no-translations to omit building them."
 else
-echo "Translations will not be built."
+	echo "Translations will not be built."
+fi
+echo " "
+if [ $BUILD_TESTS = "ON" ]; then
+  echo "Tests will be built."
+  echo "You can use -s or --skip-tests to omit building them."
+else
+	echo "Tests will not be built."
 fi
 echo " "
 echo "###"
@@ -245,9 +260,9 @@
   # Compile Widelands
   compile_widelands () {
 if [ $buildtool = "ninja" ] || [ $buildtool = "ninja-build" ] ; then
-  cmake -G Ninja .. -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DOPTION_BUILD_WEBSITE_TOOLS=$BUILD_WEBSITE -DOPTION_BUILD_TRANSLATIONS=$BUILD_TRANSLATIONS -DOPTION_ASAN=$USE_ASAN
+  cmake -G Ninja .. -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DOPTION_BUILD_WEBSITE_TOOLS=$BUILD_WEBSITE -DOPTION_BUILD_TRANSLATIONS=$BUILD_TRANSLATIONS -DOPTION_BUILD_TESTS=$BUILD_TESTS -DOPTION_ASAN=$USE_ASAN
 else
-  cmake .. -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DOPTION_BUILD_WEBSITE_TOOLS=$BUILD_WEBSITE -DOPTION_BUILD_TRANSLATIONS=$BUILD_TRANSLATIONS -DOPTION_ASAN=$USE_

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1826744-lobby-commands into lp:widelands

[Notabilis]

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.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1826744-lobby-commands/+merge/368285
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1826744-lobby-commands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/unify-program-parsers into lp:widelands

[Notabilis]

Review: Approve

Looking good now, thanks.
-- 
https://code.launchpad.net/~widelands-dev/widelands/unify-program-parsers/+merge/367936
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/list-directories-in-cpp.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/unify-program-parsers into lp:widelands

[Notabilis]

Looking mostly good, two comments in the diff.

I went through it commit by commit while ignoring the merges of trunk and 
list-directories-in-cpp. I hope that is okay? I am a bit unsure about that 
since there seem to be changes in the full diff that I can't find in the single 
commits.

Diff comments:

> 
> === modified file 'src/logic/editor_game_base.cc'
> --- src/logic/editor_game_base.cc 2019-04-26 05:52:49 +
> +++ src/logic/editor_game_base.cc 2019-06-06 06:00:01 +
> @@ -113,50 +113,57 @@
>   * throws an exception if something goes wrong
>   */
>  void EditorGameBase::create_tempfile_and_save_mapdata(FileSystem::Type const 
> type) {
> - // should only be called when a map was already loaded
> - assert(map_.filesystem());
> -
> - g_fs->ensure_directory_exists(kTempFileDir);
> -
> - std::string filename = kTempFileDir + g_fs->file_separator() + 
> timestring() + "_mapdata";
> - std::string complete_filename = filename + kTempFileExtension;
> -
> - // if a file with that name already exists, then try a few name 
> modifications
> - if (g_fs->file_exists(complete_filename)) {
> - int suffix;
> - for (suffix = 0; suffix <= 9; suffix++) {
> - complete_filename = filename + "-" + 
> std::to_string(suffix) + kTempFileExtension;
> - if (!g_fs->file_exists(complete_filename))
> - break;
> - }
> - if (suffix > 9) {
> - throw 
> wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all 
> considered "
> -  "filenames a file already existed");
> - }
> - }
> -
> - // create tmp_fs_
> - tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename, type));
> -
> - // save necessary map data (we actually save the whole map)
> - std::unique_ptr wms(new 
> Widelands::MapSaver(*tmp_fs_, *this));
> - wms->save();
> -
> - // swap map fs
> - std::unique_ptr mapfs(tmp_fs_->make_sub_file_system("."));
> - map_.swap_filesystem(mapfs);
> - mapfs.reset();
> -
> - // This is just a convenience hack:
> - // If tmp_fs_ is a zip filesystem then - because of the way zip 
> filesystems are currently
> - // implemented -
> - // the file is still in zip mode right now, which means that the file 
> isn't finalized yet, i.e.,
> - // not even a valid zip file until zip mode ends. To force ending the 
> zip mode (thus finalizing
> - // the file)
> - // we simply perform a (otherwise useless) filesystem request.
> - // It's not strictly necessary, but this way we get a valid zip file 
> immediately istead of
> - // at some unkown later point (when an unzip operation happens or a 
> filesystem object destructs).
> - tmp_fs_->file_exists("binary");
> + if (!map_.filesystem()) {
> + return;

Is silently doing nothing the right behavior here? Shouldn't it throw a 
wrong_gamestate or something like that?

> + }
> +
> + // save map data to temporary file and reassign map fs
> + try {
> + g_fs->ensure_directory_exists(kTempFileDir);
> +
> + std::string filename = kTempFileDir + g_fs->file_separator() + 
> timestring() + "_mapdata";
> + std::string complete_filename = filename + kTempFileExtension;
> +
> + // if a file with that name already exists, then try a few name 
> modifications
> + if (g_fs->file_exists(complete_filename)) {
> + int suffix;
> + for (suffix = 0; suffix <= 9; suffix++) {
> + complete_filename = filename + "-" + 
> std::to_string(suffix) + kTempFileExtension;
> + if (!g_fs->file_exists(complete_filename))
> + break;
> + }
> + if (suffix > 9) {
> + throw 
> wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all 
> considered "
> +  "filenames a 
> file already existed");
> + }
> + }
> +
> + // create tmp_fs_
> + tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename, 
> type));
> +
> + // save necessary map data (we actually save the whole map)
> + std::unique_ptr wms(new 
> Widelands::MapSaver(*tmp_fs_, *this));
> + wms->save();
> +
> + // swap map fs
> + std::unique_ptr 
> mapfs(tmp_fs_->make_sub_file_system("."));
> + map_.swap_filesystem(mapfs);
> + mapfs.reset();
> +
> + // This is just a convenience hack:
> + // If tmp_fs_ is a zip filesystem then - because of the way zip 
> filesystems are currently
> + // implemented -
> + // the file is still in zip 

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/reed-compatibility into lp:widelands

[Notabilis]

Sorry, I am having second thoughts about the goto-part. Actually, I am not only 
unsure about the goto-replacement but over the whole code part independent of 
your changes. :-/
If I understand it right, the code iterates over the possible jobs in the 
production site. If there is a job the loaded worker can work on, we iterate 
over all working positions of the site and check if *any* of these is empty, so 
we can assign the worker. Shouldn't we only check the working positions that 
are for the required job? Otherwise, the code could break with production sites 
which have multiple types of worker positions (Mines probably? These need a 
miner and a master miner).

Maybe I am just confused and the code is fine, but it would be good if someone 
could check it.
-- 
https://code.launchpad.net/~widelands-dev/widelands/reed-compatibility/+merge/367935
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/reed-compatibility into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1826744-lobby-commands into lp:widelands

[Notabilis]

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).
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1826744-lobby-commands/+merge/368285
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1826744-lobby-commands into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/reed-compatibility into lp:widelands

[Notabilis]

So ... you created a diff of 800 lines by passing objects around so you can 
call a method in only a handful of places?? I like it! :)
The changes are mostly looking good to me. Replacing the GOTO might be broken, 
though, see my diff comment below.

I haven't tested the changes, mostly because I have no sensible idea how to do 
so. But based on the code I guess it shouldn't break anything.

Diff comments:

> 
> === modified file 'src/map_io/map_buildingdata_packet.cc'
> --- src/map_io/map_buildingdata_packet.cc 2019-05-18 11:58:43 +
> +++ src/map_io/map_buildingdata_packet.cc 2019-05-26 03:34:22 +
> @@ -615,14 +618,15 @@
>   if 
> (worker_descr.can_act_as(working_position.first)) {
>   while (wp->worker || 
> wp->worker_request) {
>   ++wp;
> - if (!--count)
> - goto 
> end_working_position;
> + if (!--count) {
> + continue;
> + }

While I appreciate getting rid of GOTO, I am not sure if the logic change here 
is intentional. I think the continue will influence the while() loop while I 
guess that you want to influence the for() loop?
A possible replacement would be setting a flag and break-ing within the 
while(), then afterwards check for the flag, resetting it and using continue.

>   }
>   found_working_position = true;
>   break;
> - } else
> + } else {
>   wp += count;
> - end_working_position:;
> + }
>   }
>  
>   if (!found_working_position)
> 
> === modified file 'src/map_io/map_flagdata_packet.h'
> --- src/map_io/map_flagdata_packet.h  2019-02-23 11:00:49 +
> +++ src/map_io/map_flagdata_packet.h  2019-05-26 03:34:22 +
> @@ -21,7 +21,14 @@
>  #define WL_MAP_IO_MAP_FLAGDATA_PACKET_H
>  
>  #include "map_io/map_data_packet.h"
> +#include "map_io/tribes_legacy_lookup_table.h"
>  
> -MAP_DATA_PACKET(MapFlagdataPacket)
> +namespace Widelands {
> +class MapFlagdataPacket {
> +public:
> + void read(FileSystem&, EditorGameBase&, bool, MapObjectLoader&, const 
> TribesLegacyLookupTable& tribes_lookup_table);
> + void write(FileSystem&, EditorGameBase&, MapObjectSaver&);
> +};
> +}

Maybe provide a second macro for the extended version? Just an idea, this way 
is okay as well.

>  
>  #endif  // end of include guard: WL_MAP_IO_MAP_FLAGDATA_PACKET_H


-- 
https://code.launchpad.net/~widelands-dev/widelands/reed-compatibility/+merge/367935
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/reed-compatibility into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1826744-lobby-commands into lp:widelands

[Notabilis]

Ah, okay. Now it makes sense. :)
The /help command visible in the diff is within an if-block that only triggers 
for superusers.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1826744-lobby-commands/+merge/368285
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1826744-lobby-commands into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1826744-lobby-commands into lp:widelands

[Notabilis]

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?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1826744-lobby-commands/+merge/368285
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1826744-lobby-commands into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1826744-lobby-commands into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1826744-lobby-commands into lp:widelands.

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

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1826744 in widelands: "Implement lobby commands"
  https://bugs.launchpad.net/widelands/+bug/1826744

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1826744-lobby-commands/+merge/368285

/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.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1826744-lobby-commands into lp:widelands.
=== modified file 'src/network/internet_gaming.cc'
--- src/network/internet_gaming.cc	2019-06-02 09:29:59 +
+++ src/network/internet_gaming.cc	2019-06-03 19:31:05 +
@@ -459,14 +459,15 @@
 			return;
 
 		} else if (cmd == IGPCMD_ERROR) {
-			std::string errortype = packet.string();
+			const std::string errortype = packet.string();
 			if (errortype != IGPCMD_LOGIN && errortype != IGPCMD_PWD_CHALLENGE) {
 log("InternetGaming: Strange ERROR in connecting state: %s\n", errortype.c_str());
 throw WLWarning(
    _("Mixed up"), _("The metaserver sent a strange ERROR during connection"));
 			}
 			// Clients login request got rejected
-			logout(packet.string());
+			const std::string message = packet.string();
+			logout(message);
 			set_error();
 			return;
 
@@ -682,6 +683,14 @@
 }
 			}
 
+			else if (subcmd == IGPCMD_CMD) {
+// Something went wrong with the command
+message += _("Command could not be executed.");
+message =
+   (boost::format("%s %s") % message % InternetGamingMessages::get_message(reason))
+	  .str();
+			}
+
 			else if (subcmd == IGPCMD_GAME_OPEN) {
 // Something went wrong with the newly opened game
 message = InternetGamingMessages::get_message(reason);
@@ -901,6 +910,15 @@
 		// beginning
 		// with a "/" - let's see...
 
+		if (msg == "/help") {
+			format_and_add_chat("", "", true, _("Supported admin commands:"));
+			format_and_add_chat("", "", true, _("/motd   sets a permanent greeting message"));
+			format_and_add_chat("", "", true, _("/announcement   send a one time system message"));
+			format_and_add_chat("", "", true, _("/warnsend a private system message to the given user"));
+			format_and_add_chat("", "", true, _("/kick   removes the given user or game from the metaserver"));
+			return;
+		}
+
 		// Split up in "cmd" "arg"
 		std::string cmd, arg;
 		std::string temp = msg.substr(1);  // cut off '/'
@@ -944,6 +962,15 @@
 			m.string(arg);
 			net->send(m);
 			return;
+		} else if (!arg.empty() && (cmd == "warn" || cmd == "kick")) {
+			// warn a user by sending a private system message or
+			// kick a user or game from the metaserver
+			SendPacket m;
+			m.string(IGPCMD_CMD);
+			m.string(cmd);
+			m.string(arg);
+			net->send(m);
+			return;
 		} else {
 			// let everything else pass
 			goto normal;

=== modified file 'src/network/internet_gaming_protocol.h'
--- src/network/internet_gaming_protocol.h	2019-06-01 16:31:13 +
+++ src/network/internet_gaming_protocol.h	2019-06-03 19:31:05 +
@@ -299,6 +299,15 @@
 static const std::string IGPCMD_CHAT = "CHAT";
 
 /**
+ * Sent by the client to issue a superuser command.
+ *
+ * The client sends this message to the metaserver with the following payload:
+ * \li string:the command
+ * \li string:arbitrary parameters.
+ */
+static const std::string IGPCMD_CMD = "CMD";
+
+/**
  * Sent by the metaserver to inform the client, that the list of games was changed. No payload is
  * sent,
  * as e.g. clients in a game are not really interested about other games and we want to keep traffic

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-checkpwd-command into lp:widelands

[Notabilis]

The related change to the metaserver has been deployed, so this branch can be 
tested and merged.

Bunnybot fails due to transient errors.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-checkpwd-command/+merge/368225
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-checkpwd-command.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1830376-lanless-lan-game into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1830376-lanless-lan-game into lp:widelands.

Commit message:
Use the loopback network interface for LAN announcements.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1830376 in widelands: "Unable to start local multiplayer without 
internet connection"
  https://bugs.launchpad.net/widelands/+bug/1830376

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1830376-lanless-lan-game/+merge/368231

Allow the loopback network interface for LAN announcements, making it possible 
to use the LAN mode locally even when the network is disconnected.

Bonus fix: LAN games running on the same host now appear in the LAN lobby.
Also fixing missing whitespace in two error messages.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1830376-lanless-lan-game into lp:widelands.
=== modified file 'src/network/network_lan_promotion.cc'
--- src/network/network_lan_promotion.cc	2019-02-23 11:00:49 +
+++ src/network/network_lan_promotion.cc	2019-06-01 17:22:12 +
@@ -102,7 +102,7 @@
 	for (ifa = ifaddr, n = 0; ifa != nullptr; ifa = ifa->ifa_next, n++) {
 		if (ifa->ifa_addr == nullptr)
 			continue;
-		if (!(ifa->ifa_flags & IFF_BROADCAST) && !(ifa->ifa_flags & IFF_MULTICAST))
+		if (!(ifa->ifa_flags & IFF_LOOPBACK) && !(ifa->ifa_flags & IFF_BROADCAST) && !(ifa->ifa_flags & IFF_MULTICAST))
 			continue;
 		switch (ifa->ifa_addr->sa_family) {
 		case AF_INET:
@@ -291,7 +291,7 @@
 			// Remove this interface id from the set
 			it = interface_indices_v6.erase(it);
 			if (interface_indices_v6.empty()) {
-log("[LAN] Warning: No more multicast capable IPv6 interfaces."
+log("[LAN] Warning: No more multicast capable IPv6 interfaces. "
 "Other LAN players won't find your game.\n");
 			}
 		} else {
@@ -359,7 +359,7 @@
 	throw WLWarning(_("Failed to use the local network!"),
 	/** TRANSLATORS: %s is a list of alternative ports with "or" */
 	_("Widelands was unable to use the local network. "
-	  "Maybe some other process is already running a server on port %s"
+	  "Maybe some other process is already running a server on port %s "
 	  "or your network setup is broken."),
 	i18n::localize_list(ports_list, i18n::ConcatenateWith::OR).c_str());
 }

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-checkpwd-command into lp:widelands

[Notabilis]

Is fixed, thanks!
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-checkpwd-command/+merge/368225
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/net-checkpwd-command.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/net-checkpwd-command into lp:widelands

[Notabilis]

Notabilis has proposed merging lp:~widelands-dev/widelands/net-checkpwd-command 
into lp:widelands.

Commit message:
Checking metaserver password without doing a full login.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/net-checkpwd-command/+merge/368225

Currently, the online settings dialog does a full login to the metaserver to 
check the entered password. This branch avoid this by using a new CHECK_PWD 
command to verify whether the password is correct.

Should be merged after (and can't really be tested before) 
https://github.com/widelands/widelands_metaserver/pull/55 is deployed.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/net-checkpwd-command into lp:widelands.
=== modified file 'src/network/internet_gaming.cc'
--- src/network/internet_gaming.cc	2019-05-25 08:25:55 +
+++ src/network/internet_gaming.cc	2019-06-01 12:37:21 +
@@ -235,6 +235,53 @@
 	reset();
 }
 
+bool InternetGaming::check_password(const std::string& nick,
+		   const std::string& pwd,
+		   const std::string& metaserver,
+		   uint32_t port) {
+	reset();
+
+	meta_ = metaserver;
+	port_ = port;
+	initialize_connection();
+
+	// Has to be set for the password challenge later on
+	authenticator_ = pwd;
+
+	log("InternetGaming: Verifying password.\n");
+	{
+		SendPacket s;
+		s.string(IGPCMD_CHECK_PWD);
+		s.string(boost::lexical_cast(kInternetGamingProtocolVersion));
+		s.string(nick);
+		s.string(build_id());
+		net->send(s);
+	}
+
+	// Now let's see, whether the metaserver is answering
+	uint32_t const secs = time(nullptr);
+	state_ = CONNECTING;
+	while (kInternetGamingTimeout > time(nullptr) - secs) {
+		handle_metaserver_communication(false);
+		if (state_ != CONNECTING) {
+			if (state_ == LOBBY) {
+SendPacket s;
+s.string(IGPCMD_DISCONNECT);
+s.string("CONNECTION_CLOSED");
+net->send(s);
+reset();
+return true;
+			} else if (error()) {
+reset();
+return false;
+			}
+		}
+	}
+	log("InternetGaming: No answer from metaserver!\n");
+	reset();
+	return false;
+}
+
 /**
  * Handle situation when reading from socket failed.
  */
@@ -265,7 +312,7 @@
 }
 
 /// handles all communication between the metaserver and the client
-void InternetGaming::handle_metaserver_communication() {
+void InternetGaming::handle_metaserver_communication(bool relogin_on_error) {
 	if (error())
 		return;
 	try {
@@ -278,7 +325,7 @@
 			// Process all available packets
 			std::unique_ptr packet = net->try_receive();
 			if (packet) {
-handle_packet(*packet);
+handle_packet(*packet, relogin_on_error);
 			} else {
 // Nothing more to receive
 break;
@@ -315,7 +362,7 @@
 			set_error();
 			waittimeout_ = std::numeric_limits::max();
 			log("InternetGaming: reached a timeout for an awaited answer of the metaserver!\n");
-			if (!relogin()) {
+			if (relogin_on_error && !relogin()) {
 // Do not try to relogin again automatically.
 reset();
 set_error();
@@ -328,7 +375,7 @@
 	if (time(nullptr) - lastping_ > 240) {
 		// Try to relogin
 		set_error();
-		if (!relogin()) {
+		if (relogin_on_error && !relogin()) {
 			// Do not try to relogin again automatically.
 			reset();
 			set_error();
@@ -337,7 +384,7 @@
 }
 
 /// Handle one packet received from the metaserver.
-void InternetGaming::handle_packet(RecvPacket& packet) {
+void InternetGaming::handle_packet(RecvPacket& packet, bool relogin_on_error) {
 	std::string cmd = packet.string();
 
 	// First check if everything is fine or whether the metaserver broke up with the client.
@@ -347,7 +394,7 @@
 		if (reason == "CLIENT_TIMEOUT") {
 			// Try to relogin
 			set_error();
-			if (!relogin()) {
+			if (relogin_on_error && !relogin()) {
 // Do not try to relogin again automatically.
 reset();
 set_error();
@@ -405,6 +452,12 @@
 			asctime(gmtime()));
 			return;
 
+		} else if (cmd == IGPCMD_PWD_OK) {
+			const time_t now = time(nullptr);
+			log("InternetGaming: Password check successfully at UTC %s", asctime(gmtime()));
+			state_ = LOBBY;
+			return;
+
 		} else if (cmd == IGPCMD_ERROR) {
 			std::string errortype = packet.string();
 			if (errortype != IGPCMD_LOGIN && errortype != IGPCMD_PWD_CHALLENGE) {

=== modified file 'src/network/internet_gaming.h'
--- src/network/internet_gaming.h	2019-05-15 09:56:36 +
+++ src/network/internet_gaming.h	2019-06-01 12:37:21 +
@@ -79,6 +79,23 @@
 	bool relogin();
 	void logout(const std::string& msgcode = "CONNECTION_CLOSED");
 
+	/**
+	 * Connects to the metaserver and checks the password without logging in.
+	 *
+	 * Note that the user might be logged in with another username and as unregistered
+	 * if the user account is already in use by another client.
+	 * @warning Resets the curren

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1809838-blackroot-target into lp:widelands

[Notabilis]

The proposal to merge lp:~widelands-dev/widelands/bug-1809838-blackroot-target 
into lp:widelands has been updated.

Status: Needs review => Rejected

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1809838-blackroot-target/+merge/361290
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1809838-blackroot-target.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1809838-document-mill into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1809838-document-mill into lp:widelands.

Commit message:
Documenting "strange" behavior of atlanteans mill for players.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1809838 in widelands: "Blackroot flour economy target is ignored"
  https://bugs.launchpad.net/widelands/+bug/1809838

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1809838-document-mill/+merge/364704

The mill is always grinding blackroot, even when the economy target is met. 
This branch documents this behavior for players so it isn't considered/reported 
as bug.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1809838-document-mill into lp:widelands.
=== modified file 'data/tribes/buildings/productionsites/atlanteans/mill/helptexts.lua'
--- data/tribes/buildings/productionsites/atlanteans/mill/helptexts.lua	2018-05-04 20:38:53 +
+++ data/tribes/buildings/productionsites/atlanteans/mill/helptexts.lua	2019-03-18 20:58:41 +
@@ -18,7 +18,7 @@
 
 function building_helptext_note()
-- TRANSLATORS#: Note helptext for a building
-   return ""
+   return "When no cornmeal is required, the mill will try to produce blackroot flour even when there is no demand for it."
 end
 
 function building_helptext_performance()

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1815277-persistence-memory-leak into lp:widelands

[Notabilis]

It seems as if I was wrong about my assumption regarding the Eris function, its 
garbage collector should clean everything up. But I will do some more digging, 
maybe I can figure out where the leak is.

If this branch makes it better for you, I have no objection to merging it.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1815277-persistence-memory-leak/+merge/362963
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1815277-persistence-memory-leak into 
lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-memleak-net-ui into lp:widelands

[Notabilis]

Notabilis has proposed merging lp:~widelands-dev/widelands/bug-memleak-net-ui 
into lp:widelands.

Commit message:
Fixing memory leak in network UI.


Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-memleak-net-ui/+merge/362945

The leak is reported to me after joining and starting a (localhost) LAN game.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-memleak-net-ui into lp:widelands.
=== modified file 'src/network/gameclient.cc'
--- src/network/gameclient.cc	2018-12-13 07:24:01 +
+++ src/network/gameclient.cc	2019-02-10 13:34:15 +
@@ -173,8 +173,8 @@
 	game.set_write_syncstream(g_options.pull_section("global").get_bool("write_syncstreams", true));
 
 	try {
-		UI::ProgressWindow* loader_ui = new UI::ProgressWindow();
-		d->modal = loader_ui;
+		std::unique_ptr loader_ui(new UI::ProgressWindow());
+		d->modal = loader_ui.get();
 		std::vector tipstext;
 		tipstext.push_back("general_game");
 		tipstext.push_back("multiplayer");
@@ -182,7 +182,7 @@
 			tipstext.push_back(get_players_tribe());
 		} catch (NoTribe) {
 		}
-		GameTips tips(*loader_ui, tipstext);
+		GameTips tips(*loader_ui.get(), tipstext);
 
 		loader_ui->step(_("Preparing game"));
 
@@ -199,9 +199,9 @@
 		game.set_ibase(igb);
 		igb->set_chat_provider(*this);
 		if (!d->settings.savegame) {  //  new map
-			game.init_newgame(loader_ui, d->settings);
+			game.init_newgame(loader_ui.get(), d->settings);
 		} else {  // savegame
-			game.init_savegame(loader_ui, d->settings);
+			game.init_savegame(loader_ui.get(), d->settings);
 		}
 		d->time.reset(game.get_gametime());
 		d->lasttimestamp = game.get_gametime();
@@ -209,7 +209,7 @@
 
 		d->modal = igb;
 		game.run(
-		   loader_ui,
+		   loader_ui.get(),
 		   d->settings.savegame ?
 		  Widelands::Game::Loaded :
 		  d->settings.scenario ? Widelands::Game::NewMPScenario : Widelands::Game::NewNonScenario,

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands

[Notabilis]

I tested(?) it by enforcing a desync (modifying player.cc and using std::rand() 
as ID for new economies, then changing economy targets ingame). The new *.wse 
files were created and could be parsed by the python script. No idea what else 
can be tested.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-better-syncstreams/+merge/361922
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-better-syncstreams.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands

[Notabilis]

As far as I am concerned, this branch is ready for review and merge now.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-better-syncstreams/+merge/361922
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands

[Notabilis]

The proposal to merge lp:~widelands-dev/widelands/bug-better-syncstreams into 
lp:widelands has been updated.

Description changed to:

[Ready for review and merge]

Since I am not really able to get useful information out of the existing 
syncstream files, this branch adds further information to the syncstream 
describing the type of the syncstream entries.

Additionally, create a smaller syncstream extract file next to the syncstream 
file which contains only the last few seconds. This should be enough to debug 
the desync but reduces the size of the to-be-uploaded files.

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-better-syncstreams/+merge/361922
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands

[Notabilis]

The proposal to merge lp:~widelands-dev/widelands/bug-better-syncstreams into 
lp:widelands has been updated.

Commit message changed to:

Print more information in syncstreams. Create additional smaller syncstream 
files containing the last few seconds leading to a desync.

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-better-syncstreams/+merge/361922
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands

[Notabilis]

Thanks for the review. I answered some of your comments below and will push a 
merge-ready version later on.

Diff comments:

> 
> === modified file 'src/logic/game.cc'
> --- src/logic/game.cc 2018-12-13 07:24:01 +
> +++ src/logic/game.cc 2019-01-20 23:22:08 +
> @@ -603,6 +608,47 @@
>  }
>  
>  /**
> + * Switches to the next part of the syncstream excerpt.
> + */
> +void Game::report_sync_request() {
> + syncwrapper_.current_excerpt_id_ = (syncwrapper_.current_excerpt_id_ + 
> 1) % SyncWrapper::kExcerptSize;
> + syncwrapper_.excerpts_buffer_[syncwrapper_.current_excerpt_id_].clear();
> +}
> +
> +/**
> + * Triggers writing of syncstream excerpt and adds the playernumber of the 
> desynced player
> + * to the stream.
> + * Playernumber should be negative when called by network clients
> + */
> +void Game::report_desync(int32_t playernumber) {
> + std::string filename = syncwrapper_.dumpfname_;
> + filename.replace(filename.length() - kSyncstreamExtension.length(), 
> kSyncstreamExtension.length(), kSyncstreamExcerptExtension);

That function is also stripping the directory from the path, so I would have to 
add it again as well. Do you want me to do so? Using that function might be a 
bit easier to read, apart from that I don't see any advantage.

> + std::unique_ptr file(g_fs->open_stream_write(filename));
> + assert(file != nullptr);
> + // Write revision, branch and build type of this build to the file
> + file->unsigned_32(build_id().length());
> + file->text(build_id());
> + file->unsigned_32(build_type().length());
> + file->text(build_type());
> + file->signed_32(playernumber);
> + // Write our buffers to the file. Start with the oldest one
> + const size_t i2 = (syncwrapper_.current_excerpt_id_ + 1) % 
> SyncWrapper::kExcerptSize;
> + size_t i = i2;
> + for (;;) {
> + file->text(syncwrapper_.excerpts_buffer_[i]);
> + syncwrapper_.excerpts_buffer_[i].clear();
> + i = (i + 1) % SyncWrapper::kExcerptSize;
> + if (i == i2) {

Right... I totally forgot about this loop type, thanks!

> + break;
> + }
> + }
> + file->unsigned_8(Syncstream::Desync);
> + file->signed_32(playernumber);
> + // Restart buffers
> + syncwrapper_.current_excerpt_id_ = 0;
> +}
> +
> +/**
>   * Calculate the current synchronization checksum and copy
>   * it into the given array, without affecting the subsequent
>   * checksumming process.
> 
> === modified file 'src/logic/game.h'
> --- src/logic/game.h  2018-12-13 07:24:01 +
> +++ src/logic/game.h  2019-01-20 23:22:08 +
> @@ -63,6 +63,51 @@
>   gs_ending
>  };
>  
> +// The entry types that are written to the syncstream
> +// The IDs are a number in the higher 4 bits and the length in bytes in the 
> lower 4 bits
> +namespace Syncstream {
> + // game.cc Game::report_desync()

I would like to have an enum, but that would result in explicit static_cast's 
each time one of these values is written to the syncstream, so I decided 
against it. Do you want me to change it?

> + // s32 id of desynced user, -1 when written on client
> + constexpr uint8_t Desync = 0x14;
> + // map_object.cc CmdDestroyMapObject::execute()
> + // u32 object serial
> + constexpr uint8_t DestroyObject = 0x24;
> + // economy.cc Economy::process_requests()
> + // u8 request type
> + // u8 request index
> + // u32 target serial
> + constexpr uint8_t ProcessRequests = 0x36;
> + // economy.cc Economy::handle_active_supplies()
> + // u32 assignments size
> + constexpr uint8_t HandleActiveSupplies = 0x44;
> + // request.cc Request::start_transfer()
> + // u32 target serial
> + // u32 source(?) serial
> + constexpr uint8_t StartTransfer = 0x58;
> + // cmd_queue.cc CmdQueue::run_queue()
> + // u32 duetime
> + // u32 command id
> + constexpr uint8_t RunQueue = 0x68;
> + // game.h Game::logic_rand_seed()
> + // u32 random seed
> + constexpr uint8_t RandomSeed = 0x74;
> + // game.cc Game::logic_rand()
> + // u32 random value
> + constexpr uint8_t Random = 0x84;
> + // map_object.cc CmdAct::execute()
> + // u32 object serial
> + constexpr uint8_t CmdAct = 0x94;
> + // battle.cc Battle::Battle()
> + // u32 first soldier serial
> + // u32 second soldier serial
> + constexpr uint8_t Battle = 0xA8;
> + // bob.cc Bob::set_position()
> + // u32 bob serial
> + // s16 position x
> + // s16 position y
> + constexpr uint8_t BobSetPosition = 0xB8;
> +}
> +
>  class Player;
>  class MapLoader;
>  class PlayerCommand;


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-better-syncstreams/+merge/361922
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands.

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1800364-reset-economy-counter into lp:widelands

[Notabilis]

See the linked bug, especially comment #10, about the reasoning of this branch.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1800364-reset-economy-counter/+merge/362938
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1800364-reset-economy-counter.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1814372-lua-gametype into lp:widelands

[Notabilis]

Okay, looking better now.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1814372-lua-gametype/+merge/362638
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1814372-lua-gametype into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1814372-lua-gametype into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1814372-lua-gametype into lp:widelands.

Commit message:
Adding lua method to get the type of the current game, i.e., single or 
multiplayer.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1814372 in widelands: "Make single/multiplayer mode accessible to lua"
  https://bugs.launchpad.net/widelands/+bug/1814372

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1814372-lua-gametype/+merge/362638

This allows scripts to differentiate between single- and multiplayer games, 
allowing to adapt to restrictions of multiplayer games.

The idea for this came up while debugging desyncs, since the lua sleep() 
command can lead to desyncs in multiplayer if not used properly (i.e., for all 
players at the same time).
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1814372-lua-gametype into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial into lp:widelands

[Notabilis]

A branch with the requested lua method is up:
https://code.launchpad.net/~widelands-dev/widelands/bug-1814372-lua-gametype/+merge/362638
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/collectors_notification into lp:widelands

[Notabilis]

Not sure whether you are waiting for me with this. As far as I am concerned, 
your (new) changes are fine and this can be merged.
-- 
https://code.launchpad.net/~widelands-dev/widelands/collectors_notification/+merge/361334
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/collectors_notification.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial into lp:widelands

[Notabilis]

You are right, my note is most likely to abstract. If I wouldn't be knowing the 
code I possibly wouldn't understand it either.
I like the idea of checking for single-/multiplayer game. Since I wasn't able 
to find a lua method for this, I guess someone has to add one for it. I can 
take a look at it in the next days.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial into lp:widelands

[Notabilis]

Sorry for the late response. Currently, these functions are indeed called for 
all players in parallel within the win condition scripts. But if this would 
change for whichever reason, the game would desync. A point where this happened 
are the functions within ui.lua which call the sleep() function in 
coroutines.lua for only some players. It could happen that a similar construct 
is created at some time in the future again.
I think the comment in coroutines.lua should be something like:

Do not use these functions in multiplayer scripting (scenarios and
winconditions) for only some players of a game. Make sure they are
called for none or all players in parallel, otherwise the games will
desynchronize.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial into lp:widelands

[Notabilis]

I haven't tested it but the code is looking good, thanks.
Regarding the documentation: Please update the documentation of 
win_condition_functions::broadcast() since it no longer waits for roadbuilding. 
Also, could you add a comment in coroutines.lua (similar to ui.lua)? In 
multiplayer, these methods should either not been used or used for all players 
at the same time.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1811583-desync-with-territorial/+merge/362272
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1811583-desync-with-territorial.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands.

Commit message:
Print more information in syncstreams.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-better-syncstreams/+merge/361922

Not ready for review and merge yet, only starting merge request for Windows 
builds.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-better-syncstreams into lp:widelands.
=== modified file 'data/tribes/buildings/productionsites/atlanteans/mill/init.lua'
--- data/tribes/buildings/productionsites/atlanteans/mill/init.lua	2018-09-06 08:21:35 +
+++ data/tribes/buildings/productionsites/atlanteans/mill/init.lua	2019-01-17 19:30:44 +
@@ -74,7 +74,7 @@
  }
   },
   produce_blackroot_flour = {
- -- TRANSLATORS: Completed/Skipped/Did not start grinding blackrootbecause ...
+ -- TRANSLATORS: Completed/Skipped/Did not start grinding blackroot because ...
  descname = _"grinding blackroot",
  actions = {
 -- No check whether we need blackroot_flour because blackroots cannot be used for anything else.

=== modified file 'src/economy/economy.cc'
--- src/economy/economy.cc	2018-12-13 07:24:01 +
+++ src/economy/economy.cc	2019-01-17 19:30:44 +
@@ -706,6 +706,7 @@
 		// alerts, so add info to the sync stream here.
 		{
 			::StreamWrite& ss = game.syncstream();
+			ss.unsigned_8(Syncstream::ProcessRequests);
 			ss.unsigned_8(req.get_type());
 			ss.unsigned_8(req.get_index());
 			ss.unsigned_32(req.target().serial());
@@ -1046,7 +1047,7 @@
 	// to avoid potential future problems caused by the supplies_ changing
 	// under us in some way.
 	::StreamWrite& ss = game.syncstream();
-	ss.unsigned_32(0x02decafa);  // appears as facade02 in sync stream
+	ss.unsigned_8(Syncstream::HandleActiveSupplies);
 	ss.unsigned_32(assignments.size());
 
 	for (const auto& temp_assignment : assignments) {

=== modified file 'src/economy/request.cc'
--- src/economy/request.cc	2018-12-13 07:24:01 +
+++ src/economy/request.cc	2019-01-17 19:30:44 +
@@ -380,7 +380,7 @@
 	assert(is_open());
 
 	::StreamWrite& ss = game.syncstream();
-	ss.unsigned_32(0x01decafa);  // appears as facade01 in sync stream
+	ss.unsigned_8(Syncstream::StartTransfer);
 	ss.unsigned_32(target().serial());
 	ss.unsigned_32(supp.get_position(game)->serial());
 

=== modified file 'src/logic/cmd_queue.cc'
--- src/logic/cmd_queue.cc	2018-12-13 07:24:01 +
+++ src/logic/cmd_queue.cc	2019-01-17 19:30:44 +
@@ -114,8 +114,7 @@
 
 			if (dynamic_cast()) {
 StreamWrite& ss = game_.syncstream();
-static uint8_t const tag[] = {0xde, 0xad, 0x00};
-ss.data(tag, 3);  // provide an easy-to-find pattern as debugging aid
+ss.unsigned_8(Syncstream::RunQueue);
 ss.unsigned_32(c.duetime());
 ss.unsigned_32(static_cast(c.id()));
 			}

=== modified file 'src/logic/filesystem_constants.h'
--- src/logic/filesystem_constants.h	2018-11-09 08:00:35 +
+++ src/logic/filesystem_constants.h	2019-01-17 19:30:44 +
@@ -49,6 +49,7 @@
 const std::string kReplayDir = "replays";
 const std::string kReplayExtension = ".wrpl";
 const std::string kSyncstreamExtension = ".wss";
+const std::string kSyncstreamExcerptExtension = ".wse";
 // The time in seconds for how long old replays/syncstreams should be kept
 // around, in seconds. Right now this is 4 weeks.
 constexpr double kReplayKeepAroundTime = 4 * 7 * 24 * 60 * 60;

=== modified file 'src/logic/game.cc'
--- src/logic/game.cc	2018-12-13 07:24:01 +
+++ src/logic/game.cc	2019-01-17 19:30:44 +
@@ -87,6 +87,8 @@
 void Game::SyncWrapper::start_dump(const std::string& fname) {
 	dumpfname_ = fname + kSyncstreamExtension;
 	dump_.reset(g_fs->open_stream_write(dumpfname_));
+	current_excerpt_id_ = 0;
+	excerpts_buffer_[current_excerpt_id_].clear();
 }
 
 void Game::SyncWrapper::data(void const* const sync_data, size_t const size) {
@@ -114,6 +116,8 @@
 			log("Writing to syncstream file %s failed. Stop synctream dump.\n", dumpfname_.c_str());
 			dump_.reset();
 		}
+		assert(current_excerpt_id_ < kExcerptSize);
+		excerpts_buffer_[current_excerpt_id_].append(static_cast(sync_data), size);
 	}
 
 	target_.data(sync_data, size);
@@ -603,6 +607,40 @@
 }
 
 /**
+ * Switches to the next part of the syncstream excerpt.
+ */
+void Game::report_sync_request() {
+	syncwrapper_.current_excerpt_id_ = (syncwrapper_.current_excerpt_id_ + 1) % SyncWrapper::kExcerptSize;
+	syncwrapper_.excerpts_buffer_[syncwrapper_.current_excerpt_id_].clear();
+}
+
+/**
+ * Triggers writing of syncstream excerpt and adds the playernumber of the desynced player
+ * to the stream.
+ */
+void Game::report_desync(uint32_t playernumber) {
+	std::string filename 

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1811030-desync-ai into lp:widelands

[Notabilis]

I compiled the game revision of the other bug report with the changes of this 
branch applied and it indeed seems to fix the issues there.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1811030-desync-ai/+merge/361689
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1811030-desync-ai.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1811030-desync-ai into lp:widelands

[Notabilis]

You should be able to test it with the savegame attached to the linked bug 
report. With trunk, the savegame desyncs immediately. With this branch you can 
continue playing. I tested it with two widelands instances running on my 
system, so no second computer/human should be required for testing.

If trying to reproduce the desync in a new game you have to wait until the AI 
starts expeditions, which might take some time.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1811030-desync-ai/+merge/361689
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1811030-desync-ai.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1811030-desync-ai into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1811030-desync-ai into lp:widelands.

Commit message:
Replacing logic_rand() with std::rand() in seafaring code of AI.
Should fix desyncs while network gaming.


Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1811030-desync-ai/+merge/361689

Calls of logic_rand() have to be done on all participants of a network game. 
Since the AI code is only executed on the host, calling logic_rand() leads to 
different random numbers on the participants computers later on, resulting in 
desynchronized games.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1811030-desync-ai into lp:widelands.
=== modified file 'src/ai/defaultai_seafaring.cc'
--- src/ai/defaultai_seafaring.cc	2018-09-04 15:48:47 +
+++ src/ai/defaultai_seafaring.cc	2019-01-11 21:07:24 +
@@ -455,7 +455,7 @@
 }
 
 Widelands::IslandExploreDirection DefaultAI::randomExploreDirection() {
-	return game().logic_rand() % 20 < 10 ? Widelands::IslandExploreDirection::kClockwise :
+	return std::rand() % 20 < 10 ? Widelands::IslandExploreDirection::kClockwise :
 	   Widelands::IslandExploreDirection::kCounterClockwise;
 }
 
@@ -486,7 +486,7 @@
 		spot_score);
 
 		// we make a decision based on the score value and random
-		if (game().logic_rand() % 8 < spot_score) {
+		if (std::rand() % 8 < spot_score) {
 			// we build a port here
 			game().send_player_ship_construct_port(*so.ship, so.ship->exp_port_spaces().front());
 			so.last_command_time = gametime;
@@ -579,15 +579,15 @@
 	assert(possible_directions.size() >= new_teritory_directions.size());
 
 	// If only open sea (no unexplored sea) is found, we don't always divert the ship
-	if (new_teritory_directions.empty() && game().logic_rand() % 100 < 80) {
+	if (new_teritory_directions.empty() && std::rand() % 100 < 80) {
 		return false;
 	}
 
 	if (!possible_directions.empty() || !new_teritory_directions.empty()) {
 		const Direction direction =
 		   !new_teritory_directions.empty() ?
-		  new_teritory_directions.at(game().logic_rand() % new_teritory_directions.size()) :
-		  possible_directions.at(game().logic_rand() % possible_directions.size());
+		  new_teritory_directions.at(std::rand() % new_teritory_directions.size()) :
+		  possible_directions.at(std::rand() % possible_directions.size());
 		game().send_player_ship_scouting_direction(*so.ship, static_cast(direction));
 
 		log("%d: %s: exploration - breaking for %s sea, dir=%u\n", pn,

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/collectors_notification into lp:widelands

[Notabilis]

Review: Approve diff, testing

Code is looking good and it seems to work as intended. A few possible 
improvements are in the diff.

I assume it is intentional that there is no message on game start?
In the commit/merge message of this branch you could add that wood gnome is 
also affected.

Diff comments:

> === modified file 'data/scripting/win_conditions/collectors.lua'
> --- data/scripting/win_conditions/collectors.lua  2018-10-13 08:51:51 
> +
> +++ data/scripting/win_conditions/collectors.lua  2019-01-02 13:47:40 
> +
> @@ -28,7 +28,11 @@
>  
> -- set the objective with the game type for all players
> broadcast_objective("win_condition", wc_descname, wc_desc)
> -
> +   
> +   -- set the maximum game time of 4 hours
> +   local max_time = 4 * 60
> +   
> +   local game = wl.Game()
> local plrs = wl.Game().players

Suggestion: Use new "game" variable

> local teams = {}
> for idx,plr in ipairs(plrs) do
> @@ -221,10 +199,14 @@
>  lost_or_won = 1
>  player:send_message(won_game_over.title, won_game_over.body)
>   end
> - if (player.team == 0) then
> -wl.game.report_result(player, lost_or_won, 
> make_extra_data(player, wc_descname, wc_version, {score=info[2]}))
> + if (count_factions(plrs) > 1) then

Suggestion: Call count_factions() only once before the loop?

> +if (player.team == 0) then
> +   wl.game.report_result(player, lost_or_won, 
> make_extra_data(player, wc_descname, wc_version, {score=info[2]}))
> +else
> +   wl.game.report_result(player, lost_or_won, 
> make_extra_data(player, wc_descname, wc_version, {score=info[3], 
> team_score=info[2]}))
> +end
>   else
> -wl.game.report_result(player, lost_or_won, 
> make_extra_data(player, wc_descname, wc_version, {score=info[3], 
> team_score=info[2]}))
> +wl.game.report_result(player, lost_or_won)
>   end
>end
> end
> 
> === modified file 'data/scripting/win_conditions/win_condition_functions.lua'
> --- data/scripting/win_conditions/win_condition_functions.lua 2018-09-28 
> 17:20:32 +
> +++ data/scripting/win_conditions/win_condition_functions.lua 2019-01-02 
> 13:47:40 +
> @@ -237,3 +237,59 @@
> table.sort(ranked_players_and_teams, function(a,b) return a["points"] > 
> b["points"] end)
> return ranked_players_and_teams
>  end
> +
> +-- RST
> +-- .. function:: format_remaining_time(remaining_time)
> +--
> +--return a message that contains the remaining game time
> +--to be used when sending status meassages
> +--
> +--:arg remaining_time:The remaining game time in minutes
> +function format_remaining_time(remaining_time)
> +   local h = 0
> +   local m = 60
> +   local time = ""
> +   set_textdomain("win_conditions")
> +   
> +   if (remaining_time ~= 60) then
> +  h = math.floor(remaining_time / 60)
> +  m = remaining_time % 60
> +   end
> +
> +   if ((h > 0) and (m > 0)) then
> +  -- TRANSLATORS: Context: 'The game will end in 2 hours and 30 minutes.'
> +  time = (ngettext("%1% hour and %2% minutes", "%1% hours and %2% 
> minutes", h, m)):bformat(h, m)
> +   elseif m > 0 then
> +  -- TRANSLATORS: Context: 'The game will end in 30 minutes.'
> +  time = (ngettext("%i minute", "%i minutes", m)):format(m)
> +   else
> +  -- TRANSLATORS: Context: 'The game will end in 2 hours.'
> +  time = (ngettext("%1% hour", "%1% hours", h)):bformat(h)
> +   end
> +   -- TRANSLATORS: Context: 'The game will end in (2 hours and) 30 minutes.'
> +   return p(_"The game will end in %s."):format(time)
> +end
> +
> +-- RST
> +-- .. function:: notification_remaining_time(max_time)
> +--
> +--calculate the remaining game time for notifications 
> +--returns the remaining time and whether the notification should popup
> +--to be used when sending status meassages

Typo "messages"

> +--status messages are to be send every 30 minutes and every 5 during the 
> last 30 minutes
> +--the message window pops up ever hour 30, 20 & 10 minutes berfore the 
> game ends.

If I understand it right this method should only be called within coroutines 
and is blocking them. Add a commend about that to the documentation? Currently 
it reads as if this method only calculates something.

> +--
> +--:arg max_time:The time maximum game time in minutes
> +function notification_remaining_time(max_time, remaining_time)
> +   local show_popup = false
> +   if (wl.Game().time < ((max_time - 30) * 60 * 1000)) then --
> +  wake_me(wl.Game().time + (30 * 60 * 1000)) -- 30 minutes
> +  remaining_time = remaining_time - 30
> +  if (remaining_time % 60 == 0) or (remaining_time == 30) then 
> show_popup = true end
> +   else
> +  wake_me(wl.Game().time + (300 * 1000)) --5 Minutes
> +  remaining_time = remaining_time - 5
> +  if ((remaining_time ~= 0) and (remaining_time % 10 == 0)) then 

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1809838-blackroot-target into lp:widelands

[Notabilis]

Launchpad failed to parse the branch. Here is the only commit:
https://bazaar.launchpad.net/~widelands-dev/widelands/bug-1809838-blackroot-target/revision/8955
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1809838-blackroot-target/+merge/361290
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1809838-blackroot-target into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1809838-blackroot-target into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1809838-blackroot-target into lp:widelands.

Commit message:
Adding missing check whether blackroot flour is required.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1809838 in widelands: "Blackroot flour economy target is ignored"
  https://bugs.launchpad.net/widelands/+bug/1809838

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1809838-blackroot-target/+merge/361290

Checking in the production program whether blackroot flour is required before 
grinding blackroot, so the economic target is respected.

It might be that this isn't checked on purpose, since different from corn there 
is no other use for blackroot. If the check shouldn't be in the production 
program, the economy target for blackroot flour should be removed instead.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1809838-blackroot-target into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/travis-skip-translations-in-debug-builds into lp:widelands

[Notabilis]

Review: Approve

Diff is looking good and based on the Travis output it seems to work as 
intented.
-- 
https://code.launchpad.net/~widelands-dev/widelands/travis-skip-translations-in-debug-builds/+merge/359347
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/travis-skip-translations-in-debug-builds.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1806230-enable-ok-button into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1806230-enable-ok-button into lp:widelands.

Commit message:
Enabling OK button in map options of editor when tags are changed.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1806230 in widelands: "Editor: Map tags not changeable"
  https://bugs.launchpad.net/widelands/+bug/1806230

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1806230-enable-ok-button/+merge/361163

Adding the missing linkage between changes to the tabs and enabling the OK 
button in the map option menu of the editor.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1806230-enable-ok-button into lp:widelands.
=== modified file 'src/editor/ui_menus/main_menu_map_options.cc'
--- src/editor/ui_menus/main_menu_map_options.cc	2018-12-13 07:24:01 +
+++ src/editor/ui_menus/main_menu_map_options.cc	2018-12-19 21:39:17 +
@@ -129,6 +129,7 @@
 	// TODO(GunChleoc): We need team images in the listselect here,
 	// so map editors will be able to delete entries.
 	// This is waiting for the new RT renderer.
+	// TODO(Notabilis): Add onChanged-code below after this is added
 	teams_list_.add("Not implemented yet.", "", nullptr, false);
 
 	unsigned int nr_players = static_cast(eia().egbase().map().get_nrplayers());
@@ -148,6 +149,9 @@
 	author_.changed.connect(boost::bind(::changed, this));
 	descr_->changed.connect(boost::bind(::changed, this));
 	hint_->changed.connect(boost::bind(::changed, this));
+	for (const auto& tag : tags_checkboxes_) {
+		tag.second->changed.connect(boost::bind(::changed, this));
+	}
 
 	ok_.sigclicked.connect(boost::bind(::clicked_ok, boost::ref(*this)));
 	cancel_.sigclicked.connect(boost::bind(::clicked_cancel, boost::ref(*this)));

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1808169-disable-focus into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1808169-disable-focus into lp:widelands.

Commit message:
Allowing hotkey usage while windows are open.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1808169 in widelands: "scrolling not possible with arrow keys"
  https://bugs.launchpad.net/widelands/+bug/1808169

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1808169-disable-focus/+merge/361097

Allowing to use arrow keys while windows (e.g., ware statistics) are open.

Changed two points:
- The UI::Panel no longer grabs the keyboard focus when it is clicked but 
shouldn't grab it
- Some components (buttons, slider, checkbox) no longer accept the keyboard 
focus. I don't know why they ever did so, though. It possibly was connected to 
the Panel being able to switch between components with the Tab key. But since 
nearly no component handles any keys, I guess it is no (big) loss. But still my 
main problem with this merge request is: Does someone knows about or finds a 
functionality that breaks due to the no longer applied keyboard focus?
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1808169-disable-focus into lp:widelands.
=== modified file 'src/ui_basic/button.cc'
--- src/ui_basic/button.cc	2018-12-13 07:24:01 +
+++ src/ui_basic/button.cc	2018-12-18 20:42:33 +
@@ -60,7 +60,8 @@
  title_image_(title_image),
  background_style_(g_gr->styles().button_style(init_style)) {
 	set_thinks(false);
-	set_can_focus(true);
+	// Don't allow focus
+	assert(!get_can_focus());
 }
 
 Button::Button  //  for textual buttons. If h = 0, h will resize according to the font's height.
@@ -143,8 +144,6 @@
 	if (enabled_ == on)
 		return;
 
-	set_can_focus(on);
-
 	// disabled buttons should look different...
 	if (on)
 		enabled_ = true;
@@ -316,7 +315,6 @@
 		return false;
 
 	if (enabled_) {
-		focus();
 		grab_mouse(true);
 		pressed_ = true;
 		if (repeating_) {

=== modified file 'src/ui_basic/checkbox.cc'
--- src/ui_basic/checkbox.cc	2018-12-13 07:24:01 +
+++ src/ui_basic/checkbox.cc	2018-12-18 20:42:33 +
@@ -50,7 +50,6 @@
 	uint16_t h = pic->height();
 	set_desired_size(w, h);
 	set_size(w, h);
-	set_can_focus(true);
 	set_flags(Has_Custom_Picture, true);
 }
 
@@ -98,7 +97,6 @@
  * Args: enabled  true if the checkbox should be enabled, false otherwise
  */
 void Statebox::set_enabled(bool const enabled) {
-	set_can_focus(enabled);
 	if (((flags_ & Is_Enabled) > 1) && enabled)
 		return;
 
@@ -177,7 +175,6 @@
  */
 bool Statebox::handle_mousepress(const uint8_t btn, int32_t, int32_t) {
 	if (btn == SDL_BUTTON_LEFT && (flags_ & Is_Enabled)) {
-		focus();
 		clicked();
 		return true;
 	}

=== modified file 'src/ui_basic/panel.cc'
--- src/ui_basic/panel.cc	2018-12-13 07:24:01 +
+++ src/ui_basic/panel.cc	2018-12-18 20:42:33 +
@@ -528,7 +528,7 @@
  * \return true if the mouseclick was processed, false otherwise
  */
 bool Panel::handle_mousepress(const uint8_t btn, int32_t, int32_t) {
-	if (btn == SDL_BUTTON_LEFT) {
+	if (btn == SDL_BUTTON_LEFT && get_can_focus()) {
 		focus();
 	}
 	return false;

=== modified file 'src/ui_basic/slider.cc'
--- src/ui_basic/slider.cc	2018-12-13 07:24:01 +
+++ src/ui_basic/slider.cc	2018-12-18 20:42:33 +
@@ -78,7 +78,7 @@
  bar_size_(bar_size),
  cursor_size_(cursor_size) {
 	set_thinks(false);
-	set_can_focus(true);
+	assert(!get_can_focus());
 	calculate_cursor_position();
 }
 
@@ -205,7 +205,6 @@
 	if (enabled_ == enabled)
 		return;
 
-	set_can_focus(enabled);
 	enabled_ = enabled;
 	if (!enabled) {
 		pressed_ = false;
@@ -402,7 +401,6 @@
 	if (btn != SDL_BUTTON_LEFT)
 		return false;
 
-	focus();
 	if (x >= cursor_pos_ && x <= cursor_pos_ + cursor_size_) {
 		//  click on cursor
 		cursor_pressed(x);
@@ -469,7 +467,6 @@
 	if (btn != SDL_BUTTON_LEFT)
 		return false;
 
-	focus();
 	if (y >= cursor_pos_ && y <= cursor_pos_ + cursor_size_) {
 		//  click on cursor
 		cursor_pressed(y);

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1805325-joining-lan-games into lp:widelands

[Notabilis]

I can do so, but this branch haven't been tested by a reviewer yet. Do you want 
me to merge this anyway?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1805325-joining-lan-games/+merge/359789
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1805325-joining-lan-games.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle into lp:widelands

[Notabilis]

Review: Approve diff, testing

>From what I can say without knowing the scenario it works as intended, with 
>some building gaining and some building loosing the possibility/button to 
>dismantle them. Code is looking good to me as well.

Unrelated, but noticed this when testing: The empire "outpost enhanced to 
barrier" gives more materials when dismantling than a directly constructed 
barrier. I think this is on purpose, though.
-- 
https://code.launchpad.net/~widelands-dev/widelands/empire04_unused_key_return_on_dismantle/+merge/358273
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/fix_immovabledescr_unpersist into lp:widelands

[Notabilis]

I haven't tested it, but code is looking good.
-- 
https://code.launchpad.net/~widelands-dev/widelands/fix_immovabledescr_unpersist/+merge/357941
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/fix_immovabledescr_unpersist into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1800182-focus-save-menu into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1800182-focus-save-menu into lp:widelands.

Commit message:
Setting focus to edit box when opening the game save menu.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1800182 in widelands: "Focus missing when saving game from replay"
  https://bugs.launchpad.net/widelands/+bug/1800182

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1800182-focus-save-menu/+merge/358284

When the game save menu is opened, the keyboard focus isn't set to the filename 
input edit box. Besides the inconvenience of having to click in the edit box 
before entering the filename, keyboard input also modifies the state of the 
game, e.g., opening the minimap.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1800182-focus-save-menu into lp:widelands.
=== modified file 'src/wui/game_main_menu_save_game.cc'
--- src/wui/game_main_menu_save_game.cc	2018-10-26 07:09:29 +
+++ src/wui/game_main_menu_save_game.cc	2018-11-04 08:51:30 +
@@ -115,6 +115,7 @@
 	center_to_parent();
 	move_to_top();
 
+	filename_editbox_.focus();
 	pause_game(true);
 	set_thinks(false);
 	layout();

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1800571-flag-add-ware into lp:widelands

[Notabilis]

I guess Launchpad doesn't like me since it doesn't want to show the diff here 
(again). And yes, I waited for the branch to be parsed after pushing it. :/

Anyway, here are the changes:
https://bazaar.launchpad.net/~widelands-dev/widelands/bug-1800571-flag-add-ware/revision/8904/src/logic/map_objects/tribes/carrier.cc
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1800571-flag-add-ware/+merge/358046
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1800571-flag-add-ware into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1800571-flag-add-ware into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1800571-flag-add-ware into lp:widelands.

Commit message:
Undoing "optimization" that is part of previous commit in r8903.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1800571 in widelands: "crash with r8903"
  https://bugs.launchpad.net/widelands/+bug/1800571

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1800571-flag-add-ware/+merge/358046

Commit r8903 moved some code within carrier.cc to avoid a duplicated call of a 
method in the Flag class. Unfortunately I overlooked that the call is no simple 
getter but modifies the internal state of the Flag, so it has to happen at the 
old code position.

Now doing the call to the method potentially twice, but it doesn't really 
matters performance-wise anyway.

For reference the old, buggy commit:
https://bazaar.launchpad.net/~widelands-dev/widelands/bug-1797213-idle-wares-at-flags-carrier/revision/8903
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1800571-flag-add-ware into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1797213-idle-wares-at-flags-carrier into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1797213-idle-wares-at-flags-carrier into 
lp:widelands.

Commit message:
Fixing ware staying at flag if its destination changes while it is carried.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1797213 in widelands: "Sometimes ware(s) are lying at a flag and get not 
transported."
  https://bugs.launchpad.net/widelands/+bug/1797213

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1797213-idle-wares-at-flags-carrier/+merge/357920

If a ware ends being requested while it is carried, it will be dropped at the 
next flag and not touched again. To reproduce:

- Start a new game
- Start building a Quarry and connect it to the warehouse/HQ
- While the carrier is carrying the ware, set the allowed number of trunks in 
the construction site to zero
- The ware will now be placed at the next flag and won't be moved again (that 
is: not being transported back to the warehouse)

This can be repeated over and over again in both directions (to construction 
site / to warehouse):
- Changing the request while the ware is carried will result in the ware 
getting stuck
- Changing the request while the ware is at a flag will work correctly
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1797213-idle-wares-at-flags-carrier into 
lp:widelands.
=== modified file 'src/logic/map_objects/tribes/carrier.cc'
--- src/logic/map_objects/tribes/carrier.cc	2018-09-04 15:48:47 +
+++ src/logic/map_objects/tribes/carrier.cc	2018-10-28 20:06:36 +
@@ -216,13 +216,19 @@
 			return schedule_act(game, 20);
 		}
 
-		WareInstance* otherware = flag.fetch_pending_ware(game, otherware_idx);
-
 		if (ware) {
+			const bool ware_astray = (ware->get_next_move_step(game) == nullptr);
 			// Drop our ware
 			flag.add_ware(game, *fetch_carried_ware(game));
+			// If the destination of the dropped ware changed while carrying it and we don't have
+			// anything else we should carry, we might pick it up again immediately, so check again
+			if (ware_astray && otherware_idx == kNotFoundAppropriate) {
+otherware_idx = flag.find_pending_ware(otherflag);
+			}
 		}
 
+		WareInstance* otherware = flag.fetch_pending_ware(game, otherware_idx);
+
 		// Pick up new load, if any
 		if (otherware) {
 			// pay before getting the ware, while checking for road promotion

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795871-lua-set_workers into lp:widelands

[Notabilis]

Travis failed for GCC 7 Debug Build with "No output has been received in the 
last 10m0s" when running the new test. Since all other builds succeeded, I 
think this is transitional error. At least, I am not aware of any loops related 
to this branch that could end up in an endless loop.

@bunnybot merge force
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1795871-lua-set_workers/+merge/357716
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1795871-lua-set_workers.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795871-lua-set_workers into lp:widelands

[Notabilis]

Worked fine now, thanks! So I guess this can be merged now?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1795871-lua-set_workers/+merge/357716
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1795871-lua-set_workers.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1797531-playermenu-tribe into lp:widelands

[Notabilis]

Review: Approve

Code is looking good and last player has no longer the "random" tribe.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1797531-playermenu-tribe/+merge/356695
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1797531-playermenu-tribe.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795871-lua-set_workers into lp:widelands

[Notabilis]

Thanks for adding the test. The code is looking good, but for me the test fails:

Error in Lua Coroutine
[../src/scripting/lua_errors.cc:22] [string 
"test/maps/plain.wmf/scripting/test_inputqueue..."]:9: expected '8' but was '7'!

I think the problem is that the barracks starts working immediately, consuming 
one carrier while doing so.
Possible fixes would be either to stop the barracks (is that even possible by 
script?) or connecting it to the headquarter (should stop the barracks since 
there already are enough soldiers on store).
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1795871-lua-set_workers/+merge/357716
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1795871-lua-set_workers.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1798812-replay-savegame-type into lp:widelands

[Notabilis]

Review: Approve

Looking good now, thanks!
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1798812-replay-savegame-type/+merge/357588
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1798812-replay-savegame-type.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795871-lua-set_workers into lp:widelands

[Notabilis]

Seems indeed as if something failed within Launchpad. For convenience, here is 
the change:
https://bazaar.launchpad.net/~widelands-dev/widelands/bug-1795871-lua-set_workers/revision/8897

Interesting is only the first block, the rest are added brackets.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1795871-lua-set_workers/+merge/357716
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1795871-lua-set_workers into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795871-lua-set_workers into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1795871-lua-set_workers into lp:widelands.

Commit message:
Ignore workers in WorkerQueues when setting the amount of "worker" workers by 
script.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1795871 in widelands: "Game data error: inputqueue: workersqueue: not 
found"
  https://bugs.launchpad.net/widelands/+bug/1795871

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1795871-lua-set_workers/+merge/357716

The set_workers() Lua function removes all workers that aren't explicitly given 
as parameter. In case of the barracks this leads to the carriers in the input 
queue being deleted when the trainer is added per script. For once, this isn't 
intended (I think), but more importantly this leads to a crash later on since 
the WOrkersQueue still contains the references to the deleted "input" workers.

For testing, see the attached bug report.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1795871-lua-set_workers into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1798024-heap-use-after-free into lp:widelands

[Notabilis]

Review: Approve diff, test

Fix is looking good and I loaded the provided savegame a few times without it 
crashing.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1798024-heap-use-after-free/+merge/356825
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1798024-heap-use-after-free.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1798812-replay-savegame-type into lp:widelands

[Notabilis]

Code is looking good and the game appears in the load menu.

Two issues:
- My single player replay savegame also shows up in the multiplayer load game 
screen (even before this change). Not that much of an issue, might even be 
intentional?
- The savegame isn't shown in the "save game" dialog. So I can load a game but 
when saving later on it isn't shown any longer (Huh? Where did it go?). Saving 
without seeing the game in the save dialog will trigger an "really override" 
message (Huh? For which game?), but after saving it once it will correctly 
appear in the list.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1798812-replay-savegame-type/+merge/357588
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1798812-replay-savegame-type into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795871-lua-set_workers into lp:widelands

[Notabilis]

On second thought, this fix is kind of broken. I haven't tested it but it will 
most likely fail if a building has the same worker type as an "input" worker 
and as a "worker" worker. In that case, the input workers will probably be 
deleted as well.
Unfortunately the PlayerImmovable given as a function parameter does not offer 
a way to differentiate between the two kinds of workers. To fix this, one would 
probably create a copy of this function specifically for production sites.

If this should be done, feel free to say so. Otherwise we could just merge this 
code (since it fixes an appeared bug) and wait whether it fails again (and 
maybe add a TODO comment in the code).
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1795871-lua-set_workers/+merge/357716
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1795871-lua-set_workers into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-net-error-game-connect into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-net-error-game-connect into lp:widelands.

Commit message:
Fixes on the communication between widelands and the metaserver:

- Permit answering PING requests even while logging in  


- Printing current time to console when logging in. 

  Eases finding the corresponding log entry on the metaserver when debugging
   
- Re-enabling and fixing unused code for whisper message to nonexisting player  
  
- Handling error message when trying to join a non-existing game
  
- Avoid hanging client even after being notified about the non-existing game
- Removed no longer needed error messages 

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-net-error-game-connect/+merge/357477

A bunch of smaller fixes regarding the metaserver. Nothing that should change 
anything in the "normal" case, but offers better / more correct handling of 
some errors.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-net-error-game-connect into lp:widelands.
=== modified file 'src/network/internet_gaming.cc'
--- src/network/internet_gaming.cc	2018-08-24 07:58:04 +
+++ src/network/internet_gaming.cc	2018-10-20 15:50:44 +
@@ -353,6 +353,14 @@
 			}
 		}
 		return;
+	} else if (cmd == IGPCMD_PING) {
+		// Client received a PING and should immediately PONG as requested
+		SendPacket s;
+		s.string(IGPCMD_PONG);
+		net->send(s);
+
+		lastping_ = time(nullptr);
+		return;
 	}
 
 	// Are we already online?
@@ -389,7 +397,10 @@
 			format_and_add_chat("", "", true, _("For reporting bugs, visit:"));
 			format_and_add_chat("", "", true, "https://wl.widelands.org/wiki/ReportingBugs/;);
 			state_ = LOBBY;
-			log("InternetGaming: Client %s logged in.\n", clientname_.c_str());
+			// Append UTC time to login message to ease linking between client output and
+			// metaserver logs. The string returned by asctime is terminated by \n
+			const time_t now = time(nullptr);
+			log("InternetGaming: Client %s logged in at UTC %s", clientname_.c_str(), asctime(gmtime()));
 			return;
 
 		} else if (cmd == IGPCMD_ERROR) {
@@ -408,8 +419,7 @@
 			logout();
 			set_error();
 			log("InternetGaming: Expected a LOGIN, PWD_CHALLENGE or ERROR packet from server, but "
-			"received "
-			"command %s. Maybe the metaserver is using a different protocol version?",
+			"received command %s. Maybe the metaserver is using a different protocol version?\n",
 			cmd.c_str());
 			throw WLWarning(
 			   _("Unexpected packet"),
@@ -443,15 +453,6 @@
 			format_and_add_chat("", "", true, temp);
 		}
 
-		else if (cmd == IGPCMD_PING) {
-			// Client received a PING and should immediately PONG as requested
-			SendPacket s;
-			s.string(IGPCMD_PONG);
-			net->send(s);
-
-			lastping_ = time(nullptr);
-		}
-
 		else if (cmd == IGPCMD_CHAT) {
 			// Client received a chat message
 			std::string sender = packet.string();
@@ -617,11 +618,11 @@
 			if (subcmd == IGPCMD_CHAT) {
 // Something went wrong with the chat message the user sent.
 message += _("Chat message could not be sent.");
-if (reason == "NO_SUCH_USER")
+if (reason == "NO_SUCH_USER") {
 	message = (boost::format("%s %s") % message %
-	   (boost::format(InternetGamingMessages::get_message(reason)) %
-	packet.string().c_str()))
+	   InternetGamingMessages::get_message(reason))
 	 .str();
+}
 			}
 
 			else if (subcmd == IGPCMD_GAME_OPEN) {
@@ -630,17 +631,30 @@
 // we got our answer, so no need to wait anymore
 waitcmd_ = "";
 			}
-			message = (boost::format(_("ERROR: %s")) % message).str();
+
+			else if (subcmd == IGPCMD_GAME_CONNECT && reason == "NO_SUCH_GAME") {
+log("InternetGaming: The game no longer exists, maybe it has just been closed\n");
+message = InternetGamingMessages::get_message(reason);
+assert(waitcmd_ == IGPCMD_GAME_CONNECT);
+waitcmd_ = "";
+			}
+			if (!message.empty()) {
+message = (boost::format(_("ERROR: %s")) % message).str();
+			} else {
+message = (boost::format(_("An unexpected error message has been received about command %1%: %2%"))
+			% subcmd % reason).str();
+			}
 
 			// Finally send the error message as system c

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1797792-shading-language-version-comparison into lp:widelands

[Notabilis]

Review: Approve diff, testing

Looking good now, no further objections from my side.
Thanks for the explanations in the comments.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1797792-shading-language-version-comparison/+merge/356699
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1797792-shading-language-version-comparison.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1797792-shading-language-version-comparison into lp:widelands

[Notabilis]

Review: Needs Fixing diff

Some comments to your comments in the old diff.
Also, the new diff is broken and contains the remains of a failed merge.

Diff comments:

> === modified file 'src/graphic/gl/initialize.cc'
> --- src/graphic/gl/initialize.cc  2018-10-15 05:26:10 +
> +++ src/graphic/gl/initialize.cc  2018-10-15 06:50:28 +
> @@ -178,11 +179,26 @@
>   glGetIntegerv(GL_MAX_TEXTURE_SIZE, max_texture_size);
>   log("Graphics: OpenGL: Max texture size: %u\n", *max_texture_size);
>  
> - // TODO(GunChleoc): Localize the on-screen error messages
> - // Exit if we can't detect the shading language version
>   const char* const shading_language_version_string =
>  reinterpret_cast char*>(glGetString(GL_SHADING_LANGUAGE_VERSION));
> - if (strcmp(shading_language_version_string, "(null)") == 0) {
> + log("Graphics: OpenGL: ShadingLanguage: \"%s\"\n", 
> shading_language_version_string);

Current trunk first checks for "(null)", and only if it is not "(null)" prints 
out the current ShadingLanguage string. The new code always prints the string, 
independent of its contents.

> +
> + std::vector shading_language_version_vector;
> + boost::split(shading_language_version_vector, 
> shading_language_version_string, boost::is_any_of("."));
> + if (shading_language_version_vector.size() >= 2) {

Sorry, should have written this in two comments. "1.20.1" is a good argument, 
keep this line it as ">= 2".

If "2" is reported as shading language by the driver (instead of "2.0"), 
shading_language_version_vector.size() will only be 1. So we would decline the 
version even when actually "2" > "1.20".
When I think about it: My comment regarding version "1.2" is most likely wrong 
and won't be a problem, sorry.

> + // The shading language version has been detected properly. 
> Exit if the shading language version is too old.
> + const int major_shading_language_version = 
> atoi(shading_language_version_vector.front().c_str());
> + const int minor_shading_language_version = 
> atoi(shading_language_version_vector.at(1).c_str());
> + if (major_shading_language_version < 1 || 
> (major_shading_language_version == 1 && minor_shading_language_version < 20)) 
> {
> + log("ERROR: Shading language version is too old!\n");
> + SDL_ShowSimpleMessageBox(SDL_MESSAGEBOX_ERROR, "OpenGL 
> Error",
> +  
> "Widelands won’t work because your graphics driver is too old.\nThe "
> +  
> "Shading language needs to be version 1.20 or newer.",
> +  NULL);
> + exit(1);
> + }
> + } else {
> + // Exit because we couldn't detect the shading language 
> version, so there must be a problem communicating with the graphics adapter.
>   log("ERROR: Unable to detect the shading language version!\n");
>   SDL_ShowSimpleMessageBox(SDL_MESSAGEBOX_ERROR, "OpenGL Error",
>"Widelands won't work because we were 
> unable to detect the shading "


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1797792-shading-language-version-comparison/+merge/356699
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1797792-shading-language-version-comparison.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1797792-shading-language-version-comparison into lp:widelands

[Notabilis]

Review: Approve

Code looks okay, three (minor) comments in the diff.
It works as expected on my system with en_US and de_DE locale.

Diff comments:

> === modified file 'src/graphic/gl/initialize.cc'
> --- src/graphic/gl/initialize.cc  2018-10-15 05:26:10 +
> +++ src/graphic/gl/initialize.cc  2018-10-15 06:50:28 +
> @@ -178,11 +179,26 @@
>   glGetIntegerv(GL_MAX_TEXTURE_SIZE, max_texture_size);
>   log("Graphics: OpenGL: Max texture size: %u\n", *max_texture_size);
>  
> - // TODO(GunChleoc): Localize the on-screen error messages
> - // Exit if we can't detect the shading language version
>   const char* const shading_language_version_string =
>  reinterpret_cast char*>(glGetString(GL_SHADING_LANGUAGE_VERSION));
> - if (strcmp(shading_language_version_string, "(null)") == 0) {
> + log("Graphics: OpenGL: ShadingLanguage: \"%s\"\n", 
> shading_language_version_string);

This might now be printed before the "Unable to detect version" message. No 
objection, though.

> +
> + std::vector shading_language_version_vector;
> + boost::split(shading_language_version_vector, 
> shading_language_version_string, boost::is_any_of("."));
> + if (shading_language_version_vector.size() >= 2) {

>= or == ?
Also, this might break in the future if "2" is reported as shading version. But 
I guess it is fine for now and we can fix it if this ever happens. Also it 
would be problematic if some driver reports the version as "1.2".

> + // The shading language version has been detected properly. 
> Exit if the shading language version is too old.
> + const int major_shading_language_version = 
> atoi(shading_language_version_vector.front().c_str());
> + const int minor_shading_language_version = 
> atoi(shading_language_version_vector.at(1).c_str());
> + if (major_shading_language_version < 1 || 
> (major_shading_language_version == 1 && minor_shading_language_version < 20)) 
> {
> + log("ERROR: Shading language version is too old!\n");
> + SDL_ShowSimpleMessageBox(SDL_MESSAGEBOX_ERROR, "OpenGL 
> Error",
> +  
> "Widelands won’t work because your graphics driver is too old.\nThe "
> +  
> "Shading language needs to be version 1.20 or newer.",

Maybe move the "The" into the next line? Doesn't matters, but looks strange in 
the source code. ;-)
"Shading" upper or lower case (is in middle of sentence)?

> +  NULL);
> + exit(1);
> + }
> + } else {
> + // Exit because we couldn't detect the shading language 
> version, so there must be a problem communicating with the graphics adapter.
>   log("ERROR: Unable to detect the shading language version!\n");
>   SDL_ShowSimpleMessageBox(SDL_MESSAGEBOX_ERROR, "OpenGL Error",
>"Widelands won't work because we were 
> unable to detect the shading "


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1797792-shading-language-version-comparison/+merge/356699
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1797792-shading-language-version-comparison.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1759857-territorial-lord into lp:widelands

[Notabilis]

Review: Needs Fixing testing

I wasn't able to reproduce the reported bug but it didn't crashed in my tests 
either. However, the final lost/won message is broken, since the land ownership 
is reported as:

Player 1 had 179000% of the land (3580 of 2).

Unfortunately I wasn't able to spot the error. It seems as if the local 
variable fields changes its value but I have no idea how that should happen. 
Apart from that, it worked without problems and the earlier status messages 
were correct as well.

I also looked roughly through the code and haven't noticed any obvious 
mistakes, but that doesn't mean much.

Diff comments:

> === added file 'data/scripting/win_conditions/territorial_functions.lua'
> --- data/scripting/win_conditions/territorial_functions.lua   1970-01-01 
> 00:00:00 +
> +++ data/scripting/win_conditions/territorial_functions.lua   2018-09-29 
> 05:16:29 +
> @@ -0,0 +1,296 @@
> +-- RST
> +-- territorial_functions.lua
> +-- ---
> +--
> +-- This file contains common code for the "Territorial Lord" and 
> "Territorial Time" win conditions.
> +
> +set_textdomain("win_conditions")
> +
> +include "scripting/richtext.lua"
> +include "scripting/win_conditions/win_condition_texts.lua"
> +
> +local team_str = _"Team %i"
> +local wc_has_territory = _"%1$s has %2$3.0f%% of the land (%3$i of %4$i)."
> +local wc_had_territory = _"%1$s had %2$3.0f%% of the land (%3$i of %4$i)."

I don't really like the leading space in front of the percentage. Since the 
player names most likely have different lengths anyway, the whitespace just 
looks strange.
But it is an old decision, so feel free to ignore this comment.

> +
> +-- RST
> +-- .. function:: get_buildable_fields()
> +--
> +--Collects all fields that are buildable
> +--
> +--:returns: a table with the map's buildable fields
> +--
> +function get_buildable_fields()
> +   local fields = {}
> +   local map = wl.Game().map
> +   for x=0, map.width-1 do
> +  for y=0, map.height-1 do
> + local f = map:get_field(x,y)
> + if f.buildable then
> +table.insert(fields, f)
> + end
> +  end
> +   end
> +   print("NOCOM Found " .. #fields .. " buildable fields")
> +   return fields
> +end
> +
> +-- RST
> +-- .. function:: count_owned_fields_for_all_players(fields, players)
> +--
> +--Counts all owned fields for each player.
> +--
> +--:arg fields: Table of all buildable fields
> +--:arg players: Table of all players
> +--
> +--:returns: a table with ``playernumber = count_of_owned_fields``  
> entries
> +--
> +local function count_owned_fields_for_all_players(fields, players)
> +   local owned_fields = {}
> +   -- init the landsizes for each player
> +   for idx,plr in ipairs(players) do
> +  owned_fields[plr.number] = 0
> +   end
> +
> +   for idx,f in ipairs(fields) do
> +  -- check if field is owned by a player
> +  local owner = f.owner
> +  if owner then
> + local owner_number = owner.number
> + if owned_fields[owner_number] == nil then
> +-- In case player was defeated and lost all their warehouses, 
> make sure they don't count
> +owned_fields[owner_number] = -1
> + elseif owned_fields[owner_number] >= 0 then
> +owned_fields[owner_number] = owned_fields[owner_number] + 1
> + end
> +  end
> +   end
> +   return owned_fields
> +end
> +
> +
> +-- Used by calculate_territory_points keep track of when the winner changes
> +local winning_players = {}
> +local winning_teams = {}
> +
> +
> +-- RST
> +-- .. data:: territory_points
> +--
> +--This table contains information about the current points and winning 
> status for all
> +--players and teams:
> +--
> +--.. code-block:: lua
> +--
> +--   territory_points = {
> +--  -- The currently winning team, if any. -1 means that no team is 
> currently winning.
> +--  last_winning_team = -1,
> +--  -- The currently winning player, if any. -1 means that no player 
> is currently winning.
> +--  last_winning_player = -1,
> +--  -- Remaining time in secs for victory by > 50% territory. 
> Default value is also used to calculate whether to send a report to players.
> +--  remaining_time = 10,
> +--  -- Points by player
> +--  all_player_points = {},
> +--  -- Points by rank, used to generate messages to the players
> +--  points = {}
> +--   }
> +--
> +territory_points = {
> +   -- TODO(GunChleoc): We want to be able to list multiple winners in case 
> of a draw.
> +   last_winning_team = -1,
> +   last_winning_player = -1,
> +   remaining_time = 10,
> +   all_player_points = {},
> +   points = {}
> +}
> +
> +-- RST
> +-- .. function:: calculate_territory_points(fields, players, wc_descname, 
> wc_version)
> +--
> +--First checks if a player was defeated, then fills the 
> ``territory_points`` table
> +--with current data.
> +--
> +--

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/string-fixes into lp:widelands

[Notabilis]

Review: Approve diff, testing

Changes sound good to me and output looks as expected.
-- 
https://code.launchpad.net/~widelands-dev/widelands/string-fixes/+merge/356379
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/string-fixes.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1794339-net-win-conditions into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1794339-net-win-conditions into lp:widelands.

Commit message:
Adapt received path to file with win condition for the local filesystem.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1794339 in widelands: "segfault joining game"
  https://bugs.launchpad.net/widelands/+bug/1794339

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1794339-net-win-conditions/+merge/356683

Fixes a bug where Linux players are unable to join games hosted by Windows 
players.

The paths to the files for win conditions are sent with the local path 
separators ('/' on Linux vs. '\' on Windows). While Windows is able to use both 
separators and as such doesn't have a problem receiving the Linux-style path, 
Linux is unable to use the Windows-style path and fails to load the win 
condition.

This branch fixes the issue by transforming the received path to its local 
representation, as is already done for the path of the map file.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1794339-net-win-conditions into lp:widelands.
=== modified file 'src/network/gameclient.cc'
--- src/network/gameclient.cc	2018-10-09 17:02:29 +
+++ src/network/gameclient.cc	2018-10-14 11:07:15 +
@@ -793,7 +793,7 @@
 		break;
 	}
 	case NETCMD_WIN_CONDITION: {
-		d->settings.win_condition_script = packet.string();
+		d->settings.win_condition_script = g_fs->FileSystem::fix_cross_file(packet.string());
 		break;
 	}
 

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1795570-chat-br-tags into lp:widelands

[Notabilis]

Review: Approve diff, testing

Diff is looking good and /help is printed correctly.
Just as a side note since your commit messages indicate otherwise: This also 
affects all chat messages of users, e.g., I can send "line1line2" and it is 
printed in two lines. If that is intentionally (or at least acceptable), then 
this can be merged.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1795570-chat-br-tags/+merge/356653
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1795570-chat-br-tags.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1536377-fail-gracefully-on-graphics-driver-problems into lp:widelands

[Notabilis]

Diff is looking good. I added a few smaller remarks as comments, though.
Testing by modifying the conditions displayed the message boxes for me.

Diff comments:

> === modified file 'src/graphic/gl/initialize.cc'
> --- src/graphic/gl/initialize.cc  2018-09-10 06:11:01 +
> +++ src/graphic/gl/initialize.cc  2018-09-27 09:32:24 +
> @@ -177,8 +178,30 @@
>   glGetIntegerv(GL_MAX_TEXTURE_SIZE, max_texture_size);
>   log("Graphics: OpenGL: Max texture size: %u\n", *max_texture_size);
>  
> - log("Graphics: OpenGL: ShadingLanguage: \"%s\"\n",
> - reinterpret_cast char*>(glGetString(GL_SHADING_LANGUAGE_VERSION)));
> + // TODO(GunChleoc): Localize the on-screen error messages
> + // Exit if we can't detect the shading language version
> + const char* const shading_language_version_string = 
> reinterpret_cast(glGetString(GL_SHADING_LANGUAGE_VERSION));
> + if (!strcmp(shading_language_version_string, "(null)")) {

Maybe use `== 0` instead of `!` ? When reading over it quickly it might be read 
as a check for unequal strings.

> + log("ERROR: Unable to detect the shading language version!\n");
> + SDL_ShowSimpleMessageBox(SDL_MESSAGEBOX_ERROR,
> +  "OpenGL Error",
> +  "Widelands 
> won’t work because we were unable to detect the shading language version – 
> there is an unknown problem with reading the information from the graphics 
> driver.",

On my computer a square box is displayed instead of the long hyphen.

> +  NULL);
> + exit(1);
> + }
> +
> + log("Graphics: OpenGL: ShadingLanguage: \"%s\"\n", 
> shading_language_version_string);
> +
> + // Exit if the shading language version is too old
> + const double shading_language_version = 
> atof(shading_language_version_string);
> + if (shading_language_version < 1.20) {
> + log("ERROR: Shading language version is too old!\n");

"... too old, at least version 1.20 is required!" ?

> + SDL_ShowSimpleMessageBox(SDL_MESSAGEBOX_ERROR,
> +  "OpenGL Error",
> +  "Widelands 
> won’t work because your graphics driver is too old.",
> +  NULL);
> + exit(1);
> + }
>  
>   glDrawBuffer(GL_BACK);
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1536377-fail-gracefully-on-graphics-driver-problems/+merge/355757
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1536377-fail-gracefully-on-graphics-driver-problems
 into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1794924-logger-assert into lp:widelands

[Notabilis]

Diff is looking okay, but unfortunately I am unable to force the message on 
purpose so I wasn't able to test it.
As a suggestion: Maybe use std::cerr instead of std::cout for this message?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1794924-logger-assert/+merge/355823
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1794924-logger-assert into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1794063-extra-chat-message-sound into lp:widelands

[Notabilis]

Review: Approve diff, testing

Diff is looking good and working as intended.
Thanks!

The bunnybot failures are only transient errors, so I think this can be merged.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1794063-extra-chat-message-sound/+merge/355974
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1794063-extra-chat-message-sound.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue into lp:widelands

[Notabilis]

Personally, I would prefer to keep the modifiers in this branch as they are and 
modify the minimize feature, e.g., by only minimizing on clicking the window 
title.
The bug report only mentions the priority buttons, but we have the same problem 
with the dismantle and burn buttons. Admittedly the latter ones already are in 
build19 and nobody complained about them yet. But that would also mean that the 
big buttons of this branch will probably be fine independent of the bug report.

Unfortunately I am unable to merge trunk in this branch, since I am currently 
in a "secure" WiFi where only web and mail is allowed. So no bzr for me at the 
moment.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue/+merge/354732
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue into lp:widelands

[Notabilis]

Sorry, I wasn't able to work on this the last days. Your solution looks good, 
though, thanks!
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue/+merge/354732
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands

[Notabilis]

Thanks for the bug report, the local fix and the review. Your remarks are 
hopefully all fixed now.
Do we need a translator comment for both occurrences of "Replace with %s" or is 
one comment enough when it is the same string?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1669103-no-automatic-dismantlesitewindow into lp:widelands

[Notabilis]

Review: Approve diff, testing

Code is looking good and testing works as intended.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1669103-no-automatic-dismantlesitewindow/+merge/354179
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1669103-no-automatic-dismantlesitewindow.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue into 
lp:widelands.

Commit message:
Adding support for Ctrl and Shift keys when using the increase/decrease buttons 
of input queues.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1791891 in widelands: "Ctrl/Shift-Click InputQueue buttons to change 
capacities faster"
  https://bugs.launchpad.net/widelands/+bug/1791891

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue/+merge/354732

Adding keyboard modifiers for faster adaption of input queue settings (see bug 
report).

Slight disadvantage of this branch: To allow a "decreasing" shift-click on an 
input queue that is already at minimum (and vice versa), the code to disable 
the buttons had to be removed. That means that the increase/decrease buttons 
will always look the same, even when a "normal" click on them won't change 
anything.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1791891-key-modifier-inputqueue into 
lp:widelands.
=== modified file 'src/wui/inputqueuedisplay.cc'
--- src/wui/inputqueuedisplay.cc	2018-09-10 05:59:47 +
+++ src/wui/inputqueuedisplay.cc	2018-09-11 21:08:35 +
@@ -285,7 +285,7 @@
 		return;
 	}
 	if (SDL_GetModState() & KMOD_CTRL) {
-		update_siblings(state);
+		update_siblings_priority(state);
 	}
 	igb_.game().send_player_set_ware_priority(building_, type_, index_, priority);
 }
@@ -294,11 +294,11 @@
 	// Already set option has been clicked again
 	// Unimportant for this queue, but update other queues
 	if (SDL_GetModState() & KMOD_CTRL) {
-		update_siblings(priority_radiogroup_->get_state());
+		update_siblings_priority(priority_radiogroup_->get_state());
 	}
 }
 
-void InputQueueDisplay::update_siblings(int32_t state) {
+void InputQueueDisplay::update_siblings_priority(int32_t state) {
 	// "Release" the CTRL key to avoid recursion
 	const SDL_Keymod old_modifiers = SDL_GetModState();
 	SDL_SetModState(KMOD_NONE);
@@ -340,19 +340,63 @@
  * stored here has been clicked
  */
 void InputQueueDisplay::decrease_max_fill_clicked() {
-	assert(cache_max_fill_ > 0);
 	if (!igb_.can_act(building_.owner().player_number())) {
 		return;
 	}
-	igb_.game().send_player_set_input_max_fill(building_, index_, type_, cache_max_fill_ - 1);
+
+	// Update the value of this queue if required
+	if (cache_max_fill_ > 0) {
+		igb_.game().send_player_set_input_max_fill(building_, index_, type_,
+			((SDL_GetModState() & KMOD_CTRL) ? 0 : cache_max_fill_ - 1));
+	}
+
+	// Update other queues of this building
+	if (SDL_GetModState() & KMOD_SHIFT) {
+		// Using int16_t instead of int32_t on purpose to avoid over-/underflows
+		update_siblings_fill(
+			((SDL_GetModState() & KMOD_CTRL) ? std::numeric_limits::min() : -1));
+	}
 }
 
 void InputQueueDisplay::increase_max_fill_clicked() {
-	assert(cache_max_fill_ < queue_.get_max_size());
 	if (!igb_.can_act(building_.owner().player_number())) {
 		return;
 	}
-	igb_.game().send_player_set_input_max_fill(building_, index_, type_, cache_max_fill_ + 1);
+
+	if (cache_max_fill_ < cache_size_) {
+		igb_.game().send_player_set_input_max_fill(building_, index_, type_,
+			((SDL_GetModState() & KMOD_CTRL) ? cache_size_ : cache_max_fill_ + 1));
+	}
+
+	if (SDL_GetModState() & KMOD_SHIFT) {
+		update_siblings_fill(
+			((SDL_GetModState() & KMOD_CTRL) ? std::numeric_limits::max() : 1));
+	}
+}
+
+void InputQueueDisplay::update_siblings_fill(int32_t delta) {
+	Panel* sibling = get_parent()->get_first_child();
+	// Well, at least we should be a child of our parent
+	assert(sibling != nullptr);
+	do {
+		if (sibling == this) {
+			// We already have been set
+			continue;
+		}
+		InputQueueDisplay* display = dynamic_cast(sibling);
+		if (display == nullptr) {
+			// Cast failed. Sibling is no InputQueueDisplay
+			continue;
+		}
+		uint32_t new_fill = std::max(0,
+std::min(
+	static_cast(display->cache_max_fill_) + delta,
+	display->cache_size_));
+		if (new_fill != display->cache_max_fill_) {
+			igb_.game().send_player_set_input_max_fill(
+			building_, display->index_, display->type_, new_fill);
+		}
+	} while ((sibling = sibling->get_next_sibling()));
 }
 
 void InputQueueDisplay::compute_max_fill_buttons_enabled_state() {
@@ -364,11 +408,5 @@
 			increase_max_fill_->set_enabled(false);
 		if (decrease_max_fill_)
 			decrease_max_fill_->set_enabled(false);
-	} else {
-
-		if (decrease_max_fill_)
-			decrease_max_fill_->set_enabled(cache_max_fill_ > 0);
-		if (increase_max_fill_)
-			increase_max_fill_->set_enabled(cache_max_fill_ < queue_.get_max_size());
 	}
 }

=== modified file 'src/wui/inputqueuedisplay.h'
--- src/wui/inputqueuedisplay.h	2018-09-04 15:48:47 +
+++ src/wui/inputqueuedis

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands

[Notabilis]

The proposal to merge 
lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands 
has been updated.

Description changed to:

When a client disconnects (either on purpose or due to network errors), the 
game is paused, saved, and a dialog is displayed to the host. There, the host 
can select whether to replace the client with a Normal/Weak/Very Weak/Empty AI 
or to exit the game.
No dialog is shown, if the leaving player ...
- is only an observer
- has already lost. In that case the player is replaced by the Empty AI
- has won the game. In that case the player is replaced by the Normal AI

So far, this branch seems to work fine in my testcases and could be merged.
[ignore next part, not an issue of this branch]
However, ASAN is complaining about access to already freed memory when the host 
closes the game with Alt+F4 while the dialog is shown. Strangely, there is no 
complain if the "really exit game" confirmation dialog has been shown but 
aborted.
I wasn't able to figure out what is happening there. So if someone has an idea 
what is causing the problem, please tell me or fix it yourself. Apart from this 
memory bug, the branch is ready for review and testing.
[/ignore]

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands

[Notabilis]

Thanks for the explanation, it makes much more sense now. I haven't considered 
that the dropbox needs to access "higher" parents, so it was quite a strange 
effect. But I guess that means that this branch isn't to blame and can be 
reviewed and merged.

Would you mind copying your explanation to a new bug report about the memory 
leak and targeting it for build-21? The leak doesn't seem to cause any 
immediate harm so I think we can postpone fixing it until your other branches 
are in.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1766957-sinking-ship-animation into lp:widelands

[Notabilis]

Review: Approve

Thanks, that seems to fix it.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1766957-sinking-ship-animation/+merge/353746
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1766957-sinking-ship-animation.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1766957-sinking-ship-animation into lp:widelands

[Notabilis]

Review: Needs Fixing

Thanks for looking into this.
Unfortunately, this branch doesn't fix it for me.
I took a look into this now and I think the bug might be due to a mismatch 
between animation duration (3000 ms), animation speed (7 fps) and number of 
animation frames (20 images). In some cases (strangely it seems to be random) 
this leads to 21 frames being displayed, looping and showing the first image of 
the sinking animation again. That image is more or less the same as in the idle 
animation, so it probably isn't the idle animation playing at all.
Fixing it would mean changing one of these values. I guess the easiest one 
would be the number of images by copying the (empty) 20st image and naming it 
as 21 (for both Atlanteans and Frisians).

Even when it doesn't fix the bug, I tend to merge your code change anyway. It 
seems to be the more correct behavior independent of the bug. After all, the 
code should never reach this method when the ship is currently sinking.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1766957-sinking-ship-animation/+merge/353746
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1766957-sinking-ship-animation.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/fix-codecheck-boost-sha1 into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/fix-codecheck-boost-sha1 into lp:widelands.

Commit message:
Including boost sha1 headers as system headers.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/fix-codecheck-boost-sha1/+merge/354633

CodeCheck is interpreting the boost headers in network/crypto.cc as local 
Widelands includes, since they are included as
  #include "boost/sha1.h"

Instead, they should be included as
  #include 
since boost is a system header (or at least, not local to Widelands).
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/fix-codecheck-boost-sha1 into lp:widelands.
=== modified file 'src/network/crypto.cc'
--- src/network/crypto.cc	2018-09-10 06:11:01 +
+++ src/network/crypto.cc	2018-09-10 18:43:08 +
@@ -1,9 +1,9 @@
 #include "network/crypto.h"
 
 #if BOOST_VERSION > 106700
-#include "boost/uuid/detail/sha1.hpp"
+#include 
 #else
-#include "boost/uuid/sha1.hpp"
+#include 
 #endif
 
 namespace crypto {

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands.

Commit message:
Adding a dialog for the host when the connection to a client is lost. Allows 
the host to select whether to replace the client with an AI or to exit.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1434875 in widelands: "Multiplayer network problems go unnoticed"
  https://bugs.launchpad.net/widelands/+bug/1434875

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1434875-net-client-disconnect/+merge/354531

This is not ready for merge yet!

When a client disconnects (either on purpose or due to network errors), the 
game is paused, saved, and a dialog is displayed to the host. There, the host 
can select whether to replace the client with a Normal/Weak/Very Weak/Empty AI 
or to exit the game.
No dialog is shown, if the leaving player ...
- is only an observer
- has already lost. In that case the player is replaced by the Empty AI
- has won the game. In that case the player is replaced by the Normal AI

So far, this branch seems to work fine in my testcases and could be merged.
However, ASAN is complaining about access to already freed memory when the host 
closes the game with Alt+F4 while the dialog is shown. Strangely, there is no 
complain if the "really exit game" confirmation dialog has been shown but 
aborted.
I wasn't able to figure out what is happening there. So if someone has an idea 
what is causing the problem, please tell me or fix it yourself. Apart from this 
memory bug, the branch is ready for review and testing.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1434875-net-client-disconnect into lp:widelands.
=== modified file 'src/network/gamehost.cc'
--- src/network/gamehost.cc	2018-04-07 16:59:00 +
+++ src/network/gamehost.cc	2018-09-09 17:19:05 +
@@ -31,6 +31,7 @@
 #endif
 
 #include "ai/computer_player.h"
+#include "ai/defaultai.h"
 #include "base/i18n.h"
 #include "base/md5.h"
 #include "base/warning.h"
@@ -62,6 +63,7 @@
 #include "ui_basic/progresswindow.h"
 #include "ui_fsmenu/launch_mpg.h"
 #include "wlapplication.h"
+#include "wui/game_client_disconnected.h"
 #include "wui/game_tips.h"
 #include "wui/interactive_player.h"
 #include "wui/interactive_spectator.h"
@@ -560,6 +562,25 @@
 	->instantiate(*d->game, p));
 }
 
+void GameHost::start_ai_for(uint8_t playernumber, const std::string& ai) {
+	assert(d->game->get_player(playernumber + 1)->get_ai().empty());
+	assert(d->game->get_player(playernumber + 1)->get_ai()
+			== d->settings.players.at(playernumber).ai);
+	// Inform all players about the change
+	// Has to be done at first in this method since the calls later on overwrite players[].name
+	send_system_message_code(
+		"CLIENT_X_REPLACED_WITH",
+		d->settings.players.at(playernumber).name,
+		ComputerPlayer::get_implementation(ai)->descname);
+	set_player_ai(playernumber, ai, false);
+	d->game->get_player(playernumber + 1)->set_ai(ai);
+	// Activate the ai
+	init_computer_player(playernumber + 1);
+	set_player_state(playernumber, PlayerSettings::State::kComputer);
+	assert(d->game->get_player(playernumber + 1)->get_ai()
+			== d->settings.players.at(playernumber).ai);
+}
+
 void GameHost::init_computer_players() {
 	const Widelands::PlayerNumber nr_players = d->game->map().get_nrplayers();
 	iterate_players_existing_novar(p, nr_players, *d->game) {
@@ -2254,8 +2275,7 @@
 	}
 
 	set_player_state(number, PlayerSettings::State::kOpen);
-	if (d->game)
-		init_computer_player(number + 1);
+	// Don't replace player with AI, let host choose what to do
 }
 
 void GameHost::disconnect_client(uint32_t const number,
@@ -2266,6 +2286,37 @@
 
 	Client& client = d->clients.at(number);
 
+	// If the client is linked to a player and it is the client that closes the connection
+	// and the game has already started ...
+	if (client.playernum != UserSettings::none() && reason != "SERVER_LEFT" && d->game != nullptr) {
+		// And the client hasn't lost/won yet ...
+		if (d->settings.users.at(client.usernum).result == Widelands::PlayerEndResult::kUndefined) {
+			// And the window isn't visible yet ...
+			if (!client_disconnected_.window) {
+// Show a window and ask the host player what to do with the tribe of the leaving client
+
+if (!forced_pause()) {
+	force_pause();
+}
+
+WLApplication::emergency_save(*d->game);
+
+new GameClientDisconnected(d->game->get_ipl(), client_disconnected_, this);
+			}
+		// Client was active but is a winner of the game: Replace with normal AI
+		} else if (d->settings.users.at(

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1772168-unused-key-in-luatable into lp:widelands

[Notabilis]

Diff is looking okay and the error messages are gone.

[Disclaimer: I don't know much about the lua interface.]
Basically the error message are avoided by not using get_int() anymore but 
using array_entries(). Is there a reason against fixing (?) get_int() (and 
others) so the usage of the table key is stored in the set accessed_keys_ as 
well, as is done inside array_entries() ? From the code it seems as if using 
any method except array_entries() will lead to the error message, which seems 
strange to me.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1772168-unused-key-in-luatable/+merge/354202
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1772168-unused-key-in-luatable into 
lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/scout-compatibility into lp:widelands

[Notabilis]

Review: Approve diff, short testing

Diff looks good and a newly started game with this branch using a scout didn't 
showed any apparent problems.
-- 
https://code.launchpad.net/~widelands-dev/widelands/scout-compatibility/+merge/354345
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/scout-compatibility.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/const_portdock_fleet_and_ship_functions into lp:widelands

[Notabilis]

Review: Approve diff

Diff is looking good.
I haven't bothered compiling or testing myself, but Travis seems to be fine.
-- 
https://code.launchpad.net/~widelands-dev/widelands/const_portdock_fleet_and_ship_functions/+merge/354301
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/const_portdock_fleet_and_ship_functions.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe into lp:widelands

[Notabilis]

Review: Needs Fixing

I like the proposed change. However, when starting the editor and clicking the 
"Players" button I get an exception:

widelands: ../src/logic/map.cc:411: const string& 
Widelands::Map::get_scenario_player_tribe(Widelands::PlayerNumber) const: 
Assertion `p <= get_nrplayers()' failed.

Problem is the change in diff line 81, where p can be greater than the number 
of active players.

Since the editor-user has the option to do so now anyway: Select the "random" 
tribe in line 42 of the diff?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352943
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1774591-loading-screen-tips into lp:widelands

[Notabilis]

Review: Approve

Diff is looking good and I can confirm the fix ingame.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1774591-loading-screen-tips/+merge/353556
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1774591-loading-screen-tips.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1784122-singleplayer-viewport into lp:widelands

[Notabilis]

*with a debug build, sorry.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1784122-singleplayer-viewport/+merge/353391
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1784122-singleplayer-viewport.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1784122-singleplayer-viewport into lp:widelands

[Notabilis]

That bug is really fun but the streamreader isn't to blame. ;-)
The problem is that the float_32() call not only returns the value but also 
modifies the internal state of the stream. As a consequence, the order of 
assignment to x and y matters (or more precisely, the order in which the 
respective float_32() calls are made).
It seems as if calling it twice as constructor parameters results in the 
*second* call being done first, resulting in x and y being swapped after the 
assignment. Doing only one call as parameter and assigning the other one later 
on is fine. Or just assign both later on as in this branch.
Took me quite some time of trial until I realized that...

I am using gcc version 8.2.0 with a release build. The order of the evaluation 
is undefined in the standard, so it might indeed be compiler dependent.

The null-assignment is required since the default constructor is deleted.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1784122-singleplayer-viewport/+merge/353391
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1784122-singleplayer-viewport.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1784122-singleplayer-viewport into lp:widelands

[Notabilis]

Review: Approve

Code is looking good and working as intended.

That definitely is one nasty bug...
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1784122-singleplayer-viewport/+merge/353391
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1784122-singleplayer-viewport.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe into lp:widelands

[Notabilis]

Review: Approve

Code is looking okay and the editor no longer crashes. Two observations:

- When loading the old/broken maps, we still get the "Tribe not known" message. 
Maybe only display a "note:" message and select a default tribe in that case? 
Having the wrong tribe is in most cases probably preferable to not being able 
to use the map at all. Unknown tribes in maps could also happen when 
"backporting" maps from (newer) game versions where more/custom tribes are 
available.

- When loading the "broken" map in the editor and saving again the map is still 
broken. This one is actually a bit strange, shouldn't the change in 
editorinteractive.cc avoid this as well?

Diff comments:

> === modified file 'src/editor/editorinteractive.cc'
> --- src/editor/editorinteractive.cc   2018-07-20 08:42:23 +
> +++ src/editor/editorinteractive.cc   2018-08-12 17:17:16 +
> @@ -180,7 +180,7 @@
>   loader_ui.step(_("Creating players"));
>   iterate_player_numbers(p, map->get_nrplayers()) {
>   egbase().add_player(
> -p, 0, map->get_scenario_player_tribe(p), 
> map->get_scenario_player_name(p));
> +p, 0, map->get_scenario_player_tribe(p).empty() ? 
> Widelands::get_all_tribenames().front() : map->get_scenario_player_tribe(p), 
> map->get_scenario_player_name(p));

Why no random selection here?

>   }
>  
>   ml->load_map_complete(egbase(), 
> Widelands::MapLoader::LoadType::kEditor);


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1783878_editor_random_map_tribe/+merge/352943
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1783878_editor_random_map_tribe.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1784200-single-line-escaping into lp:widelands

[Notabilis]

Thanks for the review.

Interesting to know that we will move to GitHub.
Since we are now only linking to instructions about how to report a bug, maybe 
change "Please report bugs at:" to "For instructions about how to report bugs, 
see:" ?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1784200-single-line-escaping/+merge/353446
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1784200-single-line-escaping.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1784200-single-line-escaping into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1784200-single-line-escaping into lp:widelands.

Commit message:
More strict sanitizing of chat messages. Printing a welcome message on joining 
the metaserver.

Requested reviews:
  GunChleoc (gunchleoc)
Related bugs:
  Bug #1784200 in widelands: "clash with font renderer and server messages?"
  https://bugs.launchpad.net/widelands/+bug/1784200

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1784200-single-line-escaping/+merge/353446

More strict sanitizing of chat messages to avoid future bugs with formatted 
text. All characters are now displayed as they are entered and are no longer 
interpreted as richtext.

Also, printing a message when joining the metaserver lobby, similar to the 
previous message send by the metaserver.
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-1784200-single-line-escaping.
=== modified file 'src/network/internet_gaming.cc'
--- src/network/internet_gaming.cc	2018-05-03 14:24:27 +
+++ src/network/internet_gaming.cc	2018-08-20 19:15:14 +
@@ -367,6 +367,7 @@
 
 		} else if (cmd == IGPCMD_LOGIN) {
 			// Clients request to login was granted
+			format_and_add_chat("", "", true, _("Welcome on the Widelands Metaserver!"));
 			const std::string assigned_name = packet.string();
 			if (clientname_ != assigned_name) {
 format_and_add_chat(
@@ -383,6 +384,10 @@
 reg_ = false;
 authenticator_ = crypto::sha1(clientname_ + authenticator_);
 			}
+			format_and_add_chat("", "", true, _("Our forums can be found at:"));
+			format_and_add_chat("", "", true, _("https://wl.widelands.org/forum/;));
+			format_and_add_chat("", "", true, _("Please report bugs at:"));
+			format_and_add_chat("", "", true, _("https://launchpad.net/widelands;));
 			state_ = LOBBY;
 			log("InternetGaming: Client %s logged in.\n", clientname_.c_str());
 			return;

=== modified file 'src/wui/chat_msg_layout.cc'
--- src/wui/chat_msg_layout.cc	2018-04-07 16:59:00 +
+++ src/wui/chat_msg_layout.cc	2018-08-20 19:15:14 +
@@ -46,7 +46,7 @@
 	const std::string& font_face = "serif";
 	std::string message = "";
 
-	std::string sanitized = sanitize_message(chat_message);
+	std::string sanitized = richtext_escape(chat_message.msg);
 
 	// time calculation
 	char ts[13];
@@ -99,49 +99,3 @@
 	// return the formated message
 	return message + "";
 }
-
-std::string sanitize_message(const ChatMessage& chat_message) {
-	// Escape richtext characters
-	// The goal of this code is two-fold:
-	//  1. Assuming an honest game host, we want to prevent the ability of
-	// clients to use richtext.
-	//  2. Assuming a malicious host or meta server, we want to reduce the
-	// likelihood that a bug in the richtext renderer can be exploited,
-	// by restricting the set of allowed richtext commands.
-	// Most notably, images are not allowed in richtext at all.
-	//
-	// Note that we do want host and meta server to send some richtext code,
-	// as the ability to send formatted commands is nice for the usability
-	// of meta server so we're treading a bit of a fine line here.
-
-	if (chat_message.playern >= 0) {
-		return richtext_escape(chat_message.msg);
-	}
-
-	std::string sanitized;
-	for (std::string::size_type pos = 0; pos < chat_message.msg.size(); ++pos) {
-		if (chat_message.msg[pos] == '<') {
-			static const std::string good1 = "', pos + good1.size());
-if (nextclose != std::string::npos &&
-(nextclose == pos + good1.size() || chat_message.msg[pos + good1.size()] == ' ')) {
-	sanitized += good1;
-	pos += good1.size() - 1;
-	continue;
-}
-			} else if (!chat_message.msg.compare(pos, good2.size(), good2)) {
-sanitized += good2;
-pos += good2.size() - 1;
-continue;
-			}
-
-			sanitized += "";
-		} else {
-			sanitized += chat_message.msg[pos];
-		}
-	}
-	return sanitized;
-}

=== modified file 'src/wui/chat_msg_layout.h'
--- src/wui/chat_msg_layout.h	2018-04-07 16:59:00 +
+++ src/wui/chat_msg_layout.h	2018-08-20 19:15:14 +
@@ -25,6 +25,4 @@
 // Formats 'chat_message' as richtext.
 std::string format_as_richtext(const ChatMessage& chat_message);
 
-std::string sanitize_message(const ChatMessage& chat_message);
-
 #endif  // end of include guard: WL_WUI_CHAT_MSG_LAYOUT_H

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/lenient_allowed_buildings into lp:widelands

[Notabilis]

Review: Approve code

I haven't tested it, but the code looks okay.

One could consider displaying an in-game message instead/additionally to the 
console output. The console is probably overlooked by players, but a change in 
the building definitions could lead to broken scenario save games. Personally I 
don't think that this is important, though, if such a case happens loading will 
probably break in other steps as well.
-- 
https://code.launchpad.net/~widelands-dev/widelands/lenient_allowed_buildings/+merge/351749
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/lenient_allowed_buildings.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1785404-chat-scrolling into lp:widelands

[Notabilis]

Notabilis has proposed merging 
lp:~widelands-dev/widelands/bug-1785404-chat-scrolling into lp:widelands.

Commit message:
Fixing broken auto-scrolling of multi-line text areas.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1785404 in widelands: "Chat does not autoscroll"
  https://bugs.launchpad.net/widelands/+bug/1785404

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1785404-chat-scrolling/+merge/352941

Seems to be a case of wrong direction of comparison. I would appreciate it if 
someone thinks about it for a moment before merging.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug-1785404-chat-scrolling into lp:widelands.
=== modified file 'src/ui_basic/multilinetextarea.cc'
--- src/ui_basic/multilinetextarea.cc	2018-07-28 07:02:25 +
+++ src/ui_basic/multilinetextarea.cc	2018-08-12 16:43:10 +
@@ -104,7 +104,7 @@
 		scrollbar_.set_steps(height - get_h());
 
 		if (scrollmode_ == ScrollMode::kScrollLog || scrollmode_ == ScrollMode::kScrollLogForced) {
-			if (scrollbar_.get_scrollpos() >= scrollbar_.get_steps() - 1) {
+			if (scrollbar_.get_scrollpos() < scrollbar_.get_steps() - 1) {
 scrollbar_.set_scrollpos(height - get_h());
 			}
 		}

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/inputwarequeue_display into lp:widelands

[Notabilis]

Review: Approve

I can't reproduce my bug any longer either, so as far as I am concerned this 
can go in. Thanks!
-- 
https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/inputwarequeue_display.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/inputwarequeue_display into lp:widelands

[Notabilis]

Thanks, it seems much more logical now. Mostly, that is, sometimes the "dark" 
section changes its size based on the current max-amount (i.e., 2 cloth 
requested -> 2 dark; 3 cloth requested -> only 1 dark). Might be correct inside 
the code based on how requests are calculated, guess you can't do much about 
that.

I think coal is fine, wheat and cloth are a bit hard to differ but in my 
opinion it is good enough. You can see the difference, so I think you can leave 
it as it is now.

An unfortunate bug I noticed: I have 1 cloth in my whole economy (based on ware 
statistics). With two shipyards each requesting 2 cloth, all the 2*2=4 cloth is 
displayed as "on their way" which just can't  be the case. In the end, only 1 
cloth is delivered while the rest is still reported as "on their way". I guess 
request_->get_num_transfers() should have better been called 
request_->get_num_requests() ? I haven't looked in the code, maybe there is 
some method which really returns the number of "wares on their way to fulfill 
this request". :-/
-- 
https://code.launchpad.net/~widelands-dev/widelands/inputwarequeue_display/+merge/350385
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/inputwarequeue_display into lp:widelands.

___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp