Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved performance of adding rows and columns to the WTable #154

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/Wt/WTable.C
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,11 @@ WTableRow* WTable::insertRow(int row, std::unique_ptr<WTableRow> tableRow)
for (auto &cell : tableRow->cells_) {
widgetAdded(cell.get());
}

int old_end = rows_.size();
rows_.insert(rows_.begin() + row, std::move(tableRow));
updateRowNumbers(std::min(old_end, row), rowCount() - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what situation would this not suffice?

Suggested change
updateRowNumbers(std::min(old_end, row), rowCount() - 1);
updateRowNumbers(row, rowCount() - 1);


rows_[row].get()->expand(columnCount());
repaint(RepaintFlag::SizeAffected);

Expand Down Expand Up @@ -143,6 +147,8 @@ std::unique_ptr<WTableRow> WTable::removeRow(int row)

std::unique_ptr<WTableRow> result = std::move(rows_[row]);
rows_.erase(rows_.begin() + row);
updateRowNumbers(row, rowCount() - 1);

result->setTable(nullptr);

for (auto &cell : result->cells_)
Expand Down Expand Up @@ -408,6 +414,7 @@ void WTable::moveRow(int from, int to)
if (to > (int)rows_.size())
rowAt(to);
rows_.insert(rows_.begin() + to, std::move(from_tr));
updateRowNumbers(std::min(from, to), std::max(from, to));

// make sure spans don't cause segmentation faults during rendering
auto& cells = rows_[to]->cells_;
Expand Down Expand Up @@ -451,6 +458,16 @@ void WTable::moveColumn(int from, int to)
repaint(RepaintFlag::SizeAffected);
}

void WTable::updateRowNumbers(int begin, int end) {
if (begin < 0 || begin >= rowCount() || end < 0 || end >= rowCount()) {
return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this ever happens that would be a programming error that's internal to Wt, right?

This should then either cause an alarming error message or should be omitted altogether.

}

for (size_t i = begin; i <= end; ++i) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop uses size_t, while begin and end are ints?

rows_[i]->rowNumber_ = i;
}
}

