Merge lp:~widelands-dev/widelands/fix-bug-1208130 into lp:widelands

Proposed by cghislai on 2013-08-08
Status: Merged
Merged at revision: 6715
Proposed branch: lp:~widelands-dev/widelands/fix-bug-1208130
Merge into: lp:widelands
Diff against target: 127 lines (+35/-25)
3 files modified
src/wui/buildingwindow.cc (+1/-19)
src/wui/buildingwindow.h (+0/-2)
src/wui/soldierlist.cc (+34/-4)
To merge this branch: bzr merge lp:~widelands-dev/widelands/fix-bug-1208130
Reviewer Review Type Date Requested Status
Nasenbaer Approve on 2013-08-12
Teppo Mäenpää Resubmit on 2013-08-09
SirVer 2013-08-08 Needs Fixing on 2013-08-09
Review via email: mp+179293@code.launchpad.net

Description of the change

I suggest merging Teppo's fix to the desync issue with soldiers preference. It has been tested

To post a comment you must log in.
SirVer (sirver) wrote :

This has conflicts with master - hard to see what you actually changed.

review: Needs Fixing
6703. By Teppo \<<email address hidden>\> on 2013-08-09

Merged with changes from trunk.

Teppo Mäenpää (kxq) wrote :

Hi, I have now merged trunk/6711 to this branch for a re-review.

About the merge conflict:
There was a line saying " if ( !workarea_info.size() ) return ; show_workarea();"
code checker complained about it.
I had changed that to " if ( workarea_info.empty() ) return ; show_workarea();"
In trunk it was changed to " if ( !workarea_info.empty() ) return ; show_workarea();"
As these two modifications applied to the same line were not the same, there was a conflict.

I prefer my fix in the conflicting line better, as in current trunk the workarea is shown only when there is no workarea, while here it works, as before. Of course, my view could be biased.

Do you see it a bad thing, if people like me piggy-back some compiler/codechecker warning fixes to other fixes?

=================================

What was changed:

- Removed dead code from buildingwindow (methods void act_prefer_rookies() and act_prefer_heroes() )
- Changed the soldier list so that only player can click the preference button
- Changed the soldier to use the command relay function instead of directly changing the militarysite, to avoid desyncs in multiplayer games.
- Something I did not do, presumably Charly also figured out to keep the preference-tab up-to-date for observers ?

review: Resubmit
Teppo Mäenpää (kxq) wrote :

About the last bullet in the list of changes above: Yes, the preference stays in synch now -- I forgot to fix that line in my comment. It takes me about one hour to compile widelands, so I wrote the above text while waiting for miracles to happen.. SirVer, please re-review this branch, and merge if you find it OK.

SirVer (sirver) wrote :

One hour!!?!?! Oh wow. Are you using ccache? http://ccache.samba.org/ . It takes me < 5 minutes to make a fresh build after a make clean.

I will review as soons as I find some energy for this. Thanks for working on this Teppo.

Teppo Mäenpää (kxq) wrote :

Widelands is the most heavy-weight application I have ever used (off-work, that is), by a wide margin. My computer is just slow, CPU-wise. It only hurts when working with widelands, in all other cases I prefer my no-moving-parts-except-in-the-keyboard approach. It is faster than raspberrypi.. Normal computers make horrible sounds. To be honest, I have considered buying a normal computer just to speed WL (both game and compilation) up.

I do not think that any software solutions can overcome that. Turning some optimizations off in debug builds might help.

6704. By Teppo \<<email address hidden>\> on 2013-08-11

Merged trunk/6713

Nasenbaer (nasenbaer) wrote :

