Merge lp:~widelands-dev/widelands/economy-target-profiles into lp:widelands

[Benedikt Straub]

Fix economy targets profile application when no items are selected

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 5103. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/539624587.
Appveyor build 4884. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_economy_target_profiles-4884.

[GunChleoc (gunchleoc)]

Selecting the profile is working now, but it shows up as "-" in the dropdown afterwards, although all wares have been adjusted, including the selected ones.

[Benedikt Straub (nordfriese)]

I cannot reproduce that... When nothing or everything is selected, all wares get changed and the dropdown shows the chosen profile. When some wares are selected, only those are changed and "-" appears as expected.

[GunChleoc (gunchleoc)]

Maybe I did not describe it well enough - I have recorded a video:

https://bugs.launchpad.net/widelands/+bug/1831196/comments/1

I also got a heap-use-after-free, but that's probably already fixed via one of my branches

https://code.launchpad.net/~widelands-dev/widelands/fix-dropdowns/+merge/368223

[Benedikt Straub (nordfriese)]

But this is the correct behaviour :) The dropdown always shows the profile that applies to the entire tab. IMHO this is more intuitive than making the dropdown label dependent only on the selected items…

[GunChleoc (gunchleoc)]

When I load a profile, I expect that profile to be applied and overwrite everything, no matter if I have currently selected something or not.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 5181. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/544759045.
Appveyor build 4961. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_economy_target_profiles-4961.

[Benedikt Straub (nordfriese)]

When you want to apply a profile to everything, you can select either nothing or everything. The dropdown tooltip says "Profile to apply to the selected items". All other buttons affect only the selected items, so it´s consistent that the dropdown should affect only the selection as well. Changing all wares when only some are selected would be an annoying and unexpected behaviour IMHO.

[GunChleoc (gunchleoc)]

Ah, now I get it. I' still not sure that this is what it should do. but the behavior is as described now, so let's merge this branch. Thanks for fixing!

@bunnybot merge

review: Approve

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 5207. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/548539125.
Appveyor build 4986. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_economy_target_profiles-4986.

[bunnybot (widelandsofficial)]

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

Travis build 5207. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/548539125.

[Benedikt Straub (nordfriese)]

three inputqueue fails in one build :P

@bunnybot merge force

=== modified file 'src/wui/economy_options_window.cc'
--- src/wui/economy_options_window.cc	2019-05-29 16:59:16 +0000
+++ src/wui/economy_options_window.cc	2019-06-21 06:24:18 +0000
@@ -307,29 +307,25 @@
 	const PredefinedTargets settings = economy_options_window_->get_selected_target();
 
 	bool anything_selected = false;
-	bool second_phase = false;
-
-	do {
-		for (const Widelands::DescriptionIndex& index : items) {
-			if (display_.ware_selected(index) || (second_phase && !display_.is_ware_hidden(index))) {
-				anything_selected = true;
-				if (is_wares) {
-					game.send_player_command(new Widelands::CmdSetWareTargetQuantity(
-					   game.get_gametime(), player_->player_number(), serial_, index,
-					   settings.wares.at(index)));
-				} else {
-					game.send_player_command(new Widelands::CmdSetWorkerTargetQuantity(
-					   game.get_gametime(), player_->player_number(), serial_, index,
-					   settings.workers.at(index)));
-				}
+	for (const Widelands::DescriptionIndex& index : items) {
+		if (display_.ware_selected(index)) {
+			anything_selected = true;
+			break;
+		}
+	}
+	for (const Widelands::DescriptionIndex& index : items) {
+		if (display_.ware_selected(index) || (!anything_selected && !display_.is_ware_hidden(index))) {
+			if (is_wares) {
+				game.send_player_command(new Widelands::CmdSetWareTargetQuantity(
+				   game.get_gametime(), player_->player_number(), serial_, index,
+				   settings.wares.at(index)));
+			} else {
+				game.send_player_command(new Widelands::CmdSetWorkerTargetQuantity(
+				   game.get_gametime(), player_->player_number(), serial_, index,
+				   settings.workers.at(index)));
 			}
 		}
-		if (anything_selected) {
-			return;
-		}
-		// Nothing was selected, now go through the loop again and change everything
-		second_phase = true;
-	} while (!second_phase);
+	}
 }
 
 constexpr unsigned kThinkInterval = 200;