void WTable::iterateChildren(const HandleWidgetMethod &method) const
{
for (const auto &row : rows_) {
Expand Down
1 change: 1 addition & 0 deletions src/Wt/WTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ class WT_API WTable : public WInteractWidget
void expand(int row, int column, int rowSpan, int columnSpan);
WTableCell *itemAt(int row, int column);
void setItemAt(int row, int column, std::unique_ptr<WTableCell> cell);
void updateRowNumbers(int begin, int end);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

firstRow and lastRow is probably more clear than begin and end. It's also best that you throw in some documentation that clarifies that the lastRow is inclusive.


void repaintRow(WTableRow *row);
void repaintColumn(WTableColumn *col);
Expand Down
6 changes: 4 additions & 2 deletions src/Wt/WTableRow.C
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ namespace Wt {

WTableRow::WTableRow()
: hidden_(false),
hiddenChanged_(false)
{
hiddenChanged_(false),
rowNumber_(-1)
{
implementStateless(&WTableRow::hide, &WTableRow::undoHide);
implementStateless(&WTableRow::show, &WTableRow::undoHide);
}
Expand Down Expand Up @@ -87,6 +88,7 @@ WTableCell *WTableRow::elementAt(int column)
int WTableRow::rowNum() const
{
if (table_)
if (rowNumber_ != -1) return rowNumber_;
for (unsigned i = 0; i < table_->rows_.size(); ++i)
if (table_->rows_[i].get() == this)
return i;
Comment on lines 92 to 94
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what scenario would it have to fallback to this code?

Also: this loop is not guarded by the above if (table_) anymore. The readbility-misleading-indentation clang-tidy check catches this.

It seems to me the entire function could be simplified to return rowNumber_;.

Expand Down
1 change: 1 addition & 0 deletions src/Wt/WTableRow.h
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ class WT_API WTableRow : public WObject
std::string id_;
WT_USTRING styleClass_;
bool hidden_, hiddenChanged_, wasHidden_;
size_t rowNumber_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be an int for consistency.


void updateDom(DomElement& element, bool all);
void setTable(WTable *table);
Expand Down
1 change: 1 addition & 0 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ IF(ENABLE_LIBWTTEST)
utils/Base64Test.C
utils/EraseWord.C
utils/ParseNumber.C
widgets/WTableTest.C
widgets/WSpinBoxTest.C
Comment on lines +35 to 36
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
widgets/WTableTest.C
widgets/WSpinBoxTest.C
widgets/WSpinBoxTest.C
widgets/WTableTest.C

T comes after S in the alphabet.

length/WLengthTest.C
color/WColorTest.C
Expand Down
269 changes: 269 additions & 0 deletions test/widgets/WTableTest.C
Original file line number Diff line number Diff line change
@@ -0,0 +1,269 @@
/*
* Copyright (C) 2010 Emweb bvba, Kessel-Lo, Belgium.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Copyright (C) 2010 Emweb bvba, Kessel-Lo, Belgium.
* Copyright (C) 2022 Emweb bv, Herent, Belgium.

*
* See the LICENSE file for terms of use.
*/
#include <boost/test/unit_test.hpp>

#include <Wt/WApplication.h>
#include <Wt/WTable.h>
#include <Wt/Test/WTestEnvironment.h>

using namespace Wt;

namespace {
Wt::Test::WTestEnvironment environment;
Wt::WApplication testApp(environment);
Comment on lines +15 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't create a WTestEnvironment and WApplication. Every test should create its own, so that tests are nicely isolated.


void CheckRowNumbers(WTable* table)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void CheckRowNumbers(WTable* table)
void checkRowNumbers(WTable* table)

Wt uses lowerCamelCase for functions.

{
for (auto i = 0; i < table->rowCount(); ++i) {
BOOST_REQUIRE_EQUAL(table->rowAt(i)->rowNum(), i);
}
}
void CheckColumnNumbers(WTable* table)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void CheckColumnNumbers(WTable* table)
void checkColumnNumbers(WTable* table)

{
for (auto i = 0; i < table->columnCount(); ++i) {
BOOST_REQUIRE_EQUAL(table->columnAt(i)->columnNum(), i);
}
}
}

BOOST_AUTO_TEST_SUITE( WTableTest )

BOOST_AUTO_TEST_CASE( elementAt1 )
{
auto table = cpp14::make_unique<WTable>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cpp14::make_unique was deprecated: use std::unique now instead.

BOOST_REQUIRE_NO_THROW(table->elementAt(5, 1));

auto rowCount = table->rowCount();

BOOST_REQUIRE_EQUAL(rowCount, 6);
CheckRowNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( elementAt2 )
{
auto table = cpp14::make_unique<WTable>();
BOOST_REQUIRE_NO_THROW(table->elementAt(1, 5));

auto columnCount = table->columnCount();

BOOST_REQUIRE_EQUAL(columnCount, 6);
CheckColumnNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( elementAt3 )
{
auto table = cpp14::make_unique<WTable>();
BOOST_REQUIRE_NO_THROW(table->elementAt(5, 4));

auto rowCount = table->rowCount();
auto columnCount = table->columnCount();

BOOST_REQUIRE_EQUAL(rowCount, 6);
BOOST_REQUIRE_EQUAL(columnCount, 5);
CheckRowNumbers(table.get());
CheckColumnNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( rowAt )
{
auto table = cpp14::make_unique<WTable>();
BOOST_REQUIRE_NO_THROW(table->rowAt(5));

auto rowCount = table->rowCount();

BOOST_REQUIRE_EQUAL(rowCount, 6);
CheckRowNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( columnAt )
{
auto table = cpp14::make_unique<WTable>();
BOOST_REQUIRE_NO_THROW(table->columnAt(5));

auto columnCount = table->columnCount();

BOOST_REQUIRE_EQUAL(columnCount, 6);
CheckColumnNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( insertRow1 )
{
auto table = cpp14::make_unique<WTable>();
table->rowAt(2);
BOOST_REQUIRE_NO_THROW(table->insertRow(1));

auto rowCount = table->rowCount();

BOOST_REQUIRE_EQUAL(rowCount, 4);
CheckRowNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( insertRow2 )
{
auto table = cpp14::make_unique<WTable>();
table->rowAt(2);
BOOST_REQUIRE_NO_THROW(table->insertRow(3));

auto rowCount = table->rowCount();

BOOST_REQUIRE_EQUAL(rowCount, 4);
CheckRowNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( insertRow3 )
{
auto table = cpp14::make_unique<WTable>();
BOOST_REQUIRE_NO_THROW(table->insertRow(0));
}

BOOST_AUTO_TEST_CASE( insertColumn1 )
{
auto table = cpp14::make_unique<WTable>();
table->columnAt(2);
BOOST_REQUIRE_NO_THROW(table->insertColumn(1));

auto columnCount = table->columnCount();

BOOST_REQUIRE_EQUAL(columnCount, 4);
CheckColumnNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( insertColumn2 )
{
auto table = cpp14::make_unique<WTable>();
table->columnAt(2);
BOOST_REQUIRE_NO_THROW(table->insertColumn(3));

auto columnCount = table->columnCount();

BOOST_REQUIRE_EQUAL(columnCount, 4);
CheckColumnNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( insertColumn3 )
{
auto table = cpp14::make_unique<WTable>();
BOOST_REQUIRE_NO_THROW(table->insertColumn(0));
}

BOOST_AUTO_TEST_CASE( removeRow1 )
{
auto table = cpp14::make_unique<WTable>();
table->rowAt(5);
BOOST_REQUIRE_NO_THROW(table->removeRow(2));

auto rowCount = table->rowCount();

BOOST_REQUIRE_EQUAL(rowCount, 5);
CheckRowNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( removeRow2 )
{
auto table = cpp14::make_unique<WTable>();
table->rowAt(0);
BOOST_REQUIRE_NO_THROW(table->removeRow(0));
}

BOOST_AUTO_TEST_CASE( removeColumn1 )
{
auto table = cpp14::make_unique<WTable>();
table->columnAt(5);
BOOST_REQUIRE_NO_THROW(table->removeColumn(2));

auto columnCount = table->columnCount();

BOOST_REQUIRE_EQUAL(columnCount, 5);
CheckColumnNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( removeColumn2 )
{
auto table = cpp14::make_unique<WTable>();
table->columnAt(0);
BOOST_REQUIRE_NO_THROW(table->removeColumn(0));
}

BOOST_AUTO_TEST_CASE( moveRow1 )
{
auto table = cpp14::make_unique<WTable>();
table->rowAt(5);

WTableCell* cell2 = table->elementAt(2, 0);
WTableCell* cell0 = table->elementAt(0, 0);

BOOST_REQUIRE_NO_THROW(table->moveRow(2, 0));

BOOST_REQUIRE_EQUAL(table->elementAt(0, 0), cell2);
BOOST_REQUIRE_EQUAL(table->elementAt(1, 0), cell0);
BOOST_REQUIRE_NE(table->elementAt(2, 0), cell2);

BOOST_REQUIRE_EQUAL(table->rowCount(), 6);
CheckRowNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( moveRow2, * boost::unit_test::disabled() )
{
auto table = cpp14::make_unique<WTable>();
table->rowAt(5);

WTableCell* cell2 = table->elementAt(2, 0);

BOOST_REQUIRE_NO_THROW(table->moveRow(2, 7));

BOOST_REQUIRE_NE(table->elementAt(2, 0), cell2);
BOOST_REQUIRE_EQUAL(table->elementAt(7, 0), cell2);

BOOST_REQUIRE_EQUAL(table->rowCount(), 8);
CheckRowNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( moveColumn1 )
{
auto table = cpp14::make_unique<WTable>();
table->columnAt(5);

WTableCell* cell2 = table->elementAt(0, 2);
WTableCell* cell0 = table->elementAt(0, 0);

BOOST_REQUIRE_NO_THROW(table->moveColumn(2, 0));

BOOST_REQUIRE_EQUAL(table->elementAt(0, 0), cell2);
BOOST_REQUIRE_EQUAL(table->elementAt(0, 1), cell0);
BOOST_REQUIRE_NE(table->elementAt(0, 2), cell2);

BOOST_REQUIRE_EQUAL(table->columnCount(), 6);
CheckColumnNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( moveColumn2, * boost::unit_test::disabled() )
{
auto table = cpp14::make_unique<WTable>();
table->columnAt(5);

WTableCell* cell2 = table->elementAt(0, 2);

BOOST_REQUIRE_NO_THROW(table->moveColumn(2, 7));

BOOST_REQUIRE_EQUAL(table->elementAt(0, 7), cell2);
BOOST_REQUIRE_NE(table->elementAt(0, 2), cell2);

BOOST_REQUIRE_EQUAL(table->columnCount(), 8);
CheckColumnNumbers(table.get());
}

BOOST_AUTO_TEST_CASE( clear )
{
auto table = cpp14::make_unique<WTable>();
table->elementAt(4, 4);

BOOST_REQUIRE_NO_THROW(table->clear());

BOOST_REQUIRE_EQUAL(table->rowCount(), 0);
BOOST_REQUIRE_EQUAL(table->columnCount(), 0);
}

BOOST_AUTO_TEST_SUITE_END()