Looks perfectly fine to me :).
Thank you for the fix Teppo!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/wui/buildingwindow.cc'
2--- src/wui/buildingwindow.cc 2013-08-10 16:57:13 +0000
3+++ src/wui/buildingwindow.cc 2013-08-11 03:39:50 +0000
4@@ -423,24 +423,6 @@
5 die();
6 }
7
8-void Building_Window::act_prefer_rookies()
9-{
10- if (is_a(Widelands::MilitarySite, &m_building))
11- igbase().game().send_player_militarysite_set_soldier_preference
12- (m_building, Widelands::MilitarySite::kPrefersRookies);
13-
14- die();
15-}
16-
17-void
18-Building_Window::act_prefer_heroes()
19-{
20- if (is_a(Widelands::MilitarySite, &m_building))
21- igbase().game().send_player_militarysite_set_soldier_preference
22- (m_building, Widelands::MilitarySite::kPrefersHeroes);
23-
24- die();
25-}
26
27 /**
28 ===============
29@@ -501,7 +483,7 @@
30 } else {
31 workarea_info = m_building.descr().m_workarea_info;
32 }
33- if (!workarea_info.empty()) {
34+ if (workarea_info.empty()) {
35 return;
36 }
37 m_workarea_job_id = igbase().show_work_area(workarea_info, m_building.get_position());
38
39=== modified file 'src/wui/buildingwindow.h'
40--- src/wui/buildingwindow.h 2013-08-02 14:20:21 +0000
41+++ src/wui/buildingwindow.h 2013-08-11 03:39:50 +0000
42@@ -66,8 +66,6 @@
43 void toggle_workarea();
44 void configure_workarea_button();
45 void act_start_stop();
46- void act_prefer_rookies();
47- void act_prefer_heroes();
48 void act_start_or_cancel_expedition();
49 void act_enhance(Widelands::Building_Index);
50 void clicked_goto();
51
52=== modified file 'src/wui/soldierlist.cc'
53--- src/wui/soldierlist.cc 2013-07-27 10:35:55 +0000
54+++ src/wui/soldierlist.cc 2013-08-11 03:39:50 +0000
55@@ -368,6 +368,7 @@
56 void mouseover(const Soldier * soldier);
57 void eject(const Soldier * soldier);
58 void set_soldier_preference(int32_t changed_to);
59+ void think();
60
61 Interactive_GameBase & m_igb;
62 Widelands::Building & m_building;
63@@ -406,6 +407,7 @@
64
65 UI::Box * buttons = new UI::Box(this, 0, 0, UI::Box::Horizontal);
66
67+ bool can_act = m_igb.can_act(m_building.owner().player_number());
68 if (upcast(Widelands::MilitarySite, ms, &building)) {
69 m_soldier_preference.add_button
70 (buttons, Point(0, 0), g_gr->images().get("pics/prefer_rookies.png"), _("Prefer Rookies"));
71@@ -421,8 +423,12 @@
72 if (ms->get_soldier_preference() == Widelands::MilitarySite::kPrefersHeroes) {
73 m_soldier_preference.set_state(1);
74 }
75- m_soldier_preference.changedto.connect
76- (boost::bind(&SoldierList::set_soldier_preference, this, _1));
77+ if (can_act) {
78+ m_soldier_preference.changedto.connect
79+ (boost::bind(&SoldierList::set_soldier_preference, this, _1));
80+ } else {
81+ m_soldier_preference.set_enabled(false);
82+ }
83 }
84 buttons->add_inf_space();
85 buttons->add
86@@ -437,6 +443,28 @@
87 return *dynamic_cast<SoldierControl *>(&m_building);
88 }
89
90+void SoldierList::think()
91+{
92+ // Only update the soldiers pref radio if player is spectator
93+ if (m_igb.can_act(m_building.owner().player_number())) {
94+ return;
95+ }
96+ if (upcast(Widelands::MilitarySite, ms, &m_building)) {
97+ switch (ms->get_soldier_preference()) {
98+ case Widelands::MilitarySite::kPrefersRookies:
99+ m_soldier_preference.set_state(0);
100+ break;
101+ case Widelands::MilitarySite::kPrefersHeroes:
102+ m_soldier_preference.set_state(1);
103+ break;
104+ default:
105+ m_soldier_preference.set_state(-1);
106+ break;
107+ }
108+ }
109+}
110+
111+
112 void SoldierList::mouseover(const Soldier * soldier)
113 {
114 if (!soldier) {
115@@ -474,8 +502,10 @@
116 void SoldierList::set_soldier_preference(int32_t changed_to) {
117 upcast(Widelands::MilitarySite, ms, &m_building);
118 assert(ms);
119- ms->set_soldier_preference
120- (changed_to == 0 ? Widelands::MilitarySite::kPrefersRookies : Widelands::MilitarySite::kPrefersHeroes);
121+ m_igb.game().send_player_militarysite_set_soldier_preference
122+ (m_building, changed_to == 0 ?
123+ Widelands::MilitarySite::kPrefersRookies:
124+ Widelands::MilitarySite::kPrefersHeroes);
125 }
126
127 UI::Panel * create_soldier_list