Review: Approve
Testing worked fine, one comment in the diff.
As a suggestion: What about moving spectators to the first player position on
new games, too?
Diff comments:
>
> === modified file 'src/wui/interactive_gamebase.cc'
> --- src/wui/interactive_gamebase.cc 2018-04-07 16:59:00 +0000
> +++ src/wui/interactive_gamebase.cc 2018-04-16 07:21:13 +0000
> @@ -148,6 +148,27 @@
> hide_minimap();
> }
>
> +void InteractiveGameBase::start() {
> + InteractiveBase::start();
This call is new (as in: hasn't been done in start() before), right? Seems to
just call the empty Panel:start().
Is there a reason that this is called? The previous empty override was making
explicit that it isn't.
> + // Multiplayer games don't save the view position, so we go to the
> starting position instead
> + if (is_multiplayer()) {
> + Widelands::PlayerNumber pln = player_number();
> + const Widelands::PlayerNumber max =
> game().map().get_nrplayers();
> + if (pln == 0) {
> + // Spectator, use the view of the first viable player
> + for (pln = 1; pln <= max; ++pln) {
> + if (game().get_player(pln)) {
> + break;
> + }
> + }
> + }
> + // Adding a check, just in case there was no viable player
> found for spectator
> + if (game().get_player(pln)) {
> +
> map_view()->scroll_to_field(game().map().get_starting_pos(pln),
> MapView::Transition::Jump);
> + }
> + }
> +}
> +
> void InteractiveGameBase::on_buildhelp_changed(const bool value) {
> toggle_buildhelp_->set_perm_pressed(value);
> }
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1623375-multiplayer-starting-view/+merge/343287
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1623375-multiplayer-starting-view.
_______________________________________________
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
