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

Proposed by Benedikt Straub
Status: Merged
Merged at revision: 9153
Proposed branch: lp:~widelands-dev/widelands/economy-target-profiles
Merge into: lp:widelands
Diff against target: 50 lines (+17/-21)
1 file modified
src/wui/economy_options_window.cc (+17/-21)
To merge this branch: bzr merge lp:~widelands-dev/widelands/economy-target-profiles
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+368181@code.launchpad.net

Commit message

Fix economy targets profile application when no items are selected

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

Revision history for this message
Benedikt Straub (nordfriese) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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

Revision history for this message
Benedikt Straub (nordfriese) wrote :

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…

Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
Benedikt Straub (nordfriese) wrote :

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.

9120. By Nordfriese

Merged trunk

Revision history for this message
GunChleoc (gunchleoc) wrote :

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
Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

Revision history for this message
Benedikt Straub (nordfriese) wrote :

three inputqueue fails in one build :P

@bunnybot merge force

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/wui/economy_options_window.cc'
2--- src/wui/economy_options_window.cc 2019-05-29 16:59:16 +0000
3+++ src/wui/economy_options_window.cc 2019-06-21 06:24:18 +0000
4@@ -307,29 +307,25 @@
5 const PredefinedTargets settings = economy_options_window_->get_selected_target();
6
7 bool anything_selected = false;
8- bool second_phase = false;
9-
10- do {
11- for (const Widelands::DescriptionIndex& index : items) {
12- if (display_.ware_selected(index) || (second_phase && !display_.is_ware_hidden(index))) {
13- anything_selected = true;
14- if (is_wares) {
15- game.send_player_command(new Widelands::CmdSetWareTargetQuantity(
16- game.get_gametime(), player_->player_number(), serial_, index,
17- settings.wares.at(index)));
18- } else {
19- game.send_player_command(new Widelands::CmdSetWorkerTargetQuantity(
20- game.get_gametime(), player_->player_number(), serial_, index,
21- settings.workers.at(index)));
22- }
23+ for (const Widelands::DescriptionIndex& index : items) {
24+ if (display_.ware_selected(index)) {
25+ anything_selected = true;
26+ break;
27+ }
28+ }
29+ for (const Widelands::DescriptionIndex& index : items) {
30+ if (display_.ware_selected(index) || (!anything_selected && !display_.is_ware_hidden(index))) {
31+ if (is_wares) {
32+ game.send_player_command(new Widelands::CmdSetWareTargetQuantity(
33+ game.get_gametime(), player_->player_number(), serial_, index,
34+ settings.wares.at(index)));
35+ } else {
36+ game.send_player_command(new Widelands::CmdSetWorkerTargetQuantity(
37+ game.get_gametime(), player_->player_number(), serial_, index,
38+ settings.workers.at(index)));
39 }
40 }
41- if (anything_selected) {
42- return;
43- }
44- // Nothing was selected, now go through the loop again and change everything
45- second_phase = true;
46- } while (!second_phase);
47+ }
48 }
49
50 constexpr unsigned kThinkInterval = 200;

Subscribers

People subscribed via source and target branches

to status/vote changes: