← Back to team overview

widelands-dev team mailing list archive

Re: [Merge] lp:~widelands-dev/widelands/bug-1358880-ship-statistics into lp:widelands

 

Review: Needs Fixing

Great work, I really like the new list!

Most of the code looks good, three small bugs and some other comments, though.

The bugs:
- Hotkey 'e' even works on non-seafaring maps
- Missing "ship name" column in the list when there currently are no ships
- Memory leak with create_shipinfo

Two non code related comments (feel free to ignore) :
- I would like a button for opening the ship window without actually moving there. This could, e.g., be useful to start constructing a port while busy elsewhere. Currently both buttons move the view to the ship, maybe have one button for the window and one button to move to the ship?
- I am not really happy with calling the window "ship statistics" (in code and UI). For me, that sounds like a graph showing the number of ships (or so), and not like a list of all ships.

For the other comments, see the diff.

Diff comments:

> === modified file 'data/tribes/scripting/help/controls.lua'
> --- data/tribes/scripting/help/controls.lua	2017-12-06 08:16:46 +0000
> +++ data/tribes/scripting/help/controls.lua	2018-04-16 15:39:30 +0000
> @@ -52,6 +62,8 @@
>                 dl(help_format_hotkey("I"), _"Toggle stock inventory") ..
>                 -- TRANSLATORS: This is an access key combination. The hotkey is 'b'
>                 dl(help_format_hotkey("B"), _"Toggle building statistics") ..
> +               -- TRANSLATORS: This is an access key combination. The hotkey is 'p'

I guess you mean 'e' here?

> +               dl(help_format_hotkey("E"), _"Toggle seafaring statistics") ..
>                 -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
>                 dl(help_format_hotkey(pgettext("hotkey", "Home")), _"Center main mapview on starting location") ..
>                 -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
> @@ -101,5 +103,28 @@
>                 dl(help_format_hotkey("G"), _"Jump to the location corresponding to the current message") ..
>                 -- TRANSLATORS: This is an access key combination. Localize, but do not change the key.
>                 dl(help_format_hotkey(pgettext("hotkey", "Delete")), _"Archive/Restore the current message")
> -          )
> +          ) ..
> +
> +         -- TRANSLATORS: Heading in "Controls" help
> +         h2(_"Ship Statistics") ..
> +         p(
> +               -- TRANSLATORS: This is the helptext for an access key combination.
> +               dl(help_format_hotkey(pgettext("hotkey", "Alt + 0")), _"Show all ships") ..
> +               -- TRANSLATORS: This is the helptext for an access key combination.
> +               dl(help_format_hotkey(pgettext("hotkey", "Alt + 1")), _"Show idle ships") ..
> +               -- TRANSLATORS: This is the helptext for an access key combination.
> +               dl(help_format_hotkey(pgettext("hotkey", "Alt + 2")), _"Show ships shipping wares and workers") ..
> +               -- TRANSLATORS: This is the helptext for an access key combination.
> +               dl(help_format_hotkey(pgettext("hotkey", "Alt + 3")), _"Show waiting expeditions") ..
> +               -- TRANSLATORS: This is the helptext for an access key combination.
> +               dl(help_format_hotkey(pgettext("hotkey", "Alt + 4")), _"Show scouting expeditions") ..
> +               -- TRANSLATORS: This is the helptext for an access key combination.
> +               dl(help_format_hotkey(pgettext("hotkey", "Alt + 5")), _"Show colonizing expeditions and expeditions with port space found") ..

I am not quite sure what "colonizing expeditions" are. Ships currently building ports?

> +               -- TRANSLATORS: This is the helptext for an access key combination.
> +               dl(help_format_hotkey("G"), _"Center the map on the selected ship") ..
> +               -- TRANSLATORS: This is the helptext for an access key combination.
> +               dl(help_format_hotkey("O"), _"Go to the selected ship and open its window") ..
> +               -- TRANSLATORS: This is the helptext for an access key combination.
> +               dl(help_format_hotkey("W"), _"Watch the selected ship")
> +         )
>  }
> 
> === modified file 'src/logic/map_objects/tribes/ship.cc'
> --- src/logic/map_objects/tribes/ship.cc	2018-04-07 16:59:00 +0000
> +++ src/logic/map_objects/tribes/ship.cc	2018-04-16 15:39:30 +0000
> @@ -847,14 +849,14 @@
>  	             pgettext("ship", "Expedition"), _("Expedition Ready"),
>  	             _("An expedition ship is waiting for your commands."),
>  	             "images/wui/buildings/start_expedition.png");
> -	Notifications::publish(NoteShipMessage(this, NoteShipMessage::Message::kWaitingForCommand));
> +	Notifications::publish(NoteShip(this, NoteShip::Action::kWaitingForCommand));

You could use set_ship_state_and_notify() here.

