widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1653460-panel-init-width into lp:widelands.

Commit message:
Fixed assertion failure in int UI::Panel::get_inner_h():

- Fixed bug with automatic column sizing for tables where the columns were already being resized before all of them were added.
- Allow getting inner width/height for panels that have their borders set but w/h == 0. Added assertions and fallbacks to set_size and set_desired_size to make sure that width/height will never be negative.
- Explicitly initialize all variables in ui_basic.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1653460 in widelands: "UI::Panel::get_inner_h() const: Assertion `tborder_ + bborder_ <= h_' failed."
  https://bugs.launchpad.net/widelands/+bug/1653460

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1653460-panel-init-width/+merge/318358

The assertion in the bug should now be fixed for all panels containing tables:

* Fullscreen menus:

    - campaign_select
    - internet_lobby
    - launch_mpg
    - launch_spg
    - loadgame
    - mapselect
    - netsetup_lan

* In-game:
    - game_message_menu
    - game_summary
    - productionsitewindow
    - trainingsitewindow
    - tribal_encyclopedia

* Editor:
    - help
    - main_menu_load_map
    - main_menu_save_map

I don't know why this happened only sometimes and not all the time though.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1653460-panel-init-width into lp:widelands.

=== modified file 'src/ui_basic/box.cc'
--- src/ui_basic/box.cc	2017-02-25 13:27:40 +0000
+++ src/ui_basic/box.cc	2017-02-27 14:05:30 +0000
@@ -98,7 +98,7 @@
 	int maxbreadth = mindesiredbreadth_;
 
 	for (uint32_t idx = 0; idx < items_.size(); ++idx) {
-		int depth, breadth;
+		int depth, breadth = 0;
 		get_item_desired_size(idx, &depth, &breadth);
 
 		totaldepth += depth;
@@ -137,7 +137,7 @@
 	int totaldepth = 0;
 
 	for (size_t idx = 0; idx < items_.size(); ++idx) {
-		int depth, unused;
+		int depth, unused = 0;
 		get_item_desired_size(idx, &depth, &unused);
 		totaldepth += depth;
 	}
@@ -221,7 +221,7 @@
 		totalbreadth -= Scrollbar::kSize;
 
 	for (uint32_t idx = 0; idx < items_.size(); ++idx) {
-		int depth, breadth;
+		int depth, breadth = 0;
 		get_item_size(idx, &depth, &breadth);
 
 		if (items_[idx].type == Item::ItemPanel) {
@@ -371,7 +371,7 @@
 
 	switch (it.type) {
 	case Item::ItemPanel: {
-		int32_t breadth, maxbreadth;
+		int32_t breadth, maxbreadth = 0;
 
 		if (orientation_ == Horizontal) {
 			breadth = it.u.panel.panel->get_inner_h();

=== modified file 'src/ui_basic/listselect.cc'
--- src/ui_basic/listselect.cc	2017-02-23 19:38:51 +0000
+++ src/ui_basic/listselect.cc	2017-02-27 14:05:30 +0000
@@ -69,10 +69,9 @@
 	scrollbar_.moved.connect(boost::bind(&BaseListselect::set_scrollpos, this, _1));
 
 	if (selection_mode_ == ListselectLayout::kShowCheck) {
-		int pic_h;
 		check_pic_ = g_gr->images().get("images/ui_basic/list_selected.png");
 		max_pic_width_ = check_pic_->width();
-		pic_h = check_pic_->height();
+		int pic_h = check_pic_->height();
 		if (pic_h > lineheight_)
 			lineheight_ = pic_h;
 	} else {

=== modified file 'src/ui_basic/multilineeditbox.cc'
--- src/ui_basic/multilineeditbox.cc	2017-02-23 17:58:25 +0000
+++ src/ui_basic/multilineeditbox.cc	2017-02-27 14:05:30 +0000
@@ -303,7 +303,7 @@
 			if (d_->cursor_pos < d_->text.size()) {
 				d_->refresh_ww();
 
-				uint32_t cursorline, cursorpos;
+				uint32_t cursorline, cursorpos = 0;
 				d_->ww.calc_wrapped_pos(d_->cursor_pos, cursorline, cursorpos);
 
 				if (cursorline + 1 < d_->ww.nrlines()) {
@@ -332,7 +332,7 @@
 			if (d_->cursor_pos > 0) {
 				d_->refresh_ww();
 
-				uint32_t cursorline, cursorpos;
+				uint32_t cursorline, cursorpos = 0;
 				d_->ww.calc_wrapped_pos(d_->cursor_pos, cursorline, cursorpos);
 
 				if (cursorline > 0) {
@@ -361,7 +361,7 @@
 			} else {
 				d_->refresh_ww();
 
-				uint32_t cursorline, cursorpos;
+				uint32_t cursorline, cursorpos = 0;
 				d_->ww.calc_wrapped_pos(d_->cursor_pos, cursorline, cursorpos);
 
 				d_->set_cursor_pos(d_->ww.line_offset(cursorline));
@@ -379,7 +379,7 @@
 			} else {
 				d_->refresh_ww();
 
-				uint32_t cursorline, cursorpos;
+				uint32_t cursorline, cursorpos = 0;
 				d_->ww.calc_wrapped_pos(d_->cursor_pos, cursorline, cursorpos);
 
 				if (cursorline + 1 < d_->ww.nrlines())
@@ -488,7 +488,7 @@
 void MultilineEditbox::Data::scroll_cursor_into_view() {
 	refresh_ww();
 
-	uint32_t cursorline, cursorpos;
+	uint32_t cursorline, cursorpos = 0;
 	ww.calc_wrapped_pos(cursor_pos, cursorline, cursorpos);
 
 	int32_t lineheight = textstyle.font->height();

=== modified file 'src/ui_basic/multilinetextarea.cc'
--- src/ui_basic/multilinetextarea.cc	2017-02-23 19:38:51 +0000
+++ src/ui_basic/multilinetextarea.cc	2017-02-27 14:05:30 +0000
@@ -79,7 +79,7 @@
  * and adjust scrollbar settings accordingly.
  */
 void MultilineTextarea::recompute() {
-	uint32_t height;
+	int height = 0;
 
 	// We wrap the text twice. We need to do this to account for the presence/absence of the
 	// scollbar.

=== modified file 'src/ui_basic/panel.cc'
--- src/ui_basic/panel.cc	2017-01-25 18:55:59 +0000
+++ src/ui_basic/panel.cc	2017-02-27 14:05:30 +0000
@@ -240,8 +240,12 @@
 	if (nw == w_ && nh == h_)
 		return;
 
-	w_ = nw;
-	h_ = nh;
+	assert(nw >= 0);
+	assert(nh >= 0);
+
+	// Make sure that we never get negative width/height in release builds.
+	w_ = std::max(0, nw);
+	h_ = std::max(0, nh);
 
 	if (parent_)
 		move_inside_parent();
@@ -282,9 +286,12 @@
 
 	assert(w < 3000);
 	assert(h < 3000);
+	assert(w >= 0);
+	assert(h >= 0);
 
-	desired_w_ = w;
-	desired_h_ = h;
+	// Make sure that we never get negative width/height in release builds.
+	desired_w_ = std::max(0, w);
+	desired_h_ = std::max(0, h);
 	if (!get_layout_toplevel() && parent_) {
 		parent_->update_desired_size();
 	} else {
@@ -351,6 +358,7 @@
  * Set the size of the inner area (total area minus border)
  */
 void Panel::set_inner_size(int const nw, int const nh) {
+	assert(nw >= 0 && nh >= 0);
 	set_size(nw + lborder_ + rborder_, nh + tborder_ + bborder_);
 }
 
@@ -366,6 +374,15 @@
 	bborder_ = b;
 }
 
+int Panel::get_inner_w() const {
+	assert(w_ == 0 || lborder_ + rborder_ <= w_);
+	return (w_ == 0 ? 0 : w_ - (lborder_ + rborder_));
+}
+int Panel::get_inner_h() const {
+	assert(h_ == 0 || tborder_ + bborder_ <= h_);
+	return (h_ == 0 ? 0 : h_ - (tborder_ + bborder_));
+}
+
 /**
  * Make this panel the top-most panel in the parent's Z-order.
  */

=== modified file 'src/ui_basic/panel.h'
--- src/ui_basic/panel.h	2017-01-25 18:55:59 +0000
+++ src/ui_basic/panel.h	2017-02-27 14:05:30 +0000
@@ -180,14 +180,8 @@
 		return bborder_;
 	}
 
-	int get_inner_w() const {
-		assert(lborder_ + rborder_ <= w_);
-		return w_ - (lborder_ + rborder_);
-	}
-	int get_inner_h() const {
-		assert(tborder_ + bborder_ <= h_);
-		return h_ - (tborder_ + bborder_);
-	}
+	int get_inner_w() const;
+	int get_inner_h() const;
 
 	const Panel* get_next_sibling() const {
 		return next_;

=== modified file 'src/ui_basic/scrollbar.cc'
--- src/ui_basic/scrollbar.cc	2017-01-25 18:55:59 +0000
+++ src/ui_basic/scrollbar.cc	2017-02-27 14:05:30 +0000
@@ -130,7 +130,7 @@
 }
 
 Scrollbar::Area Scrollbar::get_area_for_point(int32_t x, int32_t y) {
-	int32_t extent;
+	int32_t extent = 0;
 
 	// Out of panel
 	if (x < 0 || x >= static_cast<int32_t>(get_w()) || y < 0 || y >= static_cast<int32_t>(get_h()))

=== modified file 'src/ui_basic/table.cc'
--- src/ui_basic/table.cc	2017-02-26 11:57:15 +0000
+++ src/ui_basic/table.cc	2017-02-27 14:05:30 +0000
@@ -108,7 +108,7 @@
 	//  If there would be existing entries, they would not get the new column.
 	assert(size() == 0);
 
-	uint32_t complete_width = 0;
+	int complete_width = 0;
 	for (const Column& col : columns_) {
 		complete_width += col.width;
 	}
@@ -133,7 +133,6 @@
 			flexible_column_ = columns_.size() - 1;
 		}
 	}
-	layout();
 }
 
 void Table<void*>::set_column_title(uint8_t const col, const std::string& title) {
@@ -209,8 +208,7 @@
 	if (entries == 0) {
 		entries = size();
 	}
-	int tablewidth;
-	int tableheight;
+	int tablewidth, tableheight = 0;
 	get_desired_size(&tablewidth, &tableheight);
 	tableheight = headerheight_ + 2 + get_lineheight() * entries;
 	set_desired_size(tablewidth, tableheight);
@@ -606,15 +604,13 @@
 
 	// Find a column to resize
 	size_t resizeable_column = std::numeric_limits<size_t>::max();
-	if (flexible_column_ != std::numeric_limits<size_t>::max()) {
+	if (flexible_column_ < columns_.size()) {
 		resizeable_column = flexible_column_;
 	} else {
 		// Use the widest column
-		int all_columns_width = scrollbar_ && scrollbar_->is_enabled() ? scrollbar_->get_w() : 0;
 		uint32_t widest_width = columns_[resizeable_column].width;
 		for (size_t i = 1; i < columns_.size(); ++i) {
 			const uint32_t width = columns_[i].width;
-			all_columns_width += width;
 			if (width > widest_width) {
 				widest_width = width;
 				resizeable_column = i;
@@ -630,8 +626,9 @@
 		}
 		if (all_columns_width != get_w()) {
 			Column& column = columns_.at(resizeable_column);
-			column.width = column.width + get_w() - all_columns_width;
+			column.width = std::max(0, column.width + get_w() - all_columns_width);
 			column.btn->set_size(column.width, column.btn->get_h());
+
 			int offset = 0;
 			for (const auto& col : columns_) {
 				col.btn->set_pos(Vector2i(offset, col.btn->get_y()));

=== modified file 'src/ui_basic/table.h'
--- src/ui_basic/table.h	2017-02-26 11:57:15 +0000
+++ src/ui_basic/table.h	2017-02-27 14:05:30 +0000
@@ -184,6 +184,8 @@
 	void set_column_title(uint8_t col, const std::string& title);
 	void set_column_compare(uint8_t col, const CompareFn& fn);
 
+	void layout() override;
+
 	void clear();
 	void set_sort_column(uint8_t const col) {
 		assert(col < columns_.size());
@@ -280,11 +282,10 @@
 private:
 	bool default_compare_string(uint32_t column, uint32_t a, uint32_t b);
 	bool sort_helper(uint32_t a, uint32_t b);
-	void layout() override;
 
 	struct Column {
 		Button* btn;
-		uint32_t width;
+		int width;
 		Align alignment;
 		CompareFn compare;
 	};
@@ -293,8 +294,8 @@
 	static const int32_t ms_darken_value = -20;
 
 	Columns columns_;
-	uint32_t total_width_;
-	uint32_t headerheight_;
+	int total_width_;
+	const uint32_t headerheight_;
 	int32_t lineheight_;
 	const Image* button_background_;
 	Scrollbar* scrollbar_;

=== modified file 'src/ui_basic/tabpanel.cc'
--- src/ui_basic/tabpanel.cc	2017-02-23 19:38:51 +0000
+++ src/ui_basic/tabpanel.cc	2017-02-27 14:05:30 +0000
@@ -139,7 +139,7 @@
 	// size of contents
 	if (active_ < tabs_.size()) {
 		Panel* const panel = tabs_[active_]->panel;
-		int panelw, panelh;
+		int panelw, panelh = 0;
 
 		panel->get_desired_size(&panelw, &panelh);
 		// TODO(unknown):  the panel might be bigger -> add a scrollbar in that case

=== modified file 'src/ui_basic/textarea.cc'
--- src/ui_basic/textarea.cc	2017-02-23 19:38:51 +0000
+++ src/ui_basic/textarea.cc	2017-02-27 14:05:30 +0000
@@ -163,7 +163,7 @@
 	int32_t y = get_y();
 
 	update_desired_size();
-	int w, h;
+	int w, h = 0;
 	get_desired_size(&w, &h);
 
 	switch (align_) {

=== modified file 'src/ui_basic/window.cc'
--- src/ui_basic/window.cc	2017-02-24 10:21:37 +0000
+++ src/ui_basic/window.cc	2017-02-27 14:05:30 +0000
@@ -128,7 +128,7 @@
  */
 void Window::update_desired_size() {
 	if (center_panel_ && !is_minimal_) {
-		int innerw, innerh;
+		int innerw, innerh = 0;
 		center_panel_->get_desired_size(&innerw, &innerh);
 		set_desired_size(
 		   innerw + get_lborder() + get_rborder(), innerh + get_tborder() + get_bborder());

=== modified file 'src/ui_fsmenu/campaign_select.cc'
--- src/ui_fsmenu/campaign_select.cc	2017-02-26 11:57:15 +0000
+++ src/ui_fsmenu/campaign_select.cc	2017-02-27 14:05:30 +0000
@@ -358,6 +358,7 @@
 
 void FullscreenMenuCampaignMapSelect::layout() {
 	// TODO(GunChleoc): Implement when we have box layout for the details.
+	table_.layout();
 }
 
 std::string FullscreenMenuCampaignMapSelect::get_map() {

=== modified file 'src/ui_fsmenu/internet_lobby.cc'
--- src/ui_fsmenu/internet_lobby.cc	2017-02-23 19:38:51 +0000
+++ src/ui_fsmenu/internet_lobby.cc	2017-02-27 14:05:30 +0000
@@ -153,6 +153,7 @@
 
 void FullscreenMenuInternetLobby::layout() {
 	// TODO(GunChleoc): Box layout and then implement
+	clientsonline_list_.layout();
 }
 
 /// think function of the UI (main loop)

=== modified file 'src/ui_fsmenu/loadgame.cc'
--- src/ui_fsmenu/loadgame.cc	2017-02-26 11:57:15 +0000
+++ src/ui_fsmenu/loadgame.cc	2017-02-27 14:05:30 +0000
@@ -188,6 +188,7 @@
 		ta_mapname_.set_tooltip(_("The map that this game is based on"));
 		delete_.set_tooltip(_("Delete this game"));
 	}
+	set_thinks(false);
 	minimap_icon_.set_visible(false);
 
 	back_.sigclicked.connect(boost::bind(&FullscreenMenuLoadGame::clicked_back, boost::ref(*this)));
@@ -240,6 +241,7 @@
 
 void FullscreenMenuLoadGame::layout() {
 	// TODO(GunChleoc): Implement when we have box layout for the details.
+	table_.layout();
 }
 
 void FullscreenMenuLoadGame::think() {

=== modified file 'src/ui_fsmenu/netsetup_lan.cc'
--- src/ui_fsmenu/netsetup_lan.cc	2017-02-23 19:38:51 +0000
+++ src/ui_fsmenu/netsetup_lan.cc	2017-02-27 14:05:30 +0000
@@ -126,6 +126,7 @@
 
 void FullscreenMenuNetSetupLAN::layout() {
 	// TODO(GunChleoc): Box layout and then implement
+	opengames.layout();
 }
 
 void FullscreenMenuNetSetupLAN::think() {

=== modified file 'src/wui/game_message_menu.cc'
--- src/wui/game_message_menu.cc	2017-02-26 11:57:15 +0000
+++ src/wui/game_message_menu.cc	2017-02-27 14:05:30 +0000
@@ -152,6 +152,7 @@
 	   ColTimeSent, boost::bind(&GameMessageMenu::compare_time_sent, this, _1, _2));
 
 	list->set_sort_column(ColTimeSent);
+	list->layout();
 
 	set_can_focus(true);
 	focus();

=== modified file 'src/wui/game_summary.cc'
--- src/wui/game_summary.cc	2017-02-25 13:27:40 +0000
+++ src/wui/game_summary.cc	2017-02-27 14:05:30 +0000
@@ -208,6 +208,7 @@
 	if (!players_status.empty()) {
 		players_table_->select(current_player_position);
 	}
+	players_table_->layout();
 }
 
 void GameSummaryScreen::continue_clicked() {

=== modified file 'src/wui/maptable.cc'
--- src/wui/maptable.cc	2017-02-26 11:57:15 +0000
+++ src/wui/maptable.cc	2017-02-27 14:05:30 +0000
@@ -76,4 +76,5 @@
 		}
 	}
 	sort();
+	layout();
 }