Thanks for the review :)

And you were right about the efficiency concerns, the seafaring check is 
causing a slowdown. I have reduced it to be checked once every 2 minutes.

Diff comments:

> 
> === modified file 'src/ui_basic/tabpanel.cc'
> --- src/ui_basic/tabpanel.cc  2017-08-08 17:39:40 +0000
> +++ src/ui_basic/tabpanel.cc  2018-04-07 17:36:19 +0000
> @@ -225,6 +225,9 @@
>  }
>  
>  bool TabPanel::remove_last_tab(const std::string& tabname) {
> +     if (tabs_.empty()) {
> +             return false;
> +     }

This crashes when you try to remove a tab when there are no tabs. It happened 
to me before I moved the tab deletion from init() to reset()

>       if (tabs_.back()->get_name() == tabname) {
>               tabs_.pop_back();
>               if (active_ > tabs_.size() - 1) {
> 
> === modified file 'src/wui/building_statistics_menu.cc'
> --- src/wui/building_statistics_menu.cc       2017-12-16 10:48:12 +0000
> +++ src/wui/building_statistics_menu.cc       2018-04-07 17:36:19 +0000
> @@ -315,6 +206,189 @@
>       productivity_labels_.clear();
>  }
>  
> +void BuildingStatisticsMenu::reset() {
> +     update();  // In case a building got removed, make sure to deselect it 
> first
> +
> +     const int last_selected_tab = tab_assignments_[tab_panel_.active()];
> +
> +     tab_panel_.remove_last_tab("building_stats_ports");
> +     tab_panel_.remove_last_tab("building_stats_mines");
> +     tab_panel_.remove_last_tab("building_stats_big");
> +     tab_panel_.remove_last_tab("building_stats_medium");
> +     tab_panel_.remove_last_tab("building_stats_small");
> +
> +     // Clean state if buildings disappear from list
> +     const DescriptionIndex nr_buildings = 
> iplayer().egbase().tribes().nrbuildings();
> +     building_buttons_.clear();
> +     building_buttons_.resize(nr_buildings);
> +     owned_labels_.clear();
> +     owned_labels_.resize(nr_buildings);
> +     productivity_labels_.clear();
> +     productivity_labels_.resize(nr_buildings);
> +
> +     // Ensure that defunct buttons disappear
> +     for (int tab_index = 0; tab_index < kNoOfBuildingTabs; ++tab_index) {
> +             if (tabs_[tab_index] != nullptr) {
> +                     tabs_[tab_index]->die();
> +             }
> +     }
> +
> +     init(last_selected_tab);
> +
> +     // Reset navigator
> +     building_name_.set_text("");
> +     if (has_selection_) {
> +             if (building_buttons_[current_building_type_] != nullptr) {
> +                     set_current_building_type(current_building_type_);
> +             } else {
> +                     has_selection_ = false;
> +             }
> +     }
> +}
> +
> +void BuildingStatisticsMenu::init(int last_selected_tab) {
> +     // We want to add player tribe's buildings in correct order
> +     const Widelands::Player& player = iplayer().player();
> +     const TribeDescr& tribe = player.tribe();
> +     std::vector<DescriptionIndex> buildings_to_add[kNoOfBuildingTabs];
> +     // Add the player's own tribe's buildings.
> +     for (DescriptionIndex index : tribe.buildings()) {
> +             if (own_building_is_valid(index)) {
> +                     
> buildings_to_add[find_tab_for_building(*tribe.get_building_descr(index))].push_back(index);
> +             }
> +     }
> +
> +     // We want to add other tribes' buildings on the bottom. Only add the 
> ones that the player owns.
> +     for (DescriptionIndex index = 0; index < 
> iplayer().egbase().tribes().nrbuildings(); ++index) {
> +             if (foreign_tribe_building_is_valid(index)) {
> +                     
> buildings_to_add[find_tab_for_building(*tribe.get_building_descr(index))].push_back(index);
> +             }
> +     }
> +
> +     // Now create the tab contents and add the building buttons
> +     int row_counters[kNoOfBuildingTabs];
> +     for (int tab_index = 0; tab_index < kNoOfBuildingTabs; ++tab_index) {
> +             int current_column = 0;
> +             tabs_[tab_index] = new UI::Box(&tab_panel_, 0, 0, 
> UI::Box::Vertical);
> +             UI::Box* row = new UI::Box(tabs_[tab_index], 0, 0, 
> UI::Box::Horizontal);
> +             row_counters[tab_index] = 0;
> +
> +             for (const Widelands::DescriptionIndex id : 
> buildings_to_add[tab_index]) {
> +                     const BuildingDescr& descr = 
> *iplayer().egbase().tribes().get_building_descr(id);
> +                     add_button(id, descr, row);
> +                     ++current_column;
> +                     if (current_column == 1) {
> +                             ++row_counters[tab_index];
> +                     } else if (current_column == kColumns) {
> +                             tabs_[tab_index]->add(row, 
> UI::Box::Resizing::kFullSize);
> +                             tabs_[tab_index]->add_space(6);
> +                             row = new UI::Box(tabs_[tab_index], 0, 0, 
> UI::Box::Horizontal);
> +                             current_column = 0;
> +                     }
> +             }
> +             // Add final row
> +             if (current_column != 0) {
> +                     tabs_[tab_index]->add(row, 
> UI::Box::Resizing::kFullSize);
> +             }
> +     }
> +
> +     // Show the tabs that have buttons on them
> +     int tab_counter = 0;
> +     auto add_tab = [this, row_counters, &tab_counter, last_selected_tab](
> +        int tab_index, const std::string& name, const std::string& image, 
> const std::string& descr) {
> +             if (row_counters[tab_index] > 0) {
> +                     tab_panel_.add(name, g_gr->images().get(image), 
> tabs_[tab_index], descr);
> +                     if (last_selected_tab == tab_index) {
> +                             tab_panel_.activate(tab_counter);
> +                     }
> +                     tab_assignments_[tab_counter] = tab_index;
> +                     row_counters_[tab_counter] = row_counters[tab_index];
> +                     ++tab_counter;
> +             }
> +     };
> +     add_tab(BuildingTab::Small, "building_stats_small",
> +             "images/wui/fieldaction/menu_tab_buildsmall.png", _("Small 
> buildings"));
> +     add_tab(BuildingTab::Medium, "building_stats_medium",
> +             "images/wui/fieldaction/menu_tab_buildmedium.png", _("Medium 
> buildings"));
> +     add_tab(BuildingTab::Big, "building_stats_big", 
> "images/wui/fieldaction/menu_tab_buildbig.png",
> +             _("Big buildings"));
> +     add_tab(BuildingTab::Mines, "building_stats_mines",
> +             "images/wui/fieldaction/menu_tab_buildmine.png", _("Mines"));
> +     add_tab(BuildingTab::Ports, "building_stats_ports",
> +             "images/wui/fieldaction/menu_tab_buildport.png", _("Ports"));
> +
> +     update();
> +}
> +
> +bool 
> BuildingStatisticsMenu::own_building_is_valid(Widelands::DescriptionIndex 
> index) const {
> +     const Widelands::Player& player = iplayer().player();
> +     const BuildingDescr& descr = *player.tribe().get_building_descr(index);
> +     // Skip seafaring buildings if not needed
> +     if (descr.needs_seafaring() && 
> !iplayer().game().map().allows_seafaring() &&
> +         player.get_building_statistics(index).empty()) {
> +             return false;
> +     }
> +     if (descr.type() == MapObjectType::CONSTRUCTIONSITE ||
> +         descr.type() == MapObjectType::DISMANTLESITE) {
> +             return false;
> +     }
> +     // Only add allowed buildings or buildings that are owned by the player.
> +     if ((player.is_building_type_allowed(index) && (descr.is_buildable() || 
> descr.is_enhanced())) ||
> +         !player.get_building_statistics(index).empty()) {
> +             return true;
> +     }
> +     return false;
> +}
> +
> +bool BuildingStatisticsMenu::foreign_tribe_building_is_valid(
> +   Widelands::DescriptionIndex index) const {
> +     const Widelands::Player& player = iplayer().player();
> +     if (!player.tribe().has_building(index) && 
> !player.get_building_statistics(index).empty()) {

player.tribe().has_building(index) is always false is this context and can be 
dropped. -> not true, because this function is called for all buildings in 
tribes().

> +             const BuildingDescr& descr = 
> *iplayer().egbase().tribes().get_building_descr(index);
> +             if (descr.type() == MapObjectType::CONSTRUCTIONSITE ||
> +                 descr.type() == MapObjectType::DISMANTLESITE) {
> +                     return false;
> +             }
> +             return true;
> +     }
> +     return false;
> +}
> +
> +int BuildingStatisticsMenu::find_tab_for_building(const 
> Widelands::BuildingDescr& descr) const {
> +     assert(descr.type() != MapObjectType::CONSTRUCTIONSITE);
> +     assert(descr.type() != MapObjectType::DISMANTLESITE);
> +     if (descr.get_ismine()) {
> +             return BuildingTab::Mines;
> +     } else if (descr.get_isport()) {
> +             return BuildingTab::Ports;
> +     } else {
> +             switch (descr.get_size()) {
> +             case BaseImmovable::SMALL:
> +                     return BuildingTab::Small;
> +             case BaseImmovable::MEDIUM:
> +                     return BuildingTab::Medium;
> +             case BaseImmovable::BIG:
> +                     return BuildingTab::Big;
> +             default:
> +                     throw wexception(
> +                        "Building statictics: Found building without a size: 
> %s", descr.name().c_str());
> +             }
> +     }
> +     NEVER_HERE();
> +}
> +
> +void BuildingStatisticsMenu::update_building_list() {
> +     for (DescriptionIndex index = 0; index < 
> iplayer().egbase().tribes().nrbuildings(); ++index) {

Good idea. I've even turned nrbuildings into a member variable, because the 
value never changes after the game is loaded.

> +             const bool should_have_this_building =
> +                own_building_is_valid(index) || 
> foreign_tribe_building_is_valid(index);
> +             const bool has_this_building = building_buttons_[index] != 
> nullptr;
> +             if (should_have_this_building != has_this_building) {
> +                     reset();
> +                     return;
> +             }
> +     }
> +}
> +
>  /**
>   * Adds 3 buttons per building type.
>   *


-- 
https://code.launchpad.net/~widelands-dev/widelands/smaller_building_statistics/+merge/342828
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands.

_______________________________________________
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

Reply via email to