>  }
>  
>  /// Initializes / changes the direction of scouting to @arg direction
>  /// @note only called via player command
>  void Ship::exp_scouting_direction(Game&, WalkingDir scouting_direction) {
>  	assert(expedition_);
> -	ship_state_ = ShipStates::kExpeditionScouting;
> +	set_ship_state_and_notify(ShipStates::kExpeditionScouting, NoteShip::Action::kDestinationChanged);
>  	expedition_->scouting_direction = scouting_direction;
>  	expedition_->island_exploration = false;
>  }
> 
> === modified file 'src/logic/player.cc'
> --- src/logic/player.cc	2018-04-07 16:59:00 +0000
> +++ src/logic/player.cc	2018-04-16 15:39:30 +0000
> @@ -372,6 +373,18 @@
>  	game->cmdqueue().enqueue(new CmdDeleteMessage(game->get_gametime(), player_number_, message_id));
>  }
>  
> +const std::set<Serial>& Player::ships() const {
> +	return ships_;
> +}
> +void Player::add_ship(Serial ship) {
> +	ships_.insert(ship);
> +}
> +void Player::remove_ship(Serial ship) {
> +	if (ships_.count(ship) == 1) {
> +		ships_.erase(ship);
> +	}

Using ships.find() + erase(iter) could be a bit faster. But I don't know for sure and it doesn't matters anyway.

> +}
> +
>  /*
>  ===============
>  Return filtered buildcaps that take the player's territory into account.
> @@ -1303,7 +1316,7 @@
>  		remaining_shipnames_.erase(it);
>  		return new_name;
>  	}
> -	return "Ship";
> +	return (boost::format(pgettext("shipname", "Ship %d")) % ships_.size()).str();

I think this will return strange results after sinking a ship, i.e. having two (or more) with the same number.
Maybe add an explicit counter for the additional ships?

>  }
>  
>  /**
> 
> === modified file 'src/scripting/lua_game.cc'
> --- src/scripting/lua_game.cc	2018-04-07 16:59:00 +0000
> +++ src/scripting/lua_game.cc	2018-04-16 15:39:30 +0000
> @@ -672,30 +672,16 @@
>  */
>  int LuaPlayer::get_ships(lua_State* L) {
>  	EditorGameBase& egbase = get_egbase(L);
> -	const Map& map = egbase.map();
>  	PlayerNumber p = (get(L, egbase)).player_number();
>  	lua_newtable(L);
>  	uint32_t cidx = 1;
> -
> -	std::set<OPtr<Ship>> found_ships;
> -	for (int16_t y = 0; y < map.get_height(); ++y) {
> -		for (int16_t x = 0; x < map.get_width(); ++x) {

Oh dear... Glad I didn't knew about this code. ^^

> -			FCoords f = map.get_fcoords(Coords(x, y));
> -			// there are too many bobs on the map so we investigate
> -			// only bobs on water
> -			if (f.field->nodecaps() & MOVECAPS_SWIM) {
> -				for (Bob* bob = f.field->get_first_bob(); bob; bob = bob->get_next_on_field()) {
> -					if (upcast(Ship, ship, bob)) {
> -						if (ship->get_owner()->player_number() == p && !found_ships.count(ship)) {
> -							found_ships.insert(ship);
> -							lua_pushuint32(L, cidx++);
> -							LuaMaps::upcasted_map_object_to_lua(L, ship);
> -							lua_rawset(L, -3);
> -						}
> -					}
> -				}
> -			}
> -		}
> +	for (const auto& serial : egbase.player(p).ships()) {
> +		Widelands::MapObject* obj = egbase.objects().get_object(serial);
> +		assert(obj->descr().type() == Widelands::MapObjectType::SHIP);
> +		upcast(Widelands::Ship, ship, obj);
> +		lua_pushuint32(L, cidx++);
> +		LuaMaps::upcasted_map_object_to_lua(L, ship);
> +		lua_rawset(L, -3);
>  	}
>  	return 1;
>  }
> 
> === modified file 'src/ui_basic/table.cc'
> --- src/ui_basic/table.cc	2018-04-07 16:59:00 +0000
> +++ src/ui_basic/table.cc	2018-04-16 15:39:30 +0000
> @@ -551,6 +551,21 @@
>  	layout();
>  }
>  
> +/**
> + * Remove the given table entry if it exists.
> + */
> +void Table<void*>::remove_entry(const void* const entry) {
> +	EntryRecord* er = find(entry);
> +	if (er != nullptr) {
> +		for (uint32_t i = 0; i < entry_records_.size(); ++i) {

find() is iterating over the table entries as well. Skip the find() call and iterate immediately?
Not sure whether that is really more efficient on average since I don't know about possible speed differences between for-each and for(int).

> +			if (entry_records_[i] == er) {
> +				remove(i);
> +				return;
> +			}
> +		}
> +	}
> +}
> +
>  bool Table<void*>::sort_helper(uint32_t a, uint32_t b) {
>  	if (sort_descending_) {
>  		return columns_[sort_column_].compare(b, a);
> 
> === modified file 'src/wui/interactive_player.cc'
> --- src/wui/interactive_player.cc	2018-04-07 16:59:00 +0000
> +++ src/wui/interactive_player.cc	2018-04-16 15:39:30 +0000
> @@ -459,6 +460,14 @@
>  			}
>  			return true;
>  
> +		case SDLK_e:

This should only do something on seafaring maps but works on all maps.

> +			if (main_windows_.seafaring_stats.window == nullptr) {
> +				new SeafaringStatisticsMenu(*this, main_windows_.seafaring_stats);
> +			} else {
> +				main_windows_.seafaring_stats.toggle();
> +			}
> +			return true;
> +
>  		case SDLK_s:
>  			if (code.mod & (KMOD_LCTRL | KMOD_RCTRL))
>  				new GameMainMenuSaveGame(*this, main_windows_.savegame);
> 
> === added file 'src/wui/seafaring_statistics_menu.cc'
> --- src/wui/seafaring_statistics_menu.cc	1970-01-01 00:00:00 +0000
> +++ src/wui/seafaring_statistics_menu.cc	2018-04-16 15:39:30 +0000
> @@ -0,0 +1,568 @@
> +/*
> + * Copyright (C) 2017-2018 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + */
> +
> +#include "wui/seafaring_statistics_menu.h"
> +
> +#include <memory>
> +
> +#include <boost/bind.hpp>
> +#include <boost/format.hpp>
> +
> +#include "economy/fleet.h"
> +#include "graphic/graphic.h"
> +#include "logic/game.h"
> +#include "logic/player.h"
> +#include "logic/playercommand.h"
> +#include "ui_basic/box.h"
> +#include "wui/interactive_player.h"
> +#include "wui/shipwindow.h"
> +#include "wui/watchwindow.h"
> +
> +inline InteractivePlayer& SeafaringStatisticsMenu::iplayer() const {
> +	return dynamic_cast<InteractivePlayer&>(*get_parent());
> +}
> +
> +constexpr int kPadding = 5;
> +constexpr int kButtonSize = 34;
> +
> +SeafaringStatisticsMenu::SeafaringStatisticsMenu(InteractivePlayer& plr,
> +                                                 UI::UniqueWindow::Registry& registry)
> +   : UI::UniqueWindow(&plr, "seafaring_statistics", &registry, 355, 375, _("Seafaring Statistics")),
> +     main_box_(this, kPadding, kPadding, UI::Box::Vertical, get_inner_w(), get_inner_h(), kPadding),
> +     filter_box_(
> +        &main_box_, 0, 0, UI::Box::Horizontal, get_inner_w() - 2 * kPadding, kButtonSize, kPadding),
> +     idle_btn_(&filter_box_,
> +               "filter_ship_idle",
> +               0,
> +               0,
> +               kButtonSize,
> +               kButtonSize,
> +               g_gr->images().get("images/ui_basic/but0.png"),
> +               status_to_image(ShipFilterStatus::kIdle)),
> +     waiting_btn_(&filter_box_,
> +                  "filter_ship_waiting",
> +                  0,
> +                  0,
> +                  kButtonSize,
> +                  kButtonSize,
> +                  g_gr->images().get("images/ui_basic/but0.png"),
> +                  status_to_image(ShipFilterStatus::kExpeditionWaiting)),
> +     scouting_btn_(&filter_box_,
> +                   "filter_ship_scouting",
> +                   0,
> +                   0,
> +                   kButtonSize,
> +                   kButtonSize,
> +                   g_gr->images().get("images/ui_basic/but0.png"),
> +                   status_to_image(ShipFilterStatus::kExpeditionScouting)),
> +     portspace_btn_(&filter_box_,
> +                    "filter_ship_portspace",
> +                    0,
> +                    0,
> +                    kButtonSize,
> +                    kButtonSize,
> +                    g_gr->images().get("images/ui_basic/but0.png"),
> +                    status_to_image(ShipFilterStatus::kExpeditionPortspaceFound)),
> +     shipping_btn_(&filter_box_,
> +                   "filter_ship_transporting",
> +                   0,
> +                   0,
> +                   kButtonSize,
> +                   kButtonSize,
> +                   g_gr->images().get("images/ui_basic/but0.png"),
> +                   status_to_image(ShipFilterStatus::kShipping)),
> +     ship_filter_(ShipFilterStatus::kAll),
> +     navigation_box_(
> +        &main_box_, 0, 0, UI::Box::Horizontal, get_inner_w() - 2 * kPadding, kButtonSize, kPadding),
> +     watchbtn_(&navigation_box_,
> +               "seafaring_stats_watch_button",
> +               0,
> +               0,
> +               kButtonSize,
> +               kButtonSize,
> +               g_gr->images().get("images/ui_basic/but2.png"),
> +               g_gr->images().get("images/wui/menus/menu_watch_follow.png"),
> +               (boost::format(_("%1% (Hotkey: %2%)")) %
> +                /** TRANSLATORS: Tooltip in the seafaring statistics window */
> +                _("Watch the selected ship") %
> +                pgettext("hotkey", "W"))
> +                  .str()),
> +     openwindowbtn_(&navigation_box_,
> +                    "seafaring_stats_watch_button",
> +                    0,
> +                    0,
> +                    kButtonSize,
> +                    kButtonSize,
> +                    g_gr->images().get("images/ui_basic/but2.png"),
> +                    g_gr->images().get("images/ui_basic/fsel.png"),
> +                    (boost::format(_("%1% (Hotkey: %2%)")) %
> +                     /** TRANSLATORS: Tooltip in the seafaring statistics window */
> +                     _("Go to the selected ship and open its window") %
> +                     pgettext("hotkey", "O"))
> +                       .str()),
> +     centerviewbtn_(&navigation_box_,
> +                    "seafaring_stats_center_main_mapview_button",
> +                    0,
> +                    0,
> +                    kButtonSize,
> +                    kButtonSize,
> +                    g_gr->images().get("images/ui_basic/but2.png"),
> +                    g_gr->images().get("images/wui/ship/menu_ship_goto.png"),
> +                    (boost::format(_("%1% (Hotkey: %2%)")) %
> +                     /** TRANSLATORS: Tooltip in the seafaring statistics window */
> +                     _("Center the map on the selected ship") %
> +                     pgettext("hotkey", "G"))
> +                       .str()),
> +     table_(&main_box_,
> +            0,
> +            0,
> +            get_inner_w() - 2 * kPadding,
> +            100,
> +            g_gr->images().get("images/ui_basic/but1.png")) {
> +
> +	const Widelands::TribeDescr& tribe = iplayer().player().tribe();
> +	colony_icon_ = tribe.get_worker_descr(tribe.builder())->icon();
> +
> +	// Buttons for ship states
> +	main_box_.add(&filter_box_, UI::Box::Resizing::kFullSize);
> +	filter_box_.add(&idle_btn_);
> +	filter_box_.add(&shipping_btn_);
> +	filter_box_.add(&waiting_btn_);
> +	filter_box_.add(&scouting_btn_);
> +	filter_box_.add(&portspace_btn_);
> +
> +	main_box_.add(&table_, UI::Box::Resizing::kExpandBoth);
> +
> +	// Navigation buttons
> +	main_box_.add(&navigation_box_, UI::Box::Resizing::kFullSize);
> +	navigation_box_.add(&watchbtn_);
> +	navigation_box_.add_inf_space();
> +	navigation_box_.add(&openwindowbtn_);
> +	navigation_box_.add(&centerviewbtn_);
> +	main_box_.set_size(get_inner_w() - 2 * kPadding, get_inner_h() - 2 * kPadding);
> +
> +	// Configure actions
> +	idle_btn_.sigclicked.connect(
> +	   boost::bind(&SeafaringStatisticsMenu::filter_ships, this, ShipFilterStatus::kIdle));
> +	shipping_btn_.sigclicked.connect(
> +	   boost::bind(&SeafaringStatisticsMenu::filter_ships, this, ShipFilterStatus::kShipping));
> +	waiting_btn_.sigclicked.connect(boost::bind(
> +	   &SeafaringStatisticsMenu::filter_ships, this, ShipFilterStatus::kExpeditionWaiting));
> +	scouting_btn_.sigclicked.connect(boost::bind(
> +	   &SeafaringStatisticsMenu::filter_ships, this, ShipFilterStatus::kExpeditionScouting));
> +	portspace_btn_.sigclicked.connect(boost::bind(
> +	   &SeafaringStatisticsMenu::filter_ships, this, ShipFilterStatus::kExpeditionPortspaceFound));
> +	ship_filter_ = ShipFilterStatus::kAll;
> +	set_filter_ships_tooltips();
> +
> +	watchbtn_.sigclicked.connect(boost::bind(&SeafaringStatisticsMenu::watch_ship, this));
> +	openwindowbtn_.sigclicked.connect(boost::bind(&SeafaringStatisticsMenu::open_ship_window, this));
> +	centerviewbtn_.sigclicked.connect(boost::bind(&SeafaringStatisticsMenu::center_view, this));
> +
> +	// Configure table
> +	table_.selected.connect(boost::bind(&SeafaringStatisticsMenu::selected, this));
> +	table_.double_clicked.connect(boost::bind(&SeafaringStatisticsMenu::double_clicked, this));
> +	table_.add_column(
> +	   0, pgettext("ship", "Name"), "", UI::Align::kLeft, UI::TableColumnType::kFlexible);
> +	table_.add_column(200, pgettext("ship", "Status"));
> +	table_.set_sort_column(ColName);
> +	fill_table();
> +
> +	set_can_focus(true);
> +	set_thinks(false);
> +	table_.focus();
> +
> +	shipnotes_subscriber_ =
> +	   Notifications::subscribe<Widelands::NoteShip>([this](const Widelands::NoteShip& note) {
> +		   if (iplayer().get_player() == note.ship->get_owner()) {
> +			   switch (note.action) {
> +			   case Widelands::NoteShip::Action::kDestinationChanged:
> +			   case Widelands::NoteShip::Action::kWaitingForCommand:
> +			   case Widelands::NoteShip::Action::kGained:
> +				   update_ship(*note.ship);
> +				   break;
> +			   case Widelands::NoteShip::Action::kLost:
> +				   remove_ship(note.ship->serial());
> +				   break;
> +			   default:
> +				   NEVER_HERE();
> +			   }
> +		   }
> +		});
> +}
> +
> +const std::string
> +SeafaringStatisticsMenu::status_to_string(SeafaringStatisticsMenu::ShipFilterStatus status) const {
> +	switch (status) {
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kIdle:
> +		return pgettext("ship_state", "Idle");
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kShipping:
> +		return pgettext("ship_state", "Shipping");
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kExpeditionWaiting:
> +		return pgettext("ship_state", "Waiting");
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kExpeditionScouting:
> +		return pgettext("ship_state", "Scouting");
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kExpeditionPortspaceFound:
> +		return pgettext("ship_state", "Port Space Found");
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kExpeditionColonizing:
> +		return pgettext("ship_state", "Founding a Colony");
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kAll:
> +		return "All";  // The user shouldn't see this, so we don't localize
> +	default:
> +		NEVER_HERE();
> +	}
> +}
> +
> +const Image*
> +SeafaringStatisticsMenu::status_to_image(SeafaringStatisticsMenu::ShipFilterStatus status) const {
> +	std::string filename = "";
> +	switch (status) {
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kIdle:
> +		filename = "images/wui/stats/ship_stats_idle.png";
> +		break;
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kShipping:
> +		filename = "images/wui/stats/ship_stats_shipping.png";
> +		break;
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kExpeditionWaiting:
> +		filename = "images/wui/buildings/start_expedition.png";
> +		break;
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kExpeditionScouting:
> +		filename = "images/wui/ship/ship_explore_island_cw.png";
> +		break;
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kExpeditionPortspaceFound:
> +		filename = "images/wui/ship/ship_construct_port_space.png";
> +		break;
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kExpeditionColonizing:
> +		return colony_icon_;
> +	case SeafaringStatisticsMenu::ShipFilterStatus::kAll:
> +		filename = "images/wui/ship/ship_scout_ne.png";
> +		break;
> +	default:
> +		NEVER_HERE();
> +	}
> +	return g_gr->images().get(filename);
> +}
> +
> +const SeafaringStatisticsMenu::ShipInfo*
> +SeafaringStatisticsMenu::create_shipinfo(const Widelands::Ship& ship) const {
> +	if (&ship == nullptr) {
> +		return new ShipInfo();
> +	}
> +	const Widelands::Ship::ShipStates state = ship.get_ship_state();
> +	ShipFilterStatus status = ShipFilterStatus::kAll;
> +	switch (state) {
> +	case Widelands::Ship::ShipStates::kTransport:
> +		if (ship.get_destination(iplayer().game()) != nullptr) {
> +			status = ShipFilterStatus::kShipping;
> +		} else {
> +			status = ShipFilterStatus::kIdle;
> +		}
> +		break;
> +	case Widelands::Ship::ShipStates::kExpeditionWaiting:
> +		status = ShipFilterStatus::kExpeditionWaiting;
> +		break;
> +	case Widelands::Ship::ShipStates::kExpeditionScouting:
> +		status = ShipFilterStatus::kExpeditionScouting;
> +		break;
> +	case Widelands::Ship::ShipStates::kExpeditionPortspaceFound:
> +		status = ShipFilterStatus::kExpeditionPortspaceFound;
> +		break;
> +	case Widelands::Ship::ShipStates::kExpeditionColonizing:
> +		status = ShipFilterStatus::kExpeditionColonizing;
> +		break;
> +	case Widelands::Ship::ShipStates::kSinkRequest:
> +	case Widelands::Ship::ShipStates::kSinkAnimation:
> +		status = ShipFilterStatus::kAll;
> +	}
> +	return new ShipInfo(ship.get_shipname(), status, ship.serial());
> +}
> +
> +void SeafaringStatisticsMenu::set_entry_record(UI::Table<uintptr_t>::EntryRecord* er,
> +                                               const ShipInfo& info) {
> +	if (info.status != ShipFilterStatus::kAll) {
> +		er->set_string(ColName, info.name);
> +		er->set_picture(ColStatus, status_to_image(info.status), status_to_string(info.status));
> +	}
> +}
> +
> +Widelands::Ship* SeafaringStatisticsMenu::serial_to_ship(Widelands::Serial serial) const {
> +	Widelands::MapObject* obj = iplayer().game().objects().get_object(serial);
> +	assert(obj->descr().type() == Widelands::MapObjectType::SHIP);
> +	upcast(Widelands::Ship, ship, obj);
> +	return ship;
> +}
> +
> +void SeafaringStatisticsMenu::update_ship(const Widelands::Ship& ship) {
> +	assert(iplayer().get_player() == ship.get_owner());
> +	const ShipInfo* info = create_shipinfo(ship);
> +	// Remove ships that don't satisfy the filter
> +	if (ship_filter_ != ShipFilterStatus::kAll && !satisfies_filter(*info, ship_filter_)) {
> +		remove_ship(info->serial);
> +		return;
> +	}
> +	// Try to find the ship in the table
> +	if (data_.count(info->serial) == 1) {
> +		const ShipInfo* old_info = data_[info->serial].get();
> +		if (info->status != old_info->status) {
> +			// The status has changed - we need an update
> +			data_[info->serial] = std::unique_ptr<const ShipInfo>(info);
> +			UI::Table<uintptr_t>::EntryRecord* er = table_.find(info->serial);
> +			set_entry_record(er, *info);
> +		}
> +	} else {
> +		// This is a new ship or it was filtered away before
> +		data_.insert(std::make_pair(info->serial, std::unique_ptr<const ShipInfo>(info)));
> +		UI::Table<uintptr_t>::EntryRecord& er = table_.add(info->serial);
> +		set_entry_record(&er, *info);
> +	}
> +	table_.sort();
> +	set_buttons_enabled();
> +}
> +
> +void SeafaringStatisticsMenu::remove_ship(Widelands::Serial serial) {
> +	if (data_.count(serial) == 1) {
> +		table_.remove_entry(serial);
> +		data_.erase(data_.find(serial));
> +		if (!table_.empty() && !table_.has_selection()) {
> +			table_.select(0);
> +		}
> +		set_buttons_enabled();
> +	}
> +}
> +
> +void SeafaringStatisticsMenu::update_entry_record(UI::Table<uintptr_t>::EntryRecord& er,
> +                                                  const ShipInfo& info) {
> +	er.set_picture(ColStatus, status_to_image(info.status), status_to_string(info.status));
> +}
> +
> +void SeafaringStatisticsMenu::selected() {
> +	set_buttons_enabled();
> +}
> +
> +void SeafaringStatisticsMenu::double_clicked() {
> +	if (table_.has_selection()) {
> +		center_view();
> +	}
> +}
> +
> +void SeafaringStatisticsMenu::set_buttons_enabled() {

Maybe call the method update_buttons_state() ? The current name sounds like they will always be set to enabled.

> +	centerviewbtn_.set_enabled(table_.has_selection());
> +	openwindowbtn_.set_enabled(table_.has_selection());
> +	watchbtn_.set_enabled(table_.has_selection());
> +}
> +
> +bool SeafaringStatisticsMenu::handle_key(bool down, SDL_Keysym code) {
> +	if (down) {
> +		switch (code.sym) {
> +		// Don't forget to change the tooltips if any of these get reassigned
> +		case SDLK_g:
> +			center_view();
> +			return true;
> +		case SDLK_o:
> +			open_ship_window();
> +			return true;
> +		case SDLK_w:
> +			watch_ship();
> +			return true;
> +		case SDLK_0:
> +			if (code.mod & KMOD_ALT) {
> +				filter_ships(ShipFilterStatus::kAll);
> +				return true;
> +			}
> +			return false;
> +		case SDLK_1:
> +			if (code.mod & KMOD_ALT) {
> +				filter_ships(ShipFilterStatus::kIdle);
> +				return true;
> +			}
> +			return false;
> +		case SDLK_2:
> +			if (code.mod & KMOD_ALT) {
> +				filter_ships(ShipFilterStatus::kShipping);
> +				return true;
> +			}
> +			return false;
> +		case SDLK_3:
> +			if (code.mod & KMOD_ALT) {
> +				filter_ships(ShipFilterStatus::kExpeditionWaiting);
> +				return true;
> +			}
> +			return false;
> +		case SDLK_4:
> +			if (code.mod & KMOD_ALT) {
> +				filter_ships(ShipFilterStatus::kExpeditionScouting);
> +				return true;
> +			}
> +			return false;
> +		case SDLK_5:
> +			if (code.mod & KMOD_ALT) {
> +				filter_ships(ShipFilterStatus::kExpeditionPortspaceFound);
> +				return true;
> +			}
> +			return false;
> +		case SDL_SCANCODE_KP_PERIOD:
> +		case SDLK_KP_PERIOD:
> +			if (code.mod & KMOD_NUM)
> +				break;
> +		/* no break */
> +		default:
> +			break;  // not handled
> +		}
> +	}
> +
> +	return table_.handle_key(down, code);
> +}
> +
> +void SeafaringStatisticsMenu::center_view() {
> +	if (table_.has_selection()) {
> +		Widelands::Ship* ship = serial_to_ship(table_.get_selected());
> +		iplayer().map_view()->scroll_to_field(ship->get_position(), MapView::Transition::Smooth);
> +	}
> +}
> +
> +void SeafaringStatisticsMenu::watch_ship() {
> +	if (table_.has_selection()) {
> +		Widelands::Ship* ship = serial_to_ship(table_.get_selected());
> +		WatchWindow* window = show_watch_window(iplayer(), ship->get_position());
> +		window->follow(ship);
> +	}
> +}
> +
> +void SeafaringStatisticsMenu::open_ship_window() {
> +	if (table_.has_selection()) {
> +		center_view();
> +		Widelands::Ship* ship = serial_to_ship(table_.get_selected());
> +		iplayer().show_ship_window(ship);
> +	}
> +}
> +
> +void SeafaringStatisticsMenu::filter_ships(ShipFilterStatus status) {
> +	switch (status) {
> +	case ShipFilterStatus::kExpeditionWaiting:
> +		toggle_filter_ships_button(waiting_btn_, status);
> +		break;
> +	case ShipFilterStatus::kExpeditionScouting:
> +		toggle_filter_ships_button(scouting_btn_, status);
> +		break;
> +	// We're grouping the "colonizing" status with the port space.
> +	case ShipFilterStatus::kExpeditionColonizing:
> +	case ShipFilterStatus::kExpeditionPortspaceFound:
> +		toggle_filter_ships_button(portspace_btn_, status);
> +		break;
> +	case ShipFilterStatus::kShipping:
> +		toggle_filter_ships_button(shipping_btn_, status);
> +		break;
> +	case ShipFilterStatus::kIdle:
> +		toggle_filter_ships_button(idle_btn_, status);
> +		break;
> +	case ShipFilterStatus::kAll:
> +		set_filter_ships_tooltips();
> +		ship_filter_ = ShipFilterStatus::kAll;
> +		waiting_btn_.set_perm_pressed(false);
> +		scouting_btn_.set_perm_pressed(false);
> +		portspace_btn_.set_perm_pressed(false);
> +		shipping_btn_.set_perm_pressed(false);
> +		idle_btn_.set_perm_pressed(false);
> +		break;
> +	}
> +	fill_table();
> +}
> +
> +void SeafaringStatisticsMenu::toggle_filter_ships_button(UI::Button& button,
> +                                                         ShipFilterStatus status) {
> +	set_filter_ships_tooltips();
> +	if (button.style() == UI::Button::Style::kPermpressed) {
> +		button.set_perm_pressed(false);
> +		ship_filter_ = ShipFilterStatus::kAll;
> +	} else {
> +		waiting_btn_.set_perm_pressed(false);
> +		scouting_btn_.set_perm_pressed(false);
> +		portspace_btn_.set_perm_pressed(false);
> +		shipping_btn_.set_perm_pressed(false);
> +		idle_btn_.set_perm_pressed(false);
> +		button.set_perm_pressed(true);
> +		ship_filter_ = status;
> +
> +		/** TRANSLATORS: %1% is a tooltip, %2% is the corresponding hotkey */
> +		button.set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
> +		                    /** TRANSLATORS: Tooltip in the messages window */
> +		                    % _("Show all ships") % pgettext("hotkey", "Alt + 0"))
> +		                      .str());
> +	}
> +}
> +
> +void SeafaringStatisticsMenu::set_filter_ships_tooltips() {
> +
> +	idle_btn_.set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
> +	                       /** TRANSLATORS: Tooltip in the messages window */
> +	                       % _("Show idle ships") % pgettext("hotkey", "Alt + 1"))
> +	                         .str());
> +	shipping_btn_.set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
> +	                           /** TRANSLATORS: Tooltip in the messages window */
> +	                           % _("Show ships shipping wares and workers") %
> +	                           pgettext("hotkey", "Alt + 2"))
> +	                             .str());
> +	waiting_btn_.set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
> +	                          /** TRANSLATORS: Tooltip in the messages window */
> +	                          % _("Show waiting expeditions") % pgettext("hotkey", "Alt + 3"))
> +	                            .str());
> +	scouting_btn_.set_tooltip((boost::format(_("%1% (Hotkey: %2%)"))
> +	                           /** TRANSLATORS: Tooltip in the messages window */
> +	                           % _("Show scouting expeditions") % pgettext("hotkey", "Alt + 4"))
> +	                             .str());
> +	portspace_btn_.set_tooltip(
> +	   (boost::format(_("%1% (Hotkey: %2%)"))
> +	    /** TRANSLATORS: Tooltip in the messages window */
> +	    % _("Show colonizing expeditions and expeditions with port space found") %
> +	    pgettext("hotkey", "Alt + 5"))
> +	      .str());
> +}
> +
> +bool SeafaringStatisticsMenu::satisfies_filter(const ShipInfo& info, ShipFilterStatus filter) {
> +	return filter == info.status || (filter == ShipFilterStatus::kExpeditionPortspaceFound &&
> +	                                 info.status == ShipFilterStatus::kExpeditionColonizing);
> +}
> +
> +void SeafaringStatisticsMenu::fill_table() {

I don't really understand this method. I think set_buttons_enabled() should disable all buttons. I intended to complain that they aren't re-enabled when something is selected later on but that seems to be done somehow. Well, everything fine then, I guess.
Ah, some "hidden" callback by using table_.select(0) maybe? Possibly make it explicit by moving set_buttons_enabled() to the end of the method.

> +	const Widelands::Serial last_selection =
> +	   table_.has_selection() ? table_.get_selected() : Widelands::INVALID_INDEX;
> +	table_.clear();
> +	data_.clear();
> +	set_buttons_enabled();
> +	for (const auto& serial : iplayer().player().ships()) {
> +		Widelands::Ship* ship = serial_to_ship(serial);
> +		assert(iplayer().get_player() == ship->get_owner());
> +		const ShipInfo* info = create_shipinfo(*ship);
> +		if (info->status != ShipFilterStatus::kAll) {
> +			if (ship_filter_ == ShipFilterStatus::kAll || satisfies_filter(*info, ship_filter_)) {
> +				data_.insert(std::make_pair(serial, std::unique_ptr<const ShipInfo>(info)));
> +				UI::Table<uintptr_t const>::EntryRecord& er =
> +				   table_.add(serial, serial == last_selection);
> +				set_entry_record(&er, *info);
> +			}
> +		}
> +	}
> +
> +	if (!table_.empty()) {
> +		table_.sort();
> +		if (!table_.has_selection()) {
> +			table_.select(0);
> +		}
> +	}
> +}
> 
> === added file 'src/wui/seafaring_statistics_menu.h'
> --- src/wui/seafaring_statistics_menu.h	1970-01-01 00:00:00 +0000
> +++ src/wui/seafaring_statistics_menu.h	2018-04-16 15:39:30 +0000
> @@ -0,0 +1,159 @@
> +/*
> + * Copyright (C) 2017-2018 by the Widelands Development Team
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + */
> +
> +#ifndef WL_WUI_SEAFARING_STATISTICS_MENU_H
> +#define WL_WUI_SEAFARING_STATISTICS_MENU_H
> +
> +#include <memory>
> +#include <unordered_map>
> +
> +#include "base/i18n.h"
> +#include "logic/map_objects/tribes/ship.h"
> +#include "notifications/notifications.h"
> +#include "ui_basic/box.h"
> +#include "ui_basic/button.h"
> +#include "ui_basic/table.h"
> +#include "ui_basic/unique_window.h"
> +
> +class InteractivePlayer;
> +
> +/// Shows a list of the ships owned by the interactive player with filtering and navigation options.
> +struct SeafaringStatisticsMenu : public UI::UniqueWindow {
> +	SeafaringStatisticsMenu(InteractivePlayer&, UI::UniqueWindow::Registry&);
> +
> +private:
> +	/// For identifying the columns in the table.
> +	enum Cols { ColName, ColStatus };
> +	/**
> +	 * A list of ship status that we can filter for. This differs a bit from that the Widelands::Ship

"from that" or "from what" ? Okay, probably you will be right... ;)

> +	 * class has, so we define our own.
> +	 * */
> +	enum class ShipFilterStatus {
> +		kIdle,
> +		kShipping,
> +		kExpeditionWaiting,
> +		kExpeditionScouting,
> +		kExpeditionPortspaceFound,
> +		kExpeditionColonizing,
> +		kAll
> +	};
> +
> +	/// Returns the localized strings that we use to display the 'status' in the table.
> +	const std::string status_to_string(ShipFilterStatus status) const;
> +	/// Returns the icon that we use to represent the 'status' in the table and on the filter
> +	/// buttons.
> +	const Image* status_to_image(ShipFilterStatus status) const;
> +
> +	/// The dataset that we need to display ships in the table.
> +	struct ShipInfo {
> +		ShipInfo(const std::string& init_name,
> +		         const ShipFilterStatus init_status,
> +		         const Widelands::Serial init_serial)
> +		   : name(init_name), status(init_status), serial(init_serial) {
> +		}
> +		ShipInfo() : ShipInfo("", ShipFilterStatus::kAll, 0) {
> +		}
> +		ShipInfo(const ShipInfo& other) : ShipInfo(other.name, other.status, other.serial) {
> +		}
> +		bool operator==(const ShipInfo& other) const {
> +			return serial == other.serial;
> +		}
> +		bool operator<(const ShipInfo& other) const {
> +			return serial < other.serial;
> +		}
> +
> +		const std::string name;
> +		ShipFilterStatus status;
> +		const Widelands::Serial serial;
> +	};
> +
> +	/// Creates our dataset from a 'ship'. Make sure to take ownership of the result.

I might miss something, but it seems you are leaking the memory on both calls of this method. Return an object (its small after all) or a smart pointer?
Edit: Okay, ASAN is with me on this.

> +	const ShipInfo* create_shipinfo(const Widelands::Ship& ship) const;
> +	/// Uses the 'serial' to identify and get a ship from the game.
> +	Widelands::Ship* serial_to_ship(Widelands::Serial serial) const;
> +
> +	/// Convenience function to upcast the panel's parent.
> +	InteractivePlayer& iplayer() const;
> +
> +	/// A ship was selected, so enable the navigation buttons.
> +	void selected();
> +	/// A ship was double clicked. Centers main view on ship.
> +	void double_clicked();
> +	/// Handle filter and navigation hotkeys
> +	bool handle_key(bool down, SDL_Keysym code) override;
> +
> +	/// Enables the navigation buttons if a ship is selected, disables them otherwise.
> +	void set_buttons_enabled();
> +	/// Center the mapview on the currently selected ship.
> +	void center_view();
> +	/// Follow the selected ship in a watch window.
> +	void watch_ship();
> +	/// Center the mapview on the currently selected ship and open its window.
> +	void open_ship_window();
> +
> +	/**
> +	 * Updates the status for the ship. If the ship is new and satisfies the 'ship_filter_',
> +	 * adds it to the table and data. If it doesn't satisfy the 'ship_filter_', removes the ship
> +	 * instead.
> +	 * */
> +	void update_ship(const Widelands::Ship&);
> +	/// If we listed the ship, remove it from table and data.
> +	void remove_ship(Widelands::Serial serial);
> +	/// Sets the contents for the entry record in the table.
> +	void set_entry_record(UI::Table<uintptr_t>::EntryRecord*, const ShipInfo& info);
> +	/// Updates the ship status display in the table.
> +	void update_entry_record(UI::Table<uintptr_t>::EntryRecord& er, const ShipInfo&);
> +	/// Rebuilds data and table with all ships that satisfy the current 'ship_filter_'.
> +	void fill_table();
> +
> +	/// Show only the ships that have the given status. Toggle the appropriate buttons.
> +	void filter_ships(ShipFilterStatus status);
> +	/// Helper for filter_ships
> +	void toggle_filter_ships_button(UI::Button&, ShipFilterStatus);
> +	/// Helper for filter_ships
> +	void set_filter_ships_tooltips();
> +
> +	/// We group colonizing status with port space found. Anything else needs to have an identical
> +	/// status.
> +	bool satisfies_filter(const ShipInfo& info, ShipFilterStatus filter);
> +
> +	const Image* colony_icon_;
> +	UI::Box main_box_;
> +	// Buttons for ship states
> +	UI::Box filter_box_;
> +	UI::Button idle_btn_;
> +	UI::Button waiting_btn_;
> +	UI::Button scouting_btn_;
> +	UI::Button portspace_btn_;
> +	UI::Button shipping_btn_;
> +	ShipFilterStatus ship_filter_;
> +	// Navigation buttons
> +	UI::Box navigation_box_;
> +	UI::Button watchbtn_;
> +	UI::Button openwindowbtn_;
> +	UI::Button centerviewbtn_;
> +
> +	// Data
> +	UI::Table<uintptr_t> table_;
> +	std::unordered_map<Widelands::Serial, std::unique_ptr<const ShipInfo>> data_;
> +
> +	std::unique_ptr<Notifications::Subscriber<Widelands::NoteShip>> shipnotes_subscriber_;
> +};
> +
> +#endif  // end of include guard: WL_WUI_SEAFARING_STATISTICS_MENU_H
> 
> === modified file 'src/wui/shipwindow.cc'
> --- src/wui/shipwindow.cc	2018-04-07 16:59:00 +0000
> +++ src/wui/shipwindow.cc	2018-04-16 15:39:30 +0000
> @@ -279,12 +301,20 @@
>  
>  /// Move the main view towards the current ship location
>  void ShipWindow::act_goto() {
> -	igbase_.map_view()->scroll_to_field(ship_.get_position(), MapView::Transition::Smooth);

Hm, I have seen this code before... Guess the "branch dependency" works just fine. -_-

> +	Widelands::Ship* ship = ship_.get(igbase_.egbase());
> +	if (ship == nullptr) {
> +		return;
> +	}
> +	igbase_.map_view()->scroll_to_field(ship->get_position(), MapView::Transition::Smooth);
>  }
>  
>  /// Move the main view towards the current destination of the ship
>  void ShipWindow::act_destination() {
> -	if (PortDock* destination = ship_.get_destination(igbase_.egbase())) {
> +	Widelands::Ship* ship = ship_.get(igbase_.egbase());
> +	if (ship == nullptr) {
> +		return;
> +	}
> +	if (PortDock* destination = ship->get_destination(igbase_.egbase())) {
>  		igbase_.map_view()->scroll_to_field(
>  		   destination->get_warehouse()->get_position(), MapView::Transition::Smooth);
>  	}


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1358880-ship-statistics/+merge/343293
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ships_optr.


References