widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands.

Commit message:
Building Statistics now only show relevant buildings
- Military sites not belonging to the tribe are omitted, unless the payer currently owns one
- Seafaring and allowed buildings are dynamically checked
- Show "under construction" navigation for buildings being enhanced

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1636511 in widelands: "Buildings being upgraded not showing up in Building Statistics box."
  https://bugs.launchpad.net/widelands/+bug/1636511

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/smaller_building_statistics/+merge/342828

Fixes the attached bug.

Since the Frisians were added, the building statistics got too big due to all the military sites - the navigation became impossible to reach on 800x600 resolution without moving the window around. So, I have implemented a dynamic approach that will only show allowed and currently owned buildings.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/smaller_building_statistics into lp:widelands.

=== modified file 'data/tribes/buildings/warehouses/atlanteans/port/init.lua'
--- data/tribes/buildings/warehouses/atlanteans/port/init.lua	2017-09-02 19:53:03 +0000
+++ data/tribes/buildings/warehouses/atlanteans/port/init.lua	2018-04-07 16:30:59 +0000
@@ -8,6 +8,7 @@
    helptext_script = dirname .. "helptexts.lua",
    icon = dirname .. "menu.png",
    size = "port",
+   needs_seafaring = true,
 
    buildcost = {
       log = 3,

=== modified file 'data/tribes/buildings/warehouses/barbarians/port/init.lua'
--- data/tribes/buildings/warehouses/barbarians/port/init.lua	2017-09-02 19:53:03 +0000
+++ data/tribes/buildings/warehouses/barbarians/port/init.lua	2018-04-07 16:30:59 +0000
@@ -8,6 +8,7 @@
    helptext_script = dirname .. "helptexts.lua",
    icon = dirname .. "menu.png",
    size = "port",
+   needs_seafaring = true,
 
    buildcost = {
       log = 3,

=== modified file 'data/tribes/buildings/warehouses/empire/port/init.lua'
--- data/tribes/buildings/warehouses/empire/port/init.lua	2017-09-02 19:53:03 +0000
+++ data/tribes/buildings/warehouses/empire/port/init.lua	2018-04-07 16:30:59 +0000
@@ -8,6 +8,7 @@
    helptext_script = dirname .. "helptexts.lua",
    icon = dirname .. "menu.png",
    size = "port",
+   needs_seafaring = true,
 
    buildcost = {
       log = 3,

=== modified file 'data/tribes/buildings/warehouses/frisians/port/init.lua'
--- data/tribes/buildings/warehouses/frisians/port/init.lua	2018-02-16 14:53:04 +0000
+++ data/tribes/buildings/warehouses/frisians/port/init.lua	2018-04-07 16:30:59 +0000
@@ -8,6 +8,7 @@
    helptext_script = dirname .. "helptexts.lua",
    icon = dirname .. "menu.png",
    size = "port",
+   needs_seafaring = true,
 
    buildcost = {
       brick = 6,

=== 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 16:30:59 +0000
@@ -225,6 +225,9 @@
 }
 
 bool TabPanel::remove_last_tab(const std::string& tabname) {
+	if (tabs_.empty()) {
+		return false;
+	}
 	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 16:30:59 +0000
@@ -120,118 +120,11 @@
      low_production_(33),
      has_selection_(false) {
 
-	for (int i = 0; i < kNoOfBuildingTabs; ++i) {
-		row_counters_[i] = 0;
-		tabs_[i] = new UI::Box(&tab_panel_, 0, 0, UI::Box::Vertical);
-	}
-
-	tab_panel_.add("building_stats_small",
-	               g_gr->images().get("images/wui/fieldaction/menu_tab_buildsmall.png"),
-	               tabs_[BuildingTab::Small], _("Small buildings"));
-	tab_panel_.add("building_stats_medium",
-	               g_gr->images().get("images/wui/fieldaction/menu_tab_buildmedium.png"),
-	               tabs_[BuildingTab::Medium], _("Medium buildings"));
-	tab_panel_.add("building_stats_big",
-	               g_gr->images().get("images/wui/fieldaction/menu_tab_buildbig.png"),
-	               tabs_[BuildingTab::Big], _("Big buildings"));
-	tab_panel_.add("building_stats_mines",
-	               g_gr->images().get("images/wui/fieldaction/menu_tab_buildmine.png"),
-	               tabs_[BuildingTab::Mines], _("Mines"));
-
-	// Only show the ports tab for seafaring maps
-	if (iplayer().game().map().allows_seafaring()) {
-		tab_panel_.add("building_stats_ports",
-		               g_gr->images().get("images/wui/fieldaction/menu_tab_buildport.png"),
-		               tabs_[BuildingTab::Ports], _("Ports"));
-	}
-
 	const DescriptionIndex nr_buildings = parent.egbase().tribes().nrbuildings();
 	building_buttons_ = std::vector<UI::Button*>(nr_buildings);
 	owned_labels_ = std::vector<UI::Textarea*>(nr_buildings);
 	productivity_labels_ = std::vector<UI::Textarea*>(nr_buildings);
 
-	// Column counters
-	int columns[kNoOfBuildingTabs] = {0, 0, 0, 0, 0};
-
-	// Row containers
-	UI::Box* rows[kNoOfBuildingTabs];
-	for (int i = 0; i < kNoOfBuildingTabs; ++i) {
-		rows[i] = new UI::Box(tabs_[i], 0, 0, UI::Box::Horizontal);
-	}
-
-	// We want to add player tribe's buildings in correct order
-	const TribeDescr& tribe = iplayer().player().tribe();
-	std::vector<DescriptionIndex> buildings_to_add;
-	for (DescriptionIndex index : tribe.buildings()) {
-		// Only add headquarter types that are owned by player.
-		const BuildingDescr& descr = *tribe.get_building_descr(index);
-		const Widelands::Player& player = iplayer().player();
-		if (descr.is_buildable() || descr.is_enhanced() ||
-		    !player.get_building_statistics(index).empty()) {
-			buildings_to_add.push_back(index);
-		}
-	}
-
-	// We want to add other tribes' militarysites on the bottom
-	for (DescriptionIndex index = 0; index < nr_buildings; ++index) {
-		const BuildingDescr& descr = *parent.egbase().tribes().get_building_descr(index);
-		if (descr.type() == MapObjectType::MILITARYSITE && !tribe.has_building(index)) {
-			buildings_to_add.push_back(index);
-		}
-	}
-
-	for (DescriptionIndex id : buildings_to_add) {
-		const BuildingDescr& descr = *tribe.get_building_descr(id);
-
-		if (descr.type() != MapObjectType::CONSTRUCTIONSITE &&
-		    descr.type() != MapObjectType::DISMANTLESITE) {
-			if (descr.get_ismine()) {
-				if (add_button(id, descr, BuildingTab::Mines, *rows[BuildingTab::Mines],
-				               &columns[BuildingTab::Mines])) {
-					rows[BuildingTab::Mines] =
-					   new UI::Box(tabs_[BuildingTab::Mines], 0, 0, UI::Box::Horizontal);
-				}
-			} else if (descr.get_isport()) {
-				if (add_button(id, descr, BuildingTab::Ports, *rows[BuildingTab::Ports],
-				               &columns[BuildingTab::Ports])) {
-					rows[BuildingTab::Ports] =
-					   new UI::Box(tabs_[BuildingTab::Ports], 0, 0, UI::Box::Horizontal);
-				}
-			} else {
-				switch (descr.get_size()) {
-				case BaseImmovable::SMALL:
-					if (add_button(id, descr, BuildingTab::Small, *rows[BuildingTab::Small],
-					               &columns[BuildingTab::Small])) {
-						rows[BuildingTab::Small] =
-						   new UI::Box(tabs_[BuildingTab::Small], 0, 0, UI::Box::Horizontal);
-					}
-					break;
-				case BaseImmovable::MEDIUM:
-					if (add_button(id, descr, BuildingTab::Medium, *rows[BuildingTab::Medium],
-					               &columns[BuildingTab::Medium])) {
-						rows[BuildingTab::Medium] =
-						   new UI::Box(tabs_[BuildingTab::Medium], 0, 0, UI::Box::Horizontal);
-					}
-					break;
-				case BaseImmovable::BIG:
-					if (add_button(id, descr, BuildingTab::Big, *rows[BuildingTab::Big],
-					               &columns[BuildingTab::Big])) {
-						rows[BuildingTab::Big] =
-						   new UI::Box(tabs_[BuildingTab::Big], 0, 0, UI::Box::Horizontal);
-					}
-					break;
-				default:
-					throw wexception(
-					   "Building statictics: Found building without a size: %s", descr.name().c_str());
-				}
-			}
-		}
-	}
-
-	for (int i = 0; i < kNoOfBuildingTabs; ++i) {
-		tabs_[i]->add(rows[i]);
-	}
-
 	set_label_font(&owned_label_);
 	set_label_font(&construction_label_);
 	set_label_font(&unproductive_label_);
@@ -306,7 +199,7 @@
 	unproductive_percent_.cancel.connect(
 	   boost::bind(&BuildingStatisticsMenu::low_production_reset_focus, boost::ref(*this)));
 
-	update();
+	init();
 }
 
 BuildingStatisticsMenu::~BuildingStatisticsMenu() {
@@ -315,6 +208,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()) {
+		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) {
+		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.
  *
@@ -322,10 +398,10 @@
  * - Buildings owned, steps through constructionsites
  * - Productivity, steps though buildings with low productivity and stopped buildings
  */
-bool BuildingStatisticsMenu::add_button(
-   DescriptionIndex id, const BuildingDescr& descr, int tab_index, UI::Box& row, int* column) {
-
-	UI::Box* button_box = new UI::Box(&row, 0, 0, UI::Box::Vertical);
+void BuildingStatisticsMenu::add_button(DescriptionIndex id,
+                                        const BuildingDescr& descr,
+                                        UI::Box* row) {
+	UI::Box* button_box = new UI::Box(row, 0, 0, UI::Box::Vertical);
 	building_buttons_[id] = new UI::Button(
 	   button_box, (boost::format("building_button%s") % id).str(), 0, 0, kBuildGridCellWidth,
 	   kBuildGridCellHeight, g_gr->images().get("images/ui_basic/but1.png"),
@@ -347,25 +423,10 @@
 	productivity_labels_[id]->set_fixed_width(kBuildGridCellWidth);
 	button_box->add(productivity_labels_[id]);
 
-	row.add(button_box);
+	row->add(button_box);
 
 	building_buttons_[id]->sigclicked.connect(
 	   boost::bind(&BuildingStatisticsMenu::set_current_building_type, boost::ref(*this), id));
-
-	// For dynamic window height
-	if (*column == 0) {
-		++row_counters_[tab_index];
-	}
-
-	// Check if the row is full
-	++*column;
-	if (*column == kColumns) {
-		tabs_[tab_index]->add(&row);
-		tabs_[tab_index]->add_space(6);
-		*column = 0;
-		return true;
-	}
-	return false;
 }
 
 void BuildingStatisticsMenu::jump_building(JumpTarget target, bool reverse) {
@@ -496,28 +557,30 @@
  * Update this statistic
  */
 void BuildingStatisticsMenu::think() {
+	// Update statistics
+	const int32_t gametime = iplayer().game().get_gametime();
+
+	if (was_minimized_ || (gametime - lastupdate_) > kUpdateTimeInGametimeMs) {
+		update_building_list();
+		update();
+		lastupdate_ = gametime;
+	}
+	// Make sure we don't have a delay with displaying labels when we restore the window.
+	was_minimized_ = is_minimal();
+
 	// Adjust height to current tab
 	if (is_minimal()) {
 		tab_panel_.set_size(0, 0);
 	} else {
-		int tab_height =
+		const int tab_height =
 		   35 +
-		   row_counters_[tab_panel_.active()] * (kBuildGridCellHeight + kLabelHeight + kLabelHeight);
+		   row_counters_[tab_panel_.active()] * (kBuildGridCellHeight + kLabelHeight + kLabelHeight) +
+		   kMargin;
 		tab_panel_.set_size(kWindowWidth, tab_height);
 		set_size(
-		   get_w(), tab_height + kMargin + 4 * kButtonRowHeight + get_tborder() + get_bborder());
+		   get_w(), tab_height + kMargin + navigation_panel_.get_h() + get_tborder() + get_bborder());
 		navigation_panel_.set_pos(Vector2i(0, tab_height + kMargin));
 	}
-
-	// Update statistics
-	const int32_t gametime = iplayer().game().get_gametime();
-
-	if (was_minimized_ || (gametime - lastupdate_) > kUpdateTimeInGametimeMs) {
-		update();
-		lastupdate_ = gametime;
-	}
-	// Make sure we don't have a delay with displaying labels when we restore the window.
-	was_minimized_ = is_minimal();
 }
 
 /*
@@ -676,7 +739,9 @@
 		}
 
 		std::string owned_text;
-		if (player.tribe().has_building(id) && (building.is_buildable() || building.is_enhanced())) {
+		const bool can_construct_this_building = player.tribe().has_building(id) &&
+				(building.is_buildable() || building.is_enhanced());
+		if (can_construct_this_building) {
 			/** TRANSLATORS: Buildings: owned / under construction */
 			owned_text = (boost::format(_("%1%/%2%")) % nr_owned % nr_build).str();
 		} else {
@@ -694,7 +759,7 @@
 			no_owned_label_.set_visible(true);
 			navigation_buttons_[NavigationButton::NextOwned]->set_visible(true);
 			navigation_buttons_[NavigationButton::PrevOwned]->set_visible(true);
-			if (player.tribe().has_building(id) && building.is_buildable()) {
+			if (can_construct_this_building) {
 				no_construction_label_.set_text(nr_build > 0 ? std::to_string(nr_build) : "");
 				navigation_buttons_[NavigationButton::NextConstruction]->set_enabled(nr_build > 0);
 				navigation_buttons_[NavigationButton::PrevConstruction]->set_enabled(nr_build > 0);

=== modified file 'src/wui/building_statistics_menu.h'
--- src/wui/building_statistics_menu.h	2017-11-26 14:44:23 +0000
+++ src/wui/building_statistics_menu.h	2018-04-07 16:30:59 +0000
@@ -65,13 +65,21 @@
 		NextUnproductive
 	};
 
+	/// Initialize the buttons
+	void reset();
+	void init(int last_selected_tab = 0);
+
+	bool own_building_is_valid(Widelands::DescriptionIndex index) const;
+	bool foreign_tribe_building_is_valid(Widelands::DescriptionIndex index) const;
+	int find_tab_for_building(const Widelands::BuildingDescr& descr) const;
+
+	void update_building_list();
+
 	/// Adds a button for the building type belonging to the id and descr to the tab.
 	/// Returns true when a new row needs to be created.
-	bool add_button(Widelands::DescriptionIndex id,
+	void add_button(Widelands::DescriptionIndex id,
 	                const Widelands::BuildingDescr& descr,
-	                int tab_index,
-	                UI::Box& row,
-	                int* column);
+	                UI::Box* row);
 
 	/// Jumps to the next / previous appropriate building
 	void jump_building(JumpTarget target, bool reverse);
@@ -96,6 +104,7 @@
 	UI::TabPanel tab_panel_;
 	UI::Box* tabs_[kNoOfBuildingTabs];
 	int row_counters_[kNoOfBuildingTabs];
+	int tab_assignments_[kNoOfBuildingTabs];
 
 	/// Button with building icon
 	std::vector<UI::Button*> building_buttons_;