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

Proposed by Notabilis
Status: Merged
Merged at revision: 9197
Proposed branch: lp:~widelands-dev/widelands/bug-1842396-no-tribe
Merge into: lp:widelands
Diff against target: 69 lines (+13/-9)
4 files modified
src/logic/game_settings.h (+4/-1)
src/network/gameclient.cc (+7/-4)
src/network/gamehost.cc (+1/-2)
src/wlapplication.cc (+1/-2)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1842396-no-tribe
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+372281@code.launchpad.net

Commit message

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

Description of the change

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

Seems to be a copy&paste or refactoring bug: When creating the tips as a single player or multiplayer host, the check for the thrown exception is done.

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

Continuous integration builds have changed state:

Travis build 5387. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/580833741.
Appveyor build 5157. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1842396_no_tribe-5157.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Thanks for fixing :)

Wouldn't it be better to use if(Widelands::tribe_exists(tribename)) rather than triggering the exception?

review: Approve
Revision history for this message
Notabilis (notabilis27) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

I think we should fix all of them. How about sticking them into a new function bool has_players_tribe() in game_settings.h?

9195. By Notabilis

Changing throw...catch code to a separate function call.

Revision history for this message
Notabilis (notabilis27) wrote :

Sounds got to me, the changes are online.
Thanks for the reviews!

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 5391. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/581403115.
Appveyor build 5161. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1842396_no_tribe-5161.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Thanks for fixing :)

@bunnybot merge

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/logic/game_settings.h'
--- src/logic/game_settings.h 2019-04-27 11:21:21 +0000
+++ src/logic/game_settings.h 2019-09-05 20:05:41 +0000
@@ -222,10 +222,13 @@
222 virtual void set_peaceful_mode(bool peace) = 0;222 virtual void set_peaceful_mode(bool peace) = 0;
223 virtual bool is_peaceful_mode() = 0;223 virtual bool is_peaceful_mode() = 0;
224224
225 bool has_players_tribe() {
226 return UserSettings::highest_playernum() >= settings().playernum;
227 }
225 // For retrieving tips texts228 // For retrieving tips texts
226 struct NoTribe {};229 struct NoTribe {};
227 const std::string& get_players_tribe() {230 const std::string& get_players_tribe() {
228 if (UserSettings::highest_playernum() < settings().playernum)231 if (!has_players_tribe())
229 throw NoTribe();232 throw NoTribe();
230 return settings().players[settings().playernum].tribe;233 return settings().players[settings().playernum].tribe;
231 }234 }
232235
=== modified file 'src/network/gameclient.cc'
--- src/network/gameclient.cc 2019-08-28 06:12:07 +0000
+++ src/network/gameclient.cc 2019-09-05 20:05:41 +0000
@@ -149,10 +149,13 @@
149 * Show progress dialog and load map or saved game.149 * Show progress dialog and load map or saved game.
150 */150 */
151InteractiveGameBase* GameClientImpl::init_game(GameClient* parent, UI::ProgressWindow* loader) {151InteractiveGameBase* GameClientImpl::init_game(GameClient* parent, UI::ProgressWindow* loader) {
152152 std::vector<std::string> tipstext;
153 const std::string& tribename = parent->get_players_tribe();153 tipstext.push_back("general_game");
154 assert(Widelands::tribe_exists(tribename));154 tipstext.push_back("multiplayer");
155 GameTips tips(*loader, {"general_game", "multiplayer", tribename});155 if (parent->has_players_tribe()) {
156 tipstext.push_back(parent->get_players_tribe());
157 }
158 GameTips tips(*loader, tipstext);
156159
157 modal = loader;160 modal = loader;
158161
159162
=== modified file 'src/network/gamehost.cc'
--- src/network/gamehost.cc 2019-08-28 06:12:07 +0000
+++ src/network/gamehost.cc 2019-09-05 20:05:41 +0000
@@ -643,9 +643,8 @@
643 std::vector<std::string> tipstext;643 std::vector<std::string> tipstext;
644 tipstext.push_back("general_game");644 tipstext.push_back("general_game");
645 tipstext.push_back("multiplayer");645 tipstext.push_back("multiplayer");
646 try {646 if (d->hp.has_players_tribe()) {
647 tipstext.push_back(d->hp.get_players_tribe());647 tipstext.push_back(d->hp.get_players_tribe());
648 } catch (GameSettingsProvider::NoTribe) {
649 }648 }
650 std::unique_ptr<GameTips> tips(new GameTips(*loader_ui, tipstext));649 std::unique_ptr<GameTips> tips(new GameTips(*loader_ui, tipstext));
651650
652651
=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc 2019-08-28 06:12:07 +0000
+++ src/wlapplication.cc 2019-09-05 20:05:41 +0000
@@ -1339,9 +1339,8 @@
1339 std::vector<std::string> tipstext;1339 std::vector<std::string> tipstext;
1340 tipstext.push_back("general_game");1340 tipstext.push_back("general_game");
1341 tipstext.push_back("singleplayer");1341 tipstext.push_back("singleplayer");
1342 try {1342 if (sp.has_players_tribe()) {
1343 tipstext.push_back(sp.get_players_tribe());1343 tipstext.push_back(sp.get_players_tribe());
1344 } catch (GameSettingsProvider::NoTribe) {
1345 }1344 }
1346 GameTips tips(loader_ui, tipstext);1345 GameTips tips(loader_ui, tipstext);
13471346

Subscribers

People subscribed via source and target branches

to status/vote changes: