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", ®istry, 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(¢erviewbtn_);
> + 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.
_______________________________________________
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
