widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1346965-pointer-table into lp:widelands.

Commit message:
Fix assert violation and memory leak with table column buttons. All table columns now always have a button, even if the column title is empty.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1346965 in widelands: "Called C++ object pointer is null in ui_basic/table.cc (set_column_title)"
  https://bugs.launchpad.net/widelands/+bug/1346965

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1346965-pointer-table/+merge/300974

Completely removed the code that was causing scan-build to complain. The design decision is that if you want a table without heading, use a listselect - multiple columns should always have headings in the UI. We weren't using empty column titles anyway.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1346965-pointer-table into lp:widelands.

=== modified file 'src/ui_basic/table.cc'
--- src/ui_basic/table.cc	2016-03-25 17:01:05 +0000
+++ src/ui_basic/table.cc	2016-07-23 09:24:01 +0000
@@ -76,6 +76,9 @@
 	for (const EntryRecord * entry : entry_records_) {
 		delete entry;
 	}
+	for (Column& column : columns_) {
+		delete column.btn;
+	}
 }
 
 /// Add a new column to this table.
@@ -99,17 +102,16 @@
 
 	{
 		Column c;
-		c.btn = nullptr;
-		if (title.size()) {
-			c.btn =
-				new Button
-					(this, title,
-					 complete_width, 0, width, headerheight_,
-					 g_gr->images().get("images/ui_basic/but3.png"),
-					 title, tooltip_string, true, false);
-			c.btn->sigclicked.connect
-				(boost::bind(&Table::header_button_clicked, boost::ref(*this), columns_.size()));
-		}
+		// All columns have a title button that is clickable for sorting.
+		// The title text can be empty.
+		c.btn =
+			new Button
+				(this, title,
+				 complete_width, 0, width, headerheight_,
+				 g_gr->images().get("images/ui_basic/but3.png"),
+				 title, tooltip_string, true, false);
+		c.btn->sigclicked.connect
+			(boost::bind(&Table::header_button_clicked, boost::ref(*this), columns_.size()));
 		c.width = width;
 		c.alignment = alignment;
 		c.is_checkbox_column = is_checkbox_column;
@@ -144,25 +146,8 @@
 {
 	assert(col < columns_.size());
 	Column & column = columns_.at(col);
-	if (!column.btn && !title.empty()) { //  no title before, but now
-		uint32_t complete_width = 0;
-		for (uint8_t i = 0; i < col; ++i)
-			complete_width += columns_.at(i).width;
-		column.btn =
-			new Button
-				(this, title,
-				 complete_width, 0, column.width, headerheight_,
-				 g_gr->images().get("images/ui_basic/but3.png"),
-				 title, "", true, false);
-		column.btn->sigclicked.connect
-			(boost::bind(&Table::header_button_clicked, boost::ref(*this), col));
-	} else if (title.empty()) { //  had title before, not now
-		if (column.btn) {
-			delete column.btn;
-			column.btn = nullptr;
-		}
-	} else
-		column.btn->set_title(title);
+	assert(column.btn);
+	column.btn->set_title(title);
 }
 
 /**